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

Annex/Ingest.hs

@@ -1,6 +1,6 @@
 {- 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.
  -}
@@ -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' :: LockDownConfig -> FilePath -> Annex (Either SomeException LockedDown)
-lockDown' cfg file = tryIO $ ifM crippledFileSystem
+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) $
+		setperms
-			freezeContent file'
 		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. -}

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
+{- Checks if the write permissions are as freezeContent should set them.
-isContentWritePermOk file = ifM crippledFileSystem
+ -
-	( return True
+ - 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 GroupShared = want sharedret 
-	go AllShared = want writeModes
+		(includemodes [ownerWriteMode, groupWriteMode])
-	go _ = return True
+	go AllShared = want sharedret (includemodes writeModes)
-	want wantmode =
+	go _ = want Just (excludemodes writeModes)
+
+	want mk f =
 		liftIO (catchMaybeIO $ fileMode <$> R.getFileStatus file) >>= return . \case
-			Nothing -> True
+			Just havemode -> mk (f havemode)
-			Just havemode -> havemode == combineModes (havemode:wantmode)
+			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. -}

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 <id@joeyh.name>  Tue, 03 Aug 2021 12:22:45 -0400
 

Command/Fsck.hs

@@ -260,8 +260,9 @@ verifyLocationLog key keystatus ai = do
 			KeyUnlockedThin -> thawContent obj
 			KeyLockedThin -> thawContent obj
 			_ -> freezeContent obj
-		unlessM (isContentWritePermOk obj) $
+		checkContentWritePerm obj >>= \case
-			warning $ "** Unable to set correct write mode for " ++ fromRawFilePath obj ++ " ; perhaps you don't own that file"
+			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
 

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
+I also worried about ACLS, but it seems like ACLs do not have this
-use setfacl to make this happen:
+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
+	nobody@darkstar:/home/joey$ echo test >> foo
-	chmod: foo: new permissions are r--rw-r--, not r--r--r--
+	joey@darkstar:~>chmod a-w foo
-	- exit 1
-	joey@darkstar:~>perl -e 'chmod(400)' 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
+I've verified that git-annex add also clears the write ACL.
-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..
 """]]

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.
+"""]]