feat(encryption) [3/N] Support encryption: KMS#2339
feat(encryption) [3/N] Support encryption: KMS#2339xanderbailey wants to merge 8 commits intoapache:mainfrom
Conversation
| /// If `true`, callers can use [`generate_key`](Self::generate_key) for atomic | ||
| /// key generation and wrapping, which is more secure than generating a key | ||
| /// locally and then wrapping it. | ||
| fn supports_key_generation(&self) -> bool { |
There was a problem hiding this comment.
A note on the supports_key_generation() + generate_key() pattern: this mirrors Java's interface. The supports_key_generation bool is a runtime capability check. Not sure if this is very idiomatic, I kept the Java pattern here since the EncryptionManager (coming in a later PR) needs to branch on this to decide between kms.generate_key() (atomic server-side generation, e.g. AWS KMS GenerateDataKey) vs generating locally + kms.wrap_key(). Open to feedback on whether we should do something else here?
There was a problem hiding this comment.
I think it's fine to use this pattern. But I think we don't need to provide default implementation? Java provides default implementation to avoid breaking compatibility, we don't need this.
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Mostly LGTM, only a naming question.
| /// | ||
| /// Returned by [`KeyManagementClient::generate_key`] when the KMS supports | ||
| /// atomic key generation and wrapping. | ||
| pub struct KeyGenerationResult { |
There was a problem hiding this comment.
Result is so widely used in rust. It can be a bit weired to read Result<KeyGenerationResult>
How about finding a better name?
(I'm not strong on this)
There was a problem hiding this comment.
That's a very fair point, have changed to GeneratedKey? I think that's better maybe?
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks @xanderbailey, the trait design is clean and the InMemoryKeyManagementClient is a solid test utility.
One minor nit: the key_size field on InMemoryKeyManagementClient controls the size of randomly generated master keys, but could be misread as controlling wrap/unwrap behavior. Since this is a testing utility that people will read as example code, something like master_key_size might be clearer about what it governs.
|
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @xanderbailey for this pr! Generally LGTM, just left some commets to resolve.
| #[async_trait] | ||
| pub trait KeyManagementClient: Send + Sync + std::fmt::Debug { | ||
| /// Wrap (encrypt) a key using a wrapping key managed by the KMS. | ||
| async fn wrap_key(&self, key: &[u8], wrapping_key_id: &str) -> Result<Vec<u8>>; |
There was a problem hiding this comment.
Should this also be SensitiveBytes?
There was a problem hiding this comment.
The wrapped key is cipher text so I think this is fine.
| /// If `true`, callers can use [`generate_key`](Self::generate_key) for atomic | ||
| /// key generation and wrapping, which is more secure than generating a key | ||
| /// locally and then wrapping it. | ||
| fn supports_key_generation(&self) -> bool { |
There was a problem hiding this comment.
I think it's fine to use this pattern. But I think we don't need to provide default implementation? Java provides default implementation to avoid breaking compatibility, we don't need this.
| /// | ||
| /// Returned by [`KeyManagementClient::generate_key`] when the KMS supports | ||
| /// atomic key generation and wrapping. | ||
| pub struct GeneratedKey { |
There was a problem hiding this comment.
We should avoid pub field pattern.
| /// # } | ||
| /// ``` | ||
| #[derive(Clone)] | ||
| pub struct InMemoryKeyManagementClient { |
There was a problem hiding this comment.
| pub struct InMemoryKeyManagementClient { | |
| pub struct MemoryKeyManagementClient { |
Following conventions in other structs.
|
|
||
| impl InMemoryKeyManagementClient { | ||
| /// Creates a new in-memory KMS with 128-bit AES keys. | ||
| pub fn new() -> Self { |
There was a problem hiding this comment.
We could derive default if we make AesKeySize::Bits128 default?
There was a problem hiding this comment.
4 participants
Which issue does this PR close?
Part of #2034
What changes are included in this PR?
Adds the
KeyManagementClienttrait and an in-memory implementation for testing.KeyManagementClienttrait mirrors Java'sKeyManagementClientinterface.InMemoryKeyManagementClientfor testing-only KMS that stores master keys in memory to wrap/unwrap. Supports configurable key sizes and explicit key bytes for cross-language interop tests.Are these changes tested?
Tests covered for
InMemoryKeyManagementClient