Class for parsing and evaluating RFC 5280 NameConstraints.

net/cert/internal/name_constraints.h

Index: net/cert/internal/name_constraints.h
diff --git a/net/cert/internal/name_constraints.h b/net/cert/internal/name_constraints.h
new file mode 100644
index 0000000000000000000000000000000000000000..cd3bba3bb9ddbe6e20a6a9786465110cf7731336
--- /dev/null
+++ b/net/cert/internal/name_constraints.h
@@ -0,0 +1,128 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_CERT_INTERNAL_NAME_CONSTRAINTS_H_
+#define NET_CERT_INTERNAL_NAME_CONSTRAINTS_H_
+
+#include <vector>
+
+#include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
+#include "net/base/ip_address_number.h"
+
+namespace net {
+
+namespace der {
+class Input;
+}  // namespace der
+
+enum GeneralNameTypes {

[Ryan Sleevi, 2015/09/24 22:06:19]

Document / reference where these names come from, and whether or not the IDs are
significant.

[mattm, 2015/09/30 04:52:31]

Done.

+  GENERAL_NAME_OTHER_NAME = 1 << 0,
+  GENERAL_NAME_RFC822_NAME = 1 << 1,
+  GENERAL_NAME_DNS_NAME = 1 << 2,
+  GENERAL_NAME_X400_ADDRESS = 1 << 3,
+  GENERAL_NAME_DIRECTORY_NAME = 1 << 4,
+  GENERAL_NAME_EDI_PARTY_NAME = 1 << 5,
+  GENERAL_NAME_UNIFORM_RESOURCE_IDENTIFIER = 1 << 6,
+  GENERAL_NAME_IP_ADDRESS = 1 << 7,
+  GENERAL_NAME_REGISTERED_ID = 1 << 8,
+};
+
+// Parses a NameConstraints extension value and allows testing whether names are
+// allowed under those constraints as defined by RFC 5280 section 4.2.1.10.
+class NET_EXPORT NameConstraints {
+ public:
+  // TODO(mattm): This may need to be split out into a public class, since
+  // GeneralNames is used other places in a certificate also...
+  struct GeneralNames {

[Ryan Sleevi, 2015/09/24 22:06:19]

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.

[mattm, 2015/09/30 04:52:31]

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.

+    GeneralNames();
+    ~GeneralNames();
+
+    // ASCII hostnames.

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

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.

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

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.)

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

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:07]

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

+    std::vector<std::string> dns_names;
+
+    // DER-encoded Name values (not including the Sequence tag).
+    std::vector<std::vector<uint8_t>> directory_names;

[Ryan Sleevi, 2015/09/24 22:06:19]

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)

[mattm, 2015/09/30 04:52:31]

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.)

+
+    // iPAddresses. For Subject Alternative Name this will be 4 bytes for IPv4
+    // or 16 bytes for IPv6. For Name Constraints, it will be ip + netmask
+    // (8 bytes for IPv4, 32 bytes for IPv6).

[Ryan Sleevi, 2015/09/24 22:06:19]

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.

[mattm, 2015/09/30 04:52:31]

done
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.

+    std::vector<std::vector<uint8_t>> ip_addresses;
+
+    // Which name types were present, as a bitfield of GeneralNameTypes.
+    // Includes both the supported and unsupported types (although unsupported
+    // ones may not be recorded depending on the context, like non-critical name
+    // constraints.)
+    int present_name_types = 0;
+  };
+
+  ~NameConstraints();
+
+  // Parses a DER-encoded NameConstraints extension and initializes this object.
+  // Should only be called once on a given NameConstraints object (whether
+  // successful or not).

[Ryan Sleevi, 2015/09/24 22:06:19]

This part of the comment isn't relevant, is it? Since this is a Create function.

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

Done.

+  // |extension_value| should be the extnValue from the extension (not including
+  // the OCTET STRING tag). |is_critical| should be true if the extension was
+  // marked critical.  Returns true if the extension was parsed successfully,

[Ryan Sleevi, 2015/09/24 22:06:19]

single space after sentences. This file is inconsistent, but //net primarily
does single space.

[mattm, 2015/09/30 04:52:31]

Done.

+  // after which the IsPermitted methods may be used to test if Names are
+  // permitted by the NameConstraints.

[Ryan Sleevi, 2015/09/24 22:06:19]

This comment seems out of date?

[mattm, 2015/09/30 04:52:31]

Done.

+  // The object lifetime is not bound to the lifetime of |extension_value| data.
+  static scoped_ptr<NameConstraints> CreateFromDer(
+      const der::Input& extension_value,
+      bool is_critical);
+
+  // Tests if a certificate is allowed by the name constraints.

[Ryan Sleevi, 2015/09/24 22:06:19]

This isn't really testing a certificate though, it's testing a name (or set of
names, really)

IsPermittedName(...) ?

[mattm, 2015/09/30 04:52:31]

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...

+  // |subject_rdn_sequence| should be the DER-encoded value of the subject's
+  // RDNSequence (not including Sequence tag), and may be an empty ASN.1
+  // sequence.  |subject_alt_name_extnvalue_tlv| should be the extnValue of the
+  // subjectAltName extension (including the OCTET STRING tag & length), or
+  // empty if the cert did not have a subjectAltName extension.  |is_leaf_cert|
+  // should be true if the certificate is the leaf of the certificate chain, in
+  // which case subject commonName hostname/ip checking is done.
+  bool IsPermittedCert(const der::Input& subject_rdn_sequence,
+                       const der::Input& subject_alt_name_extnvalue_tlv,
+                       bool is_leaf_cert) const;

[Ryan Sleevi, 2015/09/24 22:06:19]

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,
};

?

[mattm, 2015/09/30 04:52:31]

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.)

+
+  // Returns true if the ASCII hostname |name| is permitted.
+  // |name| may be a wildcard hostname (starts with "*."). Eg, "*.bar.com"
+  // would not be permitted if "bar.com" is permitted and "foo.bar.com" is
+  // excluded, while "*.baz.com" would only be permitted if "baz.com" is
+  // permitted.
+  bool IsPermittedDNSName(const std::string& name) const;
+
+  // Returns true if the directoryName |name_rdn_sequence| is permitted.
+  // |name_rdn_sequence| should be the DER-encoded RDNSequence value (not
+  // including the Sequence tag.)
+  bool IsPermittedDirectoryName(const der::Input& name_rdn_sequence) const;
+
+  // Returns true if the iPAddress |ip| is permitted.
+  bool IsPermittedIP(const IPAddressNumber& ip) const;
+
+  // Returns a bitfield of GeneralNameTypes of all the types constrained by this
+  // NameConstraints. Name types that aren't supported will only be present if
+  // the name constraint they appeared in was marked critical.
+  //
+  // RFC 5280 section 4.2.1.10 says:
+  // Applications conforming to this profile MUST be able to process name
+  // constraints that are imposed on the directoryName name form and SHOULD be
+  // able to process name constraints that are imposed on the rfc822Name,
+  // uniformResourceIdentifier, dNSName, and iPAddress name forms.
+  // 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.
+  int ConstrainedNameTypes() const;
+
+ private:
+  bool Parse(const der::Input& extension_value,
+             bool is_critical) WARN_UNUSED_RESULT;
+
+  GeneralNames permitted_subtrees_;
+  GeneralNames excluded_subtrees_;
+};
+
+}  // namespace net
+
+#endif  // NET_CERT_INTERNAL_NAME_CONSTRAINTS_H_