RFC 2459 name comparison.

RFC 2459 name comparison.

BUG=410574
Committed: https://crrev.com/057b015555a4af92d9cf5108f9d2a7f4581378f3
Cr-Commit-Position: refs/heads/master@{#342750}

[mattm, 2015-05-13 00:26:49]

mattm@chromium.org changed reviewers:
+ rsleevi@chromium.org

[mattm, 2015-05-13 00:28:21]

Need to do something different with the test files since git thinks half of them
are text files, but I probably can't depend on the "openssl" binary being
available. Maybe zip them up or something?

[Ryan Sleevi, 2015-05-13 01:27:53]

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:15: class PrintableStringNormalizer {
Document :)

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:20: bool ReadByte(uint8_t* out) {
Because these are non-trivial, my suggestion would be to move the
implementations of these methods below their class declaration.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:43: *out = base::ToLowerASCII(c);
So this doesn't enforce that |c| is validly encoded according to the
PrintableString rules (with possible exceptions)

See http://www.itu.int/rec/T-REC-X.680-200811-I/en

A-Z
a-z
0-9
space
' ( ) + , - . / : = ?

are the valid characters

(That said, "*" is so frequently mis-encoded due to OpenSSL I suspect we'll need
to support it)

See https://bugzilla.mozilla.org/show_bug.cgi?id=1155767 for Mozilla's bug on
this.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:66: };
So as a design point, this won't scale when it comes to RFC 3280/5280 rules,
because you'll be comparing separate (but known) encodings like UTF-8 to
PrintableString.

My suggestion here would be that rather than treat this as a parser functor,
treat this as a normalization function. That is, for a given der::Input, yield
the "canonical" form (or failure). For PrintableString, the canonical form would
be a case-normalized, whitespace-eliminated form (data & length)

Then you can easily introduce additional string types by adding to the
normalization.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:85: return false;
This ends up being pretty inefficient in a tight loop, which is why
pre-normalizing can help.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:158: return false;
braces

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:164: return false;
braces

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:45: std::tr1::tuple<const char*,
const char*>> {
use ::testing::tuple, not std::str1::tuple

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:47: std::string value_type()
const { return std::tr1::get<0>(GetParam()); }
::testing::get<0>

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:51:
TEST_P(VerifyNameMatchSimpleTest, ExactEquality) {
Document each of these tests

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:55: std::string der_extra_attr =
document what you're testing

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:60: std::string der_extra_rdn =
Document

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:134: // 5280, different types
are transcoded to unicode, so this will need to be
s/unicode/Unicode/

[mattm, 2015-05-13 03:21:28]

Patchset #3 (id:40001) has been deleted

[mattm, 2015-05-13 03:24:06]

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:15: class PrintableStringNormalizer {
On 2015/05/13 01:27:52, Ryan Sleevi wrote:
> Document :)

Acknowledged.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:20: bool ReadByte(uint8_t* out) {
On 2015/05/13 01:27:52, Ryan Sleevi wrote:
> Because these are non-trivial, my suggestion would be to move the
> implementations of these methods below their class declaration.

Acknowledged.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:43: *out = base::ToLowerASCII(c);
On 2015/05/13 01:27:52, Ryan Sleevi wrote:
> So this doesn't enforce that |c| is validly encoded according to the
> PrintableString rules (with possible exceptions)
> 
> See http://www.itu.int/rec/T-REC-X.680-200811-I/en
> 
> A-Z
> a-z
> 0-9
> space
> ' ( ) + , - . / : = ?
> 
> are the valid characters
> 
> (That said, "*" is so frequently mis-encoded due to OpenSSL I suspect we'll
need
> to support it)
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1155767 for Mozilla's bug on
> this.

Done.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:66: };
On 2015/05/13 01:27:52, Ryan Sleevi wrote:
> So as a design point, this won't scale when it comes to RFC 3280/5280 rules,
> because you'll be comparing separate (but known) encodings like UTF-8 to
> PrintableString.
> 
> My suggestion here would be that rather than treat this as a parser functor,
> treat this as a normalization function. That is, for a given der::Input, yield
> the "canonical" form (or failure). For PrintableString, the canonical form
would
> be a case-normalized, whitespace-eliminated form (data & length)
> 
> Then you can easily introduce additional string types by adding to the
> normalization.

Done.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:85: return false;
On 2015/05/13 01:27:52, Ryan Sleevi wrote:
> This ends up being pretty inefficient in a tight loop, which is why
> pre-normalizing can help.

Acknowledged.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:158: return false;
On 2015/05/13 01:27:52, Ryan Sleevi wrote:
> braces

Done.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:164: return false;
On 2015/05/13 01:27:52, Ryan Sleevi wrote:
> braces

Done.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:45: std::tr1::tuple<const char*,
const char*>> {
On 2015/05/13 01:27:53, Ryan Sleevi wrote:
> use ::testing::tuple, not std::str1::tuple

Done.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:47: std::string value_type()
const { return std::tr1::get<0>(GetParam()); }
On 2015/05/13 01:27:53, Ryan Sleevi wrote:
> ::testing::get<0>

Done.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:51:
TEST_P(VerifyNameMatchSimpleTest, ExactEquality) {
On 2015/05/13 01:27:53, Ryan Sleevi wrote:
> Document each of these tests

Done.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:55: std::string der_extra_attr =
On 2015/05/13 01:27:53, Ryan Sleevi wrote:
> document what you're testing

Acknowledged. (I think the test comment addresses this, but I can explicitly
document these parts if you still think it needs more clarification.)

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:60: std::string der_extra_rdn =
On 2015/05/13 01:27:53, Ryan Sleevi wrote:
> Document

Acknowledged.

https://codereview.chromium.org/1125333005/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:134: // 5280, different types
are transcoded to unicode, so this will need to be
On 2015/05/13 01:27:53, Ryan Sleevi wrote:
> s/unicode/Unicode/

Done.

[nharper, 2015-05-14 19:21:43]

nharper@chromium.org changed reviewers:
+ nharper@chromium.org

[nharper, 2015-05-14 19:21:45]

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:18: der::ByteReader peaker(*reader);
nit: peeker

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:55: (c >= '+' && c <= ':') || c == '=' ||
c == '?') {
Add comments indicating what the ranges '\'' thru ')' and '+' thru ':' are?

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:77: if (a_tag != der::kOid || b_tag !=
der::kOid)
Since you want to match specific tags, why not do this:

if (!a->ReadTag(der::kOid, &a_value))
  return false;
if (!b->ReadTag(der::kOid, &b_value))
  return false;

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:87: return false;
After reading the attribute type and value, you should check that there is
nothing left to parse:

if (a->HasMore() || b->HasMore())
  return false;

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:141: }
Add a check that there's nothing in the inputs after the rdn sequence:

if (a_parser.HasMore() || b_parser.HasMore())
  return false;

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:135: std::tr1::make_tuple(false,
"UNIVERSALSTRING")));
Should these be ::testing::make_tuple?

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:191: }
All of these test cases assume a valid ASN.1 structure is encoded. Add test
cases for improper structures? (i.e. an AttributeTypeAndValue that has something
after the AttributeType and AttributeValue, or a Name that has something
following the RDNSequence)

[mattm, 2015-05-14 21:38:57]

Thanks, updated.

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:18: der::ByteReader peaker(*reader);
On 2015/05/14 19:21:44, nharper wrote:
> nit: peeker

doh.

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:55: (c >= '+' && c <= ':') || c == '=' ||
c == '?') {
On 2015/05/14 19:21:44, nharper wrote:
> Add comments indicating what the ranges '\'' thru ')' and '+' thru ':' are?

Done.

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:77: if (a_tag != der::kOid || b_tag !=
der::kOid)
On 2015/05/14 19:21:44, nharper wrote:
> Since you want to match specific tags, why not do this:
> 
> if (!a->ReadTag(der::kOid, &a_value))
>   return false;
> if (!b->ReadTag(der::kOid, &b_value))
>   return false;

Done.

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:87: return false;
On 2015/05/14 19:21:44, nharper wrote:
> After reading the attribute type and value, you should check that there is
> nothing left to parse:
> 
> if (a->HasMore() || b->HasMore())
>   return false;

Done.

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:141: }
On 2015/05/14 19:21:44, nharper wrote:
> Add a check that there's nothing in the inputs after the rdn sequence:
> 
> if (a_parser.HasMore() || b_parser.HasMore())
>   return false;

Done.

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:135: std::tr1::make_tuple(false,
"UNIVERSALSTRING")));
On 2015/05/14 19:21:44, nharper wrote:
> Should these be ::testing::make_tuple?

oops, done.

https://codereview.chromium.org/1125333005/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_name_match_unittest.cc:191: }
On 2015/05/14 19:21:44, nharper wrote:
> All of these test cases assume a valid ASN.1 structure is encoded. Add test
> cases for improper structures? (i.e. an AttributeTypeAndValue that has
something
> after the AttributeType and AttributeValue, or a Name that has something
> following the RDNSequence)

