New issue
Advanced search Search tips
Starred by 20 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 17
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug


Sign in to add a comment
link

Issue 347402: Use BoringSSL asynchronous certificate verification callback

Reported by rsleevi@chromium.org, Feb 27 2014 Project Member

Issue description

Currently, OpenSSL supports asynchronous retrieval of client certificates. However, it expects certificate verification to happen synchronously.

We should update the OpenSSL interface to support (but not require) asynchronous certificate validation as part of the SSL state machine, using a similar mechanism to the existing callbacks for client certs and origin bound certs.
 

Comment 1 by rsleevi@chromium.org, Feb 27 2014

Blocking: chromium:338883

Comment 2 by rsleevi@chromium.org, Apr 18 2014

Labels: M-38
Owner: davidben@chromium.org

Comment 3 by lafo...@chromium.org, Aug 25 2015

Status: Archived
Slated to an old branch and no activity in the bug in over a year suggest that this can be archived.

Comment 4 by davidben@chromium.org, Jul 26 2016

Labels: -M-38
Owner: ----
Status: Available (was: Archived)
Summary: Patch BoringSSL to support asynchronous certificate verification (was: Patch OpenSSL to support asynchronous certificate verification)
This is still a thing we'd like to get to at some point.

Comment 5 by davidben@chromium.org, Dec 21 2016

Blocking: 630147
We'll want this for TLS 1.3. Specifically, the certificate should be validated before we prompt on CertificateRequest.

That we don't do this in TLS 1.2 sounds bad, but it doesn't matter because there's nothing binding the certificate to the CertificateRequest at this point in TLS 1.2 anyway! (So the hostname field in client cert prompts are always a lie. On top of that, since the selected cert is sent in the clear, we're also lying that the certificate will only be revealed to the target host. Client certs in TLS 1.2 are somewhat a mess.)

In TLS 1.3, however, the CertificateVerify signature does cover CertificateRequest and the handshake is encrypted, so there is a reason to do the operations in the other.

Linking this to the TLS 1.3 tracking bug, though it's not a hard blocker for shipping anything because it's not a regression relative to TLS 1.2. Just something we should do.

Comment 6 by davidben@chromium.org, Jul 16 2017

Owner: davidben@chromium.org
Status: Started (was: Available)
https://boringssl-review.googlesource.com/c/17964/

Comment 7 by bugdroid1@chromium.org, Jul 17 2017

Project Member
The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl/+/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6

commit 3a1dd46e4e902ce669616a9bb5f54a9e7c801df6
Author: David Benjamin <davidben@google.com>
Date: Mon Jul 17 20:55:23 2017

Add async certificate verification callback.

This also serves as a certificate verification callback for
CRYPTO_BUFFER-based consumers. Remove the silly
SSL_CTX_i_promise_to_verify_certs_after_the_handshake placeholder.

Bug:  54 ,  chromium:347402 
Change-Id: I4c6b445cb9cd7204218acb2e5d1625e6f37aff6f
Reviewed-on: https://boringssl-review.googlesource.com/17964
Reviewed-by: David Benjamin <davidben@google.com>

