From b5460fc2a070b829b899d93cac77cc6e93a5c35d Mon Sep 17 00:00:00 2001 From: Arthur Gautier Date: Tue, 25 Apr 2023 08:55:30 -0700 Subject: [PATCH 1/2] x509-cert: make SKI optional in leaf certificate [RFC5280 Section 4.2.1.2] recommends the SKI to be included but other specifications (IEEE 801.1AR Section 8.10.2 subjectKeyIdentifier) says it should not be included. This introduces a tunable in the Leaf profile under the `hazmat` feature not to include it. [RFC5280 Section 4.2.1.2]: https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.2 --- x509-cert/src/builder.rs | 17 +++++++++++++- x509-cert/tests/builder.rs | 45 +++++++++++++++++++++++++++++++++----- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/x509-cert/src/builder.rs b/x509-cert/src/builder.rs index 399e7ee25..e8f8ae48f 100644 --- a/x509-cert/src/builder.rs +++ b/x509-cert/src/builder.rs @@ -92,6 +92,14 @@ pub enum Profile { enable_key_agreement: bool, /// should the key encipherment flag of KeyUsage be enabled enable_key_encipherment: bool, + /// should the subject key identifier extension be included + /// + /// From [RFC 5280 Section 4.2.1.2]: + /// For end entity certificates, subject key identifiers SHOULD be + /// derived from the public key. Two common methods for generating key + /// identifiers from the public key are identified above. + #[cfg(feature = "hazmat")] + include_subject_key_identifier: bool, }, #[cfg(feature = "hazmat")] /// Opt-out of the default extensions @@ -128,7 +136,14 @@ impl Profile { let mut extensions: vec::Vec = vec::Vec::new(); - extensions.push(SubjectKeyIdentifier::try_from(spk)?.to_extension(tbs)?); + match self { + #[cfg(feature = "hazmat")] + Profile::Leaf { + include_subject_key_identifier: false, + .. + } => {} + _ => extensions.push(SubjectKeyIdentifier::try_from(spk)?.to_extension(tbs)?), + } // Build Authority Key Identifier match self { diff --git a/x509-cert/tests/builder.rs b/x509-cert/tests/builder.rs index 27961dfe7..3c9ffd0aa 100644 --- a/x509-cert/tests/builder.rs +++ b/x509-cert/tests/builder.rs @@ -90,9 +90,11 @@ fn leaf_certificate() { let issuer = Name::from_str("CN=World domination corporation,O=World domination Inc,C=US").unwrap(); let profile = Profile::Leaf { - issuer, + issuer: issuer.clone(), enable_key_agreement: false, enable_key_encipherment: false, + #[cfg(feature = "hazmat")] + include_subject_key_identifier: true, }; let subject = Name::from_str("CN=service.domination.world").unwrap(); @@ -100,9 +102,15 @@ fn leaf_certificate() { SubjectPublicKeyInfoOwned::try_from(RSA_2048_DER_EXAMPLE).expect("get rsa pub key"); let signer = ecdsa_signer(); - let builder = - CertificateBuilder::new(profile, serial_number, validity, subject, pub_key, &signer) - .expect("Create certificate"); + let builder = CertificateBuilder::new( + profile, + serial_number.clone(), + validity.clone(), + subject.clone(), + pub_key.clone(), + &signer, + ) + .expect("Create certificate"); let certificate = builder.build::>().unwrap(); @@ -110,7 +118,7 @@ fn leaf_certificate() { println!("{}", openssl::check_certificate(pem.as_bytes())); // TODO(baloo): not too sure we should tackle those in this API. - let ignored = &[ + let mut ignored = vec![ "e_sub_cert_aia_missing", "e_sub_cert_crl_distribution_points_missing", "w_sub_cert_aia_does_not_contain_issuing_ca_url", @@ -126,7 +134,30 @@ fn leaf_certificate() { "e_sub_cert_eku_missing", ]; - zlint::check_certificate(pem.as_bytes(), ignored); + zlint::check_certificate(pem.as_bytes(), &ignored); + + #[cfg(feature = "hazmat")] + { + let profile = Profile::Leaf { + issuer, + enable_key_agreement: false, + enable_key_encipherment: false, + include_subject_key_identifier: false, + }; + let builder = + CertificateBuilder::new(profile, serial_number, validity, subject, pub_key, &signer) + .expect("Create certificate"); + + let certificate = builder.build::>().unwrap(); + + let pem = certificate.to_pem(LineEnding::LF).expect("generate pem"); + println!("{}", openssl::check_certificate(pem.as_bytes())); + + // Ignore warning about leaf not having SKI extension (this is a warning not a fail, as + // denoted by the `w_` prefix. + ignored.push("w_ext_subject_key_identifier_missing_sub_cert"); + zlint::check_certificate(pem.as_bytes(), &ignored); + } } #[test] @@ -140,6 +171,8 @@ fn pss_certificate() { issuer, enable_key_agreement: false, enable_key_encipherment: false, + #[cfg(feature = "hazmat")] + include_subject_key_identifier: true, }; let subject = Name::from_str("CN=service.domination.world").unwrap(); From ba485b6b7c31abd4239d68317a85da5cc4f57285 Mon Sep 17 00:00:00 2001 From: Arthur Gautier Date: Tue, 25 Apr 2023 09:35:58 -0700 Subject: [PATCH 2/2] fixup warnings --- x509-cert/tests/builder.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x509-cert/tests/builder.rs b/x509-cert/tests/builder.rs index 3c9ffd0aa..e7db2d7b8 100644 --- a/x509-cert/tests/builder.rs +++ b/x509-cert/tests/builder.rs @@ -118,7 +118,7 @@ fn leaf_certificate() { println!("{}", openssl::check_certificate(pem.as_bytes())); // TODO(baloo): not too sure we should tackle those in this API. - let mut ignored = vec![ + let ignored = vec![ "e_sub_cert_aia_missing", "e_sub_cert_crl_distribution_points_missing", "w_sub_cert_aia_does_not_contain_issuing_ca_url", @@ -155,6 +155,7 @@ fn leaf_certificate() { // Ignore warning about leaf not having SKI extension (this is a warning not a fail, as // denoted by the `w_` prefix. + let mut ignored = ignored; ignored.push("w_ext_subject_key_identifier_missing_sub_cert"); zlint::check_certificate(pem.as_bytes(), &ignored); }