[PM-20361] Signature keys #207

quexten · 2025-04-05T10:13:10Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-20361

📔 Objective

Signing operations

This PR adds signing/verifying operations, and keys, including serialization to COSE for both the keys, and for the signatures. We enforce strong domain separation by adding a namespace. This ensures not having to worry about cross-protocol attacks / swapping messages between contexts.

Two signing operations are provided; sign and sign detached. The former signs an object and returns a signedobject, containing the payload. To get the payload, the signature must be verified (guaranteed by the interface). The latter returns signature and serialized message. This is useful when multiple signatures are needed for one message; in this case a countersignature can be created, so that we can have multiple signatures for the same object, in case a trust relationship needs to be proven in multiple directions.

Signing is implemented in 3 layers documented at the top of sign.rs.

The high level layer accepts a struct that is serializable and namespace, and returns a signature and the serialized representation, or the signedobject. The serialization may not be canonical, so the consumer should store the serialized representation instead of their input struct. The reverse operation - verify / getverifiedpayload - returns the deserialized struct.

The middle layer accepts byte array plus namespace.

The low layer is the direct implementation of the signature, and accepts byte array (no namespace). This ensures that implementing a new signature scheme only requires implementing this low level operation.

The one implemented algorithm type is ed25519.

Keys are represented as enums such that it is easy to add other signing key types (ML-DSA for post-quantum crypto, PoC here: #216) later on.

The signature is represented as a Cose Sign1 message (https://www.rfc-editor.org/rfc/rfc9052.html#name-signing-with-one-signer).

The namespace is separated by setting the protected header. Since this is included in the signed data, and since this is validated on verifying the signature, and since the values are unique, domain separation is enforced. We only ever want to expose the safe function outside of the crate (if we even expose it out of the crate).

PublicKeyOwnershipClaim

The new cryptographic identity is the signature keypair. Thus, a way to tie this identity to an asymmetric encryption keypair is needed, so that a trusted user can receive messages(keys) securely. For this, a user signs a publickeyownershipclaim, which another user can verify against the trusted verifying key, plus the public key downloaded from the server, to ensure the public key is a valid public key for the trusted identity.

⏰ 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

👍 (:+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

github-actions · 2025-04-05T10:17:49Z

Checkmarx One – Scan Summary & Details – dc6eb5ad-fb91-461e-b59e-85caa55ff29c

Great job, no security vulnerabilities found in this Pull Request

codecov · 2025-04-05T10:21:56Z

Codecov Report

Attention: Patch coverage is 87.39255% with 132 lines in your changes missing coverage. Please review.

Project coverage is 70.97%. Comparing base (046d1e5) to head (e373638).

Files with missing lines	Patch %	Lines
crates/bitwarden-core/src/mobile/crypto.rs	34.54%	36 Missing ⚠️
...es/bitwarden-crypto/src/keys/signing_crypto_key.rs	89.66%	31 Missing ⚠️
crates/bitwarden-crypto/src/signing/sign.rs	96.84%	11 Missing ⚠️
crates/bitwarden-crypto/src/store/context.rs	88.65%	11 Missing ⚠️
crates/bitwarden-wasm-internal/src/pure_crypto.rs	83.05%	10 Missing ⚠️
crates/bitwarden-core/src/auth/login/api_key.rs	0.00%	6 Missing ⚠️
crates/bitwarden-core/src/auth/login/password.rs	0.00%	6 Missing ⚠️
crates/bitwarden-core/src/auth/tde.rs	0.00%	5 Missing ⚠️
...s/bitwarden-core/src/client/encryption_settings.rs	70.00%	3 Missing ⚠️
crates/bitwarden-core/src/mobile/crypto_client.rs	0.00%	3 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
+ Coverage   69.94%   70.97%   +1.02%     
==========================================
  Files         214      219       +5     
  Lines       16935    17932     +997     
==========================================
+ Hits        11846    12728     +882     
- Misses       5089     5204     +115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

quexten · 2025-04-17T17:22:32Z

Adding both @MGibson1 and @Hinton to the reviews here. This is a completely new (w.r.t. bitwarden) type of cryptographic object, operation and key.

crates/bitwarden-crypto/src/cose.rs

Hinton · 2025-05-12T15:14:51Z

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

+enum SigningCryptoKeyEnum {
+    Ed25519(ed25519_dalek::SigningKey),
+}


issue: Suffixing types with the type name is not a convention in rust.

Do you have a suggestion for naming here? For a similar construct, SymmetricCryptoKeys, we can use enums directly (at the moment) because there is no KeyId / it is only present one one variant. On signing/verifying keys, it is always present, so it makes sense to not have it in the variants. So there is an "outer struct" (at the moment SigningKey, VerifyingKey), and an inner enum (for the different algorithm options) needed.

I don't know what the idiom here is. Maybe "RawSigningCryptoKey"? "SigningCryptoKeyWithoutId"?

I hesitate to rename the outer struct, because it is more widely accessible by users (other consumers), so naming it something like SigningCryptoKeyWithId would make it needlessly complex.

Renamed to RawSigningkey

crates/bitwarden-crypto/src/error.rs

Hinton · 2025-05-19T10:27:47Z

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

@@ -14,6 +14,7 @@ mod asymmetric_crypto_key;
 pub use asymmetric_crypto_key::{
    AsymmetricCryptoKey, AsymmetricEncryptable, AsymmetricPublicCryptoKey,
 };
+mod signing_crypto_key;


issue: The key namespace is getting crowed. I think we need to separate cryptographic keys (asymmetric, symmetric) from Bitwarden concepts like user, master keys. I'm not sure where signing keys falls here? It might be beneficial to tie the keys to the enc string implementations?

This doesn't block this PR but I'd want us to figure this out before adding more files to crypto.

I agree. I've been meaning to both look at simplifying some bitwarden-crypto interfaces, but cleaning up some concepts here, but on my priority lists that's after getting cose symmetric crypto + (user) signing done.

MasterKey, PinKey, etc. are just specially named SymmetricKeys. So I feel that the bitwarden labeled concepts, should be separated from the "pure" crypto concepts.

Hinton · 2025-05-19T10:32:34Z

crates/bitwarden-crypto/src/signing/mod.rs

question: Why is this file so far away from the other signing code?

Signing namespace definitions did not really feel like something that belongs into keys. But then again, maybe the same is the case for implementing signing / verifying on the respective structs?

Should these be moved out to signing too?

Moved the signing code to the signing module now. The cryptokey only contains the encoding to/from cose / conversion to verifying key now.

Hinton · 2025-05-19T11:07:11Z

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

+        let (key_id, Some(algorithm), key_type) = (cose_key.key_id, cose_key.alg, cose_key.kty)
+        else {
+            return Err(CryptoError::InvalidKey);
+        };
+        let key_id: [u8; 16] = key_id
+            .as_slice()
+            .try_into()
+            .map_err(|_| CryptoError::InvalidKey)?;
+        let key_id: KeyId = key_id.into();
+
+        match (key_type, algorithm) {


sugestion (optional): I don't think using let syntax for tuples are worth it unless you use some pattern matching. I think the below is more readable?

Suggested change

let (key_id, Some(algorithm), key_type) = (cose_key.key_id, cose_key.alg, cose_key.kty)

else {

return Err(CryptoError::InvalidKey);

};

let key_id: [u8; 16] = key_id

.as_slice()

.try_into()

.map_err(|_| CryptoError::InvalidKey)?;

let key_id: KeyId = key_id.into();

match (key_type, algorithm) {

let Some(algorithm) = cose_key.alg else {

return Err(CryptoError::InvalidKey);

};

let key_id: [u8; 16] = cose_key.key_id

.as_slice()

.try_into()

.map_err(|_| CryptoError::InvalidKey)?;

let key_id: KeyId = key_id.into();

match (cose_key.kty, algorithm) {

Hinton · 2025-05-19T11:11:48Z

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

question: Encoding the payload is currently up to the consumer and it'd not be surprised if teams adopt various ways to go from data types to byte slices. Can we make the interfaces nicer somehow by handling the serialization for them?

I agree, but had planned to move the implementation for that someplace else; the byte signing would not be exposed to the consumers. But maybe it should happen earlier...

Regardless of where it lives, an interface such as:

pub fn sign_detatched<Message: Serialize>(message: Message, namespace: SigningNamespace, signing_key: ...) -> Result<Signature> {

is what I had in mind. We'll need to handle this in the crypto crate anyways, if we want to provide a content_type header. I suggest just using cbor encoding (contentType: application/cbor) here.

Does this sound reasonable to you?

Ok, so for signing (non-detached), I suggest the following implemented on the SigningKey (and a public version on the key_context):

pub(crate) fn sign<Message: Serialize>(&self, message: Message, namespace: SigningNamespace) -> Result<SignedObject, CryptoError> {

For signing (detatched), I suggest:

pub(crate) fn sign_detatched<Message: Serialize>(&self, message: Message, namespace: SigningNamespace) -> Result<(Signature, Vec<u8>), CryptoError>

Both will internally serialize to cbor, and call a private, sign_..._bytes function that additionally takes a CoapContentFormat, and set this on the Cose object.

For the latter, despite taking a non-serialized struct, we need to return to the callers the serialized version, since serialization MAY be non-canonical. I.e an update of ciborium could result in the same struct being serialized slightly differently, thus invalidating the signatures. The caller is responsible for storing the serialized object, and cannot rely on the unserialized version.

Thoughts?

Also cc @MGibson1 in case you are not watching this thread.

Ok, this is resolved now. The sign api that is exposed only exposes structs. We now have 3 layers to this api: High level is generic, accepting a struct that implement Serialize/Deserialize + namespace, mid level works on byte arrays + namespace, low level works on just byte arrays.

This layering makes both the outside exposed interface easy, but also implementing new signature types / schemes easy.

This is also documented at the top of sign.rs

Hinton · 2025-05-19T11:28:06Z

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

+        let mut namespace = None;
+        for (key, value) in &self.0.protected.header.rest {
+            if let Label::Int(key) = key {
+                if *key == SIGNING_NAMESPACE {
+                    namespace.replace(value);
+                }
+            }
+        }
+        let Some(namespace) = namespace else {
+            return Err(CryptoError::InvalidNamespace);
+        };
+        let Some(namespace) = namespace.as_integer() else {
+            return Err(CryptoError::InvalidNamespace);
+        };


suggestion: We can rewrite this as:

Suggested change

let mut namespace = None;

for (key, value) in &self.0.protected.header.rest {

if let Label::Int(key) = key {

if *key == SIGNING_NAMESPACE {

namespace.replace(value);

}

}

}

let Some(namespace) = namespace else {

return Err(CryptoError::InvalidNamespace);

};

let Some(namespace) = namespace.as_integer() else {

return Err(CryptoError::InvalidNamespace);

};

let namespace = self

.0

.protected

.header

.rest

.iter()

.find_map(|(key, value)| {

if let Label::Int(key) = key {

if *key == SIGNING_NAMESPACE {

return value.as_integer();

}

}

None

})

.ok_or(CryptoError::InvalidNamespace)?;

Hinton · 2025-05-19T11:29:15Z

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

+        let namespace: i128 = namespace.into();
+        SigningNamespace::try_from_i64(namespace as i64)


suggestion: Instead of silently discarding information we could use the try_into. However we should never get such high integers.

Suggested change

let namespace: i128 = namespace.into();

SigningNamespace::try_from_i64(namespace as i64)

SigningNamespace::try_from_i64(

namespace

.try_into()

.map_err(|_| CryptoError::InvalidNamespace)?,

)

Hinton · 2025-05-19T12:50:11Z

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

+                if crv == iana::EllipticCurve::Ed25519.to_i64().into() {
+                    let verifying_key_bytes: &[u8; 32] = x
+                        .as_bytes()
+                        .ok_or(CryptoError::InvalidKey)?
+                        .as_slice()
+                        .try_into()
+                        .map_err(|_| CryptoError::InvalidKey)?;
+                    let verifying_key =
+                        ed25519_dalek::VerifyingKey::from_bytes(verifying_key_bytes)
+                            .map_err(|_| CryptoError::InvalidKey)?;
+                    Ok(VerifyingKey {
+                        id: key_id,
+                        inner: VerifyingKeyEnum::Ed25519(verifying_key),
+                    })
+                } else {
+                    Err(CryptoError::InvalidKey)
+                }


suggestion: Change to guard statement.

Hinton · 2025-05-19T12:52:30Z

Since there is no consumer I've only looked at the code from an idiomatic rust perspective. I would prefer if we have a working implementation done before merging this since we otherwise won't get a full perspective review.

Hinton · 2025-05-20T09:19:56Z

crates/bitwarden-crypto/src/cose.rs

+/// Labels
+///


Suggested change

/// Labels

///

// Labels

… add to key context (#257) ## 🎟️ Tracking https://bitwarden.atlassian.net/browse/PM-20361 ## 📔 Objective Adds signing keys to the key context, and adds functions to make signature keys to the sdk. ## ⏰ 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  - 👍 (`:+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: Daniel García <[email protected]>

quexten · 2025-05-21T10:30:24Z

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

@@ -0,0 +1,113 @@
+//! This module provides functionality to generate a cryptographic fingerprint for a public key.


Note, this duplicates the notion of fingerprints; there is also bitwarden-crypto/src/fingerprint.rs. Suggestion: Rename the latter to "fingerprint-phrase" or "human-readable-fingerprint" - which is the mechanism to get a human-readable phrase from a canonical byte array (be that the public key DER; or in the future the cryptographic fingerprint (derived with the new fingerprint.rs) from the verifying key).

quexten · 2025-05-21T10:51:03Z

crates/bitwarden-crypto/src/signing/sign.rs

+    /// assert!(verifying_key.verify_signature(&serialized_message.as_ref(), &namespace, &signature));
+    /// ```
+    #[allow(unused)]
+    pub fn sign_detached<Message: Serialize>(


Note: These must be pub because otherwise we cannot use doctests as usage examples. It is important to me to have runnable working examples, but i would prefer to keep these pub(crate) and only expose the keycontext variants. Not sure what to do here...

quexten · 2025-05-21T11:25:23Z

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

        &self.key
    }
 }

+impl FingerprintableKey for AsymmetricPublicCryptoKey {
+    fn fingerprint_parts(&self) -> Vec<Vec<u8>> {


Thought: DER is a valid canonical encoding; and is implemented for rsa / ed25519 by rustcrypto. However, I'm hesitant to tie ourselves to DER here, because this means that new schemes (ML-DSA, or a hybrid on ML-DSA + ed25519) would not be supported.

…l into km/cose-signatures

sonarqubecloud · 2025-05-21T12:26:32Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.8% Duplication on New Code

See analysis details on SonarQube Cloud

quexten changed the title ~~Km/cose signatures~~ [PM-20361] Signature keys Apr 17, 2025

quexten commented Apr 17, 2025

View reviewed changes

crates/bitwarden-crypto/src/keys/signing_crypto_key.rs Outdated Show resolved Hide resolved

quexten requested review from Hinton and MGibson1 April 17, 2025 17:22

quexten marked this pull request as ready for review April 17, 2025 17:38

quexten requested a review from a team as a code owner April 17, 2025 17:38

quexten requested a review from addisonbeck April 17, 2025 17:38

quexten marked this pull request as draft April 30, 2025 11:15

Base automatically changed from km/cose to main May 6, 2025 15:13

Implement signing keys and signing

c9b80df

quexten force-pushed the km/cose-signatures branch from f8044bd to c9b80df Compare May 6, 2025 17:27

quexten added 3 commits May 6, 2025 19:31

Remove unused code

2ad9f59

Add test vector

b0c3dae

Cargo fmt

2e8bd92

quexten marked this pull request as ready for review May 6, 2025 17:50

quexten and others added 3 commits May 7, 2025 13:49

Make ed25519 dependency version a range

f7f1e2e

Add signed object

6deb284

Merge branch 'main' into km/cose-signatures

f42f719

quexten requested review from dani-garcia and removed request for addisonbeck May 19, 2025 11:26

quexten and others added 3 commits May 19, 2025 13:43

Merge branch 'main' into km/cose-signatures

61496cc

Replace magic value

8354476

Clean up crate::error reference

04d5329

Hinton reviewed May 19, 2025

View reviewed changes

quexten added 2 commits May 19, 2025 15:56

Add comments to signing and verifying key

efd23cc

Move and clarify comment, drop optional verifying key from signing key

d6737cf

quexten marked this pull request as draft May 20, 2025 08:58

Hinton reviewed May 20, 2025

View reviewed changes

quexten and others added 11 commits May 21, 2025 10:52

Remove incorrect login response change

6c725d2

Fix comment

fc895d8

Cleanup

0e33fee

Add comments

6308cc5

Cleanup

971eb17

Remove unrelated change

12387f8

Cleanup

f3eb525

Fix init

d6bbae0

Cleanup

2ab6620

Fix sorting of dependencies

17e0a07

quexten commented May 21, 2025

View reviewed changes

quexten added 3 commits May 21, 2025 12:37

Attempt to fix build

8c9f00f

Add more default nil values for signing key to fix ios build

06b26ba

Fix docs

a2e95e7

quexten commented May 21, 2025

View reviewed changes

quexten added 2 commits May 21, 2025 12:54

Fix build

e6ab5ef

Fix doc

4842309

quexten commented May 21, 2025

View reviewed changes

quexten and others added 7 commits May 21, 2025 13:26

Merge branch 'main' into km/cose-signatures

0e3bcb6

Add tests to purecrypto

d6e6e99

Remove code to make test vectors

0578743

Merge branch 'km/cose-signatures' of github.com:bitwarden/sdk-interna…

4182ad4

…l into km/cose-signatures

Cleanup

38ec3eb

Fix docs

4467b0d

Merge branch 'main' into km/cose-signatures

e373638

quexten marked this pull request as ready for review May 21, 2025 12:21

		let namespace: i128 = namespace.into();
		SigningNamespace::try_from_i64(namespace as i64)

		@@ -0,0 +1,113 @@
		//! This module provides functionality to generate a cryptographic fingerprint for a public key.

[PM-20361] Signature keys #207

Are you sure you want to change the base?

[PM-20361] Signature keys #207

Uh oh!

Conversation

quexten commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

Signing operations

PublicKeyOwnershipClaim

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job, no security vulnerabilities found in this Pull Request

Uh oh!

codecov bot commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

quexten commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quexten May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quexten May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quexten May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hinton commented May 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quexten May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quexten commented Apr 5, 2025 •

edited

Loading

github-actions bot commented Apr 5, 2025 •

edited

Loading

codecov bot commented Apr 5, 2025 •

edited

Loading

quexten commented Apr 17, 2025 •

edited

Loading

quexten May 20, 2025 •

edited

Loading

quexten May 20, 2025 •

edited

Loading

quexten May 21, 2025 •

edited

Loading

quexten May 21, 2025 •

edited

Loading