zio_crypt: establish platform interface; rework common code to use it - #18884
Open
robn wants to merge 7 commits into
Open
zio_crypt: establish platform interface; rework common code to use it#18884robn wants to merge 7 commits into
robn wants to merge 7 commits into
Conversation
Member
Author
|
Just to give an idea of one of the many places we can go with this, here's the backend for userspace using OpenSSL instead of ICP: zio_crypt_os_openssl.c |
Sponsored-by: TrueNAS Signed-off-by: Rob Norris <rob.norris@truenas.com>
This makes it harder to work with zfs_uio_t internally ourselves, for no apparently good reason. If a caller has a const iovec/bvec that they want to wrap in a uio, its up to them to either cast away the const or copy the data as appropriate. As it is, there does not appear to be any places within OpenZFS that require such action. Sponsored-by: TrueNAS Signed-off-by: Rob Norris <rob.norris@truenas.com>
On FreeBSD, it's only used in debug output anyway; there's definitely no need for it to not just be a normal string in both. Sponsored-by: TrueNAS Signed-off-by: Rob Norris <rob.norris@truenas.com>
As we pull platform-specific things out of zio_crypt, we'll need somewhere to move things to. Sponsored-by: TrueNAS Signed-off-by: Rob Norris <rob.norris@truenas.com>
This lifts out and generalises the common parts of linux/zio_crypt and freebsd/zio_crypt into a common version, with a platform/backend-specific API for the not-common parts. The common parts here involve key management, data assembly for encrypt/decrypt, on-disk formats and so on - the "logic" side of the equation, which are subtle and definitely shouldn't be duplicated. The platform-specific parts meanwhile are mostly just glue to get in and out of the platform-provided cryptographic suite. To be clear - this is not a particular _good_ API, but further improvement requires changes in the logic code. Establishing a clear boundary will allow that change to be worked on more safely. In the header, we add platform-specific types for the two places where incompatible concepts leaked through from the implementations: - zio_crypt_session_t: binds a zio_crypt_key_t to the implementation such that a whole "instance" doesn't need to be initialised for every operation. Previously ICP's crypto_ctx_template_t or FreeBSD's freebsd_crypt_session_t. - zio_crypt_hmac_t: stackable context for HMAC state during generation. Previously ICP's crypto_context_t or FreeBSD's struct hmac_ctx. This was internal to platform zio_crypt before. The most awkward part of this change is related to use of UIOs (though its not less awkward than before, just more visible). Both the ICP and the FreeBSD "crypto_os" shim expect data for encrypt/decrypt in a UIO, but the data layouts are different, which is where much of the difference between the two version of zio_crypt were. For now, to keep the change limited to only zio_crypt and not the underlying cryptosystems, we have the backend prepare the UIOs for encrypt/decrypt to use and simply tell the caller where to put the data. The backend is then free to organise other data before or after caller data as it pleases. The other difficult part is the complex set of arguments to zio_encrypt_os() and zio_decrypt_os(). This, again, is related to how they are called and their need to handle session re-keying. This appears to be the smallest API possible without structural changes in zio_crypt itself. Note that this commit keeps the existing platform zio_crypt.c implementations in tree to make it easier to compare them all with a three-way diff. The next commits will remove them and replace them with just the API implmentations. Sponsored-by: TrueNAS Signed-off-by: Rob Norris <rob.norris@truenas.com>
This is now a generic implementation for any platform that wants to use the ICP for its crypto. It exists in the "common" module source, linked and wired for Linux and libzpool. Sponsored-by: TrueNAS Signed-off-by: Rob Norris <rob.norris@truenas.com>
This uses our internal "crypto_os" shim as the backend to the FreeBSD kernel crypto. There's no particular reason this separate shim needs to exist anymore, but we keep it for now to keep this interface small. Sponsored-by: TrueNAS Signed-off-by: Rob Norris <rob.norris@truenas.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
[Sponsors: TrueNAS]
Motivation and Context
This lifts out and generalises the common parts of linux/zio_crypt and freebsd/zio_crypt into a common version, with a platform/backend-specific API for the not-common parts.
The common parts here involve key management, data assembly for encrypt/decrypt, on-disk formats and so on - the "logic" side of the equation, which are subtle and definitely shouldn't be duplicated. The platform-specific parts meanwhile are mostly just glue to get in and out of the platform-provided cryptographic suite.
To be clear - this is not a particular good API, but further improvement requires changes in the logic code. Establishing a clear boundary will allow that change to be worked on more safely in future PRs.
Description
First two commits are just filling out a bit of the UIO API and relaxing
constrequirements on Linux UIOs which don't exist and don't need to exist.Next two are setting up platform-specific
zio_crypt_os.h, and removing a cross-platform wart inzio_crypt.hthat is of no consequence but would just confuse the next commits.Next is the commit with summary matching this PR, which switches in a common
zio_crypt.cand defines an internal API for it to use to access the platform-specific cryptosystem. The commit message describes the change. The most important thing to note for review is that this commit does not remove the previous platform-specific implementations, leaving them in-tree for review via a 3-way diff, since all three versions have subtle differences in construction (but not in logic) to handle the different backends they target.The next two commits remove the existing implementations entirely, and switch over to small, tight implementations of just the internal platform API against the target cryptosystem: ICP on Linux & userspace, "crypto_os" on FreeBSD (our very weird shim over the kernel crypto).
How Has This Been Tested?
zfs_change-keyandzfs_createtest tags on Linux and FreeBSD during development; together they exercise most (all?) of thedsl_crypt->zio_cryptpaths.zdb -K ... -dddd ..., without issue.ztestruns, which exercises encrypted datasetsImportant to note that this should be mostly wiring, not actualy crypto stuff, so for the most part as long as everything round-trips correctly, it should be right.
Types of Changes
Checklist
Signed-off-by.