Add CRL handling #77

tristanmiller-spruceid · 2023-12-01T16:12:24Z

I haven't had a lot of success finding test vectors that exactly math the mdoc spec. Here's my stab at a first pass.

src/definitions/x509/crl.rs

cobward · 2023-12-01T17:00:03Z

src/definitions/x509/crl.rs

+
+/// Given a cert, download and verify the associated crl listed in the cert, and verify the cert
+/// against the crl
+pub async fn fetch_and_validate_crl(cert: &TbsCertificate) -> Result<(), Error> {


In general the CRL isn't necessarily signed by the PK in the cert. For 18013-5 the IACA root cert is the signer for all CRLs in the certificate chain, so you might need to update this function to take in a "crl_signer_cert" parameter.

So I tried a slightly different strategy since there are other certs that need to be checked, but we don't want to have to download the CRL multiple times.

I therefore separated fetch_and_validate_crl from check_cert_against cert_lists, allowing higher level code with more context of the cert tree to iterate on that structure, fetching CRL and validating it against the root cert, then separately using that CRL against any other certs it sees fit.

Does that work for you?

I'm not sure this is quite correct. From my understanding it is possible for a child certificate to have a different CRL endpoint than the root. Perhaps I'm wrong on that?

@justAnIdentity do you know?

I've changed "root_cert" to "crl_signing_cert" to hopefully alleviate these concerns.

…ng and decryption errors under errors

src/definitions/x509/crl.rs

sbihel · 2023-12-07T11:39:00Z

src/definitions/x509/crl.rs

+}
+
+async fn fetch_crl(url: &str) -> Result<Vec<u8>, Error> {
+    let bytes = reqwest::get(url)


It would be good to create a CrlVerifier struct to avoid re-creating the reqwest client every time

I'm don't believe that matches our data access pattern. AFAIK, there isn't really a case where we need to batch many if even multiple requests for CRLs.

sbihel · 2023-12-07T11:52:35Z

src/definitions/x509/crl.rs

+    SignatureInWrongFormat,
+}
+
+pub const OID_EC_CURVE_P256: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.2.840.10045.3.1.7");


This library should burden itself with defining mappings. It should instead rely on the pkcs8 crate with the AssociatedOid trait. Then the verification function can accept an object that implements both DecodePublicKey and the Verifier trait from the signature trait -- or something along these lines, maybe DynAssociatedAlgorithmIdentifier will be useful. Then it's on the consumer to pick the "crypto backend". And it allows to build an object that can support multiple types of keys.

A "default" verifier can be provided by this library, but feature gated.

- Also add a CLI tool for validating issuer certificates.

justAnIdentity · 2024-01-02T09:21:59Z

src/definitions/x509/crl.rs

+            uris
+        }
+        DistributionPointName::NameRelativeToCRLIssuer(_) => Err(
+            Error::DistributionPointMalformed("contained relative to issuer name"),


I think using a NameRelativeToCRLIssuer is allowed according to RFC8250.
If you don't want to support that yet, that's fine.

I was going off of RF5280 4.2.1.13. where it says

Conforming CAs SHOULD NOT use nameRelativeToCRLIssuer to specify distribution point names.

But I might have misunderstood the scope of that statement.

cobward · 2024-01-02T16:04:03Z

src/definitions/x509/crl.rs

+    if cert.subject_public_key_info.algorithm != crl.signature_algorithm {
+        return Err(Error::SignatureTypeMismatch(
+            Box::new(cert.subject_public_key_info.algorithm.clone()),
+            Box::new(crl.signature_algorithm),
+        ));
+    }


I think this is wrong, the crl should be signed by the issuer public key, not the subject public key.

I'm going to respond to this and the comment below simultaneously. That comment copied below for posterity:

@tristanmiller-spruceid could you please address the outstanding issues?

The CRL is verified against the certificate subject's public key, instead of the certificate issuer. This only works if the certificate being checked is self-signed (e.g. the root). The current API does not support checking the CRL of an intermediate certificate, unless it happens that the intermediate certificate and the root certificate have the CRL.

To resolve this please consider implementing the public API as described in the issue:
#76 (comment)

First, given the complexity of certificate chains, I've split the public API of the crl crate into two halves, 1) given any cert in the chain, grab the CRLs that this cert signs and check that CRL's validity (fetch_and_validate_crl) and 2) given a CRL and a cert, return if the cert is revoked (check_cert_against_cert_lists). This allows for relatively arbitrary certificate chains as implemented by higher level code where there may be several layers of certificate chains.

So it's expected that calling code will iterate through their cert tree chain, attach all CRLs to that chain each grabbed with fetch_and_validate_crl, and validate each node in that chain against each of the child CRLs with check_cert_against_cert_lists.

Additionally, the CRLs are signed by the subject public key of the the cert who's distribution point specified them. That does not stop from checking the CRL of an intermediate certificate, one simply needs to call fetch_and_validate_crl against the intermediate certs as well. The argument to fetch_and_validate_crl has had it's name changed from root_cert to crl_signing_cert to make that more clear.

Additionally, the CRLs are signed by the subject public key of the the cert who's distribution point specified them.

Correct me if I'm wrong, but I believe CRLs are signed by the issuer of the cert whose distribution point specified them, as you pointed out:

RFC 5280 6.3.1. CRL Processing (b)(1)

[V]erify that the CRL issuer matches the certificate issuer.

For a root CA, this happens to be itself as it is self-signed.

That does not stop from checking the CRL of an intermediate certificate, one simply needs to call fetch_and_validate_crl against the intermediate certs as well. The argument to fetch_and_validate_crl has had it's name changed from root_cert to crl_signing_cert to make that more clear.

The problem is that for an intermediate certificate the fetch_and_validate_crl function will fail because the intermediate certificate does not sign its own CRL, the CRL issuer is the certificate issuer.

