Use the new B64 in bitwarden-core and bitwarden-crypto #409
Conversation
| .map_err(|_| LoginError::InvalidOrganizationId)?; | ||
| let encryption_key = SymmetricCryptoKey::try_from(client_state.encryption_key)?; | ||
| let encryption_key = | ||
| SymmetricCryptoKey::try_from(client_state.encryption_key.to_string())?; |
There was a problem hiding this comment.
@quexten I think we want to implement From<B64> for SymmetricCryptoKey?
There was a problem hiding this comment.
I wonder how many more use-cases there are. If you are b64 encoding your unencrypted key, that to me indicates that the use-case is not supported long-term since keys should not leave the SDK, so we'd be making ergonomic improvements for paths we want to discourage.
Are there other areas we'd want to b64 encode a key in?
(Maybe: Never lock? Biometric unlock?)
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #409 +/- ##
==========================================
+ Coverage 76.10% 76.15% +0.04%
==========================================
Files 264 264
Lines 24045 24070 +25
==========================================
+ Hits 18299 18330 +31
+ Misses 5746 5740 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hinton
changed the title
| AccessTokenInvalidError::InvalidBase64Length { | ||
| expected: 16, | ||
| got: e.len(), | ||
| got: encryption_key.as_ref().len(), |
There was a problem hiding this comment.
I wonder if we should implement Deref<[u8]> for B64, then you could remove the as_ref():
let encryption_key = Zeroizing::new((*encryption_key).try_into().map_err(|_| {
AccessTokenInvalidError::InvalidBase64Length {
expected: 16,
got: encryption_key.len(),
}
})?);There was a problem hiding this comment.
Maybe. At the same time I like that we're explicit when we need to go B64 -> slice. In your suggestion there is an ambiguity of what len returns. The length of bytes or length of the b64 encoded string?
There was a problem hiding this comment.
True, but then as_ref is not a particularly descriptive trait either. If we wanted to be unambiguous I feel like we should use an as_slice function.
| private_key: STANDARD.encode(key.to_der()?), | ||
| public_key: b64, | ||
| private_key: key.to_der()?.as_ref().into(), | ||
| public_key: spki.as_ref().into(), |
There was a problem hiding this comment.
Do we want a direct B64 -> Bytes and Bytes -> B64 conversion? It should avoid the need to call as_ref as much
There was a problem hiding this comment.
I think that makes sense.
Note, for some instantiations of Bytes such as utf8, (maybe octet stream, pkcs8privatekey) we don't need / want the implementation, so we may add bounds and a marker trait for the variant that we allow the conversion for?
|
|
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-25121
📔 Objective
Migrates most usages of B64 into the new
B64struct.⏰ 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