[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/handshake_client.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/ssl_test.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/test/bssl_shim.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/tls_method.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/internal.h
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/include/openssl/ssl.h
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/tls13_server.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/tls13_client.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/ssl_x509.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/test/runner/runner.go
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/ssl_lib.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/s3_both.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/test/test_config.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/test/test_config.h
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/include/openssl/ssl3.h
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/handshake_server.cc
[modify] https://crrev.com/3a1dd46e4e902ce669616a9bb5f54a9e7c801df6/ssl/tls13_both.cc

Comment 8 by davidben@chromium.org, Jul 17 2017

Summary: Use BoringSSL asynchronous certificate verification callback (was: Patch BoringSSL to support asynchronous certificate verification)
Alright, the callback exists now, though we still need to make SSLClientSocketImpl use it. Renaming the bug since the bugs blocking on it were assuming this was actually the Chromium-side work anyway.

It's not quite as straight-forward as using the callback though since Chromium currently re-verifies the certificate on resumption. We'll need to either mess around with that slightly, get the same effects differently (arguably this should kick in before offering the session and the same re-checks applied on socket reuse), or change BoringSSL to do the same.

Comment 9 by davidben@chromium.org, Nov 10 2017

Blocking: -338883

Comment 10 by davidben@chromium.org, Dec 5 2017

Blocking: 792238

Comment 11 by bugdroid1@chromium.org, May 14 2018

Project Member
The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl/+/5b220ee70df2de898ec55e79a6824e85479fe1c5

commit 5b220ee70df2de898ec55e79a6824e85479fe1c5
Author: David Benjamin <davidben@google.com>
Date: Mon May 14 19:10:48 2018

Add APIs to query authentication properties of SSL_SESSIONs.

This is so Chromium can verify the session before offering it, rather
than doing it after the handshake (at which point it's too late to punt
the session) as we do today. This should, in turn, allow us to finally
verify certificates off a callback and order it correctly relative to
CertificateRequest in TLS 1.3.

(It will also order "correctly" in TLS 1.2, but this is useless. TLS 1.2
does not bind the CertificateRequest to the certificate at the point the
client needs to act on it.)

Bug:  chromium:347402 
Change-Id: I0daac2868c97b820aead6c3a7e4dc30d8ba44dc4
Reviewed-on: https://boringssl-review.googlesource.com/28405
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>

[modify] https://crrev.com/5b220ee70df2de898ec55e79a6824e85479fe1c5/ssl/ssl_session.cc
[modify] https://crrev.com/5b220ee70df2de898ec55e79a6824e85479fe1c5/include/openssl/ssl.h

Comment 12 by davidben@chromium.org, Jun 25 2018

Blocking: 795089

Comment 13 by davidben@chromium.org, Jul 2 2018

Blocking: 13934

Comment 14 by davidben@chromium.org, Aug 10

Owner: jselover@chromium.org
Jesse's working on this now.

Comment 15 by bugdroid1@chromium.org, Aug 10

Project Member
The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl/+/1c337e566d98a46798fe76c170c35e8c1a3ca6ea

commit 1c337e566d98a46798fe76c170c35e8c1a3ca6ea
Author: Jesse Selover <jselover@google.com>
Date: Fri Aug 10 20:06:22 2018

Option to reverify certs on resumption.

Works in the 1.3 and 1.2 client handshakes, not implemented on the
server for now.
Creates an SSL_CTX option to reverify the server certificate on session
resumption. Reverification only runs the client's certificate verify callback.
Adds new states to the client handshakes: state_reverify_server_certificate in
TLS 1.2, and state_server_certificate_reverify in TLS 1.3.
Adds a negative test to make sure that by default we don't verify the
certificate on resumption, and positive tests that make sure we do when the
new option is set.

Change-Id: I3a47ff3eacb3099df4db4c5bc57f7c801ceea8f1
Bug:  chromium:347402 
Reviewed-on: https://boringssl-review.googlesource.com/29984
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

[modify] https://crrev.com/1c337e566d98a46798fe76c170c35e8c1a3ca6ea/ssl/handshake_client.cc
[modify] https://crrev.com/1c337e566d98a46798fe76c170c35e8c1a3ca6ea/ssl/internal.h
[modify] https://crrev.com/1c337e566d98a46798fe76c170c35e8c1a3ca6ea/ssl/tls13_client.cc
[modify] https://crrev.com/1c337e566d98a46798fe76c170c35e8c1a3ca6ea/ssl/test/runner/runner.go
[modify] https://crrev.com/1c337e566d98a46798fe76c170c35e8c1a3ca6ea/ssl/ssl_lib.cc
[modify] https://crrev.com/1c337e566d98a46798fe76c170c35e8c1a3ca6ea/include/openssl/ssl.h
[modify] https://crrev.com/1c337e566d98a46798fe76c170c35e8c1a3ca6ea/ssl/test/test_config.cc
[modify] https://crrev.com/1c337e566d98a46798fe76c170c35e8c1a3ca6ea/ssl/test/test_config.h
[modify] https://crrev.com/1c337e566d98a46798fe76c170c35e8c1a3ca6ea/ssl/handshake.cc

Comment 17 by bugdroid1@chromium.org, Nov 16

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b394dda5873218dc240b018429c9908fb8ce96f

commit 7b394dda5873218dc240b018429c9908fb8ce96f
Author: Jesse Selover <jselover@chromium.org>
Date: Fri Nov 16 22:22:17 2018

Roll src/third_party/boringssl/src e6eef1ca1..f241a59dc

https://boringssl.googlesource.com/boringssl/+log/e6eef1ca16a022e476bbaedffef044597cfc8f4b..f241a59dcca617c5b9d9880a8a9fd92996a654be

The following commits have Chromium bugs associated:
  f241a59dc In 0RTT mode, reverify the server certificate before sending early data.

Bug:  347402 
Change-Id: I3c4e186cf94b8338cea5fa8ec0000d960923500f
Reviewed-on: https://chromium-review.googlesource.com/c/1340825
Commit-Queue: Jesse Selover <jselover@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608988}
[modify] https://crrev.com/7b394dda5873218dc240b018429c9908fb8ce96f/DEPS

