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

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

BUG=410574
Committed: https://crrev.com/58423c13595e87949813b6f86a2f0f8ffd116740
Cr-Commit-Position: refs/heads/master@{#343486}

[eroman, 2015-08-13 00:29:03]

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

[eroman, 2015-08-13 00:29:03]

(I split out the Certificate parsing from the TBSCertificate + Certificate
parsing CL I had sent you, and made it a separate review. This makes it clearer
to read which tests are being done for each, as they are now separated)

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

lgtm. Sorry I apparently forgot to publish the review here.

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:16: WARN_UNUSED_RESULT bool
IsSequenceTLV(const der::Input& input) {
[Oh. You'd convinced me in the other CL that it's fine to leave that to the
caller. I have no complaints about actually checking either though. Your call.]

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:16: struct ParsedCertificate;
Any reason for the forward-decl like this? You could also just put
ParsedCertificate first, right?

https://codereview.chromium.org/1288193003/diff/100001/net/data/parse_certifi...
File net/data/parse_certificate_unittest/cert_algorithm_not_sequence.pem
(right):

https://codereview.chromium.org/1288193003/diff/100001/net/data/parse_certifi...
net/data/parse_certificate_unittest/cert_algorithm_not_sequence.pem:1: This is a
synthesized Certificate, where the signature algorithm is an INTEGER rather than
a SEQUENCE.
Nit: Perhaps wrap these lines?

[eroman, 2015-08-14 17:41:01]

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:16: WARN_UNUSED_RESULT bool
IsSequenceTLV(const der::Input& input) {
On 2015/08/14 17:14:50, David Benjamin wrote:
> [Oh. You'd convinced me in the other CL that it's fine to leave that to the
> caller. I have no complaints about actually checking either though. Your
call.]

Since the tests rely on this now, I will go ahead and keep it :)

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:16: struct ParsedCertificate;
On 2015/08/14 17:14:50, David Benjamin wrote:
> Any reason for the forward-decl like this? You could also just put
> ParsedCertificate first, right?

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.

https://codereview.chromium.org/1288193003/diff/100001/net/data/parse_certifi...
File net/data/parse_certificate_unittest/cert_algorithm_not_sequence.pem
(right):

https://codereview.chromium.org/1288193003/diff/100001/net/data/parse_certifi...
net/data/parse_certificate_unittest/cert_algorithm_not_sequence.pem:1: This is a
synthesized Certificate, where the signature algorithm is an INTEGER rather than
a SEQUENCE.
On 2015/08/14 17:14:50, David Benjamin wrote:
> Nit: Perhaps wrap these lines?

Done for all the test files.

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

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:16: struct ParsedCertificate;
On 2015/08/14 17:41:00, eroman wrote:
> On 2015/08/14 17:14:50, David Benjamin wrote:
> > Any reason for the forward-decl like this? You could also just put
> > ParsedCertificate first, right?
> 
> 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.

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

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:29: ParsedCertificate* out)
WARN_UNUSED_RESULT;
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.)

[davidben, 2015-08-14 17:50:42]

https://codereview.chromium.org/1288193003/diff/140001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1288193003/diff/140001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:19: // success and sets the results in
|out|.
Oh! One more thing that occurred to me that's actually pretty important to
document:

The resulting ParsedCertificate is valid as long as |certificate_tlv|'s contents
are.

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

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:29: ParsedCertificate* out)
WARN_UNUSED_RESULT;
On 2015/08/14 17:45:39, David Benjamin wrote:
> 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:

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

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

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!

[eroman, 2015-08-14 18:03:07]

Good point. Added this comment (which if you like I will mimic for the other
functions):

// On failure |out| has an undefined state. Some of its fields may have been
// updated during parsing, whereas others were not changed.

[davidben, 2015-08-14 18:06:28]

On 2015/08/14 17:57:48, eroman wrote:
>
https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
> File net/cert/internal/parse_certificate.h (right):
> 
>
https://codereview.chromium.org/1288193003/diff/100001/net/cert/internal/pars...
> net/cert/internal/parse_certificate.h:29: ParsedCertificate* out)
> WARN_UNUSED_RESULT;
> On 2015/08/14 17:45:39, David Benjamin wrote:
> > 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:
> 
> 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.

