-
Notifications
You must be signed in to change notification settings - Fork 349
feat: add ES512 algorithm support using P-521 elliptic curve #478
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| //! Implementations of the [`JwtSigner`] and [`JwtVerifier`] traits for the | ||
| //! ECDSA family of algorithms using RustCrypto | ||
|
arsenin-kitsoft marked this conversation as resolved.
|
||
|
|
||
| use crate::algorithms::AlgorithmFamily; | ||
| use crate::crypto::{JwtSigner, JwtVerifier}; | ||
| use crate::errors::{ErrorKind, Result, new_error}; | ||
|
Comment on lines
2
to
5
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep the newline |
||
|
|
@@ -11,7 +10,10 @@ use p256::ecdsa::{ | |
| use p384::ecdsa::{ | ||
| Signature as Signature384, SigningKey as SigningKey384, VerifyingKey as VerifyingKey384, | ||
| }; | ||
| use rsa::pkcs8::DecodePrivateKey; | ||
| use p521::ecdsa::{ | ||
| Signature as Signature521, SigningKey as SigningKey521, VerifyingKey as VerifyingKey521, | ||
| }; | ||
| use pkcs8::DecodePrivateKey; | ||
| use signature::{Error, Signer, Verifier}; | ||
|
|
||
| macro_rules! define_ecdsa_signer { | ||
|
|
@@ -85,3 +87,68 @@ define_ecdsa_signer!(Es384Signer, Algorithm::ES384, SigningKey384); | |
|
|
||
| define_ecdsa_verifier!(Es256Verifier, Algorithm::ES256, VerifyingKey256, Signature256); | ||
| define_ecdsa_verifier!(Es384Verifier, Algorithm::ES384, VerifyingKey384, Signature384); | ||
|
|
||
| // P521 (ES512) signer - uses sign() instead of sign_recoverable() since P521 doesn't support it | ||
| pub struct Es512Signer(SigningKey521); | ||
|
|
||
| impl Es512Signer { | ||
| pub(crate) fn new(encoding_key: &EncodingKey) -> Result<Self> { | ||
| if encoding_key.family() != AlgorithmFamily::Ec { | ||
| return Err(new_error(ErrorKind::InvalidKeyFormat)); | ||
| } | ||
|
|
||
| // Use pkcs8 to parse the PKCS8 wrapper and extract the ECPrivateKey DER | ||
| use pkcs8::PrivateKeyInfo; | ||
| let private_key_info = PrivateKeyInfo::try_from(encoding_key.inner()) | ||
|
Comment on lines
+101
to
+102
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either fully qualify it with |
||
| .map_err(|_| ErrorKind::InvalidKeyFormat)?; | ||
|
|
||
| // The private_key field contains the DER-encoded ECPrivateKey | ||
| let ec_private_key_der = private_key_info.private_key; | ||
|
|
||
| // Use simple_asn1 to parse the ECPrivateKey structure | ||
| use simple_asn1::ASN1Block; | ||
| let asn1_blocks = | ||
| simple_asn1::from_der(ec_private_key_der).map_err(|_| ErrorKind::InvalidKeyFormat)?; | ||
|
|
||
| // Find the OCTET STRING containing the 66-byte private key | ||
| for block in asn1_blocks { | ||
| if let ASN1Block::Sequence(_, entries) = block { | ||
| // ECPrivateKey ::= SEQUENCE { | ||
| // version INTEGER, | ||
| // privateKey OCTET STRING, // This is what we need (index 1) | ||
| // parameters [0] ECParameters OPTIONAL, | ||
| // publicKey [1] BIT STRING OPTIONAL | ||
| // } | ||
| if entries.len() >= 2 { | ||
| if let ASN1Block::OctetString(_, key_bytes) = &entries[1] { | ||
| if key_bytes.len() == 66 { | ||
| let mut field_bytes = p521::FieldBytes::default(); | ||
| field_bytes.copy_from_slice(key_bytes); | ||
| return Ok(Self( | ||
| SigningKey521::from_bytes(&field_bytes) | ||
| .map_err(|_| ErrorKind::InvalidEcdsaKey)?, | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+114
to
+135
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the length of |
||
|
|
||
| Err(new_error(ErrorKind::InvalidKeyFormat)) | ||
| } | ||
| } | ||
|
|
||
| impl Signer<Vec<u8>> for Es512Signer { | ||
| fn try_sign(&self, msg: &[u8]) -> std::result::Result<Vec<u8>, Error> { | ||
| let signature: Signature521 = self.0.sign(msg); | ||
| Ok(signature.to_vec()) | ||
| } | ||
| } | ||
|
Comment on lines
+141
to
+146
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discard the |
||
|
|
||
| impl JwtSigner for Es512Signer { | ||
| fn algorithm(&self) -> Algorithm { | ||
| Algorithm::ES512 | ||
| } | ||
| } | ||
|
|
||
| define_ecdsa_verifier!(Es512Verifier, Algorithm::ES512, VerifyingKey521, Signature521); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,6 +164,8 @@ pub enum KeyAlgorithm { | |
| ES256, | ||
| /// ECDSA using SHA-384 | ||
| ES384, | ||
| /// ECDSA using SHA-512 | ||
| ES512, | ||
|
|
||
| /// RSASSA-PKCS1-v1_5 using SHA-256 | ||
| RS256, | ||
|
|
@@ -207,6 +209,7 @@ impl FromStr for KeyAlgorithm { | |
| "HS512" => Ok(KeyAlgorithm::HS512), | ||
| "ES256" => Ok(KeyAlgorithm::ES256), | ||
| "ES384" => Ok(KeyAlgorithm::ES384), | ||
| "ES512" => Ok(KeyAlgorithm::ES512), | ||
| "RS256" => Ok(KeyAlgorithm::RS256), | ||
| "RS384" => Ok(KeyAlgorithm::RS384), | ||
| "PS256" => Ok(KeyAlgorithm::PS256), | ||
|
|
@@ -444,6 +447,7 @@ impl Jwk { | |
| Algorithm::HS512 => KeyAlgorithm::HS512, | ||
| Algorithm::ES256 => KeyAlgorithm::ES256, | ||
| Algorithm::ES384 => KeyAlgorithm::ES384, | ||
| Algorithm::ES512 => KeyAlgorithm::ES512, | ||
| Algorithm::RS256 => KeyAlgorithm::RS256, | ||
| Algorithm::RS384 => KeyAlgorithm::RS384, | ||
| Algorithm::RS512 => KeyAlgorithm::RS512, | ||
|
|
@@ -531,7 +535,7 @@ impl Jwk { | |
| } | ||
| EllipticCurve::Ed25519 => { | ||
| format!( | ||
| r#"{{"crv":{},"kty":{},"x":"{}"}}"#, | ||
| r#"{{crv:{},"kty":{},"x":"{}"}}"#, | ||
|
Comment on lines
-534
to
+538
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems to regress #485? |
||
| serde_json::to_string(&a.curve).unwrap(), | ||
| serde_json::to_string(&a.key_type).unwrap(), | ||
| a.x, | ||
|
|
@@ -613,6 +617,7 @@ mod tests { | |
| assert_eq!(key_alg_result, KeyAlgorithm::UNKNOWN_ALGORITHM); | ||
| } | ||
|
|
||
| #[cfg(any(feature = "rust_crypto", feature = "aws_lc_rs"))] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't gate any other tests on one of the features being selected, so this seems unnecessary |
||
| #[test] | ||
| #[wasm_bindgen_test] | ||
| fn check_thumbprint() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a 0.14-rc8 published recently on crates.io, 0.13 is 2 years old. I think the new version might make the rust-crypto code easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.14 is in heavy development for the last half year, based on rc history. Are you sure you want an rc dep in your crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p256 and p384 are the same story. I've tried to update them all to latest rc and tests were not failing, but I don't think this should be in scope for this particular PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Keats plz confirm if you want to update all p* crates (same maintainer) to rc version or we could skip it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main question was whether the 0.14 would remove some of the custom code needed in this PR. I haven't really followed when it would release, we shouldn't depend on a rc though