Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds RSA signature support to the httpsig library as requested in issue #9. The implementation introduces breaking API changes where all key construction methods now require an explicit AlgorithmName parameter, as RSA keys cannot be reliably identified by their structure alone. The version has been bumped from 0.0.21 to 0.0.22 to reflect these breaking changes. RSA support is made optional via the "rsasig" feature flag due to security concerns about non-constant time operations (Marvin Attack).
Changes:
- Added RSA signature algorithm support (RsaV1_5Sha256 and RsaPssSha512) with optional "rsasig" feature flag
- Introduced breaking API changes requiring
&AlgorithmNameparameter for all key construction methods (from_pem, from_der, from_bytes, from_base64) - Updated
get_key_ids()toget_alg_key_ids()to return both algorithm and key ID information - Updated all tests, examples, and documentation to use the new API
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| httpsig/src/crypto/mod.rs | Added RSA algorithm variants to AlgorithmName enum and implemented FromStr trait |
| httpsig/src/crypto/asymmetric.rs | Added RSA key types, updated all key construction methods to require AlgorithmName parameter, implemented RSA signing/verification |
| httpsig/src/crypto/symmetric.rs | Updated SharedKey::from_base64 to require AlgorithmName parameter |
| httpsig/src/error.rs | Added InvalidAlgorithmName error variant |
| httpsig/src/signature_params.rs | Updated test to use new API with AlgorithmName parameter |
| httpsig/src/lib.rs | Updated all tests to use new key construction API |
| httpsig/Cargo.toml | Added optional rsa dependency and rsasig feature flag, version bump to 0.0.22 |
| httpsig-hyper/src/hyper_http.rs | Renamed get_key_ids to get_alg_key_ids, updated all tests to use new API |
| httpsig-hyper/src/lib.rs | Updated tests to extract algorithm from signature headers and use new API |
| httpsig-hyper/Cargo.toml | Added rsasig feature, updated httpsig dependency version |
| httpsig-hyper/examples/*.rs | Updated examples to use new key construction API |
| httpsig-hyper/README.md | Updated documentation to show new API usage pattern |
| README.md | Updated feature list and usage examples, documented RSA as optional feature |
| Cargo.toml | Version bump to 0.0.22 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
httpsig/src/crypto/asymmetric.rs
Outdated
| .unwrap_or_default(), | ||
| #[cfg(feature = "rsasig")] | ||
| Self::RsaPssSha512(vk) => vk | ||
| .as_ref() | ||
| .to_pkcs1_der() | ||
| .map(|der| der.as_bytes().to_vec()) | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
The use of unwrap_or_default() for RSA key_id generation could silently produce an empty key_id on serialization failure. This would result in an empty string being hashed, which could lead to incorrect key identification or potential security issues. Consider returning an error or using a more explicit fallback strategy.
| .unwrap_or_default(), | |
| #[cfg(feature = "rsasig")] | |
| Self::RsaPssSha512(vk) => vk | |
| .as_ref() | |
| .to_pkcs1_der() | |
| .map(|der| der.as_bytes().to_vec()) | |
| .unwrap_or_default(), | |
| .expect("failed to serialize RSA public key (PKCS#1 DER) for key_id"), | |
| #[cfg(feature = "rsasig")] | |
| Self::RsaPssSha512(vk) => vk | |
| .as_ref() | |
| .to_pkcs1_der() | |
| .map(|der| der.as_bytes().to_vec()) | |
| .expect("failed to serialize RSA public key (PKCS#1 DER) for key_id"), |
httpsig/src/crypto/asymmetric.rs
Outdated
| Self::RsaV1_5Sha256(vk) => vk | ||
| .as_ref() | ||
| .to_pkcs1_der() | ||
| .map(|der| der.as_bytes().to_vec()) | ||
| .unwrap_or_default(), | ||
| #[cfg(feature = "rsasig")] | ||
| Self::RsaPssSha512(vk) => vk | ||
| .as_ref() | ||
| .to_pkcs1_der() | ||
| .map(|der| der.as_bytes().to_vec()) | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
The use of unwrap_or_default() for RSA key_id generation could silently produce an empty key_id on serialization failure. This would result in an empty string being hashed, which could lead to incorrect key identification or potential security issues. Consider returning an error or using a more explicit fallback strategy.
| Self::RsaV1_5Sha256(vk) => vk | |
| .as_ref() | |
| .to_pkcs1_der() | |
| .map(|der| der.as_bytes().to_vec()) | |
| .unwrap_or_default(), | |
| #[cfg(feature = "rsasig")] | |
| Self::RsaPssSha512(vk) => vk | |
| .as_ref() | |
| .to_pkcs1_der() | |
| .map(|der| der.as_bytes().to_vec()) | |
| .unwrap_or_default(), | |
| Self::RsaV1_5Sha256(vk) => match vk.as_ref().to_pkcs1_der() { | |
| Ok(der) => der.as_bytes().to_vec(), | |
| Err(err) => { | |
| trace_error!("Failed to serialize RSA public key to PKCS#1 DER for key_id (RsaV1_5Sha256): {}", err); | |
| b"rsa-der-serialization-failed".to_vec() | |
| } | |
| }, | |
| #[cfg(feature = "rsasig")] | |
| Self::RsaPssSha512(vk) => match vk.as_ref().to_pkcs1_der() { | |
| Ok(der) => der.as_bytes().to_vec(), | |
| Err(err) => { | |
| trace_error!("Failed to serialize RSA public key to PKCS#1 DER for key_id (RsaPssSha512): {}", err); | |
| b"rsa-der-serialization-failed".to_vec() | |
| } | |
| }, |
httpsig/src/crypto/asymmetric.rs
Outdated
| // read RsaPublicKey in SubjectPublicKeyInfo format in PKCS#8, which is DER encoded RSAPublicKey in PKCS#1 | ||
| let pk = RsaPublicKey::from_pkcs1_der(bytes).map_err(|e| HttpSigError::ParsePublicKeyError(e.to_string()))?; | ||
| Ok(Self::RsaV1_5Sha256(pkcs1v15::VerifyingKey::new(pk))) | ||
| } | ||
| #[cfg(feature = "rsasig")] | ||
| AlgorithmName::RsaPssSha512 => { | ||
| debug!("Read RSA-PSS public key"); | ||
| // read RsaPublicKey in SubjectPublicKeyInfo format in PKCS#8, which is DER encoded RSAPublicKey in PKCS#1 |
There was a problem hiding this comment.
The comment states "read RsaPublicKey in SubjectPublicKeyInfo format in PKCS#8" but the code is actually using from_pkcs1_der() which expects the key to be in PKCS#1 format (DER encoded RSAPublicKey), not PKCS#8 (SubjectPublicKeyInfo). The comment should be corrected to reflect the actual format being used.
| // read RsaPublicKey in SubjectPublicKeyInfo format in PKCS#8, which is DER encoded RSAPublicKey in PKCS#1 | |
| let pk = RsaPublicKey::from_pkcs1_der(bytes).map_err(|e| HttpSigError::ParsePublicKeyError(e.to_string()))?; | |
| Ok(Self::RsaV1_5Sha256(pkcs1v15::VerifyingKey::new(pk))) | |
| } | |
| #[cfg(feature = "rsasig")] | |
| AlgorithmName::RsaPssSha512 => { | |
| debug!("Read RSA-PSS public key"); | |
| // read RsaPublicKey in SubjectPublicKeyInfo format in PKCS#8, which is DER encoded RSAPublicKey in PKCS#1 | |
| // read RsaPublicKey in PKCS#1 DER format | |
| let pk = RsaPublicKey::from_pkcs1_der(bytes).map_err(|e| HttpSigError::ParsePublicKeyError(e.to_string()))?; | |
| Ok(Self::RsaV1_5Sha256(pkcs1v15::VerifyingKey::new(pk))) | |
| } | |
| #[cfg(feature = "rsasig")] | |
| AlgorithmName::RsaPssSha512 => { | |
| debug!("Read RSA-PSS public key"); | |
| // read RsaPublicKey in PKCS#1 DER format |
README.md
Outdated
| ~~- [ ] RSASSA-PKCS1-v1_5 using SHA-256~~ | ||
|
|
||
| At this point, we have no plan to support RSA signature due to [the problem related to the non-constant time operation](https://github.com/RustCrypto/RSA/issues/19), i.e., [Mervin Attack](https://people.redhat.com/~hkario/marvin/). | ||
| At this point, **RSA signature is non-default** due to [the problem related to the non-constant time operation](https://github.com/RustCrypto/RSA/issues/19), i.e., [Mervin Attack](https://people.redhat.com/~hkario/marvin/). If you want to use RSA signature, please enable the `rsa-signature` feature flag in your `Cargo.toml`. |
There was a problem hiding this comment.
The documentation mentions using the feature flag "rsa-signature" but the actual feature flag defined in Cargo.toml is "rsasig". This inconsistency will cause confusion for users. Either update the documentation to match the actual feature flag name "rsasig", or rename the feature flag in Cargo.toml to "rsa-signature" for better clarity.
README.md
Outdated
| @@ -17,12 +17,10 @@ This crates provides a basic library [httpsig](./httpsig) and [its extension](./ | |||
| - [x] Ed25519 | |||
| - [x] ECDSA-P256 using SHA-256 | |||
| - [ ] ECDSA-P384 using SHA-384 | |||
There was a problem hiding this comment.
The README indicates that ECDSA-P384 using SHA-384 is not implemented (unchecked checkbox), but the code shows full implementation of EcdsaP384Sha384 including signing, verification, and tests. The checkbox should be marked as completed [x] to accurately reflect the implementation status.
| - [ ] ECDSA-P384 using SHA-384 | |
| - [x] ECDSA-P384 using SHA-384 |
httpsig-hyper/src/hyper_http.rs
Outdated
| signature_params_eddsa.set_key_info(&secret_key_eddsa); | ||
|
|
||
| let secret_key_p256 = SecretKey::from_pem(P256_SECERT_KEY).unwrap(); | ||
| let secret_key_p256 = SecretKey::from_pem(&AlgorithmName::EcdsaP256Sha256, P256_SECERT_KEY).unwrap(); |
There was a problem hiding this comment.
The constant name has a typo: "SECERT" should be "SECRET". This usage should be updated to P256_SECRET_KEY to match the corrected constant name.
| let secret_key_p256 = SecretKey::from_pem(&AlgorithmName::EcdsaP256Sha256, P256_SECERT_KEY).unwrap(); | |
| let secret_key_p256 = SecretKey::from_pem(&AlgorithmName::EcdsaP256Sha256, P256_SECRET_KEY).unwrap(); |
httpsig/src/crypto/asymmetric.rs
Outdated
| } | ||
| // rsa | ||
| #[cfg(feature = "rsasig")] | ||
| algorithm_oids::rsaEncryption => pki.private_key, |
There was a problem hiding this comment.
Missing algorithm validation for RSA keys. When the OID is rsaEncryption, the code doesn't verify that the provided algorithm parameter matches an RSA algorithm variant (RsaV1_5Sha256 or RsaPssSha512). This could allow creating a key with mismatched algorithm and key type, potentially leading to incorrect signature operations. Similar validation is performed for ECDSA (lines 119-121) and Ed25519 (lines 130-132).
| algorithm_oids::rsaEncryption => pki.private_key, | |
| algorithm_oids::rsaEncryption => { | |
| // assert algorithm | |
| match alg { | |
| AlgorithmName::RsaV1_5Sha256 | AlgorithmName::RsaPssSha512 => {} | |
| _ => return Err(HttpSigError::ParsePrivateKeyError("Algorithm mismatch".to_string())), | |
| } | |
| pki.private_key | |
| } |
httpsig/src/crypto/asymmetric.rs
Outdated
| /// Create key id, created by SHA-256 hash of the public key bytes, then encoded in base64 | ||
| /// - For ECDSA keys, use the uncompressed SEC1 encoding of the public key point as the byte representation. | ||
| /// - For Ed25519 keys, use the raw 32-byte public key. | ||
| /// - For RSA keys, use the DER encoding of the RSAPublicKey structure (SubjectPublicKeyInfo) as defined in PKCS#1. |
There was a problem hiding this comment.
The comment mentions "SubjectPublicKeyInfo" but should be more specific. The actual structure being encoded is "RSAPublicKey" in PKCS#1 format, not "SubjectPublicKeyInfo". SubjectPublicKeyInfo is a PKCS#8 structure that wraps the RSAPublicKey. This is confirmed by the use of to_pkcs1_der() in the code below.
| /// - For RSA keys, use the DER encoding of the RSAPublicKey structure (SubjectPublicKeyInfo) as defined in PKCS#1. | |
| /// - For RSA keys, use the DER encoding of the RSAPublicKey structure in PKCS#1 format. |
httpsig-hyper/src/hyper_http.rs
Outdated
| assert_eq!(key_ids[0].1.as_ref().unwrap(), "gjrE7ACMxgzYfFHgabgf4kLTg1eKIdsJ94AiFTFj1is="); | ||
| } | ||
|
|
||
| const P256_SECERT_KEY: &str = r##"-----BEGIN PRIVATE KEY----- |
There was a problem hiding this comment.
The constant name has a typo: "SECERT" should be "SECRET". This constant should be renamed to P256_SECRET_KEY for consistency and correctness.
| const P256_SECERT_KEY: &str = r##"-----BEGIN PRIVATE KEY----- | |
| const P256_SECRET_KEY: &str = r##"-----BEGIN PRIVATE KEY----- |
httpsig/src/crypto/asymmetric.rs
Outdated
| algorithm_oids::rsaEncryption => spki_ref | ||
| .subject_public_key | ||
| .as_bytes() | ||
| .ok_or(HttpSigError::ParsePublicKeyError("Invalid public key".to_string()))?, |
There was a problem hiding this comment.
Missing algorithm validation for RSA keys. When the OID is rsaEncryption, the code doesn't verify that the provided algorithm parameter matches an RSA algorithm variant (RsaV1_5Sha256 or RsaPssSha512). This could allow creating a key with mismatched algorithm and key type, potentially leading to incorrect signature verification operations. Similar validation is performed for ECDSA (lines 312-314) and Ed25519 (lines 323-325).
| algorithm_oids::rsaEncryption => spki_ref | |
| .subject_public_key | |
| .as_bytes() | |
| .ok_or(HttpSigError::ParsePublicKeyError("Invalid public key".to_string()))?, | |
| algorithm_oids::rsaEncryption => { | |
| // assert algorithm is an RSA variant | |
| match alg { | |
| AlgorithmName::RsaV1_5Sha256 | AlgorithmName::RsaPssSha512 => {} | |
| _ => return Err(HttpSigError::ParsePublicKeyError("Algorithm mismatch".to_string())), | |
| } | |
| spki_ref | |
| .subject_public_key | |
| .as_bytes() | |
| .ok_or(HttpSigError::ParsePublicKeyError("Invalid public key".to_string()))? | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -15,10 +15,11 @@ rust-version.workspace = true | |||
| [features] | |||
| default = ["blocking"] | |||
There was a problem hiding this comment.
The "rsa-signature" feature is enabled by default in httpsig-hyper, but the README explicitly states that "RSA signature is non-default due to the problem related to the non-constant time operation (Marvin Attack)". This is inconsistent - if RSA support should be opt-in for security reasons (as stated), then it should not be in the default features of httpsig-hyper. Consider removing "rsa-signature" from the default features to align with the stated security policy.
| if algorithm_name != *alg { | ||
| return Err(HttpSigError::ParsePrivateKeyError("Algorithm mismatch".to_string())); | ||
| } | ||
| let sk_bytes = sec1::EcPrivateKey::try_from(pki.private_key) | ||
| .map_err(|e| HttpSigError::ParsePrivateKeyError(format!("Error decoding EcPrivateKey: {e}")))? | ||
| .private_key; | ||
| (algorithm_name, sk_bytes) | ||
| sk_bytes | ||
| } | ||
| // ed25519 | ||
| algorithm_oids::Ed25519 => (AlgorithmName::Ed25519, &pki.private_key[2..]), | ||
| algorithm_oids::Ed25519 => { | ||
| // assert algorithm | ||
| if AlgorithmName::Ed25519 != *alg { | ||
| return Err(HttpSigError::ParsePrivateKeyError("Algorithm mismatch".to_string())); | ||
| } | ||
| &pki.private_key[2..] | ||
| } | ||
| // rsa | ||
| #[cfg(feature = "rsa-signature")] | ||
| algorithm_oids::rsaEncryption => { | ||
| // assert algorithm | ||
| match alg { | ||
| AlgorithmName::RsaV1_5Sha256 | AlgorithmName::RsaPssSha512 => {} | ||
| _ => return Err(HttpSigError::ParsePrivateKeyError("Algorithm mismatch".to_string())), | ||
| } |
There was a problem hiding this comment.
The algorithm validation logic (checking that the provided AlgorithmName matches the key type extracted from key material) is a critical security feature but lacks test coverage. Consider adding tests that verify algorithm mismatch errors are properly raised when attempting to parse keys with incorrect algorithm names, for example: trying to parse an Ed25519 key with AlgorithmName::EcdsaP256Sha256, or parsing an RSA key with AlgorithmName::Ed25519.
| Self::RsaV1_5Sha256(vk) => vk | ||
| .as_ref() | ||
| .to_pkcs1_der() | ||
| .map(|der| der.as_bytes().to_vec()) | ||
| .unwrap_or(b"rsa-der-serialization-failed".to_vec()), | ||
| #[cfg(feature = "rsa-signature")] | ||
| Self::RsaPssSha512(vk) => vk | ||
| .as_ref() | ||
| .to_pkcs1_der() | ||
| .map(|der| der.as_bytes().to_vec()) | ||
| .unwrap_or(b"rsa-der-serialization-failed".to_vec()), |
There was a problem hiding this comment.
The key_id generation for RSA keys uses an unwrap_or fallback to a hardcoded error message string. If RSA DER serialization fails, this will generate a consistent but invalid key_id instead of returning an error. This could lead to silent failures where keys are incorrectly identified. Consider returning an error from the key_id method or logging a warning when serialization fails.
| bytes = { version = "1.11.1" } | ||
|
|
||
| # rsa is optional | ||
| rsa = { version = "0.10.0-rc.15", default-features = false, optional = true, features = [ |
There was a problem hiding this comment.
The rsa crate version "0.10.0-rc.15" is a release candidate (pre-release version). Consider using a stable version of the rsa crate for production use to avoid potential breaking changes or bugs. If a stable 0.10.0 version is not yet available, document this dependency choice and the plan to upgrade when a stable version is released.
| rsa = { version = "0.10.0-rc.15", default-features = false, optional = true, features = [ | |
| rsa = { version = "0.10.0", default-features = false, optional = true, features = [ |
This PR implements RSA signature support (#9) along with breaking changes of APIs.
Since we cannot infer the exact algorithm and parameters only from key string, especially for RSA DER keys, we changed the API of constructing secret keys and public keys from strings with AlgorithmName arg.