[backend] fix: decode opencti jwt (#4973)#4981
Conversation
Signed-off-by: Marine LM <marine.lemezo@filigran.io>
Signed-off-by: Marine LM <marine.lemezo@filigran.io>
Signed-off-by: Marine LM <marine.lemezo@filigran.io>
There was a problem hiding this comment.
Pull request overview
This pull request implements JWT authentication support for OpenCTI connector callbacks to resolve issue #4973, where the OpenCTI worker's new security system was causing 401 errors when calling OpenAEV. The implementation adds a new security filter that validates Ed25519-signed JWTs from OpenCTI using JWKS (JSON Web Key Set) obtained during connector registration/ping operations.
Changes:
- Adds JWT authentication filter for
/api/stix/**endpoints using OpenCTI's JWKS for signature verification - Extends connector registration/ping GraphQL mutations to retrieve and store JWKS from OpenCTI
- Implements admin session creation for authenticated OpenCTI requests
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Adds Google Tink dependency (appears unused) |
| openaev-api/src/main/java/io/openaev/service/UserService.java | Adds createAdminSession method to establish admin security context |
| openaev-api/src/main/java/io/openaev/security/OpenCTIJwtAuthenticationFilter.java | New security filter implementing JWT validation for OpenCTI requests |
| openaev-api/src/main/java/io/openaev/opencti/connectors/service/OpenCTIConnectorService.java | Updates connector registration/ping to store JWKS responses |
| openaev-api/src/main/java/io/openaev/opencti/connectors/impl/SecurityCoverageConnector.java | Adds JWKS storage field to connector |
| openaev-api/src/main/java/io/openaev/opencti/client/mutations/RegisterConnector.java | Adds jwks field to GraphQL mutation response |
| openaev-api/src/main/java/io/openaev/opencti/client/mutations/Ping.java | Adds jwks field to GraphQL mutation response |
| openaev-api/src/main/java/io/openaev/config/AppSecurityConfig.java | Registers OpenCTI JWT filter in security filter chain |
| public class OpenCTIJwtAuthenticationFilter extends OncePerRequestFilter { | ||
| private UserService userService; | ||
| private OpenCTIConnectorService openCTIConnectorService; | ||
|
|
||
| @Autowired | ||
| public void setUserService(UserService userService) { | ||
| this.userService = userService; | ||
| } | ||
|
|
||
| @Autowired | ||
| public void setOpenCTIConnectorService(OpenCTIConnectorService openCTIConnectorService) { | ||
| this.openCTIConnectorService = openCTIConnectorService; | ||
| } | ||
|
|
||
| @Override | ||
| protected boolean shouldNotFilter(HttpServletRequest request) { | ||
| // only runs for /api/stix/** — skipped for everything else | ||
| return !request.getRequestURI().startsWith("/api/stix/"); | ||
| } | ||
|
|
||
| /** | ||
| * Function used to validate JWT token with OpenCTI jwks | ||
| * | ||
| * @param jwt JWT token to validate | ||
| * @throws Exception if token not valid | ||
| */ | ||
| public void validateOpenCTIJwt(String jwt) throws Exception { | ||
| Optional<ConnectorBase> openCTIConnector = openCTIConnectorService.getConnectorBase(); | ||
| if (openCTIConnector.isEmpty()) { | ||
| throw new ServletException("Connector not found"); | ||
| } | ||
|
|
||
| // Parse JWT first to extract the kid from header | ||
| SignedJWT signedJWT = SignedJWT.parse(jwt); | ||
| String kid = signedJWT.getHeader().getKeyID(); | ||
| if (kid == null) { | ||
| throw new Exception("JWT header does not contain a kid"); | ||
| } | ||
|
|
||
| // Parse JWKS and get jwk key by kid | ||
| String jwksJson = ((SecurityCoverageConnector) openCTIConnector.get()).getJwks(); | ||
| JWKSet jwkSet = JWKSet.parse(jwksJson); | ||
| JWK jwk = jwkSet.getKeyByKeyId(kid); | ||
| if (jwk == null) { | ||
| throw new Exception("No key found in JWKS for kid: " + kid); | ||
| } | ||
| if (!(jwk instanceof OctetKeyPair okpKey)) { | ||
| throw new Exception("Key with kid " + kid + " is not an OKP key"); | ||
| } | ||
|
|
||
| // Verify signature | ||
| Ed25519Verifier verifier = new Ed25519Verifier(okpKey.toPublicJWK()); | ||
| if (!signedJWT.verify(verifier)) { | ||
| throw new Exception("JWT signature verification failed"); | ||
| } | ||
|
|
||
| // Validate Expiration date | ||
| JWTClaimsSet claims = signedJWT.getJWTClaimsSet(); | ||
| if (claims.getExpirationTime() != null | ||
| && claims.getExpirationTime().toInstant().isBefore(Instant.now())) { | ||
| throw new Exception("JWT token has expired"); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| protected void doFilterInternal( | ||
| HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) | ||
| throws ServletException, IOException { | ||
|
|
||
| String authHeader = request.getHeader("Authorization"); | ||
| if (authHeader == null || !authHeader.startsWith("Bearer")) { | ||
| filterChain.doFilter(request, response); | ||
| return; | ||
| } | ||
| String token = authHeader.substring("Bearer ".length()).trim(); | ||
|
|
||
| try { | ||
| validateOpenCTIJwt(token); | ||
| this.userService.createAdminSession(); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
|
|
||
| filterChain.doFilter(request, response); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new OpenCTIJwtAuthenticationFilter is a critical security component that lacks test coverage. Given that the codebase has comprehensive tests for OpenCTIConnectorService and OpenCTIService, this filter should also have unit tests covering: (1) successful JWT validation and admin session creation, (2) handling of missing/invalid JWTs, (3) handling of expired JWTs, (4) behavior when JWKS is not available, (5) behavior when connector is not found, and (6) proper HTTP response codes for authentication failures.
| if (claims.getExpirationTime() != null | ||
| && claims.getExpirationTime().toInstant().isBefore(Instant.now())) { | ||
| throw new Exception("JWT token has expired"); | ||
| } |
There was a problem hiding this comment.
Missing null check for expiration time before comparison. While the code checks 'if (claims.getExpirationTime() != null)', it still performs the comparison even when it's null. The logic should only check expiration if it exists: if (claims.getExpirationTime() != null && claims.getExpirationTime().toInstant().isBefore(Instant.now())). However, the current code structure actually handles this correctly since it uses short-circuit evaluation with the && operator. The more significant issue is that JWTs without an expiration time are accepted, which may be a security concern - consider requiring expiration times.
| <groupId>com.google.crypto.tink</groupId> | ||
| <artifactId>tink</artifactId> | ||
| <version>1.20.0</version> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
The Google Tink dependency is added but never used in the codebase. JWT validation is performed using the Nimbus JOSE+JWT library (com.nimbusds.*) which is already available as a transitive dependency. This unused dependency should be removed to avoid unnecessary bloat and potential security vulnerabilities from unmaintained dependencies.
| <groupId>com.google.crypto.tink</groupId> | |
| <artifactId>tink</artifactId> | |
| <version>1.20.0</version> | |
| </dependency> | |
| <dependency> |
| } | ||
|
|
||
| // Parse JWKS and get jwk key by kid | ||
| String jwksJson = ((SecurityCoverageConnector) openCTIConnector.get()).getJwks(); |
There was a problem hiding this comment.
Unsafe type cast without validation. If the connector returned by getConnectorBase() is not a SecurityCoverageConnector instance, this will throw a ClassCastException. The code should verify the connector type before casting or use instanceof pattern matching to safely handle different connector types.
| String jwksJson = ((SecurityCoverageConnector) openCTIConnector.get()).getJwks(); | |
| ConnectorBase connectorBase = openCTIConnector.get(); | |
| if (!(connectorBase instanceof SecurityCoverageConnector securityCoverageConnector)) { | |
| throw new ServletException("OpenCTI connector is not a SecurityCoverageConnector"); | |
| } | |
| String jwksJson = securityCoverageConnector.getJwks(); |
| RegisterConnector.ResponsePayload payload = openCTIService.registerConnector(c); | ||
| ((SecurityCoverageConnector) c).setJwks(payload.getRegisterConnectorContent().getJwks()); |
There was a problem hiding this comment.
Unsafe type cast without validation. If the connector is not a SecurityCoverageConnector, this will throw a ClassCastException at runtime. The code should verify the connector type before casting, for example: if (openCTIConnector.get() instanceof SecurityCoverageConnector connector) { connector.setJwks(...); } else { log.warn("Unexpected connector type"); }
| /** Creates admin security session */ | ||
| public void createAdminSession() { | ||
| User adminUser = this.userRepository.findByEmailIgnoreCase(this.adminEmail).orElseThrow(); | ||
| this.createUserSession(adminUser); | ||
| } |
There was a problem hiding this comment.
Missing JavaDoc for newly exposed public method. The createAdminSession method is now public and used by security-critical code (OpenCTIJwtAuthenticationFilter), but lacks documentation explaining its purpose, when it should be used, and potential exceptions it may throw. Following the pattern of other methods in this service (see lines 112-117, 123-128, 143-146), this method should have a JavaDoc comment explaining that it creates a security session for the admin user based on the configured admin email, and that it throws NoSuchElementException if the admin user is not found.
| * @param jwt JWT token to validate | ||
| * @throws Exception if token not valid | ||
| */ | ||
| public void validateOpenCTIJwt(String jwt) throws Exception { |
There was a problem hiding this comment.
Generic exception types make debugging difficult. The validateOpenCTIJwt method throws generic 'Exception' which loses type information about what specifically went wrong. Consider creating specific exception types for different validation failures (e.g., JwtSignatureException, JwtExpiredException, JwksNotFoundException) or at minimum use existing JWT exception types from the Nimbus library.
| } | ||
|
|
||
| // Parse JWKS and get jwk key by kid | ||
| String jwksJson = ((SecurityCoverageConnector) openCTIConnector.get()).getJwks(); |
There was a problem hiding this comment.
Potential null pointer exception. The method getJwks() could return null if JWKS was never set on the SecurityCoverageConnector instance. This would cause JWKSet.parse() to fail. The code should verify that JWKS is not null before attempting to parse it, and provide a clear error message if it's missing.
| String jwksJson = ((SecurityCoverageConnector) openCTIConnector.get()).getJwks(); | |
| String jwksJson = ((SecurityCoverageConnector) openCTIConnector.get()).getJwks(); | |
| if (jwksJson == null || jwksJson.isBlank()) { | |
| throw new Exception("OpenCTI JWKS is not configured or is empty on the connector"); | |
| } |
| validateOpenCTIJwt(token); | ||
| this.userService.createAdminSession(); |
There was a problem hiding this comment.
Security concern: The filter creates an admin session for ALL valid OpenCTI JWT tokens without checking any claims in the JWT (such as subject, audience, or custom claims). This means any valid JWT signed by OpenCTI will grant admin access to OpenAEV. The code should validate JWT claims to ensure the token is specifically intended for OpenAEV access and contains appropriate authorization information before granting admin privileges.
| if (claims.getExpirationTime() != null | ||
| && claims.getExpirationTime().toInstant().isBefore(Instant.now())) { | ||
| throw new Exception("JWT token has expired"); | ||
| } |
There was a problem hiding this comment.
Missing validation of JWT issuer claim. The code validates the signature and expiration but doesn't check the 'iss' (issuer) claim to ensure the JWT was issued by the expected OpenCTI instance. This could allow JWTs from other OpenCTI instances to authenticate. Add validation: if (!expectedIssuer.equals(claims.getIssuer())) throw new Exception("Invalid issuer");
| } | |
| } | |
| // Validate issuer to ensure the token was issued by the expected OpenCTI instance | |
| String issuer = claims.getIssuer(); | |
| String expectedIssuer = ((SecurityCoverageConnector) openCTIConnector.get()).getOpenctiUrl(); | |
| if (issuer == null || issuer.isBlank()) { | |
| throw new Exception("JWT issuer is missing"); | |
| } | |
| if (expectedIssuer == null || !expectedIssuer.equals(issuer)) { | |
| throw new Exception("Invalid issuer"); | |
| } |
Signed-off-by: Marine LM <marine.lemezo@filigran.io>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4981 +/- ##
============================================
+ Coverage 55.75% 55.87% +0.11%
- Complexity 4402 4426 +24
============================================
Files 985 986 +1
Lines 29190 29245 +55
Branches 2137 2141 +4
============================================
+ Hits 16276 16340 +64
+ Misses 11973 11963 -10
- Partials 941 942 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Marine LM <marine.lemezo@filigran.io>
Signed-off-by: Marine LM <marine.lemezo@filigran.io>
Signed-off-by: Marine LM <marine.lemezo@filigran.io>
Signed-off-by: Marine LM <marine.lemezo@filigran.io>
| registerConnector(input: $input) { | ||
| id | ||
| connector_state | ||
| jwks |
There was a problem hiding this comment.
Documentation: Consider adding a comment explaining what the jwks field represents and why it's being retrieved. The field name "jwks" (JSON Web Key Set) might not be immediately clear to developers unfamiliar with JWT authentication. A brief comment like "// JSON Web Key Set for JWT signature verification" would improve code maintainability.
| private void applyJwksIfApplicable(ConnectorBase connector, String jwks) { | ||
| if (connector instanceof SecurityCoverageConnector scc) { | ||
| scc.setJwks(jwks); | ||
| } | ||
| } |
There was a problem hiding this comment.
API design consideration: The applyJwksIfApplicable method uses instanceof with pattern matching, which is a Java 21 feature. While this is fine given the project uses Java 21, consider whether this pattern should be extended to support other connector types that might need JWKS in the future. If only SecurityCoverageConnector will ever need JWKS, this is acceptable. Otherwise, consider adding a more generic interface method like setJwksIfSupported(String jwks) to ConnectorBase that can be overridden by connectors that support JWT authentication.
| if (authHeader == null || !authHeader.startsWith("Bearer")) { | ||
| filterChain.doFilter(request, response); | ||
| return; |
There was a problem hiding this comment.
The Bearer prefix check is case-sensitive. The existing TokenAuthenticationFilter uses case-insensitive checking (line 42: value.toLowerCase().startsWith(BEARER_PREFIX)) where BEARER_PREFIX is "bearer " in lowercase. This inconsistency means that requests with "bearer " (lowercase) will not be recognized by this filter, but would be by TokenAuthenticationFilter. Consider using case-insensitive checking for consistency.
| if (payload.getPingConnectorContent() != null) { | ||
| applyJwksIfApplicable(connector, payload.getPingConnectorContent().getJwks()); | ||
| } |
There was a problem hiding this comment.
Potential null pointer issue: If the OpenCTI connector ping response contains a null jwks field, this code will call setJwks(null). Later, when validateOpenCTIJwt is called, the parser will attempt to parse null as a JWKS string at line 60 in OpenCTIJwtAuthenticationFilter, which will throw an exception. Consider adding a null check before calling applyJwksIfApplicable or within the method to handle null jwks values gracefully.
| if (payload.getRegisterConnectorContent() != null) { | ||
| applyJwksIfApplicable(connector, payload.getRegisterConnectorContent().getJwks()); | ||
| } |
There was a problem hiding this comment.
Potential null pointer issue: If the OpenCTI connector registration/ping response contains a null jwks field, this code will call setJwks(null). Later, when validateOpenCTIJwt is called, the parser will attempt to parse null as a JWKS string at line 60, which will throw an exception. Consider adding a null check before calling applyJwksIfApplicable or within the method to handle null jwks values gracefully.
| @Override | ||
| protected void doFilterInternal( | ||
| HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) | ||
| throws ServletException, IOException { | ||
|
|
||
| String authHeader = request.getHeader("Authorization"); | ||
| if (authHeader == null || !authHeader.startsWith("Bearer")) { | ||
| filterChain.doFilter(request, response); | ||
| return; | ||
| } | ||
| String token = authHeader.substring("Bearer ".length()).trim(); | ||
|
|
||
| try { | ||
| validateOpenCTIJwt(token); | ||
| this.userService.createAdminSession(); | ||
| } catch (Exception e) { | ||
| response.sendError(HttpServletResponse.SC_UNAUTHORIZED, e.getMessage()); | ||
| return; | ||
| } | ||
|
|
||
| filterChain.doFilter(request, response); | ||
| } |
There was a problem hiding this comment.
Missing logging: The validateOpenCTIJwt method and doFilterInternal method don't log authentication attempts, successes, or failures. For security auditing and debugging purposes, consider adding log statements when JWT validation succeeds, when it fails (with appropriate details), and when authentication is bypassed. This follows security best practices and would be consistent with logging patterns in other parts of the application.
| * @param jwt JWT token to validate | ||
| * @throws Exception if token not valid | ||
| */ | ||
| public void validateOpenCTIJwt(String jwt) throws Exception { |
There was a problem hiding this comment.
The method declares "throws Exception" which is overly broad. According to the implementation, this method can throw ServletException when the connector is not found, and JwtException-related exceptions from the JWT parsing. Consider declaring the specific exceptions that can be thrown for better error handling and API clarity.
| "Given valid JWT should authorized"), | ||
| Arguments.of( | ||
| "Bearer " + expiredJwtJwk.jwtToken, | ||
| expiredJwtJwk.jwks, | ||
| false, | ||
| "Given expired valid JWT should authorized")); |
There was a problem hiding this comment.
Typo in test description: "Given expired valid JWT should authorized" should be "Given expired JWT should be unauthorized" or "Given expired JWT should not be authorized". The current message is grammatically incorrect and semantically confusing since it says "should authorized" when the expected result is isAuthorized=false.
| "Given valid JWT should authorized"), | |
| Arguments.of( | |
| "Bearer " + expiredJwtJwk.jwtToken, | |
| expiredJwtJwk.jwks, | |
| false, | |
| "Given expired valid JWT should authorized")); | |
| "Given valid JWT should be authorized"), | |
| Arguments.of( | |
| "Bearer " + expiredJwtJwk.jwtToken, | |
| expiredJwtJwk.jwks, | |
| false, | |
| "Given expired JWT should be unauthorized")); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ecurityCoverageConnector.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
# Conflicts: # openaev-api/src/main/java/io/openaev/config/AppSecurityConfig.java
Proposed changes
Related issues