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

Issue 1923433002: Certificate path builder for new certificate verification library (Closed)

Created:
4 years, 8 months ago by mattm
Modified:
4 years, 5 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Certificate path builder for new certificate verification library Builds paths from a target certificate to a trust anchor and attempts to verify them according to RFC 5280. Supports asynchronous intermediate lookups (eg, AIA fetching) and backtracking. This implementation uses a depth-first strategy which is simple and uses minimal resources, but may not be optimal. BUG=410574 Committed: https://crrev.com/76f636098ccda9f283614545352b4964e7ec2b5c Cr-Commit-Position: refs/heads/master@{#404239}

Patch Set 1 : . #

Total comments: 6

Patch Set 2 : rebase #

Patch Set 3 : wip: refcount CertThing, replace VerifyCertificateChain, move a bit more parsing into CertThing, etc #

Patch Set 4 : wip: Make CertPathIter build the full path including the trust anchor #

Total comments: 11

Patch Set 5 : wip: avoid net error codes, async issuer callback change, change how CertSources are supplied, misc… #

Patch Set 6 : wip: IsTrustedCertificate compares Name+SPKI, not full cert DER #

Patch Set 7 : cast compile fix #

Patch Set 8 : misc comments #

Patch Set 9 : rebase #

Patch Set 10 : split CertSource, CertSourceStatic, CertThing, TrustStore into separate files #

Patch Set 11 : compile fix, UAF fix #

Patch Set 12 : misc cleanups, win_clang compile fix? #

Patch Set 13 : merge the rest of FullyParsedCert into CertThing #

Patch Set 14 : CertIssuersIter shouldn't return the same issuer twice, misc cleanups #

Patch Set 15 : misc #

Patch Set 16 : rebase #

Patch Set 17 : remove ParsedCertificate struct, rename CertThing to ParsedCertificate #

Patch Set 18 : update and rebase on top of https://codereview.chromium.org/2030693002/ #

Patch Set 19 : remove VerifyCertificateChain #

Patch Set 20 : rebase on 2036033002 #

Patch Set 21 : Refactoring verify_certificate_chain_unittest #

Patch Set 22 : restore verify_certificate_chain_pkits_unittest #

Patch Set 23 : rebase #

Total comments: 58

Patch Set 24 : changes for comment #12 #

Patch Set 25 : rebase #

Patch Set 26 : rebase #

Total comments: 26

Patch Set 27 : changes for comment #16 #

Total comments: 32

Patch Set 28 : rebase #

