Add X509Certificate::IsIssuedByEncoded()

Add X509Certificate::IsIssuedByEncoded()

This new method is used to ensure that a given client certificate
is issued by one of the CA names listed by the server, as they appear
in the SSL Handshake "Certificate Request" message.

The patch also adds two new X509CertificateTest unit tests,
moves existing hard-coded DN tables to net/base/test_certificate_data.h to
share them between multiple test sources, and adds a
few new DN tables too.

R=rsleevi@chromium.org,wtc@chromium.org,agl@chromium.org
BUG=134418
NOTRY=true

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

[digit1, 2012-12-13 18:55:59]

This patch corresponds to the change suggested by Ryan in the comments of
https://chromiumcodereview.appspot.com/11458012/

This is a first version, I could only compile it for Android, linux and
linux_redux (will use the try bots for win / mac / ios).

Note that this code make some rather strong assumptions because I didn't find
some platform-specific APIs to do otherwise:

- OpenSSL: The X509_NAME_cmp() implementation makes a
  rather "naive" comparison of the list of RDNs. I.e.
  it is order-dependent, so if the certificate issuer
  lists the RDNs in a different order than in the X.500
  name sent by the server, they won't match.

  (my understanding is that order should not matter for
  that kind of comparison).

- NSS: I couldn't find a way to to a non-order dependent
  comparison either. Actually, the only API I found is
  essentially performing a byte-wise comparison of the
  DER-encoded values. I'm really unsure this is sufficient.

- Win/Mac: The changes here are purely speculative.

So please let me know if there is a better way to implement all this (I'm pretty
sure there is).

Thanks

[Ryan Sleevi, 2012-12-13 19:49:05]

Thanks digit

This looks great, and is mostly ready to go. I mentioned the same sort of bug
repeatedly, which is that rather than checking intermediate[x]->subject, you
should be checking intermediate[x]->issuer - since you may be missing the final
issuer certificate.

For the shared code between (IOS, NSS), I'd suggest taking a vector of CERTName
& a vector of std::strings.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_ios.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_ios.cc:75: valid_issuers))
It's a shame to have to reparse the CERTName here (we have it in
cert_handle->issuer)

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_ios.cc:82: return true;
BUG: Rather then checking the subject, you should be checking issuer names here
as well.

It's legal for intermediate_ca_certs[.size() - 1].issuer == some_valid_issuer,
which would be missed by this algorithm.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_mac.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_mac.cc:368: valid_issuers))
STYLE: multi-line conditionals should have braces (also lines 373-375)

BUG: See IOS bug about subject vs ISSUER

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_nss.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_nss.cc:161: valid_issuers))
STYLE: See comments re braces (also line 166-168)

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_nss.cc:167: valid_issuers))
BUG: See comment in ios re subject vs issuer

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_openssl.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_openssl.cc:523: issuer = X509_get_subject_name(*it);
BUG: See previous comments re: subject vs issuer

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_openssl.cc:529: // and 'cert_names'.
Not sure this comment really makes sense. Is it a continuation of some comment?
which?

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_openssl.cc:537: break;
This breaks the inner loop but keeps spinning the outer loop.

Just return true here, and return false on line 542?

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_win.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_win.cc:129: const std::vector<std::string>&
issuer_names) {
STYLE: Indenting is wrong

bool IsCertNameBlobInIsuserList(
    CERT_NAME_BLOB* name_blob,
    const std::vector<std::string>& issuer_names) {
}

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_win.cc:490: valid_issuers))
STYLE: Indenting is messy here

if (IsCertNameBlobInIssuerList(&cert_handle->pCertInfo->Issuer,
                               valid_issuers)) {
  return true;
}

Same on 496 as well

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
File net/base/x509_util_nss.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
net/base/x509_util_nss.cc:541: if (SECITEM_ItemsAreEqual(&issuer, name))
use the CERTName comparison

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
File net/base/x509_util_nss.h (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
net/base/x509_util_nss.h:80: SECItem* name,
see comments in iOS about making this a CERTName and saving some name parsing

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
net/base/x509_util_nss.h:81: const std::vector<std::string>& valid_issuers);
Perhaps rather then passing a single SECItem for a name, along with a vector of
strings, you have it pass in a vector of CERTName (collected from the
CERTCertificate*s) and a vector of strings (collected from the SSL handshake)

[digit1, 2012-12-14 17:54:33]

By the way, it seems that the order of RDN elements in a DER-encoded DN is
strictly mandated by their encoding values. At least that's what is said in the
"Names" section of the x509 guide at
http://www.cs.auckland.ac.nz/~pgut001/pubs/x509guide.txt

This probably means I shouldn't worry about this at all. Let me know if this is
wrong.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_ios.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_ios.cc:75: valid_issuers))
On 2012/12/13 19:49:05, Ryan Sleevi wrote:
> It's a shame to have to reparse the CERTName here (we have it in
> cert_handle->issuer)

I know, but cert_handle->issuer is a CertPrincipal which, has you already
mentioned, should only be used for presentation. That's why I intentionally
avoided using it here.

I also wanted to use CERT_CompareName(), unfortunately NSS doesn't seem to
provide _any_ API to create a CERTName object from its DER-encoded
representation. The only alternative seems to create multiple CERTRDN objects
first, then pass then to CERT_CreateName() / CERT_AddRDN(), amd each CERTRDN is
created from a list of CERTAVA objects.

This is much more work, it requires parsing the DER encoded issuer names
explicitely, and I'm not sure it will be much better than comparing the
DER-encoded representations directly.

I'd be happy if you or wtc@ had a better suggestion for this issue.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_ios.cc:82: return true;
That makes sense, I'll fix all checks.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_mac.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_mac.cc:368: valid_issuers))
On 2012/12/13 19:49:05, Ryan Sleevi wrote:
> STYLE: multi-line conditionals should have braces (also lines 373-375)
> 
> BUG: See IOS bug about subject vs ISSUER

Done.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_openssl.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_openssl.cc:523: issuer = X509_get_subject_name(*it);
On 2012/12/13 19:49:05, Ryan Sleevi wrote:
> BUG: See previous comments re: subject vs issuer

Done.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_openssl.cc:529: // and 'cert_names'.
Yes, it's a left-over, I've removed it.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_openssl.cc:537: break;
Doh. Thanks for this.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_win.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_win.cc:129: const std::vector<std::string>&
issuer_names) {
On 2012/12/13 19:49:05, Ryan Sleevi wrote:
> STYLE: Indenting is wrong
> 
> bool IsCertNameBlobInIsuserList(
>     CERT_NAME_BLOB* name_blob,
>     const std::vector<std::string>& issuer_names) {
> }

Done.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_win.cc:490: valid_issuers))
On 2012/12/13 19:49:05, Ryan Sleevi wrote:
> STYLE: Indenting is messy here
> 
> if (IsCertNameBlobInIssuerList(&cert_handle->pCertInfo->Issuer,
>                                valid_issuers)) {
>   return true;
> }
> 
> Same on 496 as well

Done.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
File net/base/x509_util_nss.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
net/base/x509_util_nss.cc:541: if (SECITEM_ItemsAreEqual(&issuer, name))
On 2012/12/13 19:49:05, Ryan Sleevi wrote:
> use the CERTName comparison

That's not do-able for the reason explained previously (no NSS API to create a
CERTName from a DER-encoded byte array).
I've added a comment explaining this.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
File net/base/x509_util_nss.h (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
net/base/x509_util_nss.h:81: const std::vector<std::string>& valid_issuers);
Yes, I've actually changed the function to take a vector of CERTCertificate*,
which makes it easier to reuse for iOS too.

[agl, 2012-12-14 18:02:11]

On Fri, Dec 14, 2012 at 12:54 PM,  <digit@chromium.org> wrote:
> By the way, it seems that the order of RDN elements in a DER-encoded DN is
> strictly mandated by their encoding values. At least that's what is said in
> the
> "Names" section of the x509 guide at
> http://www.cs.auckland.ac.nz/~pgut001/pubs/x509guide.txt
>
> This probably means I shouldn't worry about this at all. Let me know if this
> is
> wrong.

