ensure default serial generation fits 20 bytes

[BiagioFesta]

By default, s/n is generated taking digest of pub-key of the certificate.

However, if the slice number representation is a negative number, then write_bigint_bytes is going to append an additional 0-byte to ensure the positive sign.

See: https://github.com/qnighy/yasna.rs/blob/b7e65f9a4c317494cce2d18ea02b3d6eaaea7985/src/writer/mod.rs#L493-L495

So it is possible the bigint encoding will take 21 bytes instead of 20.

This CR sets MSB of digest to 0 to ensure encoding will take exactly 20 bytes

[est31]

As per briansmith/webpki#232, webpki does not accept such serials. For truly random serials, 50% of the generated certificates have 1 at that position. So I wonder why this doesn't result in CI errors?

[BiagioFesta]

As per briansmith/webpki#232, webpki does not accept such serials. For truly random serials, 50% of the generated certificates have 1 at that position. So I wonder why this doesn't result in CI errors?

Do you have in mind a particular test that could fail?

[BiagioFesta]

We can obtain the same result (more or less) with alternatives:

• let sl = &hash.as_ref()[..19]. This will avoid one line and mut, but we are "wasting" 7 bits
• Avoid heap allocation with array and memcpy.

I opted for the most coincided approach with Vec, considering I don't expect hot-path here

[cpu]

As per briansmith/webpki#232, webpki does not accept such serials.

This was changed in rustls/webpki: rustls/webpki#36 and that's what CI uses

The diff in this branch looks correct to me on first glance.

[cpu]

Thank you

[est31]

Do you have in mind a particular test that could fail?

most of the tests in rcgen/tests/webpki.rs generate certificates with random keys and therefore I'd expect 50% to fail. Regarding the explanation that this is fixed in rustls-webpki, we have only switched to it in July in 1e149e8, but the problem seems to exist since April: e8dc6cd / 5056d6b. The CI on the main branch is reported as green though in the period between April and July.

Eventually I'm fine with merging this even with this discrepancy unresolved, given that we have large degree of freedom to set the serial number, but I'd still like to find out why CI wasn't failing.

[cpu]

Regarding the explanation that this is fixed in rustls-webpki, we have only switched to it in July in 1e149e8, but the problem seems to exist since April: e8dc6cd / 5056d6b. The CI on the main branch is reported as green though in the period between April and July.

I went back to dde0e47, the last rev before the switch to rustls/webpki. I generated certificates with rcgen until I got one with a 21 byte encoded serial (verified with x509_parser::parse_x509_certificate(&der).unwrap().tbs_certificate.raw_serial().len()).

Parsing the same DER with briansmith/webpki at version 0.22, using webpki::EndEntityCert::try_from produced no error. The parsing logic uses big_endian_without_leading_zero from *ring* to strip a potential leading zero octet before verifying the serial number isn't greater than 20 octets. I haven't dug back in time to figure out when this was introduced - the lack of versioned tags in those repos makes it an annoying process.

Tldr; Serial number handling was further relaxed in rustls/webpki, but this misencoding was being handled as a special case in briansmith/webpki as well, and that masked the error in rcgen.

[cpu]

Some checks haven’t completed yet

Rebasing on main should fix this.

[est31]

Tldr; Serial number handling was further relaxed in rustls/webpki, but this misencoding was being handled as a special case in briansmith/webpki as well, and that masked the error in rcgen.

thanks for analyzing this. I have approved the PR. We can merge it now.