Fix imported server certs being distrusted in NSS 3.13.

Fix imported server certs being distrusted in NSS 3.13.

Add support for intentionally distrusting certs.  (Not exposed in the UI yet.)

BUG=116411
TEST=CertDatabaseNSSTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139719

[mattm, 2012-03-29 23:25:53]

I'm not familiar with the OpenSSL trust mechanisms, so let me know if the
TrustBits enum change makes sense.

[Ryan Sleevi, 2012-03-29 23:35:13]

This really doesn't have any comparison for OpenSSL. The addition is very much
an NSS-only change, which makes the "CertDatabase as a generic interface" a bit
problematic.

Perhaps it should be split up at some point. Comments below.

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database.h#new...
net/base/cert_database.h:94: TRUST_TERMINAL_RECORD = 1 << 3,
This is unfortunately very specific to NSS, which I think makes it hard to see
this as a generic interface.

While I realize this is an existing issue, it might be worth considering moving
this into an #ifdef for NSS.

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database.h#new...
net/base/cert_database.h:169: TrustBits trust_bits,
OpenSSL and Android have no such concept.

Android: http://developer.android.com/reference/android/security/KeyChain.html

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:91: }
Can you use the net/test/cert_test_util function here?

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:649: X509Certificate::FORMAT_AUTO);
Use the net/base/cert_test_util helpers here (and for the tests below)

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsNSSCertTrust.h (right):

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsNSSCertTrust.h:70: PRBool
checkObjSign = PR_TRUE);
Is this our code or is this upstream?

Does the license block need to be updated?

[wtc, 2012-03-30 22:00:50]

Review comments on patch set 2:

Thanks for writing the changelist.  There are some hard
issues we need to resolve, such as the trust bits used to
represent neutral trust (that is, derives the trust from its
issuer in the usual way) or explicit distrust.  So my review
ignores those issues for now.

http://codereview.chromium.org/9940001/diff/4003/chrome/browser/certificate_m...
File chrome/browser/certificate_manager_model.h (right):

http://codereview.chromium.org/9940001/diff/4003/chrome/browser/certificate_m...
chrome/browser/certificate_manager_model.h:89: net::CertDatabase::TrustBits
trust_bits,

Document the new trust_bits parameter.

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database.h#new...
net/base/cert_database.h:94: TRUST_TERMINAL_RECORD = 1 << 3,

We need to think this through.  The low-level NSS trust flags
are hard to understand.  Ideally we should create a more
intuitive set of trust flags here and map them to the NSS
trust flags internally.

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database.h#new...
net/base/cert_database.h:169: TrustBits trust_bits,

On 2012/03/29 23:35:13, Ryan Sleevi wrote:
> OpenSSL and Android have no such concept.

One solution might be to make ImportCACerts and
ImportServerCert do nothing but importing the certs, and
require a separate SetCertTrust call to set the trust.
This allows us to implement ImportCACerts and
ImportServerCert (with neutral trust) with OpenSSL and
Android.

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsNSSCertTrust.h (right):

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsNSSCertTrust.h:68: PRBool
HasTerminalRecord(PRBool checkSSL = PR_TRUE,

HasExplicitTrustOrDistrust may be a better name for
this function.  But HasTerminalRecord reflects the underlying
NSS code more closely.

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsNSSCertTrust.h:70: PRBool
checkObjSign = PR_TRUE);

On 2012/03/29 23:35:13, Ryan Sleevi wrote:
> Is this our code or is this upstream?

These files are our permanent fork of Mozilla's certificate
UI code (called Personal Security Manager).  We don't need
to merge with the current upstream.  But we need to keep
the MPL license.

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsNSSCertTrust.h:89: void
SetTerminalServerRecord();

This function should be named SetDistrustedServer.

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsNSSCertTrust.h:91: void
SetTerminalRecord();

This should be named SetDistrustedCert.

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsNSSCertTrust.h:93: void
SetTrustedPeer();

This should be named SetTrustedCert to avoid the use of
"Peer".

[mattm, 2012-03-30 22:16:56]

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database.h#new...
net/base/cert_database.h:169: TrustBits trust_bits,
On 2012/03/30 22:00:50, wtc wrote:
> 
> On 2012/03/29 23:35:13, Ryan Sleevi wrote:
> > OpenSSL and Android have no such concept.
> 
> One solution might be to make ImportCACerts and
> ImportServerCert do nothing but importing the certs, and
> require a separate SetCertTrust call to set the trust.
> This allows us to implement ImportCACerts and
> ImportServerCert (with neutral trust) with OpenSSL and
> Android.

Looking at the link Ryan posted, I'm not sure android should  use these
functions at all.  It looks like the only way to import a cert there is through
an Intent (does it involve user confirmation even if the app supplies the cert
to add?).  Synchronous import functions like these might not really be usable
for android?

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsNSSCertTrust.h (right):

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsNSSCertTrust.h:89: void
SetTerminalServerRecord();
On 2012/03/30 22:00:50, wtc wrote:
> 
> This function should be named SetDistrustedServer.

But a trusted server cert should have CERTDB_TRUSTED | CERTDB_TERMINAL_RECORD,
the way I was using it was to first set the SetTerminalServerRecord() and then
AddPeerTrust, calling it SetDistrustedServer would make that code confusing. 
But perhaps it makes more sense to change the AddPeerTrust to add the
CERTDB_TERMINAL_RECORD too (is there any reason you would want to have
CERTDB_TRUSTED without CERTDB_TERMINAL_RECORD?)

[wtc, 2012-03-30 22:39:07]

mattm: I think the way nsNSSCertTrust sets the NSS trust
flags could be improved.  It may even be clearer for us to
set the trust flags directly rather than using nsNSSCertTrust.

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsNSSCertTrust.h (right):

http://codereview.chromium.org/9940001/diff/4003/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsNSSCertTrust.h:89: void
SetTerminalServerRecord();

On 2012/03/30 22:16:56, mattm wrote:
>
> But a trusted server cert should have CERTDB_TRUSTED | CERTDB_TERMINAL_RECORD,
> the way I was using it was to first set the SetTerminalServerRecord() and then
> AddPeerTrust, calling it SetDistrustedServer would make that code confusing. 