I believe that standard says that you should trim whitespace from the
string values before comparing. I can't remember what it says about
order.

In practice, however, a byte-wise compare of the DER encoding is what's used.



Cheers

AGL

[Ryan Sleevi, 2012-12-14 18:16:41]

Trim whitespace, normalize case, normalize i18n representations, do order-less
comparison for the SETs and in-order comparison for the SEQUENCEs.

This is a subset of the X.500 rules, and goes back to RFC 2459. In a surprising
move for Apple, this is one of the few places where they followed the specs very
closely, which means that all platform APIs (NSS, CryptoAPI, CDSA) are doing
name comparison correctly. Apple even optimized a step further, by storing the
normalized name form, which it then stores re-encoded, so that it can just do a
DER->DER comparison of normalized name forms.

While we can add something to NSS to do so (which seems useful and a trivial
function for NSS, since it already has the normalization code), at least using
the correct type seems the proper way.

Apple & NSS follow RFC 3280 rules - in that they don't handle IDNA - but
Microsoft follows RFC 5280 rules, which do.

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_ios.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_ios.cc:75: valid_issuers))
On 2012/12/14 17:54:33, digit1 wrote:
> On 2012/12/13 19:49:05, Ryan Sleevi wrote:
> > It's a shame to have to reparse the CERTName here (we have it in
> > cert_handle->issuer)
> 
> I know, but cert_handle->issuer is a CertPrincipal which, has you already
> mentioned, should only be used for presentation. That's why I intentionally
> avoided using it here.

cert_handle->issuer is a CERTName, which is the canonical type, not a
CertPrincipal.

Note, I was talking about cert_handle->issuer, not this->issuer_

> 
> I also wanted to use CERT_CompareName(), unfortunately NSS doesn't seem to
> provide _any_ API to create a CERTName object from its DER-encoded
> representation. The only alternative seems to create multiple CERTRDN objects
> first, then pass then to CERT_CreateName() / CERT_AddRDN(), amd each CERTRDN
is
> created from a list of CERTAVA objects.

typedef scoped_ptr_malloc<CERTName, NSSDestroyer<CERTName, CERT_DestroyName> >
ScopedCERTName;


SECItem data;
data.len = ...
data.data = ...;

ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
if (!arena.get());
  return false;
ScopedCERTName name(PORT_ArenaZNew(arena, CERTName));
if (!name.get())
  return false;
SECStatus rv = SEC_ASN1DecodeItem(arena.get(), name.get(),
SEC_ASN1_GET(CERT_NameTemplate), &data);
if (rv != SECSuccess)
  return false;

/* Setting ->arena will ensure that name releases arena when it's done. If this
is all stack local, then you don't need to do this and just let the arena pool
handle cleanup */
name->arena = arena.release();

/* do something with a CERTName here */

return true;

> 
> This is much more work, it requires parsing the DER encoded issuer names
> explicitely, and I'm not sure it will be much better than comparing the
> DER-encoded representations directly.

It also yields the RFC-correct output, which comparing the DER-encoded
representations does not.

> 
> I'd be happy if you or wtc@ had a better suggestion for this issue.
>

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
File net/base/x509_util_nss.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
net/base/x509_util_nss.cc:541: if (SECITEM_ItemsAreEqual(&issuer, name))
On 2012/12/14 17:54:33, digit1 wrote:
> On 2012/12/13 19:49:05, Ryan Sleevi wrote:
> > use the CERTName comparison
> 
> That's not do-able for the reason explained previously (no NSS API to create a
> CERTName from a DER-encoded byte array).
> I've added a comment explaining this.

See comments about the NSS API to use to do this :)

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
File net/base/x509_util_nss.h (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_util_...
net/base/x509_util_nss.h:81: const std::vector<std::string>& valid_issuers);
On 2012/12/14 17:54:33, digit1 wrote:
> Yes, I've actually changed the function to take a vector of CERTCertificate*,
> which makes it easier to reuse for iOS too.

That works too

[Ryan Sleevi, 2012-12-14 19:09:20]

On Fri, Dec 14, 2012 at 9:54 AM,  <digit@chromium.org> wrote:
> By the way, it seems that the order of RDN elements in a DER-encoded DN is
> strictly mandated by their encoding values. At least that's what is said in
> the
> "Names" section of the x509 guide at
> http://www.cs.auckland.ac.nz/~pgut001/pubs/x509guide.txt
>
> This probably means I shouldn't worry about this at all. Let me know if this
> is
> wrong.

This is how things SHOULD be encoded when encoding DER.

In practice though, most certificate *DE*coders use BER processing
rules, since DER is strictly a subset of BER, and as a result, have
not been particular about enforcing this requirement - as long as the
structure decodes under BER rules.

There was some discussion of this back when CertID was being worked on
- http://www.ietf.org/mail-archive/web/certid/current/msg00140.html
for the start of the thread.

I have seen CAs that do insane/inane things, such as encoding the
entire name "hierarchy" via an RDN + AVA pairs, and failing to
properly sort it. These are typically enterprise CAs, although I think
it was a Spanish regional CA where I also saw this.

Comparing via the name comparison functions should "Do the Right Thing" though.

[digit1, 2012-12-18 16:19:24]

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
File net/base/x509_certificate_ios.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/3001/net/base/x509_certi...
net/base/x509_certificate_ios.cc:75: valid_issuers))
Thank you so much for this, I've implemented this in net/base/x509_util_nss.cc,
this makes the use of CERT_CompareName() possible.

[digit1, 2012-12-19 12:07:43]

I've added David Roger to review the iOS portion of the patch. Any reviewer
recommendations for the Windows one?

Apart from that, this should now be stable, though I'd like to add a unit test
or two.

[droger, 2012-12-19 12:26:23]

LGTM

https://chromiumcodereview.appspot.com/11579002/diff/20012/net/base/x509_cert...
File net/base/x509_certificate_ios.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/20012/net/base/x509_cert...
net/base/x509_certificate_ios.cc:75: // Convert to scoped CERTName* list.
Optional nit: you could consider sharing more code with x509_certificate_nss.cc,
for example by adding a new method to x509_util_nss:
IsIssuedByEncoded(const x509_util_ios::NSSCertChain& chain,
                  const std::vector<std::string>& valid_issuers)

It's only a few lines, so it may not worth it.  I'm just throwing this out as an
idea. If you've already discussed that issue, just ignore this comment.

https://chromiumcodereview.appspot.com/11579002/diff/20012/net/base/x509_cert...
net/base/x509_certificate_ios.cc:80: return false;
Nit: I think you need curly braces for multi-line ifs. If you change this, also
change the similar code in x509_certificate_nss.cc.

[droger, 2012-12-19 12:27:26]

Oops, ignore my first comment, it was wrong.

> Optional nit: you could consider sharing more code with
x509_certificate_nss.cc,
> for example by adding a new method to x509_util_nss:
> IsIssuedByEncoded(const x509_util_ios::NSSCertChain& chain,
>                   const std::vector<std::string>& valid_issuers)
> 
> It's only a few lines, so it may not worth it.  I'm just throwing this out as
an
> idea. If you've already discussed that issue, just ignore this comment.
>

[digit1, 2012-12-19 12:58:42]

https://chromiumcodereview.appspot.com/11579002/diff/20012/net/base/x509_cert...
File net/base/x509_certificate_ios.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/20012/net/base/x509_cert...
net/base/x509_certificate_ios.cc:75: // Convert to scoped CERTName* list.
That's what I tried originally in a previous patch, but as you discovered, it's
not really possible :)

https://chromiumcodereview.appspot.com/11579002/diff/20012/net/base/x509_cert...
net/base/x509_certificate_ios.cc:80: return false;
Thanks, I've fixed this in the latest patch.

[Ryan Sleevi, 2012-12-21 22:09:50]

Mostly nits.

Also, please add unit tests. You can see in x509_cert_types_unittest.cc we have
some DER-encoded distinguished names as part of the unit test data.

