ref(service): Generalize encryption util - #615
Conversation
Move authenticated encryption into a reusable Cipher with method-level serde generics while preserving the existing envelope format. BREAKING CHANGE: Rename service.resumable_token_encryption to service.encryption.
This comment has been minimized.
This comment has been minimized.
lcian
changed the title
lcian
changed the title
This comment has been minimized.
This comment has been minimized.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 28141dd. Configure here.
| /// | ||
| /// Returns `None` if the envelope is malformed, cannot be authenticated, or does not contain | ||
| /// the requested type. | ||
| pub(crate) fn decrypt<T>(&self, envelope: &[u8]) -> Option<T> |
There was a problem hiding this comment.
I'm not sure if this is right or if we should rather return a Result here.
Option can still be mapped externally and has the advantage of not exposing the source Error in case anything fails, which could provide some info to an attacker.
So perhaps Option is the right fit here.
There was a problem hiding this comment.
It is useful to keep the source error here for internal error tracking and rather restrict sources in the public API.
| /// persistent keyring for sessions that must survive restarts. Keep old keys configured while | ||
| /// their sessions may still be active; removing a key intentionally invalidates those sessions. | ||
| /// Values must be raw AES-256 key bytes. | ||
| /// This is used to instantiate [`Cipher`] which can encrypt/decrypt arbitrary payloads for |
There was a problem hiding this comment.
nit: This should better be directed at an operator rather than engineer, hence describe what functionality this is used for and how it changes objectstore's behavior, rather than the code-level constructs.
We can put relevant code comments onto the cipher method.
| /// Currently, this is only used for session tokens of the Resumable Uploads API. | ||
| /// | ||
| /// When this config is absent, Objectstore generates a fresh in-memory key at startup. | ||
| /// This might not work for requests that need to survive a deployment or multi-instance |
There was a problem hiding this comment.
This will not work for certain.
There was a problem hiding this comment.
For resumable uploads it will not work, for other things that we use this in the future, it might...
| /// | ||
| /// Returns `None` if the envelope is malformed, cannot be authenticated, or does not contain | ||
| /// the requested type. | ||
| pub(crate) fn decrypt<T>(&self, envelope: &[u8]) -> Option<T> |
There was a problem hiding this comment.
It is useful to keep the source error here for internal error tracking and rather restrict sources in the public API.
| /// File-backed secrets should use `${file:PATH}` so they are loaded during configuration | ||
| /// deserialization. | ||
| #[serde(default)] | ||
| pub keys: BTreeMap<String, Bytes>, |
There was a problem hiding this comment.
It's nice that this works with Bytes, but if we need exactly 32 raw key bytes we can:
- Implement our own newtype that performs the deserialization
- Consider to store the bytes inline as
[u8; 32].
2 participants
Authenticated encryption is now moved from
resumable.rsto a reusableobjectstore-service::encryption::Cipherstruct whoseencryptanddecryptmethods work with any serde type.resumable_token_encryptionhas been renamed tocipher/encryptionthroughout, so configuration now happens as follows: