Skip to content

[Innovation-Sprint/PM-19065] Add opaque authentication / unlock #185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

68 changes: 67 additions & 1 deletion crates/bitwarden-core/src/mobile/crypto_client.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use bitwarden_crypto::CryptoError;
use bitwarden_crypto::{opaque_ke::opaque::OpaqueImpl, CryptoError, SymmetricCryptoKey};
#[cfg(feature = "internal")]
use bitwarden_crypto::{AsymmetricEncString, EncString};

Expand Down Expand Up @@ -81,6 +81,72 @@
) -> Result<VerifyAsymmetricKeysResponse, CryptoError> {
verify_asymmetric_keys(request)
}

pub fn opaque_register_start(
&self,
password: &str,
config: &bitwarden_crypto::opaque_ke::types::CipherConfiguration,
) -> Result<
bitwarden_crypto::opaque_ke::types::ClientRegistrationStartResult,
bitwarden_crypto::OpaqueError,
> {
config.start_client_registration(password)
}

Check warning on line 94 in crates/bitwarden-core/src/mobile/crypto_client.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/mobile/crypto_client.rs#L85-L94

Added lines #L85 - L94 were not covered by tests

pub fn opaque_register_finish(
&self,
password: &str,
config: &bitwarden_crypto::opaque_ke::types::CipherConfiguration,
registration_response: &[u8],
state: &[u8],
) -> Result<
bitwarden_crypto::opaque_ke::types::ClientRegistrationFinishResult,
bitwarden_crypto::OpaqueError,
> {
config.finish_client_registration(state, registration_response, password)
}

Check warning on line 107 in crates/bitwarden-core/src/mobile/crypto_client.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/mobile/crypto_client.rs#L96-L107

Added lines #L96 - L107 were not covered by tests

pub fn opaque_login_start(
&self,
password: &str,
config: &bitwarden_crypto::opaque_ke::types::CipherConfiguration,
) -> Result<
bitwarden_crypto::opaque_ke::types::ClientLoginStartResult,
bitwarden_crypto::OpaqueError,
> {
config.start_client_login(password)
}

Check warning on line 118 in crates/bitwarden-core/src/mobile/crypto_client.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/mobile/crypto_client.rs#L109-L118

Added lines #L109 - L118 were not covered by tests

pub fn opaque_login_finish(
&self,
password: &str,
config: &bitwarden_crypto::opaque_ke::types::CipherConfiguration,
login_response: &[u8],
state: &[u8],
) -> Result<
bitwarden_crypto::opaque_ke::types::ClientLoginFinishResult,
bitwarden_crypto::OpaqueError,
> {
config.finish_client_login(state, login_response, password)
}

Check warning on line 131 in crates/bitwarden-core/src/mobile/crypto_client.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/mobile/crypto_client.rs#L120-L131

Added lines #L120 - L131 were not covered by tests

/// 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)
}

Check warning on line 141 in crates/bitwarden-core/src/mobile/crypto_client.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/mobile/crypto_client.rs#L135-L141

Added lines #L135 - L141 were not covered by tests

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)
}

Check warning on line 149 in crates/bitwarden-core/src/mobile/crypto_client.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/mobile/crypto_client.rs#L143-L149

Added lines #L143 - L149 were not covered by tests
Comment on lines +132 to +149
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dani-garcia will these APIs work in the more limited mobile environment? I don't believe mobile can create a SymmetricCryptoKey for example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