You could move these into net/base/test_certificate_data.h, so that they're in a
common location for the two unit tests, then load several of our test
certificates ( see net/base/cert_test_util.h ) and do positive and negative
matching unit tests.

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
File net/base/x509_certificate_ios.cc (right):

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
net/base/x509_certificate_ios.cc:79: valid_issuers, arena.get(), &issuers)) {
nit: indent is incorrect here

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
net/base/x509_certificate_ios.cc:82: // Do the comparison.
nit: Nix this comment

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
File net/base/x509_certificate_nss.cc (right):

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
net/base/x509_certificate_nss.cc:171: valid_issuers, arena.get(), &issuers))
nit: Both indent and bracing are incorrect here

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
File net/base/x509_certificate_openssl.cc (right):

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
net/base/x509_certificate_openssl.cc:512: cert_names(sk_X509_NAME_new_null());
Not sure why you're not just using a vector here?

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.cc
File net/base/x509_util_nss.cc (right):

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.cc...
net/base/x509_util_nss.cc:278: crypto::NSSDestroyer<CERTName, CERT_DestroyName>
> ScopedCERTName;
style nit: This indenting is wrong

typedef scoped_ptr_malloc<
    CERTName,
    crypto::NSSDestroyer<CERTName, CERT_DestroyName> > ScopedCERTName;

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.cc...
net/base/x509_util_nss.cc:299: SEC_ASN1_GET(CERT_NameTemplate), &item);
style: This indent is incorrect (minimally, should be exactly 4 spaces here),
but can be rewritten as follows

SECStatus rv = SEC_ASN1DecodeItem(
    arena, name.get(), SEC_ASN1_GET(CERT_NameTemplate), &item);

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.cc...
net/base/x509_util_nss.cc:573: }
RAII-safe alternative:

std::vector<CERTName*> result;
for (size_t i = 0; ...) {
  ...
}
if (result.size() == encoded_issuers.size()) {
  out->swap(result);
  return true;
}

for (size_t i = 0; i < result.size(); ++i)
  CERT_DestroyName(result[i]);
return false;

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.h
File net/base/x509_util_nss.h (right):

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.h#...
net/base/x509_util_nss.h:81: // intermediate CERTName objects pushed to |out|.
nit: I generally dislike APIs that require "On failure, the caller must cleanup
some temporary", since it prevents writing easy shortcuts.

Having |out| be unmodified on failure is easy (see next line) and lets you write

if (!GetIssuersFromEncodedList(...))
  return false;

Which is the better pattern.

[digit1, 2013-01-07 13:58:40]

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
File net/base/x509_certificate_ios.cc (right):

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
net/base/x509_certificate_ios.cc:79: valid_issuers, arena.get(), &issuers)) {
On 2012/12/21 22:09:50, Ryan Sleevi wrote:
> nit: indent is incorrect here

Done.

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
net/base/x509_certificate_ios.cc:82: // Do the comparison.
On 2012/12/21 22:09:50, Ryan Sleevi wrote:
> nit: Nix this comment

Done.

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
File net/base/x509_certificate_nss.cc (right):

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
net/base/x509_certificate_nss.cc:171: valid_issuers, arena.get(), &issuers))
On 2012/12/21 22:09:50, Ryan Sleevi wrote:
> nit: Both indent and bracing are incorrect here

Done.

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
File net/base/x509_certificate_openssl.cc (right):

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_certificate...
net/base/x509_certificate_openssl.cc:512: cert_names(sk_X509_NAME_new_null());
That works too.

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.cc
File net/base/x509_util_nss.cc (right):

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.cc...
net/base/x509_util_nss.cc:278: crypto::NSSDestroyer<CERTName, CERT_DestroyName>
> ScopedCERTName;
On 2012/12/21 22:09:50, Ryan Sleevi wrote:
> style nit: This indenting is wrong
> 
> typedef scoped_ptr_malloc<
>     CERTName,
>     crypto::NSSDestroyer<CERTName, CERT_DestroyName> > ScopedCERTName;

Done.

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.cc...
net/base/x509_util_nss.cc:299: SEC_ASN1_GET(CERT_NameTemplate), &item);
On 2012/12/21 22:09:50, Ryan Sleevi wrote:
> style: This indent is incorrect (minimally, should be exactly 4 spaces here),
> but can be rewritten as follows
> 
> SECStatus rv = SEC_ASN1DecodeItem(
>     arena, name.get(), SEC_ASN1_GET(CERT_NameTemplate), &item);

