Add parsing code for TBSCertificate's "validity" field.

Add parsing code for TBSCertificate's "validity" field.

BUG=410574
Committed: https://crrev.com/6101b9183d409f650d8e9498780214b8d2b028fa
Cr-Commit-Position: refs/heads/master@{#345720}

[eroman, 2015-08-10 18:57:57]

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

[eroman, 2015-08-10 18:57:58]

Still working on some failure tests, but otherwise ready for review

[eroman, 2015-08-14 17:11:15]

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

[eroman, 2015-08-14 17:11:15]

+mattm as possible reviewer (this one is pretty straight forward)

[davidben, 2015-08-14 18:23:10]

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:160: der::Input validity_tlv;
Since it's just two fields, I wonder if you should just put not_before and
not_after fields directly into ParsedCertificate. (I dunno what's the criteria
for deciding what does or doesn't get parsed in each pass and probably missed
some design discussions.)

[eroman, 2015-08-14 18:37:04]

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:160: der::Input validity_tlv;
On 2015/08/14 18:23:10, David Benjamin wrote:
> Since it's just two fields, I wonder if you should just put not_before and
> not_after fields directly into ParsedCertificate. (I dunno what's the criteria
> for deciding what does or doesn't get parsed in each pass and probably missed
> some design discussions.)

I discuss the design overview in this message:
https://codereview.chromium.org/1279963003/#msg2

My rough guideline was that any non-SEQUENCE of Certificate/TBSCertificate would
be parsed fully, and the SEQUENCEs would be deferred to a subsequent parsing
call.

The thinking behind this was primarily to simplify an incremental
implementation, but also with a secondary consideration for performance
(consumers can pick out the pieces they want without having to fully parse
stuff).

In this case it may be better to just parse Validity up-front; I can go ahead
and do that. The only real downside (for me the coder) is it involves needing to
update all the pseudo-TBSCertificate test data :)

[mattm, 2015-08-18 01:11:40]

It lgtm. I don't have strong feelings about keeping it separate or not, this
seems reasonable to me though. I'll defer to David.

[davidben, 2015-08-18 17:24:27]

lgtm. I think this field is small enough that it makes sense to inline into
ParsedCertificate, but I don't have as much context and will defer to you all.

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:147: // wild may.
Nit: This probably can be word-wrapped more loosely.

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:147: // wild may.
[If we don't have example certs, where did this behavior come from? Some other
implementation I'm guessing?]

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
File net/cert/internal/parse_certificate_unittest.cc (right):

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
net/cert/internal/parse_certificate_unittest.cc:315: // however no time will
fall in this range.
Nit: This looks like a wrapped funny.

[eroman, 2015-08-18 17:34:51]

>I think this field is small enough that it makes sense to inline into
ParsedCertificate

Sounds reasonable I will give that a try (and rework the test data accordingly).

I think I will still expose the helper publicly for the sake of unit-testing it
separately.

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1285593003/diff/80001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:147: // wild may.
On 2015/08/18 17:24:27, David Benjamin wrote:
> [If we don't have example certs, where did this behavior come from? Some other
> implementation I'm guessing?]

I am not sure, I will ask Nick about the history.

Also see the email thread titled "Has anyone written parsing for
TBSCertificate.validity?" where I raise the issue about whether "relaxed" flavor
is needed.

[eroman, 2015-08-19 00:22:24]

OK, I inlined the validity fields into ParsedTbsCertificate as discussed.

  * Updated the test data accordingly

  * I also changed SUBJECT while at it, since it previously used 0A in an OCTET
STRING, and openssl ends up printing a newline when hex dumping which bothered
me.

  * Using strict time parsing rather than the Relaxed version. Added a
corresponding test, using the CT cert that fails.

[mattm, 2015-08-21 21:51:05]

lgtm

https://codereview.chromium.org/1285593003/diff/120001/net/data/parse_certifi...
File net/data/parse_certificate_unittest/tbs_v3_extensions.pem (left):

https://codereview.chromium.org/1285593003/diff/120001/net/data/parse_certifi...
net/data/parse_certificate_unittest/tbs_v3_extensions.pem:4: extensions)
does contain extensions?

[eroman, 2015-08-21 23:20:14]

https://codereview.chromium.org/1285593003/diff/120001/net/data/parse_certifi...
File net/data/parse_certificate_unittest/tbs_v3_extensions.pem (left):

https://codereview.chromium.org/1285593003/diff/120001/net/data/parse_certifi...
net/data/parse_certificate_unittest/tbs_v3_extensions.pem:4: extensions)
On 2015/08/21 21:51:05, mattm wrote:
> does contain extensions?

Indeed, the comment is wrong. Should say the opposite (that it contains
extensions)

[eroman, 2015-08-26 22:01:24]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-08-26 22:01:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/1285593003/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-26 22:02:47]

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

[commit-bot: I haz the power, 2015-08-26 23:27:51]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-08-26 23:28:32]

Patchset 8 (id:??) landed as
https://crrev.com/6101b9183d409f650d8e9498780214b8d2b028fa
Cr-Commit-Position: refs/heads/master@{#345720}