Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1976433002:
Add new ParsedCertificate class, move TrustStore to own file. (Closed)
|
Created:
4 years, 7 months ago by mattm Modified:
4 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, sheretov+watch_chromium.org, dougsteed+watch_chromium.org, vadimgo+watch_chromium.org, ryanchung+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@cert-parsing-remove-old-parsedcertificate Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd new ParsedCertificate class, move TrustStore to own file.
This consolidates the certificate parsing from various places in
verify_certificate_chain.cc into a single class that pre-parses all the
important information.
The relevant places are all changed to use the new ParsedCertificate
class, and TrustStore is separated into its own file.
BUG=410574
Committed: https://crrev.com/cd67ff335dfbebef70acaf5026fcd1eafef3e68f
Cr-Commit-Position: refs/heads/master@{#397863}
Patch Set 1 : . #
Total comments: 57
Patch Set 2 : changes for comments 6-7 #Patch Set 3 : more comments #
Total comments: 16
Patch Set 4 : review changes, remove unused var #Patch Set 5 : rebase #Patch Set 6 : lil cleanup #
Total comments: 2
Patch Set 7 : ScopedCheckUnreferencedCerts #
Total comments: 3
Patch Set 8 : fix cast AddTrustAnchorForTest #Patch Set 9 : rebase #
Messages
Total messages: 28 (10 generated)
Description was changed from ========== Add new ParsedCertificate class, move TrustStore to own file. This consolidates the certificate parsing from various places in verify_certificate_chain.cc into a single class that pre-parses all the important information. The relevant places are all changed to used the new ParsedCertificate class, and TrustStore is separated into its own file. BUG=410574 ========== to ========== Add new ParsedCertificate class, move TrustStore to own file. This consolidates the certificate parsing from various places in verify_certificate_chain.cc into a single class that pre-parses all the important information. The relevant places are all changed to use the new ParsedCertificate class, and TrustStore is separated into its own file. BUG=410574 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
mattm@chromium.org changed reviewers: + eroman@chromium.org, rsleevi@chromium.org
2nd part split off of https://codereview.chromium.org/1923433002/ child of https://codereview.chromium.org/1969293002/
https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.cc (left): https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:177: // TODO(eroman): Simplify this. The certificate chain verification nice, much better! https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:64: store_.AddTrustedCertificate(std::move(root)); Rather than needing "root" temporary, how about moving the CHECK into AddTrustCertificate instead? AddTrustCertificate(x) { CHECK(x); ... } I think we generally want to enforce that anyway to be safe. Also std::move() of refptr feels strange. https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:194: if (!GetExtensionValue(cert->unconsumed_extensions(), net::ExtKeyUsageOid(), This doesn't seem right, are you sure? I believe we have a direct accessor now for ext key usage which will need to be checked instead. https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:207: if (GetExtensionValue(cert->unconsumed_extensions(), Same here. https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:292: net::ParsedCertificate::DataSource::INTERNAL_COPY)); EXTERNAL_REFERENCE to match previous code. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... File net/cert/internal/parsed_certificate.cc (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.cc:47: return nullptr; nit: curly braces? https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.cc:54: // will fail on valid but unsupported signature algorithms. What current consumer is this allowance made for? Is this for parsing a trust anchor in a PKITS test? https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.cc:67: return nullptr; Same question throughout -- if we consider these multiline if or not. I have seen conflicting interpreations of this in the chromium codebase, but most //net code seems to interpret it as a multiline AFAICT. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... File net/cert/internal/parsed_certificate.h (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:14: #include "net/cert/internal/parse_certificate.h" This is a weird dependency, but I guess unavoidable right now. We will need to further smooth out the differences in these files. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:23: // XXX Rename. TODO. That said I think you can just remove this line. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:30: // NameConstraints::CreateFromDer is done. Is there maybe a more general description we can give? These aren't the right words, but something along the lines of accessors that return der::Input are unvalidated, whereas accessors that return objects have been validated. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:46: // nullptr on line wrap. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:72: bool has_supported_signature_algorithm() const { It is a bit ambiguous from the name how this relates to validity. i.e. an invalid algorithm (but of a supported type) might have been provided and has_supported_signature_algorithm() will be false. The current behavior is good, just wondering if we should name it differently. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:77: const SignatureAlgorithm& signature_algorithm() const { Add a DCHECK() ? (de-referencing wont' necessarily cause a crash, since return value is a const reference). https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:83: const std::string& normalized_subject() const { return normalized_subject_; } der::Input ? https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:86: const std::string& normalized_issuer() const { return normalized_issuer_; } der::Input ? https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:90: // Returns the ParsedBasicConstraints struct. Caller should check should --> must https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:93: return basic_constraints_; Add a CHECK or DCHECK ? https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:98: // Returns the KeyUsage BitString. Caller should check should--> must Or alternately we could guarantee that the key usage is empty bitstring when not provided? https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:116: // Returns true if the certificate had a NameConstraints extension. nit: had --> has. All of these properties correspond directly to the wrapped DER data. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:174: ExtensionsMap unconsumed_extensions_; From this layer "unconsumed" is a bit ambiguous. Maybe unparsed_extensions ? https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/trust... File net/cert/internal/trust_store.cc (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/trust... net/cert/internal/trust_store.cc:37: // First compare the ParsedCertificate pointers as an optimization, fall Do we currently use this or is it just a nice-to-have ? https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/trust... File net/cert/internal/trust_store.h (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/trust... net/cert/internal/trust_store.h:38: std::vector<scoped_refptr<ParsedCertificate>>* matches) const; Should we make an alias for this somewhere? (appears in several places). CertList, CertificateList, ParsedCertificates, etc. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/trust... net/cert/internal/trust_store.h:46: std::unordered_multimap<base::StringPiece, Sounds good, however this may be problematic if we ever support deletion from the trust store (since key may belong to a removed certificate). https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/verif... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:47: return cert.normalized_subject() == cert.normalized_issuer(); yay! https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:59: return !(time < cert.parsed_tbs().validity_not_before) && maybe we can call the function just cert.tbs() (and have like der_tbs() or something if the other is needed). https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:384: // working_issuer_name variable in RFC 5280 section 6.1.2: thanks for updating the comments throughout! https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/verif... File net/cert/internal/verify_certificate_chain_unittest.cc (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain_unittest.cc:113: net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE)); I think this is the third place using this pattern. Might be appropriate to extract to a helper
Started to go through this, but brainfried; just publishing some draft comments I had. https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:64: store_.AddTrustedCertificate(std::move(root)); On 2016/05/12 18:12:29, eroman wrote: > Rather than needing "root" temporary, how about moving the CHECK into > AddTrustCertificate instead? > > AddTrustCertificate(x) { > CHECK(x); > ... > } > > I think we generally want to enforce that anyway to be safe. Also std::move() of > refptr feels strange. Why? That's the recommended style. https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:252: for (const std::string& cert_der : certs) { fwiw, const auto& is also fine here https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:292: net::ParsedCertificate::DataSource::INTERNAL_COPY)); On 2016/05/12 18:12:29, eroman wrote: > EXTERNAL_REFERENCE to match previous code. Why would EXTERNAL_REFERENCE be safe here? |anchor| is kept past the end of this function.
https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:292: net::ParsedCertificate::DataSource::INTERNAL_COPY)); On 2016/05/12 20:41:46, Ryan Sleevi wrote: > On 2016/05/12 18:12:29, eroman wrote: > > EXTERNAL_REFERENCE to match previous code. > > Why would EXTERNAL_REFERENCE be safe here? |anchor| is kept past the end of this > function. It is safe because the function mandates those lifetimes: https://code.google.com/p/chromium/codesearch#chromium/src/components/cast_ce... We don't actually use this function in Chrome code, but Ryan (from Chromecast) wanted it for some cast tests they have. The lifetime expectations are confirmed in this code-review comment: https://codereview.chromium.org/1888913005/diff/80001/extensions/common/cast/...
https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:64: store_.AddTrustedCertificate(std::move(root)); On 2016/05/12 18:12:29, eroman wrote: > Rather than needing "root" temporary, how about moving the CHECK into > AddTrustCertificate instead? > > AddTrustCertificate(x) { > CHECK(x); > ... > } I think in this case the CHECK makes sense since it's baked in data that should never fail. In other cases we would want to more gracefully handle a failure to parse, so having the CHECK in AddTrustedCertificate is sort of extraneous. Style-wise it would probably be a DCHECK rather than a CHECK anyway. > I think we generally want to enforce that anyway to be safe. Also std::move() of > refptr feels strange. Does feel strange if you're used to only seeing it on scoped_ptr, but this allows you to pass a reference directly instead of doing an unnecessary ref/unref cycle. Obviously not that important in this case, but I think it's good to get in the habit of doing it. https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:194: if (!GetExtensionValue(cert->unconsumed_extensions(), net::ExtKeyUsageOid(), On 2016/05/12 18:12:29, eroman wrote: > This doesn't seem right, are you sure? > > I believe we have a direct accessor now for ext key usage which will need to be > checked instead. ParsedCertificate extracts KeyUsage extension, but not ExtKeyUsage. I did wonder about this in general though, if we add more extension handling natively to ParsedCertificate, any existing code which was pulling that extension from unconsumed_extensions() would break unless it was updated to use the accessor for that specific extension. I guess as long as the users are just in chromium that isn't too hard to handle. But perhaps we could add a method to DCHECK that the extension wasn't handled by ParsedCertificate, so that any code that would be broken gets a more obvious failure? Something like DCHECK(ParsedCertificate::DoesNotConsumeExtension(net::ExtKeyUsageOid()) Would be even better if it could be a compile failure, don't know if that would be possible.. Or could just keep a separate full list of the extensions (without the parsed ones removed), and then such code would continue working even if it does unnecessary re-parsing.. https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:207: if (GetExtensionValue(cert->unconsumed_extensions(), On 2016/05/12 18:12:29, eroman wrote: > Same here. CertificatePolicies isn't currently consumed by ParsedCertificate https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:252: for (const std::string& cert_der : certs) { On 2016/05/12 20:41:46, Ryan Sleevi wrote: > fwiw, const auto& is also fine here Done. https://codereview.chromium.org/1976433002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:292: net::ParsedCertificate::DataSource::INTERNAL_COPY)); On 2016/05/12 18:12:29, eroman wrote: > EXTERNAL_REFERENCE to match previous code. Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... File net/cert/internal/parsed_certificate.cc (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.cc:47: return nullptr; On 2016/05/12 18:12:29, eroman wrote: > nit: curly braces? Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.cc:54: // will fail on valid but unsupported signature algorithms. On 2016/05/12 18:12:29, eroman wrote: > What current consumer is this allowance made for? > Is this for parsing a trust anchor in a PKITS test? There are several in tests (PKITS and ours), they are all for target or intermediate cert though. Mainly it goes back to the discussion about being able to generate more usable/contextual error messages. Perhaps it's too soon to worry about that and I should just reject it here? https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.cc:67: return nullptr; On 2016/05/12 18:12:29, eroman wrote: > Same question throughout -- if we consider these multiline if or not. I have > seen conflicting interpreations of this in the chromium codebase, but most //net > code seems to interpret it as a multiline AFAICT. yeah, I always forget that. done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... File net/cert/internal/parsed_certificate.h (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:23: // XXX Rename. On 2016/05/12 18:12:29, eroman wrote: > TODO. That said I think you can just remove this line. oops, yeah. I forgot to remove that. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:30: // NameConstraints::CreateFromDer is done. On 2016/05/12 18:12:29, eroman wrote: > Is there maybe a more general description we can give? > > These aren't the right words, but something along the lines of accessors that > return der::Input are unvalidated, whereas accessors that return objects have > been validated. I'll try to come up with a better wording. In the meantime I wanted to upload the rest of my changes for review... https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:46: // nullptr on On 2016/05/12 18:12:30, eroman wrote: > line wrap. Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:72: bool has_supported_signature_algorithm() const { On 2016/05/12 18:12:29, eroman wrote: > It is a bit ambiguous from the name how this relates to validity. i.e. an > invalid algorithm (but of a supported type) might have been provided and > has_supported_signature_algorithm() will be false. > > The current behavior is good, just wondering if we should name it differently. Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:77: const SignatureAlgorithm& signature_algorithm() const { On 2016/05/12 18:12:29, eroman wrote: > Add a DCHECK() ? > (de-referencing wont' necessarily cause a crash, since return value is a const > reference). Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:83: const std::string& normalized_subject() const { return normalized_subject_; } On 2016/05/12 18:12:30, eroman wrote: > der::Input ? Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:86: const std::string& normalized_issuer() const { return normalized_issuer_; } On 2016/05/12 18:12:30, eroman wrote: > der::Input ? Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:90: // Returns the ParsedBasicConstraints struct. Caller should check On 2016/05/12 18:12:30, eroman wrote: > should --> must Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:93: return basic_constraints_; On 2016/05/12 18:12:29, eroman wrote: > Add a CHECK or DCHECK ? Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:98: // Returns the KeyUsage BitString. Caller should check On 2016/05/12 18:12:30, eroman wrote: > should--> must done. > Or alternately we could guarantee that the key usage is empty bitstring when not > provided? I think it's better to just require checking, since an empty key usage isn't equivalent to not having a key usage. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:116: // Returns true if the certificate had a NameConstraints extension. On 2016/05/12 18:12:29, eroman wrote: > nit: had --> has. > All of these properties correspond directly to the wrapped DER data. Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:174: ExtensionsMap unconsumed_extensions_; On 2016/05/12 18:12:30, eroman wrote: > From this layer "unconsumed" is a bit ambiguous. > Maybe unparsed_extensions ? Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/trust... File net/cert/internal/trust_store.cc (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/trust... net/cert/internal/trust_store.cc:37: // First compare the ParsedCertificate pointers as an optimization, fall On 2016/05/12 18:12:30, eroman wrote: > Do we currently use this or is it just a nice-to-have ? In this CL it could be used if we add the anchor to the chain in BuildSimplePathToTrustAnchor and then DCHECK(IsTrustedCertificate(..)) in VerifyCertificateChainAssumingTrustedRoot. And will be used in non-DCHECK situations in the path builder. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/trust... File net/cert/internal/trust_store.h (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/trust... net/cert/internal/trust_store.h:38: std::vector<scoped_refptr<ParsedCertificate>>* matches) const; On 2016/05/12 18:12:30, eroman wrote: > Should we make an alias for this somewhere? (appears in several places). > > CertList, CertificateList, ParsedCertificates, etc. I had one at one point, but it means you have to increase the amount of places that #include that header, or re-alias it in a bunch of different headers.. Maybe we're ok with just including parsed_certificate.h in more of these headers? https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/trust... net/cert/internal/trust_store.h:46: std::unordered_multimap<base::StringPiece, On 2016/05/12 18:12:30, eroman wrote: > Sounds good, however this may be problematic if we ever support deletion from > the trust store (since key may belong to a removed certificate). Good point, I'll keep that in mind. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/verif... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:59: return !(time < cert.parsed_tbs().validity_not_before) && On 2016/05/12 18:12:30, eroman wrote: > maybe we can call the function just cert.tbs() (and have like der_tbs() or > something if the other is needed). Done. https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/verif... File net/cert/internal/verify_certificate_chain_unittest.cc (right): https://codereview.chromium.org/1976433002/diff/40001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain_unittest.cc:113: net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE)); On 2016/05/12 18:12:30, eroman wrote: > I think this is the third place using this pattern. Might be appropriate to > extract to a helper Done.
LGTM. Sorry didn't mean to stall you -- remaining feedback can be addressed as followups. https://codereview.chromium.org/1976433002/diff/80001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/80001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:61: CHECK(root = net::ParsedCertificate::CreateFromCertificateData( nit: I prefer the style where these are separate lines. i.e "CHECK(root)" https://codereview.chromium.org/1976433002/diff/80001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:177: // * THe Extended Key Usage must includ TLS Client Auth optional: While here can fix my "THe" typo, and "includ" :) https://codereview.chromium.org/1976433002/diff/80001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:254: // No reference to the ParsedCertificate is kept past the end of this optional: You could also add weight to this by adding a DCHECK() on RefCounted::HasOneRef() upon exit. https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/parse... File net/cert/internal/parsed_certificate.h (right): https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:54: // Creates a ParsedCertificate and appends it to |chain|. Returns true if the nit: add spaces between the method declarations (throughout) https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:62: // Creates a ParsedCertificate, copying the data from |data|. same https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:93: const der::Input normalized_subject() const { optional nit: remove the "const" from return type (throughout) -- i think it obscures a bit that a copy is being returned to the casual reader, and doesn't have particularly useful API properties. https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/trust... File net/cert/internal/trust_store.cc (right): https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/trust... net/cert/internal/trust_store.cc:29: for (auto it = range.first; it != range.second; ++it) Does this work? matches->insert(matches->end(), range.first, range.second); https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/verif... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:475: certs->push_back(std::move(trust_anchors[0])); Should there be a TODO here, noting that only one match is tested?
Patchset #4 (id:100001) has been deleted
mattm@chromium.org changed reviewers: + sheretov@chromium.org
+sheretov for cast OWNERS https://codereview.chromium.org/1976433002/diff/80001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/80001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:61: CHECK(root = net::ParsedCertificate::CreateFromCertificateData( On 2016/05/31 18:28:57, eroman wrote: > nit: I prefer the style where these are separate lines. i.e "CHECK(root)" Done. https://codereview.chromium.org/1976433002/diff/80001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:177: // * THe Extended Key Usage must includ TLS Client Auth On 2016/05/31 18:28:57, eroman wrote: > optional: While here can fix my "THe" typo, and "includ" :) Done. https://codereview.chromium.org/1976433002/diff/80001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:254: // No reference to the ParsedCertificate is kept past the end of this On 2016/05/31 18:28:57, eroman wrote: > optional: You could also add weight to this by adding a DCHECK() on > RefCounted::HasOneRef() upon exit. Done. https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/parse... File net/cert/internal/parsed_certificate.h (right): https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:54: // Creates a ParsedCertificate and appends it to |chain|. Returns true if the On 2016/05/31 18:28:57, eroman wrote: > nit: add spaces between the method declarations (throughout) Done. https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:62: // Creates a ParsedCertificate, copying the data from |data|. On 2016/05/31 18:28:57, eroman wrote: > same Done. https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/parse... net/cert/internal/parsed_certificate.h:93: const der::Input normalized_subject() const { On 2016/05/31 18:28:57, eroman wrote: > optional nit: remove the "const" from return type (throughout) -- i think it > obscures a bit that a copy is being returned to the casual reader, and doesn't > have particularly useful API properties. Done. https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/trust... File net/cert/internal/trust_store.cc (right): https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/trust... net/cert/internal/trust_store.cc:29: for (auto it = range.first; it != range.second; ++it) On 2016/05/31 18:28:57, eroman wrote: > Does this work? > > matches->insert(matches->end(), range.first, range.second); That tries to insert a std::pair<StringPiece, scoped_refptr<ParsedCertificate>> into matches.. probably could be done with some <functional> magic, but doubt it would be clearer that the for loop. https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/verif... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/1976433002/diff/80001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:475: certs->push_back(std::move(trust_anchors[0])); On 2016/05/31 18:28:57, eroman wrote: > Should there be a TODO here, noting that only one match is tested? Done.
https://codereview.chromium.org/1976433002/diff/160001/components/cast_certif... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/160001/components/cast_certif... components/cast_certificate/cast_cert_validator.cc:281: // Verify that nothing saved a reference to the input certs, since the backing optional: Alternately could add this as like: ScopedCheckUnreferencedCerts check(&input_chain) so it runs checks in the failure cases too, and doesn't require uglifying the return statement here.
https://codereview.chromium.org/1976433002/diff/160001/components/cast_certif... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/160001/components/cast_certif... components/cast_certificate/cast_cert_validator.cc:281: // Verify that nothing saved a reference to the input certs, since the backing On 2016/05/31 20:00:03, eroman wrote: > optional: Alternately could add this as like: > ScopedCheckUnreferencedCerts check(&input_chain) > > so it runs checks in the failure cases too, and doesn't require uglifying the > return statement here. Done.
lgtm
sheretov: ping
Sorry - didn't realize I was on the reviewers list. https://codereview.chromium.org/1976433002/diff/180001/components/cast_certif... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/180001/components/cast_certif... components/cast_certificate/cast_cert_validator.cc:309: bool AddTrustAnchorForTest(const uint8_t* data, size_t length) { This looks strange to me. The old code was adding the passed-in cert to the trust store. The new code ignores the arguments, and instead adds the hard-coded root cert to the trust store. However, kCastRootCaDer should have already been in the trust store, based on logic in CastTrustStore::CastTrustStore(). So this entire function looks like a no-op.
https://codereview.chromium.org/1976433002/diff/180001/components/cast_certif... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/180001/components/cast_certif... components/cast_certificate/cast_cert_validator.cc:309: bool AddTrustAnchorForTest(const uint8_t* data, size_t length) { On 2016/06/03 22:08:02, sheretov wrote: > This looks strange to me. The old code was adding the passed-in cert to the > trust store. The new code ignores the arguments, and instead adds the > hard-coded root cert to the trust store. However, kCastRootCaDer should have > already been in the trust store, based on logic in > CastTrustStore::CastTrustStore(). So this entire function looks like a no-op. Agreed, this is wrong. I should have noticed during review.
https://codereview.chromium.org/1976433002/diff/180001/components/cast_certif... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1976433002/diff/180001/components/cast_certif... components/cast_certificate/cast_cert_validator.cc:309: bool AddTrustAnchorForTest(const uint8_t* data, size_t length) { On 2016/06/03 22:08:02, sheretov wrote: > This looks strange to me. The old code was adding the passed-in cert to the > trust store. The new code ignores the arguments, and instead adds the > hard-coded root cert to the trust store. However, kCastRootCaDer should have > already been in the trust store, based on logic in > CastTrustStore::CastTrustStore(). So this entire function looks like a no-op. Eek, thanks. Fixed.
lgtm
The CQ bit was checked by mattm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, sheretov@chromium.org Link to the patchset: https://codereview.chromium.org/1976433002/#ps220001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976433002/220001
Message was sent while issue was closed.
Description was changed from ========== Add new ParsedCertificate class, move TrustStore to own file. This consolidates the certificate parsing from various places in verify_certificate_chain.cc into a single class that pre-parses all the important information. The relevant places are all changed to use the new ParsedCertificate class, and TrustStore is separated into its own file. BUG=410574 ========== to ========== Add new ParsedCertificate class, move TrustStore to own file. This consolidates the certificate parsing from various places in verify_certificate_chain.cc into a single class that pre-parses all the important information. The relevant places are all changed to use the new ParsedCertificate class, and TrustStore is separated into its own file. BUG=410574 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add new ParsedCertificate class, move TrustStore to own file. This consolidates the certificate parsing from various places in verify_certificate_chain.cc into a single class that pre-parses all the important information. The relevant places are all changed to use the new ParsedCertificate class, and TrustStore is separated into its own file. BUG=410574 ========== to ========== Add new ParsedCertificate class, move TrustStore to own file. This consolidates the certificate parsing from various places in verify_certificate_chain.cc into a single class that pre-parses all the important information. The relevant places are all changed to use the new ParsedCertificate class, and TrustStore is separated into its own file. BUG=410574 Committed: https://crrev.com/cd67ff335dfbebef70acaf5026fcd1eafef3e68f Cr-Commit-Position: refs/heads/master@{#397863} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/cd67ff335dfbebef70acaf5026fcd1eafef3e68f Cr-Commit-Position: refs/heads/master@{#397863} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