Done.

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.cc...
net/base/x509_util_nss.cc:573: }
On 2012/12/21 22:09:50, Ryan Sleevi wrote:
> RAII-safe alternative:
> 
> std::vector<CERTName*> result;
> for (size_t i = 0; ...) {
>   ...
> }
> if (result.size() == encoded_issuers.size()) {
>   out->swap(result);
>   return true;
> }
> 
> for (size_t i = 0; i < result.size(); ++i)
>   CERT_DestroyName(result[i]);
> return false;

Done.

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.h
File net/base/x509_util_nss.h (right):

https://codereview.chromium.org/11579002/diff/31001/net/base/x509_util_nss.h#...
net/base/x509_util_nss.h:81: // intermediate CERTName objects pushed to |out|.
Thanks, I'll fix this.

[digit1, 2013-01-07 17:53:22]

The latest patch adds a unit test that is slightly limited, each tested
certificate is tested against its direct issuer DN. I'd like to add a new test
that actually checks a certificate chain's intermediate CA issuers too.

I'm still uploading this simpler version to launch all the new tests on all
build bots, in order to detected implementation problems asap.

Note that this moves the hard-coded DN constants to test_certificate_data.h, and
also adds two new DNs here (ThawteDN + MITDN)

[Ryan Sleevi, 2013-01-07 18:11:13]

LGTM, mod BUG below.

https://chromiumcodereview.appspot.com/11579002/diff/43002/net/base/test_cert...
File net/base/test_certificate_data.h (right):

https://chromiumcodereview.appspot.com/11579002/diff/43002/net/base/test_cert...
net/base/test_certificate_data.h:488: const uint8 VARIABLE_IS_NOT_USED
VerisignDN[] = {
We'll want to fix the variable naming in a future cleanup CL.

https://chromiumcodereview.appspot.com/11579002/diff/43002/net/base/x509_util...
File net/base/x509_util_nss.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/43002/net/base/x509_util...
net/base/x509_util_nss.cc:589: if (CERT_CompareName(valid_issuers[i],
cert_issuer))
CERT_CompareName returns a SECComparison.

if (CERT_CompareName(valid_issuers[i], cert_issuer) == SECEqual) {
}

[digit1, 2013-01-08 14:25:13]

Patch set 13 adds a new unit test (and corresponding hard-coded DN tables) to
check that IsIssuedByEncoded() also works when the issuers are listed in the
chain's intermediate certificate (instead of the certificate's own Issuer
entry).

Patch set 14 is the same than 13, for some reason "git cl upload" complained
that base files were missing, but it looks like everything went to the review
server correctly.

[digit1, 2013-01-08 14:37:23]

https://chromiumcodereview.appspot.com/11579002/diff/58014/net/base/x509_cert...
File net/base/x509_certificate_unittest.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/58014/net/base/x509_cert...
net/base/x509_certificate_unittest.cc:394: ImportCertFromFile(certs_dir,
"salesforce_com_test.pem");
mmm, I really don't know what caused this, I'll fix it.

[Ryan Sleevi, 2013-01-08 20:14:18]

https://chromiumcodereview.appspot.com/11579002/diff/58015/net/base/test_cert...
File net/base/test_certificate_data.h (right):

https://chromiumcodereview.appspot.com/11579002/diff/58015/net/base/test_cert...
net/base/test_certificate_data.h:485: // Compute END - START, save it into LEN.
This comment needs reworking, because it's misleading or broken:

1) countryName is not required in the name, so trying to be clearer about which
fields contain it
2) Easier way to compute the length

// To output the subject or issuer of a certificate:
//
// openssl asnparse -i -inform PEM -in <cert>
//
// The output will contain
// SEQUENCE  [This is the issuer name]
//   ...
// SEQUENCE  [This is the validity period]
//   UTCTIME (or GENERALTIME)
//   UTCTIME
// SEQUENCE  [This is the subject]
//   ...
//
// The OFFSET is the first column before the : (eg: '21:d=2', the offset is 21)
for the SEQUENCE you're interested in
// The LENGTH is hl + l

