[PM-24127] Implement password-protected key envelope #335
Conversation
…ernal into km/cose-content-format
There was a problem hiding this comment.
question: Are we planning on implementing more safe interfaces, and what decides if something is safe or not? Is everything else in the care unsafe? Such as decrypting EncStrings?
I suppose the goal is to ask people to use the higher level abstractions here rather than rolling their own using primitives?
There was a problem hiding this comment.
Yeah, the goal here is higher-level abstractions with the general goals of:
- Safe to use
- Easy mental model (high level)
- Hard to misuse
Though, I don't think we have exact criteria defined yet.
Each of these should have an easy to understand usage story associated with it. "I want to protect a key, and have a password. That is done with a PasswordProtectedKeyEnvelope".
The next safe interface I thought about proposing - it is still draft / RFC - is DataEnvelope: #336, with the usage story: "I have a struct of data and want to protect it. That is done with a DataEnvelope, and I receive back a sealed envelope and an envelope key".
There was a problem hiding this comment.
I think it's worth adding a reference to the safe module from the root readme in crypto? It might be hard for people to find it otherwise.
## High level abstractions
The crate provides high level abstractions under the `safe` namespace. These should be heavily prioritized over the lower level primitives when developing new functionality.
| // Alice wants to change her password; also her KDF settings are below the minimums. | ||
| // Re-sealing will update the password, and KDF settings. | ||
| let envelope = envelope | ||
| .reseal(pin, "0000") | ||
| .expect("The password should be valid"); | ||
| disk.save( | ||
| "vault_key_envelope", | ||
| (&envelope).try_into().expect("Saving envelope should work"), | ||
| ); |
There was a problem hiding this comment.
question: Do you have a strategy for how to migrate infrequently changed keys? You could imagine a user setting up pin once and keeping it for years. In which time we may have upgraded the minimum kdf settings.
There was a problem hiding this comment.
For the pin-specific case this can be done by either:
- Using re-seal(pin) on unlock with the pin
- Using the
userkey_encrypted_pin
I'm not sure whether the FFI interface should force this migration by returning an optional update version of the envelope, or whether this should be a manual process called by each client.
For master-password, we would only be able to use re-seal(master-password) on login/unlock with MP, since we don't store userkey_encrypted_master_password.
There was a problem hiding this comment.
Since this is a new interface now would be the optimal time to decide how it should be handled as the scope to introduce this later will be significantly larger. But it's also completely valid to postpone it.
Ideally there would be a pattern that forces the consumer to handle it, since it's often easy to miss. Throwing an error on initialization and forcing them through an upgrade path before being allowed to consume maybe. But this could also easily be missed as we probably don't have a good way to test
Going Vec to a wrapper, enum { Envelope(PaasswordProtectedKeyenvelope), UpgradeRequired } maybe?
There was a problem hiding this comment.
💭
I would think we want whatever owns the storage layer for the envelopes to own being responsible for migrating these. At the moment, that would be the clients but could be the SDK in the future. enum { Envelope(PaasswordProtectedKeyenvelope), UpgradeRequired } reminds me of the ASP.NET Core Identity's PasswordHasher and makes sense to me since it is flexible to storage ownership changes.
For TS clients, KM could just own a storage service layer unsealFromStorage(pin) and that would handle saving a new reseal if UpgradeRequired on use.
@quexten would this be a big increase in scope?
There was a problem hiding this comment.
I think we should look into this whenever we also want to take on the work to manage PINs with SDK state management, especially since it is otherwise tricky to make forced upgrades work nicely with crypto initialization of the SDK. And the scope increase to do that here would be significant.
| Self::seal_ref_with_settings( | ||
| key_to_seal, | ||
| password, | ||
| &Argon2RawSettings::default_for_platform(), |
There was a problem hiding this comment.
question: Would we ever want to apply a more restrictive default if we get say a numeric low digit pin? Like bumping up the iteration count to slightly slow down brute force attacks.
There was a problem hiding this comment.
I think that makes sense. The argon2 RFC [1] has a procedure specified on how to pick argon2 settings. They do also specify a method where you first pick a fixed memory setting that is uniformly safe, then auto-adjust the iteration count based on acceptable unlock time. We could implement this fairly easily.
However, only applying it on weak passwords would leak that the password is a weak password by looking at the envelope's KDF settings. I'm not sure if that's something we are concerned about.
There was a problem hiding this comment.
Hmm true, you could let the caller decide if it's "high probability of low entropy" like pins since I suspect many users use pin when they have complex MP. But I'm good marking this as out of scope, mostly wanted you to think of this scenario.
crates/bitwarden-crypto/src/safe/password_protected_key_envelope.rs
Outdated
Show resolved
Hide resolved
crates/bitwarden-crypto/src/safe/password_protected_key_envelope.rs
Outdated
Show resolved
Hide resolved
| impl Argon2RawSettings { | ||
| /// Creates default Argon2 settings based on the platform. This currently is a static preset | ||
| /// based on the target os | ||
| fn default_for_platform() -> Self { | ||
| // iOS has memory limitations in the auto-fill context. So, the memory is halved | ||
| // but the iterations are doubled | ||
| if cfg!(target_os = "ios") { | ||
| // The SECOND RECOMMENDED option from: https://datatracker.ietf.org/doc/rfc9106/, with halved memory and doubled iteration count | ||
| Self { | ||
| iterations: 6, | ||
| memory: 32 * 1024, // 32 MiB | ||
| parallelism: 4, | ||
| salt: make_salt(), | ||
| } | ||
| } else { | ||
| // The SECOND RECOMMENDED option from: https://datatracker.ietf.org/doc/rfc9106/ | ||
| // The FIRST RECOMMENDED option currently still has too much memory consumption for most | ||
| // clients except desktop. | ||
| Self { | ||
| iterations: 3, | ||
| memory: 64 * 1024, // 64 MiB | ||
| parallelism: 4, | ||
| salt: make_salt(), | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
question: Does the use of platform specific KDF settings imply these should be device bound and not be synced? I could see a desire to share PasswordProtected envelopes.
suggestion: Highlight this to consumers to prevent any potential misuse. Or ideally name them local or device bound.
There was a problem hiding this comment.
Renamed.
As for the question, its' fine to sync these, but syncing these has a few implications:
-
If the target device cannot handle the settings, this may cause issues (such as we are currently still seeing with high argon2 memory + iOS autofill) and differing unsealing times
-
Differing unsealing times may be OK, if it is either short on all devices, or slow on some but is also an infrequent action (login)
These are not different from the currently shared master-key-encrypted-userkey, which has the same constraints.
There was a problem hiding this comment.
The difference with user key is that the user picks the values. This interface does not allow you to customize it by design and therefore if someone creates a value outside of iOS and it's synced it would be the same as not having a lower limit for iOS in the first place.
There was a problem hiding this comment.
I think for the synced vs local use-case, we should just have two separate functions. Maybe the synced case would be seal_with_custom_kdf, where the caller provides the KDF setting.
Since we don't use this for the synced use-case currently, we don't implement that functionality yet.
There was a problem hiding this comment.
What about including a Purpose enum when you create the envelope? That would make it clear for consumers you shouldn't sync this key yet. But I don't have a strong preference and this should be seen as non blocking.
enum Purpose {
Local,
// Synced TBD
}There was a problem hiding this comment.
Yeah, that looks very nice. I'll not add it in this PR, but will look into adding this as follow-up.
crates/bitwarden-crypto/src/safe/password_protected_key_envelope.rs
Outdated
Show resolved
Hide resolved
…b.com:bitwarden/sdk-internal into km/beeep/safe-password-protected-key-envelope
Co-authored-by: Oscar Hinton <[email protected]>
|
Noting here: I lifted in a fair bit of non-pin-specific changes from the follow-up PR to this PR since the last set of reviews. |
|
There was a problem hiding this comment.
My primary concerns are resolved. I'd still like to see some elegant way to force upgrades as adding it later will be significantly more complicated. And a note to the root readme to look in safe before implementing anything.
But I'll leave it up to the team to decide if it happens now or when the need arises.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-24127
📔 Objective
The current masterkey logic is complex to understand for using teams (auth), and also prone to error. When any setting changes / gets out of sync, such as the email, or kdf, then decryption fails. The masterkey is further too widely scoped, used both in an authentication protocol, and in unlock decryption.
This PR introduces a PasswordProtectedKeyEnvelope. The goal is to protect a symmetric key with a password securely. Internally, this uses a KDF, and the KDF settings (argon2 parameters, and random salt) are stored on the serialized object.
That means that the only thing needed to unlock this structure is the correct password, everything else is stored on the object, making this process much less error prone. At the same time the interface is easier to use.
An example is provided to show usage.
A follow-up PR will add an unlock method / enrollment for PIN based on this new cryptographic API.
Note: Only argon2 is supported here. The PasswordProtectedKeyEnvelope's settings are completely decoupled from the account settings, and we don't need to provide backwards compatibility to non-recommended legacy cryptographic algorithms (pbkdf2).
⏰ 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