X509CertificateBytes: Allow invalid serial numbers for now.

X509CertificateBytes: Allow invalid serial numbers for now.

BUG=721778
Review-Url: https://codereview.chromium.org/2881023003
Cr-Commit-Position: refs/heads/master@{#472040}
Committed: https://chromium.googlesource.com/chromium/src/+/312644de1876bf9eb520f4cfaef5f51eae1dfaf2

[mattm, 2017-05-15 22:23:31]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 22:24:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-15 22:29:25]

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

[eroman, 2017-05-15 22:42:10]

The policy change and implementation lgtm; my comments are just about the
testing code.

https://codereview.chromium.org/2881023003/diff/20001/net/cert/x509_certifica...
File net/cert/x509_certificate_unittest.cc (right):

https://codereview.chromium.org/2881023003/diff/20001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:369:
GetTestNetDataDirectory().AppendASCII("parse_certificate_unittest");
Can you add these tests to parsed_certificate_unittest.cc ?

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
File net/data/parse_certificate_unittest/serial_negative.pem (right):

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
net/data/parse_certificate_unittest/serial_negative.pem:1: This cert has an
invalid serial number which is negative.
At the risk of causing you more work, we have some similar tests for the TBS
certificate parsing, i.e. search for "serial" here:

https://cs.chromium.org/chromium/src/net/cert/internal/parse_certificate_unit...

Perhaps consider converting those into parsed certificate unit-tests?

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
File net/data/parse_certificate_unittest/serial_zero_padded.pem (right):

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
net/data/parse_certificate_unittest/serial_zero_padded.pem:1: This cert has a
valid serial number which has a preceding zero padding byte due
Assuming these tests are running on ios, you will want to run:

   tools/update_ios_bundle_data.py

to update BUILD.gn

[mattm, 2017-05-15 23:16:28]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[mattm, 2017-05-15 23:16:44]

https://codereview.chromium.org/2881023003/diff/20001/net/cert/x509_certifica...
File net/cert/x509_certificate_unittest.cc (right):

https://codereview.chromium.org/2881023003/diff/20001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:369:
GetTestNetDataDirectory().AppendASCII("parse_certificate_unittest");
On 2017/05/15 22:42:09, eroman wrote:
> Can you add these tests to parsed_certificate_unittest.cc ?

Done. (Though it only tests the default configuration. Do you want me to add
tests for the allow_invalid_serial_numbers option in parsed_certificate_unittest
too?)

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
File net/data/parse_certificate_unittest/serial_negative.pem (right):

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
net/data/parse_certificate_unittest/serial_negative.pem:1: This cert has an
invalid serial number which is negative.
On 2017/05/15 22:42:10, eroman wrote:
> At the risk of causing you more work, we have some similar tests for the TBS
> certificate parsing, i.e. search for "serial" here:
> 
>
https://cs.chromium.org/chromium/src/net/cert/internal/parse_certificate_unit...
> 
> Perhaps consider converting those into parsed certificate unit-tests?

Did you want to remove the ones from parse_certificate_unittest in favor of
parsed_certificate_unittest? Or just add them to parsed_certificate_unittest?

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
File net/data/parse_certificate_unittest/serial_zero_padded.pem (right):

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
net/data/parse_certificate_unittest/serial_zero_padded.pem:1: This cert has a
valid serial number which has a preceding zero padding byte due
On 2017/05/15 22:42:10, eroman wrote:
> Assuming these tests are running on ios, you will want to run:
> 
>    tools/update_ios_bundle_data.py
> 
> to update BUILD.gn

Done.

[commit-bot: I haz the power, 2017-05-15 23:17:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-05-15 23:56:47]

https://codereview.chromium.org/2881023003/diff/20001/net/cert/x509_certifica...
File net/cert/x509_certificate_unittest.cc (right):

https://codereview.chromium.org/2881023003/diff/20001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:369:
GetTestNetDataDirectory().AppendASCII("parse_certificate_unittest");
On 2017/05/15 23:16:44, mattm wrote:
> On 2017/05/15 22:42:09, eroman wrote:
> > Can you add these tests to parsed_certificate_unittest.cc ?
> 
> Done. (Though it only tests the default configuration. Do you want me to add
> tests for the allow_invalid_serial_numbers option in
parsed_certificate_unittest
> too?)