Comment 18 by bugdroid1@chromium.org, Dec 3

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/daf8790ae7984fbe1917c8446a7c289cff533b1c

commit daf8790ae7984fbe1917c8446a7c289cff533b1c
Author: Jesse Selover <jselover@chromium.org>
Date: Mon Dec 03 20:44:30 2018

Replace ignore_cert_errors with an SSLConfig-level bool.

ignore_certificate_errors in HttpNetworkSession, which is set by the
--ignore-certificate-errors command-line flag, is currently
implemented by continuing to use the SSLClientSocket after Connect()
fails with a certificate error. Since we currently don't verify until
after the handshake, it's safe to ignore the errors and we still allow
Read() and Write(). At higher levels, we map the certificate errors
back to OK.

This is confusing and will longer work when certificate verification
happens inside the handshake. A certificate error will mean the
handshake hasn't completed. Instead, route the boolean into
SSLClientSocketImpl and map the error to OK there. This allows us to
remove the error-mapping logic at each of the higher levels.

Bug:  347402 
Change-Id: I7318e7e9d9e0a3cb0287555b3fd24c9347cc9821
Reviewed-on: https://chromium-review.googlesource.com/c/1343054
Commit-Queue: Jesse Selover <jselover@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613232}
[modify] https://crrev.com/daf8790ae7984fbe1917c8446a7c289cff533b1c/net/http/http_network_session.cc
[modify] https://crrev.com/daf8790ae7984fbe1917c8446a7c289cff533b1c/net/http/http_proxy_client_socket_wrapper.cc
[modify] https://crrev.com/daf8790ae7984fbe1917c8446a7c289cff533b1c/net/http/http_stream_factory_job.cc
[modify] https://crrev.com/daf8790ae7984fbe1917c8446a7c289cff533b1c/net/http/http_stream_factory_job.h
[modify] https://crrev.com/daf8790ae7984fbe1917c8446a7c289cff533b1c/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/daf8790ae7984fbe1917c8446a7c289cff533b1c/net/socket/ssl_client_socket_pool.cc
[modify] https://crrev.com/daf8790ae7984fbe1917c8446a7c289cff533b1c/net/socket/ssl_client_socket_unittest.cc
[modify] https://crrev.com/daf8790ae7984fbe1917c8446a7c289cff533b1c/net/ssl/ssl_config.cc
[modify] https://crrev.com/daf8790ae7984fbe1917c8446a7c289cff533b1c/net/ssl/ssl_config.h

Comment 19 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01529f01a2a085773355c0e38d826f22e28163ba

commit 01529f01a2a085773355c0e38d826f22e28163ba
Author: Matt Menke <mmenke@chromium.org>
Date: Fri Dec 14 18:13:01 2018

Remove SSLSocketParams::ignore_certificate_errors

As of https://chromium-review.googlesource.com/c/chromium/src/+/1343054,
it's no longer respected.