Patch Set 29 : changes for review comment #20 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4048 lines, -452 lines) Patch
M components/cast_certificate/cast_cert_validator.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +3 lines, -3 lines 0 comments Download
M components/cast_certificate/cast_cert_validator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 3 chunks +31 lines, -36 lines 0 comments Download
M net/cert/internal/cert_issuer_source.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +3 lines, -5 lines 0 comments Download
M net/cert/internal/cert_issuer_source_aia.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +2 lines, -3 lines 0 comments Download
M net/cert/internal/cert_issuer_source_aia.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +3 lines, -5 lines 0 comments Download
M net/cert/internal/cert_issuer_source_aia_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -1 line 0 comments Download
M net/cert/internal/cert_issuer_source_static.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +2 lines, -3 lines 0 comments Download
M net/cert/internal/cert_issuer_source_static.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +2 lines, -5 lines 0 comments Download
M net/cert/internal/cert_issuer_source_static_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 4 chunks +4 lines, -4 lines 0 comments Download
M net/cert/internal/parsed_certificate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +3 lines, -0 lines 0 comments Download
M net/cert/internal/parsed_certificate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -1 line 0 comments Download
A net/cert/internal/path_builder.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +172 lines, -0 lines 0 comments Download
A net/cert/internal/path_builder.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +628 lines, -0 lines 0 comments Download
A + net/cert/internal/path_builder_pkits_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 11 chunks +57 lines, -48 lines 0 comments Download
A net/cert/internal/path_builder_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +1108 lines, -0 lines 0 comments Download
A net/cert/internal/path_builder_verify_certificate_chain_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +56 lines, -0 lines 0 comments Download
M net/cert/internal/trust_store.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +3 lines, -5 lines 0 comments Download
M net/cert/internal/trust_store.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +8 lines, -6 lines 0 comments Download
M net/cert/internal/verify_certificate_chain.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 4 chunks +11 lines, -21 lines 0 comments Download
M net/cert/internal/verify_certificate_chain.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 4 chunks +1 line, -57 lines 0 comments Download
M net/cert/internal/verify_certificate_chain_pkits_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +8 lines, -13 lines 0 comments Download
A net/cert/internal/verify_certificate_chain_typed_unittest.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +305 lines, -0 lines 0 comments Download
M net/cert/internal/verify_certificate_chain_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +22 lines, -235 lines 0 comments Download
A net/data/verify_certificate_chain_unittest/generate-key-rollover.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +92 lines, -0 lines 0 comments Download
A net/data/verify_certificate_chain_unittest/key-rollover-longrolloverchain.pem View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +489 lines, -0 lines 0 comments Download
A net/data/verify_certificate_chain_unittest/key-rollover-newchain.pem View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +311 lines, -0 lines 0 comments Download
A net/data/verify_certificate_chain_unittest/key-rollover-oldchain.pem View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +311 lines, -0 lines 0 comments Download
A net/data/verify_certificate_chain_unittest/key-rollover-rolloverchain.pem View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +400 lines, -0 lines 0 comments Download
M net/net.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 3 chunks +11 lines, -1 line 0 comments Download

Dependent Patchsets:

Messages