That would be nice if you want to add it.

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
File net/data/parse_certificate_unittest/serial_negative.pem (right):

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
net/data/parse_certificate_unittest/serial_negative.pem:1: This cert has an
invalid serial number which is negative.
On 2017/05/15 23:16:44, mattm wrote:
> On 2017/05/15 22:42:10, eroman wrote:
> > At the risk of causing you more work, we have some similar tests for the TBS
> > certificate parsing, i.e. search for "serial" here:
> > 
> >
>
https://cs.chromium.org/chromium/src/net/cert/internal/parse_certificate_unit...
> > 
> > Perhaps consider converting those into parsed certificate unit-tests?
> 
> Did you want to remove the ones from parse_certificate_unittest in favor of
> parsed_certificate_unittest? Or just add them to parsed_certificate_unittest?

My main motivation is to not duplicate similar .pem files.

i.e. these are mostly redundant now:

  tbs_negative_serial_number.pem
  tbs_serial_number_21_octets_leading_0.pem
  tbs_serial_number_26_octets.pem


The relationship between parse[d]_certifificate_unittest needs some
clarification (separately)
I would be OK in this CL with removing the duplicate tbs_*.pem tests, and having
the test data used in parsed_certificate + x509_certificate

WDYT?

[mattm, 2017-05-16 00:33:57]

https://codereview.chromium.org/2881023003/diff/20001/net/cert/x509_certifica...
File net/cert/x509_certificate_unittest.cc (right):

https://codereview.chromium.org/2881023003/diff/20001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:369:
GetTestNetDataDirectory().AppendASCII("parse_certificate_unittest");
On 2017/05/15 23:56:47, eroman wrote:
> On 2017/05/15 23:16:44, mattm wrote:
> > On 2017/05/15 22:42:09, eroman wrote:
> > > Can you add these tests to parsed_certificate_unittest.cc ?
> > 
> > Done. (Though it only tests the default configuration. Do you want me to add
> > tests for the allow_invalid_serial_numbers option in
> parsed_certificate_unittest
> > too?)
> 
> That would be nice if you want to add it.

Done.

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
File net/data/parse_certificate_unittest/serial_negative.pem (right):

https://codereview.chromium.org/2881023003/diff/20001/net/data/parse_certific...
net/data/parse_certificate_unittest/serial_negative.pem:1: This cert has an
invalid serial number which is negative.
On 2017/05/15 23:56:47, eroman wrote:
> On 2017/05/15 23:16:44, mattm wrote:
> > On 2017/05/15 22:42:10, eroman wrote:
> > > At the risk of causing you more work, we have some similar tests for the
TBS
> > > certificate parsing, i.e. search for "serial" here:
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/net/cert/internal/parse_certificate_unit...
> > > 
> > > Perhaps consider converting those into parsed certificate unit-tests?
> > 
> > Did you want to remove the ones from parse_certificate_unittest in favor of
> > parsed_certificate_unittest? Or just add them to
parsed_certificate_unittest?
> 
> My main motivation is to not duplicate similar .pem files.
> 
> i.e. these are mostly redundant now:
> 
>   tbs_negative_serial_number.pem
>   tbs_serial_number_21_octets_leading_0.pem
>   tbs_serial_number_26_octets.pem
> 
> 
> The relationship between parse[d]_certifificate_unittest needs some
> clarification (separately)
> I would be OK in this CL with removing the duplicate tbs_*.pem tests, and
having
> the test data used in parsed_certificate + x509_certificate
> 
> WDYT?

Done.

[mattm, 2017-05-16 04:42:36]

The CQ bit was checked by mattm@chromium.org

[mattm, 2017-05-16 04:42:37]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2881023003/#ps60001
(title: "review changes 2")

[commit-bot: I haz the power, 2017-05-16 04:42:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 05:32:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 05:32:48]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mattm, 2017-05-16 05:41:03]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2017-05-16 05:41:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 07:18:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 07:18:21]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mattm, 2017-05-16 07:26:48]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2017-05-16 07:27:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 08:13:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494919608166690,
"parent_rev": "55085162c036ab80c4392d24cca15c28d7456848", "commit_rev":
"312644de1876bf9eb520f4cfaef5f51eae1dfaf2"}

[commit-bot: I haz the power, 2017-05-16 08:13:32]

Description was changed from

==========
X509CertificateBytes: Allow invalid serial numbers for now.

BUG=721778
==========

to

==========
X509CertificateBytes: Allow invalid serial numbers for now.

BUG=721778

Review-Url: https://codereview.chromium.org/2881023003
Cr-Commit-Position: refs/heads/master@{#472040}
Committed:
https://chromium.googlesource.com/chromium/src/+/312644de1876bf9eb520f4cfaef5...
==========

[commit-bot: I haz the power, 2017-05-16 08:13:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/312644de1876bf9eb520f4cfaef5...