Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(13)

Issue 1080593003: Pass in a non-null CertVerifier into SSLClientSocket. (Closed)

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

Description

Pass in a non-null CertVerifier into SSLClientSocket. SslHmacChannelAuthenticator passes in a null one which crashes but the IsAllowedBadCert check, as well as inconsistent ability to use X509Certificate in the sandbox masks the issue most of the time. This also fixes FakeStreamSocket to propogate disconnects to the peer, which is needed to add a test for this case. (If SSLClientSocket doesn't like a certificate, it just ceremoniously disconnects the connection right after the handshake.) This test crashed before this CL outside the sandbox. (Inside the sandbox, it's possible that it worked on some platforms due to the sandbox breaking net::X509Certificate. I didn't do a survey.) BUG=none Committed: https://crrev.com/9bbf3290520788bfe634a286a30965cd43f4dd78 Cr-Commit-Position: refs/heads/master@{#326886}

Patch Set 1 #

Patch Set 2 : revise comment (try jobs on patch set 1) #

Total comments: 7

Patch Set 3 : fix comment typo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+100 lines, -20 lines) Patch
M net/socket/ssl_client_socket_nss.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_openssl.cc View 1 chunk +1 line, -0 lines 0 comments Download
M remoting/protocol/fake_stream_socket.h View 1 1 chunk +6 lines, -3 lines 0 comments Download
M remoting/protocol/fake_stream_socket.cc View 1 2 4 chunks +24 lines, -9 lines 0 comments Download
M remoting/protocol/message_reader_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M remoting/protocol/ssl_hmac_channel_authenticator.h View 2 chunks +2 lines, -0 lines 0 comments Download
M remoting/protocol/ssl_hmac_channel_authenticator.cc View 4 chunks +33 lines, -0 lines 0 comments Download
M remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc View 5 chunks +31 lines, -7 lines 0 comments Download

Messages

Total messages: 13 (5 generated)
davidben
https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_stream_socket.h File remoting/protocol/fake_stream_socket.h (right): https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_stream_socket.h#newcode50 remoting/protocol/fake_stream_socket.h:50: void set_next_write_error(int error) { next_write_error_ = error; } It'd ...
5 years, 9 months ago (2015-04-23 19:38:15 UTC) #2
Ryan Sleevi
//net LGTM
5 years, 9 months ago (2015-04-23 20:25:14 UTC) #3
Sergey Ulanov
https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_stream_socket.cc File remoting/protocol/fake_stream_socket.cc (right): https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_stream_socket.cc#newcode55 remoting/protocol/fake_stream_socket.cc:55: // Complete pending ead if any. typo: ead https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/ssl_hmac_channel_authenticator.cc ...
5 years, 9 months ago (2015-04-24 00:34:45 UTC) #4
Sergey Ulanov
LGTM after offline discussion
5 years, 9 months ago (2015-04-24 20:03:22 UTC) #5
davidben
https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_stream_socket.cc File remoting/protocol/fake_stream_socket.cc (right): https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_stream_socket.cc#newcode55 remoting/protocol/fake_stream_socket.cc:55: // Complete pending ead if any. On 2015/04/24 00:34:45, ...
5 years, 9 months ago (2015-04-24 20:52:44 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1080593003/40001
5 years, 9 months ago (2015-04-24 20:54:47 UTC) #11
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 9 months ago (2015-04-24 21:50:15 UTC) #12
commit-bot: I haz the power
5 years, 9 months ago (2015-04-24 21:51:03 UTC) #13
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/9bbf3290520788bfe634a286a30965cd43f4dd78
Cr-Commit-Position: refs/heads/master@{#326886}

Powered by Google App Engine
This is Rietveld 408576698