Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8817021:
Add a new SSLClientSocket::was_origin_cert_sent() method (Closed)
|
Created:
9 years ago by Ryan Hamilton Modified:
9 years ago Reviewers:
wtc CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd a new method to SSLClientSocket:
was_origin_cert_sent()
This will help SpdySession decide to send a CREDENTIAL frame.
BUG=106103
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113409
Patch Set 1 : '' #
Total comments: 12
Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 4
Patch Set 5 : '' #
Messages
Total messages: 13 (0 generated)
Hi Wan-Teh, Here is a mostly trivial CL that is a first step in supporting the SPDY CREDENTIAL frame.
LGTM. High-level comments 1. Please decide if it is enough to report whether the origin-bound cert extension was negotiated, or we should report the stronger condition that an origin-bound cert was sent. 2. If we just need to report if the extension was negotiated, then the existing static method OriginBoundCertNegotiated in SSLClientSocketNSS should be converted to the new, non-static method. http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket.h File net/socket/ssl_client_socket.h (right): http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket.h... net/socket/ssl_client_socket.h:127: virtual bool WasOriginBoundCertNegotiated() = 0; This method should report the stronger condition: whether an origin-bound cert was sent. This is because the server may echo the origin-bound cert extension in ServerHello but not request an origin-bound cert. So I suggest renaming this method WasOriginBoundCertSent and updating the comment accordingly. Nit: it may be a good idea to explain what this method is intended for. Note: an alternative API design would be a generic method that returns the client certificate sent to the server, and then we inspect the client certificate to determine whether it is a regular client certificate or origin-bound certificate. But let's go with the simpler OBC-specific method now. It is possible that it is enough to simply report whether the origin-bound cert extension was negoriated. http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket.h... net/socket/ssl_client_socket.h:139: virtual bool was_spdy_negotiated() const; Can you find out why was_npn_negotiated() and was_spdy_negotiated() are named in all lowercase and not abstract methods (= 0)? http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_m... File net/socket/ssl_client_socket_mac.cc (right): http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_m... net/socket/ssl_client_socket_mac.cc:801: // TODO(rch): support Origin Bound Certificates. Remove this TODO. SSLClientSocketMac uses Mac OS X's Secure Transport library, which we can't modify to support new TLS extensions. http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_n... File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_n... net/socket/ssl_client_socket_nss.cc:565: return OriginBoundCertNegotiated(nss_fd_); As I noted in ssl_client_socket.h, I believe this method should report the stronger condition that an origin-bound cert was sent, so it can't simply call the OriginBoundCertNegotiated method. If we decide that it is acceptable to report whether the origin-bound cert extension was negotiated, then we should remove the static method OriginBoundCertNegotiated and move its code to this new method. http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_w... File net/socket/ssl_client_socket_win.cc (right): http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_w... net/socket/ssl_client_socket_win.cc:564: // TODO(rch): support Origin Bound Certificates. Remove this TODO. SSLClientSocketWin uses the Windows SChannel library, which we can't modify to support new TLS extensions.
I switched to using a local was_origin_bound_cert_sent_ member field of SSLClientSocket (ala was_spdy_negotiated_) which simplified the CL considerably. http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket.h File net/socket/ssl_client_socket.h (right): http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket.h... net/socket/ssl_client_socket.h:127: virtual bool WasOriginBoundCertNegotiated() = 0; On 2011/12/06 19:22:11, wtc wrote: > > This method should report the stronger condition: whether an > origin-bound cert was sent. This is because the server may > echo the origin-bound cert extension in ServerHello but not > request an origin-bound cert. I thought that if the OBC extension was negotiated, the server MUST request an origin bound certificate. I guess that's not the case. > So I suggest renaming this method WasOriginBoundCertSent > and updating the comment accordingly. Done. > Nit: it may be a good idea to explain what this method is > intended for. Done. > Note: an alternative API design would be a generic method that > returns the client certificate sent to the server, and then > we inspect the client certificate to determine whether it is > a regular client certificate or origin-bound certificate. > But let's go with the simpler OBC-specific method now. *nod* > It is possible that it is enough to simply report whether > the origin-bound cert extension was negoriated. This is an interesting distinction that I'm surprised to learn exists :> But if it does, I think your proposal is probably what we want. http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket.h... net/socket/ssl_client_socket.h:139: virtual bool was_spdy_negotiated() const; On 2011/12/06 19:22:11, wtc wrote: > > Can you find out why was_npn_negotiated() and was_spdy_negotiated() > are named in all lowercase and not abstract methods (= 0)? This is because these methods are simple getters for the local members: was_npn_negotiated_; was_spdy_negotiated_; I guess you're pointing out that we could also add a member: was_origin_bound_cert_sent_ and then name the method was_origin_bound_cert_sent(). I guess this would be more appropriate? I'll do this instead. http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_m... File net/socket/ssl_client_socket_mac.cc (right): http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_m... net/socket/ssl_client_socket_mac.cc:801: // TODO(rch): support Origin Bound Certificates. On 2011/12/06 19:22:11, wtc wrote: > > Remove this TODO. SSLClientSocketMac uses Mac OS X's > Secure Transport library, which we can't modify to support > new TLS extensions. Done. http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_n... File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_n... net/socket/ssl_client_socket_nss.cc:565: return OriginBoundCertNegotiated(nss_fd_); On 2011/12/06 19:22:11, wtc wrote: > > As I noted in ssl_client_socket.h, I believe this method > should report the stronger condition that an origin-bound > cert was sent, so it can't simply call the > OriginBoundCertNegotiated method. > > If we decide that it is acceptable to report whether the > origin-bound cert extension was negotiated, then we should > remove the static method OriginBoundCertNegotiated and > move its code to this new method. Done. (FWIW, this static method is called by the static method PlatformClientAuthHandler so I chose to keep it static in the earlier CL) http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_w... File net/socket/ssl_client_socket_win.cc (right): http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_w... net/socket/ssl_client_socket_win.cc:564: // TODO(rch): support Origin Bound Certificates. On 2011/12/06 19:22:11, wtc wrote: > > Remove this TODO. SSLClientSocketWin uses the Windows SChannel > library, which we can't modify to support new TLS extensions. Done.
Patch Set 4 LGTM. There is a bug (see my last comment below). http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket.h File net/socket/ssl_client_socket.h (right): http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket.h... net/socket/ssl_client_socket.h:139: virtual bool was_spdy_negotiated() const; On 2011/12/06 20:03:47, Ryan Hamilton wrote: > > I guess you're pointing out that we could also add a member: > > was_origin_bound_cert_sent_ > > and then name the method was_origin_bound_cert_sent(). > > I guess this would be more appropriate? I'll do this instead. I just wanted you to find out why was_npn_negotiated() and was_spdy_negotiated() are declared and defined that way. I am not sure which way is better, but it'd be nice to standardize on one way. http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_n... File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/8817021/diff/26/net/socket/ssl_client_socket_n... net/socket/ssl_client_socket_nss.cc:565: return OriginBoundCertNegotiated(nss_fd_); On 2011/12/06 20:03:47, Ryan Hamilton wrote: > (FWIW, this static method is called by the static method > PlatformClientAuthHandler so I chose to keep it static in the earlier CL) I see. PlatformClientAuthHandler is a static method only because it is called by C code as a C function. Once we enter PlatformClientAuthHandler, it can obtain the object in question and call non-static methods on that object. http://codereview.chromium.org/8817021/diff/13001/net/socket/ssl_client_socket.h File net/socket/ssl_client_socket.h (right): http://codereview.chromium.org/8817021/diff/13001/net/socket/ssl_client_socke... net/socket/ssl_client_socket.h:162: // True if an Origin Bound Certificate was sent. Please use lowercase for "origin-bound certificate". http://codereview.chromium.org/8817021/diff/13001/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/8817021/diff/13001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_nss.cc:2190: rv = SECFailure; We also need to call set_was_origin_bound_cert_sent(true) if result == OK.
Thanks! http://codereview.chromium.org/8817021/diff/13001/net/socket/ssl_client_socket.h File net/socket/ssl_client_socket.h (right): http://codereview.chromium.org/8817021/diff/13001/net/socket/ssl_client_socke... net/socket/ssl_client_socket.h:162: // True if an Origin Bound Certificate was sent. On 2011/12/06 20:47:03, wtc wrote: > > Please use lowercase for "origin-bound certificate". Done. http://codereview.chromium.org/8817021/diff/13001/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/8817021/diff/13001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_nss.cc:2190: rv = SECFailure; On 2011/12/06 20:47:03, wtc wrote: > > We also need to call set_was_origin_bound_cert_sent(true) > if result == OK. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/8817021/15001
Try job failure for 8817021-15001 (retry) on linux_clang for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/8817021/15001
Try job failure for 8817021-15001 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/8817021/15001
Try job failure for 8817021-15001 (retry) on mac_rel for step "browser_tests". It's a second try, previously, steps "browser_tests, ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/8817021/15001
Change committed as 113409 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
