v2 encryption policy support #16
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
|
Note, I haven't added tests for the new functionality yet. I'll do that next, but @josephlr please let me know if you have any early comments. Also FYI, I plan to migrate |
ebiggers
force-pushed
the
v2-policy-support
branch
3 times, most recently
from
3b5e089
to
da56824
Compare
This is useful for developers using ctags or cscope.
Move the keyutils definitions to a header keyutils.h so that they don't clutter up fscryptctl.c. The Makefile doesn't know to pick up new headers as dependencies, so fix it to do so.
Add a macro ARRAY_SIZE() like the Linux kernel has, so that we don't have to hard-code array sizes. Also add two missing static keywords.
It is best practice to always use O_CLOEXEC.
FS_IOC_SET_ENCRYPTION_POLICY will fail with ENOTDIR on a non-directory anyway, so we don't need to bother with O_DIRECTORY.
Dispatch the commands via a table so that we don't need to hard-code a series of string comparisons.
Instead of hard-coding the fscrypt UAPI definitions in fscryptctl.c, just add and use the latest fscrypt.h UAPI header from the Linux kernel. This provides a lot of new definitions, which later patches will use.
Various FS_* constants have been renamed to FSCRYPT_* in the latest UAPI header (though there are still compatibility definitions). Also, fscrypt_policy is now fscrypt_policy_v1. Upgrade to using the new names.
Update the list of available encryption modes: - Add the Adiantum encryption mode (supported since Linux v5.0). - Remove AES-256-GCM and AES-256-CBC, since the kernel has never actually suported those, and their constants have been re-reserved. - Stop using the special value "FSCRYPT_MODE_INVALID"; it's not needed.
It's not worthwhile bothering to work around FS_IOC_GET_ENCRYPTION_POLICY returning a different error code for "not encrypted" in old kernels, since most users of fscryptctl won't care, and also because the fix has been backported to the 4.9 LTS kernels.
Always recognize the standard "--" argument to indicate "end of options". Otherwise fscryptctl can't operate on files that are literally named "-h", "-v", "--help", or "--version". Recognize "--" even for commands that don't currently take options, and moreover, reject unknown options in such commands. Otherwise the fscryptctl commands work inconsistently, and later adding options to commands that don't currently take options would be a breaking change.
get_policy uses the FS_IOC_GET_ENCRYPTION_POLICY ioctl to retrieve the specified file or directory's encryption policy, then displays it. But that only works for v1 encryption policies. Linux 5.4 added a new ioctl FS_IOC_GET_ENCRYPTION_POLICY_EX which is more flexible and can retrieve both v1 and v2 encryption policies. Make get_policy use the new ioctl if the kernel supports it and display the resulting the v1 or v2 encryption policy. Otherwise, fall back to the old ioctl and display the v1 policy. Also adjust the output for v1 policies slightly.
Extend the set_policy command to support setting v2 encryption policies, in addition to v1 encryption policies which it currently supports. This uses the same ioctl FS_IOC_SET_ENCRYPTION_POLICY, where the 'version' field at the beginning of the struct is used to determine whether the struct is fscrypt_policy_v1 or fscrypt_policy_v2. The command sets a v2 policy when the user gives the longer key specification used in such policies (a 32 hex-char master_key_identifier rather than a 16 hex-char master_key_descriptor). Also add support for specifying the DIRECT_KEY, IV_INO_LBLK_64, and IV_INO_LBLK_32 flags in the policy. These are mostly for v2 policies, though DIRECT_KEY can technically be used with v1 too.
Split policy_error() into describe_get_policy_error() and
describe_set_policy_error() so that the error codes for
FS_IOC_GET_ENCRYPTION_POLICY{,_EX} and FS_IOC_SET_ENCRYPTION_POLICY
aren't mixed up.
Move the ENOTTY and EOPNOTSUPP handling into a new function
describe_fscrypt_error(), which will be useful for the other fscrypt
ioctls too.
Add a command which wraps the FS_IOC_ADD_ENCRYPTION_KEY ioctl. This is the new way to add a fscrypt key to the kernel, and v2 encryption policies require it. Although FS_IOC_ADD_ENCRYPTION_KEY also supports adding keys for v1 policies, that works a bit differently from adding keys for v2 policies. For simplicity, don't support adding v1 policy keys in add_key yet. People can use the legacy insert_key command if they want to continue to use fscryptctl with v1 policies. Also for simplicity, don't support using the fscrypt_add_key_arg::key_id field yet. For now the raw key is always required on stdin, like it was with insert_key.
Add a command which wraps the FS_IOC_REMOVE_ENCRYPTION_KEY ioctl (or FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS). This basically undoes the effect of FS_IOC_ADD_ENCRYPTION_KEY (i.e. 'fscryptctl add_key'). The exact semantics are a bit complicated, though; see the kernel documentation for details.
Add a command which wraps the FS_IOC_GET_ENCRYPTION_KEY_STATUS ioctl. This gets the status of an encryption key on a mounted filesystem.
ebiggers
force-pushed
the
v2-policy-support
branch
from
da56824
to
a232365
Compare
|
I've updated the tests and switched to GitHub Actions now. I might make a few more updates, but I think this pull request is mostly complete now. |
|
@josephlr please let me know if you're planning to review this -- thanks! |
ebiggers
force-pushed
the
v2-policy-support
branch
5 times, most recently
from
5c6576a
to
c3540e6
Compare
|
Ping. |
ebiggers
force-pushed
the
v2-policy-support
branch
2 times, most recently
from
5e8a2f9
to
2abb15b
Compare
josephlr
approved these changes
Feb 2, 2021
There was a problem hiding this comment.
Thanks soooooo much for doing all of this. I added some additional changes on top of your patchset. Mostly minor stuff, let me know what you think. It's mostly:
- Formatting nits
- Changing how
VERSIONis handled - Fixing tests
- Making sure
-std=c99works
Merge this PR if you think they all look good (or feel free to change them if you don't).
| # the License. | ||
| # | ||
|
|
||
| name: CI |
There was a problem hiding this comment.
This doesn't seem to be running on this PR, do we need to merge this PR first? Or do we need to enable Github Actions somewhere else?
There was a problem hiding this comment.
I think it has to be merged first; that's what happened for fscrypt, right? I'm not sure why, though. I was able to test the actions by making a pull request into my own repository (https://github.com/ebiggers/fscryptctl/pull/1/checks), so that worked somehow.
There was a problem hiding this comment.
Sounds good, let's see what happens
josephlr
force-pushed
the
v2-policy-support
branch
from
911b474
to
70f3217
Compare
ebiggers
commented
Feb 2, 2021
ebiggers
commented
Feb 2, 2021
ebiggers
commented
Feb 2, 2021
Thanks for the review! Your changes generally look good, but I left some minor comments. Also, some of your changes are just fixing up mine. Should I go ahead and fold those in, so we keep a cleaner history? |
Ya, you should fold these in. Having a clearer history is nice. |
The get_descriptor and insert_key commands are only useful for v1 encryption policies, so mark them as deprecated, move them to a dedicated section of the source file, and update the help output. (The replacement for insert_key is add_key. The replacement for get_descriptor is also add_key, as it doesn't seem that useful to have the key identifier computation as a separate command. We could add a compute_identifier command later if it's needed, though.)
fscryptctl currently only supports 64-byte master keys. However the kernel supports shorter master keys if AES-256-XTS isn't being used. (And there's also a chance that at some point the kernel will be changed to allow 32-byte master keys even for AES-256-XTS, as 256 bits of entropy is enough.) So change fscryptctl to allow variable-length master keys. While doing this, clean up read_key() to use read() instead of fread() to avoid a temporary buffer that may not get cleared, improve some variable names, and change insert_logon_key() to return bool.
Rewrite the README to get it fully up-to-date. Describe the purpose of fscryptctl more accurately. Since it's an advanced tool, generally prefer to refer to the kernel documentation or fscrypt documentation instead of documenting everything yet again. Document the new commands, briefly explain the difference between v1 and v2 encryption policies insofar as fscryptctl is concerned, and update the examples to show using a v2 policy rather than v1.
Arrange the Makefile targets into sections so that the Makefile is easier to understand. Document all the overridable variables at the top, and add support for PREFIX and BINDIR. Also clean up a few other things such as removing the unnecessary NAME variable and the 'default' target.
- Add tests for all new functionality. That primarily includes v2 policy support in get_policy and set_policy; the new commands add_key, remove_key, and key_status; support for more encryption modes; and support for key sizes other than 64 bytes. - Update to Python 3, as Python 2 is no longer supported by python.org. - Change 'make test' to no longer require root. It now just accepts an empty directory on a filesystem that supports encryption. It no longer requires that it be a mountpoint, and it no longer unmount and remounts the filesystem. (Unmounting was used to test fully removing encryption keys for v1 policies, but that wasn't testing fscryptctl itself anyway, so having it in the fscryptctl test suite didn't add too much. And it's not needed for v2 policies.) - Change 'make test-setup' and 'make test-teardown' run sudo themselves, like they do in the Makefile for 'fscrypt', so that the user doesn't need to explicitly run them under 'sudo'. - Add 'make test-all' as shorthand for test-setup + test + test-teardown. - Support wrapping the fscryptctl binary with valgrind. - Bump the kernel requirement to run the tests up to 5.4 or later.
travis-ci.org is being shut down. Moreover, travis.yml isn't working with the updated tests, as the tests now require Python 3 and a newer Linux kernel version. Switch to GitHub Actions and add a new CI configuration file which provides much better test coverage.
- Allow VERSION to be defined outside the Makefile - Just modify CPPFLAGS instead of manually adding to make rule - Define default VERSION in the source file - Switch to using v1.2.3 style versions These changes (especially the last), make it easier to use an alternative build system (like bazel) while keeping the same behavior. Signed-off-by: Joe Richey <joerichey@google.com>
These functions generally don't exist on Linux, and even if they did, we would need to define __STDC_WANT_LIB_EXT1__ to get them. Signed-off-by: Joe Richey <joerichey@google.com>
This is necessary when compiling against a specific C version, as O_CLOEXEC was added in a later POSIX standard. We could technically do: #define _POSIX_C_SOURCE 200809L But defining _GNU_SOURCE seems easier Signed-off-by: Joe Richey <joerichey@google.com>
We only use C99 for this program, so we should say so and test for it. Signed-off-by: Joe Richey <joerichey@google.com>
Since not many people are using fscryptctl yet, and v1 encryption policies have some annoying limitations which people keep running into, and all major users of filesystem encryption have already switched to v2 by default, it's better to just drop the fscryptctl support for v1 policies and only support v2. This keeps things much simpler going forward, and reduces the chance that people misconfigure things. If anyone really needs to continue to use v1-encrypted directories with fscryptctl (despite the known limitations with key visibility and so on), they can just use an older version of fscryptctl.
ebiggers
force-pushed
the
v2-policy-support
branch
from
527405f
to
d2066cd
Compare
|
@josephlr: I've addressed my comments, folded some of your changes, and moved the commit removing v1 policy support back to the end and added some more information to that commit message. I also opened a test pull request at https://github.com/ebiggers/fscryptctl/pull/2/checks, and all the github actions are passing. Let me know if there's anything else you think needs to be done before I merge this -- thanks! |
|
@ebiggers this looks great!!! Thanks so much for all the work you did on this. |
|
Looks like the CI is working: https://github.com/google/fscryptctl/actions/runs/535285203 |
Closed
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.
Update
fscryptctlto support v2 encryption policies, which are supported by Linux 5.4 and later.This primarily involved updating the existing
get_policyandset_policycommands to support v2 policies, and adding new commandsadd_key,remove_key, andkey_statuswhich manage encryption keys in the new filesystem-level keyring.This is basically the
fscryptctlequivalent of google/fscrypt#148.I also cleaned up a bunch of other things.
See the individual commits for details.