-
Notifications
You must be signed in to change notification settings - Fork 130
make CertificateParams::signed_by
accept impl PublicKeyData
rather than KeyPair
for the key being signed
#288
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
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 |
---|---|---|
|
@@ -147,9 +147,9 @@ impl CertificateParams { | |
/// | ||
/// The returned [`Certificate`] may be serialized using [`Certificate::der`] and | ||
/// [`Certificate::pem`]. | ||
pub fn signed_by( | ||
pub fn signed_by<K: PublicKeyData>( | ||
self, | ||
key_pair: &KeyPair, | ||
key_pair: &K, | ||
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. Nit: I'd prefer to spell this as |
||
issuer: &Certificate, | ||
issuer_key: &KeyPair, | ||
) -> Result<Certificate, Error> { | ||
|
@@ -160,7 +160,8 @@ impl CertificateParams { | |
key_pair: issuer_key, | ||
}; | ||
|
||
let subject_public_key_info = key_pair.public_key_der(); | ||
let subject_public_key_info = | ||
yasna::construct_der(|writer| key_pair.serialize_public_key_der(writer)); | ||
let der = self.serialize_der_with_signer(key_pair, issuer)?; | ||
Ok(Certificate { | ||
params: self, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,9 @@ pub enum Error { | |
#[cfg(not(feature = "crypto"))] | ||
/// Missing serial number | ||
MissingSerialNumber, | ||
/// X509 parsing error | ||
#[cfg(feature = "x509-parser")] | ||
X509, | ||
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. I think we might want to include a |
||
} | ||
|
||
impl fmt::Display for Error { | ||
|
@@ -91,6 +94,8 @@ impl fmt::Display for Error { | |
)?, | ||
#[cfg(not(feature = "crypto"))] | ||
MissingSerialNumber => write!(f, "A serial number must be specified")?, | ||
#[cfg(feature = "x509-parser")] | ||
X509 => write!(f, "X509 error")?, | ||
}; | ||
Ok(()) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,90 @@ impl fmt::Debug for KeyPairKind { | |
} | ||
} | ||
|
||
/// A public key | ||
#[cfg(feature = "x509-parser")] | ||
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. There's no need to guard the type directly. |
||
#[derive(Debug)] | ||
pub struct SubjectPublicKey { | ||
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. bike-shedding: I think this would be better called |
||
pub(crate) alg: &'static SignatureAlgorithm, | ||
pub(crate) subject_public_key: Vec<u8>, | ||
} | ||
|
||
#[cfg(feature = "x509-parser")] | ||
impl SubjectPublicKey { | ||
/// Create a `SubjectPublicKey` value from a PEM-encoded SubjectPublicKeyInfo string | ||
pub fn from_pem(pem_str: &str) -> Result<Self, Error> { | ||
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. This needs the feature guards from |
||
let spki_der = pem::parse(pem_str)._err()?.into_contents(); | ||
Self::from_der(&spki_der) | ||
} | ||
|
||
/// Create a `SubjectPublicKey` value from DER-encoded SubjectPublicKeyInfo bytes | ||
pub fn from_der(spki_der: &[u8]) -> Result<Self, Error> { | ||
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 should guard this constructor. |
||
use x509_parser::{ | ||
prelude::FromDer, | ||
x509::{AlgorithmIdentifier, SubjectPublicKeyInfo}, | ||
}; | ||
|
||
let (rem, spki) = SubjectPublicKeyInfo::from_der(spki_der).map_err(|_| Error::X509)?; | ||
if !rem.is_empty() { | ||
return Err(Error::X509); | ||
} | ||
let alg = SignatureAlgorithm::iter() | ||
.find(|alg| { | ||
let bytes = yasna::construct_der(|writer| { | ||
alg.write_oids_sign_alg(writer); | ||
}); | ||
let Ok((rest, aid)) = AlgorithmIdentifier::from_der(&bytes) else { | ||
return false; | ||
}; | ||
if !rest.is_empty() { | ||
return false; | ||
} | ||
aid == spki.algorithm | ||
}) | ||
.ok_or(Error::UnsupportedSignatureAlgorithm)?; | ||
Ok(Self { | ||
alg, | ||
subject_public_key: Vec::from(spki.subject_public_key.as_ref()), | ||
}) | ||
} | ||
} | ||
|
||
#[cfg(feature = "x509-parser")] | ||
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. This doesn't need the guard. |
||
impl PublicKeyData for SubjectPublicKey { | ||
fn alg(&self) -> &SignatureAlgorithm { | ||
self.alg | ||
} | ||
fn raw_bytes(&self) -> &[u8] { | ||
Comment on lines
+112
to
+113
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. nit: whitespace between fns |
||
&self.subject_public_key | ||
} | ||
} | ||
|
||
#[cfg(all(test, feature = "x509-parser", feature = "pem"))] | ||
mod pkd_test { | ||
use super::{KeyPair, PublicKeyData, SubjectPublicKey}; | ||
use crate::{PKCS_ECDSA_P256_SHA256, PKCS_ECDSA_P384_SHA384, PKCS_ED25519}; | ||
|
||
#[test] | ||
fn test_subject_public_key_parsing() { | ||
// NOTE: the other algorithms supported by this crate don't support keygen | ||
for alg in [ | ||
&PKCS_ED25519, | ||
&PKCS_ECDSA_P256_SHA256, | ||
&PKCS_ECDSA_P384_SHA384, | ||
] { | ||
let kp = KeyPair::generate_for(alg).expect("keygen"); | ||
let pem = kp.public_key_pem(); | ||
let der = kp.public_key_der(); | ||
|
||
let pkd_pem = SubjectPublicKey::from_pem(&pem).expect("from pem"); | ||
assert_eq!(kp.raw_bytes(), pkd_pem.raw_bytes()); | ||
|
||
let pkd_der = SubjectPublicKey::from_der(&der).expect("from der"); | ||
assert_eq!(kp.raw_bytes(), pkd_der.raw_bytes()); | ||
} | ||
} | ||
} | ||
|
||
/// A key pair used to sign certificates and CSRs | ||
/// | ||
/// Note that ring, the underlying library to handle RSA keys | ||
|
@@ -689,7 +773,7 @@ impl<T> ExternalError<T> for Result<T, pem::PemError> { | |
} | ||
} | ||
|
||
pub(crate) trait PublicKeyData { | ||
pub trait PublicKeyData { | ||
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. Needs some tweaks, as discussed here, please put those in a separate commit. Also needs documentation. |
||
fn alg(&self) -> &SignatureAlgorithm; | ||
|
||
fn raw_bytes(&self) -> &[u8]; | ||
|
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.
We'll want to revert this.