Allow negative serials in all certificates, not just trust anchors. #10
Conversation
dequbed
force-pushed
the
feature/serial_neg_ee
branch
2 times, most recently
from
987b26e to
b9ea527
Compare
This follows the crypt implementation of Go, as per the following commit: golang/go@a0ea93d Signed-off-by: Nadja Reitzenstein <me@dequbed.space>
Signed-off-by: Nadja Reitzenstein <me@dequbed.space>
dequbed
force-pushed
the
feature/serial_neg_ee
branch
from
b9ea527 to
65c1a4d
Compare
|
cc @cpu |
| let anchors = vec![webpki::TrustAnchor::try_from_cert_der(ca).unwrap()]; | ||
| let anchors = webpki::TLSServerTrustAnchors(&anchors); | ||
|
|
||
| //#[allow(clippy::unreadable_literal)] // TODO: Make this clear. |
There was a problem hiding this comment.
Just a note to other reviewers: this TODO is not unique to this work, but another instance of a pattern established by 2 other tests in this file. It would be nice to fix these up but I don't think it needs to be done in this branch.
| assert_eq!( | ||
| Ok(()), | ||
| cert.verify_is_valid_tls_server_cert(ALL_SIGALGS, &anchors, &[], time) | ||
| ); |
There was a problem hiding this comment.
very tiny nit: I think this might be expressed more idiomatically as :
| assert_eq!( | |
| Ok(()), | |
| cert.verify_is_valid_tls_server_cert(ALL_SIGALGS, &anchors, &[], time) | |
| ); | |
| assert!( | |
| cert.verify_is_valid_tls_server_cert(ALL_SIGALGS, &anchors, &[], time) | |
| .is_ok()); |
There was a problem hiding this comment.
I think assert_eq!() probably has better ergonomics in case of failure (that is, the failure output is easier to diagnose). But I would find it more idiomatic to have actual, then expected.
| //#[allow(clippy::unreadable_literal)] // TODO: Make this clear. | ||
| let time = webpki::Time::from_seconds_since_unix_epoch(1667401500); |
There was a problem hiding this comment.
Let's remove the TODO and add a comment with the RFC 3339 value of the timestamp. I'll do the same for the other instances in a follow-up.
| //#[allow(clippy::unreadable_literal)] // TODO: Make this clear. | |
| let time = webpki::Time::from_seconds_since_unix_epoch(1667401500); | |
| //#[allow(clippy::unreadable_literal)] | |
| let time = webpki::Time::from_seconds_since_unix_epoch(1667401500); // 2022-11-02T15:05:00Z | |
| if value.big_endian_without_leading_zero().len() > 20 { | ||
| return Err(Error::BadDER); | ||
| } |
There was a problem hiding this comment.
Is removing this length check intentional? The rest of the PR motivation is about allowing negative serials.
With that said, this constraint is clearly on issuers and not verifiers.
| // However, we don't enforce these constraints, as there are widely-deployed trust anchors | ||
| // and many X.509 implementations in common use that violate these constraints. |
There was a problem hiding this comment.
Perhaps we can further quote RFC5280 here:
Note: Non-conforming CAs may issue certificates with serial numbers
that are negative or zero. Certificate users SHOULD be prepared to
gracefully handle such certificates.
|
This was merged through #36. Thanks for working on this! |
Based on discussion in Discord: https://discord.com/channels/976380008299917365/1015156984007381033/1037367413357948928
This follows the crypt implementation of Go, as per the following commit: golang/go@a0ea93d