I see.  Yes, that would make the code confusing.

> But perhaps it makes more sense to change the AddPeerTrust to add the
> CERTDB_TERMINAL_RECORD too (is there any reason you would want to have
> CERTDB_TRUSTED without CERTDB_TERMINAL_RECORD?)

I suspect CERTDB_TRUSTED is ignored if CERTDB_TERMINAL_RECORD
is not set.  Here is the NSS code:
http://mxr.mozilla.org/security/ident?i=CERTDB_TRUSTED

You see that when NSS sets CERTDB_TRUSTED, it always
sets CERTDB_TERMINAL_RECORD at the same time.

Similarly, NSS code that tests CERTDB_TRUSTED seems
to always test CERTDB_TERMINAL_RECORD first.  For
example, the cert_CheckLeafTrust function works that
way:
http://mxr.mozilla.org/security/ident?i=cert_CheckLeafTrust

[Ryan Sleevi, 2012-03-30 22:42:36]

I'll let wtc do the review, I was just responding to the OpenSSL-related
changes.

I'm concerned this abstraction doesn't really make sense for OpenSSL - or OS
X/Windows for that matter - but I don't think that's a reason to block this
change, as it's a pre-existing issue.

The flags don't really make sense for OpenSSL, but given that everything within
the _openssl implementation is NOTIMPLEMENTED, that's likely fine. I doubt it
ever will be implemented, at least as the interface is today, and we can work on
that at some other point.

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database.h#new...
net/base/cert_database.h:169: TrustBits trust_bits,
On 2012/03/30 22:16:56, mattm wrote:
> On 2012/03/30 22:00:50, wtc wrote:
> > 
> > On 2012/03/29 23:35:13, Ryan Sleevi wrote:
> > > OpenSSL and Android have no such concept.
> > 
> > One solution might be to make ImportCACerts and
> > ImportServerCert do nothing but importing the certs, and
> > require a separate SetCertTrust call to set the trust.
> > This allows us to implement ImportCACerts and
> > ImportServerCert (with neutral trust) with OpenSSL and
> > Android.
> 
> Looking at the link Ryan posted, I'm not sure android should  use these
> functions at all.  It looks like the only way to import a cert there is
through
> an Intent (does it involve user confirmation even if the app supplies the cert
> to add?).  Synchronous import functions like these might not really be usable
> for android?

Correct. And they'd like to fix that (and work is in progress somewhat).

I took a second look through cert_database_openssl, and it seems the entire
thing is NotImplemented. So really, we should remove the USE_OPENSSL from the
#if defined() on line 108 and accept that this is a huge NSS-specialization for
now. I'm fine with that. I think the only reason it was added was because of all
the NSS-specifics in the CertificateManagerModel that wanted to be re-used for
OpenSSL.

The only thing about this that is generic (today) is CheckUserCert/AddUserCert,
and both of those need to be made non-blocking callbacks at some point that run
on worker threads.

(AddObserver/RemoveObserver are questionably so - we want to implement them
correctly on other platforms, but as noticed in issues like
http://crbug.com/97462, this requires some UI-thread hopping for OS X, and FILE
thread for Win, so this is not really the same across platforms)

[mattm, 2012-05-16 03:30:45]

Updated.  I followed Ryan's suggestion of just making this NSS only, and changed
the TrustBits to have a hopefully more understandable way to specify distrust.

http://codereview.chromium.org/9940001/diff/4003/chrome/browser/certificate_m...
File chrome/browser/certificate_manager_model.h (right):

http://codereview.chromium.org/9940001/diff/4003/chrome/browser/certificate_m...
chrome/browser/certificate_manager_model.h:89: net::CertDatabase::TrustBits
trust_bits,
On 2012/03/30 22:00:50, wtc wrote:
> 
> Document the new trust_bits parameter.

Done.

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:91: }
On 2012/03/29 23:35:13, Ryan Sleevi wrote:
> Can you use the net/test/cert_test_util function here?

Done.

http://codereview.chromium.org/9940001/diff/4003/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:649: X509Certificate::FORMAT_AUTO);
On 2012/03/29 23:35:13, Ryan Sleevi wrote:
> Use the net/base/cert_test_util helpers here (and for the tests below)

Done.

[mattm, 2012-05-16 03:35:30]

Oh, and I followed Wan-Teh's suggestion that using nsNSSCertTrust isn't really
buying us much, switched to just checking the NSS trust values directly.

http://codereview.chromium.org/9940001/diff/24003/chrome/third_party/mozilla_...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

http://codereview.chromium.org/9940001/diff/24003/chrome/third_party/mozilla_...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1049: if
(cert->nickname && cert->trust->sslFlags & CERTDB_USER)
This version only checks the sslFlags when checking cert type.  Do you think
this is okay or should be check all the flags like the old version?

[Ryan Sleevi, 2012-05-16 03:57:23]

LGTM with a few comments and one or two maybe-bugs.

http://codereview.chromium.org/9940001/diff/24003/chrome/third_party/mozilla_...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

http://codereview.chromium.org/9940001/diff/24003/chrome/third_party/mozilla_...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1049: if
(cert->nickname && cert->trust->sslFlags & CERTDB_USER)
On 2012/05/16 03:35:30, mattm wrote:
> This version only checks the sslFlags when checking cert type.  Do you think
> this is okay or should be check all the flags like the old version?

For Linux, where users may have pre-existing certs/database/keys, I think it's
probably important to check them all, since we're implementing a PSM-equivalent
here.

If this was just ChromeOS, I think we'd be fine.

http://codereview.chromium.org/9940001/diff/30003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/certificate_manager_handler2.cc (right):

http://codereview.chromium.org/9940001/diff/30003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/certificate_manager_handler2.cc:811:
net::CertDatabase::UNTRUSTED,  // TODO(mattm): Add UI for trust.
open a bug for these two?

http://codereview.chromium.org/9940001/diff/30003/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/9940001/diff/30003/net/base/cert_database.h#ne...
net/base/cert_database.h:84: // not inherit trust from the issuer cert chain,
and the cert will be trusted
I don't see TRUST_TERMINAL_RECORD here, just EXPLICIT_DISTRUST

http://codereview.chromium.org/9940001/diff/30003/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/9940001/diff/30003/net/base/cert_database_nss....
net/base/cert_database_nss.cc:219: const unsigned kTrustedCA = CERTDB_TRUSTED_CA
| CERTDB_TRUSTED_CLIENT_CA;
Is this correct, to be passing CERTDB_TRUSTED_CLIENT_CA ?

According to
http://mxr.mozilla.org/security/source/security/nss/lib/certdb/certdb.h#17 ,
they're trusted for issuing client certs, but not necessarily servers.

This may only present a UI issue, since I don't believe any security decisions
are done based on this (please confirm the higher layers - certainly
CertVerifier-et-al don't)

http://codereview.chromium.org/9940001/diff/30003/net/base/cert_database_nss_...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/30003/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:619: scoped_refptr<CertVerifyProc>
verify_proc(CertVerifyProc::CreateDefault());
Blergh, this is a bug I should clean up.

This should be explicitly calling CertVerifyProcNSS, since we really are just
wanting access into the NSS routines for these checks.

http://codereview.chromium.org/9940001/diff/30003/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

http://codereview.chromium.org/9940001/diff/30003/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:196:
SetCertTrust(certificates[0].get(), net::SERVER_CERT, trustBits);
(Preexisting) bug?

What happens if |certificates.empty()| ?

All the code above handles it (eg: the for loop does nothing), but this could be
bad.

[mattm, 2012-05-16 22:30:50]

http://codereview.chromium.org/9940001/diff/24003/chrome/third_party/mozilla_...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

http://codereview.chromium.org/9940001/diff/24003/chrome/third_party/mozilla_...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1049: if
(cert->nickname && cert->trust->sslFlags & CERTDB_USER)
On 2012/05/16 03:57:23, Ryan Sleevi wrote:
> On 2012/05/16 03:35:30, mattm wrote:
> > This version only checks the sslFlags when checking cert type.  Do you think
> > this is okay or should be check all the flags like the old version?
> 
> For Linux, where users may have pre-existing certs/database/keys, I think it's
> probably important to check them all, since we're implementing a
PSM-equivalent
> here.
> 
> If this was just ChromeOS, I think we'd be fine.

Done.

http://codereview.chromium.org/9940001/diff/30003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/certificate_manager_handler2.cc (right):

http://codereview.chromium.org/9940001/diff/30003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/certificate_manager_handler2.cc:811:
net::CertDatabase::UNTRUSTED,  // TODO(mattm): Add UI for trust.
On 2012/05/16 03:57:23, Ryan Sleevi wrote:
> open a bug for these two?

filed / added links.

http://codereview.chromium.org/9940001/diff/30003/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/9940001/diff/30003/net/base/cert_database.h#ne...
net/base/cert_database.h:84: // not inherit trust from the issuer cert chain,
and the cert will be trusted
On 2012/05/16 03:57:23, Ryan Sleevi wrote:
> I don't see TRUST_TERMINAL_RECORD here, just EXPLICIT_DISTRUST
> 

fixed.

http://codereview.chromium.org/9940001/diff/30003/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/9940001/diff/30003/net/base/cert_database_nss....
net/base/cert_database_nss.cc:219: const unsigned kTrustedCA = CERTDB_TRUSTED_CA
| CERTDB_TRUSTED_CLIENT_CA;
On 2012/05/16 03:57:23, Ryan Sleevi wrote:
> Is this correct, to be passing CERTDB_TRUSTED_CLIENT_CA ?
> 
> According to
> http://mxr.mozilla.org/security/source/security/nss/lib/certdb/certdb.h#17 ,
> they're trusted for issuing client certs, but not necessarily servers.
> 
> This may only present a UI issue, since I don't believe any security decisions
> are done based on this (please confirm the higher layers - certainly
> CertVerifier-et-al don't)

Yeah, it's only used for the certificate manager trust editing.

I did fix the issue where it was checking for both rather than either.

http://codereview.chromium.org/9940001/diff/30003/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

http://codereview.chromium.org/9940001/diff/30003/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:196:
SetCertTrust(certificates[0].get(), net::SERVER_CERT, trustBits);
On 2012/05/16 03:57:23, Ryan Sleevi wrote:
> (Preexisting) bug?
> 
> What happens if |certificates.empty()| ?
> 
> All the code above handles it (eg: the for loop does nothing), but this could
be
> bad.

Done.

[Ryan Sleevi, 2012-05-16 22:52:13]

one nit, but changes lgtm

http://codereview.chromium.org/9940001/diff/23006/chrome/third_party/mozilla_...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

http://codereview.chromium.org/9940001/diff/23006/chrome/third_party/mozilla_...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1055: if
(cert->nickname && all_flags & CERTDB_USER)
nit: because of operator precedence, I find it helpful to wrap these in parens
(and on 1057)

&& (all_flags & CERTDB_USER)

(all_flags & CERTDB_VALID_CA) ||

[wtc, 2012-05-16 23:37:12]

Patch set 6 LGTM.

High-level comments:

1. I think we should have separate explicit distrust flags
for SSL, email, and object signing.

2. The GetCertType() function should also inspect a
certificate's contents to determine if the certificate is
a server certificate.  Testing the CERTDB_TERMINAL_RECORD
bit in trust->sslFlags is not enough.

http://codereview.chromium.org/9940001/diff/23006/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/9940001/diff/23006/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:932:
net::CertDatabase::TrustBits trust = web_trust ?

Please find out if web_trust should apply to a server cert.

http://codereview.chromium.org/9940001/diff/23006/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/certificate_manager2_browsertest.js
(right):

http://codereview.chromium.org/9940001/diff/23006/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/certificate_manager2_browsertest.js:21: // Mac
and Windows go to native certificate manager.

Add something like "Certificate manager isn't implemented
if OpenSSL is used".

http://codereview.chromium.org/9940001/diff/23006/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/9940001/diff/23006/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:4431: ['use_nss==0', {

This is the "else" of the 'use_nss==1' test on line 4421
above, right?

http://codereview.chromium.org/9940001/diff/23006/chrome/common/net/x509_cert...
File chrome/common/net/x509_certificate_model_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/23006/chrome/common/net/x509_cert...
chrome/common/net/x509_certificate_model_unittest.cc:35: // this directly.

Should this comment be updated?

http://codereview.chromium.org/9940001/diff/23006/chrome/common/net/x509_cert...
chrome/common/net/x509_certificate_model_unittest.cc:68:
net::CertDatabase::EXPLICIT_DISTRUST));

Does it matter whether we pass UNTRUSTED or EXPLICIT_DISTRUST
to this SetCertTrust call?

I suspect you had to change this to EXPLICIT_DISTRUST
because the GetCertType() is determining server certs
incorrectly.

http://codereview.chromium.org/9940001/diff/23006/chrome/third_party/mozilla_...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

http://codereview.chromium.org/9940001/diff/23006/chrome/third_party/mozilla_...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1059: if
(trust.sslFlags & CERTDB_TERMINAL_RECORD)

This test should be supplemented by another cert that
inspect |cert| to determine if it is a server cert.
Off the top of my head, I don't know if we have such an
NSS function.  Perhaps CERT_CheckCertUsage andCERT_CheckKeyUsage and
CERT_KeyUsageAndTypeForCertUsage?

(We do have such a function for CA cert: CERT_IsCACert.)

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database.h#ne...
net/base/cert_database.h:84: // of whether it would otherwise inherit trust from
the issuer chain.

The difference between UNTRUSTED and EXPLICIT_DISTRUST
should be clarified.

UNTRUSTED probably should be renamed to something like
TRUST_NORMAL or TRUST_DEFAULT.

We probably should replace EXPLICIT_DISTRUST with three
separate flags: DISTRUSTED_SSL, DISTRUSTED_EMAIL, and
DISTRUSTED_OBJ_SIGN.

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database.h#ne...
net/base/cert_database.h:161: // not given any trust.

The new trust_bits parameter should be documented.

We should reconsider the prototype of this function.  If we
import the server cert with explicit trust or distrust,
then it doesn't make sense to import its intermediate CA
certs.  Intermediate CA certs are only useful when the
server cert doesn't have a "terminal" trust record.

Does Chromium ever call this function with intermediate
CA certs in |certificates|?  If not, then I suggest we
change the prototype to take a single X509Certificate
instead.

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database_nss....
net/base/cert_database_nss.cc:239: if ((trust.sslFlags & (CERTDB_TRUSTED |
CERTDB_TERMINAL_RECORD)) ==

I think we should also check trust.emailFlags and
trust.objectSigningFlags.

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database_nss....
net/base/cert_database_nss.cc:242: else if ((trust.sslFlags &
CERTDB_TERMINAL_RECORD) ||

Remove "else".

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database_nss....
net/base/cert_database_nss.cc:242: else if ((trust.sslFlags &
CERTDB_TERMINAL_RECORD) ||

Remove "else".

We also need to check that CERTDB_TRUSTED is not set.

It is better to perform the tests like this:

  TrustBits r = UNTRUSTED;
  if (trust.sslFlags & CERTDB_TERMINAL_RECORD) {
    if (trust.sslFlags & CERTDB_TRUSTED)
      r |= TRUSTED_SSL;
    else
      r |= EXPLICIT_DISTRUST;  // or DISTRUSTED_SSL
  }
  // repeat for trust.emailFlags and trust.objectSigningFlags
  return r;

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database_nss_...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:282: EXPECT_EQ(unsigned(CERTDB_VALID_CA |
CERTDB_TRUSTED_CA |

Is the 'unsigned' cast necessary?  Just curious.

http://codereview.chromium.org/9940001/diff/23006/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

http://codereview.chromium.org/9940001/diff/23006/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:236:
CERTDB_TRUSTED_CLIENT_CA;

Nit: add curly braces around the body of this if statement because it spans two
lines.

http://codereview.chromium.org/9940001/diff/23006/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:246:
trust.objectSigningFlags |= CERTDB_TERMINAL_RECORD;

It seems that we should not change trust.emailFlags and trust.objectSigningFlags
for (SSL) server certificates.

[mattm, 2012-05-18 03:40:54]

+gspencer for question re: chrome/browser/chromeos/cros/onc_network_parser.cc

http://codereview.chromium.org/9940001/diff/23006/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/9940001/diff/23006/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:932:
net::CertDatabase::TrustBits trust = web_trust ?
On 2012/05/16 23:37:12, wtc wrote:
> 
> Please find out if web_trust should apply to a server cert.

I pinged gspencer to verify but haven't received a reply yet.

http://codereview.chromium.org/9940001/diff/23006/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/certificate_manager2_browsertest.js
(right):

http://codereview.chromium.org/9940001/diff/23006/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/certificate_manager2_browsertest.js:21: // Mac
and Windows go to native certificate manager.
On 2012/05/16 23:37:12, wtc wrote:
> 
> Add something like "Certificate manager isn't implemented
> if OpenSSL is used".

Done.

http://codereview.chromium.org/9940001/diff/23006/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/9940001/diff/23006/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:4431: ['use_nss==0', {
On 2012/05/16 23:37:12, wtc wrote:
> 
> This is the "else" of the 'use_nss==1' test on line 4421
> above, right?

Oh, yeah.  Done.

http://codereview.chromium.org/9940001/diff/23006/chrome/common/net/x509_cert...
File chrome/common/net/x509_certificate_model_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/23006/chrome/common/net/x509_cert...
chrome/common/net/x509_certificate_model_unittest.cc:35: // this directly.
On 2012/05/16 23:37:12, wtc wrote:
> 
> Should this comment be updated?

Oops, updated this test too.

http://codereview.chromium.org/9940001/diff/23006/chrome/common/net/x509_cert...
chrome/common/net/x509_certificate_model_unittest.cc:68:
net::CertDatabase::EXPLICIT_DISTRUST));
On 2012/05/16 23:37:12, wtc wrote:
> 
> Does it matter whether we pass UNTRUSTED or EXPLICIT_DISTRUST
> to this SetCertTrust call?
> 
> I suspect you had to change this to EXPLICIT_DISTRUST
> because the GetCertType() is determining server certs
> incorrectly.

Yeah, the first test after the #else is already testing the UNTRUSTED case.

http://codereview.chromium.org/9940001/diff/23006/chrome/third_party/mozilla_...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

http://codereview.chromium.org/9940001/diff/23006/chrome/third_party/mozilla_...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1055: if
(cert->nickname && all_flags & CERTDB_USER)
On 2012/05/16 22:52:13, Ryan Sleevi wrote:
> nit: because of operator precedence, I find it helpful to wrap these in parens
> (and on 1057)
> 
> && (all_flags & CERTDB_USER)
> 
> (all_flags & CERTDB_VALID_CA) || 

Done.

http://codereview.chromium.org/9940001/diff/23006/chrome/third_party/mozilla_...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1059: if
(trust.sslFlags & CERTDB_TERMINAL_RECORD)
On 2012/05/16 23:37:12, wtc wrote:
> 
> This test should be supplemented by another cert that
> inspect |cert| to determine if it is a server cert.
> Off the top of my head, I don't know if we have such an
> NSS function.  Perhaps CERT_CheckCertUsage andCERT_CheckKeyUsage and
> CERT_KeyUsageAndTypeForCertUsage?
> 
> (We do have such a function for CA cert: CERT_IsCACert.)

Filed http://crbug.com/128633.

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database.h#ne...
net/base/cert_database.h:84: // of whether it would otherwise inherit trust from
the issuer chain.
On 2012/05/16 23:37:12, wtc wrote:
> 
> The difference between UNTRUSTED and EXPLICIT_DISTRUST
> should be clarified.
> 
> UNTRUSTED probably should be renamed to something like
> TRUST_NORMAL or TRUST_DEFAULT.
> 
> We probably should replace EXPLICIT_DISTRUST with three
> separate flags: DISTRUSTED_SSL, DISTRUSTED_EMAIL, and
> DISTRUSTED_OBJ_SIGN.

Done.

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database.h#ne...
net/base/cert_database.h:161: // not given any trust.
On 2012/05/16 23:37:12, wtc wrote:
> 
> The new trust_bits parameter should be documented.
> 
> We should reconsider the prototype of this function.  If we
> import the server cert with explicit trust or distrust,
> then it doesn't make sense to import its intermediate CA
> certs.  Intermediate CA certs are only useful when the
> server cert doesn't have a "terminal" trust record.
> 
> Does Chromium ever call this function with intermediate
> CA certs in |certificates|?  If not, then I suggest we
> change the prototype to take a single X509Certificate
> instead.

When using the "import" option in the server tab of certificate manager, the
user could select a file with multiple certs in it.  But, we doesn't do anything
to ensure the server cert must be first in the list as ImportServerCert
requires, so I'm not too sure if this counts as actually intending to call it
with multiple certs or if I just used CreateCertificateListFromBytes since it's
easy to use with FORMAT_AUTO.

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database_nss....
net/base/cert_database_nss.cc:242: else if ((trust.sslFlags &
CERTDB_TERMINAL_RECORD) ||
On 2012/05/16 23:37:12, wtc wrote:
> 
> Remove "else".
> 
> We also need to check that CERTDB_TRUSTED is not set.
> 
> It is better to perform the tests like this:
> 
>   TrustBits r = UNTRUSTED;
>   if (trust.sslFlags & CERTDB_TERMINAL_RECORD) {
>     if (trust.sslFlags & CERTDB_TRUSTED)
>       r |= TRUSTED_SSL;
>     else
>       r |= EXPLICIT_DISTRUST;  // or DISTRUSTED_SSL
>   }
>   // repeat for trust.emailFlags and trust.objectSigningFlags
>   return r;

Done.

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database_nss_...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/23006/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:282: EXPECT_EQ(unsigned(CERTDB_VALID_CA |
CERTDB_TRUSTED_CA |
On 2012/05/16 23:37:12, wtc wrote:
> 
> Is the 'unsigned' cast necessary?  Just curious.

Yeah, the NSS defines are signed, but the trust struct uses unsigned, which
causes a compiler warning.

http://codereview.chromium.org/9940001/diff/23006/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

http://codereview.chromium.org/9940001/diff/23006/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:246:
trust.objectSigningFlags |= CERTDB_TERMINAL_RECORD;
On 2012/05/16 23:37:12, wtc wrote:
> 
> It seems that we should not change trust.emailFlags and
trust.objectSigningFlags
> for (SSL) server certificates.

Done.

[Greg Spencer (Chromium), 2012-05-18 04:44:35]

On 2012/05/18 03:40:54, mattm wrote:
> On 2012/05/16 23:37:12, wtc wrote:
> > Please find out if web_trust should apply to a server cert.
> I pinged gspencer to verify but haven't received a reply yet.

Sorry, I did see your earlier message, but forgot to respond.

I don't know the answer to this, sorry.  I would suspect that it should not (it
seems to me that it would make sense that only a CA would have web trust) but I
don't know for sure.  Perhaps you can look at the allowed trust bits in NSS to
find out what they use?

-Greg.

[mattm, 2012-05-21 20:48:04]

On 2012/05/18 04:44:35, Greg Spencer (Chromium) wrote:
> On 2012/05/18 03:40:54, mattm wrote:
> > On 2012/05/16 23:37:12, wtc wrote:
> > > Please find out if web_trust should apply to a server cert.
> > I pinged gspencer to verify but haven't received a reply yet.
> 
> Sorry, I did see your earlier message, but forgot to respond.
> 
> I don't know the answer to this, sorry.  I would suspect that it should not
(it
> seems to me that it would make sense that only a CA would have web trust) but
I
> don't know for sure.  Perhaps you can look at the allowed trust bits in NSS to
> find out what they use?
> 
> -Greg.

I found the ONC spec linked from:
http://www.chromium.org/chromium-os/chromiumos-design-docs/open-network-confi...

Copy-paste isn't working from that doc, but from page 11, it looks like "Web"
trust should apply to both Authority and Server certificate types.

[wtc, 2012-05-22 00:28:38]

Patch set 7 LGTM.

Most of the changes I suggest below are comment changes,
but there is one important issue: I believe that to explicitly
trust a CA certificate, especially an intermediate CA
certificate, we also need to set the CERTDB_TERMINAL_RECORD
bit.  This requires changing two places: where we set the
bit, and where we test the bit.

http://codereview.chromium.org/9940001/diff/27005/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/9940001/diff/27005/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:831: if (trust_type == "Web")
{

It may be a good idea to copy what "Web" trust means
from the PDF file you cited: "Web" implies that the
certificate is to be trusted for HTTPS SSL identification.

http://codereview.chromium.org/9940001/diff/27005/chrome/common/net/x509_cert...
File chrome/common/net/x509_certificate_model_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/27005/chrome/common/net/x509_cert...
chrome/common/net/x509_certificate_model_unittest.cc:50: // Test GetCertType
with server certs and default trust.  Currently this

In these comments, "GetCertType" is a little confusing because
the test calls x509_certificate_model::GetType.  It may be
clearer to fully qualify GetCertType in the comments.

http://codereview.chromium.org/9940001/diff/27005/chrome/third_party/mozilla_...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

http://codereview.chromium.org/9940001/diff/27005/chrome/third_party/mozilla_...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1059: if
(trust.sslFlags & CERTDB_TERMINAL_RECORD)

Please add
  // TODO(mattm): http://crbug.com/128633.
Thanks.

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database.h#ne...
net/base/cert_database.h:84: // use, regardless of whether it would otherwise
inherit trust from the issuer

Nit: use => usage

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database.h#ne...
net/base/cert_database.h:90: TRUST_DEFAULT         =      0,

Please document TRUST_DEFAULT.  You can copy from line 168.

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database_nss....
net/base/cert_database_nss.cc:222: TrustBits r = TRUST_DEFAULT;

Nit: use |trust_bits| instead of |r|?

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database_nss....
net/base/cert_database_nss.cc:226: r |= TRUSTED_SSL;

This isn't quite right for an intermediate CA certificate.
I believe that an intermediate CA certificate still needs
the CERTDB_TERMINAL_RECORD bit set to get TRUSTED_SSL.
So I think for an intermediate CA certificate, we should say:

  TrustBits r = TRUST_DEFAULT;
  if (trust.sslFlags & CERTDB_TERMINAL_RECORD) {
    if (trust.sslFlags & kTrustedCA)
      r |= TRUSTED_SSL;
    else
      r |= DISTRUSTED_SSL;
  }

NOTE: the above code may not be correct for a root CA
certificate, which may not need the CERTDB_TERMINAL_RECORD
bit to be set.  So the code for a root certificate might be
simply:

    if (trust.sslFlags & kTrustedCA)
      r |= TRUSTED_SSL;
    else
      r |= DISTRUSTED_SSL;

You can test cert->os_cert_handle()->isRoot to determine
if |cert| is a root CA or intermediate CA certificate.
But I am not sure about this.  So I suggest you use the
code that tests CERTDB_TERMINAL_RECORD for now.

http://codereview.chromium.org/9940001/diff/27005/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

http://codereview.chromium.org/9940001/diff/27005/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:216: // DCHECK
that caller isn't setting trust and distrust for the same use.
Perhaps the function should return false if the caller sets
trust and distrust for the same use?

Since |ssl_flags| looks similar to the |sslFlags| member of
CERTCertTrust, I suggest that we name these variables
ssl_bits or ssl_trust_bits or even kSSLTrustBits. etc.

http://codereview.chromium.org/9940001/diff/27005/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:235:
trust.sslFlags |= CERTDB_TRUSTED_CA | CERTDB_TRUSTED_CLIENT_CA;

The CERTDB_TERMINAL_RECORD bit also needs to be set for explicit
trust.

[mattm, 2012-05-26 03:41:35]

Updated (patch set 9).  Please see especially the changes in
cert_database_nss_unittest.cc and nsNSSCertificateDB.cpp.

http://codereview.chromium.org/9940001/diff/27005/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/9940001/diff/27005/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:831: if (trust_type == "Web")
{
On 2012/05/22 00:28:39, wtc wrote:
> 
> It may be a good idea to copy what "Web" trust means
> from the PDF file you cited: "Web" implies that the
> certificate is to be trusted for HTTPS SSL identification.

Done.

http://codereview.chromium.org/9940001/diff/27005/chrome/common/net/x509_cert...
File chrome/common/net/x509_certificate_model_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/27005/chrome/common/net/x509_cert...
chrome/common/net/x509_certificate_model_unittest.cc:50: // Test GetCertType
with server certs and default trust.  Currently this
On 2012/05/22 00:28:39, wtc wrote:
> 
> In these comments, "GetCertType" is a little confusing because
> the test calls x509_certificate_model::GetType.  It may be
> clearer to fully qualify GetCertType in the comments.

Done.

http://codereview.chromium.org/9940001/diff/27005/chrome/third_party/mozilla_...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

http://codereview.chromium.org/9940001/diff/27005/chrome/third_party/mozilla_...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1059: if
(trust.sslFlags & CERTDB_TERMINAL_RECORD)
On 2012/05/22 00:28:39, wtc wrote:
> 
> Please add
>   // TODO(mattm): http://crbug.com/128633.
> Thanks.

Done.

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database.h#ne...
net/base/cert_database.h:84: // use, regardless of whether it would otherwise
inherit trust from the issuer
On 2012/05/22 00:28:39, wtc wrote:
> 
> Nit: use => usage

Done.

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database.h#ne...
net/base/cert_database.h:90: TRUST_DEFAULT         =      0,
On 2012/05/22 00:28:39, wtc wrote:
> 
> Please document TRUST_DEFAULT.  You can copy from line 168.

Done.

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/9940001/diff/27005/net/base/cert_database_nss....
net/base/cert_database_nss.cc:222: TrustBits r = TRUST_DEFAULT;
On 2012/05/22 00:28:39, wtc wrote:
> 
> Nit: use |trust_bits| instead of |r|?

Done.

http://codereview.chromium.org/9940001/diff/27005/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

http://codereview.chromium.org/9940001/diff/27005/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:216: // DCHECK
that caller isn't setting trust and distrust for the same use.
On 2012/05/22 00:28:39, wtc wrote:
> Perhaps the function should return false if the caller sets
> trust and distrust for the same use?

done.

> Since |ssl_flags| looks similar to the |sslFlags| member of
> CERTCertTrust, I suggest that we name these variables
> ssl_bits or ssl_trust_bits or even kSSLTrustBits. etc.

done.

http://codereview.chromium.org/9940001/diff/27005/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:235:
trust.sslFlags |= CERTDB_TRUSTED_CA | CERTDB_TRUSTED_CLIENT_CA;
On 2012/05/22 00:28:39, wtc wrote:
> 
> The CERTDB_TERMINAL_RECORD bit also needs to be set for explicit
> trust.

I added some tests and it does not seem to be necessary.  I did notice a
different problem though,, where distrusting an intermediate cert would not have
any effect if the root cert was trusted.  Seems to be because we were using
CERTDB_VALID_CA|CERTDB_TERMINAL_RECORD.  I changed it to use just
CERTDB_TERMINAL_RECORD in that case, which appears to work.

[wtc, 2012-05-30 00:19:24]

Patch set 10 LGTM.

It may make sense to check this in after minimal cleanup,
which we track down the hard issues.  This is because it is
hard for me to review a big CL repeatedly.

http://codereview.chromium.org/9940001/diff/27005/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

http://codereview.chromium.org/9940001/diff/27005/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:235:
trust.sslFlags |= CERTDB_TRUSTED_CA | CERTDB_TRUSTED_CLIENT_CA;

On 2012/05/26 03:41:35, mattm wrote:
>
> I added some tests and it does not seem to be necessary.  I did notice a
> different problem though,, where distrusting an intermediate cert would not
have
> any effect if the root cert was trusted.  Seems to be because we were using
> CERTDB_VALID_CA|CERTDB_TERMINAL_RECORD.  I changed it to use just
> CERTDB_TERMINAL_RECORD in that case, which appears to work.

We should get to the bottom of this.  I suspect that
the new problem you noticed is a NSS libpkix bug.  We
can verify this by editing net/base/cert_verify_proc_nss.cc
and changing CertVerifyProcNSS::VerifyInternal to call
CERT_VerifyCert instead of PKIXVerifyCert.  Please see
if your tests report different reports.

If you are not sure how to make this change, please ask me.

I will also start an email thread with the NSS developer
Bob Relyea about the correct NSS trust flags to use.

http://codereview.chromium.org/9940001/diff/45002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/9940001/diff/45002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:832: // "Web" implies that
the certificate is to be trusted for HTTPS SSL

Nit: remove "HTTPS"?  Seems redundant with "SSL".

http://codereview.chromium.org/9940001/diff/45002/net/base/cert_database_nss_...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/45002/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:122: // can break following tests.

We should track down exactly which NSS change caused this.

Between NSS 3.12.9 and NSS 3.13.4, there are the following
NSS releases:

NSS_3_12_9_1_RTM
NSS_3_12_10_RTM
NSS_3_12_11_RTM
NSS_3_13_RTM
NSS_3_13_1_RTM
NSS_3_13_2_RTM
NSS_3_13_3_RTM

Could you narrow down the version range in which this
behavior changed?  If you don't have time, I can do this
bisection.

http://codereview.chromium.org/9940001/diff/45002/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:692: EXPECT_EQ(0L,
certs[0]->os_cert_handle()->trust->objectSigningFlags);

sslFlags, emailFlags, and objectSigningFlags are of the type
unsigned int.  So it seems that the 0 constants should have
the suffix U, if they need a suffix.

http://codereview.chromium.org/9940001/diff/45002/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:940: NULL, &verify_result2);

Nit: indentation of the arguments is wrong.  The other unit
tests have the same problem.

http://codereview.chromium.org/9940001/diff/45002/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

http://codereview.chromium.org/9940001/diff/45002/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:227:
NOTREACHED() << trustBits;

Nit: since trustBits is already logged in the error message,
we don't need to log it again in NOTREACHED().

[wtc, 2012-05-30 00:20:26]

On 2012/05/30 00:19:24, wtc wrote:
> 
> It may make sense to check this in after minimal cleanup,
> which we track down the hard issues.

Sorry about the typo.  This should read:
  It may make sense to check this in after minimal cleanup
  while we track down the hard issues.

[wtc, 2012-05-30 00:39:53]

http://codereview.chromium.org/9940001/diff/45002/net/base/cert_database_nss_...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/45002/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:122: // can break following tests.

I happened to see this CERT_ChangeCertTrust call in Mozilla's
nsNSSCertificateDB::DeleteCertificate:
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSC...

Could it be related?

[mattm, 2012-05-30 22:40:58]

http://codereview.chromium.org/9940001/diff/27005/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

http://codereview.chromium.org/9940001/diff/27005/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:235:
trust.sslFlags |= CERTDB_TRUSTED_CA | CERTDB_TRUSTED_CLIENT_CA;
On 2012/05/30 00:19:24, wtc wrote:
> 
> On 2012/05/26 03:41:35, mattm wrote:
> >
> > I added some tests and it does not seem to be necessary.  I did notice a
> > different problem though,, where distrusting an intermediate cert would not
> have
> > any effect if the root cert was trusted.  Seems to be because we were using
> > CERTDB_VALID_CA|CERTDB_TERMINAL_RECORD.  I changed it to use just
> > CERTDB_TERMINAL_RECORD in that case, which appears to work.
> 
> We should get to the bottom of this.  I suspect that
> the new problem you noticed is a NSS libpkix bug.  We
> can verify this by editing net/base/cert_verify_proc_nss.cc
> and changing CertVerifyProcNSS::VerifyInternal to call
> CERT_VerifyCert instead of PKIXVerifyCert.  Please see
> if your tests report different reports.
> 
> If you are not sure how to make this change, please ask me.
> 
> I will also start an email thread with the NSS developer
> Bob Relyea about the correct NSS trust flags to use.

Just tested this.

With PKIXVerifyCert:

CERTDB_VALID_CA|CERTDB_TERMINAL_RECORD verifies successfully
CERTDB_TERMINAL_RECORD gives SEC_ERROR_UNTRUSTED_CERT

With CERT_VerifyCert:

CERTDB_VALID_CA|CERTDB_TERMINAL_RECORD gives SEC_ERROR_UNTRUSTED_ISSUER
CERTDB_TERMINAL_RECORD gives SEC_ERROR_UNTRUSTED_ISSUER

http://codereview.chromium.org/9940001/diff/45002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/9940001/diff/45002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:832: // "Web" implies that
the certificate is to be trusted for HTTPS SSL
On 2012/05/30 00:19:24, wtc wrote:
> 
> Nit: remove "HTTPS"?  Seems redundant with "SSL".

Done.

http://codereview.chromium.org/9940001/diff/45002/net/base/cert_database_nss_...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/9940001/diff/45002/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:122: // can break following tests.
On 2012/05/30 00:19:24, wtc wrote:
> 
> We should track down exactly which NSS change caused this.
> 
> Between NSS 3.12.9 and NSS 3.13.4, there are the following
> NSS releases:
> 
> NSS_3_12_9_1_RTM
> NSS_3_12_10_RTM
> NSS_3_12_11_RTM
> NSS_3_13_RTM
> NSS_3_13_1_RTM
> NSS_3_13_2_RTM
> NSS_3_13_3_RTM
> 
> Could you narrow down the version range in which this
> behavior changed?  If you don't have time, I can do this
> bisection.

Okay, after further testing I had narrowed it down but began to wonder if it was
due to the parts of tests being disabled on older versions of NSS.  Indeed, it
just turns out that with the tests being fully run that particular state didn't
occur.  If I made the tests follow the same path in newer NSS, or wrote a test
specifically to check for this, it does happen in all NSS versions I tested.

I'll update the comment and check in as is, we can decide separately if we want
to handle deletion differently.

http://codereview.chromium.org/9940001/diff/45002/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:692: EXPECT_EQ(0L,
certs[0]->os_cert_handle()->trust->objectSigningFlags);
On 2012/05/30 00:19:24, wtc wrote:
> 
> sslFlags, emailFlags, and objectSigningFlags are of the type
> unsigned int.  So it seems that the 0 constants should have
> the suffix U, if they need a suffix.

oops, fixed.

http://codereview.chromium.org/9940001/diff/45002/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:940: NULL, &verify_result2);
On 2012/05/30 00:19:24, wtc wrote:
> 
> Nit: indentation of the arguments is wrong.  The other unit
> tests have the same problem.

Done.

http://codereview.chromium.org/9940001/diff/45002/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

http://codereview.chromium.org/9940001/diff/45002/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:227:
NOTREACHED() << trustBits;
On 2012/05/30 00:19:24, wtc wrote:
> 
> Nit: since trustBits is already logged in the error message,
> we don't need to log it again in NOTREACHED().

Done.

[commit-bot: I haz the power, 2012-05-30 22:41:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/9940001/52003

[commit-bot: I haz the power, 2012-05-30 22:42:01]

Presubmit check for 9940001-52003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui/options2
    chrome/browser/chromeos/cros
    chrome/browser/resources/options2

Presubmit checks took 6.1s to calculate.

[mattm, 2012-05-30 22:48:13]

+stevenjb for chrome/browser/chromeos/cros
+jhawkins for */options2/

[James Hawkins, 2012-05-30 22:53:20]

LGTM

[stevenjb, 2012-05-30 23:02:27]

OWNER lgtm for chromeos/cros, but would like a lgtm from gspencer@ or mnissler@
also.

[Greg Spencer (Chromium), 2012-05-30 23:18:24]

Reviewed onc_network_parser*.

http://codereview.chromium.org/9940001/diff/52003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser_unittest.cc (left):

http://codereview.chromium.org/9940001/diff/52003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser_unittest.cc:36: #include
"net/third_party/mozilla_security_manager/nsNSSCertTrust.h"
Why are this include and namespace alias required?  They don't appear to be used
in this file.

[mattm, 2012-05-30 23:21:50]

http://codereview.chromium.org/9940001/diff/52003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser_unittest.cc (left):

http://codereview.chromium.org/9940001/diff/52003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser_unittest.cc:36: #include
"net/third_party/mozilla_security_manager/nsNSSCertTrust.h"
On 2012/05/30 23:18:29, Greg Spencer (Chromium) wrote:
> Why are this include and namespace alias required?  They don't appear to be
used
> in this file.

They should have been removed in https://chromiumcodereview.appspot.com/9791050,
but I missed them then.

[Greg Spencer (Chromium), 2012-05-30 23:24:54]

LGTM for onc_network_parser*

http://codereview.chromium.org/9940001/diff/52003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser_unittest.cc (left):

http://codereview.chromium.org/9940001/diff/52003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser_unittest.cc:36: #include
"net/third_party/mozilla_security_manager/nsNSSCertTrust.h"
On 2012/05/30 23:21:50, mattm wrote:
> They should have been removed in ...,
> but I missed them then.

Sorry, I misread the diff as inserting them, rather than removing. Duh.

[commit-bot: I haz the power, 2012-05-30 23:28:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/9940001/52003

[commit-bot: I haz the power, 2012-05-30 23:48:58]

Try job failure for 9940001-52003 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-30 23:54:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/9940001/52003

[commit-bot: I haz the power, 2012-05-31 02:29:43]

Change committed as 139719