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
{- git-annex content ingestion
-
- - Copyright 2010-2020 Joey Hess <id@joeyh.name>
+ - Copyright 2010-2021 Joey Hess <id@joeyh.name>
-
- Licensed under the GNU AGPL version 3 or higher.
-}
import Annex.FileMatcher
import qualified Utility.RawFilePath as R
-import Control.Exception (IOException)
-
data LockedDown = LockedDown
{ lockDownConfig :: LockDownConfig
, keySource :: KeySource
- 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
(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
where
file' = toRawFilePath file
- nohardlink = withTSDelta $ liftIO . nohardlink'
+ nohardlink = do
+ setperms
+ withTSDelta $ liftIO . nohardlink'
nohardlink' delta = do
cache <- genInodeCache file' delta
}
withhardlink tmpdir = do
- when (lockingFile cfg) $
- freezeContent file'
+ setperms
withTSDelta $ \delta -> liftIO $ do
(tmpfile, h) <- openTempFile (fromRawFilePath tmpdir) $
relatedTemplate $ "ingest-" ++ takeFileName file
, 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. -}
noUmask,
freezeContent,
freezeContent',
- isContentWritePermOk,
+ checkContentWritePerm,
thawContent,
thawContent',
createContentDir,
- 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 $
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. -}
* 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 <id@joeyh.name> Tue, 03 Aug 2021 12:22:45 -0400
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
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.
"""]]
--- /dev/null
+[[!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.
+"""]]
projects / git-annex.git / commitdiff