Skip to content

fix: correct validation for cert chain #11

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kt81
Copy link
Contributor

@kt81 kt81 commented Apr 7, 2025

It appears SignedDataVerifier was not working correctly. This change addresses the issue.

  • Updated to explicitly use CustomRootTrust
  • Following this change, the AllowUnknownCertificateAuthority option has been removed.
  • Added support for accepting multiple Root CAs, aligning with libraries in other languages.
  • Disabled revocation checking (set to false) during tests. This is because tests use self-signed certificates, which likely prevent revocation checks from working correctly.

Please let me know if you find any issues or have any questions. 🐻

@flagbug
Copy link
Member

flagbug commented Apr 7, 2025

@kt81 Thanks for the PR! Could you please elaborate on what "was not working correctly" means exactly? What is the incorrect behavior you observed?

@kt81
Copy link
Contributor Author

kt81 commented Apr 8, 2025

@flagbug
Thank you for asking, and apologies that my previous explanation wasn't sufficiently clear.

Here's a description of the issues observed before the changes in this PR:

  • The tests were passing regardless of the root certificate content provided via RootCaBase64Encoded in the test setup. Even if we changed it to various other root certificates, the tests still passed when they arguably shouldn't have.
  • In the VerifyAndDecode_ChainCertificateCompromised_Fails test case, which is expected to fail specifically with a chain validation exception, it was incorrectly throwing a signature exception instead.

This PR addresses these two problems.

My understanding of the situation before this PR is that the root certificate provided (via RootCaBase64Encoded) was likely being evaluated somewhat in isolation. It seemed that as long as any technically valid root certificate was supplied (even a custom self-signed one unrelated to the actual JWS chain), the CheckCertificateChain would incorrectly pass for all JWS test cases. This PR aims to correct this logic to perform proper chain validation against the specified root(s).

@kt81
Copy link
Contributor Author

kt81 commented Apr 15, 2025

Hi @flagbug,

Please let me know if you have any questions or concerns after reviewing my previous explanation. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants