Skip to content

Commit 3b676cf

Browse files
authored
ServerVerifier incorrect ext. policy early fail. (#12455)
* Fail early when building ServerVerifier with ext. policy that doesn't require SAN. * Remove `validate`, move check to `new`. * Refactor test_subject_alt_name_absent_in_server_profile.
1 parent 1664aea commit 3b676cf

File tree

4 files changed

+101
-29
lines changed

4 files changed

+101
-29
lines changed

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

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -702,10 +702,11 @@ mod tests {
702702
use cryptography_x509::extensions::{BasicConstraints, Extension};
703703
use cryptography_x509::oid::BASIC_CONSTRAINTS_OID;
704704

705-
use super::{Criticality, ExtensionValidator};
705+
use super::{Criticality, ExtensionPolicy, ExtensionValidator};
706706
use crate::certificate::tests::PublicKeyErrorOps;
707707
use crate::ops::tests::{cert, v1_cert_pem};
708708
use crate::ops::{CryptoOps, VerificationCertificate};
709+
use crate::policy::extension::CertificateType;
709710
use crate::policy::{Policy, PolicyDefinition, Subject, ValidationResult};
710711
use crate::types::DNSName;
711712

@@ -764,7 +765,8 @@ mod tests {
764765
None,
765766
None,
766767
None,
767-
);
768+
)
769+
.expect("failed to create policy definition");
768770
let policy = Policy::new(&policy_def, ());
769771

770772
// Test a policy that stipulates that a given extension MUST be present.
@@ -812,7 +814,8 @@ mod tests {
812814
None,
813815
None,
814816
None,
815-
);
817+
)
818+
.expect("failed to create policy definition");
816819
let policy = Policy::new(&policy_def, ());
817820

818821
// Test a validator that stipulates that a given extension CAN be present.
@@ -852,7 +855,8 @@ mod tests {
852855
None,
853856
None,
854857
None,
855-
);
858+
)
859+
.expect("failed to create policy definition");
856860
let policy = Policy::new(&policy_def, ());
857861

858862
// Test a validator that stipulates that a given extension MUST NOT be present.
@@ -888,7 +892,8 @@ mod tests {
888892
None,
889893
None,
890894
None,
891-
);
895+
)
896+
.expect("failed to create policy definition");
892897
let policy = Policy::new(&policy_def, ());
893898

894899
// Test a present policy that stipulates that a given extension MUST be critical.
@@ -926,7 +931,8 @@ mod tests {
926931
None,
927932
None,
928933
None,
929-
);
934+
)
935+
.expect("failed to create policy definition");
930936
let policy = Policy::new(&policy_def, ());
931937

932938
// Test a maybe present validator that stipulates that a given extension MUST be critical.
@@ -950,4 +956,39 @@ mod tests {
950956
)
951957
.is_err());
952958
}
959+
960+
#[test]
961+
fn test_subject_alt_name_absent_in_server_profile() {
962+
// This certificate doesn't have a SAN extension.
963+
let cert_pem = v1_cert_pem();
964+
let cert = cert(&cert_pem);
965+
let ops = PublicKeyErrorOps {};
966+
967+
let policy_def = PolicyDefinition::server(
968+
ops,
969+
Subject::DNS(DNSName::new("example.com").unwrap()),
970+
epoch(),
971+
None,
972+
None,
973+
None,
974+
)
975+
.expect("failed to create policy definition");
976+
let policy = Policy::new(&policy_def, ());
977+
978+
let extensions = &cert
979+
.extensions()
980+
.map_err(|_| "duplicate extensions")
981+
.expect("failed to get extensions");
982+
983+
let result = ExtensionPolicy::new_permit_all().permits(
984+
&policy,
985+
CertificateType::EE,
986+
&VerificationCertificate::new(&cert, ()),
987+
&extensions,
988+
);
989+
assert_eq!(
990+
result.unwrap_err().to_string(),
991+
"leaf server certificate has no subjectAltName"
992+
);
993+
}
953994
}

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

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -247,8 +247,8 @@ impl<'a, B: CryptoOps + 'a> PolicyDefinition<'a, B> {
247247
extended_key_usage: ObjectIdentifier,
248248
ca_extension_policy: Option<ExtensionPolicy<'a, B>>,
249249
ee_extension_policy: Option<ExtensionPolicy<'a, B>>,
250-
) -> Self {
251-
Self {
250+
) -> Result<Self, &'static str> {
251+
let retval = Self {
252252
ops,
253253
max_chain_depth: max_chain_depth.unwrap_or(DEFAULT_MAX_CHAIN_DEPTH),
254254
subject,
@@ -261,7 +261,24 @@ impl<'a, B: CryptoOps + 'a> PolicyDefinition<'a, B> {
261261
.unwrap_or_else(ExtensionPolicy::new_default_webpki_ca),
262262
ee_extension_policy: ee_extension_policy
263263
.unwrap_or_else(ExtensionPolicy::new_default_webpki_ee),
264+
};
265+
266+
// NOTE: If subject is set (server profile), we do not accept
267+
// EE extension policies that allow the SAN extension to be absent.
268+
// Even without this check, `self.ee_extension_policy.permits(self, CertificateType::EE, ...)`
269+
// would fail, but we want to fail early and provide a more specific error message.
270+
if retval.subject.is_some()
271+
&& !matches!(
272+
retval.ee_extension_policy.subject_alternative_name,
273+
ExtensionValidator::Present { .. }
274+
)
275+
{
276+
return Err(
277+
"An EE extension policy used for server verification must require the subjectAltName extension to be present.",
278+
);
264279
}
280+
281+
Ok(retval)
265282
}
266283

267284
/// Create a new policy with suitable defaults for client certification
@@ -276,7 +293,7 @@ impl<'a, B: CryptoOps + 'a> PolicyDefinition<'a, B> {
276293
max_chain_depth: Option<u8>,
277294
ca_extension_policy: Option<ExtensionPolicy<'a, B>>,
278295
ee_extension_policy: Option<ExtensionPolicy<'a, B>>,
279-
) -> Self {
296+
) -> Result<Self, &'static str> {
280297
Self::new(
281298
ops,
282299
None,
@@ -297,7 +314,7 @@ impl<'a, B: CryptoOps + 'a> PolicyDefinition<'a, B> {
297314
max_chain_depth: Option<u8>,
298315
ca_extension_policy: Option<ExtensionPolicy<'a, B>>,
299316
ee_extension_policy: Option<ExtensionPolicy<'a, B>>,
300-
) -> Self {
317+
) -> Result<Self, &'static str> {
301318
Self::new(
302319
ops,
303320
Some(subject),

src/rust/src/x509/verify/mod.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ impl PolicyBuilder {
186186
None => datetime_now(py)?,
187187
};
188188

189-
let policy_definition = OwnedPolicyDefinition::new(None, |_subject| {
189+
let policy_definition = OwnedPolicyDefinition::try_new(None, |_subject| {
190190
PolicyDefinition::client(
191191
PyCryptoOps {},
192192
time,
@@ -198,7 +198,8 @@ impl PolicyBuilder {
198198
.as_ref()
199199
.map(|p| p.get().clone_inner_policy()),
200200
)
201-
});
201+
.map_err(pyo3::exceptions::PyValueError::new_err)
202+
})?;
202203

203204
let py_policy = PyPolicy {
204205
policy_definition,
@@ -242,7 +243,7 @@ impl PolicyBuilder {
242243
.expect("subject_owner for ServerVerifier can not be None"),
243244
)?;
244245

245-
Ok::<PyCryptoPolicyDefinition<'_>, pyo3::PyErr>(PolicyDefinition::server(
246+
PolicyDefinition::server(
246247
PyCryptoOps {},
247248
subject,
248249
time,
@@ -253,7 +254,8 @@ impl PolicyBuilder {
253254
self.ee_ext_policy
254255
.as_ref()
255256
.map(|p| p.get().clone_inner_policy()),
256-
))
257+
)
258+
.map_err(pyo3::exceptions::PyValueError::new_err)
257259
})?;
258260

259261
let py_policy = PyPolicy {

tests/x509/verification/test_verification.py

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -454,11 +454,19 @@ def test_custom_cb_pass(self, extension_type: Type[x509.ExtensionType]):
454454
ca_ext_policy = ExtensionPolicy.webpki_defaults_ca()
455455
ee_ext_policy = ExtensionPolicy.webpki_defaults_ee()
456456

457-
ee_ext_policy = ee_ext_policy.may_be_present(
458-
extension_type,
459-
Criticality.AGNOSTIC,
460-
self._make_validator_cb(extension_type),
461-
)
457+
if extension_type is x509.SubjectAlternativeName:
458+
# subjectAltName must be required for server verification
459+
ee_ext_policy = ee_ext_policy.require_present(
460+
extension_type,
461+
Criticality.AGNOSTIC,
462+
self._make_validator_cb(extension_type),
463+
)
464+
else:
465+
ee_ext_policy = ee_ext_policy.may_be_present(
466+
extension_type,
467+
Criticality.AGNOSTIC,
468+
self._make_validator_cb(extension_type),
469+
)
462470

463471
builder = PolicyBuilder().store(self.store)
464472
builder = builder.time(self.validation_time).max_chain_depth(16)
@@ -561,23 +569,27 @@ def test_no_subject_alt_name(self):
561569
verified_client = builder.build_client_verifier().verify(leaf, [])
562570
assert verified_client.subjects is None
563571

564-
# For ServerVerifier, SAN must be present,
565-
# even if the ExtensionPolicy permits it's absence.
572+
# Trying to build a ServerVerifier with an EE ExtensionPolicy
573+
# that doesn't require SAN extension must fail.
566574
with pytest.raises(
567-
VerificationError,
568-
match="leaf server certificate has no subjectAltName",
575+
ValueError,
576+
match=(
577+
"An EE extension policy used for server verification"
578+
" must require the subjectAltName extension to be present."
579+
),
569580
):
570-
# SAN must be present for ServerVerifier,
571-
#
572-
builder.build_server_verifier(DNSName("example.com")).verify(
573-
leaf, []
574-
)
581+
builder.build_server_verifier(DNSName("example.com"))
575582

576583
def test_wrong_subject_alt_name(self):
584+
ee_extension_policy = (
585+
ExtensionPolicy.webpki_defaults_ee().require_present(
586+
x509.SubjectAlternativeName, Criticality.AGNOSTIC, None
587+
)
588+
)
577589
builder = PolicyBuilder().store(self.store)
578590
builder = builder.time(self.validation_time)
579591
builder = builder.extension_policies(
580-
ExtensionPolicy.webpki_defaults_ca(), ExtensionPolicy.permit_all()
592+
ExtensionPolicy.webpki_defaults_ca(), ee_extension_policy
581593
)
582594

583595
builder.build_client_verifier().verify(self.leaf, [])

0 commit comments

Comments
 (0)