Add logic to verify and get signing key from chain #70
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
❌ Unreviewed dependencies found
|
| // In order to verify the signature, we need to access the original DER | ||
| // bytes | ||
| der_bytes: &'a [u8], | ||
| certificate: X509Certificate, | ||
| der_bytes: Vec<u8>, |
There was a problem hiding this comment.
Got rid of the lifetimes here, it prevented the PEM decoding in the code, since pem generates a vector, and since this logic inherently needs alloc behind the scenes it doesn't seem like we gain much trying to minimize vector usage.
| /// Returning the signing key from the leaf certificate | ||
| /// | ||
| /// The chain will be verified against the `trust_root` and the `unix_time`. | ||
| pub fn signing_key( | ||
| &self, | ||
| trust_root: VerifiedCertificate, | ||
| unix_time: Duration, | ||
| ) -> Result<VerifyingKey> { |
There was a problem hiding this comment.
This might be doing too much, but TBH I'm still on the fence on the "unverified***"->"verified***" logic I have for certs and CRLs.
| #[cfg(test)] | ||
| mod test { |
There was a problem hiding this comment.
I think this is going to need some more robust testing, thinking of grabbing some of the test vectors from https://csrc.nist.gov/projects/pki-testing
Codecov Report
@@ Coverage Diff @@
## main #70 +/- ##
==========================================
+ Coverage 97.35% 97.63% +0.27%
==========================================
Files 6 7 +1
Lines 1476 1648 +172
==========================================
+ Hits 1437 1609 +172
Misses 39 39
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| /// Returning the signing key from the leaf certificate | ||
| /// | ||
| /// The chain will be verified against the `trust_root` and the `unix_time`. | ||
| pub fn signing_key( |
There was a problem hiding this comment.
This will need to be passed a collection of CRLs added a bullet item to #50 to keep track of this
| // In order to verify the signature, we need to access the original DER | ||
| // bytes | ||
| der_bytes: &'a [u8], | ||
| certificate: X509Certificate, | ||
| der_bytes: Vec<u8>, |
| #[parameterized( | ||
| root = { ROOT_CA }, | ||
| processor = { PROCESSOR_CA }, | ||
| leaf = { LEAF_CERT }, | ||
| )] |
There was a problem hiding this comment.
Why parametrize these values if there's only one value for each param? Consider removing this usage if there isn't a good reason...
There was a problem hiding this comment.
I'm not sure I follow. Parameterized tests allow one to re-use the same testing behavior with different inputs(parameters). The number of parameters per test seems orthogonal to that use.
For this specific case I'll admit it doesn't gain much, other than if one wants to add another certificate to try it's one line vs 4 and doesn't require as much thought on the naming.
For example the explicit creation of dedicated tests would be something like:
fn try_from_root_ca() {
assert!(UnverifiedCertificate::try_from(ROOT_CA).is_ok());
}
fn try_from_processor_ca() {
assert!(UnverifiedCertificate::try_from(PROCESSOR_CA).is_ok());
}
fn try_from_leaf_cert() {
assert!(UnverifiedCertificate::try_from(LEAF_CERT).is_ok());
}
nick-mobilecoin
force-pushed
the
nick/chain
branch
from
6bb0e3d
to
e3ab151
Compare
nick-mobilecoin
force-pushed
the
nick/chain
branch
from
e3ab151
to
224104a
Compare
nick-mobilecoin
force-pushed
the
nick/chain
branch
from
224104a
to
687cf04
Compare
|
punting on implementing x509 chain parsing logic. |
Motivation