Improve error messages and suggestions #219
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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.
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.
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.
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.
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.
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.
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.
This isn't actually a valid error since crypto.NewKeyFromReader() handles re-allocating the buffer to a larger size if it fills up.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Lots of improvements to
fscrypterror handling, e.g. using custom error types that allow us to pass more information up to the CLI tool to give better suggestions. Also fix lots of cases where errors were being wrapped "backwards". See the individual commits for details.This pull request is on top of #217 and #218.