feat(sdk): enhance assertion verification to support jwk and x509 certificates #322
Conversation
…tificates Signed-off-by: Scott Hamrick <2623452+cshamrick@users.noreply.github.com>
Summary of ChangesHello @cshamrick, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the TDF SDK's capability to verify assertions by integrating support for public keys embedded directly within JSON Web Token (JWT) headers. Specifically, it enables the SDK to validate assertion signatures using either JSON Web Keys (JWK) or X.509 certificate chains ( Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances assertion verification by adding support for JWK and X.509 certificates from the JWT header, with a fallback to configured keys. The implementation is mostly sound, but I've identified a few areas for improvement. There's a bug in the exception handling within Manifest.java that could lead to incorrect error propagation. I've also suggested a simplification for creating a JWS verifier to improve maintainability. Most importantly, the new verification paths that use jwk and x5c from the JWT header are not covered by tests, which is a critical gap. Please add tests to cover these new code paths.
Signed-off-by: Scott Hamrick <2623452+cshamrick@users.noreply.github.com>
Signed-off-by: Scott Hamrick <2623452+cshamrick@users.noreply.github.com>
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request adds support for verifying TDF assertions using JWK and X.509 certificates from the JWT header, which is a great enhancement. The changes are well-structured and include corresponding tests.
However, I've identified a few areas for improvement:
- A critical security vulnerability in the handling of X.509 certificates from the
x5cheader, where the certificate chain is not validated. - A potential for an unhandled exception when processing non-RSA keys from a
jwkheader. - An opportunity to simplify and improve the robustness of JWK string generation by using the existing
nimbus-jose-jwtlibrary features.
My review includes specific comments and suggestions to address these points.
| // Check for X.509 certificate chain in header | ||
| if (verifier == null && header.getX509CertChain() != null && !header.getX509CertChain().isEmpty()) { | ||
| try { | ||
| X509Certificate cert = X509CertUtils.parse(header.getX509CertChain().get(0).decode()); | ||
| if (cert.getPublicKey() instanceof RSAPublicKey) { | ||
| verifier = createVerifier((RSAPublicKey) cert.getPublicKey()); | ||
| } else { | ||
| throw new SDKException("Unsupported public key type in X.509 certificate"); | ||
| } | ||
| } catch (IllegalArgumentException e) { | ||
| throw new SDKException("Invalid Base64 in X.509 certificate in JWT header", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation for handling the x5c header in the JWT simply extracts the public key from the first certificate in the chain without performing any trust validation. This is a significant security vulnerability. An attacker could forge a JWT, sign it with their own key, and embed the corresponding public key within the x5c header. The system would then trust this key and validate the signature, effectively bypassing all trust checks.
The certificate chain provided in x5c should be validated against a configured trust store (containing trusted root or intermediate CAs) before its public key is used for signature verification.
I am not providing a code suggestion as a full implementation would require significant changes, including setting up a trust store. However, this vulnerability must be addressed. A possible approach would involve using java.security.cert.CertPathValidator.
| // Check for JWK in header | ||
| if (header.getJWK() != null) { | ||
| try { | ||
| verifier = createVerifier(header.getJWK()); | ||
| } catch (JOSEException e) { | ||
| throw new SDKException("Invalid JWK in JWT header", e); | ||
| } | ||
| } | ||
|
|
||
| // Check for X.509 certificate chain in header |
| } | ||
|
|
||
| @Test | ||
| void testVerifyAssertionWithX5cHeaderInJWT() throws Exception { |
There was a problem hiding this comment.
I was hoping the JOSE API would support a x5c chain more gracefully
Signed-off-by: Scott Hamrick <2623452+cshamrick@users.noreply.github.com>
Signed-off-by: Scott Hamrick <2623452+cshamrick@users.noreply.github.com>
Signed-off-by: Scott Hamrick <2623452+cshamrick@users.noreply.github.com>
Signed-off-by: Scott Hamrick <2623452+cshamrick@users.noreply.github.com>
Signed-off-by: Scott Hamrick <2623452+cshamrick@users.noreply.github.com>
Signed-off-by: Scott Hamrick <2623452+cshamrick@users.noreply.github.com>
|
X-Test Failure Report✅ js-v0.4.34 |
X-Test Failure Report✅ js-v0.4.34 |
This PR adds support for verifying TDF assertions using JWK and X.509 certificates embedded in the JWT header.
CryptoUtils.java: A newgetPublicKeyJWKmethod was added to convert an RSA public key into a JWK string.Manifest.java: The assertion verification logic was updated. It now checks for and uses jwk and x5c (X.509 certificate chain) headers within the JWT to verify signatures before falling back to the previous verification method.TDF.java: Exception handling was updated to catchCertificateException.TDFTest.java: New tests,testSimpleTDFWithAssertionWithJWKandtestSimpleTDFWithAssertionWithX5C, were added to validate the new verification flows.TestUtil.java: AcreateTestCertificatemethod was added to generate self-signed X.509 certificates for testing.