Copilot commented on code in PR #4334:
URL: https://github.com/apache/solr/pull/4334#discussion_r3142270928
##########
solr/modules/jwt-auth/src/java/org/apache/solr/security/jwt/JWTAuthPlugin.java:
##########
@@ -561,150 +566,195 @@ protected JWTAuthenticationResponse authenticate(String
authorizationHeader) {
String jwtCompact = parseAuthorizationHeader(authorizationHeader);
if (jwtCompact != null) {
try {
+ // Pre-parse JWT (without signature verification) to get issuer and
check algorithm
+ SignedJWT signedJWT;
try {
- JwtClaims jwtClaims = jwtConsumer.processToClaims(jwtCompact);
- String principal = jwtClaims.getStringClaimValue(principalClaim);
- if (principal == null || principal.isEmpty()) {
- return new JWTAuthenticationResponse(
- AuthCode.PRINCIPAL_MISSING,
- "Cannot identify principal from JWT. Required claim "
- + principalClaim
- + " missing. Cannot authenticate");
+ signedJWT = SignedJWT.parse(jwtCompact);
+ } catch (ParseException e) {
+ if (e.getMessage() != null
+ && e.getMessage().contains("Invalid JOSE Compact
Serialization")) {
+ return new JWTAuthenticationResponse(AuthCode.JWT_PARSE_ERROR,
e.getMessage());
}
- if (claimsMatchCompiled != null) {
- for (Map.Entry<String, Pattern> entry :
claimsMatchCompiled.entrySet()) {
- String claim = entry.getKey();
- if (jwtClaims.hasClaim(claim)) {
- Object claimValue = jwtClaims.getClaimValue(claim);
- String claimValueStr = (claimValue != null) ?
String.valueOf(claimValue) : "";
- if (!entry.getValue().matcher(claimValueStr).matches()) {
- return new JWTAuthenticationResponse(
- AuthCode.CLAIM_MISMATCH,
- "Claim "
- + claim
- + "="
- + claimValueStr
- + " does not match required regular expression "
- + entry.getValue().pattern());
- }
- } else {
- return new JWTAuthenticationResponse(
- AuthCode.CLAIM_MISMATCH,
- "Claim " + claim + " is required but does not exist in
JWT");
- }
- }
+ return new JWTAuthenticationResponse(
+ AuthCode.JWT_PARSE_ERROR, "Failed to parse JWT: " +
e.getMessage());
+ }
+
+ // Check algorithm allowlist before full processing
+ if (algAllowlist != null
+ &&
!algAllowlist.contains(signedJWT.getHeader().getAlgorithm().getName())) {
+ return new JWTAuthenticationResponse(
+ AuthCode.JWT_VALIDATION_EXCEPTION,
+ new RuntimeException(
+ "Algorithm "
+ + signedJWT.getHeader().getAlgorithm()
+ + " is not a permitted algorithm"));
+ }
+
+ // Extract unverified issuer to pass to key selector via context
+ String tokenIssuer;
+ try {
+ tokenIssuer = signedJWT.getJWTClaimsSet().getIssuer();
+ } catch (ParseException e) {
+ return new JWTAuthenticationResponse(
+ AuthCode.JWT_PARSE_ERROR, "Malformed claim: " +
e.getMessage());
+ }
+
+ JWTClaimsSet jwtClaims;
+ try {
+ // Use the already-parsed SignedJWT to avoid redundant parsing and
ParseException
+ jwtClaims =
+ jwtProcessor.process(
+ signedJWT, new
IssuerAwareJWSKeySelector.IssuerContext(tokenIssuer));
+ } catch (BadJWSException e) {
+ return new JWTAuthenticationResponse(AuthCode.SIGNATURE_INVALID,
e);
+ } catch (BadJWTException e) {
+ String msg = e.getMessage();
+ if (msg != null && (msg.startsWith("Expired JWT") ||
msg.contains("expir"))) {
+ return new JWTAuthenticationResponse(
+ AuthCode.JWT_EXPIRED, "Authentication failed due to expired
JWT token. " + msg);
}
- if (!requiredScopes.isEmpty() && !jwtClaims.hasClaim(CLAIM_SCOPE))
{
- // Fail if we require scopes but they don't exist
+ return new
JWTAuthenticationResponse(AuthCode.JWT_VALIDATION_EXCEPTION, e);
+ } catch (BadJOSEException e) {
+ return new
JWTAuthenticationResponse(AuthCode.JWT_VALIDATION_EXCEPTION, e);
+ } catch (JOSEException e) {
+ return new
JWTAuthenticationResponse(AuthCode.JWT_VALIDATION_EXCEPTION, e);
+ }
+
+ // Validate issuer value against configured issuers (only when iss
is explicitly
+ // configured)
+ if (requireIssuer
+ && tokenIssuer != null
+ && issuerConfigs.stream().anyMatch(ic -> ic.getIss() != null)) {
+ boolean issuerMatched =
+ issuerConfigs.stream()
+ .anyMatch(ic -> ic.getIss() != null &&
ic.getIss().equals(tokenIssuer));
+ if (!issuerMatched) {
Review Comment:
Issuer value checking is currently gated on `requireIssuer` (and
`tokenIssuer != null`). With `requireIss=false`, a token that *does* contain an
`iss` claim that doesn't match any configured issuer can still be accepted
(especially when exactly one issuer is configured, since the key selector falls
back to that issuer). Previously, `requireIss` meant the claim was optional,
not that mismatching issuer values should be accepted when present. Consider
validating `iss` whenever at least one configured issuer has a non-null `iss`
(regardless of `requireIssuer`), only allowing a missing `iss` when
`requireIssuer` is false and a single issuer is configured.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
