Add IssuerAuth, MdocAuth, ReaderAuth checks #75

justAnIdentity · 2023-11-22T17:35:24Z

This PR adds support for the mdoc holder to validate the ReaderAuth, and the mdoc reader to validate the mdoc auth and issuer auth.

Remaining outstanding work items:

Check CRL of DS and IACA certificates.
Check CRL/OCSP for reader and reader CA certificates.
Add support for producing ReaderAuth secured document requests.
Add support for key curves other than P-256.
Perform validation of particular mDL attributes (e.g. country name matches certificate).

cobward

Preliminary suggestions to improve the code. I think we need a couple of iterations to make this clearer. Since this is very security sensitive code it is really important that it is obvious what it is doing, it is easy to follow, and it is well documented. That way we can have confidence that it is implemented correctly.

To that effect encapsulating code so that we don't have multi-hundred line validation functions, and adding lots of comments would be very welcome.

src/issuance/x5chain.rs

src/presentation/mdoc_auth.rs

src/presentation/reader.rs

src/presentation/trust_anchor.rs

src/presentation/reader.rs

src/presentation/trust_anchor.rs

src/definitions/x509/extensions.rs

src/definitions/x509/error.rs

src/definitions/x509/extensions.rs

src/definitions/x509/mod.rs

src/definitions/x509/x5chain.rs

src/presentation/reader.rs

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]>

- Moved and renamed to 'src/presentation/authentication'. - Added doc comments. - Removed 'decryption' and 'parsing' from ResponseAuthenticationOutcome, as these are errors, not part of validating the response.

cobward

I'm wrapping up this work, just need a bit of help from @justAnIdentity on identifying these TODOs

src/presentation/authentication/mdoc.rs

src/definitions/x509/extensions.rs

src/definitions/x509/trust_anchor.rs

* Remove unnecessary TODO regarding 18013-7: work will be underway soon. * Remove unnecessary TODO regarding reader certificate checks - already implemented. * Ensure that all critical extensions are checked and understood.

Refactored the x5chain validation code to help debug an issue with the chain validation. Added an e2e positive test for DS->IACA validation.

src/definitions/x509/validation/extensions/mod.rs

justAnIdentity · 2024-12-18T14:08:42Z

src/definitions/x509/validation/extensions/crl_distribution_points.rs

+        "CrlDistributionPoints"
+    }
+
+    fn validate(&self, extension: &Extension) -> Vec<Error> {


I'm not 100% that I'm fully following the entire flow, but I think you should still check here that the CrlDistributionPoint extension is marked as non-critical.

My understanding is that the critical flag indicates that the consumer MUST understand the extension, otherwise it should fail the transaction. In these cases, the 18013-5 spec says that these extensions shall not be critical. But from a practical perspective we do understand these extensions, so I don't think it matters whether or not the extension is actually marked as critical. I'm leaning more toward a slightly more permissive implementation in cases where security isn't impacted, rather than strictly enforcing every requirement.

I see what you mean. My point of view is that since this is open source, developers may use this to validate their own certificates' iso compliance and this still leaves some room for them to do it incorrectly. Not a hill I'd die on though.

Makes sense. We probably want a "strict" mode and "permissive" mode to allow for that as we've discussed before. I think that's out of the scope of this work, but I'll make sure we have a task that captures this.

justAnIdentity · 2024-12-18T14:10:06Z

src/definitions/x509/validation/extensions/issuer_alternative_name.rs

+    }
+
+    fn validate(&self, extension: &Extension) -> Vec<Error> {
+        let bytes = extension.extn_value.as_bytes();


The same here. IAN should never be critical, so you could check here that it's not set as critical.

justAnIdentity · 2024-12-18T14:31:51Z

src/definitions/x509/validation/extensions/subject_key_identifier.rs

+    }
+
+    fn validate(&self, extension: &Extension) -> Vec<Error> {
+        let bytes = extension.extn_value.as_bytes();


non-critical check here too

justAnIdentity force-pushed the feat/mdoc-auth branch from 6029784 to 10867e6 Compare November 24, 2023 09:52

cobward reviewed Nov 24, 2023

View reviewed changes

sbihel changed the title ~~Feat/mdoc auth~~ Add mDoc auth and iaca validation Nov 27, 2023

cobward reviewed Dec 1, 2023

View reviewed changes

justAnIdentity changed the title ~~Add mDoc auth and iaca validation~~ Add mDoc auth Reader auth Mar 25, 2024

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 2 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]>