https://chromiumcodereview.appspot.com/11579002/diff/58015/net/base/x509_cert...
File net/base/x509_certificate_unittest.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/58015/net/base/x509_cert...
net/base/x509_certificate_unittest.cc:817:
EXPECT_TRUE(cert_chain->IsIssuedByEncoded(issuers));
Add some negative tests here as well, since a compatible interface could just
always return true for IsIssuedByEncoded without failing a test here.

[digit1, 2013-01-09 14:01:46]

https://chromiumcodereview.appspot.com/11579002/diff/58015/net/base/test_cert...
File net/base/test_certificate_data.h (right):

https://chromiumcodereview.appspot.com/11579002/diff/58015/net/base/test_cert...
net/base/test_certificate_data.h:485: // Compute END - START, save it into LEN.
On 2013/01/08 20:14:18, Ryan Sleevi wrote:
> This comment needs reworking, because it's misleading or broken:
> 
> 1) countryName is not required in the name, so trying to be clearer about
which
> fields contain it
> 2) Easier way to compute the length
> 
> // To output the subject or issuer of a certificate:
> //
> // openssl asnparse -i -inform PEM -in <cert>
> //
> // The output will contain
> // SEQUENCE  [This is the issuer name]
> //   ...
> // SEQUENCE  [This is the validity period]
> //   UTCTIME (or GENERALTIME)
> //   UTCTIME
> // SEQUENCE  [This is the subject]
> //   ...
> //
> // The OFFSET is the first column before the : (eg: '21:d=2', the offset is
21)
> for the SEQUENCE you're interested in
> // The LENGTH is hl + l

Done.

https://chromiumcodereview.appspot.com/11579002/diff/58015/net/base/x509_cert...
File net/base/x509_certificate_unittest.cc (right):

https://chromiumcodereview.appspot.com/11579002/diff/58015/net/base/x509_cert...
net/base/x509_certificate_unittest.cc:817:
EXPECT_TRUE(cert_chain->IsIssuedByEncoded(issuers));
On 2013/01/08 20:14:18, Ryan Sleevi wrote:
> Add some negative tests here as well, since a compatible interface could just
> always return true for IsIssuedByEncoded without failing a test here.

Done.

[digit1, 2013-01-09 15:31:04]

Fine, since none of the failures are related to this code in any way, I'll
commit it as NOTRY.

[commit-bot: I haz the power, 2013-01-09 15:31:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/11579002/67002

[commit-bot: I haz the power, 2013-01-09 15:31:45]

Presubmit check for 11579002-67002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  net/base/test_certificate_data.h, line 498, 84 chars \
  net/base/test_certificate_data.h, line 513, 97 chars \
  net/base/test_certificate_data.h, line 537, 85 chars \
  net/base/test_certificate_data.h, line 541, 83 chars \
  net/base/test_certificate_data.h, line 579, 96 chars

Presubmit checks took 1.7s to calculate.

[digit1, 2013-01-09 15:35:12]

On 2013/01/09 15:31:45, I haz the power (commit-bot) wrote:
> Presubmit check for 11579002-67002 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Warnings **
> Found lines longer than 80 characters (first 5 shown).
>   net/base/test_certificate_data.h, line 498, 84 chars \
>   net/base/test_certificate_data.h, line 513, 97 chars \
>   net/base/test_certificate_data.h, line 537, 85 chars \
>   net/base/test_certificate_data.h, line 541, 83 chars \
>   net/base/test_certificate_data.h, line 579, 96 chars
> 
> Presubmit checks took 1.7s to calculate.

Hmmm, these are for existing lines that weren't modified by this commit. I'll
guess I'll have to reformat them then.

[digit1, 2013-01-11 16:33:27]

Failures are not related to this test, as usual. So committing now.

[commit-bot: I haz the power, 2013-01-11 16:33:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/11579002/78001

[commit-bot: I haz the power, 2013-01-11 16:37:03]

Change committed as 176371