cmd/fscrypt: improve errors

In checkEncryptable(), check whether the directory is already encrypted
before checking whether it's empty.

Also improve the error message for when a directory is nonempty.

Finally, translate keyring.ErrKeyAddedByOtherUsers and
keyring.ErrKeyFilesOpen into errors which include the directory.

6 files changed, 135 insertions, 59 deletions

diff --git a/cli-tests/t_encrypt.out b/cli-tests/t_encrypt.out
index e3bace0..26cb451 100644
--- a/cli-tests/t_encrypt.out
+++ b/cli-tests/t_encrypt.out
@@ -7,11 +7,22 @@ ext4 filesystem "MNT" has 0 protectors and 0 policies
                         encrypted
 
 # Try to encrypt a nonempty directory
-[ERROR] fscrypt encrypt: MNT/dir: not an empty directory
-
-Encryption can only be setup on empty directories; files cannot be encrypted
-in-place. Instead, encrypt an empty directory, copy the files into that
-encrypted directory, and securely delete the originals with "shred".
+[ERROR] fscrypt encrypt: Directory "MNT/dir" cannot be
+                         encrypted because it is non-empty.
+
+Files cannot be encrypted in-place. Instead, encrypt a new directory, copy the
+files into it, and securely delete the original directory. For example:
+
+     mkdir MNT/dir.new
+     fscrypt encrypt MNT/dir.new
+     cp -a -T MNT/dir MNT/dir.new
+     find MNT/dir -type f -print0 | xargs -0 shred -n1 --remove=unlink
+     rm -rf MNT/dir
+     mv MNT/dir.new MNT/dir
+
+Caution: due to the nature of modern storage devices and filesystems, the
+original data may still be recoverable from disk. It's much better to encrypt
+your files from the start.
 ext4 filesystem "MNT" has 0 protectors and 0 policies
 
 [ERROR] fscrypt status: file or directory "MNT/dir" is not
diff --git a/cli-tests/t_lock.out b/cli-tests/t_lock.out
index c0f9279..b8c8dcb 100644
--- a/cli-tests/t_lock.out
+++ b/cli-tests/t_lock.out
@@ -33,11 +33,16 @@ desc2  No      custom protector "prot"
 contents
 
 # Try to lock directory while files busy
-[ERROR] fscrypt lock: some files using the key are still open
+[ERROR] fscrypt lock: Directory was incompletely locked because some files are
+                      still open. These files remain accessible.
 
-Directory was incompletely locked because some files are still open. These files
-remain accessible. Try killing any processes using files in the directory, then
-re-running 'fscrypt lock'.
+Try killing any processes using files in the directory, for example using:
+
+     find "MNT/dir" -print0 | xargs -0 fuser -k
+
+Then re-run:
+
+     fscrypt lock "MNT/dir"
 
 # => status should be incompletely locked
 "MNT/dir" is encrypted with fscrypt.
@@ -72,11 +77,12 @@ mkdir: cannot create directory 'MNT/dir/subdir': Required key not available
 
 # Try to lock directory while other user has unlocked
 Enter custom passphrase for protector "prot": "MNT/dir" is now unlocked and ready for use.
-[ERROR] fscrypt lock: other users have added the key too
+[ERROR] fscrypt lock: Directory "MNT/dir" couldn't be fully
+                      locked because other user(s) have unlocked it.
+
+If you want to force the directory to be locked, use:
 
-Directory couldn't be fully locked because other user(s) have unlocked it. If
-you want to force the directory to be locked, use 'sudo fscrypt lock --all-users
-DIR'.
+     sudo fscrypt lock --all-users "MNT/dir"
 contents
 "MNT/dir" is now locked.
 cat: MNT/dir/file: No such file or directory
diff --git a/cli-tests/t_setup.out b/cli-tests/t_setup.out
index ef0d133..943a781 100644
--- a/cli-tests/t_setup.out
+++ b/cli-tests/t_setup.out
@@ -26,7 +26,7 @@ Skipping creating MNT_ROOT/.fscrypt because it already exists.
 # fscrypt setup --quiet when fscrypt.conf already exists
 [ERROR] fscrypt setup: operation would be destructive
 
