Class for parsing and evaluating RFC 5280 NameConstraints.

Class for parsing and evaluating RFC 5280 NameConstraints.

BUG=410574
Committed: https://crrev.com/2c59d26f10b36166777a685e1e29da75aa7a4060
Cr-Commit-Position: refs/heads/master@{#356773}

[mattm, 2015-07-01 03:26:31]

mattm@chromium.org changed reviewers:
+ davidben@chromium.org, eroman@chromium.org, nharper@chromium.org,
rsleevi@chromium.org

[mattm, 2015-07-01 03:26:32]

There's still some open questions in the implementation and some minor extra
test cases I want to add, but I think it's clean enough to send out for initial
thoughts, and wanted to get it sent out while there's still a day left before
the holiday.

Note: depends on https://codereview.chromium.org/1125333005/
It updates verify_name_match to add a VerifyNameInSubtree function.

Also, had to make some minor changes to the interface of VerifyNameMatch in
1125333005 since when a Name is in a GeneralName the Sequence is implicit.
Should I merge that change back into 1125333005?

[mattm, 2015-07-16 23:57:08]

Patchset #3 (id:40001) has been deleted

[mattm, 2015-08-12 04:18:02]

This is ready for review now.

[mattm, 2015-08-12 20:20:43]

https://codereview.chromium.org/1214933009/diff/180001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/180001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:220: // zero? (0 is the default, so it
should not be present in DER encoding..)
By the way, I tested this in the CT db, no certs had a name constraint with
minimum specified, and 1 had a maximum set (on a directoryName constraint, and
marked critical).  So at least going by that, it wouldn't seem necessary to
implement either of these TODOs.

[eroman, 2015-08-26 19:56:45]

First pass, didn't review the logic in detail yet.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:31: enum WildcardMatchType {
WILDCARD_PARTIAL_MATCH, WILDCARD_FULL_MATCH };
nit: newline after this

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:110: };
nit: newline

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:111: // Parse a GeneralName value and add
it to |subtrees|.
nit: Parse --> Parses ?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:126: int klass = tag &
~der::kTagClassMask;
klass ?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:131: if (on_unsupported_types !=
IGNORE_UNSUPPORTED)
Rather than check this in every branch, can you generalize it?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:165:
subtrees->ip_addresses.push_back(std::vector<uint8_t>(
Should the ip be validated?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:166: value.UnsafeData(),
value.UnsafeData() + value.Length()));
der::Input() has a ToString() method. Maybe this generalizes as a CopyTo()
method

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:181: WARN_UNUSED_RESULT bool
ParseGeneralSubtrees(
nit: Why WARN_UNUSED_RESULT here but not for the other parsing functions?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:183: NameConstraints::GeneralNames*
subtrees,
nit: Should the output parameter be last?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:230: : has_other_names(false),
nit: These might be easier to write as member initializers in the header file.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:273: if
(!ParseGeneralSubtrees(excluded_subtrees_value, &excluded_subtrees_,
note: The assumption is that Parse() is only called once, since not all members
are re-set by parsing (for example, excluded_subtrees_).

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:321: if (san_names.has_other_names &&
!IsPermittedOtherName())
What is IsPermittedOtherName() testing here, that it isn't already present?

What if there was instead the type (rfc822_name, x500_address, etc) was
expressed by an enum, and a set of them was expressed by a bitfield.

Then this if dispatch and the earlier one in the switch are simplified.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
File net/cert/internal/name_constraints.h (right):

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:11: #include "net/der/input.h"
nit: can you forward-declare der::Input instead?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:19: // TODO: make this private? (requires
making some currently anonymous
TODO: --> TODO(mattm):

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:21: // out into a public class, since
GeneralNames is used other places in a
I would say not to make it private for the reasons you describe (I also prefer
being able to use anonymous functions). Can break it out later as the need
arises.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:30: // DER encoded Name values (not
including the Sequence tag).
See also https://code.google.com/p/chromium/issues/detail?id=522235. Not sure
what the best approach is short of comments right now...

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:36: std::vector<std::vector<uint8_t>>
ip_addresses;
Should we go a step further and parse this into an IPAddressNumber + mask?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:49: // Parse a DER-encoded NameConstraints
extension. |extension_value| should be
This comment isnt' really clear that this in fact initializes the
NameConstraints object. Also nit Parse --> Parses

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:50: // the extnValue octet string from the
extension, |is_critical| should be true
Unclear if this is the content, or the full TLV of the octet string.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:53: // The object lifetime is not bound to
the lifetime of |extension_value| data.
Out of curiosity what made you choose this model? What are the tradeoffs?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:59: // sequence.  |subject_alt_name| should
be the extnValue of the subjectAltName
Is this the OCTET STRING's value, or the full TLV? If just the value, how is
emptiness disambiguated from the not specified case?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:70: // considered would not be permitted if
"bar.com" is permitted and
is considered would not be --> would not be permitted

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:75: // Returns true if the directoryName
|name| is permitted.
what does |name| refer to?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
File net/cert/internal/name_constraints_unittest.cc (right):

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints_unittest.cc:14: der::Input
SequenceValueFromString(const std::string* s) {
This is the same function used in verify_name_match_unittest.cc

Does it make sense to move it to test_utils ?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints_unittest.cc:35: {token.c_str(), result},
This reads funny. Can you either change the |toke| parameter to be a const char*
(all the callers are literals), or modify PemBlockMapping to take a std::string
?

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints_unittest.cc:66: ::testing::AssertionResult
CheckNotPresent(
nit: newline

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints_unittest.cc:236: TEST_P(ParseNameConstraints,
DNSNames_ExcludeOnly) {
Underscores in Gtest names are discouraged/banned:

https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_...

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints_unittest.cc:272:
ASSERT_TRUE(LoadTestNameConstraint("directoryname", &constraints_der));
nit: I find it more readable to include the ".pem" in here, so it is a complete
filename and you can for instance code search it unambiguously.

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

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:410: };
newline

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:513: return std::string();
Why return an empty string rather than return false to indicate an error? Is the
consumer expected to test for emptiness, or are empty names allowed? I would say
here we want to fail, since failures can mean invalid der

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.h (right):

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/veri...
net/cert/internal/verify_name_match.h:24: // Compare |name_rdn_sequence| and
|parent_rdn_sequence| and return true if
Compare --> Compares

https://codereview.chromium.org/1214933009/diff/210001/net/data/name_constrai...
File net/data/name_constraints_unittest/name_constraints/directoryname.pem
(right):

https://codereview.chromium.org/1214933009/diff/210001/net/data/name_constrai...
net/data/name_constraints_unittest/name_constraints/directoryname.pem:1: 0:d=0 
hl=3 l= 172 cons: SEQUENCE
nit: The path of these data files
"net/data/name_constraints_unittest/name_constraints/" seems like it could just
directly be "net/data/name_constraints_unittest/". You separate out the scripts,
but my suggestion would be to just dump them into one directory

[mattm, 2015-08-29 01:37:19]

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:31: enum WildcardMatchType {
WILDCARD_PARTIAL_MATCH, WILDCARD_FULL_MATCH };
On 2015/08/26 19:56:43, eroman wrote:
> nit: newline after this

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:110: };
On 2015/08/26 19:56:43, eroman wrote:
> nit: newline

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:111: // Parse a GeneralName value and add
it to |subtrees|.
On 2015/08/26 19:56:43, eroman wrote:
> nit: Parse --> Parses ?

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:126: int klass = tag &
~der::kTagClassMask;
On 2015/08/26 19:56:43, eroman wrote:
> klass ?

Since class is a reserved word. But I've changed it to tag_class.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:131: if (on_unsupported_types !=
IGNORE_UNSUPPORTED)
On 2015/08/26 19:56:43, eroman wrote:
> Rather than check this in every branch, can you generalize it?

It's not checked in every branch, only the branches for unsupported types. 
I've changed it to use a bitfield and only check in one place now.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:165:
subtrees->ip_addresses.push_back(std::vector<uint8_t>(
On 2015/08/26 19:56:44, eroman wrote:
> Should the ip be validated?

Validating the ip would require a bit of extra work since the format varies
depending on the context (could be a plain ip, or an ip+netmask).

There's the question of whether this is the right place to do the validation.
For one thing, if a name of that type is in the constraints, but never appears
in any certs in the chain, then the validation would be unnecessary (as the
comment on the function says).

Also the other blocks don't validate the contents, and it wouldn't make sense to
validate only some of the types. (eg directory names require a lot of parsing to
validate. Maybe it would actually make sense to pre-parse/validate/normalize, or
memoize, or something. But right now the pre-normalizing for Names is just a
todo to consider later.)

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:181: WARN_UNUSED_RESULT bool
ParseGeneralSubtrees(
On 2015/08/26 19:56:43, eroman wrote:
> nit: Why WARN_UNUSED_RESULT here but not for the other parsing functions?

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:183: NameConstraints::GeneralNames*
subtrees,
On 2015/08/26 19:56:43, eroman wrote:
> nit: Should the output parameter be last?

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:230: : has_other_names(false),
On 2015/08/26 19:56:44, eroman wrote:
> nit: These might be easier to write as member initializers in the header file.

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:273: if
(!ParseGeneralSubtrees(excluded_subtrees_value, &excluded_subtrees_,
On 2015/08/26 19:56:43, eroman wrote:
> note: The assumption is that Parse() is only called once, since not all
members
> are re-set by parsing (for example, excluded_subtrees_).

noted in Parse method comment.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:321: if (san_names.has_other_names &&
!IsPermittedOtherName())
On 2015/08/26 19:56:43, eroman wrote:
> What is IsPermittedOtherName() testing here, that it isn't already present?

This is testing that if a SubjectAlternativeName contains an unsupported type,
and the NameConstraints (if it was marked critical) also contained that type,
then the cert is not permitted.

"If a name constraints extension that is marked as critical
   imposes constraints on a particular name form, and an instance of
   that name form appears in the subject field or subjectAltName
   extension of a subsequent certificate, then the application MUST
   either process the constraint or reject the certificate."

(That's in the comment in the header, maybe it should be repeated here ..)


> What if there was instead the type (rfc822_name, x500_address, etc) was
> expressed by an enum, and a set of them was expressed by a bitfield.
> 
> Then this if dispatch and the earlier one in the switch are simplified.

Yeah, doing this by bitfield might be nicer, changed.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
File net/cert/internal/name_constraints.h (right):

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:11: #include "net/der/input.h"
On 2015/08/26 19:56:44, eroman wrote:
> nit: can you forward-declare der::Input instead?

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:19: // TODO: make this private? (requires
making some currently anonymous
On 2015/08/26 19:56:44, eroman wrote:
> TODO: --> TODO(mattm):

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:21: // out into a public class, since
GeneralNames is used other places in a
On 2015/08/26 19:56:44, eroman wrote:
> I would say not to make it private for the reasons you describe (I also prefer
> being able to use anonymous functions). Can break it out later as the need
> arises.

Acknowledged.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:30: // DER encoded Name values (not
including the Sequence tag).
On 2015/08/26 19:56:44, eroman wrote:
> See also https://code.google.com/p/chromium/issues/detail?id=522235. Not sure
> what the best approach is short of comments right now...

Acknowledged.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:36: std::vector<std::vector<uint8_t>>
ip_addresses;
On 2015/08/26 19:56:44, eroman wrote:
> Should we go a step further and parse this into an IPAddressNumber + mask?

I guess it could, but just the complication again due to sometimes it being only
an IP and sometimes being an ip+netmask.. dunno.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:49: // Parse a DER-encoded NameConstraints
extension. |extension_value| should be
On 2015/08/26 19:56:44, eroman wrote:
> This comment isnt' really clear that this in fact initializes the
> NameConstraints object. Also nit Parse --> Parses

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:50: // the extnValue octet string from the
extension, |is_critical| should be true
On 2015/08/26 19:56:44, eroman wrote:
> Unclear if this is the content, or the full TLV of the octet string.

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:53: // The object lifetime is not bound to
the lifetime of |extension_value| data.
On 2015/08/26 19:56:44, eroman wrote:
> Out of curiosity what made you choose this model? What are the tradeoffs?

Mainly that it just seemed safer and easier to understand, since name
constraints are something that will be used for multiple levels of checking the
cert chain, and (later) will have constraints merged together from multiple
certificates in the chain, could become confusing if a NameConstraints contained
pointers into N different cert blobs.

On the other hand, the chain validation is recursive so in practice maybe it
wouldn't be a problem to just say "this NameConstraints object depends upon the
lifetime of cert foo and possibly any of the certs in higher levels of the
verification".

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:59: // sequence.  |subject_alt_name| should
be the extnValue of the subjectAltName
On 2015/08/26 19:56:44, eroman wrote:
> Is this the OCTET STRING's value, or the full TLV? If just the value, how is
> emptiness disambiguated from the not specified case?

Just the value. You're correct there could be some ambiguity there.(Although the
subjectAltName is not supposed to be empty, can't error check it here because of
the ambiguity.)

I changed it to take the TLV and added a test of empty sequence.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:70: // considered would not be permitted if
"bar.com" is permitted and
On 2015/08/26 19:56:44, eroman wrote:
> is considered would not be --> would not be permitted

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints.h:75: // Returns true if the directoryName
|name| is permitted.
On 2015/08/26 19:56:44, eroman wrote:
> what does |name| refer to?

fixed.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
File net/cert/internal/name_constraints_unittest.cc (right):

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints_unittest.cc:14: der::Input
SequenceValueFromString(const std::string* s) {
On 2015/08/26 19:56:44, eroman wrote:
> This is the same function used in verify_name_match_unittest.cc
> 
> Does it make sense to move it to test_utils ?

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints_unittest.cc:35: {token.c_str(), result},
On 2015/08/26 19:56:44, eroman wrote:
> This reads funny. Can you either change the |toke| parameter to be a const
char*
> (all the callers are literals), or modify PemBlockMapping to take a
std::string
> ?

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints_unittest.cc:66: ::testing::AssertionResult
CheckNotPresent(
On 2015/08/26 19:56:44, eroman wrote:
> nit: newline

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints_unittest.cc:236: TEST_P(ParseNameConstraints,
DNSNames_ExcludeOnly) {
On 2015/08/26 19:56:44, eroman wrote:
> Underscores in Gtest names are discouraged/banned:
> 
>
https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_...

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/name...
net/cert/internal/name_constraints_unittest.cc:272:
ASSERT_TRUE(LoadTestNameConstraint("directoryname", &constraints_der));
On 2015/08/26 19:56:44, eroman wrote:
> nit: I find it more readable to include the ".pem" in here, so it is a
complete
> filename and you can for instance code search it unambiguously.

Done.

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

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:410: };
On 2015/08/26 19:56:44, eroman wrote:
> newline

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/veri...
net/cert/internal/verify_name_match.cc:513: return std::string();
On 2015/08/26 19:56:44, eroman wrote:
> Why return an empty string rather than return false to indicate an error? Is
the
> consumer expected to test for emptiness, or are empty names allowed? I would
say
> here we want to fail, since failures can mean invalid der

It just didn't matter much for this case, but I've changed it to return a bool.

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.h (right):

https://codereview.chromium.org/1214933009/diff/210001/net/cert/internal/veri...
net/cert/internal/verify_name_match.h:24: // Compare |name_rdn_sequence| and
|parent_rdn_sequence| and return true if
On 2015/08/26 19:56:44, eroman wrote:
> Compare --> Compares

Done.

https://codereview.chromium.org/1214933009/diff/210001/net/data/name_constrai...
File net/data/name_constraints_unittest/name_constraints/directoryname.pem
(right):

https://codereview.chromium.org/1214933009/diff/210001/net/data/name_constrai...
net/data/name_constraints_unittest/name_constraints/directoryname.pem:1: 0:d=0 
hl=3 l= 172 cons: SEQUENCE
On 2015/08/26 19:56:44, eroman wrote:
> nit: The path of these data files
> "net/data/name_constraints_unittest/name_constraints/" seems like it could
just
> directly be "net/data/name_constraints_unittest/". You separate out the
scripts,
> but my suggestion would be to just dump them into one directory

Done.

[eroman, 2015-09-10 17:48:28]

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:17: const int kSupportedNameTypes =
GENERAL_NAME_DNS_NAME |
Can you add a comment explaining what this represents (supported by

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:21: enum WildcardMatchType {
WILDCARD_PARTIAL_MATCH, WILDCARD_FULL_MATCH };
Please add comments for the meaning of these.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:23: // Returns true if |name| falls in the
subtree defined by |name_space|.
Did you mean raw_name / raw_name_space?

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:35: // Wildcard handling is not specified
by RFC 5280, but since certificate
What do you mean by "certificate verification allows it" ? Is this a
compatibility thing, or specified elsewhere?

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:40: base::StringPiece name(raw_name);
Why not make the input parameter a StringPiece?

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:42: // Normalize absolute DNS names by
removing the trailing dot.
What normative requirement is this? Can you add a reference?

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:48: // Everything matches the empty name
space.
In the case where the namespace was simply '.', shouldn't that result in no
matches?

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:54: if (wildcard_matching ==
WILDCARD_PARTIAL_MATCH && name.size() > 2 &&
Under what circumstances is the searched for name containing wildcard?

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:73: if (name.size() >= name_space.size() +
2 &&
Why the check the check of size() +2 rather than size() +1 ? Is it because you
consider |name_| starting with a period to be invalid? (but we don't do other
validity checks on the input hostname, nor verify that it is ascii)

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:76: // Trailing text matches, but not in a
subtree (e.g., "foobar.com" is not a
Does name constraints not have a way to match one and only one host, rather than
always including subdomains? (I guess it could be expressed by excluding the
subdomains in a separate rule).

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:94: if (ip.size() != kIPv4AddressSize &&
ip.size() != kIPv6AddressSize)
Should this include a NOTREACHED()? This looks like an input error, but I see
why you are checking for it.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:117: // Parses a GeneralName value and add
it to |subtrees|.
nit: add it --> adds it?

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:118: // The GeneralName values are not
validated here, since failing on invalid names
Unclear why this is desirable. If it is malformed shouldn't we reject the cert?

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:147:
subtrees->dns_names.push_back(value.AsString());
Won't this mean that we accept certs with malformed DER? The encoding for
IA5String is not explicitly checked anywhere (just implicitly from
case-insensitive ASCII comparisons)

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:226: // TODO(mattm): Technically we don't
need to fail here: rather we only need
This sounds like a fine approach to me. (However will be more useful when we
have specific errors to explain the parsing failures).

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:229: // TODO(mattm): should this allow for
the case that minimum is present but
I would say no, since that is invalid DER

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:234: return true;
This means an empty GeneralSubTrees sequence will be accepted, which strictly
speaking is not compliant with the given ASN.1. Is this expected?

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:263: return false;
Note: My interpretation of multi-line if statements requiring braces was that
multiple lines in the "if" also counted. Not sure any change is needed here, but
myself and davidben (based on past review feedback) would have put curlies
around this (and others in this CL)

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
File net/cert/internal/name_constraints.h (right):

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.h:44: // DER encoded Name values (not
including the Sequence tag).
"DER-encoded" to match the rest of the comments

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.h:70: bool Parse(const der::Input&
extension_value,
When I was writing the SignatureAlgorithm class which similarly had a "parse to
initialize" mechanism, Ryan proposed using a factory method instead
(https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/internal/...)
to avoid possibility of having an invalid algorithm.

Not a big deal here since the default constructed NameConstraints isn't scary,
but you might consider that pattern here for consistency. It does cost another
allocation, but makes the "only called once" behavior explicit.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.h:75: // RDNSequence field (not including
Sequence tag), and may be an empty ASN.1
nit: This reads a bit weird to me. In particular "RDNSequence field", since my
mind was expecting a field name (name) and not a type (RDNSequence).

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.h:86: // |name| may be a wildcard hostname
(starts with "*."). Eg, "*.bar.com"
Where does this wildcard behavior come from?

[mattm, 2015-09-22 22:12:32]

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:17: const int kSupportedNameTypes =
GENERAL_NAME_DNS_NAME |
On 2015/09/10 17:48:28, eroman wrote:
> Can you add a comment explaining what this represents (supported by 

Done.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:21: enum WildcardMatchType {
WILDCARD_PARTIAL_MATCH, WILDCARD_FULL_MATCH };
On 2015/09/10 17:48:27, eroman wrote:
> Please add comments for the meaning of these.

They're described in the comment for DNSNameMatches, which is the only place
they're used. I guess I could move the description here instead, but I'm not
sure that's better.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:23: // Returns true if |name| falls in the
subtree defined by |name_space|.
On 2015/09/10 17:48:28, eroman wrote:
> Did you mean raw_name / raw_name_space?

Fixed. (raw_name)

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:35: // Wildcard handling is not specified
by RFC 5280, but since certificate
On 2015/09/10 17:48:28, eroman wrote:
> What do you mean by "certificate verification allows it" ? Is this a
> compatibility thing, or specified elsewhere?

Meant that Chrome's verifier allows it, so it's important to match the same
behavior here.
(https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_cert...)

It's described in http://tools.ietf.org/html/rfc6125#section-6.4.3

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:40: base::StringPiece name(raw_name);
On 2015/09/10 17:48:28, eroman wrote:
> Why not make the input parameter a StringPiece?

Hmm, dunno why it ended up like that. Done.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:42: // Normalize absolute DNS names by
removing the trailing dot.
On 2015/09/10 17:48:27, eroman wrote:
> What normative requirement is this? Can you add a reference?

It's another thing to match the behavior of VerifyHostname, so mentioned that.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:48: // Everything matches the empty name
space.
On 2015/09/10 17:48:28, eroman wrote:
> In the case where the namespace was simply '.', shouldn't that result in no
> matches?

I guess so. Changed.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:54: if (wildcard_matching ==
WILDCARD_PARTIAL_MATCH && name.size() > 2 &&
On 2015/09/10 17:48:28, eroman wrote:
> Under what circumstances is the searched for name containing wildcard?

When the end-entity cert has a CommonName or dNSName which specifies that the
certificate is for some wildcard hostname.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:73: if (name.size() >= name_space.size() +
2 &&
On 2015/09/10 17:48:27, eroman wrote:
> Why the check the check of size() +2 rather than size() +1 ? Is it because you
> consider |name_| starting with a period to be invalid? (but we don't do other
> validity checks on the input hostname, nor verify that it is ascii)

Yes, I suppose it is a bit extraneous to be testing that when not doing any
other validity checks on the hostname.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:76: // Trailing text matches, but not in a
subtree (e.g., "foobar.com" is not a
On 2015/09/10 17:48:27, eroman wrote:
> Does name constraints not have a way to match one and only one host, rather
than
> always including subdomains?
Correct

> (I guess it could be expressed by excluding the
> subdomains in a separate rule).

There aren't wildcards in the constraints, so you could only exclude certain
subdomains..

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:94: if (ip.size() != kIPv4AddressSize &&
ip.size() != kIPv6AddressSize)
On 2015/09/10 17:48:28, eroman wrote:
> Should this include a NOTREACHED()? This looks like an input error, but I see
> why you are checking for it.

It was operating on network-supplied data, so it shouldn't NOTREACHED.
But I changed it to do the check in ParseGeneralName after all, so I'll put a
DCHECK here.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:117: // Parses a GeneralName value and add
it to |subtrees|.
On 2015/09/10 17:48:28, eroman wrote:
> nit: add it --> adds it?

Done.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:118: // The GeneralName values are not
validated here, since failing on invalid names
On 2015/09/10 17:48:27, eroman wrote:
> Unclear why this is desirable. If it is malformed shouldn't we reject the
cert?

Done (except for directoryName, checking that would be a natural consequence of
pre-normalizing once I implement that).

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:147:
subtrees->dns_names.push_back(value.AsString());
On 2015/09/10 17:48:28, eroman wrote:
> Won't this mean that we accept certs with malformed DER? The encoding for
> IA5String is not explicitly checked anywhere (just implicitly from
> case-insensitive ASCII comparisons)

That's true. Added a check.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:226: // TODO(mattm): Technically we don't
need to fail here: rather we only need
On 2015/09/10 17:48:28, eroman wrote:
> This sounds like a fine approach to me. (However will be more useful when we
> have specific errors to explain the parsing failures).

I've reworded it from a todo to a note.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:229: // TODO(mattm): should this allow for
the case that minimum is present but
On 2015/09/10 17:48:28, eroman wrote:
> I would say no, since that is invalid DER

Yeah, I've removed the TODO.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:234: return true;
On 2015/09/10 17:48:28, eroman wrote:
> This means an empty GeneralSubTrees sequence will be accepted, which strictly
> speaking is not compliant with the given ASN.1. Is this expected?

Nope, fixed

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:263: return false;
On 2015/09/10 17:48:28, eroman wrote:
> Note: My interpretation of multi-line if statements requiring braces was that
> multiple lines in the "if" also counted. Not sure any change is needed here,
but
> myself and davidben (based on past review feedback) would have put curlies
> around this (and others in this CL)

Done.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
File net/cert/internal/name_constraints.h (right):

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.h:44: // DER encoded Name values (not
including the Sequence tag).
On 2015/09/10 17:48:28, eroman wrote:
> "DER-encoded" to match the rest of the comments

Done.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.h:70: bool Parse(const der::Input&
extension_value,
On 2015/09/10 17:48:28, eroman wrote:
> When I was writing the SignatureAlgorithm class which similarly had a "parse
to
> initialize" mechanism, Ryan proposed using a factory method instead
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/internal/...)
> to avoid possibility of having an invalid algorithm.
> 
> Not a big deal here since the default constructed NameConstraints isn't scary,
> but you might consider that pattern here for consistency. It does cost another
> allocation, but makes the "only called once" behavior explicit.

Done.

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.h:75: // RDNSequence field (not including
Sequence tag), and may be an empty ASN.1
On 2015/09/10 17:48:28, eroman wrote:
> nit: This reads a bit weird to me. In particular "RDNSequence field", since my
> mind was expecting a field name (name) and not a type (RDNSequence).

I removed the "field"

https://codereview.chromium.org/1214933009/diff/250001/net/cert/internal/name...
net/cert/internal/name_constraints.h:86: // |name| may be a wildcard hostname
(starts with "*."). Eg, "*.bar.com"
On 2015/09/10 17:48:28, eroman wrote:
> Where does this wildcard behavior come from?

(Should be addressed by the discussion in the .cc file review)

[Ryan Sleevi, 2015-09-24 22:06:20]

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:29: enum WildcardMatchType {
WILDCARD_PARTIAL_MATCH, WILDCARD_FULL_MATCH };
Document

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:31: // Returns true if |name| falls in the
subtree defined by |name_space|.
From reading this comment alone, I'm confused as to what |name_space| represents
(and why is it two words, rather than namespace?)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:53: // certificate verification behavior.
Documentation bug: You shouldn't be talking about "Chrome's certificate
validation code" in //net.

Simply "Normalize absolute DNS names by removing the trailing dot, if any" is
sufficient.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:63: size_t name_space_dot_pos =
name_space.find('.');
If there is no wildcard match, what's supposed to happen then? It's unclear
whether 71 -> 72 should have a return false, or, if not, why it's acceptable to
fall through.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:106: if (ip_constraint.size() != ip.size()
* 2)
No need for the DCHECK on lines 103-104 then, especially with this being
'attacker' supplied data (from the cert)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:118: }
We have net::IPNumberMatchesPrefix() and net::MaskPrefixLength [although I have
no idea while we have the latter]

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:125: };
Document

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:126: enum ParseGeneralNameIPAddressType {
newline

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:130: 
Document

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:134:
ParseGeneralNameUnsupportedTypeBehavior on_unsupported_types,
naming: "on_unsupported_types" doesn't seem like it fits our naming
pattern/idiom.

I'm not sure if it helps readability over a bool, given the lengths.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:142: if ((tag & der::kTagClassMask) !=
der::kTagContextSpecific)
Another place where a helper function might help (IsContextSpecific) ?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:144: int tag_class = tag &
~der::kTagClassMask;
BUG: Why are you using a signed-type, when der::Tag is uint8_t?

BUG 2: Given that a der::Tag could conceivably occupy multiple bytes, it
suggests that perhaps in the same way we have net::der::IsConstructed, we might
have net::der::GetTagClass(der::Tag tag) that returns the tag class?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:145: int name_type = 0;
Is int the right type, since you're using the enum?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:149: case 0 + der::kTagConstructed:
? This is a bit surprising/confusing. COuld you explain?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:304: if
(!ParseGeneralSubtrees(permitted_subtrees_value, is_critical,
Why not combine this conditional with the one on 303?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:318: if
(!ParseGeneralSubtrees(excluded_subtrees_value, is_critical,
Same here - why not combine with the conditional on 317?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:449: // X509Certificate::VerifyHostname),
otherwise interpret as a dNSName.
This seems tightly coupled in the comment; same layering concerns here.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:454: if (!IsPermittedDNSName(common_name))
why not combine this with 453 to be an else-if?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:473: if
(permitted_subtrees_.dns_names.empty() &&
It doesn't hurt to be extra verbose in the comments here.

// If there are no name constraints for DNS names, all names are accepted.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:479: if (DNSNameMatches(name,
excluded_name, WILDCARD_PARTIAL_MATCH))
Comments explaining nuance? :)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:483: if (DNSNameMatches(name,
permitted_name, WILDCARD_FULL_MATCH))
Ditto

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
File net/cert/internal/name_constraints.h (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:20: enum GeneralNameTypes {
Document / reference where these names come from, and whether or not the IDs are
significant.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:38: struct GeneralNames {
DESIGN: Why is this a plural field?

That is, why return GeneralName(S), itself comprised of vectors of specific name
types, as opposed to a vector of GeneralName, each with a singular value?

Is this a space optimization? A performance optimization? It's unclear from the
header alone.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:42: // ASCII hostnames.
Pedantry: In a name constraint, it isn't a hostname, since it may be of the form
".foo.com", which is not a valid hostname.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:46: std::vector<std::vector<uint8_t>>
directory_names;
Despite the MUST, I'm curious whether or not it's relevant to support these.
I'll note that Microsoft has changed their implementation like three times, so
I'm sure this is bugged.

(Just a general remark)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:50: // (8 bytes for IPv4, 32 bytes for
IPv6).
pedantry: "ipAddresses" is not a complete sentence.

From a design standpoint, it does seem confusing to have a field with four
possible sizes all mixed together.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:64: // successful or not).
This part of the comment isn't relevant, is it? Since this is a Create function.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:67: // marked critical.  Returns true if
the extension was parsed successfully,
single space after sentences. This file is inconsistent, but //net primarily
does single space.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:69: // permitted by the NameConstraints.
This comment seems out of date?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:75: // Tests if a certificate is allowed by
the name constraints.
This isn't really testing a certificate though, it's testing a name (or set of
names, really)

IsPermittedName(...) ?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:85: bool is_leaf_cert) const;
The design of this seems predicated on whether or not the SAN has a dNSName that
would constrain the common name, right? (Or, I suppose, IPAddress to constrain
the commonName)

Does it make more sense to decouple this into simply

IsPermittedName(const der::Input& subject_rdn_sequence,
                int common_name_flags);

where something like

enum CommonNameFlags {
  COMMON_NAME_UNCONSTRAINED      = 0,
  COMMON_NAME_CONSTRAINED_IF_IP  = 1 << 0,
  COMMON_NAME_CONSTRAINED_IF_DNS = 1 << 1,
};

?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
File net/cert/internal/test_helpers.cc (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
net/cert/internal/test_helpers.cc:46: if (parser.HasMore()) {
This surprises me; it doesn't seem part of the documented API contract, nor is
it clear why (in here) this is added.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
File net/cert/internal/test_helpers.h (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
net/cert/internal/test_helpers.h:36: // Parses |s| as a DER Sequence TLV and
returns a der::Input corresponding to
s/Sequence/SEQUENCE/

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
net/cert/internal/test_helpers.h:40: // Returned value's lifetime validity is
the same as for InputFromString.
Rather than back-reference to another function, it'd be helpful to be explicit
here.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
net/cert/internal/test_helpers.h:41: der::Input SequenceValueFromString(const
std::string* s);
It's not entirely clear to me why this is needed; could you explain?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.h (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/veri...
net/cert/internal/verify_name_match.h:27: // defined by RFC 5280 section 7.1. 
|name_rdn_sequence| and
single space

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/veri...
net/cert/internal/verify_name_match.h:36: // emailAddress attribute type.
Returns false if there was a parsing error.
If it doesn't, does it set |*constrained_email_address| to false? Or does it
return true with the value unmutated?

// Sets |*contained_email_address| to indicate whether or not
// |name_rdn_sequence| contains an emailAddress attribute type, and
// returns true. Returns false if |name_rdn_sequence| could not be
// parsed.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/veri...
net/cert/internal/verify_name_match.h:43: // was a parsing or normalizing error.
This makes me even more confused for symmetry with NameContainsEmailAddress -
now I wonder whether or not that function returns false if there was no
emailAddress.

[mattm, 2015-09-30 04:52:32]

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:29: enum WildcardMatchType {
WILDCARD_PARTIAL_MATCH, WILDCARD_FULL_MATCH };
On 2015/09/24 22:06:18, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:31: // Returns true if |name| falls in the
subtree defined by |name_space|.
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> From reading this comment alone, I'm confused as to what |name_space|
represents
> (and why is it two words, rather than namespace?)

renamed to dns_constraint

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:53: // certificate verification behavior.
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> Documentation bug: You shouldn't be talking about "Chrome's certificate
> validation code" in //net.
> 
> Simply "Normalize absolute DNS names by removing the trailing dot, if any" is
> sufficient.

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:63: size_t name_space_dot_pos =
name_space.find('.');
On 2015/09/24 22:06:18, Ryan Sleevi wrote:
> If there is no wildcard match, what's supposed to happen then? It's unclear
> whether 71 -> 72 should have a return false, or, if not, why it's acceptable
to
> fall through.

expanded on the comment.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:106: if (ip_constraint.size() != ip.size()
* 2)
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> No need for the DCHECK on lines 103-104 then, especially with this being
> 'attacker' supplied data (from the cert)

The intent was that this if condition is for not trying to do ipv4 vs ipv6
matches, whereas the DCHECK was for wrong lengths in general (and it's a dcheck
since ParseGeneralName should already have enforced it). I could still remove it
though since it should never be possible to hit..

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:118: }
On 2015/09/24 22:06:18, Ryan Sleevi wrote:
> We have net::IPNumberMatchesPrefix() and net::MaskPrefixLength [although I
have
> no idea while we have the latter]

Is there any guarantee the constraint netmask will be representable as a prefix
length as opposed to an arbitrary bit pattern?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:125: };
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:126: enum ParseGeneralNameIPAddressType {
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> newline

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:130: 
On 2015/09/24 22:06:18, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:134:
ParseGeneralNameUnsupportedTypeBehavior on_unsupported_types,
On 2015/09/24 22:06:18, Ryan Sleevi wrote:
> naming: "on_unsupported_types" doesn't seem like it fits our naming
> pattern/idiom.

renamed to "unsupported_type_behavior"

> I'm not sure if it helps readability over a bool, given the lengths.

It's more intended to help the readability of the call sites.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:142: if ((tag & der::kTagClassMask) !=
der::kTagContextSpecific)
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> Another place where a helper function might help (IsContextSpecific) ?

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:144: int tag_class = tag &
~der::kTagClassMask;
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> BUG: Why are you using a signed-type, when der::Tag is uint8_t?

fixed.

> BUG 2: Given that a der::Tag could conceivably occupy multiple bytes, it
> suggests that perhaps in the same way we have net::der::IsConstructed, we
might
> have net::der::GetTagClass(der::Tag tag) that returns the tag class?

Done (though I realized my naming was a bit weird. I called it GetTagNumber
instead.)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:145: int name_type = 0;
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> Is int the right type, since you're using the enum?

done (though still with present_name_types as an int, since otherwise requires
ugly casting)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:149: case 0 + der::kTagConstructed:
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> ? This is a bit surprising/confusing. COuld you explain?

Since I was only excluding the kTagClassMask (kTagContextSpecific |
kTagApplication), I get the tag number + the constructed tag for constructed
types. So the simple string types are just the tag number, while the structured
types have the constructed bit in addition to the implicit tag value.

But now I switched to using GetTagNumber, this is now changed. (Makes the switch
cleaner, but then I have to test for IsConstructed separately.)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:304: if
(!ParseGeneralSubtrees(permitted_subtrees_value, is_critical,
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> Why not combine this conditional with the one on 303?

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:318: if
(!ParseGeneralSubtrees(excluded_subtrees_value, is_critical,
On 2015/09/24 22:06:18, Ryan Sleevi wrote:
> Same here - why not combine with the conditional on 317?

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:449: // X509Certificate::VerifyHostname),
otherwise interpret as a dNSName.
On 2015/09/24 22:06:18, Ryan Sleevi wrote:
> This seems tightly coupled in the comment; same layering concerns here.

Well, I'd say that it is actually coupled, unfortunately. I removed the
reference to Chrome though. Referencing X509Certificate should be ok since
that's also in net?

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:454: if (!IsPermittedDNSName(common_name))
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> why not combine this with 453 to be an else-if?

I kinda liked how it made the IsPermittedDNSName line up with the IsPermittedIP
check. But it's not that important, changed.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:473: if
(permitted_subtrees_.dns_names.empty() &&
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> It doesn't hurt to be extra verbose in the comments here.
> 
> // If there are no name constraints for DNS names, all names are accepted.

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:479: if (DNSNameMatches(name,
excluded_name, WILDCARD_PARTIAL_MATCH))
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> Comments explaining nuance? :)

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:483: if (DNSNameMatches(name,
permitted_name, WILDCARD_FULL_MATCH))
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> Ditto

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
File net/cert/internal/name_constraints.h (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:20: enum GeneralNameTypes {
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> Document / reference where these names come from, and whether or not the IDs
are
> significant.

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:38: struct GeneralNames {
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> DESIGN: Why is this a plural field?
> 
> That is, why return GeneralName(S), itself comprised of vectors of specific
name
> types, as opposed to a vector of GeneralName, each with a singular value?
> 
> Is this a space optimization? A performance optimization? It's unclear from
the
> header alone.

Added a comment. It's mostly just an ease of use optimization since usually you
just want to operate on a certain type, and having to filter through of vector
of all the types would be inconvenient. This probably does have some minor
performance benefits too.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:42: // ASCII hostnames.
On 2015/09/24 22:06:20, Ryan Sleevi wrote:
> Pedantry: In a name constraint, it isn't a hostname, since it may be of the
form
> ".foo.com", which is not a valid hostname.

Is ".foo.com" valid in a dNSName constraint? That style is mentioned in URI
constraints, but not in dNSName (though, the dNSName section is pretty short &
ambiguous, so I can see how it could be read to allow it.)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:46: std::vector<std::vector<uint8_t>>
directory_names;
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> Despite the MUST, I'm curious whether or not it's relevant to support these.
> I'll note that Microsoft has changed their implementation like three times, so
> I'm sure this is bugged.
> 
> (Just a general remark)

In the CT database, 3/4ths of the name constraints had directoryName
constraints. (Second to dNSName which was in 4/5ths of name constraints.) I
didn't record how many of those were marked critical on a per-type basis though.
(Overall, 1/5th of name constraints were marked critical.)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:50: // (8 bytes for IPv4, 32 bytes for
IPv6).
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> pedantry: "ipAddresses" is not a complete sentence.

done

> From a design standpoint, it does seem confusing to have a field with four
> possible sizes all mixed together.

I guess so, but splitting them up by ipv4/ipv6 does lead to some expansion of
the code to loop over them or check if the field is empty.

Splitting by ip or ip+netmask wouldn't be bad, but I think would introduce the
risk of typoing the wrong variable.. unless completly splitting into separate
structs for regular GeneralNames vs GeneralNames in Constraints, but that would
introduce its own extra complications.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:64: // successful or not).
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> This part of the comment isn't relevant, is it? Since this is a Create
function.

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:67: // marked critical.  Returns true if
the extension was parsed successfully,
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> single space after sentences. This file is inconsistent, but //net primarily
> does single space.

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:69: // permitted by the NameConstraints.
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> This comment seems out of date?

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:75: // Tests if a certificate is allowed by
the name constraints.
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> This isn't really testing a certificate though, it's testing a name (or set of
> names, really)
> 
> IsPermittedName(...) ?

It doesn't take a certificate directly, but I'd say it's still specific to
checking a certificate, since it has the special rules for whether a certificate
is permitted (different rules if it's a leaf cert, different rules if names are
present in subject alt name or not, etc). The RFC always talks about checking
certificates in describing the rules...

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:85: bool is_leaf_cert) const;
On 2015/09/24 22:06:19, Ryan Sleevi wrote:
> The design of this seems predicated on whether or not the SAN has a dNSName
that
> would constrain the common name, right? (Or, I suppose, IPAddress to constrain
> the commonName)
> 
> Does it make more sense to decouple this into simply
> 
> IsPermittedName(const der::Input& subject_rdn_sequence,
>                 int common_name_flags);
> 
> where something like
> 
> enum CommonNameFlags {
>   COMMON_NAME_UNCONSTRAINED      = 0,
>   COMMON_NAME_CONSTRAINED_IF_IP  = 1 << 0,
>   COMMON_NAME_CONSTRAINED_IF_DNS = 1 << 1,
> };
> 
> ?

If I understand the question, yes it would be possible to split this into two
parts, one which checks the subjectAltName and one which checks the subject.
However that would mean moving some of the logic of the checks into the caller,
which seems worse (unless you just meant splitting it up internally but still
presenting a single method as the public interface).
(The enum would also need a COMMON_NAME_CONSTRAINED_IF_EMAIL_ADDRESS too.)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
File net/cert/internal/test_helpers.cc (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
net/cert/internal/test_helpers.cc:46: if (parser.HasMore()) {
On 2015/09/24 22:06:20, Ryan Sleevi wrote:
> This surprises me; it doesn't seem part of the documented API contract, nor is
> it clear why (in here) this is added.

Well, the contract is input is a "DER SEQUENCE tlv", so after skipping the tag
and length, there should only be the value of the sequence left, not any more
data after that.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
File net/cert/internal/test_helpers.h (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
net/cert/internal/test_helpers.h:36: // Parses |s| as a DER Sequence TLV and
returns a der::Input corresponding to
On 2015/09/24 22:06:20, Ryan Sleevi wrote:
> s/Sequence/SEQUENCE/

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
net/cert/internal/test_helpers.h:40: // Returned value's lifetime validity is
the same as for InputFromString.
On 2015/09/24 22:06:20, Ryan Sleevi wrote:
> Rather than back-reference to another function, it'd be helpful to be explicit
> here.

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/test...
net/cert/internal/test_helpers.h:41: der::Input SequenceValueFromString(const
std::string* s);
On 2015/09/24 22:06:20, Ryan Sleevi wrote:
> It's not entirely clear to me why this is needed; could you explain?

Depending on the context, Names can be implicitly tagged, or tagged normally as
a sequence. So the methods take only the value part. The test data is generated
as a normal sequence, so this helper is needed to extract the sequence value. We
could have the test data be just the sequence value, but there doesn't seem to
be a way with openssl asn1parse to do that directly so it'd require a bit of
extra hacking in the test data generation script.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/veri...
File net/cert/internal/verify_name_match.h (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/veri...
net/cert/internal/verify_name_match.h:27: // defined by RFC 5280 section 7.1. 
|name_rdn_sequence| and
On 2015/09/24 22:06:20, Ryan Sleevi wrote:
> single space

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/veri...
net/cert/internal/verify_name_match.h:36: // emailAddress attribute type.
Returns false if there was a parsing error.
On 2015/09/24 22:06:20, Ryan Sleevi wrote:
> If it doesn't, does it set |*constrained_email_address| to false? Or does it
> return true with the value unmutated?
> 
> // Sets |*contained_email_address| to indicate whether or not
> // |name_rdn_sequence| contains an emailAddress attribute type, and
> // returns true. Returns false if |name_rdn_sequence| could not be
> // parsed.

Reworded. Went with a larger change that reads more natural to me.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/veri...
net/cert/internal/verify_name_match.h:43: // was a parsing or normalizing error.
On 2015/09/24 22:06:20, Ryan Sleevi wrote:
> This makes me even more confused for symmetry with NameContainsEmailAddress -
> now I wonder whether or not that function returns false if there was no
> emailAddress.

modified to also have an explicit contained_common_name return.

[Ryan Sleevi, 2015-10-01 23:52:23]

First scan review to reply to comments. Going to do a more indepth review
tomorrow while NSS is compiling.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:118: }
On 2015/09/30 04:52:31, mattm wrote:
> Is there any guarantee the constraint netmask will be representable as a
prefix
> length as opposed to an arbitrary bit pattern?

Define guarantee ;)

RFC 1878 covers the v4 subnetting rules, and v6 always follows prefix rules. So
from a networking standpoint, "yes"

Now, in the gross and grotty world of CAs, I don't know if this is a field
people have invalidly encoded. However, if it fails to parse, you're free to
reject it :)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
File net/cert/internal/name_constraints.h (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:42: // ASCII hostnames.
On 2015/09/30 04:52:32, mattm wrote:
> On 2015/09/24 22:06:20, Ryan Sleevi wrote:
> > Pedantry: In a name constraint, it isn't a hostname, since it may be of the
> form
> > ".foo.com", which is not a valid hostname.
> 
> Is ".foo.com" valid in a dNSName constraint? That style is mentioned in URI
> constraints, but not in dNSName (though, the dNSName section is pretty short &
> ambiguous, so I can see how it could be read to allow it.)

It is not; I learned something new :)

Doesn't mean people don't use it, but it's not valid, and it's a no-op (since
you "add zero or more labels" to the hostname)

[mattm, 2015-10-06 21:55:08]

updated. (Also removed the commonName handling as discussed.)

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:118: }
On 2015/10/01 23:52:23, Ryan Sleevi (busy till 10-12) wrote:
> On 2015/09/30 04:52:31, mattm wrote:
> > Is there any guarantee the constraint netmask will be representable as a
> prefix
> > length as opposed to an arbitrary bit pattern?
> 
> Define guarantee ;)
> 
> RFC 1878 covers the v4 subnetting rules, and v6 always follows prefix rules.
So
> from a networking standpoint, "yes"
> 
> Now, in the gross and grotty world of CAs, I don't know if this is a field
> people have invalidly encoded. However, if it fails to parse, you're free to
> reject it :)

Done.

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
File net/cert/internal/name_constraints.h (right):

https://codereview.chromium.org/1214933009/diff/310001/net/cert/internal/name...
net/cert/internal/name_constraints.h:42: // ASCII hostnames.
On 2015/10/01 23:52:23, Ryan Sleevi (busy till 10-12) wrote:
> On 2015/09/30 04:52:32, mattm wrote:
> > On 2015/09/24 22:06:20, Ryan Sleevi wrote:
> > > Pedantry: In a name constraint, it isn't a hostname, since it may be of
the
> > form
> > > ".foo.com", which is not a valid hostname.
> > 
> > Is ".foo.com" valid in a dNSName constraint? That style is mentioned in URI
> > constraints, but not in dNSName (though, the dNSName section is pretty short
&
> > ambiguous, so I can see how it could be read to allow it.)
> 
> It is not; I learned something new :)
> 
> Doesn't mean people don't use it, but it's not valid, and it's a no-op (since
> you "add zero or more labels" to the hostname)

Ok, left as is but added a test of this.
(ParseNameConstraints.DNSNamesWithLeadingDot).

[eroman, 2015-10-22 00:27:05]

Are you waiting on any reviews from me on this?

[mattm, 2015-10-26 07:36:59]

On 2015/10/22 00:27:05, eroman wrote:
> Are you waiting on any reviews from me on this?

I think I'm just waiting on another look from Ryan.

[Ryan Sleevi, 2015-10-29 01:50:47]

lgtm

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:98: bool IsSuffixZero(std::vector<uint8_t>
mask, unsigned prefix_length) {
Shouldn't mask be const-ref?

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:484: // MUST be critical.
This code doesn't check the criticality; is the expectation that the validator
will do it?

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:501: // CN=*.bar.com should match a
constraint of foo.bar.com.
remove the CN= ?

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:508: // Eg, CN=*.bar.com should match a
constraint of bar.com, but not
remove CN= ?

[mattm, 2015-10-29 04:20:39]

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:98: bool IsSuffixZero(std::vector<uint8_t>
mask, unsigned prefix_length) {
On 2015/10/29 01:50:47, Ryan Sleevi wrote:
> Shouldn't mask be const-ref?

Indeed. Done.

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:484: // MUST be critical.
On 2015/10/29 01:50:47, Ryan Sleevi wrote:
> This code doesn't check the criticality; is the expectation that the validator
> will do it?

That was my thought (for example, if there are no name constraints this code
might not be called, but that condition should still be checked). I will expand
the comment to make it more clear.

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:501: // CN=*.bar.com should match a
constraint of foo.bar.com.
On 2015/10/29 01:50:47, Ryan Sleevi wrote:
> remove the CN= ?

Done.

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:508: // Eg, CN=*.bar.com should match a
constraint of bar.com, but not
On 2015/10/29 01:50:47, Ryan Sleevi wrote:
> remove CN= ?

Done.

[mattm, 2015-10-29 04:21:31]

The CQ bit was checked by mattm@chromium.org

[mattm, 2015-10-29 04:21:32]

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/1214933009/#ps430001
(title: "changes for comment #16")

[commit-bot: I haz the power, 2015-10-29 04:22:38]

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

[mattm, 2015-10-29 04:38:25]

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1214933009/diff/390001/net/cert/internal/name...
net/cert/internal/name_constraints.cc:484: // MUST be critical.
On 2015/10/29 04:20:38, mattm wrote:
> On 2015/10/29 01:50:47, Ryan Sleevi wrote:
> > This code doesn't check the criticality; is the expectation that the
validator
> > will do it?
> 
> That was my thought (for example, if there are no name constraints this code
> might not be called, but that condition should still be checked). I will
expand
> the comment to make it more clear.

Also I imagine this code will end up changed to split out the subject alt name
parsing and take the parsed form as input rather than the DER.

[commit-bot: I haz the power, 2015-10-29 05:33:28]

Committed patchset #22 (id:430001)

[commit-bot: I haz the power, 2015-10-29 05:34:12]

Patchset 22 (id:??) landed as
https://crrev.com/2c59d26f10b36166777a685e1e29da75aa7a4060
Cr-Commit-Position: refs/heads/master@{#356773}