Ryanmtate force-pushed the feat/mdoc-auth branch from 08b461e to 8085446 Compare November 18, 2024 20:30

cobward added 2 commits December 6, 2024 11:38

Linting

0ae09b6

Tidy up "Validated*" structs

9afdfac

- Moved and renamed to 'src/presentation/authentication'. - Added doc comments. - Removed 'decryption' and 'parsing' from ResponseAuthenticationOutcome, as these are errors, not part of validating the response.

cobward reviewed Dec 6, 2024

View reviewed changes

src/presentation/authentication/mdoc.rs Outdated Show resolved Hide resolved

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

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

cobward added 5 commits December 9, 2024 18:27

Clean up TODOs

47b544c

* Remove unnecessary TODO regarding 18013-7: work will be underway soon. * Remove unnecessary TODO regarding reader certificate checks - already implemented. * Ensure that all critical extensions are checked and understood.

Fix extensions validation and tests

556f144

Linting

b8c6e8e

Remove unreference file and fix doc issues

cd332c6

Refactor x5chain validation code.

2186549

Refactored the x5chain validation code to help debug an issue with the chain validation. Added an e2e positive test for DS->IACA validation.

cobward changed the title ~~Add mDoc auth Reader auth~~ Add IssuerAuth, MdocAuth, ReaderAuth checks. Dec 18, 2024

cobward changed the title ~~Add IssuerAuth, MdocAuth, ReaderAuth checks.~~ Add IssuerAuth, MdocAuth, ReaderAuth checks Dec 18, 2024

Lint

b316f32

justAnIdentity commented Dec 18, 2024

View reviewed changes

Combine ExtensionValidator impl blocks

5bd86c9

cobward approved these changes Dec 20, 2024

View reviewed changes

cobward merged commit b3a0317 into main Dec 20, 2024
1 check passed

cobward deleted the feat/mdoc-auth branch December 20, 2024 13:26

cobward restored the feat/mdoc-auth branch December 20, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IssuerAuth, MdocAuth, ReaderAuth checks #75

Add IssuerAuth, MdocAuth, ReaderAuth checks #75

justAnIdentity commented Nov 22, 2023 •

edited by cobward

Loading

cobward left a comment

cobward left a comment

justAnIdentity Dec 18, 2024

cobward Dec 18, 2024

justAnIdentity Dec 18, 2024

cobward Dec 18, 2024

justAnIdentity Dec 18, 2024

justAnIdentity Dec 18, 2024

Add IssuerAuth, MdocAuth, ReaderAuth checks #75

Add IssuerAuth, MdocAuth, ReaderAuth checks #75

Conversation

justAnIdentity commented Nov 22, 2023 • edited by cobward Loading

cobward left a comment

Choose a reason for hiding this comment

cobward left a comment

Choose a reason for hiding this comment

justAnIdentity Dec 18, 2024

Choose a reason for hiding this comment

cobward Dec 18, 2024

Choose a reason for hiding this comment

justAnIdentity Dec 18, 2024

Choose a reason for hiding this comment

cobward Dec 18, 2024

Choose a reason for hiding this comment

justAnIdentity Dec 18, 2024

Choose a reason for hiding this comment

justAnIdentity Dec 18, 2024

Choose a reason for hiding this comment

justAnIdentity commented Nov 22, 2023 •

edited by cobward

Loading