From 1a47ab1e565f7052e34b0677a1df6789e6ecf3a9 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Sat, 9 May 2020 14:52:06 -0700
Subject: cmd/fscrypt: make wrapText() support code blocks

Allow the input text to contain "code blocks" denoted by lines beginning
with ">", e.g.:

    Foo bar baz:

    > echo foo
    > echo bar

Instead of squashing these lines together, preserve the line breaks
between them and add indentation, e.g.:

    Foo bar baz:

        echo foo
        echo bar
---
 cmd/fscrypt/format.go | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/cmd/fscrypt/format.go b/cmd/fscrypt/format.go
index cc268aa..576d025 100644
--- a/cmd/fscrypt/format.go
+++ b/cmd/fscrypt/format.go
@@ -121,7 +121,8 @@ func longDisplay(f prettyFlag, defaultString ...string) string {
 // Takes an input string text, and wraps the text so that each line begins with
 // padding spaces (except for the first line), ends with a newline (except the
 // last line), and each line has length less than lineLength. If the text
-// contains a word which is too long, that word gets its own line.
+// contains a word which is too long, that word gets its own line. Paragraphs
+// and "code blocks" are preserved.
 func wrapText(text string, padding int) string {
 	// We use a buffer to format the wrapped text so we get O(n) runtime
 	var buffer bytes.Buffer
@@ -141,10 +142,18 @@ func wrapText(text string, padding int) string {
 			continue
 		}
 
+		codeBlock := (words[0] == ">")
+		if codeBlock {
+			words[0] = "    "
+			if filled != 0 {
+				buffer.WriteString("\n")
+				filled = 0
+			}
+		}
 		for _, word := range words {
 			wordLen := utf8.RuneCountInString(word)
 			// Write a newline if needed.
-			if filled != 0 && filled+1+wordLen > lineLength {
+			if filled != 0 && filled+1+wordLen > lineLength && !codeBlock {
 				buffer.WriteString("\n")
 				filled = 0
 			}
-- 
cgit v1.2.3


From e9919b0bfd00c7d228531ebafa410cbfdafcb2e3 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Sat, 9 May 2020 14:52:06 -0700
Subject: actions/config: improve config file related errors

ErrBadConfig:
	Fix backwards wrapping, include the bad config, and make it
	clear that this is an internal error.

ErrBadConfigFile:
	Fix backwards wrapping, include the config file location, and
	adjust the suggestion slightly.

ErrConfigFileExists:
	Include the config file location.

ErrNoConfigFile:
	Include the config file location, and adjust the suggestion
	slightly.
---
 actions/config.go     | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
 actions/context.go    | 12 +++---------
 cli-tests/t_setup.out | 10 +++++-----
 cmd/fscrypt/errors.go | 10 ++++++----
 4 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/actions/config.go b/actions/config.go
index 2463b95..b848d92 100644
--- a/actions/config.go
+++ b/actions/config.go
@@ -22,12 +22,12 @@ package actions
 
 import (
 	"bytes"
+	"fmt"
 	"log"
 	"os"
 	"runtime"
 	"time"
 
-	"github.com/pkg/errors"
 	"golang.org/x/sys/unix"
 
 	"github.com/google/fscrypt/crypto"
@@ -40,6 +40,46 @@ import (
 // overridden by the user of this package.
 var ConfigFileLocation = "/etc/fscrypt.conf"
 
+// ErrBadConfig is an internal error that indicates that the config struct is invalid.
+type ErrBadConfig struct {
+	Config          *metadata.Config
+	UnderlyingError error
+}
+
+func (err *ErrBadConfig) Error() string {
+	return fmt.Sprintf(`internal error: config is invalid: %s
+
+	The invalid config is %s`, err.UnderlyingError, err.Config)
+}
+
+// ErrBadConfigFile indicates that the config file is invalid.
+type ErrBadConfigFile struct {
+	Path            string
+	UnderlyingError error
+}
+
+func (err *ErrBadConfigFile) Error() string {
+	return fmt.Sprintf("%q is invalid: %s", err.Path, err.UnderlyingError)
+}
+
+// ErrConfigFileExists indicates that the config file already exists.
+type ErrConfigFileExists struct {
+	Path string
+}
+
+func (err *ErrConfigFileExists) Error() string {
+	return fmt.Sprintf("%q already exists", err.Path)
+}
+
+// ErrNoConfigFile indicates that the config file doesn't exist.
+type ErrNoConfigFile struct {
+	Path string
+}
+
+func (err *ErrNoConfigFile) Error() string {
+	return fmt.Sprintf("%q doesn't exist", err.Path)
+}
+
 const (
 	// Permissions of the config file (global readable)
 	configPermissions = 0644
@@ -67,7 +107,7 @@ func CreateConfigFile(target time.Duration, policyVersion int64) error {
 		createFlags, configPermissions)
 	switch {
 	case os.IsExist(err):
-		return ErrConfigFileExists
+		return &ErrConfigFileExists{ConfigFileLocation}
 	case err != nil:
 		return err
 	}
@@ -98,7 +138,7 @@ func getConfig() (*metadata.Config, error) {
 	configFile, err := os.Open(ConfigFileLocation)
 	switch {
 	case os.IsNotExist(err):
-		return nil, ErrNoConfigFile
+		return nil, &ErrNoConfigFile{ConfigFileLocation}
 	case err != nil:
 		return nil, err
 	}
@@ -107,7 +147,7 @@ func getConfig() (*metadata.Config, error) {
 	log.Printf("Reading config from %q\n", ConfigFileLocation)
 	config, err := metadata.ReadConfig(configFile)
 	if err != nil {
-		return nil, errors.Wrap(ErrBadConfigFile, err.Error())
+		return nil, &ErrBadConfigFile{ConfigFileLocation, err}
 	}
 
 	// Use system defaults if not specified
@@ -133,7 +173,7 @@ func getConfig() (*metadata.Config, error) {
 	}
 
 	if err := config.CheckValidity(); err != nil {
-		return nil, errors.Wrap(ErrBadConfigFile, err.Error())
+		return nil, &ErrBadConfigFile{ConfigFileLocation, err}
 	}
 
 	return config, nil
diff --git a/actions/context.go b/actions/context.go
index 0db0671..26295ec 100644
--- a/actions/context.go
+++ b/actions/context.go
@@ -40,14 +40,8 @@ import (
 	"github.com/google/fscrypt/util"
 )
 
-// Errors relating to Config files or Config structures.
-var (
-	ErrNoConfigFile     = errors.New("global config file does not exist")
-	ErrBadConfigFile    = errors.New("global config file has invalid data")
-	ErrConfigFileExists = errors.New("global config file already exists")
-	ErrBadConfig        = errors.New("invalid Config structure provided")
-	ErrLocked           = errors.New("key needs to be unlocked first")
-)
+// ErrLocked indicates that the key hasn't been unwrapped yet.
+var ErrLocked = errors.New("key needs to be unlocked first")
 
 // Context contains the necessary global state to perform most of fscrypt's
 // actions.
@@ -126,7 +120,7 @@ func newContextFromUser(targetUser *user.User) (*Context, error) {
 // which is being used with fscrypt.
 func (ctx *Context) checkContext() error {
 	if err := ctx.Config.CheckValidity(); err != nil {
-		return errors.Wrap(ErrBadConfig, err.Error())
+		return &ErrBadConfig{ctx.Config, err}
 	}
 	return ctx.Mount.CheckSetup()
 }
diff --git a/cli-tests/t_setup.out b/cli-tests/t_setup.out
index e1606ba..7d597bd 100644
--- a/cli-tests/t_setup.out
+++ b/cli-tests/t_setup.out
@@ -38,12 +38,12 @@ Metadata directories created at "MNT/.fscrypt".
                        with fscrypt
 
 # no config file
-[ERROR] fscrypt setup: global config file does not exist
+[ERROR] fscrypt setup: "FSCRYPT_CONF" doesn't exist
 
-Run "sudo fscrypt setup" to create the file.
+Run "sudo fscrypt setup" to create this file.
 
 # bad config file
-[ERROR] fscrypt setup: invalid character 'b' looking for beginning of value:
-                       global config file has invalid data
+[ERROR] fscrypt setup: "FSCRYPT_CONF" is invalid: invalid
+                       character 'b' looking for beginning of value
 
-Run "sudo fscrypt setup" to recreate the file.
+Either fix this file manually, or run "sudo fscrypt setup" to recreate it.
diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index 8bda921..e7c025f 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -79,6 +79,12 @@ func getFullName(c *cli.Context) string {
 // getErrorSuggestions returns a string containing suggestions about how to fix
 // an error. If no suggestion is necessary or available, return empty string.
 func getErrorSuggestions(err error) string {
+	switch err.(type) {
+	case *actions.ErrBadConfigFile:
+		return `Either fix this file manually, or run "sudo fscrypt setup" to recreate it.`
+	case *actions.ErrNoConfigFile:
+		return `Run "sudo fscrypt setup" to create this file.`
+	}
 	switch errors.Cause(err) {
 	case filesystem.ErrNotSetup:
 		return fmt.Sprintf(`Run "fscrypt setup %s" to use fscrypt on this filesystem.`, mountpointArg)
@@ -117,10 +123,6 @@ func getErrorSuggestions(err error) string {
 		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 actions.ErrBadConfigFile:
-		return `Run "sudo fscrypt setup" to recreate the file.`
-	case actions.ErrNoConfigFile:
-		return `Run "sudo fscrypt setup" to create the file.`
 	case actions.ErrMissingPolicyMetadata:
 		return `This file or directory has either been encrypted with
 			another tool (such as e4crypt) or the corresponding
-- 
cgit v1.2.3


From 37457cce5b0436493dba7cdac6e1af5f51d25f47 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Sat, 9 May 2020 14:52:07 -0700
Subject: actions/protector: improve errors

ErrProtectorName:
	Rename to ErrLoginProtectorName for clarity, and include the
	name and user.

ErrMissingProtectorName:
	Include the correct protector source.

ErrDuplicateName:
	Rename to ErrProtectorNameExists for clarity, and remove a level
	of wrapping by including the name directly.

ErrDuplicateUID:
	Rename to ErrLoginProtectorExists for clarity, and remove a
	level of wrapping by including the user directly.
---
 actions/protector.go           | 56 ++++++++++++++++++++++++++++++++----------
 actions/recovery.go            |  4 +--
 cli-tests/t_encrypt_custom.out |  2 +-
 cli-tests/t_encrypt_login.out  |  6 ++++-
 cmd/fscrypt/errors.go          |  6 +++--
 5 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/actions/protector.go b/actions/protector.go
index dab9c27..3278e63 100644
--- a/actions/protector.go
+++ b/actions/protector.go
@@ -22,8 +22,7 @@ package actions
 import (
 	"fmt"
 	"log"
-
-	"github.com/pkg/errors"
+	"os/user"
 
 	"github.com/google/fscrypt/crypto"
 	"github.com/google/fscrypt/metadata"
@@ -34,13 +33,44 @@ import (
 // This can be overridden by the user of this package.
 var LoginProtectorMountpoint = "/"
 
-// Errors relating to Protectors
-var (
-	ErrProtectorName        = errors.New("login protectors do not need a name")
-	ErrMissingProtectorName = errors.New("custom protectors must have a name")
-	ErrDuplicateName        = errors.New("protector with this name already exists")
-	ErrDuplicateUID         = errors.New("login protector for this user already exists")
-)
+// ErrLoginProtectorExists indicates that a user already has a login protector.
+type ErrLoginProtectorExists struct {
+	User *user.User
+}
+
+func (err *ErrLoginProtectorExists) Error() string {
+	return fmt.Sprintf("user %q already has a login protector", err.User.Username)
+}
+
+// ErrLoginProtectorName indicates that a name was given for a login protector.
+type ErrLoginProtectorName struct {
+	Name string
+	User *user.User
+}
+
+func (err *ErrLoginProtectorName) Error() string {
+	return fmt.Sprintf(`cannot assign name %q to new login protector for
+	user %q because login protectors are identified by user, not by name.`,
+		err.Name, err.User.Username)
+}
+
+// ErrMissingProtectorName indicates that a protector name is needed.
+type ErrMissingProtectorName struct {
+	Source metadata.SourceType
+}
+
+func (err *ErrMissingProtectorName) Error() string {
+	return fmt.Sprintf("%s protectors must be named", err.Source)
+}
+
+// ErrProtectorNameExists indicates that a protector name already exists.
+type ErrProtectorNameExists struct {
+	Name string
+}
+
+func (err *ErrProtectorNameExists) Error() string {
+	return fmt.Sprintf("there is already a protector named %q", err.Name)
+}
 
 // checkForProtectorWithName returns an error if there is already a protector
 // on the filesystem with a specific name (or if we cannot read the necessary
@@ -52,7 +82,7 @@ func checkForProtectorWithName(ctx *Context, name string) error {
 	}
 	for _, option := range options {
 		if option.Name() == name {
-			return errors.Wrapf(ErrDuplicateName, "name %q", name)
+			return &ErrProtectorNameExists{name}
 		}
 	}
 	return nil
@@ -68,7 +98,7 @@ func checkIfUserHasLoginProtector(ctx *Context, uid int64) error {
 	}
 	for _, option := range options {
 		if option.Source() == metadata.SourceType_pam_passphrase && option.UID() == uid {
-			return errors.Wrapf(ErrDuplicateUID, "user %q", ctx.TargetUser.Username)
+			return &ErrLoginProtectorExists{ctx.TargetUser}
 		}
 	}
 	return nil
@@ -97,12 +127,12 @@ func CreateProtector(ctx *Context, name string, keyFn KeyFunc) (*Protector, erro
 	if ctx.Config.Source == metadata.SourceType_pam_passphrase {
 		// login protectors don't need a name (we use the username instead)
 		if name != "" {
-			return nil, ErrProtectorName
+			return nil, &ErrLoginProtectorName{name, ctx.TargetUser}
 		}
 	} else {
 		// non-login protectors need a name (so we can distinguish between them)
 		if name == "" {
-			return nil, ErrMissingProtectorName
+			return nil, &ErrMissingProtectorName{ctx.Config.Source}
 		}
 		// we don't want to duplicate naming
 		if err := checkForProtectorWithName(ctx, name); err != nil {
diff --git a/actions/recovery.go b/actions/recovery.go
index 1c55ec5..458349b 100644
--- a/actions/recovery.go
+++ b/actions/recovery.go
@@ -23,8 +23,6 @@ import (
 	"os"
 	"strconv"
 
-	"github.com/pkg/errors"
-
 	"github.com/google/fscrypt/crypto"
 	"github.com/google/fscrypt/metadata"
 )
@@ -72,7 +70,7 @@ func AddRecoveryPassphrase(policy *Policy, dirname string) (*crypto.Key, *Protec
 		if err == nil {
 			break
 		}
-		if errors.Cause(err) != ErrDuplicateName {
+		if _, ok := err.(*ErrProtectorNameExists); !ok {
 			return nil, nil, err
 		}
 		seq++
diff --git a/cli-tests/t_encrypt_custom.out b/cli-tests/t_encrypt_custom.out
index 572529a..e7b8656 100644
--- a/cli-tests/t_encrypt_custom.out
+++ b/cli-tests/t_encrypt_custom.out
@@ -46,7 +46,7 @@ PROTECTOR         LINKED  DESCRIPTION
 desc6  No      custom protector "prot"
 
 # Try to use a custom protector without a name
-[ERROR] fscrypt encrypt: custom protectors must have a name
+[ERROR] fscrypt encrypt: custom_passphrase protectors must be named
 
 Use --name=PROTECTOR_NAME to specify a protector name.
 ext4 filesystem "MNT" has 0 protectors and 0 policies
diff --git a/cli-tests/t_encrypt_login.out b/cli-tests/t_encrypt_login.out
index c6eb463..7ee66a2 100644
--- a/cli-tests/t_encrypt_login.out
+++ b/cli-tests/t_encrypt_login.out
@@ -130,7 +130,11 @@ POLICY                            UNLOCKED  PROTECTORS
 desc34  Yes       desc35
 
 # Try to give a login protector a name
-[ERROR] fscrypt encrypt: login protectors do not need a name
+[ERROR] fscrypt encrypt: cannot assign name "prot" to new login protector for
+                         user "fscrypt-test-user" because login protectors are
+                         identified by user, not by name.
+
+To fix this, don't specify the --name=PROTECTOR_NAME option.
 ext4 filesystem "MNT" has 0 protectors and 0 policies
 
 ext4 filesystem "MNT_ROOT" has 0 protectors and 0 policies
diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index e7c025f..829a9d2 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -82,6 +82,10 @@ func getErrorSuggestions(err error) string {
 	switch err.(type) {
 	case *actions.ErrBadConfigFile:
 		return `Either fix this file manually, or run "sudo fscrypt setup" to recreate it.`
+	case *actions.ErrLoginProtectorName:
+		return fmt.Sprintf("To fix this, don't specify the %s option.", shortDisplay(nameFlag))
+	case *actions.ErrMissingProtectorName:
+		return fmt.Sprintf("Use %s to specify a protector name.", shortDisplay(nameFlag))
 	case *actions.ErrNoConfigFile:
 		return `Run "sudo fscrypt setup" to create this file.`
 	}
@@ -131,8 +135,6 @@ func getErrorSuggestions(err error) string {
 		return `The metadata for this encrypted directory is in an
 			inconsistent state. This most likely means the filesystem
 			metadata is corrupted.`
-	case actions.ErrMissingProtectorName:
-		return fmt.Sprintf("Use %s to specify a protector name.", shortDisplay(nameFlag))
 	case actions.ErrAccessDeniedPossiblyV2:
 		return fmt.Sprintf(`This may be caused by the directory using a v2
 		encryption policy and the current kernel not supporting it. If
-- 
cgit v1.2.3


From 209a2d1419ea575fd316bd9975fb63e40cce7a77 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Sat, 9 May 2020 14:52:07 -0700
Subject: actions/policy: improve errors

ErrMissingPolicyMetadata:
	Include the mount, directory path, and metadata path.  Also move
	the explanation into actions/ since it doesn't refer to any CLI
	command.

ErrPolicyMetadataMismatch:
	Include a lot more information.  Also start checking for
	consistency of the policy key descriptors, not just the
	encryption options.  Add a test for this.

ErrDifferentFilesystem:
	Include the mountpoints.

ErrOnlyProtector:
	Clarify the message and include the protector descriptor.

ErrAlreadyProtected:
ErrNotProtected:
	Include the policy and protector descriptors.

ErrAccessDeniedPossiblyV2:
	Make it slightly clearer what failed.  Also move the explanation
	into actions/ since it doesn't refer to any CLI command.
---
 actions/policy.go        | 134 +++++++++++++++++++++++++++++++++++++++--------
 cli-tests/t_unlock.out   |  38 ++++++++++----
 cli-tests/t_unlock.sh    |  13 +++++
 cmd/fscrypt/errors.go    |  15 ------
 cmd/fscrypt/prompt.go    |   3 +-
 filesystem/filesystem.go |  10 ++--
 6 files changed, 161 insertions(+), 52 deletions(-)

diff --git a/actions/policy.go b/actions/policy.go
index 6c2aa51..a5fd481 100644
--- a/actions/policy.go
+++ b/actions/policy.go
@@ -34,16 +34,109 @@ import (
 	"github.com/google/fscrypt/util"
 )
 
-// Errors relating to Policies
-var (
-	ErrMissingPolicyMetadata  = util.SystemError("missing policy metadata for encrypted directory")
-	ErrPolicyMetadataMismatch = util.SystemError("inconsistent metadata between filesystem and directory")
-	ErrDifferentFilesystem    = errors.New("policies may only protect files on the same filesystem")
-	ErrOnlyProtector          = errors.New("cannot remove the only protector for a policy")
-	ErrAlreadyProtected       = errors.New("policy already protected by protector")
-	ErrNotProtected           = errors.New("policy not protected by protector")
-	ErrAccessDeniedPossiblyV2 = errors.New("permission denied")
-)
+// ErrAccessDeniedPossiblyV2 indicates that a directory's encryption policy
+// couldn't be retrieved due to "permission denied", but it looks like it's due
+// to the directory using a v2 policy but the kernel not supporting it.
+type ErrAccessDeniedPossiblyV2 struct {
+	DirPath string
+}
+
+func (err *ErrAccessDeniedPossiblyV2) Error() string {
+	return fmt.Sprintf(`
+	failed to get encryption policy of %s: permission denied
+
+	This may be caused by the directory using a v2 encryption policy and the
+	current kernel not supporting it. If indeed the case, then this
+	directory can only be used on kernel v5.4 and later. You can create
+	directories accessible on older kernels by changing policy_version to 1
+	in %s.`,
+		err.DirPath, ConfigFileLocation)
+}
+
+// ErrAlreadyProtected indicates that a policy is already protected by the given
+// protector.
+type ErrAlreadyProtected struct {
+	Policy    *Policy
+	Protector *Protector
+}
+
+func (err *ErrAlreadyProtected) Error() string {
+	return fmt.Sprintf("policy %s is already protected by protector %s",
+		err.Policy.Descriptor(), err.Protector.Descriptor())
+}
+
+// ErrDifferentFilesystem indicates that a policy can't be applied to a
+// directory on a different filesystem.
+type ErrDifferentFilesystem struct {
+	PolicyMount *filesystem.Mount
+	PathMount   *filesystem.Mount
+}
+
+func (err *ErrDifferentFilesystem) Error() string {
+	return fmt.Sprintf(`cannot apply policy from filesystem %q to a
+	directory on filesystem %q. Policies may only protect files on the same
+	filesystem.`, err.PolicyMount.Path, err.PathMount.Path)
+}
+
+// ErrMissingPolicyMetadata indicates that a directory is encrypted but its
+// policy metadata cannot be found.
+type ErrMissingPolicyMetadata struct {
+	Mount      *filesystem.Mount
+	DirPath    string
+	Descriptor string
+}
+
+func (err *ErrMissingPolicyMetadata) Error() string {
+	return fmt.Sprintf(`filesystem %q does not contain the policy metadata
+	for %q. This directory has either been encrypted with another tool (such
+	as e4crypt), or the file %q has been deleted.`,
+		err.Mount.Path, err.DirPath,
+		err.Mount.PolicyPath(err.Descriptor))
+}
+
+// ErrNotProtected indicates that the given policy is not protected by the given
+// protector.
+type ErrNotProtected struct {
+	PolicyDescriptor    string
+	ProtectorDescriptor string
+}
+
+func (err *ErrNotProtected) Error() string {
+	return fmt.Sprintf(`policy %s is not protected by protector %s`,
+		err.PolicyDescriptor, err.ProtectorDescriptor)
+}
+
+// ErrOnlyProtector indicates that the last protector can't be removed from a
+// policy.
+type ErrOnlyProtector struct {
+	Policy *Policy
+}
+
+func (err *ErrOnlyProtector) Error() string {
+	return fmt.Sprintf(`cannot remove the only protector from policy %s. A
+	policy must have at least one protector.`, err.Policy.Descriptor())
+}
+
+// ErrPolicyMetadataMismatch indicates that the policy metadata for an encrypted
+// directory is inconsistent with that directory.
+type ErrPolicyMetadataMismatch struct {
+	DirPath   string
+	Mount     *filesystem.Mount
+	PathData  *metadata.PolicyData
+	MountData *metadata.PolicyData
+}
+
+func (err *ErrPolicyMetadataMismatch) Error() string {
+	return fmt.Sprintf(`inconsistent metadata between encrypted directory %q
+	and its corresponding metadata file %q.
+
+	Directory has descriptor:%s %s
+
+	Metadata file has descriptor:%s %s`,
+		err.DirPath, err.Mount.PolicyPath(err.PathData.KeyDescriptor),
+		err.PathData.KeyDescriptor, err.PathData.Options,
+		err.MountData.KeyDescriptor, err.MountData.Options)
+}
 
 // PurgeAllPolicies removes all policy keys on the filesystem from the kernel
 // keyring. In order for this to fully take effect, the filesystem may also need
@@ -161,7 +254,7 @@ func GetPolicyFromPath(ctx *Context, path string) (*Policy, error) {
 		if os.IsPermission(err) &&
 			filesystem.HaveReadAccessTo(path) &&
 			!keyring.IsFsKeyringSupported(ctx.Mount) {
-			return nil, errors.Wrapf(ErrAccessDeniedPossiblyV2, "open %s", path)
+			return nil, &ErrAccessDeniedPossiblyV2{path}
 		}
 		return nil, err
 	}
@@ -171,14 +264,13 @@ func GetPolicyFromPath(ctx *Context, path string) (*Policy, error) {
 	mountData, err := ctx.Mount.GetPolicy(descriptor)
 	if err != nil {
 		log.Printf("getting policy metadata: %v", err)
-		return nil, errors.Wrap(ErrMissingPolicyMetadata, path)
+		return nil, &ErrMissingPolicyMetadata{ctx.Mount, path, descriptor}
 	}
 	log.Printf("found data for policy %s on %q", descriptor, ctx.Mount.Path)
 
-	if !proto.Equal(pathData.Options, mountData.Options) {
-		log.Printf("options from path: %+v", pathData.Options)
-		log.Printf("options from mount: %+v", mountData.Options)
-		return nil, errors.Wrapf(ErrPolicyMetadataMismatch, "policy %s", descriptor)
+	if !proto.Equal(pathData.Options, mountData.Options) ||
+		pathData.KeyDescriptor != mountData.KeyDescriptor {
+		return nil, &ErrPolicyMetadataMismatch{path, ctx.Mount, pathData, mountData}
 	}
 	log.Print("data from filesystem and path agree")
 
@@ -290,7 +382,7 @@ func (policy *Policy) UnlockWithProtector(protector *Protector) error {
 	}
 	idx, ok := policy.findWrappedKeyIndex(protector.Descriptor())
 	if !ok {
-		return ErrNotProtected
+		return &ErrNotProtected{policy.Descriptor(), protector.Descriptor()}
 	}
 
 	var err error
@@ -321,7 +413,7 @@ func (policy *Policy) UsesProtector(protector *Protector) bool {
 // protector must both be unlocked.
 func (policy *Policy) AddProtector(protector *Protector) error {
 	if policy.UsesProtector(protector) {
-		return ErrAlreadyProtected
+		return &ErrAlreadyProtected{policy, protector}
 	}
 	if policy.key == nil || protector.key == nil {
 		return ErrLocked
@@ -372,11 +464,11 @@ func (policy *Policy) AddProtector(protector *Protector) error {
 func (policy *Policy) RemoveProtector(protector *Protector) error {
 	idx, ok := policy.findWrappedKeyIndex(protector.Descriptor())
 	if !ok {
-		return ErrNotProtected
+		return &ErrNotProtected{policy.Descriptor(), protector.Descriptor()}
 	}
 
 	if len(policy.data.WrappedPolicyKeys) == 1 {
-		return ErrOnlyProtector
+		return &ErrOnlyProtector{policy}
 	}
 
 	// Remove the wrapped key from the data
@@ -397,7 +489,7 @@ func (policy *Policy) Apply(path string) error {
 	if pathMount, err := filesystem.FindMount(path); err != nil {
 		return err
 	} else if pathMount != policy.Context.Mount {
-		return ErrDifferentFilesystem
+		return &ErrDifferentFilesystem{policy.Context.Mount, pathMount}
 	}
 
 	return metadata.SetPolicy(path, policy.data)
diff --git a/cli-tests/t_unlock.out b/cli-tests/t_unlock.out
index 29a10dd..710b063 100644
--- a/cli-tests/t_unlock.out
+++ b/cli-tests/t_unlock.out
@@ -81,21 +81,39 @@ contents
 desc1  Yes       desc2
 
 # Try to unlock with corrupt policy metadata
-[ERROR] fscrypt unlock: MNT/dir: system error: missing
-                        policy metadata for encrypted directory
-
-This file or directory has either been encrypted with another tool (such as
-e4crypt) or the corresponding filesystem metadata has been deleted.
+[ERROR] fscrypt unlock: filesystem "MNT" does not contain
+                        the policy metadata for "MNT/dir".
+                        This directory has either been encrypted with another
+                        tool (such as e4crypt), or the file
+                        "MNT/.fscrypt/policies/desc1"
+                        has been deleted.
 
 # Try to unlock with missing policy metadata
-[ERROR] fscrypt unlock: MNT/dir: system error: missing
-                        policy metadata for encrypted directory
-
-This file or directory has either been encrypted with another tool (such as
-e4crypt) or the corresponding filesystem metadata has been deleted.
+[ERROR] fscrypt unlock: filesystem "MNT" does not contain
+                        the policy metadata for "MNT/dir".
+                        This directory has either been encrypted with another
+                        tool (such as e4crypt), or the file
+                        "MNT/.fscrypt/policies/desc20"
+                        has been deleted.
 
 # Try to unlock with missing protector metadata
 [ERROR] fscrypt unlock: could not load any protectors
 
 You may need to mount a linked filesystem. Run with --verbose for more
 information.
+
+# Try to unlock with wrong policy metadata
+[ERROR] fscrypt unlock: inconsistent metadata between encrypted directory
+                        "MNT/dir1" and its corresponding
+                        metadata file
+                        "MNT/.fscrypt/policies/desc21".
+
+                        Directory has
+                        descriptor:desc21 padding:32
+                        contents:AES_256_XTS filenames:AES_256_CTS
+                        policy_version:2
+
+                        Metadata file has
+                        descriptor:desc23 padding:32
+                        contents:AES_256_XTS filenames:AES_256_CTS
+                        policy_version:2
diff --git a/cli-tests/t_unlock.sh b/cli-tests/t_unlock.sh
index 3dfba41..e32b0f7 100755
--- a/cli-tests/t_unlock.sh
+++ b/cli-tests/t_unlock.sh
@@ -67,3 +67,16 @@ mkdir "$dir"
 echo hunter2 | fscrypt encrypt --quiet --name=prot --skip-unlock "$dir"
 rm "$MNT"/.fscrypt/protectors/*
 _expect_failure "echo hunter2 | fscrypt unlock '$dir'"
+
+_print_header "Try to unlock with wrong policy metadata"
+_reset_filesystems
+mkdir "$MNT/dir1"
+mkdir "$MNT/dir2"
+echo hunter2 | fscrypt encrypt --quiet --name=dir1 --skip-unlock "$MNT/dir1"
+echo hunter2 | fscrypt encrypt --quiet --name=dir2 --skip-unlock "$MNT/dir2"
+policy1=$(find "$MNT/.fscrypt/policies/" -type f | head -1)
+policy2=$(find "$MNT/.fscrypt/policies/" -type f | tail -1)
+mv "$policy1" "$TMPDIR/policy"
+mv "$policy2" "$policy1"
+mv "$TMPDIR/policy" "$policy2"
+_expect_failure "echo hunter2 | fscrypt unlock '$MNT/dir1'"
diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index 829a9d2..3e5beed 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -127,21 +127,6 @@ func getErrorSuggestions(err error) string {
 		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 actions.ErrMissingPolicyMetadata:
-		return `This file or directory has either been encrypted with
-			another tool (such as e4crypt) or the corresponding
-			filesystem metadata has been deleted.`
-	case actions.ErrPolicyMetadataMismatch:
-		return `The metadata for this encrypted directory is in an
-			inconsistent state. This most likely means the filesystem
-			metadata is corrupted.`
-	case actions.ErrAccessDeniedPossiblyV2:
-		return fmt.Sprintf(`This may be caused by the directory using a v2
-		encryption policy and the current kernel not supporting it. If
-		indeed the case, then this directory can only be used on kernel
-		v5.4 and later. You can create directories accessible on older
-		kernels by changing policy_version to 1 in %s.`,
-			actions.ConfigFileLocation)
 	case ErrNoDestructiveOps:
 		return fmt.Sprintf("Use %s to automatically run destructive operations.", shortDisplay(forceFlag))
 	case ErrSpecifyProtector:
diff --git a/cmd/fscrypt/prompt.go b/cmd/fscrypt/prompt.go
index b854fb9..210d7bc 100644
--- a/cmd/fscrypt/prompt.go
+++ b/cmd/fscrypt/prompt.go
@@ -318,7 +318,8 @@ func optionFn(policyDescriptor string, options []*actions.ProtectorOption) (int,
 				return idx, nil
 			}
 		}
-		return 0, actions.ErrNotProtected
+		return 0, &actions.ErrNotProtected{PolicyDescriptor: policyDescriptor,
+			ProtectorDescriptor: protector.Descriptor()}
 	}
 
 	log.Printf("optionFn(%s)", policyDescriptor)
diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index e01f9ff..eb49182 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -181,9 +181,9 @@ func (m *Mount) PolicyDir() string {
 	return filepath.Join(m.BaseDir(), policyDirName)
 }
 
-// policyPath returns the full path to a regular policy file with the
+// PolicyPath returns the full path to a regular policy file with the
 // specified descriptor.
-func (m *Mount) policyPath(descriptor string) string {
+func (m *Mount) PolicyPath(descriptor string) string {
 	return filepath.Join(m.PolicyDir(), descriptor)
 }
 
@@ -512,7 +512,7 @@ func (m *Mount) AddPolicy(data *metadata.PolicyData) error {
 		return err
 	}
 
-	return m.err(m.addMetadata(m.policyPath(data.KeyDescriptor), data))
+	return m.err(m.addMetadata(m.PolicyPath(data.KeyDescriptor), data))
 }
 
 // GetPolicy looks up the policy metadata by descriptor.
@@ -521,7 +521,7 @@ func (m *Mount) GetPolicy(descriptor string) (*metadata.PolicyData, error) {
 		return nil, err
 	}
 	data := new(metadata.PolicyData)
-	return data, m.err(m.getMetadata(m.policyPath(descriptor), data))
+	return data, m.err(m.getMetadata(m.PolicyPath(descriptor), data))
 }
 
 // RemovePolicy deletes the policy metadata from the filesystem storage.
@@ -529,7 +529,7 @@ func (m *Mount) RemovePolicy(descriptor string) error {
 	if err := m.CheckSetup(); err != nil {
 		return err
 	}
-	return m.err(m.removeMetadata(m.policyPath(descriptor)))
+	return m.err(m.removeMetadata(m.PolicyPath(descriptor)))
 }
 
 // ListPolicies lists the descriptors of all policies on this filesystem.
-- 
cgit v1.2.3


From 9383d4be92981a4c956c775479bb48b7eec9db79 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Sat, 9 May 2020 14:52:07 -0700
Subject: crypto: improve errors

ErrKeyLock:
	Rename to ErrMlockUlimit for clarity.

ErrGetrandomFail:
ErrKeyAlloc:
ErrKeyFree:
ErrNegativeLength:
	Replace these with one-off unnamed errors because these were all
	returned in only one place and were never checked for.  Also
	these were all either wrapped backwards or discarded an
	underlying error, so fix that too.
---
 cmd/fscrypt/errors.go |  2 +-
 crypto/crypto.go      | 10 +++-------
 crypto/crypto_test.go |  2 +-
 crypto/key.go         | 10 +++++-----
 crypto/rand.go        |  7 +++----
 5 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index 3e5beed..f4f3ddb 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -92,7 +92,7 @@ func getErrorSuggestions(err error) string {
 	switch errors.Cause(err) {
 	case filesystem.ErrNotSetup:
 		return fmt.Sprintf(`Run "fscrypt setup %s" to use fscrypt on this filesystem.`, mountpointArg)
-	case crypto.ErrKeyLock:
+	case crypto.ErrMlockUlimit:
 		return `Too much memory was requested to be locked in RAM. The
 			current limit for this user can be checked with "ulimit
 			-l". The limit can be modified by either changing the
diff --git a/crypto/crypto.go b/crypto/crypto.go
index 9a138d0..1f64b38 100644
--- a/crypto/crypto.go
+++ b/crypto/crypto.go
@@ -50,13 +50,9 @@ import (
 
 // Crypto error values
 var (
-	ErrBadAuth        = errors.New("key authentication check failed")
-	ErrNegativeLength = errors.New("keys cannot have negative lengths")
-	ErrRecoveryCode   = errors.New("invalid recovery code")
-	ErrGetrandomFail  = util.SystemError("getrandom() failed")
-	ErrKeyAlloc       = util.SystemError("could not allocate memory for key")
-	ErrKeyFree        = util.SystemError("could not free memory of key")
-	ErrKeyLock        = errors.New("could not lock key in memory")
+	ErrBadAuth      = errors.New("key authentication check failed")
+	ErrRecoveryCode = errors.New("invalid recovery code")
+	ErrMlockUlimit  = errors.New("could not lock key in memory")
 )
 
 // panicInputLength panics if "name" has invalid length (expected != actual)
diff --git a/crypto/crypto_test.go b/crypto/crypto_test.go
index 6eb0b02..10b3d17 100644
--- a/crypto/crypto_test.go
+++ b/crypto/crypto_test.go
@@ -257,7 +257,7 @@ func TestBigKeyGen(t *testing.T) {
 	case nil:
 		key.Wipe()
 		return
-	case ErrKeyLock:
+	case ErrMlockUlimit:
 		// Don't fail just because "ulimit -l" is too low.
 		return
 	default:
diff --git a/crypto/key.go b/crypto/key.go
index 77adc95..2e57443 100644
--- a/crypto/key.go
+++ b/crypto/key.go
@@ -98,7 +98,7 @@ func NewBlankKey(length int) (*Key, error) {
 	if length == 0 {
 		return &Key{data: nil}, nil
 	} else if length < 0 {
-		return nil, errors.Wrapf(ErrNegativeLength, "length of %d requested", length)
+		return nil, errors.Errorf("requested key length %d is negative", length)
 	}
 
 	flags := keyMmapFlags
@@ -109,11 +109,11 @@ func NewBlankKey(length int) (*Key, error) {
 	// See MAP_ANONYMOUS in http://man7.org/linux/man-pages/man2/mmap.2.html
 	data, err := unix.Mmap(-1, 0, length, keyProtection, flags)
 	if err == unix.EAGAIN {
-		return nil, ErrKeyLock
+		return nil, ErrMlockUlimit
 	}
 	if err != nil {
-		log.Printf("unix.Mmap() with length=%d failed: %v", length, err)
-		return nil, ErrKeyAlloc
+		return nil, errors.Wrapf(err,
+			"failed to allocate (mmap) key buffer of length %d", length)
 	}
 
 	key := &Key{data: data}
@@ -139,7 +139,7 @@ func (key *Key) Wipe() error {
 
 		if err := unix.Munmap(data); err != nil {
 			log.Printf("unix.Munmap() failed: %v", err)
-			return ErrKeyFree
+			return errors.Wrapf(err, "failed to free (munmap) key buffer")
 		}
 	}
 	return nil
diff --git a/crypto/rand.go b/crypto/rand.go
index 4d8c044..7d1e55b 100644
--- a/crypto/rand.go
+++ b/crypto/rand.go
@@ -90,10 +90,9 @@ func (r randReader) Read(buffer []byte) (int, error) {
 	case nil:
 		return n, nil
 	case unix.EAGAIN:
-		return 0, errors.Wrap(ErrGetrandomFail, "insufficient entropy in pool")
+		err = errors.New("insufficient entropy in pool")
 	case unix.ENOSYS:
-		return 0, errors.Wrap(ErrGetrandomFail, "kernel must be v3.17 or later")
-	default:
-		return 0, errors.Wrap(ErrGetrandomFail, err.Error())
+		err = errors.New("kernel must be v3.17 or later")
 	}
+	return 0, errors.Wrap(err, "getrandom() failed")
 }
-- 
cgit v1.2.3


From fb88d74f0335cdf8218bb8dfbaa03f23773318cf Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Sat, 9 May 2020 14:52:07 -0700
Subject: keyring: improve errors

ErrAccessUserKeyring:
	Include the user, and fix the backwards wrapping.

ErrSessionUserKeyring:
	Include the user.

ErrKeyAdd:
ErrKeyRemove:
ErrKeySearch:
ErrLinkUserKeyring:
	Replace these with one-off unnamed errors because they are
	never checked for, and this makes it easier for the callers to
	provide better messages, e.g. fixing the backwards wrapping.
---
 cli-tests/t_v1_policy.out |  7 ++++---
 cmd/fscrypt/errors.go     | 16 ++++++++--------
 keyring/fs_keyring.go     | 16 ++++++++++++----
 keyring/keyring.go        | 10 ++--------
 keyring/user_keyring.go   | 45 ++++++++++++++++++++++++++++++++++++++-------
 5 files changed, 64 insertions(+), 30 deletions(-)

diff --git a/cli-tests/t_v1_policy.out b/cli-tests/t_v1_policy.out
index 0ff5219..e693bf5 100644
--- a/cli-tests/t_v1_policy.out
+++ b/cli-tests/t_v1_policy.out
@@ -11,14 +11,15 @@ can be done with --user=USERNAME. To use the root user's keyring or passphrase,
 use --user=root.
 
 # Try to use --user=root as user
-[ERROR] fscrypt encrypt: setting uids: operation not permitted: could not access
-                         user keyring
+[ERROR] fscrypt encrypt: could not access user keyring for "root": setting uids:
+                         operation not permitted
 
 You can only use --user=USERNAME to access the user keyring of another user if
 you are running as root.
 
 # Try to encrypt without user keyring in session keyring
-[ERROR] fscrypt encrypt: user keyring not linked into session keyring
+[ERROR] fscrypt encrypt: user keyring for "fscrypt-test-user" is not linked into
+                         the session keyring
 
 This is usually the result of a bad PAM configuration. Either correct the
 problem in your PAM stack, enable pam_keyinit.so, or run "keyctl link @u @s".
diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index f4f3ddb..3f7150b 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -88,6 +88,14 @@ func getErrorSuggestions(err error) string {
 		return fmt.Sprintf("Use %s to specify a protector name.", shortDisplay(nameFlag))
 	case *actions.ErrNoConfigFile:
 		return `Run "sudo fscrypt setup" to create this file.`
+	case *keyring.ErrAccessUserKeyring:
+		return fmt.Sprintf(`You can only use %s to access the user
+			keyring of another user if you are running as root.`,
+			shortDisplay(userFlag))
+	case *keyring.ErrSessionUserKeyring:
+		return `This is usually the result of a bad PAM configuration.
+			Either correct the problem in your PAM stack, enable
+			pam_keyinit.so, or run "keyctl link @u @s".`
 	}
 	switch errors.Cause(err) {
 	case filesystem.ErrNotSetup:
@@ -115,14 +123,6 @@ func getErrorSuggestions(err error) string {
 		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.ErrSessionUserKeying:
-		return `This is usually the result of a bad PAM configuration.
-			Either correct the problem in your PAM stack, enable
-			pam_keyinit.so, or run "keyctl link @u @s".`
-	case keyring.ErrAccessUserKeyring:
-		return fmt.Sprintf(`You can only use %s to access the user
-			keyring of another user if you are running as root.`,
-			shortDisplay(userFlag))
 	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
diff --git a/keyring/fs_keyring.go b/keyring/fs_keyring.go
index 262e0e5..9b949b9 100644
--- a/keyring/fs_keyring.go
+++ b/keyring/fs_keyring.go
@@ -203,7 +203,9 @@ func fsAddEncryptionKey(key *crypto.Key, descriptor string,
 
 	log.Printf("FS_IOC_ADD_ENCRYPTION_KEY(%q, %s, <raw>) = %v", mount.Path, descriptor, errno)
 	if errno != 0 {
-		return errors.Wrap(ErrKeyAdd, errno.Error())
+		return errors.Wrapf(errno,
+			"error adding key with descriptor %s to filesystem %s",
+			descriptor, mount.Path)
 	}
 	if descriptor, err = validateKeyDescriptor(&arg.Key_spec, descriptor); err != nil {
 		fsRemoveEncryptionKey(descriptor, mount, user)
@@ -266,7 +268,9 @@ func fsRemoveEncryptionKey(descriptor string, mount *filesystem.Mount,
 		}
 		return ErrKeyNotPresent
 	default:
-		return errors.Wrap(ErrKeyRemove, errno.Error())
+		return errors.Wrapf(errno,
+			"error removing key with descriptor %s from filesystem %s",
+			descriptor, mount.Path)
 	}
 }
 
@@ -298,7 +302,10 @@ func fsGetEncryptionKeyStatus(descriptor string, mount *filesystem.Mount,
 	log.Printf("FS_IOC_GET_ENCRYPTION_KEY_STATUS(%q, %s) = %v, status=%d, status_flags=0x%x",
 		mount.Path, descriptor, errno, arg.Status, arg.Status_flags)
 	if errno != 0 {
-		return KeyStatusUnknown, errors.Wrap(ErrKeySearch, errno.Error())
+		return KeyStatusUnknown,
+			errors.Wrapf(errno,
+				"error getting status of key with descriptor %s on filesystem %s",
+				descriptor, mount.Path)
 	}
 	switch arg.Status {
 	case unix.FSCRYPT_KEY_STATUS_ABSENT:
@@ -313,6 +320,7 @@ func fsGetEncryptionKeyStatus(descriptor string, mount *filesystem.Mount,
 		return KeyAbsentButFilesBusy, nil
 	default:
 		return KeyStatusUnknown,
-			errors.Wrapf(ErrKeySearch, "unknown key status (%d)", arg.Status)
+			errors.Errorf("unknown key status (%d) for key with descriptor %s on filesystem %s",
+				arg.Status, descriptor, mount.Path)
 	}
 }
diff --git a/keyring/keyring.go b/keyring/keyring.go
index fb9cc0e..5ddceaf 100644
--- a/keyring/keyring.go
+++ b/keyring/keyring.go
@@ -43,15 +43,9 @@ import (
 
 // Keyring error values
 var (
-	ErrKeyAdd                = util.SystemError("could not add key to the keyring")
-	ErrKeyRemove             = util.SystemError("could not remove key from the keyring")
-	ErrKeyNotPresent         = errors.New("key not present or already removed")
-	ErrKeyFilesOpen          = errors.New("some files using the key are still open")
 	ErrKeyAddedByOtherUsers  = errors.New("other users have added the key too")
-	ErrKeySearch             = errors.New("could not find key with descriptor")
-	ErrSessionUserKeying     = errors.New("user keyring not linked into session keyring")
-	ErrAccessUserKeyring     = errors.New("could not access user keyring")
-	ErrLinkUserKeyring       = util.SystemError("could not link user keyring into root keyring")
+	ErrKeyFilesOpen          = errors.New("some files using the key are still open")
+	ErrKeyNotPresent         = errors.New("key not present or already removed")
 	ErrV2PoliciesUnsupported = errors.New("kernel is too old to support v2 encryption policies")
 )
 
diff --git a/keyring/user_keyring.go b/keyring/user_keyring.go
index beeb36d..0ea4689 100644
--- a/keyring/user_keyring.go
+++ b/keyring/user_keyring.go
@@ -36,6 +36,29 @@ import (
 	"github.com/google/fscrypt/util"
 )
 
+// ErrAccessUserKeyring indicates that a user's keyring cannot be
+// accessed.
+type ErrAccessUserKeyring struct {
+	TargetUser      *user.User
+	UnderlyingError error
+}
+
+func (err *ErrAccessUserKeyring) Error() string {
+	return fmt.Sprintf("could not access user keyring for %q: %s",
+		err.TargetUser.Username, err.UnderlyingError)
+}
+
+// ErrSessionUserKeyring indicates that a user's keyring is not linked
+// into the session keyring.
+type ErrSessionUserKeyring struct {
+	TargetUser *user.User
+}
+
+func (err *ErrSessionUserKeyring) Error() string {
+	return fmt.Sprintf("user keyring for %q is not linked into the session keyring",
+		err.TargetUser.Username)
+}
+
 // KeyType is always logon as required by filesystem encryption.
 const KeyType = "logon"
 
@@ -67,7 +90,9 @@ func userAddKey(key *crypto.Key, description string, targetUser *user.User) erro
 	log.Printf("KeyctlAddKey(%s, %s, <data>, %d) = %d, %v",
 		KeyType, description, keyringID, keyID, err)
 	if err != nil {
-		return errors.Wrap(ErrKeyAdd, err.Error())
+		return errors.Wrapf(err,
+			"error adding key with description %s to user keyring for %q",
+			description, targetUser.Username)
 	}
 	return nil
 }
@@ -86,7 +111,9 @@ func userRemoveKey(description string, targetUser *user.User) error {
 	_, err = unix.KeyctlInt(unix.KEYCTL_UNLINK, keyID, keyringID, 0, 0)
 	log.Printf("KeyctlUnlink(%d, %d) = %v", keyID, keyringID, err)
 	if err != nil {
-		return errors.Wrap(ErrKeyRemove, err.Error())
+		return errors.Wrapf(err,
+			"error removing key with description %s from user keyring for %q",
+			description, targetUser.Username)
 	}
 	return nil
 }
@@ -106,7 +133,9 @@ func userFindKey(description string, targetUser *user.User) (int, int, error) {
 	keyID, err := unix.KeyctlSearch(keyringID, KeyType, description, 0)
 	log.Printf("KeyctlSearch(%d, %s, %s) = %d, %v", keyringID, KeyType, description, keyID, err)
 	if err != nil {
-		return 0, 0, errors.Wrap(ErrKeySearch, err.Error())
+		return 0, 0, errors.Wrapf(err,
+			"error searching for key %s in user keyring for %q",
+			description, targetUser.Username)
 	}
 	return keyID, keyringID, err
 }
@@ -123,14 +152,14 @@ func UserKeyringID(targetUser *user.User, checkSession bool) (int, error) {
 	uid := util.AtoiOrPanic(targetUser.Uid)
 	targetKeyring, err := userKeyringIDLookup(uid)
 	if err != nil {
-		return 0, errors.Wrap(ErrAccessUserKeyring, err.Error())
+		return 0, &ErrAccessUserKeyring{targetUser, err}
 	}
 
 	if !util.IsUserRoot() {
 		// Make sure the returned keyring will be accessible by checking
 		// that it is in the session keyring.
 		if checkSession && !isUserKeyringInSession(uid) {
-			return 0, ErrSessionUserKeying
+			return 0, &ErrSessionUserKeyring{targetUser}
 		}
 		return targetKeyring, nil
 	}
@@ -139,12 +168,14 @@ func UserKeyringID(targetUser *user.User, checkSession bool) (int, error) {
 	// the root user's user keyring (which will not be garbage collected).
 	rootKeyring, err := userKeyringIDLookup(0)
 	if err != nil {
-		return 0, errors.Wrap(ErrLinkUserKeyring, err.Error())
+		return 0, errors.Wrapf(err, "error looking up root's user keyring")
 	}
 
 	if rootKeyring != targetKeyring {
 		if err = keyringLink(targetKeyring, rootKeyring); err != nil {
-			return 0, errors.Wrap(ErrLinkUserKeyring, err.Error())
+			return 0, errors.Wrapf(err,
+				"error linking user keyring for %q into root's user keyring",
+				targetUser.Username)
 		}
 	}
 	return targetKeyring, nil
-- 
cgit v1.2.3


From fbc161a77962fe64e3caad80efb535d28d8c1f74 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Sat, 9 May 2020 14:52:07 -0700
Subject: metadata: improve errors

ErrBadOwners:
	Rename to ErrDirectoryNotOwned for clarity, move it from
	cmd/fscrypt/ to metadata/ where it better belongs, and improve
	the message.

ErrEncrypted:
	Rename to ErrAlreadyEncrypted for clarity, and include the path.

ErrNotEncrypted:
	Include the path.

ErrBadEncryptionOptions:
	Include the path and bad options.

ErrEncryptionNotSupported:
ErrEncryptionNotEnabled:
	Don't wrap with "get encryption policy %s", in preparation for
	wrapping these with filesystem-level context instead.

Also avoid mixing together the error handling for the "get policy" and
"set policy" ioctls.  Make it very clear how we're handling the errors
from each ioctl.
---
 cli-tests/t_encrypt.out              |  24 +++----
 cli-tests/t_encrypt_custom.out       |   4 +-
 cli-tests/t_encrypt_login.out        |   8 +--
 cli-tests/t_encrypt_raw_key.out      |   4 +-
 cli-tests/t_not_enabled.out          |   9 +--
 cli-tests/t_not_supported.out        |   3 +-
 cli-tests/t_status.out               |   8 +--
 cli-tests/t_v1_policy_fs_keyring.out |   4 +-
 cmd/fscrypt/commands.go              |  17 ++---
 cmd/fscrypt/errors.go                |   4 --
 metadata/policy.go                   | 131 +++++++++++++++++++++++++----------
 11 files changed, 133 insertions(+), 83 deletions(-)

diff --git a/cli-tests/t_encrypt.out b/cli-tests/t_encrypt.out
index af38299..e3bace0 100644
--- a/cli-tests/t_encrypt.out
+++ b/cli-tests/t_encrypt.out
@@ -3,8 +3,8 @@
 [ERROR] fscrypt encrypt: no such file or directory
 ext4 filesystem "MNT" has 0 protectors and 0 policies
 
-[ERROR] fscrypt status: get encryption policy MNT/dir: file
-                        or directory not encrypted
+[ERROR] fscrypt status: file or directory "MNT/dir" is not
+                        encrypted
 
 # Try to encrypt a nonempty directory
 [ERROR] fscrypt encrypt: MNT/dir: not an empty directory
@@ -14,8 +14,8 @@ in-place. Instead, encrypt an empty directory, copy the files into that
 encrypted directory, and securely delete the originals with "shred".
 ext4 filesystem "MNT" has 0 protectors and 0 policies
 
-[ERROR] fscrypt status: get encryption policy MNT/dir: file
-                        or directory not encrypted
+[ERROR] fscrypt status: file or directory "MNT/dir" is not
+                        encrypted
 
 # Encrypt a directory as non-root user
 ext4 filesystem "MNT" has 1 protector and 1 policy
@@ -52,16 +52,16 @@ PROTECTOR         LINKED  DESCRIPTION
 desc1  No      custom protector "prot"
 
 # Try to encrypt an already-encrypted directory
-[ERROR] fscrypt encrypt: MNT/dir: file or directory already
-                         encrypted
+[ERROR] fscrypt encrypt: file or directory "MNT/dir" is
+                         already encrypted
 
 # Try to encrypt another user's directory as a non-root user
-[ERROR] fscrypt encrypt: MNT/dir: you do not own this
-                         directory
+[ERROR] fscrypt encrypt: cannot encrypt "MNT/dir" because
+                         it's owned by another user (root).
 
-Encryption can only be setup on directories you own, even if you have write
-permission for the directory.
+                         Encryption can only be enabled on a directory you own,
+                         even if you have write access to the directory.
 ext4 filesystem "MNT" has 0 protectors and 0 policies
 
-[ERROR] fscrypt status: get encryption policy MNT/dir: file
-                        or directory not encrypted
+[ERROR] fscrypt status: file or directory "MNT/dir" is not
+                        encrypted
diff --git a/cli-tests/t_encrypt_custom.out b/cli-tests/t_encrypt_custom.out
index e7b8656..8dd15e3 100644
--- a/cli-tests/t_encrypt_custom.out
+++ b/cli-tests/t_encrypt_custom.out
@@ -51,5 +51,5 @@ desc6  No      custom protector "prot"
 Use --name=PROTECTOR_NAME to specify a protector name.
 ext4 filesystem "MNT" has 0 protectors and 0 policies
 
-[ERROR] fscrypt status: get encryption policy MNT/dir: file
-                        or directory not encrypted
+[ERROR] fscrypt status: file or directory "MNT/dir" is not
+                        encrypted
diff --git a/cli-tests/t_encrypt_login.out b/cli-tests/t_encrypt_login.out
index 7ee66a2..e8e0e41 100644
--- a/cli-tests/t_encrypt_login.out
+++ b/cli-tests/t_encrypt_login.out
@@ -139,8 +139,8 @@ ext4 filesystem "MNT" has 0 protectors and 0 policies
 
 ext4 filesystem "MNT_ROOT" has 0 protectors and 0 policies
 
-[ERROR] fscrypt status: get encryption policy MNT/dir: file
-                        or directory not encrypted
+[ERROR] fscrypt status: file or directory "MNT/dir" is not
+                        encrypted
 
 # Try to use the wrong login passphrase
 [ERROR] fscrypt encrypt: incorrect login passphrase
@@ -148,5 +148,5 @@ ext4 filesystem "MNT" has 0 protectors and 0 policies
 
 ext4 filesystem "MNT_ROOT" has 0 protectors and 0 policies
 
-[ERROR] fscrypt status: get encryption policy MNT/dir: file
-                        or directory not encrypted
+[ERROR] fscrypt status: file or directory "MNT/dir" is not
+                        encrypted
diff --git a/cli-tests/t_encrypt_raw_key.out b/cli-tests/t_encrypt_raw_key.out
index c7c46eb..8765ba2 100644
--- a/cli-tests/t_encrypt_raw_key.out
+++ b/cli-tests/t_encrypt_raw_key.out
@@ -21,5 +21,5 @@ desc1  No      raw key protector "prot"
 [ERROR] fscrypt encrypt: TMPDIR/raw_key: key file must be 32 bytes
 ext4 filesystem "MNT" has 0 protectors and 0 policies
 
-[ERROR] fscrypt status: get encryption policy MNT/dir: file
-                        or directory not encrypted
+[ERROR] fscrypt status: file or directory "MNT/dir" is not
+                        encrypted
diff --git a/cli-tests/t_not_enabled.out b/cli-tests/t_not_enabled.out
index 7d74bcf..760f9dd 100644
--- a/cli-tests/t_not_enabled.out
+++ b/cli-tests/t_not_enabled.out
@@ -2,24 +2,21 @@
 # Disable encryption on DEV
 
 # Try to encrypt a directory when encryption is disabled
-[ERROR] fscrypt encrypt: get encryption policy MNT/dir:
-                         encryption not enabled
+[ERROR] fscrypt encrypt: encryption not enabled
 
 Encryption is either disabled in the kernel config, or needs to be enabled for
 this filesystem. See the documentation on how to enable encryption on ext4
 systems (and the risks of doing so).
 
 # Try to unlock a directory when encryption is disabled
-[ERROR] fscrypt unlock: get encryption policy MNT/dir:
-                        encryption not enabled
+[ERROR] fscrypt unlock: encryption not enabled
 
 Encryption is either disabled in the kernel config, or needs to be enabled for
 this filesystem. See the documentation on how to enable encryption on ext4
 systems (and the risks of doing so).
 
 # Try to lock a directory when encryption is disabled
-[ERROR] fscrypt lock: get encryption policy MNT/dir:
-                      encryption not enabled
+[ERROR] fscrypt lock: encryption not enabled
 
 Encryption is either disabled in the kernel config, or needs to be enabled for
 this filesystem. See the documentation on how to enable encryption on ext4
diff --git a/cli-tests/t_not_supported.out b/cli-tests/t_not_supported.out
index 8af840c..dd71599 100644
--- a/cli-tests/t_not_supported.out
+++ b/cli-tests/t_not_supported.out
@@ -5,7 +5,6 @@
 Metadata directories created at "MNT/.fscrypt".
 
 # Try to encrypt a directory on tmpfs
-[ERROR] fscrypt encrypt: get encryption policy MNT/dir:
-                         encryption not supported
+[ERROR] fscrypt encrypt: encryption not supported
 
 Encryption for this type of filesystem is not supported on this kernel version.
diff --git a/cli-tests/t_status.out b/cli-tests/t_status.out
index b036712..08ce3b2 100644
--- a/cli-tests/t_status.out
+++ b/cli-tests/t_status.out
@@ -10,10 +10,10 @@ ext4 filesystem "MNT" has 0 protectors and 0 policies
 
 
 # Get status of unencrypted directory on setup mountpoint
-[ERROR] fscrypt status: get encryption policy MNT/dir: file
-                        or directory not encrypted
-[ERROR] fscrypt status: get encryption policy MNT/dir: file
-                        or directory not encrypted
+[ERROR] fscrypt status: file or directory "MNT/dir" is not
+                        encrypted
+[ERROR] fscrypt status: file or directory "MNT/dir" is not
+                        encrypted
 
 # Remove fscrypt metadata from MNT
 
diff --git a/cli-tests/t_v1_policy_fs_keyring.out b/cli-tests/t_v1_policy_fs_keyring.out
index ca32ec1..cfc8f7c 100644
--- a/cli-tests/t_v1_policy_fs_keyring.out
+++ b/cli-tests/t_v1_policy_fs_keyring.out
@@ -10,8 +10,8 @@ Either this command should be run as root, or you should set
 re-create your encrypted directories using v2 encryption policies rather than v1
 (this requires setting '"policy_version": "2"' in the "options" section of
 /etc/fscrypt.conf).
-[ERROR] fscrypt status: get encryption policy MNT/dir: file
-                        or directory not encrypted
+[ERROR] fscrypt status: file or directory "MNT/dir" is not
+                        encrypted
 
 # Encrypt directory as user with --skip-unlock
 "MNT/dir" is encrypted with fscrypt.
diff --git a/cmd/fscrypt/commands.go b/cmd/fscrypt/commands.go
index 51cf136..86816ba 100644
--- a/cmd/fscrypt/commands.go
+++ b/cmd/fscrypt/commands.go
@@ -282,11 +282,7 @@ func encryptPath(path string) (err error) {
 			}
 		}()
 	}
-	if err = policy.Apply(path); os.IsPermission(errors.Cause(err)) {
-		// EACCES at this point indicates ownership issues.
-		err = errors.Wrap(ErrBadOwners, path)
-	}
-	if err != nil {
+	if err = policy.Apply(path); err != nil {
 		return
 	}
 	if recoveryPassphrase != nil {
@@ -320,14 +316,15 @@ func checkEncryptable(ctx *actions.Context, path string) error {
 
 	log.Printf("ensuring %s supports encryption and filesystem is using fscrypt", path)
 	switch _, err := actions.GetPolicyFromPath(ctx, path); errors.Cause(err) {
-	case metadata.ErrNotEncrypted:
-		// We are not encrypted. Finally, we check that the filesystem
-		// supports encryption
-		return ctx.Mount.CheckSupport()
 	case nil:
 		// We are encrypted
-		return errors.Wrap(metadata.ErrEncrypted, path)
+		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()
+		}
 		return err
 	}
 }
diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index 3f7150b..6119862 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -57,7 +57,6 @@ var (
 	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")
-	ErrBadOwners          = errors.New("you do not own this directory")
 	ErrNotEmptyDir        = errors.New("not an empty directory")
 	ErrNotPassphrase      = errors.New("protector does not use a passphrase")
 	ErrUnknownUser        = errors.New("unknown user")
@@ -133,9 +132,6 @@ func getErrorSuggestions(err error) string {
 		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 ErrBadOwners:
-		return `Encryption can only be setup on directories you own,
-			even if you have write permission for the directory.`
 	case ErrNotEmptyDir:
 		return `Encryption can only be setup on empty directories; files
 			cannot be encrypted in-place. Instead, encrypt an empty
diff --git a/metadata/policy.go b/metadata/policy.go
index b95bf42..76c2e6f 100644
--- a/metadata/policy.go
+++ b/metadata/policy.go
@@ -22,9 +22,12 @@ package metadata
 
 import (
 	"encoding/hex"
+	"fmt"
 	"log"
 	"math"
 	"os"
+	"os/user"
+	"strconv"
 	"unsafe"
 
 	"github.com/pkg/errors"
@@ -33,38 +36,70 @@ import (
 	"github.com/google/fscrypt/util"
 )
 
-// Encryption specific errors
 var (
+	// ErrEncryptionNotSupported indicates that encryption is not supported
+	// on the given filesystem, and there is no way to enable it.
 	ErrEncryptionNotSupported = errors.New("encryption not supported")
-	ErrEncryptionNotEnabled   = errors.New("encryption not enabled")
-	ErrNotEncrypted           = errors.New("file or directory not encrypted")
-	ErrEncrypted              = errors.New("file or directory already encrypted")
-	ErrBadEncryptionOptions   = util.SystemError("invalid encryption options provided")
+
+	// ErrEncryptionNotEnabled indicates that encryption is not supported on
+	// the given filesystem, but there is a way to enable it.
+	ErrEncryptionNotEnabled = errors.New("encryption not enabled")
 )
 
-// policyIoctl is a wrapper around the ioctls that get and set encryption
-// policies: FS_IOC_GET_ENCRYPTION_POLICY, FS_IOC_GET_ENCRYPTION_POLICY_EX, and
-// FS_IOC_SET_ENCRYPTION_POLICY.  It translates the raw errno values into more
-// descriptive errors.
+// ErrAlreadyEncrypted indicates that the path is already encrypted.
+type ErrAlreadyEncrypted struct {
+	Path string
+}
+
+func (err *ErrAlreadyEncrypted) Error() string {
+	return fmt.Sprintf("file or directory %q is already encrypted", err.Path)
+}
+
+// ErrBadEncryptionOptions indicates that unsupported encryption options were given.
+type ErrBadEncryptionOptions struct {
+	Path    string
+	Options *EncryptionOptions
+}
+
+func (err *ErrBadEncryptionOptions) Error() string {
+	return fmt.Sprintf(`cannot encrypt %q because the kernel doesn't support the requested encryption options.
+
+	The options are %s`, err.Path, err.Options)
+}
+
+// ErrDirectoryNotOwned indicates a directory can't be encrypted because it's
+// owned by another user.
+type ErrDirectoryNotOwned struct {
+	Path  string
+	Owner uint32
+}
+
+func (err *ErrDirectoryNotOwned) Error() string {
+	owner := strconv.Itoa(int(err.Owner))
+	if u, e := user.LookupId(owner); e == nil && u.Username != "" {
+		owner = u.Username
+	}
+	return fmt.Sprintf(`cannot encrypt %q because it's owned by another user (%s).
+
+	Encryption can only be enabled on a directory you own, even if you have
+	write access to the directory.`, err.Path, owner)
+}
+
+// ErrNotEncrypted indicates that the path is not encrypted.
+type ErrNotEncrypted struct {
+	Path string
+}
+
+func (err *ErrNotEncrypted) Error() string {
+	return fmt.Sprintf("file or directory %q is not encrypted", err.Path)
+}
+
 func policyIoctl(file *os.File, request uintptr, arg unsafe.Pointer) error {
-	// The returned errno value can sometimes give strange errors, so we
-	// return encryption specific errors.
 	_, _, errno := unix.Syscall(unix.SYS_IOCTL, file.Fd(), request, uintptr(arg))
-	switch errno {
-	case 0:
+	if errno == 0 {
 		return nil
-	case unix.ENOTTY:
-		return ErrEncryptionNotSupported
-	case unix.EOPNOTSUPP:
-		return ErrEncryptionNotEnabled
-	case unix.ENODATA, unix.ENOENT:
-		// ENOENT was returned instead of ENODATA on some filesystems before v4.11.
-		return ErrNotEncrypted
-	case unix.EEXIST:
-		return ErrEncrypted
-	default:
-		return errno
 	}
+	return errno
 }
 
 // Maps EncryptionOptions.Padding <-> FSCRYPT_POLICY_FLAGS
@@ -125,13 +160,23 @@ func GetPolicy(path string) (*PolicyData, error) {
 	arg.Size = uint64(unsafe.Sizeof(arg.Policy))
 	policyPtr := util.Ptr(arg.Policy[:])
 	err = policyIoctl(file, unix.FS_IOC_GET_ENCRYPTION_POLICY_EX, unsafe.Pointer(&arg))
-	if err == ErrEncryptionNotSupported {
+	if err == unix.ENOTTY {
 		// Fall back to the old version of the ioctl. This works for v1 policies only.
 		err = policyIoctl(file, unix.FS_IOC_GET_ENCRYPTION_POLICY, policyPtr)
 		arg.Size = uint64(unsafe.Sizeof(unix.FscryptPolicyV1{}))
 	}
-	if err != nil {
-		return nil, errors.Wrapf(err, "get encryption policy %s", path)
+	switch err {
+	case nil:
+		break
+	case unix.ENOTTY:
+		return nil, ErrEncryptionNotSupported
+	case unix.EOPNOTSUPP:
+		return nil, ErrEncryptionNotEnabled
+	case unix.ENODATA, unix.ENOENT:
+		// ENOENT was returned instead of ENODATA on some filesystems before v4.11.
+		return nil, &ErrNotEncrypted{path}
+	default:
+		return nil, errors.Wrapf(err, "failed to get encryption policy of %q", path)
 	}
 	switch arg.Policy[0] { // arg.policy.version
 	case unix.FSCRYPT_POLICY_V1:
@@ -237,7 +282,6 @@ func SetPolicy(path string, data *PolicyData) error {
 	default:
 		err = errors.Errorf("policy version of %d is invalid", data.Options.PolicyVersion)
 	}
-
 	if err == unix.EINVAL {
 		// Before kernel v4.11, many different errors all caused unix.EINVAL to be returned.
 		// We try to disambiguate this error here. This disambiguation will not always give
@@ -247,14 +291,27 @@ func SetPolicy(path string, data *PolicyData) error {
 			err = unix.ENOTDIR
 		} else if _, policyErr := GetPolicy(path); policyErr == nil {
 			// Checking if a policy is already set on this directory
-			err = ErrEncrypted
-		} else {
-			// Default to generic "bad options".
-			err = ErrBadEncryptionOptions
+			err = unix.EEXIST
 		}
 	}
-
-	return errors.Wrapf(err, "set encryption policy %s", path)
+	switch err {
+	case nil:
+		return nil
+	case unix.EACCES:
+		var stat unix.Stat_t
+		if statErr := unix.Stat(path, &stat); statErr == nil && stat.Uid != uint32(os.Geteuid()) {
+			return &ErrDirectoryNotOwned{path, stat.Uid}
+		}
+	case unix.EEXIST:
+		return &ErrAlreadyEncrypted{path}
+	case unix.EINVAL:
+		return &ErrBadEncryptionOptions{path, data.Options}
+	case unix.ENOTTY:
+		return ErrEncryptionNotSupported
+	case unix.EOPNOTSUPP:
+		return ErrEncryptionNotEnabled
+	}
+	return errors.Wrapf(err, "failed to set encryption policy on %q", path)
 }
 
 // CheckSupport returns an error if the filesystem containing path does not
@@ -282,6 +339,10 @@ func CheckSupport(path string) error {
 			Please open an issue, filesystem %q may be corrupted.`, path)
 	case unix.EINVAL, unix.EACCES:
 		return nil
+	case unix.ENOTTY:
+		return ErrEncryptionNotSupported
+	case unix.EOPNOTSUPP:
+		return ErrEncryptionNotEnabled
 	}
-	return err
+	return errors.Wrapf(err, "unexpected error checking for encryption support on filesystem %q", path)
 }
-- 
cgit v1.2.3


From 66fb4c557644ba2c37951a7568c06c47a6c718a7 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Sat, 9 May 2020 14:52:07 -0700
Subject: filesystem: improve errors

Introduce filesystem.ErrEncryptionNotEnabled and
filesystem.ErrEncryptionNotSupported which include the Mount as context,
and translate the corresponding metadata/ errors into them.  Then make
these errors show much better suggestions.

Also replace lots of other filesystem/ errors with either custom types
or with unnamed one-off errors that include more context.  Fix backwards
wrapping in lots of cases.

Finally, don't include the mountpoint in places where it's not useful,
like OS-level errors that already include the path.
---
 actions/policy.go             |   9 +-
 cli-tests/t_not_enabled.out   |  51 +++++++---
 cli-tests/t_not_enabled.sh    |   5 +
 cli-tests/t_not_supported.out |   5 +-
 cli-tests/t_setup.out         |   4 +-
 cli-tests/t_status.out        |  20 ++--
 cli-tests/t_unlock.out        |   7 +-
 cmd/fscrypt/commands.go       |  12 +--
 cmd/fscrypt/errors.go         |  93 +++++++++++++++--
 cmd/fscrypt/status.go         |  12 +--
 filesystem/filesystem.go      | 226 ++++++++++++++++++++++++++++++------------
 filesystem/filesystem_test.go |   3 +-
 filesystem/mountpoint.go      |  26 ++---
 13 files changed, 340 insertions(+), 133 deletions(-)

diff --git a/actions/policy.go b/actions/policy.go
index a5fd481..6c48117 100644
--- a/actions/policy.go
+++ b/actions/policy.go
@@ -246,6 +246,7 @@ func GetPolicyFromPath(ctx *Context, path string) (*Policy, error) {
 	// We double check that the options agree for both the data we get from
 	// the path, and the data we get from the mountpoint.
 	pathData, err := metadata.GetPolicy(path)
+	err = ctx.Mount.EncryptionSupportError(err)
 	if err != nil {
 		// On kernels that don't support v2 encryption policies, trying
 		// to open a directory with a v2 policy simply gave EACCES. This
@@ -264,7 +265,10 @@ func GetPolicyFromPath(ctx *Context, path string) (*Policy, error) {
 	mountData, err := ctx.Mount.GetPolicy(descriptor)
 	if err != nil {
 		log.Printf("getting policy metadata: %v", err)
-		return nil, &ErrMissingPolicyMetadata{ctx.Mount, path, descriptor}
+		if _, ok := err.(*filesystem.ErrPolicyNotFound); ok {
+			return nil, &ErrMissingPolicyMetadata{ctx.Mount, path, descriptor}
+		}
+		return nil, err
 	}
 	log.Printf("found data for policy %s on %q", descriptor, ctx.Mount.Path)
 
@@ -492,7 +496,8 @@ func (policy *Policy) Apply(path string) error {
 		return &ErrDifferentFilesystem{policy.Context.Mount, pathMount}
 	}
 
-	return metadata.SetPolicy(path, policy.data)
+	err := metadata.SetPolicy(path, policy.data)
+	return policy.Context.Mount.EncryptionSupportError(err)
 }
 
 // GetProvisioningStatus returns the status of this policy's key in the keyring.
diff --git a/cli-tests/t_not_enabled.out b/cli-tests/t_not_enabled.out
index 760f9dd..4553891 100644
--- a/cli-tests/t_not_enabled.out
+++ b/cli-tests/t_not_enabled.out
@@ -2,25 +2,52 @@
 # Disable encryption on DEV
 
 # Try to encrypt a directory when encryption is disabled
-[ERROR] fscrypt encrypt: encryption not enabled
+[ERROR] fscrypt encrypt: encryption not enabled on filesystem
+                         MNT (DEV).
 
-Encryption is either disabled in the kernel config, or needs to be enabled for
-this filesystem. See the documentation on how to enable encryption on ext4
-systems (and the risks of doing so).
+To enable encryption support on this filesystem, run:
+
+     sudo tune2fs -O encrypt "DEV"
+
+Also ensure that your kernel has CONFIG_FS_ENCRYPTION=y. See the documentation
+for more details.
 
 # Try to unlock a directory when encryption is disabled
-[ERROR] fscrypt unlock: encryption not enabled
+[ERROR] fscrypt unlock: encryption not enabled on filesystem
+                        MNT (DEV).
+
+To enable encryption support on this filesystem, run:
 
-Encryption is either disabled in the kernel config, or needs to be enabled for
-this filesystem. See the documentation on how to enable encryption on ext4
-systems (and the risks of doing so).
+     sudo tune2fs -O encrypt "DEV"
+
+Also ensure that your kernel has CONFIG_FS_ENCRYPTION=y. See the documentation
+for more details.
 
 # Try to lock a directory when encryption is disabled
-[ERROR] fscrypt lock: encryption not enabled
+[ERROR] fscrypt lock: encryption not enabled on filesystem
+                      MNT (DEV).
+
+To enable encryption support on this filesystem, run:
+
+     sudo tune2fs -O encrypt "DEV"
+
+Also ensure that your kernel has CONFIG_FS_ENCRYPTION=y. See the documentation
+for more details.
+
+# Check for additional message when GRUB appears to be installed
+[ERROR] fscrypt encrypt: encryption not enabled on filesystem
+                         MNT (DEV).
+
+To enable encryption support on this filesystem, run:
+
+     sudo tune2fs -O encrypt "DEV"
+
+WARNING: you seem to have GRUB installed on this filesystem. Before doing the
+above, make sure you are using GRUB v2.04 or later; otherwise your system will
+become unbootable.
 
-Encryption is either disabled in the kernel config, or needs to be enabled for
-this filesystem. See the documentation on how to enable encryption on ext4
-systems (and the risks of doing so).
+Also ensure that your kernel has CONFIG_FS_ENCRYPTION=y. See the documentation
+for more details.
 
 # Enable encryption on DEV
 
diff --git a/cli-tests/t_not_enabled.sh b/cli-tests/t_not_enabled.sh
index 3c7d22c..fae1094 100755
--- a/cli-tests/t_not_enabled.sh
+++ b/cli-tests/t_not_enabled.sh
@@ -26,6 +26,11 @@ _expect_failure "fscrypt unlock '$dir'"
 _print_header "Try to lock a directory when encryption is disabled"
 _expect_failure "fscrypt lock '$dir'"
 
+_print_header "Check for additional message when GRUB appears to be installed"
+mkdir -p "$MNT/boot/grub"
+_expect_failure "fscrypt encrypt '$dir'"
+rm -r "${MNT:?}/boot"
+
 _print_header "Enable encryption on $DEV"
 _run_noisy_command "tune2fs -O encrypt '$DEV'"
 
diff --git a/cli-tests/t_not_supported.out b/cli-tests/t_not_supported.out
index dd71599..ecee56a 100644
--- a/cli-tests/t_not_supported.out
+++ b/cli-tests/t_not_supported.out
@@ -5,6 +5,5 @@
 Metadata directories created at "MNT/.fscrypt".
 
 # Try to encrypt a directory on tmpfs
-[ERROR] fscrypt encrypt: encryption not supported
-
-Encryption for this type of filesystem is not supported on this kernel version.
+[ERROR] fscrypt encrypt: This kernel doesn't support encryption on tmpfs
+                         filesystems.
diff --git a/cli-tests/t_setup.out b/cli-tests/t_setup.out
index 7d597bd..ef0d133 100644
--- a/cli-tests/t_setup.out
+++ b/cli-tests/t_setup.out
@@ -34,8 +34,8 @@ Use --force to automatically run destructive operations.
 Metadata directories created at "MNT/.fscrypt".
 
 # fscrypt setup filesystem (already set up)
-[ERROR] fscrypt setup: filesystem MNT: already setup for use
-                       with fscrypt
+[ERROR] fscrypt setup: filesystem MNT is already setup for
+                       use with fscrypt
 
 # no config file
 [ERROR] fscrypt setup: "FSCRYPT_CONF" doesn't exist
diff --git a/cli-tests/t_status.out b/cli-tests/t_status.out
index 08ce3b2..0d478b5 100644
--- a/cli-tests/t_status.out
+++ b/cli-tests/t_status.out
@@ -24,21 +24,25 @@ ext4 supported No
 ext4 supported No
 
 # Get status of not-setup mountpoint
-[ERROR] fscrypt status: filesystem MNT: not setup for use
+[ERROR] fscrypt status: filesystem MNT is not setup for use
                         with fscrypt
 
-Run "fscrypt setup MOUNTPOINT" to use fscrypt on this filesystem.
-[ERROR] fscrypt status: filesystem MNT: not setup for use
+Run "sudo fscrypt setup MNT" to use fscrypt on this
+filesystem.
+[ERROR] fscrypt status: filesystem MNT is not setup for use
                         with fscrypt
 
-Run "fscrypt setup MOUNTPOINT" to use fscrypt on this filesystem.
+Run "sudo fscrypt setup MNT" to use fscrypt on this
+filesystem.
 
 # Get status of unencrypted directory on not-setup mountpoint
-[ERROR] fscrypt status: filesystem MNT: not setup for use
+[ERROR] fscrypt status: filesystem MNT is not setup for use
                         with fscrypt
 
-Run "fscrypt setup MOUNTPOINT" to use fscrypt on this filesystem.
-[ERROR] fscrypt status: filesystem MNT: not setup for use
+Run "sudo fscrypt setup MNT" to use fscrypt on this
+filesystem.
+[ERROR] fscrypt status: filesystem MNT is not setup for use
                         with fscrypt
 
-Run "fscrypt setup MOUNTPOINT" to use fscrypt on this filesystem.
+Run "sudo fscrypt setup MNT" to use fscrypt on this
+filesystem.
diff --git a/cli-tests/t_unlock.out b/cli-tests/t_unlock.out
index 710b063..25430a0 100644
--- a/cli-tests/t_unlock.out
+++ b/cli-tests/t_unlock.out
@@ -81,12 +81,9 @@ contents
 desc1  Yes       desc2
 
 # Try to unlock with corrupt policy metadata
-[ERROR] fscrypt unlock: filesystem "MNT" does not contain
-                        the policy metadata for "MNT/dir".
-                        This directory has either been encrypted with another
-                        tool (such as e4crypt), or the file
+[ERROR] fscrypt unlock: fscrypt metadata file at
                         "MNT/.fscrypt/policies/desc1"
-                        has been deleted.
+                        is corrupt: unexpected EOF
 
 # Try to unlock with missing policy metadata
 [ERROR] fscrypt unlock: filesystem "MNT" does not contain
diff --git a/cmd/fscrypt/commands.go b/cmd/fscrypt/commands.go
index 86816ba..ea393bb 100644
--- a/cmd/fscrypt/commands.go
+++ b/cmd/fscrypt/commands.go
@@ -74,7 +74,7 @@ func setupAction(c *cli.Context) error {
 			return newExitError(c, err)
 		}
 		if err := setupFilesystem(c.App.Writer, actions.LoginProtectorMountpoint); err != nil {
-			if errors.Cause(err) != filesystem.ErrAlreadySetup {
+			if _, ok := err.(*filesystem.ErrAlreadySetup); !ok {
 				return newExitError(c, err)
 			}
 			fmt.Fprintf(c.App.Writer,
@@ -660,17 +660,15 @@ func statusAction(c *cli.Context) error {
 		err = writeGlobalStatus(c.App.Writer)
 	case 1:
 		path := c.Args().Get(0)
-		ctx, mntErr := actions.NewContextFromMountpoint(path, nil)
 
-		switch errors.Cause(mntErr) {
-		case nil:
+		var ctx *actions.Context
+		ctx, err = actions.NewContextFromMountpoint(path, nil)
+		if err == nil {
 			// Case (2) - mountpoint status
 			err = writeFilesystemStatus(c.App.Writer, ctx)
-		case filesystem.ErrNotAMountpoint:
+		} else if _, ok := err.(*filesystem.ErrNotAMountpoint); ok {
 			// Case (3) - file or directory status
 			err = writePathStatus(c.App.Writer, path)
-		default:
-			err = mntErr
 		}
 	default:
 		return expectedArgsErr(c, 1, true)
diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index 6119862..ef4ae64 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -30,6 +30,7 @@ import (
 
 	"github.com/pkg/errors"
 	"github.com/urfave/cli"
+	"golang.org/x/sys/unix"
 
 	"github.com/google/fscrypt/actions"
 	"github.com/google/fscrypt/crypto"
@@ -75,10 +76,69 @@ func getFullName(c *cli.Context) string {
 	return c.App.HelpName
 }
 
+func suggestEnablingEncryption(mnt *filesystem.Mount) string {
+	kconfig := "CONFIG_FS_ENCRYPTION=y"
+	switch mnt.FilesystemType {
+	case "ext4":
+		// Recommend running tune2fs -O encrypt.  But be really careful;
+		// old kernels didn't support block_size != PAGE_SIZE, and old
+		// GRUB didn't support encryption.
+		var statfs unix.Statfs_t
+		if err := unix.Statfs(mnt.Path, &statfs); err != nil {
+			return ""
+		}
+		pagesize := int64(os.Getpagesize())
+		if statfs.Bsize != pagesize && !util.IsKernelVersionAtLeast(5, 5) {
+			return fmt.Sprintf(`This filesystem uses a block size
+			(%d) other than the system page size (%d). Ext4
+			encryption didn't support this case until kernel v5.5.
+			Do *not* enable encryption on this filesystem. Either
+			upgrade your kernel to v5.5 or later, or re-create this
+			filesystem using 'mkfs.ext4 -b %d -O encrypt %s'
+			(WARNING: that will erase all data on it).`,
+				statfs.Bsize, pagesize, pagesize, mnt.Device)
+		}
+		if !util.IsKernelVersionAtLeast(5, 1) {
+			kconfig = "CONFIG_EXT4_ENCRYPTION=y"
+		}
+		s := fmt.Sprintf(`To enable encryption support on this
+		filesystem, run:
+
+		> sudo tune2fs -O encrypt %q
+		`, mnt.Device)
+		if _, err := os.Stat(filepath.Join(mnt.Path, "boot/grub")); err == nil {
+			s += `
+			WARNING: you seem to have GRUB installed on this
+			filesystem. Before doing the above, make sure you are
+			using GRUB v2.04 or later; otherwise your system will
+			become unbootable.
+			`
+		}
+		s += fmt.Sprintf(`
+		Also ensure that your kernel has %s. See the documentation for
+		more details.`, kconfig)
+		return s
+	case "f2fs":
+		if !util.IsKernelVersionAtLeast(5, 1) {
+			kconfig = "CONFIG_F2FS_FS_ENCRYPTION=y"
+		}
+		return fmt.Sprintf(`To enable encryption support on this
+		filesystem, you'll need to run:
+
+		> sudo fsck.f2fs -O encrypt %q
+
+		Also ensure that your kernel has %s. See the documentation for
+		more details.`, mnt.Device, kconfig)
+	default:
+		return `See the documentation for how to enable encryption
+		support on this filesystem.`
+	}
+}
+
 // getErrorSuggestions returns a string containing suggestions about how to fix
 // an error. If no suggestion is necessary or available, return empty string.
 func getErrorSuggestions(err error) string {
-	switch err.(type) {
+	switch e := err.(type) {
 	case *actions.ErrBadConfigFile:
 		return `Either fix this file manually, or run "sudo fscrypt setup" to recreate it.`
 	case *actions.ErrLoginProtectorName:
@@ -87,6 +147,27 @@ func getErrorSuggestions(err error) string {
 		return fmt.Sprintf("Use %s to specify a protector name.", shortDisplay(nameFlag))
 	case *actions.ErrNoConfigFile:
 		return `Run "sudo fscrypt setup" to create this file.`
+	case *filesystem.ErrEncryptionNotEnabled:
+		return suggestEnablingEncryption(e.Mount)
+	case *filesystem.ErrEncryptionNotSupported:
+		switch e.Mount.FilesystemType {
+		case "ext4":
+			if !util.IsKernelVersionAtLeast(4, 1) {
+				return "ext4 encryption requires kernel v4.1 or later."
+			}
+		case "f2fs":
+			if !util.IsKernelVersionAtLeast(4, 2) {
+				return "f2fs encryption requires kernel v4.2 or later."
+			}
+		case "ubifs":
+			if !util.IsKernelVersionAtLeast(4, 10) {
+				return "ubifs encryption requires kernel v4.10 or later."
+			}
+		}
+		return ""
+	case *filesystem.ErrNotSetup:
+		return fmt.Sprintf(`Run "sudo fscrypt setup %s" to use fscrypt
+		        on this filesystem.`, e.Mount.Path)
 	case *keyring.ErrAccessUserKeyring:
 		return fmt.Sprintf(`You can only use %s to access the user
 			keyring of another user if you are running as root.`,
@@ -97,22 +178,12 @@ func getErrorSuggestions(err error) string {
 			pam_keyinit.so, or run "keyctl link @u @s".`
 	}
 	switch errors.Cause(err) {
-	case filesystem.ErrNotSetup:
-		return fmt.Sprintf(`Run "fscrypt setup %s" to use fscrypt on this filesystem.`, mountpointArg)
 	case crypto.ErrMlockUlimit:
 		return `Too much memory was requested to be locked in RAM. The
 			current limit for this user can be checked with "ulimit
 			-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 metadata.ErrEncryptionNotSupported:
-		return `Encryption for this type of filesystem is not supported
-			on this kernel version.`
-	case metadata.ErrEncryptionNotEnabled:
-		return `Encryption is either disabled in the kernel config, or
-			needs to be enabled for this filesystem. See the
-			documentation on how to enable encryption on ext4
-			systems (and the risks of doing so).`
 	case keyring.ErrKeyFilesOpen:
 		return `Directory was incompletely locked because some files are
 			still open. These files remain accessible. Try killing
diff --git a/cmd/fscrypt/status.go b/cmd/fscrypt/status.go
index 40bb49e..02fdc74 100644
--- a/cmd/fscrypt/status.go
+++ b/cmd/fscrypt/status.go
@@ -27,12 +27,9 @@ import (
 	"strings"
 	"text/tabwriter"
 
-	"github.com/pkg/errors"
-
 	"github.com/google/fscrypt/actions"
 	"github.com/google/fscrypt/filesystem"
 	"github.com/google/fscrypt/keyring"
-	"github.com/google/fscrypt/metadata"
 )
 
 // Creates a writer which correctly aligns tabs with the specified header.
@@ -46,12 +43,13 @@ func makeTableWriter(w io.Writer, header string) *tabwriter.Writer {
 // encryptionStatus will be printed in the ENCRYPTION column. An empty string
 // indicates the filesystem should not be printed.
 func encryptionStatus(err error) string {
-	switch errors.Cause(err) {
-	case nil:
+	if err == nil {
 		return "supported"
-	case metadata.ErrEncryptionNotEnabled:
+	}
+	switch err.(type) {
+	case *filesystem.ErrEncryptionNotEnabled:
 		return "not enabled"
-	case metadata.ErrEncryptionNotSupported:
+	case *filesystem.ErrEncryptionNotSupported:
 		return "not supported"
 	default:
 		// Unknown error regarding support
diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go
index eb49182..9b5b7e2 100644
--- a/filesystem/filesystem.go
+++ b/filesystem/filesystem.go
@@ -47,23 +47,90 @@ import (
 	"golang.org/x/sys/unix"
 
 	"github.com/google/fscrypt/metadata"
-	"github.com/google/fscrypt/util"
 )
 
-// Filesystem error values
-var (
-	ErrNotAMountpoint  = errors.New("not a mountpoint")
-	ErrAlreadySetup    = errors.New("already setup for use with fscrypt")
-	ErrNotSetup        = errors.New("not setup for use with fscrypt")
-	ErrNoMetadata      = errors.New("could not find metadata")
-	ErrLinkedProtector = errors.New("not a regular protector")
-	ErrInvalidMetadata = errors.New("provided metadata is invalid")
-	ErrFollowLink      = errors.New("cannot follow filesystem link")
-	ErrLinkExpired     = errors.New("no longer exists on linked filesystem")
-	ErrMakeLink        = util.SystemError("cannot create filesystem link")
-	ErrGlobalMountInfo = util.SystemError("creating global mountpoint list failed")
-	ErrCorruptMetadata = util.SystemError("on-disk metadata is corrupt")
-)
+// ErrAlreadySetup indicates that a filesystem is already setup for fscrypt.
+type ErrAlreadySetup struct {
+	Mount *Mount
+}
+
+func (err *ErrAlreadySetup) Error() string {
+	return fmt.Sprintf("filesystem %s is already setup for use with fscrypt",
+		err.Mount.Path)
+}
+
+// ErrCorruptMetadata indicates that an fscrypt metadata file is corrupt.
+type ErrCorruptMetadata struct {
+	Path            string
+	UnderlyingError error
+}
+
+func (err *ErrCorruptMetadata) Error() string {
+	return fmt.Sprintf("fscrypt metadata file at %q is corrupt: %s",
+		err.Path, err.UnderlyingError)
+}
+
+// ErrFollowLink indicates that a protector link can't be followed.
+type ErrFollowLink struct {
+	Link            string
+	UnderlyingError error
+}
+
+func (err *ErrFollowLink) Error() string {
+	return fmt.Sprintf("cannot follow filesystem link %q: %s",
+		err.Link, err.UnderlyingError)
+}
+
+// ErrMakeLink indicates that a protector link can't be created.
+type ErrMakeLink struct {
+	Target          *Mount
+	UnderlyingError error
+}
+
+func (err *ErrMakeLink) Error() string {
+	return fmt.Sprintf("cannot create filesystem link to %q: %s",
+		err.Target.Path, err.UnderlyingError)
+}
+
+// ErrNotAMountpoint indicates that a path is not a mountpoint.
+type ErrNotAMountpoint struct {
+	Path string
+}
+
+func (err *ErrNotAMountpoint) Error() string {
+	return fmt.Sprintf("%q is not a mountpoint", err.Path)
+}
+
+// ErrNotSetup indicates that a filesystem is not setup for fscrypt.
+type ErrNotSetup struct {
+	Mount *Mount
+}
+
+func (err *ErrNotSetup) Error() string {
+	return fmt.Sprintf("filesystem %s is not setup for use with fscrypt", err.Mount.Path)
+}
+
+// ErrPolicyNotFound indicates that the policy metadata was not found.
+type ErrPolicyNotFound struct {
+	Descriptor string
+	Mount      *Mount
+}
+
+func (err *ErrPolicyNotFound) Error() string {
+	return fmt.Sprintf("policy metadata for %s not found on filesystem %s",
+		err.Descriptor, err.Mount.Path)
+}
+
+// ErrProtectorNotFound indicates that the protector metadata was not found.
+type ErrProtectorNotFound struct {
+	Descriptor string
+	Mount      *Mount
+}
+
+func (err *ErrProtectorNotFound) Error() string {
+	return fmt.Sprintf("protector metadata for %s not found on filesystem %s",
+		err.Descriptor, err.Mount.Path)
+}
 
 // SortDescriptorsByLastMtime indicates whether descriptors are sorted by last
 // modification time when being listed.  This can be set to true to get
@@ -195,15 +262,45 @@ func (m *Mount) tempMount() (*Mount, error) {
 	return &Mount{Path: tempDir}, err
 }
 
-// err modifies an error to contain the path of this filesystem.
-func (m *Mount) err(err error) error {
-	return errors.Wrapf(err, "filesystem %s", m.Path)
+// ErrEncryptionNotEnabled indicates that encryption is not enabled on the given
+// filesystem.
+type ErrEncryptionNotEnabled struct {
+	Mount *Mount
+}
+
+func (err *ErrEncryptionNotEnabled) Error() string {
+	return fmt.Sprintf("encryption not enabled on filesystem %s (%s).",
+		err.Mount.Path, err.Mount.Device)
+}
+
+// ErrEncryptionNotSupported indicates that encryption is not supported on the
+// given filesystem.
+type ErrEncryptionNotSupported struct {
+	Mount *Mount
+}
+
+func (err *ErrEncryptionNotSupported) Error() string {
+	return fmt.Sprintf("This kernel doesn't support encryption on %s filesystems.",
+		err.Mount.FilesystemType)
+}
+
+// EncryptionSupportError adds filesystem-specific context to the
+// ErrEncryptionNotEnabled and ErrEncryptionNotSupported errors from the
+// metadata package.
+func (m *Mount) EncryptionSupportError(err error) error {
+	switch err {
+	case metadata.ErrEncryptionNotEnabled:
+		return &ErrEncryptionNotEnabled{m}
+	case metadata.ErrEncryptionNotSupported:
+		return &ErrEncryptionNotSupported{m}
+	}
+	return err
 }
 
 // CheckSupport returns an error if this filesystem does not support filesystem
 // encryption.
 func (m *Mount) CheckSupport() error {
-	return m.err(metadata.CheckSupport(m.Path))
+	return m.EncryptionSupportError(metadata.CheckSupport(m.Path))
 }
 
 // CheckSetup returns an error if all the fscrypt metadata directories do not
@@ -217,7 +314,7 @@ func (m *Mount) CheckSetup() error {
 	if baseGood && policyGood && protectorGood {
 		return nil
 	}
-	return m.err(ErrNotSetup)
+	return &ErrNotSetup{m}
 }
 
 // makeDirectories creates the three metadata directories with the correct
@@ -244,21 +341,21 @@ func (m *Mount) makeDirectories() error {
 // or no files in the baseDir are created.
 func (m *Mount) Setup() error {
 	if m.CheckSetup() == nil {
-		return m.err(ErrAlreadySetup)
+		return &ErrAlreadySetup{m}
 	}
 	// We build the directories under a temp Mount and then move into place.
 	temp, err := m.tempMount()
 	if err != nil {
-		return m.err(err)
+		return err
 	}
 	defer os.RemoveAll(temp.Path)
 
 	if err = temp.makeDirectories(); err != nil {
-		return m.err(err)
+		return err
 	}
 
 	// Atomically move directory into place.
-	return m.err(os.Rename(temp.BaseDir(), m.BaseDir()))
+	return os.Rename(temp.BaseDir(), m.BaseDir())
 }
 
 // RemoveAllMetadata removes all the policy and protector metadata from the
@@ -273,12 +370,12 @@ func (m *Mount) RemoveAllMetadata() error {
 	// temp will hold the old metadata temporarily
 	temp, err := m.tempMount()
 	if err != nil {
-		return m.err(err)
+		return err
 	}
 	defer os.RemoveAll(temp.Path)
 
 	// Move directory into temp (to be destroyed on defer)
-	return m.err(os.Rename(m.BaseDir(), temp.BaseDir()))
+	return os.Rename(m.BaseDir(), temp.BaseDir())
 }
 
 func syncDirectory(dirPath string) error {
@@ -333,7 +430,7 @@ func (m *Mount) writeDataAtomic(path string, data []byte) error {
 // path. This will overwrite any existing data. The operation is atomic.
 func (m *Mount) addMetadata(path string, md metadata.Metadata) error {
 	if err := md.CheckValidity(); err != nil {
-		return errors.Wrap(ErrInvalidMetadata, err.Error())
+		return errors.Wrap(err, "provided metadata is invalid")
 	}
 
 	data, err := proto.Marshal(md)
@@ -350,20 +447,16 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata) error {
 func (m *Mount) getMetadata(path string, md metadata.Metadata) error {
 	data, err := ioutil.ReadFile(path)
 	if err != nil {
-		log.Printf("could not read metadata at %q", path)
-		if os.IsNotExist(err) {
-			return errors.Wrapf(ErrNoMetadata, "descriptor %s", filepath.Base(path))
-		}
+		log.Printf("could not read metadata from %q: %v", path, err)
 		return err
 	}
 
 	if err := proto.Unmarshal(data, md); err != nil {
-		return errors.Wrap(ErrCorruptMetadata, err.Error())
+		return &ErrCorruptMetadata{path, err}
 	}
 
 	if err := md.CheckValidity(); err != nil {
-		log.Printf("metadata at %q is not valid", path)
-		return errors.Wrap(ErrCorruptMetadata, err.Error())
+		return &ErrCorruptMetadata{path, err}
 	}
 
 	log.Printf("successfully read metadata from %q", path)
@@ -374,14 +467,11 @@ func (m *Mount) getMetadata(path string, md metadata.Metadata) error {
 // path. Works with regular or linked metadata.
 func (m *Mount) removeMetadata(path string) error {
 	if err := os.Remove(path); err != nil {
-		log.Printf("could not remove metadata at %q", path)
-		if os.IsNotExist(err) {
-			return errors.Wrapf(ErrNoMetadata, "descriptor %s", filepath.Base(path))
-		}
+		log.Printf("could not remove metadata file at %q: %v", path, err)
 		return err
 	}
 
-	log.Printf("successfully removed metadata at %q", path)
+	log.Printf("successfully removed metadata file at %q", path)
 	return nil
 }
 
@@ -394,10 +484,11 @@ func (m *Mount) AddProtector(data *metadata.ProtectorData) error {
 		return err
 	}
 	if isRegularFile(m.linkedProtectorPath(data.ProtectorDescriptor)) {
-		return m.err(ErrLinkedProtector)
+		return errors.Errorf("cannot modify linked protector %s on filesystem %s",
+			data.ProtectorDescriptor, m.Path)
 	}
 	path := m.protectorPath(data.ProtectorDescriptor)
-	return m.err(m.addMetadata(path, data))
+	return m.addMetadata(path, data)
 }
 
 // AddLinkedProtector adds a link in this filesystem to the protector metadata
@@ -419,10 +510,10 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error)
 	if err == nil {
 		existingLinkedMnt, err := getMountFromLink(string(existingLink))
 		if err != nil {
-			return false, err
+			return false, errors.Wrap(err, linkPath)
 		}
 		if existingLinkedMnt != dest {
-			return false, errors.Wrapf(ErrFollowLink, "link %q points to %q, but expected %q",
+			return false, errors.Errorf("link %q points to %q, but expected %q",
 				linkPath, existingLinkedMnt.Path, dest.Path)
 		}
 		return false, nil
@@ -435,9 +526,9 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error)
 	var newLink string
 	newLink, err = makeLink(dest, "UUID")
 	if err != nil {
-		return false, dest.err(err)
+		return false, err
 	}
-	return true, m.err(m.writeDataAtomic(linkPath, []byte(newLink)))
+	return true, m.writeDataAtomic(linkPath, []byte(newLink))
 }
 
 // GetRegularProtector looks up the protector metadata by descriptor. This will
@@ -448,7 +539,11 @@ func (m *Mount) GetRegularProtector(descriptor string) (*metadata.ProtectorData,
 	}
 	data := new(metadata.ProtectorData)
 	path := m.protectorPath(descriptor)
-	return data, m.err(m.getMetadata(path, data))
+	err := m.getMetadata(path, data)
+	if os.IsNotExist(err) {
+		err = &ErrProtectorNotFound{descriptor, m}
+	}
+	return data, err
 }
 
 // GetProtector returns the Mount of the filesystem containing the information
@@ -459,24 +554,24 @@ func (m *Mount) GetProtector(descriptor string) (*Mount, *metadata.ProtectorData
 		return nil, nil, err
 	}
 	// Get the link data from the link file
-	link, err := ioutil.ReadFile(m.linkedProtectorPath(descriptor))
+	path := m.linkedProtectorPath(descriptor)
+	link, err := ioutil.ReadFile(path)
 	if err != nil {
 		// If the link doesn't exist, try for a regular protector.
 		if os.IsNotExist(err) {
 			data, err := m.GetRegularProtector(descriptor)
 			return m, data, err
 		}
-		return nil, nil, m.err(err)
+		return nil, nil, err
 	}
-
+	log.Printf("following protector link %s", path)
 	linkedMnt, err := getMountFromLink(string(link))
 	if err != nil {
-		return nil, nil, m.err(err)
+		return nil, nil, errors.Wrap(err, path)
 	}
 	data, err := linkedMnt.GetRegularProtector(descriptor)
 	if err != nil {
-		log.Print(err)
-		return nil, nil, m.err(errors.Wrapf(ErrLinkExpired, "protector %s", descriptor))
+		return nil, nil, &ErrFollowLink{string(link), err}
 	}
 	return linkedMnt, data, nil
 }
@@ -490,10 +585,13 @@ func (m *Mount) RemoveProtector(descriptor string) error {
 	// We first try to remove the linkedProtector. If that metadata does not
 	// exist, we try to remove the normal protector.
 	err := m.removeMetadata(m.linkedProtectorPath(descriptor))
-	if errors.Cause(err) == ErrNoMetadata {
+	if os.IsNotExist(err) {
 		err = m.removeMetadata(m.protectorPath(descriptor))
+		if os.IsNotExist(err) {
+			err = &ErrProtectorNotFound{descriptor, m}
+		}
 	}
-	return m.err(err)
+	return err
 }
 
 // ListProtectors lists the descriptors of all protectors on this filesystem.
@@ -502,8 +600,7 @@ func (m *Mount) ListProtectors() ([]string, error) {
 	if err := m.CheckSetup(); err != nil {
 		return nil, err
 	}
-	protectors, err := m.listDirectory(m.ProtectorDir())
-	return protectors, m.err(err)
+	return m.listDirectory(m.ProtectorDir())
 }
 
 // AddPolicy adds the policy metadata to the filesystem storage.
@@ -512,7 +609,7 @@ func (m *Mount) AddPolicy(data *metadata.PolicyData) error {
 		return err
 	}
 
-	return m.err(m.addMetadata(m.PolicyPath(data.KeyDescriptor), data))
+	return m.addMetadata(m.PolicyPath(data.KeyDescriptor), data)
 }
 
 // GetPolicy looks up the policy metadata by descriptor.
@@ -521,7 +618,11 @@ func (m *Mount) GetPolicy(descriptor string) (*metadata.PolicyData, error) {
 		return nil, err
 	}
 	data := new(metadata.PolicyData)
-	return data, m.err(m.getMetadata(m.PolicyPath(descriptor), data))
+	err := m.getMetadata(m.PolicyPath(descriptor), data)
+	if os.IsNotExist(err) {
+		err = &ErrPolicyNotFound{descriptor, m}
+	}
+	return data, err
 }
 
 // RemovePolicy deletes the policy metadata from the filesystem storage.
@@ -529,7 +630,11 @@ func (m *Mount) RemovePolicy(descriptor string) error {
 	if err := m.CheckSetup(); err != nil {
 		return err
 	}
-	return m.err(m.removeMetadata(m.PolicyPath(descriptor)))
+	err := m.removeMetadata(m.PolicyPath(descriptor))
+	if os.IsNotExist(err) {
+		err = &ErrPolicyNotFound{descriptor, m}
+	}
+	return err
 }
 
 // ListPolicies lists the descriptors of all policies on this filesystem.
@@ -537,8 +642,7 @@ func (m *Mount) ListPolicies() ([]string, error) {
 	if err := m.CheckSetup(); err != nil {
 		return nil, err
 	}
-	policies, err := m.listDirectory(m.PolicyDir())
-	return policies, m.err(err)
+	return m.listDirectory(m.PolicyDir())
 }
 
 type namesAndTimes struct {
diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go
index 4bed96a..9b534bd 100644
--- a/filesystem/filesystem_test.go
+++ b/filesystem/filesystem_test.go
@@ -27,7 +27,6 @@ import (
 	"testing"
 
 	"github.com/golang/protobuf/proto"
-	"github.com/pkg/errors"
 
 	"github.com/google/fscrypt/crypto"
 	"github.com/google/fscrypt/metadata"
@@ -367,7 +366,7 @@ func TestLinkedProtector(t *testing.T) {
 
 	// Get the protector though the second system
 	_, err = fakeMnt.GetRegularProtector(protector.ProtectorDescriptor)
-	if errors.Cause(err) != ErrNoMetadata {
+	if _, ok := err.(*ErrProtectorNotFound); !ok {
 		t.Fatal(err)
 	}
 
diff --git a/filesystem/mountpoint.go b/filesystem/mountpoint.go
index acddbae..c830780 100644
--- a/filesystem/mountpoint.go
+++ b/filesystem/mountpoint.go
@@ -380,7 +380,7 @@ func FindMount(path string) (*Mount, error) {
 func GetMount(mountpoint string) (*Mount, error) {
 	mnt, err := FindMount(mountpoint)
 	if err != nil {
-		return nil, errors.Wrap(ErrNotAMountpoint, mountpoint)
+		return nil, &ErrNotAMountpoint{mountpoint}
 	}
 	// Check whether 'mountpoint' names the same directory as 'mnt.Path'.
 	// Use os.SameFile() (i.e., compare inode numbers) rather than compare
@@ -394,7 +394,7 @@ func GetMount(mountpoint string) (*Mount, error) {
 		return nil, err
 	}
 	if !os.SameFile(fi1, fi2) {
-		return nil, errors.Wrap(ErrNotAMountpoint, mountpoint)
+		return nil, &ErrNotAMountpoint{mountpoint}
 	}
 	return mnt, nil
 }
@@ -410,22 +410,22 @@ func getMountFromLink(link string) (*Mount, error) {
 	link = strings.TrimSpace(link)
 	linkComponents := strings.Split(link, "=")
 	if len(linkComponents) != 2 {
-		return nil, errors.Wrapf(ErrFollowLink, "link %q format is invalid", link)
+		return nil, &ErrFollowLink{link, errors.New("invalid link format")}
 	}
 	token := linkComponents[0]
 	value := linkComponents[1]
 	if token != uuidToken {
-		return nil, errors.Wrapf(ErrFollowLink, "token type %q not supported", token)
+		return nil, &ErrFollowLink{link, errors.Errorf("token type %q not supported", token)}
 	}
 
 	// See if UUID points to an existing device
 	searchPath := filepath.Join(uuidDirectory, value)
 	if filepath.Base(searchPath) != value {
-		return nil, errors.Wrapf(ErrFollowLink, "value %q is not a UUID", value)
+		return nil, &ErrFollowLink{link, errors.Errorf("invalid UUID format %q", value)}
 	}
 	deviceNumber, err := getDeviceNumber(searchPath)
 	if err != nil {
-		return nil, errors.Wrapf(ErrFollowLink, "no device with UUID %q", value)
+		return nil, &ErrFollowLink{link, errors.Errorf("no device with UUID %s", value)}
 	}
 
 	// Lookup mountpoints for device in global store
@@ -436,11 +436,11 @@ func getMountFromLink(link string) (*Mount, error) {
 	}
 	mnt, ok := mountsByDevice[deviceNumber]
 	if !ok {
-		return nil, errors.Wrapf(ErrFollowLink, "no mounts for device %q (%v)",
-			getDeviceName(deviceNumber), deviceNumber)
+		return nil, &ErrFollowLink{link, errors.Errorf("no mounts for device %q (%v)",
+			getDeviceName(deviceNumber), deviceNumber)}
 	}
 	if mnt == nil {
-		return nil, filesystemLacksMainMountError(deviceNumber)
+		return nil, &ErrFollowLink{link, filesystemLacksMainMountError(deviceNumber)}
 	}
 	return mnt, nil
 }
@@ -450,12 +450,12 @@ func getMountFromLink(link string) (*Mount, error) {
 // error is returned if the mount has no device, or no UUID.
 func makeLink(mnt *Mount, token string) (string, error) {
 	if token != uuidToken {
-		return "", errors.Wrapf(ErrMakeLink, "token type %q not supported", token)
+		return "", &ErrMakeLink{mnt, errors.Errorf("token type %q not supported", token)}
 	}
 
 	dirContents, err := ioutil.ReadDir(uuidDirectory)
 	if err != nil {
-		return "", errors.Wrap(ErrMakeLink, err.Error())
+		return "", &ErrMakeLink{mnt, err}
 	}
 	for _, fileInfo := range dirContents {
 		if fileInfo.Mode()&os.ModeSymlink == 0 {
@@ -471,6 +471,6 @@ func makeLink(mnt *Mount, token string) (string, error) {
 			return fmt.Sprintf("%s=%s", uuidToken, uuid), nil
 		}
 	}
-	return "", errors.Wrapf(ErrMakeLink, "device %q (%v) has no UUID",
-		mnt.Device, mnt.DeviceNumber)
+	return "", &ErrMakeLink{mnt, errors.Errorf("cannot determine UUID of device %q (%v)",
+		mnt.Device, mnt.DeviceNumber)}
 }
-- 
cgit v1.2.3


From 197eb371697aff066947372d10732387454fd88a Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Sat, 9 May 2020 14:52:07 -0700
Subject: cmd/fscrypt: remove ErrMaxPassphrase

This isn't actually a valid error since crypto.NewKeyFromReader()
handles re-allocating the buffer to a larger size if it fills up.
---
 cmd/fscrypt/errors.go | 1 -
 cmd/fscrypt/keys.go   | 6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index ef4ae64..4ce4504 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -47,7 +47,6 @@ const failureExitCode = 1
 var (
 	ErrCanceled           = errors.New("operation canceled")
 	ErrNoDestructiveOps   = errors.New("operation would be destructive")
-	ErrMaxPassphrase      = util.SystemError("max passphrase length exceeded")
 	ErrInvalidSource      = errors.New("invalid source type")
 	ErrPassphraseMismatch = errors.New("entered passphrases do not match")
 	ErrSpecifyProtector   = errors.New("multiple protectors available")
diff --git a/cmd/fscrypt/keys.go b/cmd/fscrypt/keys.go
index 872ca2a..77e3900 100644
--- a/cmd/fscrypt/keys.go
+++ b/cmd/fscrypt/keys.go
@@ -55,14 +55,14 @@ var (
 // struct is empty as the reader needs to maintain no internal state.
 type passphraseReader struct{}
 
-// Read gets input from the terminal until a newline is encountered. This read
-// should be called with the maximum buffer size for the passphrase.
+// Read gets input from the terminal until a newline is encountered or the given
+// buffer is full.
 func (p passphraseReader) Read(buf []byte) (int, error) {
 	// We read one byte at a time to handle backspaces
 	position := 0
 	for {
 		if position == len(buf) {
-			return position, ErrMaxPassphrase
+			return position, nil
 		}
 		if _, err := io.ReadFull(os.Stdin, buf[position:position+1]); err != nil {
 			return position, err
-- 
cgit v1.2.3


From 181600d6327ed34a3f62eda0dd03a6d2ae49e5f9 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Sat, 9 May 2020 14:52:07 -0700
Subject: 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.
---
 cli-tests/t_encrypt.out   | 21 +++++++++---
 cli-tests/t_lock.out      | 22 +++++++-----
 cli-tests/t_setup.out     |  2 +-
 cli-tests/t_v1_policy.out | 13 +++++---
 cmd/fscrypt/commands.go   | 51 +++++++++++++++-------------
 cmd/fscrypt/errors.go     | 85 +++++++++++++++++++++++++++++++++++++----------
 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
-- 
cgit v1.2.3