Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1360633002:
Implement Token Binding negotiation TLS extension (Closed)
DescriptionImplement Token Binding negotiation TLS extension
BUG=467312
Committed: https://crrev.com/736ceda358a1c2b576b41d09a108f2d1774e0e4d
Cr-Commit-Position: refs/heads/master@{#358465}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Fix TokenBindingExtension constructor #Patch Set 4 : Use switch and pref to control token binding instead of finch #Patch Set 5 : Fix memory leak; fix mac compilation issue #Patch Set 6 : rebase #
Total comments: 47
Patch Set 7 : respond to comments #Patch Set 8 : Remove unused TokenBindingExtension member from SSLContext #Patch Set 9 : rebase #Patch Set 10 : rebase #
Total comments: 10
Patch Set 11 : fix msvc error #
Total comments: 1
Patch Set 12 : Rip out TB key lookup from SSLClientSocketOpenSSL; fold TokenBindingExtension class into SSLClientS… #
Total comments: 57
Patch Set 13 : address most of davidben's comments #Patch Set 14 : Remove token binding switch and pref #Patch Set 15 : rebase #Patch Set 16 : rebase #Patch Set 17 : update to latest version of tb nego spec; tweak SSLConfig #
Total comments: 19
Patch Set 18 : respond to comments #
Total comments: 16
Patch Set 19 : nits; move custom ext callback static methods to SSLContext #Patch Set 20 : rebase #
Dependent Patchsets: Messages
Total messages: 42 (3 generated)
nharper@chromium.org changed reviewers: + davidben@chromium.org, mattm@chromium.org, rsleevi@chromium.org
https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket.cc:153: UMA_HISTOGRAM_ENUMERATION("DomainBoundCerts.Support", supported, Add a similar histogram for token binding? https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:267: DLOG(ERROR) << "Failed to register calllbacks for token binding"; dcheck? https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:347: TokenBindingExtension token_binding_extension_; unused? https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1216: net_log_.AddEvent(NetLog::TYPE_SSL_CHANNEL_ID_REQUESTED); (I'm not sure why the netlogging for channel id is different in the openssl and nss files.) I think the channel_id_service lookups should have begin event TYPE_SSL_GET_DOMAIN_BOUND_CERT (and a matching end event when the lookup finishes). Then there should be a new TYPE_SSL_TOKEN_BINDING_REQUESTED which is done when the extension is negotiated even if channel_id_key_ is already initialized, and a TOKEN_BINDING_PROVIDED when it's used https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2216: void TokenBindingExtension::SetParams(std::vector<TokenBindingParam>* params) { take a const ref https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2235: int* al) { al? https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2239: if (ext_type != kExtNum) { I'm not familiar enough with the boringssl api here, but should these ext_type tests be DCHECKs? https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2297: if (CBS_len(¶meters_list) > 0 || !valid_param) Add a comment about why you don't need to check the rest of the parameters_list https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2298: return 0; Is setting an appropriate value to *al (out_alert_value) when returning an error required or optional for this function? https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:41: class TokenBindingExtension { Maybe this should be a subclass of SSLClientSocketOpenSSL?
Just did an initial pass over it. https://codereview.chromium.org/1360633002/diff/100001/chrome/browser/net/ssl... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1360633002/diff/100001/chrome/browser/net/ssl... chrome/browser/net/ssl_config_service_manager_pref.cc:225: default_config.token_binding_params.size() > 0); This is sort of weird. We're pulling information from default_config, but losing most of it since it then turns into ECDSAP256_SHA256 unconditionally. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2233: const unsigned char** out, Nit: uint8_t https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2239: if (ext_type != kExtNum) { On 2015/09/24 22:13:12, mattm wrote: > I'm not familiar enough with the boringssl api here, but should these ext_type > tests be DCHECKs? Yeah, I think DCHECK is right. It shouldn't be called with the wrong one. (We inherited the API from OpenSSL. I guess the idea is you might reuse the callback for multiple xtns?) https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2245: goto end; Optional: Here's a scoper for you to save on the gotos. class ScopedCBB { public: ScopedCBB() { CBB_zero(&cbb_); } ~ScopedCBB() { CBB_cleanup(&cbb_); } CBB* get() { return &cbb_; } private: CBB cbb_; DISALLOW_COPY_AND_ASSIGN(ScopedCBB); }; https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2268: const unsigned char* in, Nit: uint8_t https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2283: !CBS_get_u8_length_prefixed(&in_ext, ¶meters_list)) { Also reject CBS_len(&in_ext) != 0 to reject extra bits. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2284: return 0; *al = SSL_AD_DECODE_ERROR; (This is actually the default, but it's not documented as such, so being explicit is probably good.) https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2288: return 0; Optional: I would probably fold this line and the CBS_len check into the pile of ||s above. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2297: if (CBS_len(¶meters_list) > 0 || !valid_param) On 2015/09/24 22:13:12, mattm wrote: > Add a comment about why you don't need to check the rest of the parameters_list parameters_list is only ever supposed to have one element for a client. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2297: if (CBS_len(¶meters_list) > 0 || !valid_param) If valid_param is false, we probably want to emit SSL_AD_ILLEGAL_PARAMETER rather than SSL_AD_DECODE_ERROR. Not that it really matters at all. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2298: return 0; On 2015/09/24 22:13:12, mattm wrote: > Is setting an appropriate value to *al (out_alert_value) when returning an error > required or optional for this function? *al is only processed on failure, so it can be left alone. https://codereview.chromium.org/1360633002/diff/100001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/1360633002/diff/100001/net/ssl/ssl_info.h#new... net/ssl/ssl_info.h:87: bool token_binding_negotiated; We'll need the TB type, won't we? The HTTP layer is going to be interpreting it.
https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2297: if (CBS_len(¶meters_list) > 0 || !valid_param) On 2015/09/25 21:51:50, David Benjamin wrote: > On 2015/09/24 22:13:12, mattm wrote: > > Add a comment about why you don't need to check the rest of the > parameters_list > > parameters_list is only ever supposed to have one element for a client. Right, I just meant there should be a comment stating that. Otherwise if you're just reading over the code it looks rather weird. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2298: return 0; On 2015/09/25 21:51:50, David Benjamin wrote: > On 2015/09/24 22:13:12, mattm wrote: > > Is setting an appropriate value to *al (out_alert_value) when returning an > error > > required or optional for this function? > > *al is only processed on failure, so it can be left alone. I was asking about the failure cases specifically, but it looks like you addressed that in comments above.
https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2298: return 0; On 2015/09/25 22:01:30, mattm wrote: > On 2015/09/25 21:51:50, David Benjamin wrote: > > On 2015/09/24 22:13:12, mattm wrote: > > > Is setting an appropriate value to *al (out_alert_value) when returning an > > error > > > required or optional for this function? > > > > *al is only processed on failure, so it can be left alone. > > I was asking about the failure cases specifically, but it looks like you > addressed that in comments above. Right, sorry, misread that. :-)
https://codereview.chromium.org/1360633002/diff/100001/chrome/browser/net/ssl... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1360633002/diff/100001/chrome/browser/net/ssl... chrome/browser/net/ssl_config_service_manager_pref.cc:225: default_config.token_binding_params.size() > 0); On 2015/09/25 21:51:50, David Benjamin wrote: > This is sort of weird. We're pulling information from default_config, but losing > most of it since it then turns into ECDSAP256_SHA256 unconditionally. I changed this to use the size == 1 && params[0] == P256 check that I used elsewhere. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket.cc:153: UMA_HISTOGRAM_ENUMERATION("DomainBoundCerts.Support", supported, On 2015/09/24 22:13:12, mattm wrote: > Add a similar histogram for token binding? Done. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:267: DLOG(ERROR) << "Failed to register calllbacks for token binding"; On 2015/09/24 22:13:12, mattm wrote: > dcheck? Done. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:347: TokenBindingExtension token_binding_extension_; On 2015/09/24 22:13:13, mattm wrote: > unused? It's used in several places below. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1216: net_log_.AddEvent(NetLog::TYPE_SSL_CHANNEL_ID_REQUESTED); On 2015/09/24 22:13:12, mattm wrote: > (I'm not sure why the netlogging for channel id is different in the openssl and > nss files.) > > I think the channel_id_service lookups should have begin event > TYPE_SSL_GET_DOMAIN_BOUND_CERT (and a matching end event when the lookup > finishes). > > Then there should be a new TYPE_SSL_TOKEN_BINDING_REQUESTED which is done when > the extension is negotiated even if channel_id_key_ is already initialized, and > a TOKEN_BINDING_PROVIDED when it's used (Both nss and openssl use TYPE_SSL_CHANNEL_ID_REQUESTED/PROVIDED; nss also calls NetLog::BeginEvent/EndEventWithNetErrorCode for TYPE_SSL_GET_DOMAIN_BOUND_CERT.) I added a new event TYPE_SSL_GET_TOKEN_BINDING_KEY and call BeginEvent and EndEventWithNetErrorCode for it when we do the key lookup. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2216: void TokenBindingExtension::SetParams(std::vector<TokenBindingParam>* params) { On 2015/09/24 22:13:12, mattm wrote: > take a const ref Done. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2233: const unsigned char** out, On 2015/09/25 21:51:50, David Benjamin wrote: > Nit: uint8_t Done. I've gone through and changed these signatures to match boringssl (I originally copied the signatures, variable names included, from openssl). https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2235: int* al) { On 2015/09/24 22:13:12, mattm wrote: > al? out_alert_value. Renamed. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2239: if (ext_type != kExtNum) { On 2015/09/25 21:51:50, David Benjamin wrote: > On 2015/09/24 22:13:12, mattm wrote: > > I'm not familiar enough with the boringssl api here, but should these ext_type > > tests be DCHECKs? > > Yeah, I think DCHECK is right. It shouldn't be called with the wrong one. (We > inherited the API from OpenSSL. I guess the idea is you might reuse the callback > for multiple xtns?) Done. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2245: goto end; On 2015/09/25 21:51:50, David Benjamin wrote: > Optional: Here's a scoper for you to save on the gotos. > > class ScopedCBB { > public: > ScopedCBB() { CBB_zero(&cbb_); } > ~ScopedCBB() { CBB_cleanup(&cbb_); } > > CBB* get() { return &cbb_; } > > private: > CBB cbb_; > > DISALLOW_COPY_AND_ASSIGN(ScopedCBB); > }; Would it be appropriate to put this scoper in //crypto/scoped_openssl_types.h, or should it go somewhere else? https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2268: const unsigned char* in, On 2015/09/25 21:51:50, David Benjamin wrote: > Nit: uint8_t Done. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2283: !CBS_get_u8_length_prefixed(&in_ext, ¶meters_list)) { On 2015/09/25 21:51:50, David Benjamin wrote: > Also reject CBS_len(&in_ext) != 0 to reject extra bits. Done. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2284: return 0; On 2015/09/25 21:51:50, David Benjamin wrote: > *al = SSL_AD_DECODE_ERROR; > > (This is actually the default, but it's not documented as such, so being > explicit is probably good.) Done. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2288: return 0; On 2015/09/25 21:51:50, David Benjamin wrote: > Optional: I would probably fold this line and the CBS_len check into the pile of > ||s above. Done. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2297: if (CBS_len(¶meters_list) > 0 || !valid_param) On 2015/09/25 22:01:30, mattm wrote: > On 2015/09/25 21:51:50, David Benjamin wrote: > > On 2015/09/24 22:13:12, mattm wrote: > > > Add a comment about why you don't need to check the rest of the > > parameters_list > > > > parameters_list is only ever supposed to have one element for a client. > > Right, I just meant there should be a comment stating that. Otherwise if you're > just reading over the code it looks rather weird. Done. https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:41: class TokenBindingExtension { On 2015/09/24 22:13:13, mattm wrote: > Maybe this should be a subclass of SSLClientSocketOpenSSL? Done. https://codereview.chromium.org/1360633002/diff/100001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/1360633002/diff/100001/net/ssl/ssl_info.h#new... net/ssl/ssl_info.h:87: bool token_binding_negotiated; On 2015/09/25 21:51:50, David Benjamin wrote: > We'll need the TB type, won't we? The HTTP layer is going to be interpreting it. The http layer needs the TokenBinding (i.e. public key and signed material from ekm). My current approach (in a subsequent CL) is to add a method to SSLClientSocket GetProvidedTokenBinding, which returns a const-ref to a string containing the TokenBinding, and SSLClientSocketOpenSSL has a member variable provided_token_binding_ which gets filled at the end of a successful negotiation. I think the http layer only needs that method, and what's in SSLInfo is fine.
https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:347: TokenBindingExtension token_binding_extension_; On 2015/09/28 21:43:39, nharper wrote: > On 2015/09/24 22:13:13, mattm wrote: > > unused? > > It's used in several places below. Those all look like references to SSLClientSocketOpenSSL::token_binding_extension_, does anything use SSLClientSocketOpenSSL::SSLContext::token_binding_extension_?
https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/100001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:347: TokenBindingExtension token_binding_extension_; On 2015/09/28 23:07:25, mattm wrote: > On 2015/09/28 21:43:39, nharper wrote: > > On 2015/09/24 22:13:13, mattm wrote: > > > unused? > > > > It's used in several places below. > > Those all look like references to > SSLClientSocketOpenSSL::token_binding_extension_, does anything use > SSLClientSocketOpenSSL::SSLContext::token_binding_extension_? Sorry, I didn't notice that this was in SSLContext. It is indeed unused here.
rebase
rebase
https://codereview.chromium.org/1360633002/diff/100001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/1360633002/diff/100001/net/ssl/ssl_info.h#new... net/ssl/ssl_info.h:87: bool token_binding_negotiated; On 2015/09/28 21:43:39, nharper wrote: > On 2015/09/25 21:51:50, David Benjamin wrote: > > We'll need the TB type, won't we? The HTTP layer is going to be interpreting > it. > > The http layer needs the TokenBinding (i.e. public key and signed material from > ekm). My current approach (in a subsequent CL) is to add a method to > SSLClientSocket GetProvidedTokenBinding, which returns a const-ref to a string > containing the TokenBinding, and SSLClientSocketOpenSSL has a member variable > provided_token_binding_ which gets filled at the end of a successful > negotiation. I think the http layer only needs that method, and what's in > SSLInfo is fine. I'm not sure I follow. My understanding here is that we explicitly don't want to have to split HTTP2 sessions along TB decisions, which is why the TLS extension only does a very uninteresting non-variable negotiation and then everything else is done on a per-request basis. Hence why the key lookup should happen externally and such. SSLClientSocket need only give you an API to extract the tls-unique. https://codereview.chromium.org/1360633002/diff/180001/chrome/browser/net/ssl... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1360633002/diff/180001/chrome/browser/net/ssl... chrome/browser/net/ssl_config_service_manager_pref.cc:222: net::TB_PARAM_ECDSAP256_SHA256); Hrm. Both versions are sort of odd, if the default happens to be, say, RSA. Perhaps check just the size and do the other bits as DCHECKs? How much variability do we actually need? We could also keep the interface simple like a straight-up boolean or something. Although, how is the rest of this supposed to work anyway? I thought Token Binding was intentionally a much higher-level notion, and so we probably don't want the defaults to configure the SSLConfig layer at all (leaving SSLConfig to just default it to off), but instead that boolean would control something about how HTTP works and then the HTTP code would explicitly set it to true and false depending on how it was configured[*]. [*] NB: Mind the socket pools. https://crbug.com/533571. Though as I understand, part of the point of TB was so our socket pools wouldn't see as much variability and we'd have less of this problem. https://codereview.chromium.org/1360633002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1216: &channel_id_request_); What does this do? Do we even use this key for anything at the SSL layer? In Token Binding, this should be at the HTTP layer, no? I would think instead SSLClientSocketOpenSSL just advertises the TB type in SSLInfo or whereever and then the HTTP layer does whatever else beyond that. https://codereview.chromium.org/1360633002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1360633002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:103: static const unsigned int kExtNum = 30033; I believe this is actually an ODR violation for weird reasons. There was some Chromium-dev thread. Ditto below. A lot of this can probably get stuffed into the cc file somewhere. There's actually little enough state here, that I might just fold them into SSLClientSocketOpenSSL. That class is pretty intertwined with everything anyway and it doesn't look like the individual methods here do much, outside the extension callbacks. https://codereview.chromium.org/1360633002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:149: void* parse_arg); The random static methods thus far have ended up on SSLContext so they needn't be in the header.
https://chromiumcodereview.appspot.com/1360633002/diff/100001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://chromiumcodereview.appspot.com/1360633002/diff/100001/net/ssl/ssl_inf... net/ssl/ssl_info.h:87: bool token_binding_negotiated; On 2015/10/01 16:15:17, David Benjamin wrote: > On 2015/09/28 21:43:39, nharper wrote: > > On 2015/09/25 21:51:50, David Benjamin wrote: > > > We'll need the TB type, won't we? The HTTP layer is going to be interpreting > > it. > > > > The http layer needs the TokenBinding (i.e. public key and signed material > from > > ekm). My current approach (in a subsequent CL) is to add a method to > > SSLClientSocket GetProvidedTokenBinding, which returns a const-ref to a string > > containing the TokenBinding, and SSLClientSocketOpenSSL has a member variable > > provided_token_binding_ which gets filled at the end of a successful > > negotiation. I think the http layer only needs that method, and what's in > > SSLInfo is fine. > > I'm not sure I follow. My understanding here is that we explicitly don't want to > have to split HTTP2 sessions along TB decisions, which is why the TLS extension > only does a very uninteresting non-variable negotiation and then everything else > is done on a per-request basis. > > Hence why the key lookup should happen externally and such. SSLClientSocket need > only give you an API to extract the tls-unique. The draft spec for Token Binding over HTTP (https://tools.ietf.org/html/draft-ietf-tokbind-https-01#section-2) states that once a client and server have negotiated Token Binding, clients MUST include the Sec-Token-Binding header in their HTTP requests. I interpret this to mean that a TLS connection cannot be used both with and without Token Binding. My understanding is that moving things to the application layer for Token Binding (compared to Channel ID) wasn't to make decisions (like whether or not to allow Token Binding) on a per-request basis, but was to allow for things like the federated use case, where an application layer decision results in forwarding the token binding from one origin to another origin. I think having the key lookup in the SSLClientSocket is perfectly reasonable. An SSLClientSocket is used for only one origin, and the keys are per origin. Also, the negotiation for what key params to use (and whether or not to use Token Binding at all) happens at the transport layer. The logic at the application layer is about which token bindings to include. The spec requires that it always send the provided token binding; the decision made by the application layer is which referred token bindings to include. At the application layer, it can treat all token bindings as opaque byte strings. Since the SSLClientSocket has all of the necessary info to build the provided token binding, I think it makes more sense for it to be built there and let the application layer treat it as an opaque byte string. The API that I am trying to expose to the application layer is 1) Is Token Binding Enabled? 2) What is the provided token binding? https://chromiumcodereview.appspot.com/1360633002/diff/180001/chrome/browser/... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://chromiumcodereview.appspot.com/1360633002/diff/180001/chrome/browser/... chrome/browser/net/ssl_config_service_manager_pref.cc:222: net::TB_PARAM_ECDSAP256_SHA256); On 2015/10/01 16:15:17, David Benjamin wrote: > Hrm. Both versions are sort of odd, if the default happens to be, say, RSA. > Perhaps check just the size and do the other bits as DCHECKs? > I agree that this check is odd, and when considering the context of what might happen in the future, I don't think a boolean pref is the right choice. (Aside: if we are very certain that we will only ever support ECDSA P256, then this could just be a boolean, and only worry about what the key params are in SSLClientSocket. But I'm pretty sure we will want to add support for some flavor of RSA.) I imagine when we add support for RSA keys we will want to initially gate it, and that should be reflected in this pref. Maybe this should be changed to a list pref that contains a list of supported key params? Then it would match better with the token_binding_params vector in the SSLConfig. > How much variability do we actually need? We could also keep the interface > simple like a straight-up boolean or something. > > Although, how is the rest of this supposed to work anyway? I thought Token > Binding was intentionally a much higher-level notion, and so we probably don't > want the defaults to configure the SSLConfig layer at all (leaving SSLConfig to > just default it to off), but instead that boolean would control something about > how HTTP works and then the HTTP code would explicitly set it to true and false > depending on how it was configured[*]. > > [*] NB: Mind the socket pools. https://crbug.com/533571. Though as I understand, > part of the point of TB was so our socket pools wouldn't see as much variability > and we'd have less of this problem. https://chromiumcodereview.appspot.com/1360633002/diff/180001/net/socket/ssl_... File net/socket/ssl_client_socket_openssl.cc (right): https://chromiumcodereview.appspot.com/1360633002/diff/180001/net/socket/ssl_... net/socket/ssl_client_socket_openssl.cc:1216: &channel_id_request_); On 2015/10/01 16:15:17, David Benjamin wrote: > What does this do? Do we even use this key for anything at the SSL layer? > > In Token Binding, this should be at the HTTP layer, no? I would think instead > SSLClientSocketOpenSSL just advertises the TB type in SSLInfo or whereever and > then the HTTP layer does whatever else beyond that. We use this key once at the SSL layer to generate the provided token binding once and cache it in a member variable. This is done so that the token binding remains constant. (Otherwise, if we re-sign and regenerate the token binding for each http request, the value differs, negating the advantage of HTTP/2 header compression.) https://chromiumcodereview.appspot.com/1360633002/diff/180001/net/socket/ssl_... File net/socket/ssl_client_socket_openssl.h (right): https://chromiumcodereview.appspot.com/1360633002/diff/180001/net/socket/ssl_... net/socket/ssl_client_socket_openssl.h:103: static const unsigned int kExtNum = 30033; On 2015/10/01 16:15:17, David Benjamin wrote: > I believe this is actually an ODR violation for weird reasons. There was some > Chromium-dev thread. Ditto below. A lot of this can probably get stuffed into > the cc file somewhere. > > There's actually little enough state here, that I might just fold them into > SSLClientSocketOpenSSL. That class is pretty intertwined with everything anyway > and it doesn't look like the individual methods here do much, outside the > extension callbacks. To be clear, you're suggesting removing the TokenBindingExtension class and folding its methods and members into SSLClientSocketOpenSSL? That sounds reasonable enough.
rsleevi: You've been keeping up with Token Binding far more than I have. Do you mind chiming in since I could certainly be confused as to the point of this exercise. But I think my comment is accurate? https://chromiumcodereview.appspot.com/1360633002/diff/100001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://chromiumcodereview.appspot.com/1360633002/diff/100001/net/ssl/ssl_inf... net/ssl/ssl_info.h:87: bool token_binding_negotiated; On 2015/10/01 19:12:23, nharper wrote: > On 2015/10/01 16:15:17, David Benjamin wrote: > > On 2015/09/28 21:43:39, nharper wrote: > > > On 2015/09/25 21:51:50, David Benjamin wrote: > > > > We'll need the TB type, won't we? The HTTP layer is going to be > interpreting > > > it. > > > > > > The http layer needs the TokenBinding (i.e. public key and signed material > > from > > > ekm). My current approach (in a subsequent CL) is to add a method to > > > SSLClientSocket GetProvidedTokenBinding, which returns a const-ref to a > string > > > containing the TokenBinding, and SSLClientSocketOpenSSL has a member > variable > > > provided_token_binding_ which gets filled at the end of a successful > > > negotiation. I think the http layer only needs that method, and what's in > > > SSLInfo is fine. > > > > I'm not sure I follow. My understanding here is that we explicitly don't want > to > > have to split HTTP2 sessions along TB decisions, which is why the TLS > extension > > only does a very uninteresting non-variable negotiation and then everything > else > > is done on a per-request basis. > > > > Hence why the key lookup should happen externally and such. SSLClientSocket > need > > only give you an API to extract the tls-unique. > > The draft spec for Token Binding over HTTP > (https://tools.ietf.org/html/draft-ietf-tokbind-https-01#section-2) states that > once a client and server have negotiated Token Binding, clients MUST include the > Sec-Token-Binding header in their HTTP requests. I interpret this to mean that a > TLS connection cannot be used both with and without Token Binding. My > understanding is that moving things to the application layer for Token Binding > (compared to Channel ID) wasn't to make decisions (like whether or not to allow > Token Binding) on a per-request basis, but was to allow for things like the > federated use case, where an application layer decision results in forwarding > the token binding from one origin to another origin. > > I think having the key lookup in the SSLClientSocket is perfectly reasonable. An > SSLClientSocket is used for only one origin, and the keys are per origin. Also, > the negotiation for what key params to use (and whether or not to use Token > Binding at all) happens at the transport layer. Due to CORS, we end up in situations where we must assert credentials on some connections, but not otherwise. It's also not true that an SSLClientSocket is used for only one origin. We will happily coalesce them by origins for SPDY if the certificates allow it. There's also Alt-Svc whose entire purpose in life is to coalesce more things like that. > The logic at the application layer is about which token bindings to include. The > spec requires that it always send the provided token binding; the decision made > by the application layer is which referred token bindings to include. At the > application layer, it can treat all token bindings as opaque byte strings. Since > the SSLClientSocket has all of the necessary info to build the provided token > binding, I think it makes more sense for it to be built there and let the > application layer treat it as an opaque byte string. > > The API that I am trying to expose to the application layer is > 1) Is Token Binding Enabled? > 2) What is the provided token binding?
Since it's hard to tell in the mess of quotedness, there was new text in that comment: > Due to CORS, we end up in situations where we must assert credentials on some > connections, but not otherwise. > > It's also not true that an SSLClientSocket is used for only one origin. We will > happily coalesce them by origins for SPDY if the certificates allow it. There's > also Alt-Svc whose entire purpose in life is to coalesce more things like that. Also, s/connections/requests/ in that first sentence.
On 2015/10/01 19:21:25, David Benjamin wrote: > Since it's hard to tell in the mess of quotedness, there was new text in that > comment: > > > Due to CORS, we end up in situations where we must assert credentials on some > > connections, but not otherwise. Isn't this what privacy mode is for? > > > > It's also not true that an SSLClientSocket is used for only one origin. We > will > > happily coalesce them by origins for SPDY if the certificates allow it. > There's > > also Alt-Svc whose entire purpose in life is to coalesce more things like > that. SSLClientSocketOpenSSL has a host_and_port_ field which is used for looking up the channel id key. If we're coalescing requests for multiple different origins over one SSLClientSocketOpenSSL and it uses channel id, then at least one origin will see the wrong channel id (which sounds like a bug to me). > > > Also, s/connections/requests/ in that first sentence.
On 2015/10/01 19:34:49, nharper wrote: > On 2015/10/01 19:21:25, David Benjamin wrote: > > Since it's hard to tell in the mess of quotedness, there was new text in that > > comment: > > > > > Due to CORS, we end up in situations where we must assert credentials on > some > > > connections, but not otherwise. > > Isn't this what privacy mode is for? Yeah, privacy mode was added to deal with Channel ID. I'd thought this was considered not ideal and was needed only as a consequence of Channel ID (and why we don't like Channel ID and want Token Binding because this can FINALLY be a per-request thing). Ryan should confirm this or correct me though. Sharding things like that is bad for performance and weakens things like preconnect because we need to know which context we're preconnecting for. > > > It's also not true that an SSLClientSocket is used for only one origin. We > > will > > > happily coalesce them by origins for SPDY if the certificates allow it. > > There's > > > also Alt-Svc whose entire purpose in life is to coalesce more things like > > that. > > SSLClientSocketOpenSSL has a host_and_port_ field which is used for looking up > the channel id key. If we're coalescing requests for multiple different origins > over one SSLClientSocketOpenSSL and it uses channel id, then at least one origin > will see the wrong channel id (which sounds like a bug to me). > > > > > > Also, s/connections/requests/ in that first sentence. See also: we really don't like Channel ID and want to replace it. :-)
On 2015/10/01 19:40:27, David Benjamin wrote: > Yeah, privacy mode was added to deal with Channel ID. I'd thought this was > considered not ideal and was needed only as a consequence of Channel ID (and why > we don't like Channel ID and want Token Binding because this can FINALLY be a > per-request thing). Ryan should confirm this or correct me though. Unfortunately, the Token Binding v Channel ID distinction doesn't help us eliminate the sharding. More precisely, it resolves the ambient authority security concerns, but intrinsically leaves the privacy concerns (which also exist by virtue of the notion of first- and third- party cookies), and while it's highly desirable to roll back privacy mode, it's not really feasible to 'walk back' on perceived privacy benefits (the question of whether or not the privacy benefits were tangible is a separate, and necessary, conversation, but uh, I can only engage in so many discussions, so I stopped pushing on that one) > > Sharding things like that is bad for performance and weakens things like > preconnect because we need to know which context we're preconnecting for. Preconnect already has to deal with this (for CORS' ambient authority concerns), so I don't think it weakens any more than the existing behaviour.
https://codereview.chromium.org/1360633002/diff/200001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/200001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1211: GotoState(STATE_TOKEN_BINDING_LOOKUP_COMPLETE); Why do we need to do the lookup in the SSL socket? I understand that negotiation necessarily must be done as part of the exchange, but handling any TB keys & lookups was supposed to be upliftable to the HTTP layer (by virtue of being a header), so long as the underlying transport negotiated TB.
Ryan and I chatted about where TB key lookups should be handled, and agreed that it should be moved to the HTTP layer. I will add some sort of caching lookup object (either as part of the SSLClientSocketOpenSSL or managed to have the same lifetime as it) that takes a (key, host) pair from the http layer and returns the provided token binding containing the public key and signed EKM value (from cache, or (re)computes it). This means that the HTTP layer does have to do key lookups, but it can still treat the token bindings as opaque byte strings. For this CL, I'll be removing the key lookup from SSLClientSocket, and adding a token binding key parameters field to SSLInfo (in addition to the token_binding_negotiated boolean). I think the new caching thing will go in the next CL.
On 2015/10/01 23:07:37, nharper wrote: > Ryan and I chatted about where TB key lookups should be handled, and agreed that > it should be moved to the HTTP layer. I will add some sort of caching lookup > object (either as part of the SSLClientSocketOpenSSL or managed to have the same > lifetime as it) that takes a (key, host) pair from the http layer and returns > the provided token binding containing the public key and signed EKM value (from > cache, or (re)computes it). (Talked a bit in person) - I'm not sure it needs to be (key, host) as the lookup function; I think it can just be key (something else will have looked up the given key for a host)
I think I've removed from this CL what we discussed. I also refactored away the TokenBindingExtension class. https://codereview.chromium.org/1360633002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1360633002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:149: void* parse_arg); On 2015/10/01 16:15:17, David Benjamin wrote: > The random static methods thus far have ended up on SSLContext so they needn't > be in the header. These static methods are calling the private methods. Is there a way to leave them out of the header while still declaring and defining a private method in the .cc file? I tried making them functions in the anonymous namespace in the .cc file, but that didn't work because they were calling private methods.
On 2015/10/01 23:17:51, Ryan Sleevi wrote: > (Talked a bit in person) - I'm not sure it needs to be (key, host) as the lookup > function; I think it can just be key (something else will have looked up the > given key for a host) I remembered why I thought the lookup function wanted (key, host) instead of (key) as the input. I was thinking of an alternate version that instead of taking (key) takes (host, key_type). This means moving the actual key lookup down to the cache so the HttpNetworkTransaction doesn't have to deal with the ChannelIDService for looking up keys. (I'm looking into how the ChannelIDService currently gets created and how it gets passed into SSLClientSocket, to see what's needed to get it passed in to either the HttpNetworkTransaction or the new cache object.)
ping
I think this mostly looks good. Main comments are around the pref stuff it and the big open questions around renego. Sorry about the delay. (I think I got confused whether you were waiting for me or Ryan.) https://codereview.chromium.org/1360633002/diff/180001/chrome/browser/net/ssl... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1360633002/diff/180001/chrome/browser/net/ssl... chrome/browser/net/ssl_config_service_manager_pref.cc:222: net::TB_PARAM_ECDSAP256_SHA256); On 2015/10/01 19:12:23, nharper wrote: > On 2015/10/01 16:15:17, David Benjamin wrote: > > Hrm. Both versions are sort of odd, if the default happens to be, say, RSA. > > Perhaps check just the size and do the other bits as DCHECKs? > > > I agree that this check is odd, and when considering the context of what might > happen in the future, I don't think a boolean pref is the right choice. > > (Aside: if we are very certain that we will only ever support ECDSA P256, then > this could just be a boolean, and only worry about what the key params are in > SSLClientSocket. But I'm pretty sure we will want to add support for some flavor > of RSA.) > > I imagine when we add support for RSA keys we will want to initially gate it, > and that should be reflected in this pref. Maybe this should be changed to a > list pref that contains a list of supported key params? Then it would match > better with the token_binding_params vector in the SSLConfig. I think we should at least have the DCHECK then, or someone might modify the ssl_config.cc defaults without realizing there's something else to do. Though I would actually lean towards just making it a boolean and, when we add RSA later, we sort that out later. SSLConfig is hardly a fixed interface. Either way, I think the quoted bit below about the HTTP code is the important one. > > How much variability do we actually need? We could also keep the interface > > simple like a straight-up boolean or something. > > > > Although, how is the rest of this supposed to work anyway? I thought Token > > Binding was intentionally a much higher-level notion, and so we probably don't > > want the defaults to configure the SSLConfig layer at all (leaving SSLConfig > to > > just default it to off), but instead that boolean would control something > about > > how HTTP works and then the HTTP code would explicitly set it to true and > false > > depending on how it was configured[*]. > > > > [*] NB: Mind the socket pools. https://crbug.com/533571. Though as I > understand, > > part of the point of TB was so our socket pools wouldn't see as much > variability > > and we'd have less of this problem. Put in other words, SSLConfig's defaults should *always* be the empty vector. Just like SSLConfig's defaults for NPN or ALPN are the empty vector. At the "how do I configure Chrome" level, I never want to configure "what TB types do I advertising in SSLConfig". I want to configure "do I do TB at the HTTP layer at all" and "how does TB at the HTTP layer work?". The latter might be, as you say, a separate boolean for RSA. Or whatever else we decide to do. So actually I think this file should never touch TB at all. (Or perhaps should only touch it temporarily until the HTTP bits are there if you want to advertise the extension without the HTTP bits.) Just like this file never never touches NPN or ALPN. NPN or ALPN are controlled by things like "is SPDY enabled" and, from there, set by the HTTP-level code. We've got multiple consumers of SSLClientSocket in Chromium. The WebSocket code (rather hackishly) uses the HTTP stack differently. Extensions gets an API to create SSLClientSockets. etc. I could imagine that we want --ssl-max-version to apply to all of those (maybe not extensions), but we definitely don't want --enable-token-binding to advertise TB on all of those. Advertising TB means "the code that created this socket wants TB and will create bindings as appropriate" (the server gets to use that as a signal to decide whether to require TB-bound cookies, right?), so it should only be turned on at the net/http stack's whim. https://codereview.chromium.org/1360633002/diff/220001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1360633002/diff/220001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:551: // Enables negotiating token binding (draft-ietf-tokbind-protocol-02) in the tls Nit: tls -> TLS https://codereview.chromium.org/1360633002/diff/220001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1360633002/diff/220001/chrome/common/pref_nam... chrome/common/pref_names.cc:1255: const char kEnableTokenBinding[] = "ssl.token_binding.enabled"; See comment on previous patchset about whether this pref should actually be at the SSL level. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket.cc:160: ChannelIDService* channel_id_service, Since TB won't touch ChannelIDService from SSLClientSocket, these bits should probably go. Actually, I think you want the metrics to all be at the HTTP layer as well, so that ChannelIDService is still available. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket.cc:176: } else if (!crypto::ECPrivateKey::IsSupported()) { Oh! I'll go ahead and unwind that code. (https://codereview.chromium.org/1408813002/) Assume this method always returns true. We're done having to deal with system NSS's limitations in almost all cases. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket.cc:206: ChannelIDService* channel_id_service) { Ditto re ChannelIDService not being involved. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket.cc:211: return IsChannelIDEnabled(ssl_config, channel_id_service); (This doesn't really make sense anyway since TB may be enabled while channel_id_enabled is false, but probably this method can just go.) https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:272: } Nit: No curlies, even though I prefer them. :-( https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:478: weak_factory_(this) { Initialize tb_was_negotiated_ and tb_negotiated_param_. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:633: } Reset tb_was_negotiated_ and tb_negotiated_param_. (This is totally useless. It's a remnant of some Connect/Disconnect API contract that we've half killed but not completely killed yet and never worked for SSLClientSocket anyway. See https://crbug.com/499289.) https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1124: if (tb_was_negotiated_ && !ssl_->session->extended_master_secret) { Use SSL_get_extms_support. (BoringSSL will internally ensure that all the EMS rules around session resumption check out.) https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#S... https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1124: if (tb_was_negotiated_ && !ssl_->session->extended_master_secret) { This check also doesn't work unless we're forbidding renego (which I would prefer we do). In that case, you want to tweak IsRenegotiationAllowed(). https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1126: } Nit: No curlies, even though I prefer them. :-( https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2204: CBB output, parameters_list; CBB_zero(&output); otherwise CBB_cleanup might act on an uninitialized one. Any reason not to use the scoper? goto and C++ sometimes interact weirdly when you declare variables across the point you're goto'ing across. (I guess, strictly speaking, CBB_init can't fail because mallocs don't fail. But if you're checking, we probably ought to zero it properly.) https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2206: if (!CBB_init(&output, 7)) Nit: You can || this with the next block. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2216: } Nit: No curlies, even though I prefer them. :-( https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2217: } Optional: Since this was confusing before, perhaps a comment: // |*out| will be freed by TokenBindingFreeCallback. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2232: int* out_alert_value) { So, this method may get called on a renego, and that it mucks with tb_negotiated_param_ and such is going to be a problem. I'm not sure what the renego semantics of this thing are supposed to be at all, but supposing we go with the only sane option (renegos are forbidden if you TB), this should have: if (completed_connect_) { // Token Binding may only be negotiated on the initial handshake. *out_alert_value = SSL_AD_ILLEGAL_PARAMETER; return 0; } https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2250: tb_negotiated_param_ = TokenBindingParam(param); static_cast<TokenBindingParam>(param). That or just do ssl_config_.token_binding_params[i] which is already of the right type. :-) https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2262: } This seems unnecessary now that you check equality. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2266: if (CBS_len(¶meters_list) > 0 || !valid_param) { Optional: I would actually put the CBS_len check up into the parsing bit. That seems quite legitimately an encoding error anyway. That TLS extensions some of the time (but not always) use the same syntax in both directions to the detriment of the server is really just weird, IMO. Then you can put tb_was_negotiated_ = true into line 2251, early return and do away with valid_param. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2277: } This seems unnecessary now that you check equality. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2291: DCHECK(extension_value == kTbExtNum); DCHECK_EQ https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2303: DCHECK(extension_value == kTbExtNum); DCHECK_EQ https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2315: DCHECK(extension_value == kTbExtNum); DCHECK_EQ https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:9: #include <openssl/bytestring.h> Not needed. (Actually base.h will forward-declare everything anyway. ssl.h is only here because there's some enum and those can't be forward-declared.) https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:209: // Token Binding Extension callbacks. RegisterTokenBidningExtensionCallbacks Bidning -> Binding https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3134: TEST_F(SSLClientSocketChannelIDTest, TokenBindingEnabled) { You're not using any of the Channel ID test harness, so I would just make this an SSLClientSocketTest. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3155: TEST_F(SSLClientSocketChannelIDTest, TokenBindingFailsWithEmsDisabled) { Ditto. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3172: Think it's worth having a test with where TB just isn't negotiated? Would have caught some of the uninitialized value stuff when it floated into ASan I think. https://codereview.chromium.org/1360633002/diff/220001/net/ssl/ssl_info.cc File net/ssl/ssl_info.cc (right): https://codereview.chromium.org/1360633002/diff/220001/net/ssl/ssl_info.cc#ne... net/ssl/ssl_info.cc:55: token_binding_negotiated = false; It is documented to be invalid unless token_binding_negotiated, but we probably should still initialize token_binding_key_param to avoid tempting valgrind and the *Sans.
Patchset #13 (id:240001) has been deleted
Remove token binding switch and pref
https://codereview.chromium.org/1360633002/diff/180001/chrome/browser/net/ssl... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/1360633002/diff/180001/chrome/browser/net/ssl... chrome/browser/net/ssl_config_service_manager_pref.cc:222: net::TB_PARAM_ECDSAP256_SHA256); It sounds to me like this should be removed from the SSLConfigServiceManagerPref. The controls for token binding should still stay in SSLConfig, but they should always default to being off. When we want token binding enabled, HttpNetworkTransaction::Start can set the bits in server_ssl_config_. (Token Binding requires the coordination of both the http layer and the tls layer, so it should be controlled from the http layer instead of the tls layer.) For this CL, it sounds like I can rip out all of the pref bits and command line switch and move that to the http CL. I could use some guidance on how exactly to get the switch plumbed down to HttpNetworkTransaction (presumably through some other pref). https://codereview.chromium.org/1360633002/diff/220001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1360633002/diff/220001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:551: // Enables negotiating token binding (draft-ietf-tokbind-protocol-02) in the tls On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > Nit: tls -> TLS Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket.cc:160: ChannelIDService* channel_id_service, On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > Since TB won't touch ChannelIDService from SSLClientSocket, these bits should > probably go. Actually, I think you want the metrics to all be at the HTTP layer > as well, so that ChannelIDService is still available. Gone. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket.cc:206: ChannelIDService* channel_id_service) { On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > Ditto re ChannelIDService not being involved. This method is now gone. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:272: } On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > Nit: No curlies, even though I prefer them. :-( Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:478: weak_factory_(this) { On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > Initialize tb_was_negotiated_ and tb_negotiated_param_. I'm initializing tb_was_negotiated_ to false here. I'm also initializing tb_negotiated_param_ to a valid value, even though it's meaningless when tb_was_negotiated_ is false. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:633: } On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > Reset tb_was_negotiated_ and tb_negotiated_param_. (This is totally useless. > It's a remnant of some Connect/Disconnect API contract that we've half killed > but not completely killed yet and never worked for SSLClientSocket anyway. See > https://crbug.com/499289.) I'm resetting tb_was_negotiated_, but not tb_negotiated_param_. The latter is meaningless if the former is false, and unlike the constructor where it makes sense to initialize tb_negotiated_param_ to avoid any asan/msan/valgrind issues, there's no need for that here. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1124: if (tb_was_negotiated_ && !ssl_->session->extended_master_secret) { On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > Use SSL_get_extms_support. (BoringSSL will internally ensure that all the EMS > rules around session resumption check out.) > > https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#S... Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1124: if (tb_was_negotiated_ && !ssl_->session->extended_master_secret) { On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > This check also doesn't work unless we're forbidding renego (which I would > prefer we do). In that case, you want to tweak IsRenegotiationAllowed(). I've modified IsRenegotiationAllowed to return false if TB is negotiated. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1126: } On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > Nit: No curlies, even though I prefer them. :-( Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2204: CBB output, parameters_list; On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > CBB_zero(&output); otherwise CBB_cleanup might act on an uninitialized one. Any > reason not to use the scoper? goto and C++ sometimes interact weirdly when you > declare variables across the point you're goto'ing across. > > (I guess, strictly speaking, CBB_init can't fail because mallocs don't fail. But > if you're checking, we probably ought to zero it properly.) I've changed it to use a scoper. There isn't one that already exists (that I could find) for CBB, which meant I had to define a CBB_new and CBB_free to wrap around CBB_init and CBB_cleanup. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2206: if (!CBB_init(&output, 7)) On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > Nit: You can || this with the next block. This block is gone now. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2216: } On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > Nit: No curlies, even though I prefer them. :-( Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2217: } On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > Optional: Since this was confusing before, perhaps a comment: > > // |*out| will be freed by TokenBindingFreeCallback. Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2232: int* out_alert_value) { On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > So, this method may get called on a renego, and that it mucks with > tb_negotiated_param_ and such is going to be a problem. > > I'm not sure what the renego semantics of this thing are supposed to be at all, > but supposing we go with the only sane option (renegos are forbidden if you TB), > this should have: > > if (completed_connect_) { > // Token Binding may only be negotiated on the initial handshake. > *out_alert_value = SSL_AD_ILLEGAL_PARAMETER; > return 0; > } Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2250: tb_negotiated_param_ = TokenBindingParam(param); On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > static_cast<TokenBindingParam>(param). > > That or just do ssl_config_.token_binding_params[i] which is already of the > right type. :-) Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2262: } On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > This seems unnecessary now that you check equality. That was a goof of checking equality for version_major/minor earlier. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2266: if (CBS_len(¶meters_list) > 0 || !valid_param) { On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > Optional: I would actually put the CBS_len check up into the parsing bit. That > seems quite legitimately an encoding error anyway. That TLS extensions some of > the time (but not always) use the same syntax in both directions to the > detriment of the server is really just weird, IMO. > > Then you can put tb_was_negotiated_ = true into line 2251, early return and do > away with valid_param. I moved the version checks to before the check on line 2251, to do away with valid_param and early return. I also moved the CBS_len check for parameters_list up with the rest of the parsing. A result of this (moving the version checks) is we're a little lenient in parsing a response from a server with an old version (one we don't support) that also didn't send a valid param in that we will silently continue without token binding instead of setting an alert value. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2277: } On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > This seems unnecessary now that you check equality. Ditto. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2291: DCHECK(extension_value == kTbExtNum); On 2015/10/15 21:52:08, davidben (OOO 10-21 to 10-23) wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2303: DCHECK(extension_value == kTbExtNum); On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2315: DCHECK(extension_value == kTbExtNum); On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:9: #include <openssl/bytestring.h> On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > Not needed. (Actually base.h will forward-declare everything anyway. ssl.h is > only here because there's some enum and those can't be forward-declared.) Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:209: // Token Binding Extension callbacks. RegisterTokenBidningExtensionCallbacks On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > Bidning -> Binding Done. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3134: TEST_F(SSLClientSocketChannelIDTest, TokenBindingEnabled) { On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > You're not using any of the Channel ID test harness, so I would just make this > an SSLClientSocketTest. I'm using EnableChannelID from SSLClientSocketChannelIDTest. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3155: TEST_F(SSLClientSocketChannelIDTest, TokenBindingFailsWithEmsDisabled) { On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > Ditto. Ditto. https://codereview.chromium.org/1360633002/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3172: On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > Think it's worth having a test with where TB just isn't negotiated? Would have > caught some of the uninitialized value stuff when it floated into ASan I think. I don't see any harm in adding that test. ASan didn't catch any use of uninitialized values when running that test. I haven't checked msan. https://codereview.chromium.org/1360633002/diff/220001/net/ssl/ssl_info.cc File net/ssl/ssl_info.cc (right): https://codereview.chromium.org/1360633002/diff/220001/net/ssl/ssl_info.cc#ne... net/ssl/ssl_info.cc:55: token_binding_negotiated = false; On 2015/10/15 21:52:09, davidben (OOO 10-21 to 10-23) wrote: > It is documented to be invalid unless token_binding_negotiated, but we probably > should still initialize token_binding_key_param to avoid tempting valgrind and > the *Sans. Done.
rebase
This CL (and the http one, https://codereview.chromium.org/1378613004/) have been updated.
https://codereview.chromium.org/1360633002/diff/340001/net/ssl/ssl_config.cc File net/ssl/ssl_config.cc (right): https://codereview.chromium.org/1360633002/diff/340001/net/ssl/ssl_config.cc#... net/ssl/ssl_config.cc:38: token_binding_params.push_back(TB_PARAM_ECDSAP256); I have 2 new fields in SSLConfig: token_binding_enabled, and token_binding_params. The former defaults to false (off). The behavior that makes most sense to me is that a user of token binding (e.g. HttpNetworkTransaction) only needs to flip token_binding_enabled, so I'm setting token_binding_params to a default here. I imagine in the future when we support multiple key params I'll modify SSLConfigServiceManagerPref to control set token_binding_params based on some future switch to enable a new param. Other options I could do with this: * set token_binding_params in SSLConfigServiceManagerPref now (so that in SSLConfig it's initialized to an empty vector) * rip out token_binding_params completely for now (and add it back when there are multiple supported params)
Sorry I forgot about this. I think this looks good. Just a bunch of minor nits and the comment on how SSLConfig should work. https://codereview.chromium.org/1360633002/diff/340001/net/log/net_log_event_... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/1360633002/diff/340001/net/log/net_log_event_... net/log/net_log_event_type_list.h:483: EVENT_TYPE(SSL_GET_TOKEN_BINDING_KEY) Unused? https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2221: } What's wrong with class ScopedCBB { public: ScopedCBB() { CBB_zero(&cbb_); } ~ScopedCBB() { CBB_cleanup(&cbb_); } CBB* get() { return cbb_; } private: CBB cbb_; DISALLOW_COPY_AND_ASSIGN(ScopedCBB); }; Shorter, more efficient, and that's how the API is meant to be used. I specifically added CBB_zero so that it would be scopable without complications. (CBB_cleanup is defined to be valid in the zero state.) At the least, it's probably best to not define these OpenSSL-style, so you don't collide with OpenSSL names later. Also no reason you can't use new/delete or something or assume malloc doesn't fail. https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2273: CBS_len(¶meters_list) > 0) { Super nitpicky nit: I think it's mildly better to check CBS_len of parameters_list before extension, that way you assert empty in stack order. https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2298: } Optional: std::find I think works here too. Might be cleaner? Your call. https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2311: void* add_arg) { Most of the other FooCallback wrappers were static methods on the SSLContext which then (like you're doing here) forward to the SSLClientSocketOpenSSL. You can move Register up into SSLContext too and probably just inline it. https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:216: // methods; The Free callback is a wrapper around OPENSSL_free. "; The" -> "; the" Though I'd probably just drop that bit altogether. I don't think readers of the header file need to know that's what the Free callbacbk is. https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:2593: EnableChannelID(); Is there actually any need to EnableChannelID in these tests? The two systems are pretty disconnected at this layer I think. https://codereview.chromium.org/1360633002/diff/340001/net/ssl/ssl_config.cc File net/ssl/ssl_config.cc (right): https://codereview.chromium.org/1360633002/diff/340001/net/ssl/ssl_config.cc#... net/ssl/ssl_config.cc:38: token_binding_params.push_back(TB_PARAM_ECDSAP256); On 2015/10/31 02:01:23, nharper wrote: > I have 2 new fields in SSLConfig: token_binding_enabled, and > token_binding_params. The former defaults to false (off). The behavior that > makes most sense to me is that a user of token binding (e.g. > HttpNetworkTransaction) only needs to flip token_binding_enabled, so I'm setting > token_binding_params to a default here. I imagine in the future when we support > multiple key params I'll modify SSLConfigServiceManagerPref to control set > token_binding_params based on some future switch to enable a new param. > > Other options I could do with this: > * set token_binding_params in SSLConfigServiceManagerPref now (so that in > SSLConfig it's initialized to an empty vector) > * rip out token_binding_params completely for now (and add it back when there > are multiple supported params) I think I would do one of: - Caller sets bool token_binding_enabled and SSLClientSocketOpenSSL interprets that to mean advertise the one and only configuration. - Caller sets a std::vector token_binding_params in full glory. The interpretation of the former would be roughly what this is now, but without the unnecessary member in SSLConfig. "TB in SSLClientSocket means supporting TB_PARAM_ECDSAP256, end of story." The latter would be "TB in SSLClientSocket means you give me a list of TB_PARAM values and I'll tell you which it chose. It's your problem which key types you support". The latter is probably better? I still don't think SSLConfigServiceManagerPref should have anything to do with it. SSL has no clue where the keys come from or anything. Only HttpNetworkTransaction or other HTTP bits would know that our key store is capable of doing RSA keys or P-256 keys or whatever. So it should be a configuration at the HTTP layer. That is, when we get around to adding RSA keys, it's still not going to be an SSL-level config. It's going to be a question of whether the key store (an consumer-only notion) enables RSA keys or not. The SSL code is purely a blind pass-through.
https://codereview.chromium.org/1360633002/diff/340001/net/log/net_log_event_... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/1360633002/diff/340001/net/log/net_log_event_... net/log/net_log_event_type_list.h:483: EVENT_TYPE(SSL_GET_TOKEN_BINDING_KEY) On 2015/11/04 00:40:35, davidben wrote: > Unused? removed. https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2221: } On 2015/11/04 00:40:36, davidben wrote: > What's wrong with > > class ScopedCBB { > public: > ScopedCBB() { CBB_zero(&cbb_); } > ~ScopedCBB() { CBB_cleanup(&cbb_); } > > CBB* get() { return cbb_; } > private: > CBB cbb_; > DISALLOW_COPY_AND_ASSIGN(ScopedCBB); > }; > > Shorter, more efficient, and that's how the API is meant to be used. I > specifically added CBB_zero so that it would be scopable without complications. > (CBB_cleanup is defined to be valid in the zero state.) > > At the least, it's probably best to not define these OpenSSL-style, so you don't > collide with OpenSSL names later. Also no reason you can't use new/delete or > something or assume malloc doesn't fail. Nothing's wrong with it. I thought you were suggesting using crypto::ScopedOpenSSL; writing this short ScopedCBB class seems like it's shorter and simpler. https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2273: CBS_len(¶meters_list) > 0) { On 2015/11/04 00:40:36, davidben wrote: > Super nitpicky nit: I think it's mildly better to check CBS_len of > parameters_list before extension, that way you assert empty in stack order. Done. https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2298: } On 2015/11/04 00:40:36, davidben wrote: > Optional: std::find I think works here too. Might be cleaner? Your call. std::find works, but the code is just as long (or short). I'd prefer to leave it as is. (The rest of this method is much more C-like than C++-like.) https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2311: void* add_arg) { On 2015/11/04 00:40:35, davidben wrote: > Most of the other FooCallback wrappers were static methods on the SSLContext > which then (like you're doing here) forward to the SSLClientSocketOpenSSL. You > can move Register up into SSLContext too and probably just inline it. I inlined Register in SSLContext. Are you also suggesting I move the static callback methods to SSLContext? https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:216: // methods; The Free callback is a wrapper around OPENSSL_free. On 2015/11/04 00:40:36, davidben wrote: > "; The" -> "; the" > > Though I'd probably just drop that bit altogether. I don't think readers of the > header file need to know that's what the Free callbacbk is. Done. https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:2593: EnableChannelID(); On 2015/11/04 00:40:36, davidben wrote: > Is there actually any need to EnableChannelID in these tests? The two systems > are pretty disconnected at this layer I think. They were from when I needed a ChannelIDService (and the layers were not so disconnected), but you are correct that it is no longer needed. (Likewise, it no longer needs to be an SSLClientSocketChannelIDTest, just an SSLClientSocketTest.) https://codereview.chromium.org/1360633002/diff/340001/net/ssl/ssl_config.cc File net/ssl/ssl_config.cc (right): https://codereview.chromium.org/1360633002/diff/340001/net/ssl/ssl_config.cc#... net/ssl/ssl_config.cc:38: token_binding_params.push_back(TB_PARAM_ECDSAP256); On 2015/11/04 00:40:36, davidben wrote: > On 2015/10/31 02:01:23, nharper wrote: > > I have 2 new fields in SSLConfig: token_binding_enabled, and > > token_binding_params. The former defaults to false (off). The behavior that > > makes most sense to me is that a user of token binding (e.g. > > HttpNetworkTransaction) only needs to flip token_binding_enabled, so I'm > setting > > token_binding_params to a default here. I imagine in the future when we > support > > multiple key params I'll modify SSLConfigServiceManagerPref to control set > > token_binding_params based on some future switch to enable a new param. > > > > Other options I could do with this: > > * set token_binding_params in SSLConfigServiceManagerPref now (so that in > > SSLConfig it's initialized to an empty vector) > > * rip out token_binding_params completely for now (and add it back when there > > are multiple supported params) > > I think I would do one of: > > - Caller sets bool token_binding_enabled and SSLClientSocketOpenSSL interprets > that to mean advertise the one and only configuration. > > - Caller sets a std::vector token_binding_params in full glory. > > The interpretation of the former would be roughly what this is now, but without > the unnecessary member in SSLConfig. "TB in SSLClientSocket means supporting > TB_PARAM_ECDSAP256, end of story." > > The latter would be "TB in SSLClientSocket means you give me a list of TB_PARAM > values and I'll tell you which it chose. It's your problem which key types you > support". > > The latter is probably better? I still don't think SSLConfigServiceManagerPref > should have anything to do with it. SSL has no clue where the keys come from or > anything. Only HttpNetworkTransaction or other HTTP bits would know that our key > store is capable of doing RSA keys or P-256 keys or whatever. So it should be a > configuration at the HTTP layer. > > That is, when we get around to adding RSA keys, it's still not going to be an > SSL-level config. It's going to be a question of whether the key store (an > consumer-only notion) enables RSA keys or not. The SSL code is purely a blind > pass-through. The latter sounds reasonable to me.
Last pass. Sorry, my fault. Most of this I should have noticed in the previous pass. https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2311: void* add_arg) { On 2015/11/04 02:28:04, nharper wrote: > On 2015/11/04 00:40:35, davidben wrote: > > Most of the other FooCallback wrappers were static methods on the SSLContext > > which then (like you're doing here) forward to the SSLClientSocketOpenSSL. You > > can move Register up into SSLContext too and probably just inline it. > > I inlined Register in SSLContext. Are you also suggesting I move the static > callback methods to SSLContext? Yeah. Doesn't really matter, but it's more consistent with the rest. SSLContext can call private methods of SSLClientSocketOpenSSL just fine. https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:258: if (SSL_CTX_add_client_custom_ext( Nit: This can just be if (!SSL_CTX_....) { BoringSSL functions that return success/failure just use 1/0 without -1 unless they're something weird going on like reading from a BIO. (Will fix that eventually. Right now we're stuck with it.) https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2212: } Nit: I'd put this up in line 207 with all the other ones. https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2227: return 0; *out_alert_value = SSL_AD_INTERNAL_ERROR; return -1; https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2231: return 0; *out_alert_value = SSL_AD_INTERNAL_ERROR; return -1; https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2235: return 0; *out_alert_value = SSL_AD_INTERNAL_ERROR; return -1; https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2237: retval = 1; return 1; https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:2590: ssl_options.disable_channel_id = true; Not needed either. :-) https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:2609: ssl_options.disable_channel_id = true; Ditto.
nits; move custom ext callback static methods to SSLContext
https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/340001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2311: void* add_arg) { On 2015/11/04 17:40:13, davidben wrote: > On 2015/11/04 02:28:04, nharper wrote: > > On 2015/11/04 00:40:35, davidben wrote: > > > Most of the other FooCallback wrappers were static methods on the SSLContext > > > which then (like you're doing here) forward to the SSLClientSocketOpenSSL. > You > > > can move Register up into SSLContext too and probably just inline it. > > > > I inlined Register in SSLContext. Are you also suggesting I move the static > > callback methods to SSLContext? > > Yeah. Doesn't really matter, but it's more consistent with the rest. SSLContext > can call private methods of SSLClientSocketOpenSSL just fine. I moved them into SSLContext to go with the rest of the callbacks there. https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:258: if (SSL_CTX_add_client_custom_ext( On 2015/11/04 17:40:13, davidben wrote: > Nit: This can just be if (!SSL_CTX_....) { > > BoringSSL functions that return success/failure just use 1/0 without -1 unless > they're something weird going on like reading from a BIO. (Will fix that > eventually. Right now we're stuck with it.) Done. https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2212: } On 2015/11/04 17:40:13, davidben wrote: > Nit: I'd put this up in line 207 with all the other ones. Done. https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2227: return 0; On 2015/11/04 17:40:13, davidben wrote: > *out_alert_value = SSL_AD_INTERNAL_ERROR; > return -1; Done. https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2231: return 0; On 2015/11/04 17:40:13, davidben wrote: > *out_alert_value = SSL_AD_INTERNAL_ERROR; > return -1; Done. https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2235: return 0; On 2015/11/04 17:40:13, davidben wrote: > *out_alert_value = SSL_AD_INTERNAL_ERROR; > return -1; Done. https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2237: retval = 1; On 2015/11/04 17:40:13, davidben wrote: > return 1; Done. (This was from when I was using goto.) https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:2590: ssl_options.disable_channel_id = true; On 2015/11/04 17:40:13, davidben wrote: > Not needed either. :-) Done. https://codereview.chromium.org/1360633002/diff/360001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:2609: ssl_options.disable_channel_id = true; On 2015/11/04 17:40:13, davidben wrote: > Ditto. Done.
rebase
Sorry, I missed that you'd updated the CL somehow. lgtm!
The CQ bit was checked by nharper@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1360633002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1360633002/400001
Message was sent while issue was closed.
Committed patchset #20 (id:400001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/736ceda358a1c2b576b41d09a108f2d1774e0e4d Cr-Commit-Position: refs/heads/master@{#358465} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