I suppose there's nothing stopping someone from doing:

  [0] IMPLICIT Certificate

It'd just be stupid. Blargh implicit tagging...

> 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

Maybe. Though there is something to be said for making the common case
unadorned.

I wonder if we should stick a Contents suffix to everything in parse_values?
Though that's also wordy... :-/

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

Arguably that would also help with the "we parse the TLVs twice" problem, but I
think der::Input is nice and simple and it'd be nice not to lose that.

>   * 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()".

Hrm. I must have missed or glossed over that discussion. The idea with Consume
being that it would let you parse out a SEQUENCE OF Certificate and stuff like
that? What we ended up doing for BoringSSL was to have the CBS* functions peel
elements off and then we'd have a const uint8_t* + size_t wrapper that only let
you parse a single thing. But I wasn't very happy with that. It was just the
best I could come up with. And we have the added constraint that we have to
support the Consume style of API for every type that needs a d2i compatibility
function.


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

[davidben, 2015-08-14 18:07:48]

On 2015/08/14 18:03:07, eroman wrote:
> Good point. Added this comment (which if you like I will mimic for the other
> functions):
> 
> // On failure |out| has an undefined state. Some of its fields may have been
> // updated during parsing, whereas others were not changed.

Oh, that might be worth mentioning too. I actually meant that you ought to
document that ParsedCertificate, no success, can't outlive the original
der::Input since it aliases bits of it rather than copy.

[davidben, 2015-08-14 18:07:57]

On 2015/08/14 18:07:48, David Benjamin wrote:
> On 2015/08/14 18:03:07, eroman wrote:
> > Good point. Added this comment (which if you like I will mimic for the other
> > functions):
> > 
> > // On failure |out| has an undefined state. Some of its fields may have been
> > // updated during parsing, whereas others were not changed.
> 
> Oh, that might be worth mentioning too. I actually meant that you ought to
> document that ParsedCertificate, no success, can't outlive the original
> der::Input since it aliases bits of it rather than copy.

s/no success/on success/

[eroman, 2015-08-14 18:23:03]

OK new comment block reads:

// Note that on success |out| aliases data from the input |certificate_tlv|.
// Hence the fields of the ParsedCertificate are only valid as long as
// |certificate_tlv| remains valid.
//
// On failure |out| has an undefined state. Some of its fields may have been
// updated during parsing, whereas others were not changed.

Regarding the discussion about the TLV vs contents: I will keep thinking about
that and come up with some proposals to improve this. However I plan to land
as-is initially, and iterate on it as necessary. (Whatever we decide on, makes
sense to holistically apply that pattern to both this and the existing code)

[davidben, 2015-08-14 18:26:35]

Still lgtm, to be explicit.

On 2015/08/14 18:23:03, eroman wrote:
> OK new comment block reads:
> 
> // Note that on success |out| aliases data from the input |certificate_tlv|.
> // Hence the fields of the ParsedCertificate are only valid as long as
> // |certificate_tlv| remains valid.
> //
> // On failure |out| has an undefined state. Some of its fields may have been
> // updated during parsing, whereas others were not changed.
> 
> Regarding the discussion about the TLV vs contents: I will keep thinking about
> that and come up with some proposals to improve this. However I plan to land
> as-is initially, and iterate on it as necessary. (Whatever we decide on, makes
> sense to holistically apply that pattern to both this and the existing code)

Sounds good. Yeah, while the current situation is mildly weird, it's not the end
of the world. No one's every going to have an implicitly tagged Certificate. (I
think I currently favor a Contents or Body suffix, but haven't thought about
this very much.)

[eroman, 2015-08-14 18:38:10]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2015-08-14 18:38:47]

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

[commit-bot: I haz the power, 2015-08-14 20:47:43]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-08-14 20:48:18]

Patchset 11 (id:??) landed as
https://crrev.com/58423c13595e87949813b6f86a2f0f8ffd116740
Cr-Commit-Position: refs/heads/master@{#343486}