Good idea, done. (And fixed some cases allowing empty sets/sequences that
shouldn't be allowed.)

[Ryan Sleevi, 2015-05-21 02:20:17]

Just some remarks while you're digging in

https://codereview.chromium.org/1125333005/diff/80001/net/cert/internal/verif...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/80001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:67: }
Do we know what the performance implications of this will be? I'm concerned
we'll get into a situation like https://crbug.com/457949

I'm now wondering what the implications are of the "avoid UnsafeData", and
wonder if we should instead offer some sort of ReadToString() routine for making
copies of the data.

For the short-term, it's probably OK to do

std::string output(static_cast<const char*>(in.UnsafeData()), in.Length());

for (size_t read_pos = 0, write_pos = 0, end = output.size();
     read_pos < end; ++read_pos) {
  c = output[read_pos];
  if (c == ' ')
    if (write_pos && output[write_pos - 1] != ' ')
      output[write_pos++] = ' ';
  } else if (c >= 'A' && c <= 'Z') {
    output[write_pos++] = c + ('a' - 'A');
  } else if (...)
    output[write_pos++] = c;
  } else {
    return false;
  }
}
output.resize(write_pos);


I don't know if that's worth it. It may be overkill. It's something to keep an
eye out for performance wise. The above doesn't handle the case of trailing
whitespace, and skips leading whitespace, mostly because I can't remember what's
called for.

My concern is that the reader.ReadByte() may have enough to force a de-opt for
the pointer scanning, while the above approach may be better, at least gets
string checking. I'm not sure how I feel. It could also be rewritten using
iterators, which at least gets us checked iterators, and probably should be, but
I figured I'd throw it out there.

https://codereview.chromium.org/1125333005/diff/80001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:110: } else {
don't else after a return

[mattm, 2015-06-17 03:36:19]

Updated for review comments and with unicode transcoding implemented.

https://codereview.chromium.org/1125333005/diff/80001/net/cert/internal/verif...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/80001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:67: }
On 2015/05/21 02:20:17, Ryan Sleevi wrote:
> Do we know what the performance implications of this will be? I'm concerned
> we'll get into a situation like https://crbug.com/457949
> 
> I'm now wondering what the implications are of the "avoid UnsafeData", and
> wonder if we should instead offer some sort of ReadToString() routine for
making
> copies of the data.
> 
> For the short-term, it's probably OK to do
> 
> std::string output(static_cast<const char*>(in.UnsafeData()), in.Length());
> 
> for (size_t read_pos = 0, write_pos = 0, end = output.size();
>      read_pos < end; ++read_pos) {
>   c = output[read_pos];
>   if (c == ' ')
>     if (write_pos && output[write_pos - 1] != ' ')
>       output[write_pos++] = ' ';
>   } else if (c >= 'A' && c <= 'Z') {
>     output[write_pos++] = c + ('a' - 'A');
>   } else if (...)
>     output[write_pos++] = c;
>   } else {
>     return false;
>   }
> }
> output.resize(write_pos);
> 
> 
> I don't know if that's worth it. It may be overkill. It's something to keep an
> eye out for performance wise. The above doesn't handle the case of trailing
> whitespace, and skips leading whitespace, mostly because I can't remember
what's
> called for.
> 
> My concern is that the reader.ReadByte() may have enough to force a de-opt for
> the pointer scanning, while the above approach may be better, at least gets
> string checking. I'm not sure how I feel. It could also be rewritten using
> iterators, which at least gets us checked iterators, and probably should be,
but
> I figured I'd throw it out there.

I did some simple benchmark (release build, run unittests with
--gtest_repeat=5000 --single-process-tests, retry a few times and take the best
time). I compared ReadByte vs std::string copy like your example, and also
compared one like your example but using base::StringPiece to read from the
input (to see if the extra copy mattered.)

The version with ReadByte was about 7% slower. string vs StringPiece was pretty
much in the noise. So I'll just go with std::string since that has the advantage
of checked iterators.

https://codereview.chromium.org/1125333005/diff/80001/net/cert/internal/verif...
net/cert/internal/verify_name_match.cc:110: } else {
On 2015/05/21 02:20:17, Ryan Sleevi wrote:
> don't else after a return

Done.

[davidben, 2015-06-17 13:33:45]

davidben@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2015-06-17 13:33:47]

A drive-by comment because I was morbidly curious.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:219: while (a->HasMore() && b->HasMore())
{
Since these are SETs, the order of the elements may change when you normalize
the elements. It's in lexicographic order of the DER representation, so you'll
end up sorting on the total length first I believe? Does it turn out that
everything that needs normalization doesn't change the relative order of the
lengths? If they don't need to chop off that many spaces or whatever...

If the total length doesn't change, then, so long as each attribute type appears
at most once (is that guaranteed?), I believe you know it won't have to reorder.

Actually, looking at random certificates, each RDN only has one attribute, so
maybe that's actually why you don't care? (What does it mean to have multiple of
them anyway?)

Having to reorder on normalization seems like all kinds pain though. Might just
be worth a comment if this never ends up mattering? Worst that'll happen I think
is that we treat two RDNs as different when, after applying the normalization
rules we chose, they should have been equal.

[davidben, 2015-06-17 13:33:56]

davidben@chromium.org changed reviewers:
- davidben@chromium.org

[nharper, 2015-06-17 19:16:55]

I have 2 competing thoughts here:

1) Can the conversions from UniversalString, BMPString, UTF8String,
PrintableString to utf8-encoded std::string go in the der parser code
(net/der/parse_values.cc|h)? In a similar vein, I noticed that the logic in
NormalizeUtf8String is duplicated in NormalizePrintableStringValue - if the
conversions are factored out to the der parsing code, then
NormalizePrintableStringValue can use NormalizeUtf8String like the rest of the
normalization functions.

2) For PrintableString (if #1 is done), Universal String, and BMPString, we scan
the through the input twice - once for conversion (and validation) to utf8, and
again for rfc 2459/5280 normalization. I'm curious what the performance hit is
like with the double pass and if there's a performant way to separate the DER
string parsing from the normalization and avoid the duplication in
NormalizeUtf8String/NormalizePrintableStringValue.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:168: case der::kBmpString:
Do we care about TeletexStrings?

[Ryan Sleevi, 2015-06-17 23:56:59]

On 2015/06/17 19:16:55, nharper wrote:
> I have 2 competing thoughts here:
> 
> 1) Can the conversions from UniversalString, BMPString, UTF8String,
> PrintableString to utf8-encoded std::string go in the der parser code
> (net/der/parse_values.cc|h)? 

No. The conversion is specific to DistinguishedNames, not to the ASN.1 type.

> 2) For PrintableString (if #1 is done), Universal String, and BMPString, we
scan
> the through the input twice - once for conversion (and validation) to utf8,
and
> again for rfc 2459/5280 normalization. I'm curious what the performance hit is
> like with the double pass and if there's a performant way to separate the DER
> string parsing from the normalization and avoid the duplication in
> NormalizeUtf8String/NormalizePrintableStringValue.

I'm not too worried about this.

> Do we care about TeletexStrings?

Nope! It's a nightmare of different versions and tables and hell and
implementation issues (most implementations [incorrectly] assume T61 == Latin1,
which is sillypants since T61 was meant to cover ShiftJIS cases and you get all
mojibaked)

[Ryan Sleevi, 2015-06-18 00:28:44]

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:219: while (a->HasMore() && b->HasMore())
{
On 2015/06/17 13:33:46, David Benjamin wrote:
> If the total length doesn't change, then, so long as each attribute type
appears
> at most once (is that guaranteed?), I believe you know it won't have to
reorder.

(is that guaranteed) In theory, yes. In practice, no.

X.501 (2012), Section 9.3 specifies that "The set that forms an RDN contains
exactly one AttributeTypeAndValue for each attribute which contains
distinguished values in the entry; that is, a given attribute type cannot appear
twice in the same RDN.

An attribute value that has been designated to appear in an RDN is called a
distinguished value. There may be other values of the same attribute that are
not distinguished values and thus may not be used in an RDN.

An RDN for a given entry is formed by using one distinguished value from each
attribute that has distinguished values."

RFC 5280 incorporates X.501 (2005) in 4.1.2.4 "The issuer field is defined as
the X.501 type Name". While not directly referencing X.501's requirements on
inclusions (since X.501 also had crap like primaryDistinguished bits), the above
understanding is expected.

However, with that said, the canonical example of "stupid CA crap" is

SEQUENCE {
  SET {
    DomainComponent = IA5String("com"),
    DomainComponent = IA5String("example"),
    DomainComponent = IA5String("ssl"),
  }
}

[Although I seem to remember an implementation violating 3280, which admittedly
is specific to LDAP, and using DirectoryString-utf8string for the DC]

So we end up with the same attribute type, but different values, all within the
set. In the X.501 hierarchy, these are all part of the same level of the
hiearchy, and considered alternative/equivalent naming (OK, technically, invalid
naming; but if it was a DirectoryString and a SerialNumber, they'd be considered
equivalent expressions of the same naming hierarchy)

> 
> Actually, looking at random certificates, each RDN only has one attribute, so
> maybe that's actually why you don't care? (What does it mean to have multiple
of
> them anyway?)

Most common example is sticking a serialNumber field at the same hierarchy as
the email address and the commonName.

However, I want to be explicit: "looking at random certificates" is not the way
we want to do this. We want to strictly follow the specs, and where
appropriate/necessary, relax. We also want to implement the least bits
necessary, WHEN they can be safely detached.

[nharper, 2015-06-18 00:31:04]

On 2015/06/17 23:56:59, Ryan Sleevi wrote:
> On 2015/06/17 19:16:55, nharper wrote:
> > I have 2 competing thoughts here:
> > 
> > 1) Can the conversions from UniversalString, BMPString, UTF8String,
> > PrintableString to utf8-encoded std::string go in the der parser code
> > (net/der/parse_values.cc|h)? 
> 
> No. The conversion is specific to DistinguishedNames, not to the ASN.1 type.
> 
I understand that the normalization is specific to DistinguishedNames and RFCs
2459 and 5280, and it should stay where it is.

The conversion going on here (for UniversalString and BMPString) appears to be
following the definitions in X.690 (8.23.7 and 8.23.8) of those types and
converting them to utf8, so I don't see what's specific to DistinguishedNames.
Likewise, the restriction of character set for PrintableString is defined by
X.680 37.4, so having a function that takes a PrintableString, copies it to a
std::string, and validates that the characters in the std::string are in the
restricted character set sounds like something generic to the ASN.1 type and not
specific to DistinguishedNames. These conversions are what I was suggesting
moving to the DER parser (not the normalization).

> > 2) For PrintableString (if #1 is done), Universal String, and BMPString, we
> scan
> > the through the input twice - once for conversion (and validation) to utf8,
> and
> > again for rfc 2459/5280 normalization. I'm curious what the performance hit
is
> > like with the double pass and if there's a performant way to separate the
DER
> > string parsing from the normalization and avoid the duplication in
> > NormalizeUtf8String/NormalizePrintableStringValue.
> 
> I'm not too worried about this.
> 
> > Do we care about TeletexStrings?
> 
> Nope! It's a nightmare of different versions and tables and hell and
> implementation issues (most implementations [incorrectly] assume T61 ==
Latin1,
> which is sillypants since T61 was meant to cover ShiftJIS cases and you get
all
> mojibaked)

[Ryan Sleevi, 2015-06-18 01:30:12]

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:29: for (; read_iter != output->end() &&
*read_iter == ' '; ++read_iter) {
COMMENT: I think it's important to explain why isspace() [or related] don't need
to be used, since it's not (entirely) obvious.

Of the ASCII whitespace characters (' ', '\t', '\n', '\v', '\f', '\r'),
PrintableString is only allowed to contain SPACE (Section 41.4, Table 10, of
X.680 (2008))

// It's not necessary to cover all ASCII whitespace. Per X.680, Section 41.4,
// the only whitespace character allowed is space.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:52: // = ?
I feel that this description (and the comment I suggested) probably make more
sense as an overall function level comment

// Normalizes the DER-encoded PrintableString value |in| according to
// RFC 2459, Section 4.1.2.4
//
// Briefly, normalization involves removing leading and trailing
// whitespace, folding multiple whitespace characters into a single
// whitespace character, and normalizing on case (this function
// normalizes to lowercase).
//
// During normalization, this function also validates that |in|
// is properly encoded - that is, that it restricts to the character
// set defined in X.680 (2008), Section 41.4, Table 10. X.680 defines
// the valid characters as
//   a-z A-Z 0-9 (space) ' ( ) + , - . / : = ?
//
// However, due to an old OpenSSL encoding bug, a number of
// certificates have also included '*', which has historically been
// allowed by implementations, and so is also allowed here.
//
// If |in| can be normalized, returns true and sets |output| to the
// case folded, normalized value. If |in| is invalid, returns false.
// NOTE: |output| will be modified regardless of the return, so
// callers are responsible to check the result.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:66: // case folding and whitespace
folding is performed.
Reword:

// Normalizes |output|, a UTF-8 encoded string, as if it contained
// only ASCII characters.
//
// This could be considered a partial subset of RFC 5280 rules, and
// is compatible with RFC 2459/3280.
//
// In particular, RFC 5280, Section 7.1 describes how UTF8String
// and PrintableString should be compared - using the LDAP StringPrep
// profile of RFC 4518, with case folding and whitespace compression.
// However, because it is optional for implementations and because
// it's desirable to avoid the size cost of a the StringPrep tables,
// this function treats |output| as if it was composed of ASCII.
//
// That is, rather than folding all whitespace characters, it only
// folds ' '. Rather than case folding using locale-aware handling,
// it only folds A-Z to a-z.
//
// This gives better results than outright rejecting (due to mismatched
// encodings), or from doing a strict binary comparison (the minimum
// required by RFC 5280), and is sufficient for those certificates
// publicly deployed.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:94: }
Is there any reason not to combine NormalizeUtf8String and
NormalizePrintableStringValue?

That is,

NormalizeConvertedDirectoryString(bool enforce_printable_string,
                                  std::string* directory_string) {
  for (...) {
    if (c == ' ') {
    } else if (c >= 'A' && c <= 'Z') {
    } else {
      if (enforce_printable_string && !IsLegalPrintableString(c)) {
        // Contains a character illegal for PrintableString
        return false;
      }
      *(write_iter++) = c;
    }

}

bool NormalizePrintableStringValue(const der::Input& in, std::string* output) {
  output->assign(...);
  return NormalizeConvertedDirectoryString(true, output);
}

bool NormalizeUtf8StringValue(const der::Input& in, std::string* output) {
  output->assign(...);
  return NormalizeConvertedDirectoryString(false, output);
}

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:102: // Convert BMPString value to UTF-8
and then normalize it.
STYLE: Per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...

All of these should be following descriptive ("Converts a UTF8String") rather
than imperative ("Convert a UTF8String") form.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:107: base::string16
s16(reinterpret_cast<const base::char16*>(in.UnsafeData()),
naming nit: My gut is that |s16| violates the naming rules, but I'm not too
picky here. Is there a better name?

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:114: // surrogates are not allowed.
grammar nit:

either ',' -> ';' or ', surrogates' -> '. Surrogates'

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:163: bool IsNormalizable(der::Tag tag) {
IsNormalizableDirectoryString ?

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:168: case der::kBmpString:
On 2015/06/17 19:16:55, nharper wrote:
> Do we care about TeletexStrings?

Ooops, botched commenting. No. But definitely should add a comment why

// TeletexString isn't normalized. Section 8 of RFC 5280 briefly
// describes the historical confusion between treating TeletexString
// as Latin1String vs T.61, and there are even incompatibilities within
// T.61 implementations. As this time is virtually unused, simply
// treat it with a binary comparison, as permitted by RFC 3280/5280.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:168: case der::kBmpString:
IA5String as well, which comes up with domainComponent

(Despite the obtusity of X.680 and the reference to the "International Register
of Coded Character Sets to be Used with Escape Sequences" which is such a PITA
to find, it's Registrations 1, 6, SPACE, and DELETE

https://www.itscj.ipsj.or.jp/iso-ir/001.pdf <-- Registration 1
https://www.itscj.ipsj.or.jp/iso-ir/006.pdf <-- Registration 6

Which are themselves just enumerating the ISO 646 space (the 32 control
characters, 0-9, :-?, A-Z, a-z, DEL). See
https://en.wikipedia.org/wiki/ISO/IEC_646

Which is... SURPRISE... ASCII :) [OK, strictly speaking the values of things
like '#' can vary by region, but the character itself is 0x00 - 0x1F (control
chars - Registration 1), 0x20 (space), 0x21 - 0x7E (printable characters -
Registration 6), and 0x7F (delete).

0x00 - 0x7F. ASCII :)

If you want to try to distill that into a brief comment, be my guest, but I'd go
with

// IA5String is ISO/IEC Registrations 1 and 6 from the ISO
// "International Register of Coded Character Sets to be used
// with Escape Sequences", plus space and delete. That's just the
// polite way of saying 0x00 - 0x7F, aka ASCII (or, more formally,
// ISO/IEC 646)

[davidben, 2015-06-18 01:58:26]

davidben@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2015-06-18 01:58:27]

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:219: while (a->HasMore() && b->HasMore())
{
On 2015/06/18 00:28:44, Ryan Sleevi wrote:
> However, I want to be explicit: "looking at random certificates" is not the
way
> we want to do this. We want to strictly follow the specs, and where
> appropriate/necessary, relax. We also want to implement the least bits
> necessary, WHEN they can be safely detached.

I'm quite aware of that. I think you missed the point of my comment. In context,
"looking at random certificates" was a cursory attempt to figure out whether
multiple AttributeTypeAndValue pairs ever came up. I'm saying that, in order to
strictly follow the specs, I believe we need to reorder things because of how
SETs in DER are encoded.

That is, this logic will canonicalize:
[Assume, for the sake of discussion, that CountryName and DomainComponent's OID
representations have the same length.]

  SET {
     CountryName = IA5String("aa")
     DomainComponent = PrintableString("              b")
  }

to

  SET {
     CountryName = IA5String("aa")
     DomainComponent = PrintableString("b")
  }

But this is wrong. That is not valid DER. SETs unordered and serialized by
sorting their elements by their DER representation. The tags of both pairs are
SEQUENCE, so you compare the length next, and the first child now has a longer
total length than the second. So the actual canonicalized form is:

  SET {
     DomainComponent = PrintableString("b")
     CountryName = IA5String("aa")
  }

If we want to canonicalize and do so without violating DER, we need a reordering
step. Which is going to be a huge pain, and so it may be worth considering not
doing this. If we don't, that means we are making various assumptions, hence the
discussion about whether multiple attributes ever come up in cases we have to
canonicalize, etc. We'd also want a comment explaining the intentional omission,
should we decide to make it.

[Ryan Sleevi, 2015-06-18 03:30:01]

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:219: while (a->HasMore() && b->HasMore())
{
On 2015/06/18 01:58:27, David Benjamin wrote:
> If we want to canonicalize and do so without violating DER, we need a
reordering
> step.

No, because this code doesn't return the canonicalized form back to the caller.
It just verifies a match.

Yes, there's a bug lurking here because within the RDN, you *don't* match to
make sure they have the same order, you just make sure that |b| the same number
of elements of |a| and that every value within |b| is present in |a|.

I think we're talking about the same bug, but talking different solutions. Your
remark is to sort them as part of canonicalization, which is one option. Another
would be just a linear scan.

> Which is going to be a huge pain, and so it may be worth considering not
> doing this. If we don't, that means we are making various assumptions, hence
the
> discussion about whether multiple attributes ever come up in cases we have to
> canonicalize, etc. We'd also want a comment explaining the intentional
omission,
> should we decide to make it.

I have no idea why you say it would be a huge pain. You could linear scan |b|
for every element in |a| cheaply. You could ingest both |a| and |b| into two
vectors of der::Input tuples, sort on the type, and then just iterate then (with
or without a linear scan substep for misencoded certs that have multiple values
of the same type).

You said "without violating DER" (so, re-encoding), but I'm not sure where that
would come up. Our internal indexing can use whatever sort function we want. The
only time I can think a normalized DER would come up with would be OS X
integration, but then we would potentially already have issues because of the
stringprep. That is, any normalization for OS API integration would be a
separate function solely for that integration. Here, we just need to make sure
that a and b fully intersect, and we can do so however we want.

[mattm, 2015-06-19 22:04:24]

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:52: // = ?
On 2015/06/18 01:30:12, Ryan Sleevi wrote:
> I feel that this description (and the comment I suggested) probably make more
> sense as an overall function level comment
> 
> // Normalizes the DER-encoded PrintableString value |in| according to
> // RFC 2459, Section 4.1.2.4
> //
> // Briefly, normalization involves removing leading and trailing
> // whitespace, folding multiple whitespace characters into a single
> // whitespace character, and normalizing on case (this function
> // normalizes to lowercase).
> //
> // During normalization, this function also validates that |in|
> // is properly encoded - that is, that it restricts to the character
> // set defined in X.680 (2008), Section 41.4, Table 10. X.680 defines
> // the valid characters as
> //   a-z A-Z 0-9 (space) ' ( ) + , - . / : = ?
> //
> // However, due to an old OpenSSL encoding bug, a number of
> // certificates have also included '*', which has historically been
> // allowed by implementations, and so is also allowed here.
> //
> // If |in| can be normalized, returns true and sets |output| to the
> // case folded, normalized value. If |in| is invalid, returns false.
> // NOTE: |output| will be modified regardless of the return, so
> // callers are responsible to check the result.

Done.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:66: // case folding and whitespace
folding is performed.
On 2015/06/18 01:30:12, Ryan Sleevi wrote:
> Reword:
> 
> // Normalizes |output|, a UTF-8 encoded string, as if it contained
> // only ASCII characters.
> //
> // This could be considered a partial subset of RFC 5280 rules, and
> // is compatible with RFC 2459/3280.
> //
> // In particular, RFC 5280, Section 7.1 describes how UTF8String
> // and PrintableString should be compared - using the LDAP StringPrep
> // profile of RFC 4518, with case folding and whitespace compression.
> // However, because it is optional for implementations and because
> // it's desirable to avoid the size cost of a the StringPrep tables,
> // this function treats |output| as if it was composed of ASCII.
> //
> // That is, rather than folding all whitespace characters, it only
> // folds ' '. Rather than case folding using locale-aware handling,
> // it only folds A-Z to a-z.
> //
> // This gives better results than outright rejecting (due to mismatched
> // encodings), or from doing a strict binary comparison (the minimum
> // required by RFC 5280), and is sufficient for those certificates
> // publicly deployed.

done.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:94: }
On 2015/06/18 01:30:12, Ryan Sleevi wrote:
> Is there any reason not to combine NormalizeUtf8String and
> NormalizePrintableStringValue?
> 
> That is,
> 
> NormalizeConvertedDirectoryString(bool enforce_printable_string,
>                                   std::string* directory_string) {
>   for (...) {
>     if (c == ' ') {
>     } else if (c >= 'A' && c <= 'Z') {
>     } else {
>       if (enforce_printable_string && !IsLegalPrintableString(c)) {
>         // Contains a character illegal for PrintableString
>         return false;
>       }
>       *(write_iter++) = c;
>     }
> 
> }
> 
> bool NormalizePrintableStringValue(const der::Input& in, std::string* output)
{
>   output->assign(...);
>   return NormalizeConvertedDirectoryString(true, output);
> }
> 
> bool NormalizeUtf8StringValue(const der::Input& in, std::string* output) {
>   output->assign(...);
>   return NormalizeConvertedDirectoryString(false, output);
> }

Done.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:102: // Convert BMPString value to UTF-8
and then normalize it.
On 2015/06/18 01:30:12, Ryan Sleevi wrote:
> STYLE: Per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...
> 
> All of these should be following descriptive ("Converts a UTF8String") rather
> than imperative ("Convert a UTF8String") form.

Done.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:107: base::string16
s16(reinterpret_cast<const base::char16*>(in.UnsafeData()),
On 2015/06/18 01:30:12, Ryan Sleevi wrote:
> naming nit: My gut is that |s16| violates the naming rules, but I'm not too
> picky here. Is there a better name?

yeah... maybe "in_16bit"? Trying to avoid something excessively verbose.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:114: // surrogates are not allowed.
On 2015/06/18 01:30:12, Ryan Sleevi wrote:
> grammar nit:
> 
> either ',' -> ';' or ', surrogates' -> '. Surrogates'

Done.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:163: bool IsNormalizable(der::Tag tag) {
On 2015/06/18 01:30:12, Ryan Sleevi wrote:
> IsNormalizableDirectoryString ?

Done.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:168: case der::kBmpString:
On 2015/06/18 01:30:12, Ryan Sleevi wrote:
> On 2015/06/17 19:16:55, nharper wrote:
> > Do we care about TeletexStrings?
> 
> Ooops, botched commenting. No. But definitely should add a comment why
> 
> // TeletexString isn't normalized. Section 8 of RFC 5280 briefly
> // describes the historical confusion between treating TeletexString
> // as Latin1String vs T.61, and there are even incompatibilities within
> // T.61 implementations. As this time is virtually unused, simply
> // treat it with a binary comparison, as permitted by RFC 3280/5280.

Done.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:168: case der::kBmpString:
On 2015/06/18 01:30:12, Ryan Sleevi wrote:
> IA5String as well, which comes up with domainComponent
> 
> (Despite the obtusity of X.680 and the reference to the "International
Register
> of Coded Character Sets to be Used with Escape Sequences" which is such a PITA
> to find, it's Registrations 1, 6, SPACE, and DELETE
> 
> https://www.itscj.ipsj.or.jp/iso-ir/001.pdf <-- Registration 1
> https://www.itscj.ipsj.or.jp/iso-ir/006.pdf <-- Registration 6
> 
> Which are themselves just enumerating the ISO 646 space (the 32 control
> characters, 0-9, :-?, A-Z, a-z, DEL). See
> https://en.wikipedia.org/wiki/ISO/IEC_646
> 
> Which is... SURPRISE... ASCII :) [OK, strictly speaking the values of things
> like '#' can vary by region, but the character itself is 0x00 - 0x1F (control
> chars - Registration 1), 0x20 (space), 0x21 - 0x7E (printable characters -
> Registration 6), and 0x7F (delete).
> 
> 0x00 - 0x7F. ASCII :)
> 
> If you want to try to distill that into a brief comment, be my guest, but I'd
go
> with
> 
> // IA5String is ISO/IEC Registrations 1 and 6 from the ISO
> // "International Register of Coded Character Sets to be used
> // with Escape Sequences", plus space and delete. That's just the
> // polite way of saying 0x00 - 0x7F, aka ASCII (or, more formally,
> // ISO/IEC 646)

Done.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:219: while (a->HasMore() && b->HasMore())
{
On 2015/06/17 13:33:46, David Benjamin wrote:
> Since these are SETs, the order of the elements may change when you normalize
> the elements. It's in lexicographic order of the DER representation, so you'll
> end up sorting on the total length first I believe? Does it turn out that
> everything that needs normalization doesn't change the relative order of the
> lengths? If they don't need to chop off that many spaces or whatever...
> 
> If the total length doesn't change, then, so long as each attribute type
appears
> at most once (is that guaranteed?), I believe you know it won't have to
reorder.
> 
> Actually, looking at random certificates, each RDN only has one attribute, so
> maybe that's actually why you don't care? (What does it mean to have multiple
of
> them anyway?)
> 
> Having to reorder on normalization seems like all kinds pain though. Might
just
> be worth a comment if this never ends up mattering? Worst that'll happen I
think
> is that we treat two RDNs as different when, after applying the normalization
> rules we chose, they should have been equal.

Good catch.

https://codereview.chromium.org/1125333005/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:219: while (a->HasMore() && b->HasMore())
{
On 2015/06/18 03:30:01, Ryan Sleevi wrote:
> On 2015/06/18 01:58:27, David Benjamin wrote:
> > If we want to canonicalize and do so without violating DER, we need a
> reordering
> > step.
> 
> No, because this code doesn't return the canonicalized form back to the
caller.
> It just verifies a match.
> 
> Yes, there's a bug lurking here because within the RDN, you *don't* match to
> make sure they have the same order, you just make sure that |b| the same
number
> of elements of |a| and that every value within |b| is present in |a|.
> 
> I think we're talking about the same bug, but talking different solutions.
Your
> remark is to sort them as part of canonicalization, which is one option.
Another
> would be just a linear scan.
> 
> > Which is going to be a huge pain, and so it may be worth considering not
> > doing this. If we don't, that means we are making various assumptions, hence
> the
> > discussion about whether multiple attributes ever come up in cases we have
to
> > canonicalize, etc. We'd also want a comment explaining the intentional
> omission,
> > should we decide to make it.
> 
> I have no idea why you say it would be a huge pain. You could linear scan |b|
> for every element in |a| cheaply. You could ingest both |a| and |b| into two
> vectors of der::Input tuples, sort on the type, and then just iterate then
(with
> or without a linear scan substep for misencoded certs that have multiple
values
> of the same type).
> 
> You said "without violating DER" (so, re-encoding), but I'm not sure where
that
> would come up. Our internal indexing can use whatever sort function we want.
The
> only time I can think a normalized DER would come up with would be OS X
> integration, but then we would potentially already have issues because of the
> stringprep. That is, any normalization for OS API integration would be a
> separate function solely for that integration. Here, we just need to make sure
> that a and b fully intersect, and we can do so however we want.

Did the "ingest into two vectors and match" thing. I didn't do the sort part,
since it seems unlikely any sane cert would have enough elements in a RDN for it
to matter.

[Ryan Sleevi, 2015-06-20 00:15:46]

Partial LGTM :)

I've reviewed the implementation, but not all of the tests yet, but I think I'm
OK. If either Nick or David beat me to reviewing tests, their LGTM suffices.

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:25: };
nit: newline between 25-26 & document at line 20?

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:36: // it's desirable to avoid the size
cost of a the StringPrep tables,
s/of a the/of the/

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:320: for (auto const& b_type_and_value :
b_type_and_values) {
auto const& is different than const auto&, AIUI.

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:342: // fall back to binary comparison on
unexpected input?
This TODO is worked out now, right?

(Fine to reject on parse errors IMO)

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match_unittest.cc:114: : public
::testing::TestWithParam<::testing::tuple<bool, const char*>> {
nit: Seems like it'd be less writing on lines 147-153 just to use a helper
struct rather than ::testing::tuple, but I'm mostly ambivalent, since it's only
a few lines.

[mattm, 2015-06-22 23:42:10]

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:25: };
On 2015/06/20 00:15:45, Ryan Sleevi wrote:
> nit: newline between 25-26 & document at line 20?

Done.

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:36: // it's desirable to avoid the size
cost of a the StringPrep tables,
On 2015/06/20 00:15:45, Ryan Sleevi wrote:
> s/of a the/of the/

Done.

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:320: for (auto const& b_type_and_value :
b_type_and_values) {
On 2015/06/20 00:15:45, Ryan Sleevi wrote:
> auto const& is different than const auto&, AIUI.

Done.

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:342: // fall back to binary comparison on
unexpected input?
On 2015/06/20 00:15:45, Ryan Sleevi wrote:
> This TODO is worked out now, right?
> 
> (Fine to reject on parse errors IMO)

Oh yeah. Done.

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match_unittest.cc:114: : public
::testing::TestWithParam<::testing::tuple<bool, const char*>> {
On 2015/06/20 00:15:45, Ryan Sleevi wrote:
> nit: Seems like it'd be less writing on lines 147-153 just to use a helper
> struct rather than ::testing::tuple, but I'm mostly ambivalent, since it's
only
> a few lines.

I gave it a try, but it only made those lines a little shorter and added extra
lines to define the struct w/constructor. It didn't seem overall cleaner and I
figure it's better just to keep consistency with the other tests that use tuple.

[Ryan Sleevi, 2015-06-23 07:47:54]

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/180001/net/cert/internal/veri...
net/cert/internal/verify_name_match_unittest.cc:114: : public
::testing::TestWithParam<::testing::tuple<bool, const char*>> {
On 2015/06/22 23:42:10, mattm wrote:
> On 2015/06/20 00:15:45, Ryan Sleevi wrote:
> > nit: Seems like it'd be less writing on lines 147-153 just to use a helper
> > struct rather than ::testing::tuple, but I'm mostly ambivalent, since it's
> only
> > a few lines.
> 
> I gave it a try, but it only made those lines a little shorter and added extra
> lines to define the struct w/constructor. It didn't seem overall cleaner and I
> figure it's better just to keep consistency with the other tests that use
tuple.

You shouldn't need a constructor - I would presume value_type() could be
expressed as "const char* const", and then  you could just use an initializer
list

That is

struct SomeName {
  bool expect_match;
  const char* const value_type;
} kFoo[] = {
  { true, "PRINTABLESTRING" },
  { false, "T61String" },
  ...
};

::testing::ValuesIn(kFoo);

but yeah, like I said, it's not a big deal.

[eroman, 2015-07-03 22:31:18]

eroman@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2015-07-03 22:31:20]

My 2 cents.

Apologies if there are too many cooks in the kitchen now ruining the stew :)

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:12: #include
"net/cert/internal/verify_name_match.h"
nit: this should be first

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:53: // NOTE: |output| will be modified
regardless of the return, so
nit: Suggest using WARN_UNUSED_RESULT instead of this comment.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:60: std::string::iterator write_iter =
output->begin();
Style comment:

Did you consider not doing an in-place update?

I think it would be simpler to express this with append operations on a new
string.

From a performance perspective the current approach ends up overwriting every
character anyway, so might as well reserve a new string buffer. And could
probably make the input be a der::Input() and avoid allocation there instead.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:125:
output->assign(reinterpret_cast<const char*>(in.UnsafeData()), in.Length());
Might be worth adding a method on der::Input to copy to a string.

Or per the comment above, avoid the copy here and just pass a der::Input()
directly to NormalizeDirectoryString()  (and have it output a modified string).

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:196: reinterpret_cast<const
uint32_t*>(in.UnsafeData()),
Is this actually legit?

I thought this sort of casting in c++ was undefined behavior, because when you
deref to get a uint32_t from this, it may result in unaligned memory. x86 is
forgiving, but other platforms may not be.

Unless I am off my rocker here, I think the right thing to do is iterate every 4
bytes and then just convert them directly from big-endian to platform-endiannes
int.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:198: for
(std::vector<uint32_t>::const_iterator i = in_32bit.begin();
nit: the new hotness would be for-in notation.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:320: for (const auto& a_type_and_value :
a_type_and_values) {
I am a little bit skeptical about the performance of this approach.

If our plan is to run certificate verifying directly on the network thread (and
use async interfaces for networking and cert store I/O), then without any limits
put on these sizes, then would it be possible to construct a certificate whose
verification would hang the browser?

How about sorting the values first and then just doing an equality match?

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match_unittest.cc:20: return
der::Input(reinterpret_cast<const uint8_t*>(s.data()), s.size());
My suggestion is to make this this a ctor of der::Input()  -- I had the same
desire to use std::string in one of my changes.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match_unittest.cc:23: std::string
LoadTestData(const std::string& prefix,
Can you add comments on what prefix/valuetype and suffix are? The constructed
filename otherwise is a bit mysterious to me.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match_unittest.cc:110:
::testing::Combine(::testing::ValuesIn(kValueTypes),
I gave up on reading all the gtest magic. Can you add comments on what the test
variants do and how they work?

https://codereview.chromium.org/1125333005/diff/200001/net/data/verify_name_m...
File
net/data/verify_name_match_unittest/names/ascii-PRINTABLESTRING-case_swap.der
(right):

https://codereview.chromium.org/1125333005/diff/200001/net/data/verify_name_m...
net/data/verify_name_match_unittest/names/ascii-PRINTABLESTRING-case_swap.der:1:
0w1
I suggest using PEM for all of these test files, rather than binary DER.

(Ryan suggested this to me on one of my CLs too).

PEM has several advantages:
  * Possible to read the diffs in Rietveld
  * Can copy paste directly into tools like https://lapo.it/asn1js/ (which I
find very useful)
  * You can include comments directly in the test file (not just the payload),
such as showing the pretty-printed ASN.1 structure

As an example I am using PEM for data files in this (unreviewed) changelist:
  https://codereview.chromium.org/1209283004/

And find it works quite well for that purpose.

[eroman, 2015-07-07 17:29:21]

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match_unittest.cc:20: return
der::Input(reinterpret_cast<const uint8_t*>(s.data()), s.size());
On 2015/07/03 22:31:20, eroman wrote:
> My suggestion is to make this this a ctor of der::Input()  -- I had the same
> desire to use std::string in one of my changes.

Actually seems there was already some history behind this.
Forget this comment :)

[mattm, 2015-07-16 04:35:37]

Patchset #11 (id:220001) has been deleted

[mattm, 2015-07-16 04:42:32]

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:12: #include
"net/cert/internal/verify_name_match.h"
On 2015/07/03 22:31:20, eroman wrote:
> nit: this should be first

Done.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:53: // NOTE: |output| will be modified
regardless of the return, so
On 2015/07/03 22:31:20, eroman wrote:
> nit: Suggest using WARN_UNUSED_RESULT instead of this comment.

Added WARN_UNUSED_RESULT. I left the first part of the comment since it still
seems good to explicitly point out.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:60: std::string::iterator write_iter =
output->begin();
On 2015/07/03 22:31:20, eroman wrote:
> Style comment:
> 
> Did you consider not doing an in-place update?
> 
> I think it would be simpler to express this with append operations on a new
> string.
> 
> From a performance perspective the current approach ends up overwriting every
> character anyway, so might as well reserve a new string buffer. And could
> probably make the input be a der::Input() and avoid allocation there instead.

See comment #10 for a bit of background if you haven't read that already. I
tested using a StringPiece on the der::Input data and constructing the output
from that, but didn't see any measurable performance difference vs this
approach. I do agree the StringPiece->string approach is a bit simpler code, but
one of the advantages of std::string is the checked iterators in debug mode,
which StringPiece doesn't have.  Ryan, do you have other opinions on this?

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:125:
output->assign(reinterpret_cast<const char*>(in.UnsafeData()), in.Length());
On 2015/07/03 22:31:20, eroman wrote:
> Might be worth adding a method on der::Input to copy to a string.

Done. Ryan/Nick, does that look good to you?

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:196: reinterpret_cast<const
uint32_t*>(in.UnsafeData()),
On 2015/07/03 22:31:20, eroman wrote:
> Is this actually legit?
> 
> I thought this sort of casting in c++ was undefined behavior, because when you
> deref to get a uint32_t from this, it may result in unaligned memory. x86 is
> forgiving, but other platforms may not be.

I think you're right.

> Unless I am off my rocker here, I think the right thing to do is iterate every
4
> bytes and then just convert them directly from big-endian to
platform-endiannes
> int.

I switched to using memcpy to initialize the string/vector since that seems to
match the comments from Ryan earlier.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:198: for
(std::vector<uint32_t>::const_iterator i = in_32bit.begin();
On 2015/07/03 22:31:20, eroman wrote:
> nit: the new hotness would be for-in notation.

Done.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:320: for (const auto& a_type_and_value :
a_type_and_values) {
On 2015/07/03 22:31:20, eroman wrote:
> I am a little bit skeptical about the performance of this approach.
> 
> If our plan is to run certificate verifying directly on the network thread
(and
> use async interfaces for networking and cert store I/O), then without any
limits
> put on these sizes, then would it be possible to construct a certificate whose
> verification would hang the browser?
> 
> How about sorting the values first and then just doing an equality match?

Ryan's opinion was that there are already ways to construct pathological
certificates so not to worry about it, but I could certainly change it if
needed.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match_unittest.cc:23: std::string
LoadTestData(const std::string& prefix,
On 2015/07/03 22:31:20, eroman wrote:
> Can you add comments on what prefix/valuetype and suffix are? The constructed
> filename otherwise is a bit mysterious to me.

Done.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match_unittest.cc:110:
::testing::Combine(::testing::ValuesIn(kValueTypes),
On 2015/07/03 22:31:20, eroman wrote:
> I gave up on reading all the gtest magic. Can you add comments on what the
test
> variants do and how they work?

Done.

https://codereview.chromium.org/1125333005/diff/200001/net/data/verify_name_m...
File
net/data/verify_name_match_unittest/names/ascii-PRINTABLESTRING-case_swap.der
(right):

https://codereview.chromium.org/1125333005/diff/200001/net/data/verify_name_m...
net/data/verify_name_match_unittest/names/ascii-PRINTABLESTRING-case_swap.der:1:
0w1
On 2015/07/03 22:31:20, eroman wrote:
> I suggest using PEM for all of these test files, rather than binary DER.
> 
> (Ryan suggested this to me on one of my CLs too).
> 
> PEM has several advantages:
>   * Possible to read the diffs in Rietveld
>   * Can copy paste directly into tools like https://lapo.it/asn1js/ (which I
> find very useful)
>   * You can include comments directly in the test file (not just the payload),
> such as showing the pretty-printed ASN.1 structure
> 
> As an example I am using PEM for data files in this (unreviewed) changelist:
>   https://codereview.chromium.org/1209283004/
> 
> And find it works quite well for that purpose.

Done.

[Ryan Sleevi, 2015-07-17 21:29:00]

Primarily a "More comments" pass. I think we've got a good pattern emerging from
Nick & Eric to apply here for consistency.

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/200001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:60: std::string::iterator write_iter =
output->begin();
On 2015/07/16 04:42:32, mattm wrote:
> Ryan, do you have other opinions on this?

Yeah, I mean, I'm not terribly keen on "make a copy and clean it up" - it just
seems unnecessary from a memory point of view, and doesn't buy anything from a
performance point of view (indeed, may harm it w/r/t cache lines, since in the
rewrite scenario, we can just hit the cache lines and queue the flushes)

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:200: memcpy(vector_as_array(&in_32bit),
in.UnsafeData(), in.Length());
What if in.Length() == 0? I can't remember what ISO C guarantees for 0 lengths.

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:279: using RdnVector =
std::vector<base::Tuple<der::Input, der::Tag, der::Input>>;
Should we just treat this as a struct with named members? It seems like that'd
be totally legal here.

In particular, I found lines 327-328 fairly hard to read to understand whether
or not there's typos or anything in place (note only 4 meaningful characters
different in those two lines, and it required care to make sure numbers weren't
typo'd)

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:281: WARN_UNUSED_RESULT bool
ReadRdn(der::Parser* parser, RdnVector* out) {
Document

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:286: // Read the attribute type, which
must be OBJECT IDENTIFIERs.
Why the plural?

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:320: // small, a naive linear search for
each element should be fine.
Since it tripped up Eric, maybe it's worth expanding a comment here about the
'hostile' case and why it's a non-issue (I still think it's a non issue)

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:359: // Must have at least one RDN.
I think for this, we do want to substantially expand on the comments about the
data structures in play here.

For example, see Eric's comment style in
https://codereview.chromium.org/1218753002/diff/470001/net/cert/internal/sign...
, which more thoroughly details things.

I guess here, this is a tricky thing because of Section 4.1.2.6 of RFC 5280,
compared with Section 4.1.2.4.

That is, when matching a Subject to an Issuer name, this requirement holds - it
MUST be a non-empty sequence. However, when matching a Subject to a Subject, if
the cert itself is not a CA cert, it's perfectly valid to have an empty DN and a
critical SAN (and in fact, at least one major wireless vendor does exactly this
for their captive portal)

So I don't think we want to do this check here in the name matching, and it may
need to be surfaced up to when we do Subject/Issuer comparisons, versus
name-equality comparisons.

https://codereview.chromium.org/1125333005/diff/280001/net/der/input.h
File net/der/input.h (right):

https://codereview.chromium.org/1125333005/diff/280001/net/der/input.h#newcode65
net/der/input.h:65: void CopyToString(std::string* output) const;
I think this would need to be UnsafeDataAsString() [or something to that
effect], because the risk of someone doing further pointer math on this class.

Also, see https://www.chromium.org/developers/coding-style#Method_Signatures re:
std::string* output

[mattm, 2015-07-21 00:17:47]

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:200: memcpy(vector_as_array(&in_32bit),
in.UnsafeData(), in.Length());
On 2015/07/17 21:28:59, Ryan Sleevi wrote:
> What if in.Length() == 0? I can't remember what ISO C guarantees for 0
lengths.

Oh, yeah. Zero lengths are okay, but the pointers still need to be valid. I've
added a check.

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:279: using RdnVector =
std::vector<base::Tuple<der::Input, der::Tag, der::Input>>;
On 2015/07/17 21:28:59, Ryan Sleevi wrote:
> Should we just treat this as a struct with named members? It seems like that'd
> be totally legal here.
> 
> In particular, I found lines 327-328 fairly hard to read to understand whether
> or not there's typos or anything in place (note only 4 meaningful characters
> different in those two lines, and it required care to make sure numbers
weren't
> typo'd)

Done. Also changed the loop variables to just "a" and "b" since having
foo_type_and_value repeated so many times just made things hard to parse.

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:281: WARN_UNUSED_RESULT bool
ReadRdn(der::Parser* parser, RdnVector* out) {
On 2015/07/17 21:29:00, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:286: // Read the attribute type, which
must be OBJECT IDENTIFIERs.
On 2015/07/17 21:29:00, Ryan Sleevi wrote:
> Why the plural?

fixed

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:320: // small, a naive linear search for
each element should be fine.
On 2015/07/17 21:28:59, Ryan Sleevi wrote:
> Since it tripped up Eric, maybe it's worth expanding a comment here about the
> 'hostile' case and why it's a non-issue (I still think it's a non issue)

Done.

https://codereview.chromium.org/1125333005/diff/280001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:359: // Must have at least one RDN.
On 2015/07/17 21:29:00, Ryan Sleevi wrote:
> I think for this, we do want to substantially expand on the comments about the
> data structures in play here.
> 
> For example, see Eric's comment style in
>
https://codereview.chromium.org/1218753002/diff/470001/net/cert/internal/sign...
> , which more thoroughly details things.

done.

> I guess here, this is a tricky thing because of Section 4.1.2.6 of RFC 5280,
> compared with Section 4.1.2.4.
> 
> That is, when matching a Subject to an Issuer name, this requirement holds -
it
> MUST be a non-empty sequence. However, when matching a Subject to a Subject,
if
> the cert itself is not a CA cert, it's perfectly valid to have an empty DN and
a
> critical SAN (and in fact, at least one major wireless vendor does exactly
this
> for their captive portal)
> 
> So I don't think we want to do this check here in the name matching, and it
may
> need to be surfaced up to when we do Subject/Issuer comparisons, versus
> name-equality comparisons.

removed.

https://codereview.chromium.org/1125333005/diff/280001/net/der/input.h
File net/der/input.h (right):

https://codereview.chromium.org/1125333005/diff/280001/net/der/input.h#newcode65
net/der/input.h:65: void CopyToString(std::string* output) const;
On 2015/07/17 21:29:00, Ryan Sleevi wrote:
> I think this would need to be UnsafeDataAsString() [or something to that
> effect], because the risk of someone doing further pointer math on this class.
> 
> Also, see https://www.chromium.org/developers/coding-style#Method_Signatures
re:
> std::string* output

Changed to AsString(). I don't understand the Unsafe part, creating a string
makes a copy, so it shouldn't matter what happens with this class afterwards?

[Ryan Sleevi, 2015-07-30 02:30:12]

https://codereview.chromium.org/1125333005/diff/280001/net/der/input.h
File net/der/input.h (right):

https://codereview.chromium.org/1125333005/diff/280001/net/der/input.h#newcode65
net/der/input.h:65: void CopyToString(std::string* output) const;
On 2015/07/21 00:17:47, mattm wrote:
> Changed to AsString(). I don't understand the Unsafe part, creating a string
> makes a copy, so it shouldn't matter what happens with this class afterwards?

Mostly so that no one futz's around with the data. I guess this is 'less unsafe'
than UnsafeData, in that we have the actual length as well. Oh well, caveat
coder.

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:41: // However, because it is optional
for implementations and because
s/optional for implementations/optional for 2459/3280 implementations/ ?

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:88: c == '?'))
Probably worth a comment about

// See NormalizePrintableString for the acceptable list of characters.

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:98: *(write_iter++) = c;
Just documenting a review note:

I had wondered if this might make sense as

if (write_iter != read_iter)
  *(write_iter++) = c;
else
  ++write_iter

So that we avoid mutating for already normalized strings. However, in the end, I
suspect it'll be worse performance, since we'd be adding in an extra switch, and
the overall goal (don't touch the string if it's already normalized) wouldn't
really be met, because 'virtually' every certificate includes some
capitalization in the directory string.

It still might be worth it for other cases (e.g. PrintableString, IA5String),
which would typically contain domain names, but it's hard to convince myself
that it's justified, and harder still to ask you to run the perf numbers.

Just documenting for future thoughts.

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:210:
base::WriteUnicodeCharacter(codepoint, output);
Hrm, we may want to rope in brettw@ on this, since he's been on a //base cleanup
spree, and crbug.com/79631 says this shouldn't be exposed (as does the header),
in favour of the string16 version. Also, holy crap is the performance seemingly
rather awful (or at least, frequent std::string resizes - every character).

Anyways, AFAICT, this is 'correct' from the //base API.

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:396: bool VerifyNameMatch(const
der::Input& a, const der::Input& b) {
I suspect as this matures (but no need in this CL) we'll want to be able to
pre-compute normalized values.

For example, during chain building, we may want to memoize normalization of the
subject cert before comparing against (different) issuers, and may want to
memoize on an issuer match.

That is, put differently, I suspect we'll want to have a structure we can
(temporarily) store for normalized comparisons. It might be as efficient as just
a linear encoded vector (say, re-DER encoding everything), or it might be a C++
structure where we store either a der::Input (for non-normalizable values) or an
std::string (normalized form)

I'm not so much worried about the design, other than making note of it as it
comes to use. Maybe it's worth a TODO, but probably not even that.

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:414: // assumed to have verified those
conditions.
Because of this, does it make sense to expect the caller to pass in
a_rdn_sequence / b_rdn_sequence? Since they will have at least need to have
called the ReadSequence() on a_parser/b_parser (to get the internal value).

Alternatively, we could expose a VerifyNameMatch(..., ..., ALLOW_EMPTY_NAMES),
and put that check here?

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:431: if (a_rdn_sequence.HasMore() ||
b_rdn_sequence.HasMore())
On an efficiency note, I'm wondering whether or not we should count the # of
elements in a_rdn_sequence/b_rdn_sequence before beginning normalized
comparisons, since that'll be a clear sign of name mismatch.

der::Input a_tmp(a_rdn_sequence);
der::Input b_tmp(b_rdn_sequence);

while (a_rdn_sequence.HasMore() && b_rdn_sequence.HasMore()) {
  if (!a_rdn_sequence.SkipTag(der::kSet) ||
      !b_rdn_sequence.SkipTag(der::kSet)) {
    return false;
  }
}
if (a_rdn_sequence.HasMore() || b_rdn_sequence.HasMore())
  return false;
a_rdn_sequence = a_tmp;
b_rdn_sequence = b_tmp;
while (a_rdn_sequence.HasMore() && b_rdn_sequence.HasMore()) {
  der::Parser a_rdn, b_rdn;
  if (!a_rdn_sequence.ReadConstructed(der::kSet, &a_rdn) ||
      !b_rdn_sequence.ReadConstructed(der::kSet, &b_rdn)) {
    return false;
  }
  if (!VerifyRdnMatch(&a_rdn, &b_rdn))
    return false;
}
return true;

[mattm, 2015-08-04 04:59:29]

Patchset #17 (id:330074) has been deleted

[mattm, 2015-08-04 04:59:38]

Patchset #16 (id:320002) has been deleted

[mattm, 2015-08-04 04:59:47]

Patchset #15 (id:320001) has been deleted

[mattm, 2015-08-04 05:02:04]

mattm@chromium.org changed reviewers:
+ brettw@chromium.org

[mattm, 2015-08-04 05:02:06]

[+brettw for base/strings/utf_string_conversion_utils.h]

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:41: // However, because it is optional
for implementations and because
On 2015/07/30 02:30:12, Ryan Sleevi wrote:
> s/optional for implementations/optional for 2459/3280 implementations/ ?

Done.

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:88: c == '?'))
On 2015/07/30 02:30:12, Ryan Sleevi wrote:
> Probably worth a comment about
> 
> // See NormalizePrintableString for the acceptable list of characters.

Done.

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:98: *(write_iter++) = c;
On 2015/07/30 02:30:12, Ryan Sleevi wrote:
> Just documenting a review note:
> 
> I had wondered if this might make sense as
> 
> if (write_iter != read_iter)
>   *(write_iter++) = c;
> else
>   ++write_iter
> 
> So that we avoid mutating for already normalized strings. However, in the end,
I
> suspect it'll be worse performance, since we'd be adding in an extra switch,
and
> the overall goal (don't touch the string if it's already normalized) wouldn't
> really be met, because 'virtually' every certificate includes some
> capitalization in the directory string.
> 
> It still might be worth it for other cases (e.g. PrintableString, IA5String),
> which would typically contain domain names, but it's hard to convince myself
> that it's justified, and harder still to ask you to run the perf numbers.
> 
> Just documenting for future thoughts.

Acknowledged. I was also assuming that it's faster to avoid the extra
conditional.

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:210:
base::WriteUnicodeCharacter(codepoint, output);
On 2015/07/30 02:30:12, Ryan Sleevi wrote:
> Hrm, we may want to rope in brettw@ on this, since he's been on a //base
cleanup
> spree, and crbug.com/79631 says this shouldn't be exposed (as does the
header),
> in favour of the string16 version. Also, holy crap is the performance
seemingly
> rather awful (or at least, frequent std::string resizes - every character).
> 
> Anyways, AFAICT, this is 'correct' from the //base API.

I just pinged Brett about it. He was okay with using it.

As for performance, I did a quick test. At least with the unittest data, it
seemed to be slightly faster, or at least no slower, than using ICU
(base::CodepageToUTF16) + UTF16ToUTF8. It could be worse with very long strings,
perhaps.

Hopefully UniversalString is not used very much anyway.

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:396: bool VerifyNameMatch(const
der::Input& a, const der::Input& b) {
On 2015/07/30 02:30:12, Ryan Sleevi wrote:
> I suspect as this matures (but no need in this CL) we'll want to be able to
> pre-compute normalized values.
> 
> For example, during chain building, we may want to memoize normalization of
the
> subject cert before comparing against (different) issuers, and may want to
> memoize on an issuer match.
> 
> That is, put differently, I suspect we'll want to have a structure we can
> (temporarily) store for normalized comparisons. It might be as efficient as
just
> a linear encoded vector (say, re-DER encoding everything), or it might be a
C++
> structure where we store either a der::Input (for non-normalizable values) or
an
> std::string (normalized form)
> 
> I'm not so much worried about the design, other than making note of it as it
> comes to use. Maybe it's worth a TODO, but probably not even that.

Acknowledged.

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:414: // assumed to have verified those
conditions.
On 2015/07/30 02:30:12, Ryan Sleevi wrote:
> Because of this, does it make sense to expect the caller to pass in
> a_rdn_sequence / b_rdn_sequence? Since they will have at least need to have
> called the ReadSequence() on a_parser/b_parser (to get the internal value).

Yeah, I had actually made that change in the followup basic constraints CL
anyway, since a directoryName in GeneralName has a context specific tag instead
of a universal tag. I'll move that change up to this CL.

> 
> Alternatively, we could expose a VerifyNameMatch(..., ..., ALLOW_EMPTY_NAMES),
> and put that check here?

https://codereview.chromium.org/1125333005/diff/300001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:431: if (a_rdn_sequence.HasMore() ||
b_rdn_sequence.HasMore())
On 2015/07/30 02:30:12, Ryan Sleevi wrote:
> On an efficiency note, I'm wondering whether or not we should count the # of
> elements in a_rdn_sequence/b_rdn_sequence before beginning normalized
> comparisons, since that'll be a clear sign of name mismatch.

Seems plausible. I've changed it, though I don't have a good test of whether it
helps. With the unittest it didn't seem to help (maybe even slightly slower),
but that may not be very representative of real world usage.

> der::Input a_tmp(a_rdn_sequence);
> der::Input b_tmp(b_rdn_sequence);
> 
> while (a_rdn_sequence.HasMore() && b_rdn_sequence.HasMore()) {
>   if (!a_rdn_sequence.SkipTag(der::kSet) ||
>       !b_rdn_sequence.SkipTag(der::kSet)) {
>     return false;
>   }
> }
> if (a_rdn_sequence.HasMore() || b_rdn_sequence.HasMore())
>   return false;
> a_rdn_sequence = a_tmp;
> b_rdn_sequence = b_tmp;
> while (a_rdn_sequence.HasMore() && b_rdn_sequence.HasMore()) {
>   der::Parser a_rdn, b_rdn;
>   if (!a_rdn_sequence.ReadConstructed(der::kSet, &a_rdn) ||
>       !b_rdn_sequence.ReadConstructed(der::kSet, &b_rdn)) {
>     return false;
>   }
>   if (!VerifyRdnMatch(&a_rdn, &b_rdn))
>     return false;
> }
> return true;

https://codereview.chromium.org/1125333005/diff/360001/net/cert/internal/veri...
File net/cert/internal/verify_name_match_unittest.cc (right):

https://codereview.chromium.org/1125333005/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_name_match_unittest.cc:26: ADD_FAILURE();
I know this isn't the best way.. but doing an AssertionResult + output param
everywhere this function is used would be pretty ugly and these shouldn't really
fail randomly.

[Ryan Sleevi, 2015-08-07 23:27:04]

Diff between Patchset 14 and Patchset 16 LGTM

You may wish to ping Brett directly if you're waiting for his review :)

https://codereview.chromium.org/1125333005/diff/380001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/380001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:131: output->assign(in.AsString());
performance nit:

in.AsString().swap(*output) avoids an extra copy.

[brettw, 2015-08-08 16:53:51]

base lgtm

[mattm, 2015-08-10 20:23:00]

The CQ bit was checked by mattm@chromium.org

[mattm, 2015-08-10 20:23:00]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1125333005/#ps420001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-10 20:23:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1125333005/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1125333005/420001

[commit-bot: I haz the power, 2015-08-10 21:38:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-10 21:38:51]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[mattm, 2015-08-10 22:57:56]

The CQ bit was checked by mattm@chromium.org

[mattm, 2015-08-10 22:57:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1125333005/#ps460001
(title: "ios copy_test_data")

[commit-bot: I haz the power, 2015-08-10 22:58:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1125333005/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1125333005/460001

[commit-bot: I haz the power, 2015-08-11 00:26:34]

Committed patchset #20 (id:460001)

[commit-bot: I haz the power, 2015-08-11 00:27:06]

Patchset 20 (id:??) landed as
https://crrev.com/057b015555a4af92d9cf5108f9d2a7f4581378f3
Cr-Commit-Position: refs/heads/master@{#342750}

[eroman, 2015-09-09 00:01:11]

https://codereview.chromium.org/1125333005/diff/460001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:362: if (a_type_and_values.size() !=
b_type_and_values.size())
If we are relying on a size check as part of our equality comparison, shouldn't
we also check for duplicates?

For instance given these two RDNS:

rdn1:
  Attribute1=Value1
  Attribute1=Value1

rdn2:
  Attribute1=Value1
  Attribute2=Value2

Then

VerifyRdnMatch(rdn1, rdn2) == true

However going in the other direction:

VerifyRdnMatch(rdn2, rdn1) == false


... That doesn't seem right...

[mattm, 2015-09-09 01:25:52]

https://codereview.chromium.org/1125333005/diff/460001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.cc (right):

https://codereview.chromium.org/1125333005/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:362: if (a_type_and_values.size() !=
b_type_and_values.size())
On 2015/09/09 00:01:11, eroman wrote:
> If we are relying on a size check as part of our equality comparison,
shouldn't
> we also check for duplicates?
> 
> For instance given these two RDNS:
> 
> rdn1:
>   Attribute1=Value1
>   Attribute1=Value1
> 
> rdn2:
>   Attribute1=Value1
>   Attribute2=Value2
> 
> Then
> 
> VerifyRdnMatch(rdn1, rdn2) == true
> 
> However going in the other direction:
> 
> VerifyRdnMatch(rdn2, rdn1) == false
> 
> 
> ... That doesn't seem right...

Yeah, good point. I'll send out a CL to fix that.