Implement Token Binding negotiation TLS extension

chrome/browser/net/ssl_config_service_manager_pref.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 #include "chrome/browser/net/ssl_config_service_manager.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
@@ skipping 140 matching lines @@
   void OnDisabledCipherSuitesChange(PrefService* local_state);
 
   PrefChangeRegistrar local_state_change_registrar_;
 
   // The local_state prefs (should only be accessed from UI thread)
   BooleanPrefMember rev_checking_enabled_;
   BooleanPrefMember rev_checking_required_local_anchors_;
   StringPrefMember ssl_version_min_;
   StringPrefMember ssl_version_max_;
   StringPrefMember ssl_version_fallback_min_;
+  BooleanPrefMember token_binding_enabled_;
 
   // The cached list of disabled SSL cipher suites.
   std::vector<uint16> disabled_cipher_suites_;
 
   scoped_refptr<SSLConfigServicePref> ssl_config_service_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLConfigServiceManagerPref);
 };
 
 SSLConfigServiceManagerPref::SSLConfigServiceManagerPref(
@@ skipping 11 matching lines @@
   rev_checking_required_local_anchors_.Init(
       prefs::kCertRevocationCheckingRequiredLocalAnchors,
       local_state,
       local_state_callback);
   ssl_version_min_.Init(
       prefs::kSSLVersionMin, local_state, local_state_callback);
   ssl_version_max_.Init(
       prefs::kSSLVersionMax, local_state, local_state_callback);
   ssl_version_fallback_min_.Init(
       prefs::kSSLVersionFallbackMin, local_state, local_state_callback);
+  token_binding_enabled_.Init(
+      prefs::kEnableTokenBinding, local_state, local_state_callback);
 
   local_state_change_registrar_.Init(local_state);
   local_state_change_registrar_.Add(
       prefs::kCipherSuiteBlacklist, local_state_callback);
 
   OnDisabledCipherSuitesChange(local_state);
 
   // Initialize from UI thread.  This is okay as there shouldn't be anything on
   // the IO thread trying to access it yet.
   GetSSLConfigFromPrefs(&ssl_config_service_->cached_config_);
 }
 
 // static
 void SSLConfigServiceManagerPref::RegisterPrefs(PrefRegistrySimple* registry) {
   net::SSLConfig default_config;
   registry->RegisterBooleanPref(prefs::kCertRevocationCheckingEnabled,
                                 default_config.rev_checking_enabled);
   registry->RegisterBooleanPref(
       prefs::kCertRevocationCheckingRequiredLocalAnchors,
       default_config.rev_checking_required_local_anchors);
   registry->RegisterStringPref(prefs::kSSLVersionMin, std::string());
   registry->RegisterStringPref(prefs::kSSLVersionMax, std::string());
   registry->RegisterStringPref(prefs::kSSLVersionFallbackMin, std::string());
+  registry->RegisterBooleanPref(
+      prefs::kEnableTokenBinding,
+      default_config.token_binding_params.size() == 1 &&
+          default_config.token_binding_params[0] ==
+              net::TB_PARAM_ECDSAP256_SHA256);

[davidben, 2015/10/01 16:15:17]

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.

[nharper, 2015/10/01 19:12:23]

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.

[davidben, 2015/10/15 21:52:08]

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.
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.

[nharper, 2015/10/20 22:52:18]

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).

   registry->RegisterListPref(prefs::kCipherSuiteBlacklist);
 }
 
 net::SSLConfigService* SSLConfigServiceManagerPref::Get() {
   return ssl_config_service_.get();
 }
 
 void SSLConfigServiceManagerPref::OnPreferenceChanged(
     PrefService* prefs,
     const std::string& pref_name_in) {
@@ skipping 40 matching lines @@
     config->version_min = version_min;
   }
   if (version_max) {
     uint16 supported_version_max = config->version_max;
     config->version_max = std::min(supported_version_max, version_max);
   }
   if (version_fallback_min) {
     config->version_fallback_min = version_fallback_min;
   }
   config->disabled_cipher_suites = disabled_cipher_suites_;
+  if (token_binding_enabled_.GetValue()) {
+    config->token_binding_params.clear();
+    config->token_binding_params.push_back(net::TB_PARAM_ECDSAP256_SHA256);
+  }
 }
 
 void SSLConfigServiceManagerPref::OnDisabledCipherSuitesChange(
     PrefService* local_state) {
   const base::ListValue* value =
       local_state->GetList(prefs::kCipherSuiteBlacklist);
   disabled_cipher_suites_ = ParseCipherSuites(ListValueToStringVector(value));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 //  SSLConfigServiceManager
 
 // static
 SSLConfigServiceManager* SSLConfigServiceManager::CreateDefaultManager(
     PrefService* local_state) {
   return new SSLConfigServiceManagerPref(local_state);
 }
 
 // static
 void SSLConfigServiceManager::RegisterPrefs(PrefRegistrySimple* registry) {
   SSLConfigServiceManagerPref::RegisterPrefs(registry);
 }