Add a function for parsing RFC 5280's "Certificate".

net/cert/internal/parse_certificate.h

Index: net/cert/internal/parse_certificate.h
diff --git a/net/cert/internal/parse_certificate.h b/net/cert/internal/parse_certificate.h
new file mode 100644
index 0000000000000000000000000000000000000000..d4968435826820e73b718d176e3ff3449a403744
--- /dev/null
+++ b/net/cert/internal/parse_certificate.h
@@ -0,0 +1,62 @@
+// 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_PARSE_CERTIFICATE_H_
+#define NET_CERT_INTERNAL_PARSE_CERTIFICATE_H_
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "net/base/net_export.h"
+#include "net/der/input.h"
+#include "net/der/parse_values.h"
+
+namespace net {
+
+struct ParsedCertificate;

[davidben, 2015/08/14 17:14:50]

Any reason for the forward-decl like this? You could also just put
ParsedCertificate first, right?

[eroman, 2015/08/14 17:41:00]

It was just an aesthetic decision (I can move things around if you like).

I found putting the functions first, and datastructures after that to be a bit
more readable.

ParsedTbsCertificate in particular takes up a lot of space. Mainly comments
discussing the fields. Whereas the methods felt like the first thing I wanted to
read in the file, and give more of an overview before the explosion of details.

[davidben, 2015/08/14 17:45:39]

Mmm, that's true. Do we typically do that? I don't actually know. Mostly I saw
that and my first thought was that it was a forward decl for a type defined in
another header, or that there was some annoying circular-dependency reasons.
Your call. I'm bad at having opinions on things. :-)

+
+// Parses a DER-encoded "Certificate" as specified by RFC 5280. Returns true on
+// success and sets the results in |out|.
+//
+// Refer to the per-field documention of the ParsedCertificate structure for
+// details on what validity checks parsing performs.
+//
+//       Certificate  ::=  SEQUENCE  {
+//            tbsCertificate       TBSCertificate,
+//            signatureAlgorithm   AlgorithmIdentifier,
+//            signatureValue       BIT STRING  }
+NET_EXPORT bool ParseCertificate(const der::Input& certificate_tlv,
+                                 ParsedCertificate* out) WARN_UNUSED_RESULT;

[davidben, 2015/08/14 17:45:39]

Oh transplanting my comment from the other CL since it's relevant. (Looking at
that one now.)


I don't have a good suggestion here, so maybe leave it as-is unless you have a
better idea:

It's a little weird that everything in parse_values.h parses the contents of a
TLV while this family of functions parses the whole element. (I get tripped up
when I compare the parsing code for [0] EXPLICIT Version and [1] IMPLICIT
UniqueIdentifier and see it's identical.)

[eroman, 2015/08/14 17:57:48]

I am not sure the best way to handle this either.

There is good reasons for the parse_values to take the value without the tag,
since the tag can vary. In this case the tag (SEQUENCE) is not something that
would vary.

Some possibilities:

  * Rename these functions so they include a TLV suffix. Giving suffix of tlv is
something I have been using elsewhere in this code to disambiguate

  * Express it through the type system. For instance, have a der::Sequence type,
or der::TagLengthValue. This gets a bit shaky though, and might limit the
ability to call these functions elsewhere without first reading values to a
TagLengthValue structure.

  * Take the input without the tag here and throughout. This leads back to
earlier discussion of those tradeoffs. I still fancy the idea that you express
things like "ConsumeCertificate()" rather than "ConsumeSequence();
ParseCertificate()".
I did some manual testing of the UniqueIdentifier parsing (manually constructed
since I couldn't find actual certs that used it).... I realize I lack unit-tests
for this (in the other CL), will add those in a follow-up!

+
+// ParsedCertificate contains pointers to the main fields of a DER-encoded RFC
+// 5280 "Certificate".
+//
+// ParsedCertificate is expected to be filled by ParseCertificate(), so
+// subsequent field descriptions are in terms of what ParseCertificate() sets.
+struct NET_EXPORT ParsedCertificate {
+  // Corresponds with "tbsCertificate" from RFC 5280:
+  //         tbsCertificate       TBSCertificate,
+  //
+  // This contains the full (unverified) Tag-Length-Value for a SEQUENCE. No
+  // guarantees are made regarding the value of this SEQUENCE.
+  der::Input tbs_certificate_tlv;
+
+  // Corresponds with "signatureAlgorithm" from RFC 5280:
+  //         signatureAlgorithm   AlgorithmIdentifier,
+  //
+  // This contains the full (unverified) Tag-Length-Value for a SEQUENCE. No
+  // guarantees are made regarding the value of this SEQUENCE.
+  //
+  // This can be further parsed using SignatureValue::CreateFromDer().
+  der::Input signature_algorithm_tlv;
+
+  // Corresponds with "signatureValue" from RFC 5280:
+  //         signatureValue       BIT STRING  }
+  //
+  // Parsing guarantees that this is a valid BIT STRING.
+  der::BitString signature_value;
+};
+
+}  // namespace net
+
+#endif  // NET_CERT_INTERNAL_PARSE_CERTIFICATE_H_