Bug:  347402 
Change-Id: I3e964d5384a4adc20d121a56882b27197a85c49f
Reviewed-on: https://chromium-review.googlesource.com/c/1378241
Reviewed-by: David Benjamin <davidben@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616745}
[modify] https://crrev.com/01529f01a2a085773355c0e38d826f22e28163ba/net/http/http_proxy_client_socket_pool_unittest.cc
[modify] https://crrev.com/01529f01a2a085773355c0e38d826f22e28163ba/net/http/http_proxy_client_socket_wrapper_unittest.cc
[modify] https://crrev.com/01529f01a2a085773355c0e38d826f22e28163ba/net/http/http_stream_factory_unittest.cc
[modify] https://crrev.com/01529f01a2a085773355c0e38d826f22e28163ba/net/socket/client_socket_pool_manager.cc
[modify] https://crrev.com/01529f01a2a085773355c0e38d826f22e28163ba/net/socket/ssl_client_socket_pool.cc
[modify] https://crrev.com/01529f01a2a085773355c0e38d826f22e28163ba/net/socket/ssl_client_socket_pool.h
[modify] https://crrev.com/01529f01a2a085773355c0e38d826f22e28163ba/net/socket/ssl_client_socket_pool_unittest.cc
[modify] https://crrev.com/01529f01a2a085773355c0e38d826f22e28163ba/net/spdy/spdy_test_util_common.cc
[modify] https://crrev.com/01529f01a2a085773355c0e38d826f22e28163ba/net/websockets/websocket_basic_stream_adapters_test.cc

Comment 20 by bugdroid1@chromium.org, Dec 17

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/afd8eed5bc579a35f6e530d9b588abf3220ecef7

commit afd8eed5bc579a35f6e530d9b588abf3220ecef7
Author: David Benjamin <davidben@chromium.org>
Date: Mon Dec 17 19:45:43 2018

Add some tests for asynchronous CertVerifiers.

Fill some of these in in preparation for the implementation changing.
Test that both success and failure may be reported asynchronously, and
also test that the CertVerifier::Request machinery works.

Bug:  347402 
Change-Id: I8f68c1f5be9409ee8643ac8bc951ebf7e1fd12c3
Reviewed-on: https://chromium-review.googlesource.com/c/1372338
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617190}
[modify] https://crrev.com/afd8eed5bc579a35f6e530d9b588abf3220ecef7/net/cert/mock_cert_verifier.cc
[modify] https://crrev.com/afd8eed5bc579a35f6e530d9b588abf3220ecef7/net/cert/mock_cert_verifier.h
[modify] https://crrev.com/afd8eed5bc579a35f6e530d9b588abf3220ecef7/net/socket/ssl_client_socket_unittest.cc

Comment 21 by bugdroid1@chromium.org, Jan 16

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/94c9a948334f6d5d910932ba74ce1316c199de04

commit 94c9a948334f6d5d910932ba74ce1316c199de04
Author: Jesse Selover <jselover@chromium.org>
Date: Wed Jan 16 01:18:04 2019

Use the BoringSSL callback for certificate verification.

This moves certificate verification to within the handshake, instead of
a separate step afterwards, which allows us to verify the certificate
before prompting for client certificates.

It also means that certificate errors result in incomplete handshakes,
so this also changes SSLClientSocket unit tests not to expect connected
sockets after certificate errors.

Bug:  347402 
Change-Id: I0a93da1dee5be697fa7d5c74aae206d370f97d5b
Reviewed-on: https://chromium-review.googlesource.com/c/1259123
Commit-Queue: Jesse Selover <jselover@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622963}
[modify] https://crrev.com/94c9a948334f6d5d910932ba74ce1316c199de04/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/94c9a948334f6d5d910932ba74ce1316c199de04/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/94c9a948334f6d5d910932ba74ce1316c199de04/net/socket/ssl_client_socket_impl.h
[modify] https://crrev.com/94c9a948334f6d5d910932ba74ce1316c199de04/net/socket/ssl_client_socket_unittest.cc
[modify] https://crrev.com/94c9a948334f6d5d910932ba74ce1316c199de04/net/test/android/javatests/src/org/chromium/net/test/util/WebServer.java
[modify] https://crrev.com/94c9a948334f6d5d910932ba74ce1316c199de04/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/94c9a948334f6d5d910932ba74ce1316c199de04/remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc

Comment 22 by davidben@chromium.org, Jan 16

\o/

Thanks, Jesse!

Comment 23 by davidben@chromium.org, Jan 17

Status: Fixed (was: Started)

Comment 24 by davidben@chromium.org, Jan 17

Labels: M-73

Sign in to add a comment