Remove the deprecated NSSCertDatabase::GetInstance() .

Remove the deprecated NSSCertDatabase::GetInstance() .

The NSSCertDatabase singleton for Linux is now maintained by nss_context_linux.cc .

BUG=329735
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285551

[pneubeck (no reviews), 2014-07-21 14:43:29]

ptal

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database_u...
File net/cert/nss_cert_database_unittest.cc (right):

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database_u...
net/cert/nss_cert_database_unittest.cc:62:
crypto::ScopedPK11Slot(crypto::GetPersistentNSSKeySlot())));
as with the KeygenHandler unit test, this will be updated in a follow up to not
depend on GetPersistentNSSKeySlot and instead to directly use the slot provided
by test_nssdb_.

[Ryan Sleevi, 2014-07-22 01:18:33]

https://codereview.chromium.org/405973003/diff/1/chrome/browser/net/nss_conte...
File chrome/browser/net/nss_context_linux.cc (right):

https://codereview.chromium.org/405973003/diff/1/chrome/browser/net/nss_conte...
chrome/browser/net/nss_context_linux.cc:36: return g_nss_cert_database;
The only reason this is safe is because of the DCHECK. If it wasn't, this could
be a double-write security bug.

Promote the DCHECK to a CHECK? Or make it use a LazyInstance<T>::Leaky, which
uses atomics for true thread-safety.

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database.cc
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database.c...
net/cert/nss_cert_database.cc:128: return crypto::ScopedPK11Slot();
Why is NULL valid? Requires updating the header files, but more importantly, I
think this is a breaking change and we should NEVER allow these to be NULL.

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database.h
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database.h...
net/cert/nss_cert_database.h:104: // nss_context.h.
Any chance of hiding this for future tests, and friending the tests explicitly?
Or having a bit in net_test_support that will create an NSSCertDatabase (which
is friended), and require callers go through it, which really ensures its
testing only (since only test targets can pull in net_test_support)

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database_u...
File net/cert/nss_cert_database_unittest.cc (right):

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database_u...
net/cert/nss_cert_database_unittest.cc:98: CertificateList ListCertsInSlot() {
Should this just be ListCerts? The concept of Slot is no longer applicable, is
it?

[pneubeck (no reviews), 2014-07-22 08:23:56]

https://codereview.chromium.org/405973003/diff/1/chrome/browser/net/nss_conte...
File chrome/browser/net/nss_context_linux.cc (right):

https://codereview.chromium.org/405973003/diff/1/chrome/browser/net/nss_conte...
chrome/browser/net/nss_context_linux.cc:36: return g_nss_cert_database;
On 2014/07/22 01:18:32, Ryan Sleevi wrote:
> The only reason this is safe is because of the DCHECK. If it wasn't, this
could
> be a double-write security bug.
> 
> Promote the DCHECK to a CHECK? Or make it use a LazyInstance<T>::Leaky, which
> uses atomics for true thread-safety.

The DCHECK indicates that we already don't handle access from other threads
correctly.
To be on the safe side promoting to CHECK.
Added a code, that nobody is removing the CHECK by mistake.

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database.cc
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database.c...
net/cert/nss_cert_database.cc:128: return crypto::ScopedPK11Slot();
On 2014/07/22 01:18:32, Ryan Sleevi wrote:
> Why is NULL valid? Requires updating the header files, but more importantly, I
> think this is a breaking change and we should NEVER allow these to be NULL.

Done.

Note that the DCHECK (PK11_REferenceSlot should crash on NULL) is here at the
usage site and not in the constructor, because the subclass
NSSCertDatabaseChromeOS overrides these to getters and doesn't use the
persistent_slot_.

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database.h
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database.h...
net/cert/nss_cert_database.h:104: // nss_context.h.
On 2014/07/22 01:18:32, Ryan Sleevi wrote:
> Any chance of hiding this for future tests, and friending the tests
explicitly?
> Or having a bit in net_test_support that will create an NSSCertDatabase (which
> is friended), and require callers go through it, which really ensures its
> testing only (since only test targets can pull in net_test_support)

I'd punt such a change into a separate CL and it should be consistent with
NSSCertDatabaseChromeOS (which currently has a public c'tor like after this
change).

Independent of when and how we change this. What do you suggest to protect
against? Once this class doesn't reference any global state (except NSS of
course), is it really likely that anybody will mistakenly use this constructor?


Regarding your proposal: not sure how you would whitelist the legitimate use in
chrome/browser/net/nss_context_linux.cc ?

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database_u...
File net/cert/nss_cert_database_unittest.cc (right):

