From: Joey Hess Date: Fri, 27 Aug 2021 18:33:01 +0000 (-0400) Subject: add: Detect when xattrs or perhaps ACLs prevent locking down a file's content X-Git-Tag: archive/raspbian/10.20250416-2+rpi1~1^2~87^2~43 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=a99a84f3420a1d93e4ecd4b8cf30638b48e2758e;p=git-annex.git add: Detect when xattrs or perhaps ACLs prevent locking down a file's content And fail with an informative message. I don't think ACLs can prevent removing the write bit, but I'm not sure, so kept it mentioning them as a possibility. Should git-annex lock also check if the write bits are able to be removed? Maybe, but the case I know about with xattrs involves cp -a copying NFS xattrs, and it's the copy of the file that is the problem. So when locking a file, I guess it will not be the copy. Sponsored-by: Dartmouth College's Datalad project --- diff --git a/Annex/Ingest.hs b/Annex/Ingest.hs index 3466650dd6..bed8d38e5b 100644 --- a/Annex/Ingest.hs +++ b/Annex/Ingest.hs @@ -1,6 +1,6 @@ {- git-annex content ingestion - - - Copyright 2010-2020 Joey Hess + - Copyright 2010-2021 Joey Hess - - Licensed under the GNU AGPL version 3 or higher. -} @@ -51,8 +51,6 @@ import Annex.AdjustedBranch import Annex.FileMatcher import qualified Utility.RawFilePath as R -import Control.Exception (IOException) - data LockedDown = LockedDown { lockDownConfig :: LockDownConfig , keySource :: KeySource @@ -78,7 +76,8 @@ data LockDownConfig = LockDownConfig - against some changes, like deletion or overwrite of the file, and - allows lsof checks to be done more efficiently when adding a lot of files. - - - Lockdown can fail if a file gets deleted, and Nothing will be returned. + - Lockdown can fail if a file gets deleted, or if it's unable to remove + - write permissions, and Nothing will be returned. -} lockDown :: LockDownConfig -> FilePath -> Annex (Maybe LockedDown) lockDown cfg file = either @@ -86,8 +85,8 @@ lockDown cfg file = either (return . Just) =<< lockDown' cfg file -lockDown' :: LockDownConfig -> FilePath -> Annex (Either IOException LockedDown) -lockDown' cfg file = tryIO $ ifM crippledFileSystem +lockDown' :: LockDownConfig -> FilePath -> Annex (Either SomeException LockedDown) +lockDown' cfg file = tryNonAsync $ ifM crippledFileSystem ( nohardlink , case hardlinkFileTmpDir cfg of Nothing -> nohardlink @@ -96,7 +95,9 @@ lockDown' cfg file = tryIO $ ifM crippledFileSystem where file' = toRawFilePath file - nohardlink = withTSDelta $ liftIO . nohardlink' + nohardlink = do + setperms + withTSDelta $ liftIO . nohardlink' nohardlink' delta = do cache <- genInodeCache file' delta @@ -107,8 +108,7 @@ lockDown' cfg file = tryIO $ ifM crippledFileSystem } withhardlink tmpdir = do - when (lockingFile cfg) $ - freezeContent file' + setperms withTSDelta $ \delta -> liftIO $ do (tmpfile, h) <- openTempFile (fromRawFilePath tmpdir) $ relatedTemplate $ "ingest-" ++ takeFileName file @@ -125,6 +125,16 @@ lockDown' cfg file = tryIO $ ifM crippledFileSystem , contentLocation = toRawFilePath tmpfile , inodeCache = cache } + + setperms = when (lockingFile cfg) $ do + freezeContent file' + checkContentWritePerm file' >>= \case + Just False -> giveup $ unwords + [ "Unable to remove all write permissions from" + , file + , "-- perhaps it has an xattr or ACL set." + ] + _ -> return () {- Ingests a locked down file into the annex. Updates the work tree and - index. -} diff --git a/Annex/Perms.hs b/Annex/Perms.hs index 733922c512..214fa31141 100644 --- a/Annex/Perms.hs +++ b/Annex/Perms.hs @@ -16,7 +16,7 @@ module Annex.Perms ( noUmask, freezeContent, freezeContent', - isContentWritePermOk, + checkContentWritePerm, thawContent, thawContent', createContentDir, @@ -131,6 +131,12 @@ createWorkTreeDirectory dir = do - necessary to let other users in the group lock the file. But, in a - shared repository, the current user may not be able to change a file - owned by another user, so failure to set this mode is ignored. + - + - Note that, on Linux, xattrs can sometimes prevent removing + - certain permissions from a file with chmod. (Maybe some ACLs too?) + - In such a case, this will return with the file still having some mode + - it should not normally have. checkContentWritePerm can detect when + - that happens with write permissions. -} freezeContent :: RawFilePath -> Annex () freezeContent file = unlessM crippledFileSystem $ @@ -149,19 +155,34 @@ freezeContent' sr file = do removeModes writeModes . addModes [ownerReadMode] -isContentWritePermOk :: RawFilePath -> Annex Bool -isContentWritePermOk file = ifM crippledFileSystem - ( return True +{- Checks if the write permissions are as freezeContent should set them. + - + - When the repository is shared, the user may not be able to change + - permissions of a file owned by another user. So if the permissions seem + - wrong, but the repository is shared, returns Nothing. If the permissions + - are wrong otherwise, returns Just False. + -} +checkContentWritePerm :: RawFilePath -> Annex (Maybe Bool) +checkContentWritePerm file = ifM crippledFileSystem + ( return (Just True) , withShared go ) where - go GroupShared = want [ownerWriteMode, groupWriteMode] - go AllShared = want writeModes - go _ = return True - want wantmode = + go GroupShared = want sharedret + (includemodes [ownerWriteMode, groupWriteMode]) + go AllShared = want sharedret (includemodes writeModes) + go _ = want Just (excludemodes writeModes) + + want mk f = liftIO (catchMaybeIO $ fileMode <$> R.getFileStatus file) >>= return . \case - Nothing -> True - Just havemode -> havemode == combineModes (havemode:wantmode) + Just havemode -> mk (f havemode) + Nothing -> mk True + + includemodes l havemode = havemode == combineModes (havemode:l) + excludemodes l havemode = all (\m -> intersectFileModes m havemode == nullFileMode) l + + sharedret True = Just True + sharedret False = Nothing {- Allows writing to an annexed file that freezeContent was called on - before. -} diff --git a/CHANGELOG b/CHANGELOG index 2c6cdce457..ab4f9202ca 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -22,6 +22,8 @@ git-annex (8.20210804) UNRELEASED; urgency=medium * Run cp -a with --no-preserve=xattr, to avoid problems with copied xattrs, including them breaking permissions setting on some NFS servers. + * add: Detect when xattrs or perhaps ACLs prevent locking down + a file's content, and fail with an informative message. -- Joey Hess Tue, 03 Aug 2021 12:22:45 -0400 diff --git a/Command/Fsck.hs b/Command/Fsck.hs index 2408c439d6..b34b3a12f4 100644 --- a/Command/Fsck.hs +++ b/Command/Fsck.hs @@ -260,8 +260,9 @@ verifyLocationLog key keystatus ai = do KeyUnlockedThin -> thawContent obj KeyLockedThin -> thawContent obj _ -> freezeContent obj - unlessM (isContentWritePermOk obj) $ - warning $ "** Unable to set correct write mode for " ++ fromRawFilePath obj ++ " ; perhaps you don't own that file" + checkContentWritePerm obj >>= \case + Nothing -> warning $ "** Unable to set correct write mode for " ++ fromRawFilePath obj ++ " ; perhaps you don't own that file, or perhaps it has an xattr or ACL set" + _ -> return () whenM (liftIO $ R.doesPathExist $ parentDir obj) $ freezeContentDir obj diff --git a/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount/comment_11_541e257ea04d6a7647a337998d79232c._comment b/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount/comment_11_541e257ea04d6a7647a337998d79232c._comment index 9cf2c56637..92e2d5d715 100644 --- a/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount/comment_11_541e257ea04d6a7647a337998d79232c._comment +++ b/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount/comment_11_541e257ea04d6a7647a337998d79232c._comment @@ -16,29 +16,29 @@ a file themselves with cp -a on this NFS and then git-annex adds the copy. Probably git-annex would then be unable to remove the write bit from the annex object file. -For that matter, the same could happen with ACLs. Eg, I was able to -use setfacl to make this happen: +I also worried about ACLS, but it seems like ACLs do not have this +problem, because chmod a-w causes the write ACL that was set to be +effectively unset: joey@darkstar:~>chmod -w foo joey@darkstar:~>setfacl -m g:nogroup:rw foo joey@darkstar:~>ls -l foo -r--rw-r--+ 1 joey joey 0 Aug 27 12:53 foo - joey@darkstar:~>chmod -w foo - chmod: foo: new permissions are r--rw-r--, not r--r--r-- - - exit 1 - joey@darkstar:~>perl -e 'chmod(400)' foo + nobody@darkstar:/home/joey$ echo test >> foo + joey@darkstar:~>chmod a-w foo joey@darkstar:~>ls -l foo -r--rw-r--+ 1 joey joey 0 Aug 27 12:53 foo + nobody@darkstar:/home/joey$ echo test >> foo + bash: foo: Permission denied + joey@darkstar:~>getfacl foo + # file: foo + # owner: joey + # group: joey + user::r-- + group::r-- + group:nogroup:rw- #effective:r-- + mask::r-- + other::r-- -So git-annex would be unable to clear the write bit, and would not be able -to effectively lock down the file for all users, eg user nobody can write -to the file in the above example. There's probably a way to let user joey -also write to it, but my attempt to do that with an ACL failed. - -Perhaps git-annex should clear ACLs when ingesting (and locking) files. -But perhaps users use ACLs for other purposes that would not prevent -lockdown, and so they should not be cleared. And as far as internal-use NFS -xattrs, it doesn't seem wise for git-annex to try to unset them from files -its ingesting. So I guess I'm going to punt on this broader question, -if users want to use the ACL rope, it's over there.. +I've verified that git-annex add also clears the write ACL. """]] diff --git a/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount/comment_12_80b15bc4e0c8c9ed4680da9408c1d559._comment b/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount/comment_12_80b15bc4e0c8c9ed4680da9408c1d559._comment new file mode 100644 index 0000000000..0136b0a862 --- /dev/null +++ b/doc/bugs/__34__357_out_of_984_tests_failed__34___on_NFS_lustre_mount/comment_12_80b15bc4e0c8c9ed4680da9408c1d559._comment @@ -0,0 +1,12 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 12""" + date="2021-08-27T17:13:54Z" + content=""" +I've also made git-annex add check, after removing write bits, +if the file still has write bits set. It will refuse to add a file +when it can't lock it down. + +That should avoid the NFS xattr problem in a situation where +cp -a was used to make a copy that then gets added to the annex. +"""]]