[Innovation-Sprint/PM-19065] Add opaque authentication / unlock #185
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #185 +/- ##
==========================================
+ Coverage 66.31% 66.40% +0.08%
==========================================
Files 199 204 +5
Lines 15609 16130 +521
==========================================
+ Hits 10351 10711 +360
- Misses 5258 5419 +161 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job, no security vulnerabilities found in this Pull Request |
quexten
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
quexten
changed the title
|
| password: &str, | ||
| ) -> Result<types::ClientRegistrationStartResult, Error> { | ||
| let result = | ||
| ClientRegistration::<Self>::start(&mut rand::thread_rng(), password.as_bytes())?; |
There was a problem hiding this comment.
rand::thread_rng() is correct? Most examples I had seen were using opaque_ke::rand::rngs::OsRng.
There was a problem hiding this comment.
I think this is OK because:
- We use thread_rng everywhere in the SDK, we don't use OsRng
- ThreadRng implements the CryptoRng marker trait, and is seeded via OsRng
- opaque-ke expects a
CryptoRng + RngCore, which bothOsRngbut alsoThreadRngfulfill.
I think the main difference to OsRng is speed, but I'll leave this open to @dani-garcia to comment on.
There was a problem hiding this comment.
Yeah, OsRng is reading directly from the OS entropy source, while ThreadRng just seeds a PRNG with the OS entropy.
The rand docs qualify it as ... good compromise between cryptographic security, fast generation ... https://github.com/rust-random/rand/blob/master/SECURITY.md#specific-generators
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { |
There was a problem hiding this comment.
Should we add any unhappy path tests? My thinking is the error that we return on a
wrong password should be well documented and tested so that we can handle that on the client side and show a nice error.
| /// The client side, serialized state of the registration process. | ||
| /// MUST NOT BE SHARED WITH THE SERVER and must be cleared from memory after registration | ||
| /// finish. | ||
| pub state: ByteBuf, |
There was a problem hiding this comment.
Just curious, what does ByteBuf become on the TS side? Is it special cased to become ArrayBuffer, UInt8Array, or does it copy the layout of this type? Is it actually easy to clear from memory?
There was a problem hiding this comment.
As far as I understand, this is the only way to get Uint8array in TS directly in tsify-next. Otherwise you get number[], which has to be manually converted again.
There was a problem hiding this comment.
We generally encode these using b64 in other places. @dani-garcia not sure if that is more appropriate or if we revisit our practice. (I believe we b64 encode a few things for mobile at least)
There was a problem hiding this comment.
Yeah we've had complaints in the swift native apps when using bytes as they were hard to work with, something about equality not being implemented for the swift bytes type I think.
That said, using b64 is probably just as annoying and pushing the conversion work to the clients. I wonder if this just means we need to consider having our own Bytes type that does the right conversion for each platform.
There was a problem hiding this comment.
I assume this initiative has been green lit and will move forward hence the requests for reviews. And the code should be considered "production ready".
I don't have any knowledge of the OPAQUE protocol so this review is purely from a rust and SDK perspective.
The pull request description is fairly lightweight and contains little information about what this work entails. Could we expand it a bit or at least link to the appropriate confluence documents.
As far as I can tell this has little to do with cryptographic primitives and is more a new registration / login flow. If that's accurate we should consider lifting it out of the crypto crate which would also simplify ownership if we suspect everything to not be owned by KM.
There are a lot of public types and fields in this PR. Do all of them need pub visibility or can we restrict it? It's always easier to make more things public than to decrease visibility in the future.
There was a problem hiding this comment.
issue: These does not follow https://contributing.bitwarden.com/architecture/sdk/dependencies/#explicit-ranges
There was a problem hiding this comment.
question: There seems to be little reason to use the client at all here since it's just obfuscating direct calls to types?
This is related to my comment about CipherConfiguration leaking sensitive values.
| use tsify_next::Tsify; | ||
|
|
||
| #[derive(Serialize, Deserialize, Clone)] | ||
| #[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] |
There was a problem hiding this comment.
question: This leaks the sensitive values to the presentation layer. Would we prefer that these stay inside the rust world or that we pass a object back and forth?
| /// The client side, serialized state of the registration process. | ||
| /// MUST NOT BE SHARED WITH THE SERVER and must be cleared from memory after registration | ||
| /// finish. | ||
| pub state: ByteBuf, |
There was a problem hiding this comment.
We generally encode these using b64 in other places. @dani-garcia not sure if that is more appropriate or if we revisit our practice. (I believe we b64 encode a few things for mobile at least)
| #[error("Error creating message {0}")] | ||
| Message(String), | ||
| #[error("Error deserializing message")] | ||
| Deserialize, |
| /// be accessed by having access to the upstream key. | ||
| #[derive(Deserialize, Serialize)] | ||
| #[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] | ||
| pub struct RotateableKeyset { |
There was a problem hiding this comment.
issue: Should probably be called RotatableKeyset?
Consider installing code spell checker and resolving any unknown words. And adding any appropriate to the project word list.
| encapsulating_key_material: &[u8], | ||
| encapsulated_key: &SymmetricCryptoKey, | ||
| ) -> Result<Self, CryptoError> { | ||
| let key = if encapsulating_key_material.len() == 64 { |
There was a problem hiding this comment.
question: Is there a reason we verify the length here? What are we attempting to protect against? (add a comment?)
I think it's more appropriate to use a guard statement here
if encapsulating_key_material.len() != 64 {
return Err(CryptoError::InvalidKey);
}
let key = SymmetricCryptoKey::try_from(encapsulating_key_material.to_vec())?;|
|
||
| /// Create a new rotateable keyset from an export key and an encapsulated key | ||
| /// Note: The export key must be 32 bytes | ||
| pub fn create_rotateablekeyset_from_exportkey( | ||
| &self, | ||
| export_key: &mut [u8], | ||
| encapsulated_key: &SymmetricCryptoKey, | ||
| ) -> Result<bitwarden_crypto::rotateable_keyset::RotateableKeyset, CryptoError> { | ||
| bitwarden_crypto::rotateable_keyset::RotateableKeyset::new(export_key, encapsulated_key) | ||
| } | ||
|
|
||
| pub fn decapsulate_key_from_rotateablekeyset( | ||
| &self, | ||
| rotateable_keyset: bitwarden_crypto::rotateable_keyset::RotateableKeyset, | ||
| encapsulating_key: &[u8], | ||
| ) -> Result<SymmetricCryptoKey, CryptoError> { | ||
| rotateable_keyset.decapsulate_key(encapsulating_key) | ||
| } |
There was a problem hiding this comment.
@dani-garcia will these APIs work in the more limited mobile environment? I don't believe mobile can create a SymmetricCryptoKey for example.
There was a problem hiding this comment.
The exact API won't, but we can adapt them slightly in bitwarden_uniffi
The SymmetricCryptoKey in create_rotateablekeyset_from_exportkey would be coming from the ClientRegistrationFinishResult struct in the opaque_register_finish function. I imagine in the UniFFI layer we could pass the entire struct as an opaque pointer directly so the mobile apps won't have to deal with the key themselves.
For decapsulate_key_from_rotateablekeyset, the UniFFI function would just set the result key as the user key and return Ok(()), I think
| /// The client side, serialized state of the login process. | ||
| /// MUST NOT BE SHARED WITH THE SERVER and must be cleared from memory after registration | ||
| /// finish. | ||
| pub state: ByteBuf, |
There was a problem hiding this comment.
Sounds like this state should be encapsulated by the SDK and not leave it's boundary?
| /// if the cipher parameters have a larger key size, the key will be truncated. | ||
| /// This key is consistent between authentications. | ||
| pub export_key: ByteBuf, | ||
|
|
There was a problem hiding this comment.
nit: Inconsistent use of newline.
Cryptographic primitives here refer to the most basic building blocks of Opaque, as exposed to the outside - I.e entire, high level messages (from OPAQUES view). I realize that is leading to some confusion here, so I'll refrain from the word from now on. |
|
Setting back to draft for now. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-19065
📔 Objective
Adds functions to use OPAQUE-key exchange from the clients. Additionally, adds rotateable keyset to the SDK.
Tests are provided against a local "mockserver" implementation of the server.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes