Transit BYOK export capabilities #20736
Conversation
cipherboy
force-pushed
the
cipherboy-transit-byok
branch
2 times, most recently
from
3808ad2 to
ad64780
Compare
This allows one keysutil to wrap another key, assuming that key has an type matching one of keysutil's allowed KeyTypes. This allows completing the BYOK import loop with Transit, allowing imported wrapping keys to export (wrap) other keys in transit, without having them leave in plaintext. Signed-off-by: Alexander Scheel <[email protected]>
Still respecting exportable, we allow encrypted-only export of transit keys to another cluster using the BYOK semantics. In particular, this allows an operator to securely establish key material between two separate Transit installations. This potentially allows one cluster to be used as a source cluster (encrypting a large amount of data) and a second cluster to decrypt this data later. This might be useful in hybrid or site-specific deployments of Vault for instance. Signed-off-by: Alexander Scheel <[email protected]>
Also updates to a newer version while we're here. Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
cipherboy
force-pushed
the
cipherboy-transit-byok
branch
from
a46af8f to
69f46bc
Compare
| @@ -41,6 +42,8 @@ import ( | |||
| "github.com/hashicorp/vault/sdk/helper/jsonutil" | |||
| "github.com/hashicorp/vault/sdk/helper/kdf" | |||
| "github.com/hashicorp/vault/sdk/logical" | |||
|
|
|||
| "github.com/google/tink/go/kwp/subtle" | |||
There was a problem hiding this comment.
this is a nice way to get forward secrecy, that's really cool
There was a problem hiding this comment.
tink isn't listed within the SDK's go.mod, which is technically should be if we are using it here?
There was a problem hiding this comment.
I think 812395c would've done so, but I might be wrong?
There was a problem hiding this comment.
Sorry about that, I see you added them within this PR. Somehow they merged in my head.
|
|
||
| // validationFunc verifies the ciphertext, signature or hmac based on the | ||
| // set 'feature' | ||
| validationFunc := func(keyName string) { |
There was a problem hiding this comment.
is there a reason this isn't a function external to this call?
There was a problem hiding this comment.
I think it uses variables from above and this follows the existing pattern of tests above.
There was a problem hiding this comment.
Er, and by above, I mean path_backup_test.go lol :-D
| return nil, fmt.Errorf("no such source key for export") | ||
| } | ||
| if !b.System().CachingDisabled() { | ||
| srcP.Lock(false) |
There was a problem hiding this comment.
We have a potential deadlock here, but I don't think there is much we can do about it and I don't think it's realistic people would call this with inverted params in parallel.
There was a problem hiding this comment.
Yeah, agreed. In general, I think it'd be hard to do inverted semantics unless you were using non-Transit BYOK importing. Given dest is a Transit key, it lacks a private counterpart, and so cannot be exported...
Maybe we delay this !b.System().CachingDisabled() check and first check that the src key has private counterparts, but I don't think that'll help the default case...
There was a problem hiding this comment.
Yeah and if caching is enabled disabled the lock is acquired within the b.GetPolicy so we are still hosed.
There was a problem hiding this comment.
LGTM, might want to get @schultz-is to have a look through as well.
Co-authored-by: Matt Schultz <[email protected]>
This still needs tests, but this adds the bulk of the work to support BYOK exporting of keys, now that #17934 has landed.
In particular, the combination of the two features allows an operator to securely establish key material between two separate Transit installations. This potentially allows one cluster to be used as a source cluster (encrypting a large amount of data) and a second cluster to decrypt this data later. This might be useful in hybrid or site-specific deployments of Vault for instance.
I want to make additional changes to
bump_versionbefore release, and wanted a way of testing export + import of keys easily, plus wanted to enable this particular use case.In the future, it might be nice to move
UnwrapKeyinto the shared namespace as well, now thattinkis added as a formal dependency.TODO in this PR: