net: enable CRL sets behind a command line flag.

net: enable CRL sets behind a command line flag.

This change introduces a command line flag for enabling CRL sets while the
serving side is still in development.

It contains code for NSS (revocation checking will proceed as normal on other
platforms).

BUG=none
TEST=none


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

[wtc, 2011-10-21 23:17:31]

Review comments on Patch Set 2:

I don't know why we don't need to update the VerifyInternal
function in x509_certificate_xxx.cc for the non-NSS
implementations to add the new |crl_set| parameter.

I have some suggested changes below.

http://codereview.chromium.org/8342054/diff/2001/chrome/browser/net/crl_set_f...
File chrome/browser/net/crl_set_fetcher.cc (right):

http://codereview.chromium.org/8342054/diff/2001/chrome/browser/net/crl_set_f...
chrome/browser/net/crl_set_fetcher.cc:125: static const uint8
kPublicKeySHA256[32] = {

Nit: document whose public key this is.

http://codereview.chromium.org/8342054/diff/2001/chrome/browser/net/crl_set_f...
chrome/browser/net/crl_set_fetcher.cc:136:
component.pk_hash.assign(&kPublicKeySHA256[0],

Nit: you can just say kPublicKeySHA256,

http://codereview.chromium.org/8342054/diff/2001/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/8342054/diff/2001/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:431: // Enables establishing revocation
information by downloading a set of CRLs

Nit: add "certificate" before "revocation".

"performing on-line checks" is not accurate because a properly
implemented crypto library caches the CRLs persistently.  (NSS
doesn't do that yet.)

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_status_flags.h
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_status_flags.h...
net/base/cert_status_flags.h:40: static const CertStatus
CERT_STATUS_USED_CRL_SET               = 1 << 19;

Do we really need this bit of info?  Is CERT_STATUS_REV_CHECKING_ENABLED
not enough?

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_verifier.cc
File net/base/cert_verifier.cc (right):

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:177: crl_set_.get());

Nit: omit .get().

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_verifier.h
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_verifier.h#new...
net/base/cert_verifier.h:106: CRLSet* crl_set,

Please document the new |crl_set| parameter.  You can just copy
the comment from x509_certificate.h.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:330: CRLSet* crl_set) const;

List |crl_set| (an input) before |verify_result| (the output).

Make the same change to VerifyInternal below.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:255: CERTCertificate* last = NULL;

Nit: |prev| would be better because |last| may also mean the
very last item.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:257: i != certs.end(); i++) {

Nit: ++i

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:278: serial_number = serial_number.substr(1,
serial_number.size() - 1);

I'm sorry that I'm bringing this up again.

We should not remove the leading zeros from certificate serial
numbers, unless it is advantageous.  The reason is that it
may be misinterpreted as us not knowing how ASN.1 INTEGERs
are encoded.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:285: case CRLSet::UNKNOWN:

IMPORTANT: are you sure we should ignore CRLSet::UNKNOWN?
I think we should return kCRLSetError in that case.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:289: NOTREACHED();

BUG (corner case): We should return kCRLSetError in this case.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:791: status = SECSuccess;

This assignment is not necessary.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:810: verify_result->cert_status |=
CERT_STATUS_REVOKED;

BUG: we need to set |status| to SECFailure and set the NSS
error code here:
      if (crl_set_result == kCRLSetRevoked) {
        PORT_SetError(SEC_ERROR_REVOKED_CERTIFICATE);
        status = SECFailure;
      }
and then rely on the error code path on lines 818-834.

Alternatively, we can return right here:
      if (crl_set_result == kCRLSetRevoked) {
        LOG(ERROR) << ...;  // Not sure if this is necessary
        verify_result->cert_status |= CERT_STATUS_REVOKED;
        return ERR_CERT_REVOKED;
      }

http://codereview.chromium.org/8342054/diff/2001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8342054/diff/2001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1667: ssl_config_.crl_set.get(),

Nit: omit .get().

http://codereview.chromium.org/8342054/diff/2001/net/socket/ssl_host_info.h
File net/socket/ssl_host_info.h (right):

http://codereview.chromium.org/8342054/diff/2001/net/socket/ssl_host_info.h#n...
net/socket/ssl_host_info.h:18: #include "net/base/crl_set.h"

Does a forward declaration of CRLSet work?

[agl, 2011-10-24 20:44:27]

http://codereview.chromium.org/8342054/diff/2001/chrome/browser/net/crl_set_f...
File chrome/browser/net/crl_set_fetcher.cc (right):

http://codereview.chromium.org/8342054/diff/2001/chrome/browser/net/crl_set_f...
chrome/browser/net/crl_set_fetcher.cc:125: static const uint8
kPublicKeySHA256[32] = {
On 2011/10/21 23:17:31, wtc wrote:
> 
> Nit: document whose public key this is.

Done.

http://codereview.chromium.org/8342054/diff/2001/chrome/browser/net/crl_set_f...
chrome/browser/net/crl_set_fetcher.cc:136:
component.pk_hash.assign(&kPublicKeySHA256[0],
On 2011/10/21 23:17:31, wtc wrote:
> 
> Nit: you can just say kPublicKeySHA256,

Done.

http://codereview.chromium.org/8342054/diff/2001/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/8342054/diff/2001/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:431: // Enables establishing revocation
information by downloading a set of CRLs
On 2011/10/21 23:17:31, wtc wrote:
> Nit: add "certificate" before "revocation".

Done.

> "performing on-line checks" is not accurate because a properly
> implemented crypto library caches the CRLs persistently.  (NSS
> doesn't do that yet.)

In the Windows implementation (which I've yet to write), we will be using cached
revocation information, just not on-line checks. So I believe that this comment
is accurate.

(Based on discussions with cemp, I believe that this is possible with
CryptoAPI.)

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_status_flags.h
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_status_flags.h...
net/base/cert_status_flags.h:40: static const CertStatus
CERT_STATUS_USED_CRL_SET               = 1 << 19;
On 2011/10/21 23:17:31, wtc wrote:
> Do we really need this bit of info?  Is CERT_STATUS_REV_CHECKING_ENABLED
> not enough?

I initially thought that it might be useful, but I've removed it because space
in this word isn't free.

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_verifier.cc
File net/base/cert_verifier.cc (right):

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:177: crl_set_.get());
On 2011/10/21 23:17:31, wtc wrote:
> 
> Nit: omit .get().

Done.

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_verifier.h
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/8342054/diff/2001/net/base/cert_verifier.h#new...
net/base/cert_verifier.h:106: CRLSet* crl_set,
On 2011/10/21 23:17:31, wtc wrote:
> 
> Please document the new |crl_set| parameter.  You can just copy
> the comment from x509_certificate.h.

Done.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:330: CRLSet* crl_set) const;
On 2011/10/21 23:17:31, wtc wrote:
> 
> List |crl_set| (an input) before |verify_result| (the output).
> 
> Make the same change to VerifyInternal below.

Done.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:255: CERTCertificate* last = NULL;
On 2011/10/21 23:17:31, wtc wrote:
> 
> Nit: |prev| would be better because |last| may also mean the
> very last item.

Done.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:257: i != certs.end(); i++) {
On 2011/10/21 23:17:31, wtc wrote:
> 
> Nit: ++i

Done.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:278: serial_number = serial_number.substr(1,
serial_number.size() - 1);
On 2011/10/21 23:17:31, wtc wrote:
> We should not remove the leading zeros from certificate serial
> numbers, unless it is advantageous.  The reason is that it
> may be misinterpreted as us not knowing how ASN.1 INTEGERs
> are encoded.

This has been addressed in http://codereview.chromium.org/8381017/

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:285: case CRLSet::UNKNOWN:
On 2011/10/21 23:17:31, wtc wrote:
> IMPORTANT: are you sure we should ignore CRLSet::UNKNOWN?
> I think we should return kCRLSetError in that case.

Yes. The intended semantics are that, unless a serial number is listed, then
it's not revoked. (If we accept that soft-fail, on-line checks are worthless in
any reasonable scenario then there's no harm in that. I'd be happy to try and
convince you if that if you wish.)

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:289: NOTREACHED();
On 2011/10/21 23:17:31, wtc wrote:
> 
> BUG (corner case): We should return kCRLSetError in this case.

Done.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:791: status = SECSuccess;
On 2011/10/21 23:17:31, wtc wrote:
> 
> This assignment is not necessary.

Done.

http://codereview.chromium.org/8342054/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:810: verify_result->cert_status |=
CERT_STATUS_REVOKED;
On 2011/10/21 23:17:31, wtc wrote:
> BUG: we need to set |status| to SECFailure and set the NSS
> error code here:

Ah, yes. Thank you. Done.

http://codereview.chromium.org/8342054/diff/2001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8342054/diff/2001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1667: ssl_config_.crl_set.get(),
On 2011/10/21 23:17:31, wtc wrote:
> 
> Nit: omit .get().

Done.

http://codereview.chromium.org/8342054/diff/2001/net/socket/ssl_host_info.h
File net/socket/ssl_host_info.h (right):

http://codereview.chromium.org/8342054/diff/2001/net/socket/ssl_host_info.h#n...
net/socket/ssl_host_info.h:18: #include "net/base/crl_set.h"
On 2011/10/21 23:17:31, wtc wrote:
> 
> Does a forward declaration of CRLSet work?

Done.

[wtc, 2011-10-25 01:25:35]

Patch Set 3 LGTM.  Thanks.

Just one comment nit below.

http://codereview.chromium.org/8342054/diff/7001/net/base/cert_verifier.h
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/8342054/diff/7001/net/base/cert_verifier.h#new...
net/base/cert_verifier.h:92: // avoid revocation checks over the network.

Move this paragraph after the next paragraph because the
next paragraph is still describing |flags|.

[commit-bot: I haz the power, 2011-10-25 14:55:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/8342054/15001

[agl, 2011-10-25 14:55:57]

thanks

http://codereview.chromium.org/8342054/diff/7001/net/base/cert_verifier.h
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/8342054/diff/7001/net/base/cert_verifier.h#new...
net/base/cert_verifier.h:92: // avoid revocation checks over the network.
On 2011/10/25 01:25:35, wtc wrote:
> 
> Move this paragraph after the next paragraph because the
> next paragraph is still describing |flags|.

Done.

[commit-bot: I haz the power, 2011-10-25 16:12:48]

Change committed as 107131