Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(184)

Unified Diff: components/cast_certificate/cast_cert_validator.cc

Issue 2800993002: Add a key purpose parameter to Certificate PathBuilder. (Closed)
Patch Set: More cast comments Created 3 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: components/cast_certificate/cast_cert_validator.cc
diff --git a/components/cast_certificate/cast_cert_validator.cc b/components/cast_certificate/cast_cert_validator.cc
index ccddd3757590f8add4ee4f4fa0ff1285ae85f9fa..f91d108a846a660cb1c3547107e6c109f913ae86 100644
--- a/components/cast_certificate/cast_cert_validator.cc
+++ b/components/cast_certificate/cast_cert_validator.cc
@@ -160,19 +160,9 @@ bool GetCommonNameFromSubject(const net::der::Input& subject_tlv,
return false;
}
-// Returns true if the extended key usage list |ekus| contains client auth.
-bool HasClientAuth(const std::vector<net::der::Input>& ekus) {
- for (const auto& oid : ekus) {
- if (oid == net::ClientAuth())
- return true;
- }
- return false;
-}
-
// Checks properties on the target certificate.
//
// * The Key Usage must include Digital Signature
-// * The Extended Key Usage must include TLS Client Auth
// * May have the policy 1.3.6.1.4.1.11129.2.5.2 to indicate it
// is an audio-only device.
WARN_UNUSED_RESULT bool CheckTargetCertificate(
@@ -187,10 +177,27 @@ WARN_UNUSED_RESULT bool CheckTargetCertificate(
if (!cert->key_usage().AssertsBit(net::KEY_USAGE_BIT_DIGITAL_SIGNATURE))
return false;
- // Ensure Extended Key Usage contains client auth.
- if (!cert->has_extended_key_usage() ||
- !HasClientAuth(cert->extended_key_usage()))
- return false;
+ // TODO(delete before landing): Doug/Cast OWNERs: the behavior after my
ryanchung 2017/04/07 02:02:47 These changes look fine. I guess the clientAuth EK
+ // change is a bit different than what we had before (but more
+ // standards compliant). I don't think this will cause any problems for
+ // Cast, but want to be explicit on what changed so you can properly review:
+ //
+ // * [less strict] The EKU can now be omitted in the target
+ // certificate (will be considered a match for clientAuth per RFC
+ // 5280). Previously the extension needed to be present.
+ //
+ // * [less strict] The OID anyExtendedKeyUsage will also be considered a
+ // match for clientAuth, per RFC 5280.
+ //
+ // * [MORE strict] An extended Key Usage on an intermediate
+ // restricts the EKU on target certificate. This is NOT part of RFC
+ // 5280, however is the de-facto standard that both CryptoAPI and
+ // NSS use, and for Web PKI is now part of the Baseline
+ // Requirements. More details at
+ // https://wiki.mozilla.org/CA:CertificatePolicyV2.1#Frequently_Asked_Questions
+ // I don't expect Cast was setting an EKU on intermediates in the
+ // first place, and if they were hopefully it wasn't inconsistent
+ // with the end entity certificate.
// Check for an optional audio-only policy extension.
*policy = CastDeviceCertPolicy::NONE;
@@ -281,9 +288,9 @@ bool VerifyDeviceCertUsingCustomTrustStore(
if (!net::der::EncodeTimeAsGeneralizedTime(time, &verification_time))
return false;
net::CertPathBuilder::Result result;
- net::CertPathBuilder path_builder(target_cert.get(), trust_store,
- signature_policy.get(), verification_time,
- &result);
+ net::CertPathBuilder path_builder(
+ target_cert.get(), trust_store, signature_policy.get(), verification_time,
+ net::KeyPurpose::KEY_PURPOSE_CLIENT_AUTH, &result);
path_builder.AddCertIssuerSource(&intermediate_cert_issuer_source);
path_builder.Run();
if (!result.HasValidPath()) {
« no previous file with comments | « no previous file | components/cast_certificate/cast_crl.cc » ('j') | net/cert/internal/verify_certificate_chain.h » ('J')

Powered by Google App Engine
This is Rietveld 408576698