Skip to content

Commit 1664aea

Browse files
authored
x509 Verification: Ensure SAN is always checked for servers. (#12454)
* Ensure SAN is checked for servers, irrespective of ExtensionPolicy. * Update comments to reflect code changes. * Improve naming, add comment. * Improve comment wording.
1 parent bce5c94 commit 1664aea

File tree

3 files changed

+121
-49
lines changed

3 files changed

+121
-49
lines changed

src/rust/cryptography-x509-verification/src/policy/extension.rs

Lines changed: 51 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
use std::sync::Arc;
66

7-
use cryptography_x509::extensions::{Extension, Extensions};
7+
use cryptography_x509::extensions::{Extension, Extensions, SubjectAlternativeName};
88
use cryptography_x509::oid::{
99
AUTHORITY_INFORMATION_ACCESS_OID, AUTHORITY_KEY_IDENTIFIER_OID, BASIC_CONSTRAINTS_OID,
1010
EXTENDED_KEY_USAGE_OID, KEY_USAGE_OID, NAME_CONSTRAINTS_OID, SUBJECT_ALTERNATIVE_NAME_OID,
@@ -16,6 +16,13 @@ use crate::{
1616
ops::CryptoOps, policy::Policy, ValidationError, ValidationErrorKind, ValidationResult,
1717
};
1818

19+
use super::Subject;
20+
21+
pub(crate) enum CertificateType {
22+
EE,
23+
CA,
24+
}
25+
1926
#[derive(Clone)]
2027
pub struct ExtensionPolicy<'cb, B: CryptoOps> {
2128
pub authority_information_access: ExtensionValidator<'cb, B>,
@@ -116,9 +123,10 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> {
116123
Some(Arc::new(ee::key_usage)),
117124
),
118125
// CA/B 7.1.2.7.12 Subscriber Certificate Subject Alternative Name
119-
// This validator handles both client and server cases by only matching against
120-
// the SAN if the profile contains a subject, which it won't in the client
121-
// validation case.
126+
// This validator only handles the criticality checks. Matching
127+
// SANs against the subject in the profile is handled by
128+
// `validate_subject_alternative_name_match` which is
129+
// invoked for all EE certificates, irrespective of this field's contents.
122130
subject_alternative_name: ExtensionValidator::present(
123131
Criticality::Agnostic,
124132
Some(Arc::new(ee::subject_alternative_name)),
@@ -142,6 +150,7 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> {
142150
pub(crate) fn permits<'chain>(
143151
&self,
144152
policy: &Policy<'_, B>,
153+
certificate_type: CertificateType,
145154
cert: &VerificationCertificate<'chain, B>,
146155
extensions: &Extensions<'_>,
147156
) -> ValidationResult<'chain, (), B> {
@@ -180,6 +189,12 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> {
180189
subject_alternative_name_seen = true;
181190
self.subject_alternative_name
182191
.permits(policy, cert, Some(&ext))?;
192+
193+
if let CertificateType::EE = certificate_type {
194+
// This ensures that even custom ExtensionPolicies will always
195+
// check the SAN against the policy's subject
196+
validate_subject_alternative_name_match(&policy.subject, &ext)?;
197+
}
183198
}
184199
BASIC_CONSTRAINTS_OID => {
185200
basic_constraints_seen = true;
@@ -231,10 +246,38 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> {
231246
self.extended_key_usage.permits(policy, cert, None)?;
232247
}
233248

249+
if let CertificateType::EE = certificate_type {
250+
// SAN is always required if the policy contains a specific subject (server profile).
251+
if policy.subject.is_some() && !subject_alternative_name_seen {
252+
return Err(ValidationError::new(ValidationErrorKind::Other(
253+
"leaf server certificate has no subjectAltName".into(),
254+
)));
255+
}
256+
}
257+
234258
Ok(())
235259
}
236260
}
237261

262+
/// This only verifies the SAN against `subject` if `subject` is not None.
263+
/// This allows us to handle both client and server profiles,
264+
/// **with the expectation** that `subject` is always set for server profiles.
265+
pub(crate) fn validate_subject_alternative_name_match<'chain, B: CryptoOps>(
266+
subject: &Option<Subject<'_>>,
267+
extn: &Extension<'_>,
268+
) -> ValidationResult<'chain, (), B> {
269+
if let Some(sub) = subject {
270+
let san: SubjectAlternativeName<'_> = extn.value()?;
271+
if !sub.matches(&san) {
272+
return Err(ValidationError::new(ValidationErrorKind::Other(
273+
"leaf certificate has no matching subjectAltName".into(),
274+
)));
275+
}
276+
}
277+
278+
Ok(())
279+
}
280+
238281
/// Represents different criticality states for an extension.
239282
#[derive(Clone)]
240283
pub enum Criticality {
@@ -389,9 +432,7 @@ impl<'cb, B: CryptoOps> ExtensionValidator<'cb, B> {
389432
}
390433

391434
mod ee {
392-
use cryptography_x509::extensions::{
393-
BasicConstraints, ExtendedKeyUsage, Extension, KeyUsage, SubjectAlternativeName,
394-
};
435+
use cryptography_x509::extensions::{BasicConstraints, ExtendedKeyUsage, Extension, KeyUsage};
395436

396437
use crate::ops::{CryptoOps, VerificationCertificate};
397438
use crate::policy::{Policy, ValidationError, ValidationErrorKind, ValidationResult};
@@ -415,7 +456,7 @@ mod ee {
415456
}
416457

417458
pub(crate) fn subject_alternative_name<'chain, B: CryptoOps>(
418-
policy: &Policy<'_, B>,
459+
_: &Policy<'_, B>,
419460
cert: &VerificationCertificate<'chain, B>,
420461
extn: &Extension<'_>,
421462
) -> ValidationResult<'chain, (), B> {
@@ -435,18 +476,8 @@ mod ee {
435476
_ => (),
436477
};
437478

438-
// NOTE: We only verify the SAN against the policy's subject if the
439-
// policy actually contains one. This enables both client and server
440-
// profiles to use this validator, **with the expectation** that
441-
// server profile construction requires a subject to be present.
442-
if let Some(sub) = policy.subject.as_ref() {
443-
let san: SubjectAlternativeName<'_> = extn.value()?;
444-
if !sub.matches(&san) {
445-
return Err(ValidationError::new(ValidationErrorKind::Other(
446-
"leaf certificate has no matching subjectAltName".into(),
447-
)));
448-
}
449-
}
479+
// NOTE: policy.subject is checked against SAN elsewhere (see `ExtensionPolicy::permits`)
480+
// since we always want to check that, even if a custom ExtensionPolicy with a lax validator is used.
450481

451482
Ok(())
452483
}

src/rust/cryptography-x509-verification/src/policy/mod.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ use cryptography_x509::oid::{
2222
BASIC_CONSTRAINTS_OID, EC_SECP256R1, EC_SECP384R1, EC_SECP521R1, EKU_CLIENT_AUTH_OID,
2323
EKU_SERVER_AUTH_OID,
2424
};
25+
use extension::CertificateType;
2526
use once_cell::sync::Lazy;
2627

2728
use crate::ops::CryptoOps;
@@ -424,7 +425,8 @@ impl<'a, B: CryptoOps> Policy<'a, B> {
424425
}
425426
}
426427

427-
self.ca_extension_policy.permits(self, cert, extensions)?;
428+
self.ca_extension_policy
429+
.permits(self, CertificateType::CA, cert, extensions)?;
428430

429431
Ok(())
430432
}
@@ -437,7 +439,8 @@ impl<'a, B: CryptoOps> Policy<'a, B> {
437439
) -> ValidationResult<'chain, (), B> {
438440
self.permits_basic(cert.certificate())?;
439441

440-
self.ee_extension_policy.permits(self, cert, extensions)?;
442+
self.ee_extension_policy
443+
.permits(self, CertificateType::EE, cert, extensions)?;
441444

442445
Ok(())
443446
}

tests/x509/verification/test_verification.py

