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

Issue 2725683002: Move name matching into the shared certificate validator (Closed)

Created:
3 years, 9 months ago by Ryan Sleevi
Modified:
3 years, 9 months ago
Reviewers:
mattm
CC:
chromium-reviews, cbentzel+watch_chromium.org, mac-reviews_chromium.org, net-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move name matching into the shared certificate validator In the way-old times, Chrome used to rely on the underlying OS to perform certificate name matching. Unfortunately, this meant that certificates that worked in Chrome on one platform wouldn't work in Chrome on another platform. As a result, a long time ago (e.g. more than two years ago), Chrome moved to a common name validator, which ensured that IPv6 was handled correctly, as were things like trailing dots in the hostname (used to bypass DNS suffix search). This cleans up the historical remants of that, by moving name matching for certificates into a single place, in CertVerifyProc::Verify, rather than the per-platform implementations in CertVerifyProc::VerifyInternal. The upside is this reduces code duplication. The downside is this means that MockCertVerifyProc's now need to supply certificates that *really* match their intended hostname, and can no longer fake it til they make it. BUG=308330 Review-Url: https://codereview.chromium.org/2725683002 Cr-Commit-Position: refs/heads/master@{#453735} Committed: https://chromium.googlesource.com/chromium/src/+/b88c4386cdc3ea8c18d1bfd9cbaf5017141e83f4

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -132 lines) Patch
M net/cert/cert_verify_proc.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M net/cert/cert_verify_proc_android.cc View 1 chunk +0 lines, -5 lines 0 comments Download
M net/cert/cert_verify_proc_ios.cc View 1 chunk +0 lines, -6 lines 0 comments Download
M net/cert/cert_verify_proc_mac.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M net/cert/cert_verify_proc_nss.cc View 1 chunk +0 lines, -5 lines 0 comments Download
M net/cert/cert_verify_proc_openssl.cc View 1 chunk +0 lines, -5 lines 0 comments Download
M net/cert/cert_verify_proc_unittest.cc View 4 chunks +25 lines, -43 lines 0 comments Download
M net/cert/cert_verify_proc_win.cc View 1 chunk +0 lines, -7 lines 0 comments Download
M net/data/ssl/certificates/reject_intranet_hosts.pem View 1 chunk +53 lines, -56 lines 0 comments Download
M net/data/ssl/scripts/ee.cnf View 1 chunk +3 lines, -0 lines 0 comments Download
M net/data/ssl/scripts/generate-test-certs.sh View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 12 (8 generated)
Ryan Sleevi
Matt, I realized https://codereview.chromium.org/2719273002/ was too unwieldy, so I split this out into its own ...
3 years, 9 months ago (2017-02-28 19:50:47 UTC) #2
mattm
lgtm
3 years, 9 months ago (2017-02-28 21:14:32 UTC) #7
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/2725683002/1
3 years, 9 months ago (2017-02-28 22:21:21 UTC) #9
commit-bot: I haz the power
3 years, 9 months ago (2017-02-28 23:20:06 UTC) #12
Message was sent while issue was closed.
Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/b88c4386cdc3ea8c18d1bfd9cbaf...

Powered by Google App Engine
This is Rietveld 408576698