Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat: enhance certificate chain verification #2098

Merged
merged 6 commits into from
Nov 8, 2024
Merged

Feat: enhance certificate chain verification #2098

merged 6 commits into from
Nov 8, 2024

Conversation

jpraynaud
Copy link
Member

@jpraynaud jpraynaud commented Nov 7, 2024 •
edited
Loading

Content

This PR includes enhancements to the verification of the certificate chain:

  • Improved readability of the code by creating multiple sub verification functions
  • Refactoring of the CertificateVerifier trait
  • Simplified and strengthened testing

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@jpraynaud jpraynaud self-assigned this Nov 7, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024 •
edited
Loading

Test Results

    4 files  ± 0     51 suites  ±0   11m 6s ⏱️ +27s
1 436 tests +11  1 436 ✅ +11  0 💤 ±0  0 ❌ ±0 
1 647 runs  +11  1 647 ✅ +11  0 💤 ±0  0 ❌ ±0 

Results for commit 471f1af. ± Comparison against base commit b725d10.

This pull request removes 9 and adds 20 tests. Note that renamed tests count towards both. 
mithril-common ‑ certificate_chain::certificate_verifier::tests::test_verify_certificate_chain_ko
mithril-common ‑ certificate_chain::certificate_verifier::tests::test_verify_certificate_chain_ok
mithril-common ‑ certificate_chain::certificate_verifier::tests::test_verify_certificate_ko_certificate_chain_avk_unmatch
mithril-common ‑ certificate_chain::certificate_verifier::tests::test_verify_certificate_ko_certificate_chain_previous_hash_unmatch
mithril-common ‑ certificate_chain::certificate_verifier::tests::test_verify_certificate_ko_certificate_hash_not_matching
mithril-common ‑ certificate_chain::certificate_verifier::tests::test_verify_certificate_ok_different_epochs
mithril-common ‑ certificate_chain::certificate_verifier::tests::test_verify_certificate_ok_same_epoch
mithril-common ‑ certificate_chain::certificate_verifier::tests::test_verify_multi_signature_ok
mithril-common ‑ certificate_chain::fake_certificate_retriever::tests::test_fake_certificate_fails_retrieving_unknow_certificate

mithril-common ‑ certificate_chain::certificate_verifier::tests::verify_certificate_chain_fails_when_chain_is_tampered
mithril-common ‑ certificate_chain::certificate_verifier::tests::verify_certificate_chain_success_when_chain_is_valid
mithril-common ‑ certificate_chain::certificate_verifier::tests::verify_certificate_chain_verifies_all_chained_certificates
mithril-common ‑ certificate_chain::certificate_verifier::tests::verify_certificate_ko_certificate_hash_not_matching
mithril-common ‑ certificate_chain::certificate_verifier::tests::verify_certificate_success_when_certificate_is_genesis_and_valid
mithril-common ‑ certificate_chain::certificate_verifier::tests::verify_certificate_success_when_certificate_is_standard_and_valid
mithril-common ‑ certificate_chain::certificate_verifier::tests::verify_genesis_certificate_fails_if_hash_unmatch
mithril-common ‑ certificate_chain::certificate_verifier::tests::verify_genesis_certificate_fails_if_is_not_genesis
mithril-common ‑ certificate_chain::certificate_verifier::tests::verify_genesis_certificate_fails_if_protocol_message_unmatch
mithril-common ‑ certificate_chain::certificate_verifier::tests::verify_genesis_certificate_success
…

♻️ This comment has been updated with latest results.

Alenar
Alenar approved these changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@Alenar Alenar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🔥

dlachaume
dlachaume approved these changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

sfauvel
sfauvel approved these changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jpraynaud
chore: make CertificateChainBuilderContext visibility public
3e347ef
@jpraynaud
chore: fix typos
9d2223b
@jpraynaud
refactor: simplify 'verify_certificate' in CertificateVerifier
7a17736 
- Split verification into multiple sub functions
- Clear separation of 'verify_standard_certificate' vs 'verify_genesis_certificate'
- Make 'verify_standard_certificate' stateless and part of the CertificateVerifier trait
- Rewrite 'verify_certificate' with better readability.
@jpraynaud
refactor: tests in CertificateVerifier
7d5d7ee 
- Enhance tests readability with 'assert_error_matches' macro
- Simplify tests with mock dependency injector
- Add missing test cases.
@jpraynaud
feat: test 'verify_certificate_chain' verifies all chained certificates
7800c75
@jpraynaud
chore: bump crates versions
471f1af 
- 'mithril-common' from '0.4.82' to '0.4.83'.
@jpraynaud jpraynaud force-pushed the jpraynaud/enhance-certificate-chain-verification branch from ec3bc90 to 471f1af Compare November 7, 2024 17:26
@jpraynaud jpraynaud merged commit 2e7a0dc into main Nov 8, 2024
50 checks passed
@jpraynaud jpraynaud deleted the jpraynaud/enhance-certificate-chain-verification branch November 8, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlachaume dlachaume dlachaume approved these changes

@sfauvel sfauvel sfauvel approved these changes

@Alenar Alenar Alenar approved these changes

Assignees

@jpraynaud jpraynaud

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jpraynaud @Alenar @sfauvel @dlachaume