-Use --force to automatically run destructive operations.
+If desired, use --force to automatically run destructive operations.
 
 # fscrypt setup --quiet --force when fscrypt.conf already exists
 
diff --git a/cli-tests/t_v1_policy.out b/cli-tests/t_v1_policy.out
index e693bf5..b47bcca 100644
--- a/cli-tests/t_v1_policy.out
+++ b/cli-tests/t_v1_policy.out
@@ -101,11 +101,16 @@ cat: MNT/dir/file: No such file or directory
 # Testing incompletely locking v1-encrypted directory
 Enter custom passphrase for protector "prot": "MNT/dir" is now unlocked and ready for use.
 Encrypted data removed from filesystem cache.
-[ERROR] fscrypt lock: some files using the key are still open
+[ERROR] fscrypt lock: Directory was incompletely locked because some files are
+                      still open. These files remain accessible.
 
-Directory was incompletely locked because some files are still open. These files
-remain accessible. Try killing any processes using files in the directory, then
-re-running 'fscrypt lock'.
+Try killing any processes using files in the directory, for example using:
+
+     find "MNT/dir" -print0 | xargs -0 fuser -k
+
+Then re-run:
+
+     fscrypt lock "MNT/dir"
 "MNT/dir" is encrypted with fscrypt.
 
 Policy:   desc1
