Skip to content

Commit

Permalink
net: don't pass the CRLSet in the SSLConfig.
Browse files Browse the repository at this point in the history
The SSLConfig was a poor choice of location to carry the CRLSet because the
CRLSet can be updated while Chrome is running, but the SSLConfig is relatively
static and is cached in several places in the code.

This change causes the locations which call X509Certificate::Verify to grab a
new reference to the current CRLSet.

BUG=none
TEST=compiles


Review URL: http://codereview.chromium.org/9044011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@116720 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
agl@chromium.org committed Jan 6, 2012
1 parent f6cff51 commit c83f433
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 19 deletions.
3 changes: 1 addition & 2 deletions chrome/browser/net/ssl_config_service_manager_pref.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// 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"
Expand Down Expand Up @@ -93,7 +93,6 @@ class SSLConfigServicePref : public net::SSLConfigService {

void SSLConfigServicePref::GetSSLConfig(net::SSLConfig* config) {
*config = cached_config_;
config->crl_set = GetCRLSet();
}

void SSLConfigServicePref::SetNewSSLConfig(
Expand Down
33 changes: 28 additions & 5 deletions net/base/ssl_config_service.cc
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// 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 "net/base/ssl_config_service.h"

#include "base/lazy_instance.h"
#include "base/memory/ref_counted.h"
#include "base/synchronization/lock.h"
#include "net/base/crl_set.h"
#include "net/base/ssl_config_service_defaults.h"
#include "net/base/ssl_false_start_blacklist.h"
Expand Down Expand Up @@ -61,8 +62,29 @@ bool SSLConfigService::IsKnownFalseStartIncompatibleServer(
static bool g_cached_info_enabled = false;
static bool g_false_start_enabled = true;
static bool g_dns_cert_provenance_checking = false;
base::LazyInstance<scoped_refptr<CRLSet>,
base::LeakyLazyInstanceTraits<scoped_refptr<CRLSet> > >

// GlobalCRLSet holds a reference to the global CRLSet. It simply wraps a lock
// around a scoped_refptr so that getting a reference doesn't race with
// updating the CRLSet.
class GlobalCRLSet {
public:
void Set(const scoped_refptr<CRLSet>& new_crl_set) {
base::AutoLock locked(lock_);
crl_set_ = new_crl_set;
}

scoped_refptr<CRLSet> Get() const {
base::AutoLock locked(lock_);
return crl_set_;
}

private:
scoped_refptr<CRLSet> crl_set_;
mutable base::Lock lock_;
};

base::LazyInstance<GlobalCRLSet,
base::LeakyLazyInstanceTraits<GlobalCRLSet> >
g_crl_set = LAZY_INSTANCE_INITIALIZER;

// static
Expand All @@ -87,12 +109,13 @@ bool SSLConfigService::dns_cert_provenance_checking_enabled() {

// static
void SSLConfigService::SetCRLSet(scoped_refptr<CRLSet> crl_set) {
g_crl_set.Get() = crl_set;
// Note: this can be called concurently with GetCRLSet().
g_crl_set.Get().Set(crl_set);
}

// static
scoped_refptr<CRLSet> SSLConfigService::GetCRLSet() {
return g_crl_set.Get();
return g_crl_set.Get().Get();
}

void SSLConfigService::EnableCachedInfo() {
Expand Down
4 changes: 1 addition & 3 deletions net/base/ssl_config_service.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// 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.

Expand Down Expand Up @@ -105,8 +105,6 @@ struct NET_EXPORT SSLConfig {
std::vector<std::string> next_protos;

scoped_refptr<X509Certificate> client_cert;

scoped_refptr<CRLSet> crl_set;
};

// The interface for retrieving the SSL configuration. This interface
Expand Down
3 changes: 1 addition & 2 deletions net/base/ssl_config_service_defaults.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// 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.

Expand All @@ -12,7 +12,6 @@ SSLConfigServiceDefaults::SSLConfigServiceDefaults() {
void SSLConfigServiceDefaults::GetSSLConfig(SSLConfig* config) {
*config = default_config_;
SetSSLConfigFlags(config);
config->crl_set = GetCRLSet();
}

SSLConfigServiceDefaults::~SSLConfigServiceDefaults() {
Expand Down
4 changes: 2 additions & 2 deletions net/socket/ssl_client_socket_nss.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// 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.

Expand Down Expand Up @@ -1714,7 +1714,7 @@ int SSLClientSocketNSS::DoVerifyCert(int result) {
server_cert_verify_result_ = &local_server_cert_verify_result_;
return verifier_->Verify(
server_cert_, host_and_port_.host(), flags,
ssl_config_.crl_set,
SSLConfigService::GetCRLSet(),
&local_server_cert_verify_result_,
base::Bind(&SSLClientSocketNSS::OnHandshakeIOComplete,
base::Unretained(this)),
Expand Down
6 changes: 4 additions & 2 deletions net/socket/ssl_host_info.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// 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.

Expand All @@ -8,6 +8,7 @@
#include "base/metrics/histogram.h"
#include "base/pickle.h"
#include "base/string_piece.h"
#include "net/base/crl_set.h"
#include "net/base/ssl_config_service.h"
#include "net/base/x509_certificate.h"
#include "net/socket/ssl_client_socket.h"
Expand Down Expand Up @@ -112,8 +113,9 @@ bool SSLHostInfo::ParseInner(const std::string& data) {
VLOG(1) << "Kicking off verification for " << hostname_;
verification_start_time_ = base::TimeTicks::Now();
verification_end_time_ = base::TimeTicks();
scoped_refptr<CRLSet> crl_set(SSLConfigService::GetCRLSet());
int rv = verifier_.Verify(
cert_.get(), hostname_, flags, crl_set_, &cert_verify_result_,
cert_.get(), hostname_, flags, crl_set, &cert_verify_result_,
base::Bind(&SSLHostInfo::VerifyCallback, weak_factory_.GetWeakPtr()),
// TODO(willchan): Figure out how to use NetLog here.
BoundNetLog());
Expand Down
4 changes: 1 addition & 3 deletions net/socket/ssl_host_info.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// 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.

Expand All @@ -20,7 +20,6 @@

namespace net {

class CRLSet;
class X509Certificate;
struct SSLConfig;

Expand Down Expand Up @@ -121,7 +120,6 @@ class NET_EXPORT_PRIVATE SSLHostInfo {
// These three members are taken from the SSLConfig.
bool rev_checking_enabled_;
bool verify_ev_cert_;
scoped_refptr<CRLSet> crl_set_;
base::TimeTicks verification_start_time_;
base::TimeTicks verification_end_time_;
CertVerifyResult cert_verify_result_;
Expand Down

0 comments on commit c83f433

Please sign in to comment.