Skip to content

Commit

Permalink
cert: use key_identifier_method of issuer for AKI
Browse files Browse the repository at this point in the history
Previously when issuing a certificate with an authority key identifier
(AKI) extension that's signed by an issuer certificate we had a small
bug where we used the to-be-issued certificate's param's `key_identifier_method`
to derive the key identifier of the issuing certificate to use for the
issued certificate's AKI.

Instead we should be using the issuer certificate's param's
`key_identifier_method`, taking care to mind the pre-specified variant.

We missed this with our unit testing of the pre-specified key identifier
method because we only issued a self-signed test certificate, never
issuing a certificate signed by the CA that has the customization.

This commit fixes the bug and extends test coverage to prevent
further regression.
  • Loading branch information
cpu authored and djc committed Apr 5, 2024
1 parent 7db8619 commit 72714e5
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 22 deletions.
55 changes: 38 additions & 17 deletions rcgen/src/certificate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,7 @@ impl CertificateParams {
issuer_key: &KeyPair,
) -> Result<Certificate, Error> {
let subject_public_key_info = key_pair.public_key_der();
let der = self.serialize_der_with_signer(
key_pair,
issuer_key,
&issuer.params.distinguished_name,
)?;
let der = self.serialize_der_with_signer(key_pair, issuer_key, &issuer.params)?;
Ok(Certificate {
params: self,
subject_public_key_info,
Expand All @@ -172,7 +168,7 @@ impl CertificateParams {
/// [`Certificate::pem`].
pub fn self_signed(self, key_pair: &KeyPair) -> Result<Certificate, Error> {
let subject_public_key_info = key_pair.public_key_der();
let der = self.serialize_der_with_signer(key_pair, key_pair, &self.distinguished_name)?;
let der = self.serialize_der_with_signer(key_pair, key_pair, &self)?;
Ok(Certificate {
params: self,
subject_public_key_info,
Expand Down Expand Up @@ -567,7 +563,7 @@ impl CertificateParams {
&self,
pub_key: &K,
issuer: &KeyPair,
issuer_name: &DistinguishedName,
issuer_params: &CertificateParams,
) -> Result<CertificateDer<'static>, Error> {
let der = issuer.sign_der(|writer| {
let pub_key_spki =
Expand Down Expand Up @@ -596,7 +592,7 @@ impl CertificateParams {
// Write signature algorithm
issuer.alg.write_alg_ident(writer.next());
// Write issuer name
write_distinguished_name(writer.next(), issuer_name);
write_distinguished_name(writer.next(), &issuer_params.distinguished_name);
// Write validity
writer.next().write_sequence(|writer| {
// Not before
Expand Down Expand Up @@ -626,7 +622,13 @@ impl CertificateParams {
if self.use_authority_key_identifier_extension {
write_x509_authority_key_identifier(
writer.next(),
self.key_identifier_method.derive(issuer.public_key_der()),
match &issuer_params.key_identifier_method {
KeyIdMethod::PreSpecified(aki) => aki.clone(),
#[cfg(feature = "crypto")]
_ => issuer_params
.key_identifier_method
.derive(issuer.public_key_der()),
},
);
}
// Write subject_alt_names
Expand Down Expand Up @@ -1397,24 +1399,24 @@ PITGdT9dgN88nHPCle0B1+OY+OZ5
-----END PRIVATE KEY-----"#;

let params = CertificateParams::from_ca_cert_pem(ca_cert).unwrap();
let expected_ski = vec![
let ca_ski = vec![
0x97, 0xD4, 0x76, 0xA1, 0x9B, 0x1A, 0x71, 0x35, 0x2A, 0xC7, 0xF4, 0xA1, 0x84, 0x12,
0x56, 0x06, 0xBA, 0x5D, 0x61, 0x84,
];

assert_eq!(
KeyIdMethod::PreSpecified(expected_ski.clone()),
KeyIdMethod::PreSpecified(ca_ski.clone()),
params.key_identifier_method
);

let kp = KeyPair::from_pem(ca_key).unwrap();
let ca_cert = params.self_signed(&kp).unwrap();
assert_eq!(&expected_ski, &ca_cert.key_identifier());
let ca_kp = KeyPair::from_pem(ca_key).unwrap();
let ca_cert = params.self_signed(&ca_kp).unwrap();
assert_eq!(&ca_ski, &ca_cert.key_identifier());

let (_remainder, x509) = x509_parser::parse_x509_certificate(ca_cert.der()).unwrap();
let (_, x509_ca) = x509_parser::parse_x509_certificate(ca_cert.der()).unwrap();
assert_eq!(
&expected_ski,
&x509
&ca_ski,
&x509_ca
.iter_extensions()
.find_map(|ext| match ext.parsed_extension() {
x509_parser::extensions::ParsedExtension::SubjectKeyIdentifier(key_id) => {
Expand All @@ -1424,6 +1426,25 @@ PITGdT9dgN88nHPCle0B1+OY+OZ5
})
.unwrap()
);

let ee_key = KeyPair::generate().unwrap();
let mut ee_params = CertificateParams::default();
ee_params.use_authority_key_identifier_extension = true;
let ee_cert = ee_params.signed_by(&ee_key, &ca_cert, &ee_key).unwrap();

let (_, x509_ee) = x509_parser::parse_x509_certificate(ee_cert.der()).unwrap();
assert_eq!(
&ca_ski,
&x509_ee
.iter_extensions()
.find_map(|ext| match ext.parsed_extension() {
x509_parser::extensions::ParsedExtension::AuthorityKeyIdentifier(aki) => {
aki.key_identifier.as_ref().map(|ki| ki.0.to_vec())
},
_ => None,
})
.unwrap()
);
}
}
}
8 changes: 3 additions & 5 deletions rcgen/src/csr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,9 @@ impl CertificateSigningRequestParams {
issuer: &Certificate,
issuer_key: &KeyPair,
) -> Result<Certificate, Error> {
let der = self.params.serialize_der_with_signer(
&self.public_key,
issuer_key,
&issuer.params.distinguished_name,
)?;
let der =
self.params
.serialize_der_with_signer(&self.public_key, issuer_key, &issuer.params)?;
let subject_public_key_info = yasna::construct_der(|writer| {
self.public_key.serialize_public_key_der(writer);
});
Expand Down

0 comments on commit 72714e5

Please sign in to comment.