impl<'a> Client {
Expand Down
2 changes: 2 additions & 0 deletions crates/bitwarden-crypto/Cargo.toml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ hkdf = ">=0.12.3, <0.13"
hmac = ">=0.12.1, <0.13"
num-bigint = ">=0.4, <0.5"
num-traits = ">=0.2.15, <0.3"
opaque-ke = "3.0.0"
pbkdf2 = { version = ">=0.12.1, <0.13", default-features = false }
rand = ">=0.8.5, <0.9"
rayon = ">=1.8.1, <2.0"
rsa = ">=0.9.2, <0.10"
schemars = { workspace = true }
serde = { workspace = true }
serde_bytes = "0.11.17"
sha1 = ">=0.10.5, <0.11"
sha2 = ">=0.10.6, <0.11"
subtle = ">=2.5.0, <3.0"
Expand Down
2 changes: 1 addition & 1 deletion crates/bitwarden-crypto/src/keys/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{util::hkdf_expand, Result};
/// Stretch the given key using HKDF.
/// This can be either a kdf-derived key (PIN/Master password) or
/// a random key from key connector
pub(super) fn stretch_key(key: &Pin<Box<GenericArray<u8, U32>>>) -> Result<Aes256CbcHmacKey> {
pub(crate) fn stretch_key(key: &Pin<Box<GenericArray<u8, U32>>>) -> Result<Aes256CbcHmacKey> {
Ok(Aes256CbcHmacKey {
enc_key: hkdf_expand(key, Some("enc"))?,
mac_key: hkdf_expand(key, Some("mac"))?,
Expand Down
3 changes: 3 additions & 0 deletions crates/bitwarden-crypto/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ pub use enc_string::{AsymmetricEncString, EncString};
mod error;
pub use error::CryptoError;
pub(crate) use error::Result;
pub use opaque_ke::error::OpaqueError;
mod fingerprint;
pub use fingerprint::fingerprint;
mod keys;
Expand All @@ -94,6 +95,8 @@ mod traits;
mod xchacha20;
pub use traits::{Decryptable, Encryptable, IdentifyKey, KeyId, KeyIds};
pub use zeroizing_alloc::ZeroAlloc as ZeroizingAllocator;
pub mod opaque_ke;
pub mod rotateable_keyset;

#[cfg(feature = "uniffi")]
uniffi::setup_scaffolding!();
Expand Down
23 changes: 23 additions & 0 deletions crates/bitwarden-crypto/src/opaque_ke/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
use serde::{Deserialize, Serialize};
use thiserror::Error;
#[cfg(feature = "wasm")]
use tsify_next::Tsify;

#[derive(Debug, Error, Deserialize, Serialize)]
#[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))]
pub enum OpaqueError {
#[error("Error creating message {0}")]
Message(String),
#[error("Error deserializing message")]
Deserialize,
Comment on lines +9 to +12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Unused?

#[error("Invalid input: {0}")]
InvalidInput(String),
#[error("Opaque protocol error {0}")]
Protocol(String),
}

impl From<opaque_ke::errors::ProtocolError> for OpaqueError {
fn from(error: opaque_ke::errors::ProtocolError) -> Self {
Self::Protocol(error.to_string())
}

Check warning on line 22 in crates/bitwarden-crypto/src/opaque_ke/error.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-crypto/src/opaque_ke/error.rs#L20-L22

Added lines #L20 - L22 were not covered by tests
}
92 changes: 92 additions & 0 deletions crates/bitwarden-crypto/src/opaque_ke/mock_server.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
use std::collections::HashMap;

use opaque_ke::ServerSetup;

use super::{
opaque::OpaqueImpl,
types::{CipherConfiguration, RistrettoTripleDhArgonSuite},
};
use crate::OpaqueError;

/// Mock opaque server implementation for testing purposes.
/// Stores server state and registrations in-memory
pub(crate) struct MockServer {
server_setups: HashMap<u8, Vec<u8>>,
password_files: HashMap<u8, Vec<u8>>,
server_login_state: HashMap<u8, Vec<u8>>,
id_counter: u8,
}

impl MockServer {
pub(crate) fn new() -> Self {
MockServer {
server_setups: HashMap::new(),
password_files: HashMap::new(),
server_login_state: HashMap::new(),
id_counter: 0,
}
}

pub(crate) fn register_start(
&mut self,
register_start_message: &[u8],
config: CipherConfiguration,
) -> Result<(Vec<u8>, u8), OpaqueError> {
let id = self.id_counter;
self.id_counter += 1;

let server_setup = ServerSetup::<RistrettoTripleDhArgonSuite>::new(&mut rand::thread_rng())
.serialize()
.to_vec();
let result = config
.start_server_registration(Some(&server_setup), register_start_message, "username")
.unwrap();
self.server_setups.insert(id, server_setup);
Ok((result.registration_response, id))
}

pub(crate) fn register_finish(
&mut self,
register_finish_message: &[u8],
id: u8,
config: CipherConfiguration,
) {
let result = config
.finish_server_registration(register_finish_message)
.unwrap();
self.password_files.insert(id, result.server_registration);
}

pub(crate) fn login_start(
&mut self,
login_start_message: &[u8],
id: u8,
config: CipherConfiguration,
) -> Vec<u8> {
let login_state = config
.start_server_login(
self.server_setups.get(&id).unwrap(),
self.password_files.get(&id).unwrap(),
login_start_message,
"username",
)
.unwrap();
self.server_login_state.insert(id, login_state.state);
login_state.credential_response
}

pub(crate) fn login_finish(
&self,
login_finish_message: &[u8],
id: u8,
config: CipherConfiguration,
) -> Vec<u8> {
config
.finish_server_login(
self.server_login_state.get(&id).unwrap(),
login_finish_message,
)
.unwrap()
.session_key
}
}
5 changes: 5 additions & 0 deletions crates/bitwarden-crypto/src/opaque_ke/mod.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Lacks documentation about what this is.

bitwarden-crypto is a generally well documented crate and we want to maintain that high standard.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pub mod error;
#[cfg(test)]
mod mock_server;
pub mod opaque;
pub mod types;
Loading
Loading