https://codereview.chromium.org/405973003/diff/1/net/cert/nss_cert_database_u...
net/cert/nss_cert_database_unittest.cc:98: CertificateList ListCertsInSlot() {
On 2014/07/22 01:18:32, Ryan Sleevi wrote:
> Should this just be ListCerts? The concept of Slot is no longer applicable, is
> it?

Done.

[Ryan Sleevi, 2014-07-22 08:31:58]

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
net/cert/nss_cert_database.cc:42: // TODO(pneubeck): Move this class to
chrome/browser/net/nss_context .
TODO with a bug #?

Feels gross mentioning //chrome/browser/net in //net here.

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
net/cert/nss_cert_database.h:104: // nss_context.h.
This comment isn't accurate though, is it? It's not just for tests (c.f.
//chrome/browser/net/nss_context.h)

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
File net/cert/nss_cert_database_chromeos.cc (right):

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
net/cert/nss_cert_database_chromeos.cc:24: :
NSSCertDatabase(crypto::ScopedPK11Slot() /* no persistent slot */),
Why is there no persistent slot for the ChromeOS?

What does it mean to SetCertTrust on an NSSCertDatabaseChromeOS? That violates
the API contract, doesn't it?

Why isn't it just public_slot?

[pneubeck (no reviews), 2014-07-22 09:51:50]

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
net/cert/nss_cert_database.cc:42: // TODO(pneubeck): Move this class to
chrome/browser/net/nss_context .
On 2014/07/22 08:31:58, Ryan Sleevi wrote:
> TODO with a bug #?
> 
> Feels gross mentioning //chrome/browser/net in //net here.

Done.

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
net/cert/nss_cert_database.h:104: // nss_context.h.
On 2014/07/22 08:31:58, Ryan Sleevi wrote:
> This comment isn't accurate though, is it? It's not just for tests (c.f.
> //chrome/browser/net/nss_context.h)

Yes, I realized too late this might be misleading.
Considering your other comment, I think you're not happy about the reference to
c/b/n/nss_context anyways.

Changing this to a more general warning.
Alternatively, we could just drop the comment altogether.

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
File net/cert/nss_cert_database_chromeos.cc (right):

https://codereview.chromium.org/405973003/diff/20001/net/cert/nss_cert_databa...
net/cert/nss_cert_database_chromeos.cc:24: :
NSSCertDatabase(crypto::ScopedPK11Slot() /* no persistent slot */),
On 2014/07/22 08:31:58, Ryan Sleevi wrote:
> Why is there no persistent slot for the ChromeOS?
> 
> What does it mean to SetCertTrust on an NSSCertDatabaseChromeOS? That violates
> the API contract, doesn't it?
> 
> Why isn't it just public_slot?

We only run into issues here because the current Linux impl (only one slot) is
different from the ChromeOS impl (pub + priv slot), but the ChromeOS impl is
deriving from the Linux impl and overrides impl of the Linux impl. This is
probably the worst kind of inheritance.

For now we have to make the best out of it. Since we agreed that we don't want
to deviate ChromeOS impl even more from the Linux one, I change this to two
members in the Linux impl.

[pneubeck (no reviews), 2014-07-23 14:53:25]

Steven, please take a look at the CertLoader changes; the NSSCertDatabase
implementation has changed and shouldn't return NULL on GetPrivateSlot.

[stevenjb, 2014-07-23 16:45:09]

OWNER lgtm for chromeos/

[Ryan Sleevi, 2014-07-24 23:19:10]

lgtm

https://codereview.chromium.org/405973003/diff/110001/chrome/browser/net/nss_...
File chrome/browser/net/nss_context_linux.cc (right):

https://codereview.chromium.org/405973003/diff/110001/chrome/browser/net/nss_...
chrome/browser/net/nss_context_linux.cc:12: net::NSSCertDatabase*
g_nss_cert_database = NULL;
You'll probably get LSAN yelling at you. it has a built in suppression for leaky
lazy instances. Just fwiw

https://codereview.chromium.org/405973003/diff/110001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/405973003/diff/110001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:104: // Both slots must not be NULL but can be
identical.
Well that's not very helpful of a comment. How do I know whether I know what I'm
doing?

// Creates a NSSCertDatabase that will store public information (such as
// certificates and trust records) in |public_slot|, and private information
(such as keys)
// in |private_slot|.
// In general, code should avoid creating an NSSCertDatabase directly,
// as doing so requires making opinionated decisions about where to store
// data, and instead prefer to be passed an existing NSSCertDatabase
// instance.

[pneubeck (no reviews), 2014-07-25 08:15:01]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-07-25 08:15:21]

Thanks for the review!

https://codereview.chromium.org/405973003/diff/110001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/405973003/diff/110001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:104: // Both slots must not be NULL but can be
identical.
On 2014/07/24 23:19:10, Ryan Sleevi wrote:
> Well that's not very helpful of a comment. How do I know whether I know what
I'm
> doing?
> 
> // Creates a NSSCertDatabase that will store public information (such as
> // certificates and trust records) in |public_slot|, and private information
> (such as keys)
> // in |private_slot|.
> // In general, code should avoid creating an NSSCertDatabase directly,
> // as doing so requires making opinionated decisions about where to store
> // data, and instead prefer to be passed an existing NSSCertDatabase
> // instance.

Done.

[commit-bot: I haz the power, 2014-07-25 08:17:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/405973003/130001

[commit-bot: I haz the power, 2014-07-25 10:10:04]

Change committed as 285551