diff --git a/cmd/fscrypt/commands.go b/cmd/fscrypt/commands.go
index ea393bb..8058cb3 100644
--- a/cmd/fscrypt/commands.go
+++ b/cmd/fscrypt/commands.go
@@ -297,7 +297,18 @@ func encryptPath(path string) (err error) {
 
 // checkEncryptable returns an error if the path cannot be encrypted.
 func checkEncryptable(ctx *actions.Context, path string) error {
-	log.Printf("ensuring %s is an empty and readable directory", path)
+
+	log.Printf("checking whether %q is already encrypted", path)
+	if _, err := metadata.GetPolicy(path); err == nil {
+		return &metadata.ErrAlreadyEncrypted{Path: path}
+	}
+
+	log.Printf("checking whether filesystem %s supports encryption", ctx.Mount.Path)
+	if err := ctx.Mount.CheckSupport(); err != nil {
+		return err
+	}
+
+	log.Printf("checking whether %q is an empty and readable directory", path)
 	f, err := os.Open(path)
 	if err != nil {
 		return err
@@ -307,26 +318,13 @@ func checkEncryptable(ctx *actions.Context, path string) error {
 	switch names, err := f.Readdirnames(-1); {
 	case err != nil:
 		// Could not read directory (might not be a directory)
-		log.Print(errors.Wrap(err, path))
-		return errors.Wrap(ErrNotEmptyDir, path)
-	case len(names) > 0:
-		log.Printf("directory %s is not empty", path)
-		return errors.Wrap(ErrNotEmptyDir, path)
-	}
-
-	log.Printf("ensuring %s supports encryption and filesystem is using fscrypt", path)
-	switch _, err := actions.GetPolicyFromPath(ctx, path); errors.Cause(err) {
-	case nil:
-		// We are encrypted
-		return &metadata.ErrAlreadyEncrypted{path}
-	default:
-		if _, ok := err.(*metadata.ErrNotEncrypted); ok {
-			// We are not encrypted. Finally, we check that the filesystem
-			// supports encryption
-			return ctx.Mount.CheckSupport()
-		}
+		err = errors.Wrap(err, path)
+		log.Print(err)
 		return err
+	case len(names) > 0:
+		return &ErrDirNotEmpty{path}
 	}
+	return err
 }
 
 // selectOrCreateProtector uses user input (or flags) to either create a new
@@ -410,7 +408,7 @@ func unlockAction(c *cli.Context) error {
 	if policy.IsProvisionedByTargetUser() {
 		log.Printf("policy %s is already provisioned by %v",
 			policy.Descriptor(), ctx.TargetUser.Username)
-		return newExitError(c, errors.Wrapf(ErrPolicyUnlocked, path))
+		return newExitError(c, errors.Wrapf(ErrDirAlreadyUnlocked, path))
 	}
 
 	if err := policy.Unlock(optionFn, existingKeyFn); err != nil {
@@ -499,7 +497,14 @@ func lockAction(c *cli.Context) error {
 	}
 
 	if err = policy.Deprovision(allUsersFlag.Value); err != nil {
-		if err != keyring.ErrKeyNotPresent {
+		switch err {
+		case keyring.ErrKeyNotPresent:
+			break
+		case keyring.ErrKeyAddedByOtherUsers:
+			return newExitError(c, &ErrDirUnlockedByOtherUsers{path})
+		case keyring.ErrKeyFilesOpen:
+			return newExitError(c, &ErrDirFilesOpen{path})
+		default:
 			return newExitError(c, err)
 		}
 		// Key is no longer present.  Normally that means the directory
@@ -510,7 +515,7 @@ func lockAction(c *cli.Context) error {
 		// locking the directory by dropping caches again.
 		if !policy.NeedsUserKeyring() || !isDirUnlockedHeuristic(path) {
 			log.Printf("policy %s is already fully deprovisioned", policy.Descriptor())
-			return newExitError(c, errors.Wrapf(ErrPolicyLocked, path))
+			return newExitError(c, errors.Wrapf(ErrDirAlreadyLocked, path))
 		}
 	}
 
@@ -519,7 +524,7 @@ func lockAction(c *cli.Context) error {
 			return newExitError(c, err)
 		}
 		if isDirUnlockedHeuristic(path) {
-			return newExitError(c, keyring.ErrKeyFilesOpen)
+			return newExitError(c, &ErrDirFilesOpen{path})
 		}
 	}
 
diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index 4ce4504..63ddaf4 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -55,9 +55,8 @@ var (
 	ErrKeyFileLength      = errors.Errorf("key file must be %d bytes", metadata.InternalKeyLen)
 	ErrAllLoadsFailed     = errors.New("could not load any protectors")
 	ErrMustBeRoot         = errors.New("this command must be run as root")
-	ErrPolicyUnlocked     = errors.New("this file or directory is already unlocked")
-	ErrPolicyLocked       = errors.New("this file or directory is already locked")
-	ErrNotEmptyDir        = errors.New("not an empty directory")
+	ErrDirAlreadyUnlocked = errors.New("this file or directory is already unlocked")
+	ErrDirAlreadyLocked   = errors.New("this file or directory is already locked")
 	ErrNotPassphrase      = errors.New("protector does not use a passphrase")
 	ErrUnknownUser        = errors.New("unknown user")
 	ErrDropCachesPerm     = errors.New("inode cache can only be dropped as root")
@@ -65,6 +64,38 @@ var (
 	ErrFsKeyringPerm      = errors.New("root is required to add/remove v1 encryption policy keys to/from filesystem")
 )
 
+// ErrDirFilesOpen indicates that a directory can't be fully locked because
+// files protected by the directory's policy are still open.
+type ErrDirFilesOpen struct {
+	DirPath string
+}
+
+func (err *ErrDirFilesOpen) Error() string {
+	return fmt.Sprintf(`Directory was incompletely locked because some files
+	are still open. These files remain accessible.`)
+}
+
+// ErrDirUnlockedByOtherUsers indicates that a directory can't be locked because
+// the directory's policy is still provisioned by other users.
+type ErrDirUnlockedByOtherUsers struct {
+	DirPath string
+}
+
+func (err *ErrDirUnlockedByOtherUsers) Error() string {
+	return fmt.Sprintf(`Directory %q couldn't be fully locked because other
+	user(s) have unlocked it.`, err.DirPath)
+}
+
+// ErrDirNotEmpty indicates that a directory can't be encrypted because it's not
+// empty.
+type ErrDirNotEmpty struct {
+	DirPath string
+}
+
+func (err *ErrDirNotEmpty) Error() string {
+	return fmt.Sprintf("Directory %q cannot be encrypted because it is non-empty.", err.DirPath)
+}
+
 var loadHelpText = fmt.Sprintf("You may need to mount a linked filesystem. Run with %s for more information.", shortDisplay(verboseFlag))
 
 // getFullName returns the full name of the application or command being used.
@@ -138,6 +169,37 @@ func suggestEnablingEncryption(mnt *filesystem.Mount) string {
 // an error. If no suggestion is necessary or available, return empty string.
 func getErrorSuggestions(err error) string {
 	switch e := err.(type) {
+	case *ErrDirFilesOpen:
+		return fmt.Sprintf(`Try killing any processes using files in the
+		directory, for example using:
+
+		> find %q -print0 | xargs -0 fuser -k
+
+		Then re-run:
+
+		> fscrypt lock %q`, e.DirPath, e.DirPath)
+	case *ErrDirNotEmpty:
+		dir := e.DirPath
+		newDir := dir + ".new"
+		return fmt.Sprintf(`Files cannot be encrypted in-place. Instead,
+		encrypt a new directory, copy the files into it, and securely
+		delete the original directory. For example:
+
+		> mkdir %s
+		> fscrypt encrypt %s
+		> cp -a -T %s %s
+		> find %s -type f -print0 | xargs -0 shred -n1 --remove=unlink
+		> rm -rf %s
+		> mv %s %s
+
+		Caution: due to the nature of modern storage devices and filesystems,
+		the original data may still be recoverable from disk. It's much better
+		to encrypt your files from the start.`, newDir, newDir, dir, newDir, dir, dir, newDir, dir)
+	case *ErrDirUnlockedByOtherUsers:
+		return fmt.Sprintf(`If you want to force the directory to be
+		locked, use:
+
+		> sudo fscrypt lock --all-users %q`, e.DirPath)
 	case *actions.ErrBadConfigFile:
 		return `Either fix this file manually, or run "sudo fscrypt setup" to recreate it.`
 	case *actions.ErrLoginProtectorName:
@@ -183,30 +245,17 @@ func getErrorSuggestions(err error) string {
 			-l". The limit can be modified by either changing the
 			"memlock" item in /etc/security/limits.conf or by
 			changing the "LimitMEMLOCK" value in systemd.`
-	case keyring.ErrKeyFilesOpen:
-		return `Directory was incompletely locked because some files are
-			still open. These files remain accessible. Try killing
-			any processes using files in the directory, then
-			re-running 'fscrypt lock'.`
-	case keyring.ErrKeyAddedByOtherUsers:
-		return `Directory couldn't be fully locked because other user(s)
-			have unlocked it. If you want to force the directory to
-			be locked, use 'sudo fscrypt lock --all-users DIR'.`
 	case keyring.ErrV2PoliciesUnsupported:
 		return fmt.Sprintf(`v2 encryption policies are only supported by kernel
 		version 5.4 and later. Either use a newer kernel, or change
 		policy_version to 1 in %s.`, actions.ConfigFileLocation)
 	case ErrNoDestructiveOps:
-		return fmt.Sprintf("Use %s to automatically run destructive operations.", shortDisplay(forceFlag))
+		return fmt.Sprintf("If desired, use %s to automatically run destructive operations.",
+			shortDisplay(forceFlag))
 	case ErrSpecifyProtector:
 		return fmt.Sprintf("Use %s to specify a protector.", shortDisplay(protectorFlag))
 	case ErrSpecifyKeyFile:
 		return fmt.Sprintf("Use %s to specify a key file.", shortDisplay(keyFileFlag))
-	case ErrNotEmptyDir:
-		return `Encryption can only be setup on empty directories; files
-			cannot be encrypted in-place. Instead, encrypt an empty
-			directory, copy the files into that encrypted directory,
-			and securely delete the originals with "shred".`
 	case ErrDropCachesPerm:
 		return fmt.Sprintf(`Either this command should be run as root to
 			properly clear the inode cache, or it should be run with