Lines changed: 65 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -214,33 +214,6 @@ def test_verify_fails_renders_oid(self):
214214
):
215215
verifier.verify(leaf, [])
216216

217-
def test_custom_ext_policy_no_san(self):
218-
leaf = _load_cert(
219-
os.path.join("x509", "custom", "no_sans.pem"),
220-
x509.load_pem_x509_certificate,
221-
)
222-
223-
store = Store([leaf])
224-
validation_time = datetime.datetime.fromisoformat(
225-
"2025-04-14T00:00:00+00:00"
226-
)
227-
228-
builder = PolicyBuilder().store(store)
229-
builder = builder.time(validation_time)
230-
231-
with pytest.raises(
232-
VerificationError,
233-
match="missing required extension",
234-
):
235-
builder.build_client_verifier().verify(leaf, [])
236-
237-
builder = builder.extension_policies(
238-
ExtensionPolicy.webpki_defaults_ca(), ExtensionPolicy.permit_all()
239-
)
240-
241-
verified_client = builder.build_client_verifier().verify(leaf, [])
242-
assert verified_client.subjects is None
243-
244217

245218
class TestServerVerifier:
246219
@pytest.mark.parametrize(
@@ -553,3 +526,68 @@ def validator(*_):
553526
match="Python validator must return None.",
554527
):
555528
verifier.verify(self.leaf, [])
529+
530+
def test_no_subject_alt_name(self):
531+
leaf = _load_cert(
532+
os.path.join("x509", "custom", "no_sans.pem"),
533+
x509.load_pem_x509_certificate,
534+
)
535+
536+
store = Store([leaf])
537+
validation_time = datetime.datetime.fromisoformat(
538+
"2025-04-14T00:00:00+00:00"
539+
)
540+
541+
builder = PolicyBuilder().store(store)
542+
builder = builder.time(validation_time)
543+
544+
with pytest.raises(
545+
VerificationError,
546+
match="missing required extension",
547+
):
548+
builder.build_client_verifier().verify(leaf, [])
549+
with pytest.raises(
550+
VerificationError,
551+
match="missing required extension",
552+
):
553+
builder.build_server_verifier(DNSName("example.com")).verify(
554+
leaf, []
555+
)
556+
557+
builder = builder.extension_policies(
558+
ExtensionPolicy.webpki_defaults_ca(), ExtensionPolicy.permit_all()
559+
)
560+
561+
verified_client = builder.build_client_verifier().verify(leaf, [])
562+
assert verified_client.subjects is None
563+
564+
# For ServerVerifier, SAN must be present,
565+
# even if the ExtensionPolicy permits it's absence.
566+
with pytest.raises(
567+
VerificationError,
568+
match="leaf server certificate has no subjectAltName",
569+
):
570+
# SAN must be present for ServerVerifier,
571+
#
572+
builder.build_server_verifier(DNSName("example.com")).verify(
573+
leaf, []
574+
)
575+
576+
def test_wrong_subject_alt_name(self):
577+
builder = PolicyBuilder().store(self.store)
578+
builder = builder.time(self.validation_time)
579+
builder = builder.extension_policies(
580+
ExtensionPolicy.webpki_defaults_ca(), ExtensionPolicy.permit_all()
581+
)
582+
583+
builder.build_client_verifier().verify(self.leaf, [])
584+
585+
# For ServerVerifier, SAN must be matched against the subject
586+
# even if the extension policy permits any SANs.
587+
with pytest.raises(
588+
VerificationError,
589+
match="leaf certificate has no matching subjectAltName",
590+
):
591+
builder.build_server_verifier(DNSName("wrong.io")).verify(
592+
self.leaf, []
593+
)

0 commit comments

Comments
 (0)