Total messages: 33 (13 generated)
mattm
This is my WIP path builder code if you want to do some high-level review ...
4 years, 8 months ago (2016-04-26 02:25:04 UTC) #3
eroman
Thanks Matt! I realize this is still an early WIP, but figured I would post ...
4 years, 8 months ago (2016-04-26 19:30:41 UTC) #4
mattm
On 2016/04/26 19:30:41, eroman wrote: > Thanks Matt! > > I realize this is still ...
4 years, 8 months ago (2016-04-26 21:26:02 UTC) #5
eroman
https://codereview.chromium.org/1923433002/diff/20001/net/cert/internal/path_builder.cc File net/cert/internal/path_builder.cc (right): https://codereview.chromium.org/1923433002/diff/20001/net/cert/internal/path_builder.cc#newcode544 net/cert/internal/path_builder.cc:544: for (auto& trust_anchor : matching_anchors) { How does this ...
4 years, 8 months ago (2016-04-26 22:10:11 UTC) #6
mattm
https://codereview.chromium.org/1923433002/diff/20001/net/cert/internal/path_builder.cc File net/cert/internal/path_builder.cc (right): https://codereview.chromium.org/1923433002/diff/20001/net/cert/internal/path_builder.cc#newcode544 net/cert/internal/path_builder.cc:544: for (auto& trust_anchor : matching_anchors) { On 2016/04/26 22:10:10, ...
4 years, 8 months ago (2016-04-26 22:54:16 UTC) #7
Ryan Sleevi
Some general musings; I haven't looked as in depth as I want, but the highlevel ...
4 years, 7 months ago (2016-05-04 02:42:42 UTC) #8
mattm
https://codereview.chromium.org/1923433002/diff/80001/net/cert/internal/path_builder.h File net/cert/internal/path_builder.h (right): https://codereview.chromium.org/1923433002/diff/80001/net/cert/internal/path_builder.h#newcode24 net/cert/internal/path_builder.h:24: using IssuerCallback = base::Callback<void(CertVector)>; On 2016/05/04 02:42:42, Ryan Sleevi ...
4 years, 7 months ago (2016-05-04 23:21:16 UTC) #9
mattm
I think this is ready for another look. (Note: it shows as dependent on https://codereview.chromium.org/2036033002/ ...
4 years, 6 months ago (2016-06-08 00:31:54 UTC) #11
eroman
https://codereview.chromium.org/1923433002/diff/450001/components/cast_certificate/cast_cert_validator.cc File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1923433002/diff/450001/components/cast_certificate/cast_cert_validator.cc#newcode287 components/cast_certificate/cast_cert_validator.cc:287: for (size_t i = 1; i < input_chain.size(); ++i) ...
4 years, 6 months ago (2016-06-17 01:03:23 UTC) #12
mattm
https://codereview.chromium.org/1923433002/diff/450001/components/cast_certificate/cast_cert_validator.cc File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1923433002/diff/450001/components/cast_certificate/cast_cert_validator.cc#newcode287 components/cast_certificate/cast_cert_validator.cc:287: for (size_t i = 1; i < input_chain.size(); ++i) ...
4 years, 6 months ago (2016-06-18 04:28:56 UTC) #14
eroman
https://codereview.chromium.org/1923433002/diff/450001/net/cert/internal/path_builder.h File net/cert/internal/path_builder.h (right): https://codereview.chromium.org/1923433002/diff/450001/net/cert/internal/path_builder.h#newcode83 net/cert/internal/path_builder.h:83: void AddCertIssuerSource(CertIssuerSource* cert_issuer_source); On 2016/06/18 04:28:56, mattm wrote: > ...
4 years, 6 months ago (2016-06-20 19:54:19 UTC) #15
eroman
https://codereview.chromium.org/1923433002/diff/530001/components/cast_certificate/cast_cert_validator.cc File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1923433002/diff/530001/components/cast_certificate/cast_cert_validator.cc#newcode267 components/cast_certificate/cast_cert_validator.cc:267: bool VerifyDeviceCert(const std::vector<std::string>& certs, Can you update the documentation ...
4 years, 5 months ago (2016-06-27 19:58:52 UTC) #16
mattm
https://codereview.chromium.org/1923433002/diff/530001/components/cast_certificate/cast_cert_validator.cc File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/1923433002/diff/530001/components/cast_certificate/cast_cert_validator.cc#newcode267 components/cast_certificate/cast_cert_validator.cc:267: bool VerifyDeviceCert(const std::vector<std::string>& certs, On 2016/06/27 19:58:52, eroman wrote: ...
4 years, 5 months ago (2016-06-27 23:45:45 UTC) #19
eroman
Thanks Matt for bearing through this long review! * Can you update the CL description ...
4 years, 5 months ago (2016-07-01 23:49:29 UTC) #20
mattm
https://codereview.chromium.org/1923433002/diff/590001/components/cast_certificate/cast_cert_validator.h File components/cast_certificate/cast_cert_validator.h (right): https://codereview.chromium.org/1923433002/diff/590001/components/cast_certificate/cast_cert_validator.h#newcode58 components/cast_certificate/cast_cert_validator.h:58: // * |certs[1..n-1]| are intermediates certificates to use in ...
4 years, 5 months ago (2016-07-02 02:21:52 UTC) #23
mattm
sheretov: please review cast changes, thanks!
4 years, 5 months ago (2016-07-02 02:23:34 UTC) #25
sheretov
components/cast_certificate/ changes lgtm
4 years, 5 months ago (2016-07-07 15:38:55 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1923433002/630001
4 years, 5 months ago (2016-07-07 18:16:44 UTC) #29
commit-bot: I haz the power
Committed patchset #29 (id:630001)
4 years, 5 months ago (2016-07-07 22:05:53 UTC) #31
commit-bot: I haz the power
4 years, 5 months ago (2016-07-07 22:08:52 UTC) #33
Message was sent while issue was closed.
Patchset 29 (id:??) landed as
https://crrev.com/76f636098ccda9f283614545352b4964e7ec2b5c
Cr-Commit-Position: refs/heads/master@{#404239}

Powered by Google App Engine
This is Rietveld 408576698