Skip to content

Commit 7a1ae93

Browse files
quextenMGibson1Hintondani-garcia
authored
[PM-22845] Add account key rotation (#313)
## 🎟️ Tracking https://bitwarden.atlassian.net/browse/PM-22845 ## 📔 Objective The decrypted signing key is never available to Typescript, and we need key-rotation to re-encrypt the signing key. This PR implements a part of key-rotation in the crypto crate - specifically re-encrypting the private and signing key. Long-term, the key-rotation could should be ported at higher levels to the crypto crate. ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+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 confirmed issue 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 --------- Co-authored-by: Matt Gibson <[email protected]> Co-authored-by: Oscar Hinton <[email protected]> Co-authored-by: Daniel García <[email protected]>
1 parent d5c0dc5 commit 7a1ae93

File tree

11 files changed

+246
-24
lines changed

11 files changed

+246
-24
lines changed

crates/bitwarden-core/src/auth/auth_request.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use base64::{engine::general_purpose::STANDARD, Engine};
22
use bitwarden_crypto::{
33
fingerprint, generate_random_alphanumeric, AsymmetricCryptoKey, AsymmetricPublicCryptoKey,
4-
CryptoError, PublicKeyEncryptionAlgorithm, UnsignedSharedKey,
4+
CryptoError, PublicKeyEncryptionAlgorithm, SpkiPublicKeyBytes, UnsignedSharedKey,
55
};
66
#[cfg(feature = "internal")]
77
use bitwarden_crypto::{EncString, SymmetricCryptoKey};
@@ -91,7 +91,9 @@ pub(crate) fn approve_auth_request(
9191
client: &Client,
9292
public_key: String,
9393
) -> Result<UnsignedSharedKey, ApproveAuthRequestError> {
94-
let public_key = AsymmetricPublicCryptoKey::from_der(&STANDARD.decode(public_key)?)?;
94+
let public_key = AsymmetricPublicCryptoKey::from_der(&SpkiPublicKeyBytes::from(
95+
STANDARD.decode(public_key)?,
96+
))?;
9597

9698
let key_store = client.internal.get_key_store();
9799
let ctx = key_store.context();

crates/bitwarden-core/src/auth/tde.rs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use base64::{engine::general_purpose::STANDARD, Engine};
22
use bitwarden_crypto::{
3-
AsymmetricPublicCryptoKey, DeviceKey, EncString, Kdf, SymmetricCryptoKey, TrustDeviceResponse,
4-
UnsignedSharedKey, UserKey,
3+
AsymmetricPublicCryptoKey, DeviceKey, EncString, Kdf, SpkiPublicKeyBytes, SymmetricCryptoKey,
4+
TrustDeviceResponse, UnsignedSharedKey, UserKey,
55
};
66

77
use crate::{client::encryption_settings::EncryptionSettingsError, Client};
@@ -15,7 +15,9 @@ pub(super) fn make_register_tde_keys(
1515
org_public_key: String,
1616
remember_device: bool,
1717
) -> Result<RegisterTdeKeyResponse, EncryptionSettingsError> {
18-
let public_key = AsymmetricPublicCryptoKey::from_der(&STANDARD.decode(org_public_key)?)?;
18+
let public_key = AsymmetricPublicCryptoKey::from_der(&SpkiPublicKeyBytes::from(
19+
STANDARD.decode(org_public_key)?,
20+
))?;
1921

2022
let user_key = UserKey::new(SymmetricCryptoKey::make_aes256_cbc_hmac_key());
2123
let key_pair = user_key.make_key_pair()?;

crates/bitwarden-core/src/key_management/crypto.rs

Lines changed: 57 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,10 @@ use std::collections::HashMap;
88

99
use base64::{engine::general_purpose::STANDARD, Engine};
1010
use bitwarden_crypto::{
11-
AsymmetricCryptoKey, CoseSerializable, CryptoError, EncString, Kdf, KeyDecryptable,
12-
KeyEncryptable, MasterKey, Pkcs8PrivateKeyBytes, PrimitiveEncryptable, SignatureAlgorithm,
13-
SignedPublicKey, SigningKey, SymmetricCryptoKey, UnsignedSharedKey, UserKey,
11+
dangerous_get_v2_rotated_account_keys, AsymmetricCryptoKey, CoseSerializable, CryptoError,
12+
EncString, Kdf, KeyDecryptable, KeyEncryptable, MasterKey, Pkcs8PrivateKeyBytes,
13+
PrimitiveEncryptable, RotatedUserKeys, SignatureAlgorithm, SignedPublicKey, SigningKey,
14+
SpkiPublicKeyBytes, SymmetricCryptoKey, UnsignedSharedKey, UserKey,
1415
};
1516
use bitwarden_error::bitwarden_error;
1617
use schemars::JsonSchema;
@@ -412,7 +413,9 @@ pub(super) fn enroll_admin_password_reset(
412413
use base64::{engine::general_purpose::STANDARD, Engine};
413414
use bitwarden_crypto::AsymmetricPublicCryptoKey;
414415

415-
let public_key = AsymmetricPublicCryptoKey::from_der(&STANDARD.decode(public_key)?)?;
416+
let public_key = AsymmetricPublicCryptoKey::from_der(&SpkiPublicKeyBytes::from(
417+
STANDARD.decode(public_key)?,
418+
))?;
416419
let key_store = client.internal.get_key_store();
417420
let ctx = key_store.context();
418421
// FIXME: [PM-18110] This should be removed once the key store can handle public key encryption
@@ -613,6 +616,56 @@ pub fn make_user_signing_keys_for_enrollment(
613616
})
614617
}
615618

619+
/// A rotated set of account keys for a user
620+
#[derive(Serialize, Deserialize, Debug, JsonSchema)]
621+
#[serde(rename_all = "camelCase", deny_unknown_fields)]
622+
#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
623+
#[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))]
624+
pub struct RotateUserKeysResponse {
625+
/// The verifying key
626+
pub verifying_key: String,
627+
/// Signing key, encrypted with a symmetric key (user key, org key)
628+
pub signing_key: EncString,
629+
/// The user's public key, signed by the signing key
630+
pub signed_public_key: String,
631+
/// The user's public key, without signature
632+
pub public_key: String,
633+
/// The user's private key, encrypted with the user key
634+
pub private_key: EncString,
635+
}
636+
637+
impl From<RotatedUserKeys> for RotateUserKeysResponse {
638+
fn from(rotated: RotatedUserKeys) -> Self {
639+
RotateUserKeysResponse {
640+
verifying_key: STANDARD.encode(rotated.verifying_key.to_vec()),
641+
signing_key: rotated.signing_key,
642+
signed_public_key: STANDARD.encode(rotated.signed_public_key.to_vec()),
643+
public_key: STANDARD.encode(rotated.public_key.to_vec()),
644+
private_key: rotated.private_key,
645+
}
646+
}
647+
}
648+
649+
/// Gets a set of new wrapped account keys for a user, given a new user key.
650+
///
651+
/// In the current implementation, it just re-encrypts any existing keys. This function expects a
652+
/// user to be a v2 user; that is, they have a signing key, a cose user-key, and a private key
653+
pub(crate) fn get_v2_rotated_account_keys(
654+
client: &Client,
655+
user_key: String,
656+
) -> Result<RotateUserKeysResponse, CryptoError> {
657+
let key_store = client.internal.get_key_store();
658+
let ctx = key_store.context();
659+
660+
dangerous_get_v2_rotated_account_keys(
661+
&SymmetricCryptoKey::try_from(user_key)?,
662+
AsymmetricKeyId::UserPrivateKey,
663+
SigningKeyId::UserSigningKey,
664+
&ctx,
665+
)
666+
.map(Into::into)
667+
}
668+
616669
#[cfg(test)]
617670
mod tests {
618671
use std::num::NonZeroU32;

crates/bitwarden-core/src/key_management/crypto_client.rs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@ use crate::key_management::crypto::{
1818
};
1919
use crate::{
2020
client::encryption_settings::EncryptionSettingsError,
21-
key_management::crypto::CryptoClientError, Client,
21+
key_management::crypto::{
22+
get_v2_rotated_account_keys, CryptoClientError, RotateUserKeysResponse,
23+
},
24+
Client,
2225
};
2326

2427
/// A client for the crypto operations.
@@ -69,6 +72,14 @@ impl CryptoClient {
6972
) -> Result<MakeUserSigningKeysResponse, CryptoError> {
7073
make_user_signing_keys_for_enrollment(&self.client)
7174
}
75+
76+
/// Creates a rotated set of account keys for the current state
77+
pub fn get_v2_rotated_account_keys(
78+
&self,
79+
user_key: String,
80+
) -> Result<RotateUserKeysResponse, CryptoError> {
81+
get_v2_rotated_account_keys(&self.client, user_key)
82+
}
7283
}
7384

7485
impl CryptoClient {

crates/bitwarden-crypto/src/keys/asymmetric_crypto_key.rs

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,14 @@ pub enum PublicKeyEncryptionAlgorithm {
1717
RsaOaepSha1 = 0,
1818
}
1919

20-
#[derive(Clone)]
20+
#[derive(Clone, PartialEq)]
2121
pub(crate) enum RawPublicKey {
2222
RsaOaepSha1(RsaPublicKey),
2323
}
2424

2525
/// Public key of a key pair used in a public key encryption scheme. It is used for
2626
/// encrypting data.
27-
#[derive(Clone)]
27+
#[derive(Clone, PartialEq)]
2828
pub struct AsymmetricPublicCryptoKey {
2929
inner: RawPublicKey,
3030
}
@@ -35,10 +35,11 @@ impl AsymmetricPublicCryptoKey {
3535
}
3636

3737
/// Build a public key from the SubjectPublicKeyInfo DER.
38-
pub fn from_der(der: &[u8]) -> Result<Self> {
38+
pub fn from_der(der: &SpkiPublicKeyBytes) -> Result<Self> {
3939
Ok(AsymmetricPublicCryptoKey {
4040
inner: RawPublicKey::RsaOaepSha1(
41-
RsaPublicKey::from_public_key_der(der).map_err(|_| CryptoError::InvalidKey)?,
41+
RsaPublicKey::from_public_key_der(der.as_ref())
42+
.map_err(|_| CryptoError::InvalidKey)?,
4243
),
4344
})
4445
}
@@ -166,8 +167,8 @@ mod tests {
166167

167168
use crate::{
168169
content_format::{Bytes, Pkcs8PrivateKeyDerContentFormat},
169-
AsymmetricCryptoKey, AsymmetricPublicCryptoKey, Pkcs8PrivateKeyBytes, SymmetricCryptoKey,
170-
UnsignedSharedKey,
170+
AsymmetricCryptoKey, AsymmetricPublicCryptoKey, Pkcs8PrivateKeyBytes, SpkiPublicKeyBytes,
171+
SymmetricCryptoKey, UnsignedSharedKey,
171172
};
172173

173174
#[test]
@@ -263,7 +264,8 @@ DnqOsltgPomWZ7xVfMkm9niL2OA=
263264

264265
let private_key = Pkcs8PrivateKeyBytes::from(private_key);
265266
let private_key = AsymmetricCryptoKey::from_der(&private_key).unwrap();
266-
let public_key = AsymmetricPublicCryptoKey::from_der(&public_key).unwrap();
267+
let public_key =
268+
AsymmetricPublicCryptoKey::from_der(&SpkiPublicKeyBytes::from(public_key)).unwrap();
267269

268270
let raw_key = SymmetricCryptoKey::make_aes256_cbc_hmac_key();
269271
let encrypted = UnsignedSharedKey::encapsulate_key_unsigned(&raw_key, &public_key).unwrap();

crates/bitwarden-crypto/src/keys/signed_public_key.rs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use super::AsymmetricPublicCryptoKey;
1313
use crate::{
1414
cose::CoseSerializable, error::EncodingError, util::FromStrVisitor, CoseSign1Bytes,
1515
CryptoError, PublicKeyEncryptionAlgorithm, RawPublicKey, SignedObject, SigningKey,
16-
SigningNamespace, VerifyingKey,
16+
SigningNamespace, SpkiPublicKeyBytes, VerifyingKey,
1717
};
1818

1919
#[cfg(feature = "wasm")]
@@ -114,8 +114,10 @@ impl SignedPublicKey {
114114
public_key_message.content_format,
115115
) {
116116
(PublicKeyEncryptionAlgorithm::RsaOaepSha1, PublicKeyFormat::Spki) => Ok(
117-
AsymmetricPublicCryptoKey::from_der(&public_key_message.public_key.into_vec())
118-
.map_err(|_| EncodingError::InvalidValue("public key"))?,
117+
AsymmetricPublicCryptoKey::from_der(&SpkiPublicKeyBytes::from(
118+
public_key_message.public_key.to_vec(),
119+
))
120+
.map_err(|_| EncodingError::InvalidValue("public key"))?,
119121
),
120122
}
121123
}

crates/bitwarden-crypto/src/lib.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,9 @@ pub use util::{generate_random_alphanumeric, generate_random_bytes, pbkdf2};
3131
mod wordlist;
3232
pub use wordlist::EFF_LONG_WORD_LIST;
3333
mod store;
34-
pub use store::{KeyStore, KeyStoreContext};
34+
pub use store::{
35+
dangerous_get_v2_rotated_account_keys, KeyStore, KeyStoreContext, RotatedUserKeys,
36+
};
3537
mod cose;
3638
pub use cose::CoseSerializable;
3739
mod signing;

crates/bitwarden-crypto/src/store/context.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -378,7 +378,10 @@ impl<Ids: KeyIds> KeyStoreContext<'_, Ids> {
378378
.ok_or_else(|| crate::CryptoError::MissingKeyId(format!("{key_id:?}")))
379379
}
380380

381-
fn get_asymmetric_key(&self, key_id: Ids::Asymmetric) -> Result<&AsymmetricCryptoKey> {
381+
pub(super) fn get_asymmetric_key(
382+
&self,
383+
key_id: Ids::Asymmetric,
384+
) -> Result<&AsymmetricCryptoKey> {
382385
if key_id.is_local() {
383386
self.local_asymmetric_keys.get(key_id)
384387
} else {
@@ -387,7 +390,7 @@ impl<Ids: KeyIds> KeyStoreContext<'_, Ids> {
387390
.ok_or_else(|| crate::CryptoError::MissingKeyId(format!("{key_id:?}")))
388391
}
389392

390-
fn get_signing_key(&self, key_id: Ids::Signing) -> Result<&SigningKey> {
393+
pub(super) fn get_signing_key(&self, key_id: Ids::Signing) -> Result<&SigningKey> {
391394
if key_id.is_local() {
392395
self.local_signing_keys.get(key_id)
393396
} else {
Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
use crate::{
2+
CoseKeyBytes, CoseSerializable, CoseSign1Bytes, CryptoError, EncString, KeyEncryptable, KeyIds,
3+
KeyStoreContext, SignedPublicKeyMessage, SpkiPublicKeyBytes, SymmetricCryptoKey,
4+
};
5+
6+
/// Rotated set of account keys
7+
pub struct RotatedUserKeys {
8+
/// The verifying key
9+
pub verifying_key: CoseKeyBytes,
10+
/// Signing key, encrypted with a symmetric key (user key, org key)
11+
pub signing_key: EncString,
12+
/// The user's public key, signed by the signing key
13+
pub signed_public_key: CoseSign1Bytes,
14+
/// The user's public key, without signature
15+
pub public_key: SpkiPublicKeyBytes,
16+
/// The user's private key, encrypted with the user key
17+
pub private_key: EncString,
18+
}
19+
20+
/// Re-encrypts the user's keys with the provided symmetric key for a v2 user.
21+
pub fn dangerous_get_v2_rotated_account_keys<Ids: KeyIds>(
22+
new_user_key: &SymmetricCryptoKey,
23+
current_user_private_key_id: Ids::Asymmetric,
24+
current_user_signing_key_id: Ids::Signing,
25+
ctx: &KeyStoreContext<Ids>,
26+
) -> Result<RotatedUserKeys, CryptoError> {
27+
let current_private_key = ctx.get_asymmetric_key(current_user_private_key_id)?;
28+
let current_signing_key = ctx.get_signing_key(current_user_signing_key_id)?;
29+
30+
let signed_public_key =
31+
SignedPublicKeyMessage::from_public_key(&current_private_key.to_public_key())?
32+
.sign(current_signing_key)?;
33+
Ok(RotatedUserKeys {
34+
verifying_key: current_signing_key.to_verifying_key().to_cose(),
35+
signing_key: current_signing_key
36+
.to_cose()
37+
.encrypt_with_key(new_user_key)?,
38+
signed_public_key: signed_public_key.into(),
39+
public_key: current_private_key.to_public_key().to_der()?,
40+
private_key: current_private_key
41+
.to_der()?
42+
.encrypt_with_key(new_user_key)?,
43+
})
44+
}
45+
46+
#[cfg(test)]
47+
mod tests {
48+
use super::*;
49+
use crate::{
50+
traits::tests::{TestAsymmKey, TestIds, TestSigningKey, TestSymmKey},
51+
AsymmetricCryptoKey, KeyDecryptable, KeyStore, Pkcs8PrivateKeyBytes,
52+
PublicKeyEncryptionAlgorithm, SignedPublicKey, SigningKey,
53+
};
54+
55+
#[test]
56+
fn test_account_key_rotation() {
57+
let store: KeyStore<TestIds> = KeyStore::default();
58+
let mut ctx = store.context_mut();
59+
60+
// Generate a new user key
61+
let new_user_key = SymmetricCryptoKey::make_xchacha20_poly1305_key();
62+
let current_user_private_key_id = TestAsymmKey::A(0);
63+
let current_user_signing_key_id = TestSigningKey::A(0);
64+
65+
// Make the keys
66+
ctx.generate_symmetric_key(TestSymmKey::A(0)).unwrap();
67+
ctx.make_signing_key(current_user_signing_key_id).unwrap();
68+
#[allow(deprecated)]
69+
ctx.set_asymmetric_key(
70+
current_user_private_key_id,
71+
AsymmetricCryptoKey::make(PublicKeyEncryptionAlgorithm::RsaOaepSha1),
72+
)
73+
.unwrap();
74+
75+
// Get the rotated account keys
76+
let rotated_keys = dangerous_get_v2_rotated_account_keys(
77+
&new_user_key,
78+
current_user_private_key_id,
79+
current_user_signing_key_id,
80+
&ctx,
81+
)
82+
.unwrap();
83+
84+
// Public/Private key
85+
assert_eq!(
86+
rotated_keys.public_key,
87+
ctx.get_asymmetric_key(current_user_private_key_id)
88+
.unwrap()
89+
.to_public_key()
90+
.to_der()
91+
.unwrap()
92+
);
93+
let decrypted_private_key: Vec<u8> = rotated_keys
94+
.private_key
95+
.decrypt_with_key(&new_user_key)
96+
.unwrap();
97+
let private_key =
98+
AsymmetricCryptoKey::from_der(&Pkcs8PrivateKeyBytes::from(decrypted_private_key))
99+
.unwrap();
100+
assert_eq!(
101+
private_key.to_der().unwrap(),
102+
ctx.get_asymmetric_key(current_user_private_key_id)
103+
.unwrap()
104+
.to_der()
105+
.unwrap()
106+
);
107+
108+
// Signing Key
109+
let decrypted_signing_key: Vec<u8> = rotated_keys
110+
.signing_key
111+
.decrypt_with_key(&new_user_key)
112+
.unwrap();
113+
let signing_key =
114+
SigningKey::from_cose(&CoseKeyBytes::from(decrypted_signing_key)).unwrap();
115+
assert_eq!(
116+
signing_key.to_cose(),
117+
ctx.get_signing_key(current_user_signing_key_id)
118+
.unwrap()
119+
.to_cose(),
120+
);
121+
122+
// Signed Public Key
123+
let signed_public_key = SignedPublicKey::try_from(rotated_keys.signed_public_key).unwrap();
124+
let unwrapped_key = signed_public_key
125+
.verify_and_unwrap(
126+
&ctx.get_signing_key(current_user_signing_key_id)
127+
.unwrap()
128+
.to_verifying_key(),
129+
)
130+
.unwrap();
131+
assert_eq!(
132+
unwrapped_key.to_der().unwrap(),
133+
ctx.get_asymmetric_key(current_user_private_key_id)
134+
.unwrap()
135+
.to_public_key()
136+
.to_der()
137+
.unwrap()
138+
);
139+
}
140+
}

0 commit comments

Comments
 (0)