Make *ring* optional #134
Make *ring* optional #134
Conversation
Codecov Report
@@ Coverage Diff @@
## main #134 +/- ##
==========================================
+ Coverage 95.35% 96.28% +0.93%
==========================================
Files 15 16 +1
Lines 4002 4070 +68
==========================================
+ Hits 3816 3919 +103
+ Misses 186 151 -35
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ctz
force-pushed
the
jbp-optional-ring
branch
5 times, most recently
from
7b8ed5e to
348be58
Compare
|
Haven't forgotten about this branch, I just want to review it carefully and have been tied up with other work. I will make time tomorrow :-) |
|
Same here. My high-level thought so far: have you given thought to the ergonomics of implementing separate signature traits for both rustls and webpki? It would suck a little to have a bunch of boilerplate to do them twice. |
src/der.rs
Outdated
| @@ -13,7 +13,9 @@ | |||
| // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. | |||
|
|
|||
| use crate::{calendar, time, Error}; | |||
| pub(crate) use ring::io::der::{CONSTRUCTED, CONTEXT_SPECIFIC}; | |||
|
|
|||
| pub(crate) const CONSTRUCTED: u8 = 0x20; | |||
There was a problem hiding this comment.
Style nit: top-down ordering would suggest moving these to the bottom of the module.
src/signed_data.rs
Outdated
| pub struct SignatureAlgorithm { | ||
| /// A detail-less error when a signature is not valid. | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct InvalidSignature; |
There was a problem hiding this comment.
Style nits: I would move the InvalidSignature type definition below the trait definition, and move the RingSignatureAlgorithm definition and impl above the trait definition.
I think there isn't any overlap. The closest we have is https://docs.rs/rustls/latest/rustls/client/trait.ServerCertVerifier.html#method.verify_tls13_signature but that is in terms of certificates rather than public keys, and doesn't deal with any of the pkix public key/signature algorithm matching business that needs to happen here. There's also https://docs.rs/rustls/latest/rustls/sign/trait.Signer.html but that's for signing rather than verification. |
But this is implemented for the same number of algorithms, right? And an implementation would presumably want to support both signing and verification using the same algorithms. |
|
Alright, that's fair. Renaming to make this more obvious is a good idea. It looks like ring calls them |
ctz
force-pushed
the
jbp-optional-ring
branch
4 times, most recently
from
5b0c4ff to
6b98fb7
Compare
`der::nonnegative_integer` and `der::small_nonnegative_integer` are imported from ring. This relaxes id-ce-cRLNumber decoding from being a positive integer to a non-negative integer (ie, 0 is now allowed). This is OK because RFC5240 defines it as: > CRLNumber ::= INTEGER (0..MAX)
- use oid! macro for OIDs - add thousand separators for large integer literals - remove allow(dead_code) that had no effect - split out and document calculation of DAYS_BEFORE_UNIX_EPOCH_AD
The next commit makes a breaking change to callers of these APIs, so it's a good time to drop them? Inline the single-caller `verify_is_valid_cert` function.
Rename it to `SignatureVerificationAlgorithm`. `RingAlgorithm` exists as the previous type, and implements the trait. This is a breaking change: - top level functions now need to pass a &[&dyn SignatureVerificationAlgorithm] - objects like `ECDSA_P256_SHA256` are now a `&dyn SignatureVerificationAlgorithm` so callers don't see the internal `RingAlgorithm` type.
It is a default feature. Taking it away disables all the signature algorithms, and therefore most of the tests.
fixes #130