src/definitions/x509/crl.rs

cobward

@tristanmiller-spruceid could you please address the outstanding issues?

The CRL is verified against the certificate subject's public key, instead of the certificate issuer. This only works if the certificate being checked is self-signed (e.g. the root). The current API does not support checking the CRL of an intermediate certificate, unless it happens that the intermediate certificate and the root certificate have the same CRL.

To resolve this please consider implementing the public API as described in the issue:
#76 (comment)

src/definitions/x509/crl.rs

cobward · 2024-01-12T07:28:01Z

src/definitions/x509/crl/testcrl.der

Could you move this file? We normally keep test data in the ./test directory rather than in-line.

author Arjen van Veen <[email protected]> 1701187280 +0100 committer Arjen van Veen <[email protected]> 1717744754 +0200 refactor for readability WIP clean up comments and duplicates clean up, add some comments cargo fmt clippy fix fmt assert on tests address pr comments refactor handle_response to return a validated_response, submit parsing and decryption errors under errors support creating a trust_anchor_registry from pem strings Fix x5chain encoding. X5Chain decoding fixes and version checking Improve reader validation code. - Also add a CLI tool for validating issuer certificates. Fix public key parsing Feat/reader auth cn (#79) * rebase onto feat/mdoc-auth * rebase and use mdoc-auth functions * wip experiment with cert building * small clean up * Fix inconsistency. (#78) * validated request improvements --------- Co-authored-by: Jacob <[email protected]> remove duplicate code clippy fix

Signed-off-by: Ryan Tate <[email protected]>

Co-authored-by: Jacob <[email protected]> Signed-off-by: Ryan Tate <[email protected]>

Signed-off-by: Ryan Tate <[email protected]>

justAnIdentity and others added 10 commits November 24, 2023 10:52

mdoc auth and iaca certificate validation

10867e6

refactor for readability WIP

c52d24d

clean up comments and duplicates

37c7557

clean up, add some comments

9afdde4

cargo fmt

6dbe575

clippy fix

9a274a1

fmt

81647ed

clippy fix

e8c7b1e

assert on tests

362caf1

Add CRL handling

2d45b12

cobward reviewed Dec 1, 2023

View reviewed changes

justAnIdentity and others added 9 commits December 3, 2023 13:49

address pr comments

b3cd197

refactor handle_response to return a validated_response, submit parsi…

8f69a94

…ng and decryption errors under errors

support creating a trust_anchor_registry from pem strings

2cfa41d

Errors using thiserror

811415d

Allow seperation of CRL from which cert is being checked

01b762d

Document all public interfaces

d87fbd0

Takes an array of cert_lists since that's what is fetched

92f58aa

Update comment

3b40f1b

Fix x5chain encoding.

2f89cdf

sbihel requested changes Dec 7, 2023

View reviewed changes

cobward and others added 4 commits December 11, 2023 19:37

X5Chain decoding fixes and version checking

adf41c3

Improve reader validation code.

cda4e35

- Also add a CLI tool for validating issuer certificates.

Fix public key parsing

479c8c0

Convert Error::FetchCrl for thiserror #from

9a3e3aa

sbihel approved these changes Dec 21, 2023

View reviewed changes

cobward requested a review from justAnIdentity December 21, 2023 18:25

justAnIdentity reviewed Jan 2, 2024

View reviewed changes

justAnIdentity approved these changes Jan 2, 2024

View reviewed changes

Merge branch 'feat/mdoc-auth' into feat/mdoc-auth-crl

abf844b

cobward reviewed Jan 3, 2024

View reviewed changes

cobward requested changes Jan 10, 2024

View reviewed changes

src/definitions/x509/crl.rs Outdated Show resolved Hide resolved

src/definitions/x509/crl.rs Show resolved Hide resolved

tristanmiller-spruceid added 3 commits January 10, 2024 08:52

Change root_cert to crl_signing_cert

7cbf2fc

Remove commented out code

cb6d766

find_reason_code returns Option instead of Result

3b0b901

cobward reviewed Jan 12, 2024

View reviewed changes

justAnIdentity added 2 commits June 7, 2024 09:17

mdoc auth and iaca certificate validation

4e7fc9f

justAnIdentity force-pushed the feat/mdoc-auth branch from d43c6c7 to b1ec197 Compare June 7, 2024 07:32

Ryanmtate and others added 3 commits November 16, 2024 12:14

wip: resolve merge conflicts with main

300ab3e

Signed-off-by: Ryan Tate <[email protected]>

Update src/definitions/x509/extensions.rs

8085446

Co-authored-by: Jacob <[email protected]> Signed-off-by: Ryan Tate <[email protected]>

resolve merge conflicts with mdoc-auth branch

be54eb2

Signed-off-by: Ryan Tate <[email protected]>

Base automatically changed from feat/mdoc-auth to main December 20, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CRL handling #77

Add CRL handling #77

tristanmiller-spruceid commented Dec 1, 2023

cobward Dec 1, 2023

tristanmiller-spruceid Dec 6, 2023

cobward Dec 21, 2023

cobward Dec 21, 2023

tristanmiller-spruceid Jan 10, 2024

sbihel Dec 7, 2023

tristanmiller-spruceid Dec 20, 2023

sbihel Dec 7, 2023

justAnIdentity Jan 2, 2024

tristanmiller-spruceid Jan 11, 2024

cobward Jan 2, 2024

tristanmiller-spruceid Jan 11, 2024

cobward Jan 12, 2024

cobward left a comment •

edited

Loading

cobward Jan 12, 2024

Add CRL handling #77

Are you sure you want to change the base?

Add CRL handling #77

Conversation

tristanmiller-spruceid commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cobward left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cobward left a comment •

edited

Loading