Open
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for validating licenses signed with an x5c-provided X.509 certificate chain (anchored at a trusted root + required CN check), while keeping backward compatibility for legacy licenses signed directly with the existing public key.
Changes:
- Extend
LicenseCheckerto select the JWT verification key fromx5c(validated chain) or fall back to the legacy public key. - Introduce
X509Helperfor PEM/x5c parsing, CN extraction, and PKIX chain validation. - Update environment/config and tests to exercise the new
x5c-based license format (plus legacy license path).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/main/java/org/cryptomator/common/LicenseChecker.java |
Implements x5c chain verification and dynamic selection of the JWT signing key. |
src/main/java/org/cryptomator/common/X509Helper.java |
New helper for certificate parsing, CN extraction, and PKIX validation. |
src/main/java/org/cryptomator/common/Environment.java |
Adds a system property for configuring the required CN in the license certificate chain. |
src/test/java/org/cryptomator/common/LicenseCheckerTest.java |
Updates/extends tests for legacy and x5c-based licenses with new test certificates. |
Comments suppressed due to low confidence (1)
src/test/java/org/cryptomator/common/LicenseCheckerTest.java:60
- The new
x5c-based verification path isn’t directly covered by negative tests. Consider adding tests for (1) empty/invalidx5clist, (2) invalid base64/DER entry, (3) chain that validates to the root but is missing the required CN, and (4) leaf cert with non-EC public key, to ensurecheck()reliably returnsOptional.emptyfor each failure mode.
@Test
public void testCheckValidLicense() {
String license = "eyJ4NWMiOlsiTUlJQ0REQ0NBYjZnQXdJQkFnSVVETU96eEpZMzgxSmpkTjgyY1c3cWQrQjN2aDB3QlFZREsyVndNRTR4Q3pBSkJnTlZCQVlUQWtSRk1SWXdGQVlEVlFRS0RBMVRhM2x0WVhScFl5QkhiV0pJTVNjd0pRWURWUVFEREI1TWFXTmxibk5sSUVsdWRHVnliV1ZrYVdGMFpTQkRRU0FvVkdWemRDa3dIaGNOTWpZd01qSTJNVFUwTURNMldoY05Nell3TWpJME1UVTBNRE0yV2pCSU1Rc3dDUVlEVlFRR0V3SkVSVEVXTUJRR0ExVUVDZ3dOVTJ0NWJXRjBhV01nUjIxaVNERWhNQjhHQTFVRUF3d1lUR2xqWlc1elpTQkpjM04xWlhJZ1EwRWdLRlJsYzNRcE1JR2JNQkFHQnlxR1NNNDlBZ0VHQlN1QkJBQWpBNEdHQUFRQmlVVlEwSGhaTXVBT3FpTzJsUElUK01NU0g0YmNsNkJPV25GbjIwNWJ6VGNSSTlSdVJkdHJYVk53cFwvSVB0ak1WWFRqXC9vVzByMTJIY3JFZExtaTlRSTZRQVNURUJ5V0xOVFNcL2Q5NElvWG1SWVFUbkMrUnRIK0hcLzRJMVRXWXc5MGFpaWcyeVYwRzFzMHFDZ0FpeUtzd2orU1Q2cjcxTk1cL2dlcG1sVzMrcWl2OVwvUFdqUWpCQU1CMEdBMVVkRGdRV0JCU05qQnd2K1wvaVlRdnBPT3F6MDJ1N3hhQVNTSVRBZkJnTlZIU01FR0RBV2dCUk1BU3NwMWtpYXdKbThZb0o2KzhcL3NxMjFYNHpBRkJnTXJaWEFEUVFBNE9rXC8reTBiZHptMlJVbWtIZDZRRlM2V2JCS2Y5TzR6ejNVYzdpQk1wS0lxMWtCbHErN1RiYmdNSEp1K2FZYk9EY1JXVCsrNXN4NGkyT3Nwa2dPc0oiLCJNSUlCdFRDQ0FXZWdBd0lCQWdJVUpteDhVcXRnbktjYXVRYnFiMDRUYVVCRytVSXdCUVlESzJWd01EOHhDekFKQmdOVkJBWVRBa1JGTVJZd0ZBWURWUVFLREExVGEzbHRZWFJwWXlCSGJXSklNUmd3RmdZRFZRUUREQTlNYVdObGJuTmxJRlJsYzNRZ1EwRXdIaGNOTWpZd01qSTJNVFV6TmpNMldoY05Nell3TWpJME1UVXpOak0yV2pCT01Rc3dDUVlEVlFRR0V3SkVSVEVXTUJRR0ExVUVDZ3dOVTJ0NWJXRjBhV01nUjIxaVNERW5NQ1VHQTFVRUF3d2VUR2xqWlc1elpTQkpiblJsY20xbFpHbGhkR1VnUTBFZ0tGUmxjM1FwTUNvd0JRWURLMlZ3QXlFQUczdnI4Tks3WVpzMXE2cFF0SmhIRGJUMnhNRDNDMnlzbXNuZW13MUZRbGFqWmpCa01CSUdBMVVkRXdFQlwvd1FJTUFZQkFmOENBUUF3RGdZRFZSMFBBUUhcL0JBUURBZ0VHTUIwR0ExVWREZ1FXQkJSTUFTc3Axa2lhd0ptOFlvSjYrOFwvc3EyMVg0ekFmQmdOVkhTTUVHREFXZ0JRQmlPK3lxTVRhZzU5ZUg4ZmdjRjlnTXNYZUV6QUZCZ01yWlhBRFFRQktkR0cybmpWa3JqMEwxbWVXVTROcGxaZHlHUTJxYUNxNFBSenk1OUg5WW1EUzc5M1JsTWE0TU9ad0FtVGtVUTV2YWt1dnR6MTM0SWl6NmpKa0RCZ0YiXSwiYWxnIjoiRVM1MTIifQ.eyJhdWQiOiJDcnlwdG9tYXRvciIsInN1YiI6ImNyeXB0b2JvdEBleGFtcGxlLmNvbSIsImtpZCI6IllFVjFFSXY5dllMR1lfR1RoQ0VPTGtleW1xUDBlZVhVVTdNbWxKaHFnR0EiLCJpc3MiOiJodHRwOlwvXC9sb2NhbGhvc3Q6ODA4MVwvIiwiZXhwIjozMjUwMzY4MDAwMCwiaWF0Ijo5NDY2ODQ4MDAsInNlYXRzIjozfQ.AMBwG0CnIu8FHLsmynfT-JHwE-pwjXGh8SoExaYWY6n88cL18gzI_tYl3cjZSvLVZImg-VRZi7SkhDVt5zrOXX12AdNst1jJc0HJIA0dJUiM22b3XmkIjionTXmK-Njgp9XIQV4qtYoFPXjsL4KKTa95yx5oTvDD7ZAHFxur5_nN3y4t";
Optional<DecodedJWT> decoded = licenseChecker.check(license);
Assertions.assertTrue(decoded.isPresent());
Assertions.assertEquals("cryptobot@example.com", decoded.get().getSubject());
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+95
to
+109
| private List<X509Certificate> verifyChain(Claim x5cClaim) throws GeneralSecurityException { | ||
| List<String> x5cEntries = x5cClaim.asList(String.class); | ||
| if (x5cEntries == null || x5cEntries.isEmpty()) { | ||
| throw new CertPathValidatorException("x5c claim is empty."); | ||
| } | ||
| List<X509Certificate> certChain = X509Helper.parseX5cCertificateChain(x5cEntries); | ||
| boolean containsRequiredCn = certChain.stream() // | ||
| .flatMap(cert -> X509Helper.extractCommonName(cert).stream()) // | ||
| .anyMatch(requiredChainCn::equals); | ||
| if (!containsRequiredCn) { | ||
| throw new CertPathValidatorException("x5c certificate chain does not contain required CN " + requiredChainCn); | ||
| } | ||
| X509Helper.validateChain(certChain, rootCertificate); | ||
| return certChain; | ||
| } |
There was a problem hiding this comment.
verifyChain()/parseX5cCertificateChain() can throw unchecked IllegalArgumentException (e.g., invalid Base64 in x5c entries or invalid DER in parseDerCertificate). These exceptions are not caught in check() (which only catches JWTVerificationException | GeneralSecurityException), so a crafted token could crash callers instead of returning Optional.empty. Consider catching IllegalArgumentException (and potentially other runtime parse exceptions) in check() or wrapping them into a GeneralSecurityException in verifyChain().
Comment on lines
4
to
8
| import com.auth0.jwt.algorithms.Algorithm; | ||
| import com.auth0.jwt.exceptions.JWTDecodeException; | ||
| import com.auth0.jwt.exceptions.JWTVerificationException; | ||
| import com.auth0.jwt.interfaces.Claim; | ||
| import com.auth0.jwt.interfaces.DecodedJWT; |
There was a problem hiding this comment.
JWTDecodeException is imported but not used. If you implement decoding error handling (recommended), use it in the catch; otherwise remove the unused import to keep the file clean and avoid potential build/lint failures in stricter configurations.
WalkthroughThis PR adds support for certificate chain-based license verification. It introduces a new configuration property for specifying the required certificate chain CN, a utility class for X.509 certificate operations, and refactors LicenseChecker to dynamically validate certificate chains from license key headers, with fallback to the legacy EC public key approach. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/common/X509Helper.java (1)
62-67: Use a real DN parser for CN extraction.Lines 73-110 only handle comma splitting and simple backslash escapes. They miss valid DN forms like multi-valued RDNs (
CN=...+OU=...) and hex-escaped values, so a perfectly valid reissued certificate can stop matchingrequiredChainCn.LdapName/Rdnis safer on this authorization path.Also applies to: 73-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/common/X509Helper.java` around lines 62 - 67, The current extractCommonName implementation in X509Helper (and its helpers splitRdns/unescapeRdnValue) manually splits the RFC2253 name and fails for multi-valued RDNs and hex-escaped values; replace the manual parsing with javax.naming.ldap.LdapName and javax.naming.ldap.Rdn: create a new LdapName(cert.getSubjectX500Principal().getName(X500Principal.RFC2253)), iterate its Rdn elements and for each Rdn call getType()/getValue() (or parse multi-valued Rdn via Rdn#toAttributes) to locate the "CN" attribute, returning Optional.of(value.toString()) when found, and remove splitRdns/unescapeRdnValue usage in extractCommonName. Ensure any thrown NamingException is caught and treated as Optional.empty().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/cryptomator/common/LicenseChecker.java`:
- Around line 95-107: verifyChain currently calls
X509Helper.parseX5cCertificateChain which can throw IllegalArgumentException for
malformed Base64/DER and that exception escapes the outer check() catcher;
update verifyChain to catch IllegalArgumentException around the call to
X509Helper.parseX5cCertificateChain (in method verifyChain) and convert it into
a CertPathValidatorException or another GeneralSecurityException with a clear
message (including the original exception) so the outer check() (which catches
JWTVerificationException | GeneralSecurityException) will handle malformed x5c
input and return Optional.empty().
In `@src/test/java/org/cryptomator/common/LicenseCheckerTest.java`:
- Line 44: The JWT string literals in LicenseCheckerTest (e.g., the local
variable license and the other JWT fixtures used in this test) are intentional
test data and must be annotated so secret scanners ignore them; update the test
to mark each JWT fixture with the repository's gitleaks pragma comment (or add
equivalent repo-level allowlist entries) immediately adjacent to the literals in
LicenseCheckerTest so static analysis no longer flags the inline JWTs as
secrets.
---
Nitpick comments:
In `@src/main/java/org/cryptomator/common/X509Helper.java`:
- Around line 62-67: The current extractCommonName implementation in X509Helper
(and its helpers splitRdns/unescapeRdnValue) manually splits the RFC2253 name
and fails for multi-valued RDNs and hex-escaped values; replace the manual
parsing with javax.naming.ldap.LdapName and javax.naming.ldap.Rdn: create a new
LdapName(cert.getSubjectX500Principal().getName(X500Principal.RFC2253)), iterate
its Rdn elements and for each Rdn call getType()/getValue() (or parse
multi-valued Rdn via Rdn#toAttributes) to locate the "CN" attribute, returning
Optional.of(value.toString()) when found, and remove splitRdns/unescapeRdnValue
usage in extractCommonName. Ensure any thrown NamingException is caught and
treated as Optional.empty().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92fc5f9b-4935-4ffe-94bb-04fa56c71b63
📒 Files selected for processing (4)
src/main/java/org/cryptomator/common/Environment.javasrc/main/java/org/cryptomator/common/LicenseChecker.javasrc/main/java/org/cryptomator/common/X509Helper.javasrc/test/java/org/cryptomator/common/LicenseCheckerTest.java
Comment on lines
+95
to
+107
| private List<X509Certificate> verifyChain(Claim x5cClaim) throws GeneralSecurityException { | ||
| List<String> x5cEntries = x5cClaim.asList(String.class); | ||
| if (x5cEntries == null || x5cEntries.isEmpty()) { | ||
| throw new CertPathValidatorException("x5c claim is empty."); | ||
| } | ||
| List<X509Certificate> certChain = X509Helper.parseX5cCertificateChain(x5cEntries); | ||
| boolean containsRequiredCn = certChain.stream() // | ||
| .flatMap(cert -> X509Helper.extractCommonName(cert).stream()) // | ||
| .anyMatch(requiredChainCn::equals); | ||
| if (!containsRequiredCn) { | ||
| throw new CertPathValidatorException("x5c certificate chain does not contain required CN " + requiredChainCn); | ||
| } | ||
| X509Helper.validateChain(certChain, rootCertificate); |
There was a problem hiding this comment.
Normalize malformed x5c input into the checked failure path.
X509Helper.parseX5cCertificateChain() can throw IllegalArgumentException for bad Base64/DER on Line 100. Because check() only catches JWTVerificationException | GeneralSecurityException on Line 90, a corrupt x5c header currently escapes instead of returning Optional.empty().
🛡️ Suggested fix
private List<X509Certificate> verifyChain(Claim x5cClaim) throws GeneralSecurityException {
- List<String> x5cEntries = x5cClaim.asList(String.class);
- if (x5cEntries == null || x5cEntries.isEmpty()) {
- throw new CertPathValidatorException("x5c claim is empty.");
- }
- List<X509Certificate> certChain = X509Helper.parseX5cCertificateChain(x5cEntries);
- boolean containsRequiredCn = certChain.stream() //
- .flatMap(cert -> X509Helper.extractCommonName(cert).stream()) //
- .anyMatch(requiredChainCn::equals);
- if (!containsRequiredCn) {
- throw new CertPathValidatorException("x5c certificate chain does not contain required CN " + requiredChainCn);
- }
- X509Helper.validateChain(certChain, rootCertificate);
- return certChain;
+ try {
+ List<String> x5cEntries = x5cClaim.asList(String.class);
+ if (x5cEntries == null || x5cEntries.isEmpty()) {
+ throw new CertPathValidatorException("x5c claim is empty.");
+ }
+ List<X509Certificate> certChain = X509Helper.parseX5cCertificateChain(x5cEntries);
+ boolean containsRequiredCn = certChain.stream() //
+ .flatMap(cert -> X509Helper.extractCommonName(cert).stream()) //
+ .anyMatch(requiredChainCn::equals);
+ if (!containsRequiredCn) {
+ throw new CertPathValidatorException("x5c certificate chain does not contain required CN " + requiredChainCn);
+ }
+ X509Helper.validateChain(certChain, rootCertificate);
+ return certChain;
+ } catch (IllegalArgumentException e) {
+ throw new CertPathValidatorException("Invalid x5c certificate chain.", e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private List<X509Certificate> verifyChain(Claim x5cClaim) throws GeneralSecurityException { | |
| List<String> x5cEntries = x5cClaim.asList(String.class); | |
| if (x5cEntries == null || x5cEntries.isEmpty()) { | |
| throw new CertPathValidatorException("x5c claim is empty."); | |
| } | |
| List<X509Certificate> certChain = X509Helper.parseX5cCertificateChain(x5cEntries); | |
| boolean containsRequiredCn = certChain.stream() // | |
| .flatMap(cert -> X509Helper.extractCommonName(cert).stream()) // | |
| .anyMatch(requiredChainCn::equals); | |
| if (!containsRequiredCn) { | |
| throw new CertPathValidatorException("x5c certificate chain does not contain required CN " + requiredChainCn); | |
| } | |
| X509Helper.validateChain(certChain, rootCertificate); | |
| private List<X509Certificate> verifyChain(Claim x5cClaim) throws GeneralSecurityException { | |
| try { | |
| List<String> x5cEntries = x5cClaim.asList(String.class); | |
| if (x5cEntries == null || x5cEntries.isEmpty()) { | |
| throw new CertPathValidatorException("x5c claim is empty."); | |
| } | |
| List<X509Certificate> certChain = X509Helper.parseX5cCertificateChain(x5cEntries); | |
| boolean containsRequiredCn = certChain.stream() // | |
| .flatMap(cert -> X509Helper.extractCommonName(cert).stream()) // | |
| .anyMatch(requiredChainCn::equals); | |
| if (!containsRequiredCn) { | |
| throw new CertPathValidatorException("x5c certificate chain does not contain required CN " + requiredChainCn); | |
| } | |
| X509Helper.validateChain(certChain, rootCertificate); | |
| return certChain; | |
| } catch (IllegalArgumentException e) { | |
| throw new CertPathValidatorException("Invalid x5c certificate chain.", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/cryptomator/common/LicenseChecker.java` around lines 95 -
107, verifyChain currently calls X509Helper.parseX5cCertificateChain which can
throw IllegalArgumentException for malformed Base64/DER and that exception
escapes the outer check() catcher; update verifyChain to catch
IllegalArgumentException around the call to X509Helper.parseX5cCertificateChain
(in method verifyChain) and convert it into a CertPathValidatorException or
another GeneralSecurityException with a clear message (including the original
exception) so the outer check() (which catches JWTVerificationException |
GeneralSecurityException) will handle malformed x5c input and return
Optional.empty().
|
|
||
| @Test | ||
| public void testCheckValidLegacyLicense() { | ||
| String license = "eyJhbGciOiJFUzUxMiJ9.eyJhdWQiOiJDcnlwdG9tYXRvciIsInN1YiI6ImNyeXB0b2JvdEBleGFtcGxlLmNvbSIsImtpZCI6IllFVjFFSXY5dllMR1lfR1RoQ0VPTGtleW1xUDBlZVhVVTdNbWxKaHFnR0EiLCJpc3MiOiJodHRwOlwvXC9sb2NhbGhvc3Q6ODA4MVwvIiwiZXhwIjozMjUwMzY4MDAwMCwiaWF0Ijo5NDY2ODQ4MDAsInNlYXRzIjozfQ.ATHMZ95Z3cx-ghYvbT9XBT-Z-c4BWJOK6WM3JbOjCzIO-3pddYpey0uAfMipIfLzHZypE4NPtNR4nB8z7JRGcqj8AMBpXBxZnDGXsftGHkOuHw6kTq2b-HPmcYBKFZ4hH7ptQRS9byaypFc7ftonLyYODWqar6DXtHSJtlf01jnuAdVi"; |
There was a problem hiding this comment.
Mark these JWT fixtures as intentional test data.
Static analysis is already flagging the literals on Lines 44, 54, and 82 as secrets. If they stay inline without the repo’s Gitleaks allowlist/pragma, every secret scan on this file will keep producing noisy high-severity findings and may block CI.
🔐 Minimal fix
+ // gitleaks:allow
String license = "...";Apply the pragma to each intentional JWT fixture, or add the equivalent repo-level allowlist entry.
Also applies to: 54-54, 82-82
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 44-44: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/cryptomator/common/LicenseCheckerTest.java` at line 44, The
JWT string literals in LicenseCheckerTest (e.g., the local variable license and
the other JWT fixtures used in this test) are intentional test data and must be
annotated so secret scanners ignore them; update the test to mark each JWT
fixture with the repository's gitleaks pragma comment (or add equivalent
repo-level allowlist entries) immediately adjacent to the literals in
LicenseCheckerTest so static analysis no longer flags the inline JWTs as
secrets.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.