This is an automated email from the ASF dual-hosted git repository. smolnar82 pushed a commit to branch knox_idf in repository https://gitbox.apache.org/repos/asf/knox.git
commit d9b1453d2895cd2fe75a1c60093b4935de4f495e Author: Sandor Molnar <[email protected]> AuthorDate: Mon Aug 10 20:28:14 2026 +0200 KNOX-3414: security hardening for the OIDC provider (Tier 1 + Tier 2) Fixes a batch of security and correctness findings from reviewing the squashed "Knox as OIDC Provider" feature. Reviewed and tested together. Tier 1 (critical): - 1.1 Authenticate the client on the auth-code token exchange. The JWTFederationFilter Bearer path forwards to the token endpoint without checking client_secret, so a stolen code could be redeemed by anyone holding some valid Knox JWT. The endpoint now independently binds redemption to the client: PKCE code_verifier (S256) when a challenge was stored, else a constant-time client_secret check. - 1.2 Validate the federated id_token (signature via the OP JWKS, expected issuer, audience, exp/nbf) before trusting any claim; fail closed when jwks.endpoint/issuer are not configured. - 1.3 Add knoxidf.client.registration.anonymous.allowed (default false): dynamic client registration refuses anonymous callers unless explicitly enabled. Sample knoxidf-ldap topology opts in to preserve open reg. - 1.4 Stop leaking custom claims across users: build a per-request copy of the claim map instead of mutating the shared singleton field. Tier 2 (high): - 2.1 Default issueTime to now in JWTokenAttributesBuilder so every token gets a correct iat (fixes iat=1970 on KnoxSSO cookies/assertions). - 2.2 Set SCOPE_ATTRIBUTE to the scope value (was a double getClaim -> null). - 2.3 Match redirect_uri on parsed URI components with a path/host boundary (fixes wildcard open-redirect via startsWith). - 2.5 Require S256 PKCE; reject plain. - 2.6 Use the Knox truststore for federated OP HTTP calls. - 2.7 Treat auto_consent as a server-side policy, not a client bypass. - 2.8 Escape username in LDAP DN/filter (Rdn.escapeValue). - 2.9 Require and validate state on the authorize flow. - 2.10 Null-guard the federated callback / registration and return 4xx. Tests: TokenResourceClientAuthTest, RegistrationResourceAnonymousGuardTest, FederatedOpConfigurationTest, JWTTokenTest iat assertion. Deferred to follow-up PRs: 1.5 (secrets at rest via AliasService), 2.4 + 2.11 (atomic single-use auth-code consume + schema migration across dialects; the auth-code replay window remains until 2.4 lands), Tier 3/4. Co-Authored-By: Claude Opus 4.8 <[email protected]> --- .../build/conf/topologies/knoxidf-ldap.xml | 7 + .../federation/jwt/filter/JWTFederationFilter.java | 2 +- .../gateway/service/knoxidf/AuthorizeResource.java | 151 +++++++++++++++++++-- .../service/knoxidf/RegistrationResource.java | 32 +++++ .../gateway/service/knoxidf/TokenResource.java | 87 ++++++++++-- .../knoxidf/userparams/LdapUserParamsProvider.java | 6 +- .../RegistrationResourceAnonymousGuardTest.java | 78 +++++++++++ .../knoxidf/TokenResourceClientAuthTest.java | 116 ++++++++++++++++ .../gateway/service/knoxtoken/TokenResource.java | 10 +- .../security/token/JWTokenAttributesBuilder.java | 5 +- .../services/security/token/impl/JWTTokenTest.java | 28 ++++ .../util/knoxidf/AuthorizeRequestMetadata.java | 7 + .../util/knoxidf/FederatedOpConfiguration.java | 24 ++++ .../gateway/util/knoxidf/KnoxIDFConstants.java | 5 + .../util/knoxidf/FederatedOpConfigurationTest.java | 74 ++++++++++ 15 files changed, 603 insertions(+), 29 deletions(-) diff --git a/.github/workflows/build/conf/topologies/knoxidf-ldap.xml b/.github/workflows/build/conf/topologies/knoxidf-ldap.xml index d25bb8869..f3f795467 100644 --- a/.github/workflows/build/conf/topologies/knoxidf-ldap.xml +++ b/.github/workflows/build/conf/topologies/knoxidf-ldap.xml @@ -59,6 +59,13 @@ <name>knoxidf.knox.token.limit.per.user</name> <value>-1</value> </param> + <param> + <!-- This sample topology intentionally allows open, unauthenticated client + registration (the endpoint is wired as 'anon' above). Registration refuses + anonymous callers unless this is explicitly set to true. --> + <name>knoxidf.client.registration.anonymous.allowed</name> + <value>true</value> + </param> <param> <name>token.exchange.topology.name</name> <value>knoxidf-token</value> diff --git a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java index 5109a2c09..5825c38a6 100644 --- a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java +++ b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java @@ -258,7 +258,7 @@ public class JWTFederationFilter extends AbstractJWTFilter { request.setAttribute(KnoxIDFConstants.TOKEN_ID_ATTRIBUTE, TokenUtils.getTokenId(token)); final String scope = token.getClaim(KnoxIDFConstants.SCOPE); if (scope != null) { - request.setAttribute(KnoxIDFConstants.SCOPE_ATTRIBUTE, token.getClaim(scope)); + request.setAttribute(KnoxIDFConstants.SCOPE_ATTRIBUTE, scope); } final String issuer = token.getIssuer(); if (issuer != null) { diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java index 94415de19..4fd703c7a 100644 --- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java +++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResource.java @@ -34,11 +34,15 @@ import org.apache.knox.gateway.service.knoxtoken.PasscodeTokenResourceBase; import org.apache.knox.gateway.services.GatewayServices; import org.apache.knox.gateway.services.ServiceLifecycleException; import org.apache.knox.gateway.services.ServiceType; +import org.apache.http.ssl.SSLContexts; import org.apache.knox.gateway.services.knoxidf.federation.FederatedIdentity; import org.apache.knox.gateway.services.knoxidf.federation.FederatedIdentityService; import org.apache.knox.gateway.services.security.AliasServiceException; +import org.apache.knox.gateway.services.security.KeystoreService; +import org.apache.knox.gateway.services.security.token.JWTokenAuthority; import org.apache.knox.gateway.services.security.token.TokenMetadata; import org.apache.knox.gateway.services.security.token.TokenMetadataType; +import org.apache.knox.gateway.services.security.token.TokenServiceException; import org.apache.knox.gateway.services.security.token.UnknownTokenException; import org.apache.knox.gateway.services.security.token.impl.JWT; import org.apache.knox.gateway.services.security.token.impl.JWTToken; @@ -58,14 +62,17 @@ import javax.ws.rs.POST; import javax.ws.rs.Path; import javax.ws.rs.core.Context; import javax.ws.rs.core.Response; +import javax.net.ssl.SSLContext; import java.io.UnsupportedEncodingException; import java.net.URI; +import java.security.KeyStore; +import java.net.URISyntaxException; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; -import java.text.ParseException; import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -87,6 +94,7 @@ import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.DEFAULT_SCOP import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.FEDERATED_IDENTITY_ID; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.NONCE; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.OFFLINE_ACCESS_SCOPE; +import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.PKCE_METHOD_S256; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.REDIRECT_URI; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.REDIRECT_URIS; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.RESPONSE_TYPE; @@ -114,6 +122,7 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { private ServletContext servletContext; private FederatedIdentityService federatedIdentityService; + private boolean autoConsentEnabled; @PostConstruct @Override @@ -122,6 +131,10 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { this.authorizeRequestMetadataStore = AuthorizeRequestMetadataStore.getInstance(tokenTTL); final GatewayServices services = (GatewayServices) servletContext.getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE); federatedIdentityService = services.getService(ServiceType.KNOXIDF_FEDERATED_IDENTITY_SERVICE); + // Skipping user consent is a server-side deployment decision, never a client-supplied + // request parameter: a client must not be able to bypass the consent screen by sending + // auto_consent=true. + this.autoConsentEnabled = "true".equalsIgnoreCase(servletContext.getInitParameter("knoxidf.auto.consent.enabled")); } @Override @@ -159,7 +172,7 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { } if (!hasConsent(authorizeRequestMetadata)) { - if ("true".equalsIgnoreCase(request.getParameter("auto_consent"))) { + if (autoConsentEnabled) { markConsentAccepted(authorizeRequestMetadata); } else { final String consentAuthState = UUID.randomUUID().toString(); @@ -224,11 +237,29 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { //This is the callback for the federated OP final String federatedAuthCode = request.getParameter(CODE); final String state = request.getParameter(STATE); + if (StringUtils.isBlank(state) || StringUtils.isBlank(federatedAuthCode)) { + return error("invalid_request", "Missing state or code"); + } final AuthorizeRequestMetadata authorizeRequestMetadata = authorizeRequestMetadataStore.get(state); - //at this point, there has to be exactly 1 federated OP config - final FederatedOpConfiguration federatedOpConfiguration = federatedOpConfigurationStore.get(state).stream().findFirst().get(); + if (authorizeRequestMetadata == null) { + return error("invalid_request", "Unknown or expired state"); + } + final Set<FederatedOpConfiguration> opConfigs = federatedOpConfigurationStore.get(state); + final FederatedOpConfiguration federatedOpConfiguration = opConfigs == null ? null : opConfigs.stream().findFirst().orElse(null); + if (federatedOpConfiguration == null) { + return error("invalid_request", "No federated OP configuration available for the request"); + } final Pair<String, String> federatedTokens = exchangeFederatedAuthCodeToTokens(federatedAuthCode, federatedOpConfiguration); - final FederatedIdentity federatedIdentity = resolveFederatedIdentity(federatedTokens.getLeft(), federatedOpConfiguration.getName()); + if (StringUtils.isBlank(federatedTokens.getLeft())) { + return error("invalid_request", "Federated OP did not return an id_token"); + } + final JWT federatedIdToken = new JWTToken(federatedTokens.getLeft()); + // Verify the OP's id_token (signature/issuer/audience/expiry) before trusting any claim in it. + final Response validationError = validateFederatedIdToken(federatedIdToken, federatedOpConfiguration); + if (validationError != null) { + return validationError; + } + final FederatedIdentity federatedIdentity = resolveFederatedIdentity(federatedIdToken, federatedOpConfiguration.getName()); return getAuthCodeFromKnox(authorizeRequestMetadata, Pair.of(federatedIdentity.getId(), federatedTokens.getRight())); } @@ -272,7 +303,8 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { } if (StringUtils.isNotBlank(authorizeRequestMetadata.getCodeChallenge())) { authCodeTokenMap.put(CODE_CHALLENGE, authorizeRequestMetadata.getCodeChallenge()); - authCodeTokenMap.put(CODE_CHALLENGE_METHOD, StringUtils.defaultIfBlank(authorizeRequestMetadata.getCodeChallengeMethod(), "plain")); + // Method is validated to be S256 in verifyParams; store it as-is (no 'plain' default). + authCodeTokenMap.put(CODE_CHALLENGE_METHOD, authorizeRequestMetadata.getCodeChallengeMethod()); } if (federatedTokens != null) { authCodeTokenMap.put(FEDERATED_IDENTITY_ID, federatedTokens.getLeft()); @@ -313,17 +345,35 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { return error("invalid_scope", "One or more requested scopes are not allowed"); } + // PKCE: only the S256 challenge method is supported. 'plain' (and an unspecified method, + // which OAuth would default to 'plain') offers no protection and is rejected. + if (StringUtils.isNotBlank(authorizeRequestMetadata.getCodeChallenge()) + && !PKCE_METHOD_S256.equals(authorizeRequestMetadata.getCodeChallengeMethod())) { + return error("invalid_request", "Unsupported code_challenge_method; only S256 is supported"); + } + return null; } return basicVerificationResponse; } private boolean matchesRedirectUri(String requestedUri, Set<String> registeredUris) { + final URI requested = parseUri(requestedUri); + if (requested == null) { + return false; + } for (String registered : registeredUris) { if (registered.endsWith("*")) { - String prefix = registered.substring(0, registered.length() - 1); - if (requestedUri.startsWith(prefix)) { - return true; + // Wildcard is a path-prefix match, but the origin (scheme/host/port) must match + // exactly. Comparing parsed components prevents a bare startsWith from letting + // "https://good.example*"; match "https://good.example.evil.com";. + final URI base = parseUri(registered.substring(0, registered.length() - 1)); + if (base != null && sameOrigin(base, requested)) { + final String basePath = base.getPath() == null ? "" : base.getPath(); + final String reqPath = requested.getPath() == null ? "" : requested.getPath(); + if (reqPath.startsWith(basePath)) { + return true; + } } } else if (registered.equals(requestedUri)) { return true; @@ -332,6 +382,23 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { return false; } + private static URI parseUri(String value) { + if (StringUtils.isBlank(value)) { + return null; + } + try { + return new URI(value); + } catch (URISyntaxException e) { + return null; + } + } + + private static boolean sameOrigin(URI a, URI b) { + return a.getScheme() != null && a.getScheme().equalsIgnoreCase(b.getScheme()) + && a.getHost() != null && a.getHost().equalsIgnoreCase(b.getHost()) + && a.getPort() == b.getPort(); + } + private Pair<String, String> exchangeFederatedAuthCodeToTokens(String federatedAuthCode, FederatedOpConfiguration opConfig) { String federatedIdToken = null; String federatedAccessToken = null; @@ -354,7 +421,7 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { params.add(new BasicNameValuePair(CLIENT_ID, opConfig.getClientId())); params.add(new BasicNameValuePair(CLIENT_SECRET, opConfig.getClientSecret())); - try (CloseableHttpClient httpClient = HttpClients.createDefault()) { + try (CloseableHttpClient httpClient = createFederatedHttpClient()) { HttpPost post = new HttpPost(opConfig.getTokenEndpoint()); post.setHeader("Content-Type", "application/x-www-form-urlencoded"); post.setEntity(new UrlEncodedFormEntity(params, StandardCharsets.UTF_8)); @@ -369,8 +436,68 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { } } - private FederatedIdentity resolveFederatedIdentity(String federatedIdToken, String opName) throws ParseException { - final JWT jwt = new JWTToken(federatedIdToken); + /** + * Builds the HTTP client used for the back-channel token request to the federated OP. The + * OP's TLS certificate must be validated against the Gateway's configured truststore + * ({@code gateway.truststore.*}) rather than the process-wide default, so this mirrors the + * outbound-dispatch clients. When no Gateway truststore is configured we fall back to the + * default client (JVM default trust material), never to an unvalidated client. + */ + private CloseableHttpClient createFederatedHttpClient() throws Exception { + final KeystoreService keystoreService = getGatewayServices().getService(ServiceType.KEYSTORE_SERVICE); + final KeyStore trustStore = keystoreService.getTruststoreForHttpClient(); + if (trustStore != null) { + final SSLContext sslContext = SSLContexts.custom().loadTrustMaterial(trustStore, null).build(); + return HttpClients.custom().setSSLContext(sslContext).build(); + } + return HttpClients.createDefault(); + } + + /** + * Verifies the federated OP's id_token before any claim in it is trusted: + * <ul> + * <li>signature against the OP's JWKS and {@code exp}/{@code nbf} (via {@link JWTokenAuthority});</li> + * <li>{@code iss} equals the configured OP issuer;</li> + * <li>{@code aud} contains our client_id registered at the OP.</li> + * </ul> + * Fails closed: if the OP is not configured with a JWKS endpoint, expected issuer and client_id, + * the token cannot be verified and the federated login is refused. + * + * @return an error {@link Response} if verification fails, or {@code null} if the token is valid. + */ + private Response validateFederatedIdToken(final JWT idToken, final FederatedOpConfiguration opConfig) { + final String jwksEndpoint = opConfig.getJwksEndpoint(); + final String expectedIssuer = opConfig.getIssuer(); + final String expectedAudience = opConfig.getClientId(); + if (StringUtils.isBlank(jwksEndpoint) || StringUtils.isBlank(expectedIssuer) || StringUtils.isBlank(expectedAudience)) { + return error("invalid_request", "Federated OP is missing jwks.endpoint/issuer/clientId configuration; cannot verify id_token"); + } + + try { + final JWTokenAuthority authority = getGatewayServices().getService(ServiceType.TOKEN_SERVICE); + // Verifies the signature against the OP's JWKS and checks exp/nbf. + if (!authority.verifyToken(idToken, Collections.singleton(new URI(jwksEndpoint)), opConfig.getSignatureAlgorithm(), null)) { + return error("invalid_request", "Federated id_token signature or expiry verification failed"); + } + } catch (URISyntaxException e) { + return error("invalid_request", "Invalid jwks.endpoint configured for federated OP"); + } catch (TokenServiceException e) { + return error("invalid_request", "Federated id_token verification error"); + } + + if (!expectedIssuer.equals(idToken.getIssuer())) { + return error("invalid_request", "Federated id_token issuer mismatch"); + } + + final String[] audiences = idToken.getAudienceClaims(); + if (audiences == null || !Arrays.asList(audiences).contains(expectedAudience)) { + return error("invalid_request", "Federated id_token audience mismatch"); + } + + return null; + } + + private FederatedIdentity resolveFederatedIdentity(final JWT jwt, String opName) { final String issuer = jwt.getIssuer(); final String subject = jwt.getSubject(); return federatedIdentityService.findByProviderAndSubject(opName.toUpperCase(Locale.US), issuer, subject).orElseGet(() -> persistFederatedIdentity(jwt, opName)); diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/RegistrationResource.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/RegistrationResource.java index 05675e491..ed3ad7411 100644 --- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/RegistrationResource.java +++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/RegistrationResource.java @@ -18,6 +18,7 @@ package org.apache.knox.gateway.service.knoxidf; import com.nimbusds.jose.KeyLengthException; import org.apache.commons.lang3.StringUtils; +import org.apache.knox.gateway.security.SubjectUtils; import org.apache.knox.gateway.service.knoxtoken.ClientCredentialsResource; import org.apache.knox.gateway.services.ServiceLifecycleException; import org.apache.knox.gateway.services.security.AliasServiceException; @@ -25,12 +26,14 @@ import org.apache.knox.gateway.services.security.token.TokenMetadata; import org.glassfish.jersey.process.internal.RequestScoped; import javax.annotation.PostConstruct; +import javax.servlet.ServletContext; import javax.servlet.ServletException; import javax.ws.rs.Consumes; import javax.ws.rs.FormParam; import javax.ws.rs.GET; import javax.ws.rs.POST; import javax.ws.rs.Path; +import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import java.net.URI; @@ -41,6 +44,7 @@ import java.util.List; import java.util.Map; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.BASE_RESORCE_PATH; +import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CLIENT_REGISTRATION_ANONYMOUS_ALLOWED; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.DEFAULT_SCOPES; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFUtils.error; @@ -49,13 +53,22 @@ import static org.apache.knox.gateway.util.knoxidf.KnoxIDFUtils.error; public class RegistrationResource extends ClientCredentialsResource { static final String RESOURCE_PATH = BASE_RESORCE_PATH + "/client"; + private static final String ANONYMOUS_PRINCIPAL = "anonymous"; + private List<String> redirectUris; private List<String> allowedScopes; + boolean anonymousRegistrationAllowed; + + @Context + private ServletContext servletContext; @PostConstruct @Override public void init() throws ServletException, AliasServiceException, ServiceLifecycleException, KeyLengthException { super.init(); + // Secure by default: unless the deployment explicitly opts in, an anonymous caller cannot + // register a client even when the topology wires this endpoint as 'anon'. + this.anonymousRegistrationAllowed = Boolean.parseBoolean(servletContext.getInitParameter(CLIENT_REGISTRATION_ANONYMOUS_ALLOWED)); } @Override @@ -75,6 +88,13 @@ public class RegistrationResource extends ClientCredentialsResource { @Consumes(MediaType.APPLICATION_FORM_URLENCODED) public Response registerClient(@FormParam("redirect_uris") String redirectUris, @FormParam("allowed_scopes") String allowedScopes) { + if (anonymousRegistrationDenied()) { + return error("access_denied", "Anonymous client registration is disabled. Set '" + + CLIENT_REGISTRATION_ANONYMOUS_ALLOWED + "' to true in the KNOXIDF service configuration to enable it."); + } + if (StringUtils.isBlank(redirectUris)) { + return error("invalid_request", "redirect_uris must be provided"); + } this.redirectUris = Arrays.asList(redirectUris.split(",")); final Response redirectUriVerificationResponse = verifyRedirectUris(); if (redirectUriVerificationResponse != null) { @@ -92,6 +112,18 @@ public class RegistrationResource extends ClientCredentialsResource { return super.doPost(); } + /** + * @return {@code true} when the request must be rejected because an anonymous caller is + * attempting to register a client while open registration has not been explicitly enabled. + */ + boolean anonymousRegistrationDenied() { + return !anonymousRegistrationAllowed && isAnonymousCaller(); + } + + private boolean isAnonymousCaller() { + return ANONYMOUS_PRINCIPAL.equalsIgnoreCase(SubjectUtils.getCurrentEffectivePrincipalName()); + } + private Response verifyRedirectUris() { if (redirectUris == null || redirectUris.isEmpty()) { return error("invalid_request", "redirect_uris must be provided"); diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/TokenResource.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/TokenResource.java index 54e697eb9..97e24378c 100644 --- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/TokenResource.java +++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/TokenResource.java @@ -18,6 +18,7 @@ package org.apache.knox.gateway.service.knoxidf; import com.nimbusds.jose.KeyLengthException; import org.apache.commons.lang3.StringUtils; +import org.apache.knox.gateway.config.GatewayConfig; import org.apache.knox.gateway.service.knoxidf.userparams.UserParamsProvider; import org.apache.knox.gateway.service.knoxidf.userparams.UserParamsProviderFactory; import org.apache.knox.gateway.service.knoxtoken.PasscodeTokenResourceBase; @@ -26,6 +27,7 @@ import org.apache.knox.gateway.services.ServiceLifecycleException; import org.apache.knox.gateway.services.ServiceType; import org.apache.knox.gateway.services.knoxidf.federation.FederatedIdentity; import org.apache.knox.gateway.services.knoxidf.federation.FederatedIdentityService; +import org.apache.knox.gateway.services.security.AliasService; import org.apache.knox.gateway.services.security.AliasServiceException; import org.apache.knox.gateway.services.security.token.JWTokenAttributesBuilder; import org.apache.knox.gateway.services.security.token.JWTokenAuthority; @@ -35,6 +37,7 @@ import org.apache.knox.gateway.services.security.token.TokenServiceException; import org.apache.knox.gateway.services.security.token.TokenUtils; import org.apache.knox.gateway.services.security.token.UnknownTokenException; import org.apache.knox.gateway.services.security.token.impl.JWT; +import org.apache.knox.gateway.services.security.token.impl.TokenMAC; import org.apache.knox.gateway.util.ServletRequestUtils; import javax.annotation.PostConstruct; @@ -55,6 +58,7 @@ import java.util.Base64; import java.util.HashMap; import java.util.Map; +import static org.apache.knox.gateway.security.CommonTokenConstants.CLIENT_SECRET; import static org.apache.knox.gateway.security.CommonTokenConstants.GRANT_TYPE; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.AUTH_CODE; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.BASE_RESORCE_PATH; @@ -65,7 +69,6 @@ import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CODE_CHALLEN import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CODE_VERIFIER; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.FEDERATED_IDENTITY_ID; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.OFFLINE_ACCESS_SCOPE; -import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.PKCE_METHOD_PLAIN; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.PKCE_METHOD_S256; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.REDIRECT_URI; import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.REFRESH_TOKEN; @@ -88,6 +91,7 @@ public class TokenResource extends PasscodeTokenResourceBase { private FederatedIdentityService federatedIdentityService; private long refreshTokenTTL; + TokenMAC tokenMAC; @Override public String getPrefix() { @@ -102,6 +106,13 @@ public class TokenResource extends PasscodeTokenResourceBase { this.userParamsProvider = UserParamsProviderFactory.getUserParamsProvider(servletContext); final GatewayServices services = (GatewayServices) servletContext.getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE); federatedIdentityService = services.getService(ServiceType.KNOXIDF_FEDERATED_IDENTITY_SERVICE); + // Build the same passcode MAC the JWTFederationFilter uses so the token endpoint can + // independently authenticate a client_secret (see validateAuthCode). The HMAC key alias is + // guaranteed to exist by this point (PasscodeTokenResourceBase#setupTokenStateService + // generates it if absent). + final GatewayConfig gatewayConfig = (GatewayConfig) servletContext.getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE); + final AliasService aliasService = services.getService(ServiceType.ALIAS_SERVICE); + this.tokenMAC = new TokenMAC(gatewayConfig.getKnoxTokenHashAlgorithm(), aliasService.getPasswordFromAliasForGateway(TokenMAC.KNOX_TOKEN_HASH_KEY_ALIAS_NAME)); setRefreshTokenTTL(); } @@ -276,15 +287,22 @@ public class TokenResource extends PasscodeTokenResourceBase { throw new AuthTokenValidationError("Invalid auth_code: expired"); } else if (!associateRedirectUri.equals(redirectUri)) { throw new AuthTokenValidationError("Invalid redirect_uri: " + redirectUri); - } else { - final String associatedClientId = authCodeTokenMetadata.getMetadata(CLIENT_ID); - final String clientId = getRequestParam(CLIENT_ID); - if (!associatedClientId.equals(clientId)) { - throw new AuthTokenValidationError("Invalid client_id: " + clientId); - } } - // PKCE validation + final String associatedClientId = authCodeTokenMetadata.getMetadata(CLIENT_ID); + final String clientId = getRequestParam(CLIENT_ID); + if (!associatedClientId.equals(clientId)) { + throw new AuthTokenValidationError("Invalid client_id: " + clientId); + } + + // Client authentication (defense in depth). A stolen auth code must not be redeemable by + // a party that merely holds some valid Knox JWT: the JWTProvider (JWTFederationFilter) + // Bearer path forwards such a request to this endpoint without ever checking + // client_secret. So the token endpoint independently binds the redemption to the + // legitimate client here. The caller must prove client identity via EITHER: + // - PKCE: a code_verifier matching the challenge stored at authorize time (S256), or + // - the client's client_secret (constant-time compared against the stored passcode). + // A code is rejected when neither is satisfiable. final String codeChallenge = authCodeTokenMetadata.getMetadata(CODE_CHALLENGE); if (StringUtils.isNotBlank(codeChallenge)) { final String codeChallengeMethod = authCodeTokenMetadata.getMetadata(CODE_CHALLENGE_METHOD); @@ -295,16 +313,63 @@ public class TokenResource extends PasscodeTokenResourceBase { if (!validatePKCE(codeVerifier, codeChallenge, codeChallengeMethod)) { throw new AuthTokenValidationError("Invalid code_verifier"); } + } else if (!isValidClientSecret(clientId, getRequestParam(CLIENT_SECRET))) { + throw new AuthTokenValidationError("Invalid client authentication"); } } catch (UnknownTokenException e) { throw new AuthTokenValidationError("Unknown auth_code"); } } + /** + * Authenticates a confidential client on the token endpoint by validating the presented + * {@code client_secret} against the stored passcode of the client identified by {@code clientId}. + * <p> + * The wire format of {@code client_secret} matches what registration returns and what + * {@link org.apache.knox.gateway.provider.federation.jwt.filter.JWTFederationFilter} expects: + * {@code Base64(Base64(tokenId)::Base64(rawPasscode))}. The embedded {@code tokenId} must equal + * {@code clientId}, and {@code HMAC(tokenId, issueTime, userName, rawPasscode)} must equal the + * stored passcode hash. The comparison is constant-time. + * + * @return {@code true} only if the secret is well-formed, bound to {@code clientId}, and matches. + */ + boolean isValidClientSecret(final String clientId, final String clientSecret) { + if (StringUtils.isBlank(clientId) || StringUtils.isBlank(clientSecret)) { + return false; + } + try { + final String[] tokenIdAndPasscode = decodeBase64(clientSecret).split("::"); + if (tokenIdAndPasscode.length != 2) { + return false; + } + final String tokenId = decodeBase64(tokenIdAndPasscode[0]); + final String rawPasscode = decodeBase64(tokenIdAndPasscode[1]); + // The client_secret must belong to exactly the client redeeming the code. + if (!tokenId.equals(clientId)) { + return false; + } + final TokenMetadata clientMetadata = tokenStateService.getTokenMetadata(tokenId); + final String storedPasscode = clientMetadata == null ? null : clientMetadata.getPasscode(); + if (StringUtils.isBlank(storedPasscode)) { + return false; + } + final long issueTime = tokenStateService.getTokenIssueTime(tokenId); + final String userName = clientMetadata.getUserName(); + final byte[] computed = tokenMAC.hash(tokenId, issueTime, userName, rawPasscode).getBytes(StandardCharsets.UTF_8); + return MessageDigest.isEqual(computed, storedPasscode.getBytes(StandardCharsets.UTF_8)); + } catch (UnknownTokenException | RuntimeException e) { + return false; + } + } + + private String decodeBase64(final String value) { + return new String(Base64.getDecoder().decode(value.getBytes(StandardCharsets.UTF_8)), StandardCharsets.UTF_8); + } + private boolean validatePKCE(String codeVerifier, String codeChallenge, String method) { - if (PKCE_METHOD_PLAIN.equals(method)) { - return codeVerifier.equals(codeChallenge); - } else if (PKCE_METHOD_S256.equals(method)) { + // Only S256 is supported. 'plain' provides no protection and is rejected (the authorize + // endpoint already refuses to store a non-S256 challenge; this is defense in depth). + if (PKCE_METHOD_S256.equals(method)) { try { return generateS256Challenge(codeVerifier).equals(codeChallenge); } catch (NoSuchAlgorithmException e) { diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/userparams/LdapUserParamsProvider.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/userparams/LdapUserParamsProvider.java index a17248451..080cc32de 100644 --- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/userparams/LdapUserParamsProvider.java +++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/userparams/LdapUserParamsProvider.java @@ -30,6 +30,7 @@ import javax.naming.directory.SearchControls; import javax.naming.directory.SearchResult; import javax.naming.ldap.InitialLdapContext; import javax.naming.ldap.LdapContext; +import javax.naming.ldap.Rdn; import javax.servlet.ServletContext; import java.util.ArrayList; import java.util.HashMap; @@ -98,7 +99,10 @@ public class LdapUserParamsProvider implements UserParamsProvider { try { ctx = createSystemContext(); - String userDn = String.format(Locale.US, ldapUserDnTemplate, subjectName); + // Escape the subject before interpolating it into the DN template. Without escaping a + // crafted subject (e.g. "x,ou=admins") could inject additional DN components (LDAP + // injection). Rdn.escapeValue escapes the RDN value per RFC 2253. + String userDn = String.format(Locale.US, ldapUserDnTemplate, Rdn.escapeValue(subjectName)); SearchControls controls = new SearchControls(); controls.setSearchScope(SearchControls.OBJECT_SCOPE); diff --git a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/RegistrationResourceAnonymousGuardTest.java b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/RegistrationResourceAnonymousGuardTest.java new file mode 100644 index 000000000..11a84b4c2 --- /dev/null +++ b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/RegistrationResourceAnonymousGuardTest.java @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.knox.gateway.service.knoxidf; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.security.PrivilegedAction; + +import javax.security.auth.Subject; + +import org.apache.knox.gateway.security.PrimaryPrincipal; +import org.junit.Test; + +/** + * Verifies finding 1.3: dynamic client registration refuses anonymous callers unless the + * deployment explicitly opts in via {@code knoxidf.client.registration.anonymous.allowed}. The + * endpoint is wired as {@code anon} in the sample topologies, so this resource-level check is what + * keeps registration closed by default. + */ +public class RegistrationResourceAnonymousGuardTest { + + /** Exposes injection of the opt-in flag without running the full JAX-RS/servlet lifecycle. */ + static final class TestableRegistrationResource extends RegistrationResource { + TestableRegistrationResource(final boolean anonymousRegistrationAllowed) { + this.anonymousRegistrationAllowed = anonymousRegistrationAllowed; + } + } + + private static boolean deniedAs(final String principalName, final boolean anonymousAllowed) { + final TestableRegistrationResource resource = new TestableRegistrationResource(anonymousAllowed); + if (principalName == null) { + // No security context at all. + return resource.anonymousRegistrationDenied(); + } + final Subject subject = new Subject(); + subject.getPrincipals().add(new PrimaryPrincipal(principalName)); + return Subject.doAs(subject, (PrivilegedAction<Boolean>) resource::anonymousRegistrationDenied); + } + + @Test + public void testAnonymousCallerRejectedByDefault() { + // Default (flag false): the AnonymousAuthFilter principal ("anonymous") must be turned away. + assertTrue(deniedAs("anonymous", false)); + } + + @Test + public void testAnonymousCallerAllowedWhenExplicitlyEnabled() { + // The deliberate open-registration deployment mode: opt in and the anonymous caller is allowed. + assertFalse(deniedAs("anonymous", true)); + } + + @Test + public void testAuthenticatedCallerAlwaysAllowed() { + // A real authenticated principal is never subject to the anonymous gate, regardless of the flag. + assertFalse(deniedAs("alice", false)); + assertFalse(deniedAs("alice", true)); + } + + @Test + public void testAnonymousMatchIsCaseInsensitive() { + assertTrue(deniedAs("ANONYMOUS", false)); + } +} diff --git a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceClientAuthTest.java b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceClientAuthTest.java new file mode 100644 index 000000000..7cde79747 --- /dev/null +++ b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceClientAuthTest.java @@ -0,0 +1,116 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.knox.gateway.service.knoxidf; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.nio.charset.StandardCharsets; +import java.util.Base64; + +import org.apache.knox.gateway.services.security.token.TokenMetadata; +import org.apache.knox.gateway.services.security.token.TokenStateService; +import org.apache.knox.gateway.services.security.token.impl.TokenMAC; +import org.easymock.EasyMock; +import org.junit.Before; +import org.junit.Test; + +/** + * Verifies that the token endpoint independently authenticates a confidential client on the + * authorization_code grant (finding 1.1). This closes the gap where the JWTFederationFilter Bearer + * path forwards a request without checking client_secret: a stolen auth code must not be redeemable + * without proving client identity. + */ +public class TokenResourceClientAuthTest { + + private static final String CLIENT_ID = "client-abc"; + private static final String USER_NAME = "alice"; + private static final long ISSUE_TIME = 1_700_000_000_000L; + private static final String RAW_PASSCODE = "0f1e2d3c-4b5a-6978-8796-a5b4c3d2e1f0"; + + private TokenMAC tokenMAC; + + private TestableTokenResource resource; + + /** + * Exposes injection of the inherited (protected) token-state service and the package-private MAC + * so the client-authentication logic can be exercised without the full JAX-RS/servlet lifecycle. + */ + static final class TestableTokenResource extends TokenResource { + void inject(final TokenStateService tokenStateService, final TokenMAC mac) { + this.tokenStateService = tokenStateService; + this.tokenMAC = mac; + } + } + + @Before + public void setUp() throws Exception { + // A deterministic MAC shared by "the server" (stored hash) and the resource under test. + tokenMAC = new TokenMAC("HmacSHA256", "0123456789abcdef0123456789abcdef".toCharArray()); + final String storedPasscodeHash = tokenMAC.hash(CLIENT_ID, ISSUE_TIME, USER_NAME, RAW_PASSCODE); + + final TokenMetadata clientMetadata = EasyMock.createNiceMock(TokenMetadata.class); + EasyMock.expect(clientMetadata.getUserName()).andReturn(USER_NAME).anyTimes(); + EasyMock.expect(clientMetadata.getPasscode()).andReturn(storedPasscodeHash).anyTimes(); + EasyMock.replay(clientMetadata); + + final TokenStateService tokenStateService = EasyMock.createNiceMock(TokenStateService.class); + EasyMock.expect(tokenStateService.getTokenMetadata(CLIENT_ID)).andReturn(clientMetadata).anyTimes(); + EasyMock.expect(tokenStateService.getTokenIssueTime(CLIENT_ID)).andReturn(ISSUE_TIME).anyTimes(); + EasyMock.replay(tokenStateService); + + resource = new TestableTokenResource(); + resource.inject(tokenStateService, tokenMAC); + } + + private static String wireSecret(final String tokenId, final String rawPasscode) { + final String inner = Base64.getEncoder().encodeToString(tokenId.getBytes(StandardCharsets.UTF_8)) + + "::" + Base64.getEncoder().encodeToString(rawPasscode.getBytes(StandardCharsets.UTF_8)); + return Base64.getEncoder().encodeToString(inner.getBytes(StandardCharsets.UTF_8)); + } + + @Test + public void testValidClientSecretIsAccepted() { + assertTrue(resource.isValidClientSecret(CLIENT_ID, wireSecret(CLIENT_ID, RAW_PASSCODE))); + } + + @Test + public void testWrongPasscodeIsRejected() { + assertFalse(resource.isValidClientSecret(CLIENT_ID, wireSecret(CLIENT_ID, "not-the-real-passcode"))); + } + + @Test + public void testSecretBoundToDifferentClientIsRejected() { + // A secret whose embedded tokenId does not match the client_id redeeming the code must fail, + // even if the secret itself is otherwise well-formed. + assertFalse(resource.isValidClientSecret("some-other-client", wireSecret(CLIENT_ID, RAW_PASSCODE))); + } + + @Test + public void testBlankSecretIsRejected() { + assertFalse(resource.isValidClientSecret(CLIENT_ID, null)); + assertFalse(resource.isValidClientSecret(CLIENT_ID, "")); + } + + @Test + public void testMalformedSecretIsRejected() { + // Not base64 / no "tokenId::passcode" structure. + assertFalse(resource.isValidClientSecret(CLIENT_ID, "!!!not-base64!!!")); + assertFalse(resource.isValidClientSecret(CLIENT_ID, + Base64.getEncoder().encodeToString("no-separator".getBytes(StandardCharsets.UTF_8)))); + } +} diff --git a/gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java b/gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java index ca59dc1eb..7b714d343 100644 --- a/gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java +++ b/gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java @@ -1169,12 +1169,16 @@ public class TokenResource { handleDelegatedAuthentication(subject, jwtAttributesBuilder); } + // This resource is a @Singleton, so hardCodedClaimMappings is shared across all requests and + // must never be mutated per-request. Merge the topology-configured mappings with this request's + // user params into a fresh map; otherwise one user's params would leak into other users' tokens. + final Map<String, Object> customAttributes = new HashMap<>(hardCodedClaimMappings); if (userContext.userParams != null) { - hardCodedClaimMappings.putAll(userContext.userParams); + customAttributes.putAll(userContext.userParams); } - if (!hardCodedClaimMappings.isEmpty()) { - jwtAttributesBuilder.setCustomAttributes(hardCodedClaimMappings); + if (!customAttributes.isEmpty()) { + jwtAttributesBuilder.setCustomAttributes(customAttributes); } jwtAttributes = jwtAttributesBuilder.build(); diff --git a/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/JWTokenAttributesBuilder.java b/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/JWTokenAttributesBuilder.java index 8bc70d43d..1e4fb96b5 100644 --- a/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/JWTokenAttributesBuilder.java +++ b/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/JWTokenAttributesBuilder.java @@ -28,7 +28,10 @@ public class JWTokenAttributesBuilder { private String userName; private List<String> audiences; private String algorithm; - private long issueTime; + // Default to the builder's creation time so every issued token carries a correct 'iat'. + // Callers that need a specific issue time (e.g. managed-token flows) override this via + // setIssueTime(). Without this default, JWTToken would emit iat=epoch-0 (1970). + private long issueTime = System.currentTimeMillis(); private long expires; private String signingKeystoreName; private String signingKeystoreAlias; diff --git a/gateway-spi/src/test/java/org/apache/knox/gateway/services/security/token/impl/JWTTokenTest.java b/gateway-spi/src/test/java/org/apache/knox/gateway/services/security/token/impl/JWTTokenTest.java index c4ab7d7ba..becb9c32c 100644 --- a/gateway-spi/src/test/java/org/apache/knox/gateway/services/security/token/impl/JWTTokenTest.java +++ b/gateway-spi/src/test/java/org/apache/knox/gateway/services/security/token/impl/JWTTokenTest.java @@ -86,6 +86,34 @@ public class JWTTokenTest { assertTrue("Missing ALG claim in JWT header", token.getHeader().contains(ALGO)); } + @Test + public void testIssueTimeDefaultsToNow() throws Exception { + // Regression: JWTToken always emits 'iat'. When a caller does not set the issue time, the + // builder must default it to "now" rather than leaving it at epoch-0 (1970). + final long before = System.currentTimeMillis(); + final JWT token = new JWTToken(new JWTokenAttributesBuilder() + .setUserName("[email protected]").setAlgorithm("RS256").build()); + final long after = System.currentTimeMillis(); + + final Date issueTime = token.getJWTClaimsSet().getIssueTime(); + assertNotNull("iat must be present", issueTime); + // JWT 'iat' has second precision, so allow the surrounding second as slack. + assertTrue("iat must be ~now, not 1970 (was " + issueTime + ")", + issueTime.getTime() >= (before - 1000L) && issueTime.getTime() <= (after + 1000L)); + } + + @Test + public void testIssueTimeIsHonouredWhenSet() throws Exception { + final long explicit = 1_600_000_000_000L; // 2020-09-13 + final JWT token = new JWTToken(new JWTokenAttributesBuilder() + .setUserName("[email protected]").setAlgorithm("RS256").setIssueTime(explicit).build()); + + final Date issueTime = token.getJWTClaimsSet().getIssueTime(); + assertNotNull(issueTime); + // second precision + assertEquals(explicit / 1000L, issueTime.getTime() / 1000L); + } + @Test public void testPrivateUUIDClaim() throws Exception { JWT token = new JWTToken(new JWTokenAttributesBuilder().setAudiences(singletonList("https://login.example.com";)).setUserName("[email protected]").setAlgorithm("RS256").build()); diff --git a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/AuthorizeRequestMetadata.java b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/AuthorizeRequestMetadata.java index 239e0baf2..4f01eaddf 100644 --- a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/AuthorizeRequestMetadata.java +++ b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/AuthorizeRequestMetadata.java @@ -71,6 +71,13 @@ public final class AuthorizeRequestMetadata { return error("invalid_request", "Missing redirect_uri"); } + // Require state for CSRF protection: it is echoed back on the redirect and the client + // must match it against the value it generated. Without it the auth-code flow is open to + // login-CSRF, and redirectToAuthSuccess would NPE URL-encoding a null state. + if (state == null || state.isEmpty()) { + return error("invalid_request", "Missing state"); + } + // Verify scope(s) if (requestedScopes == null || requestedScopes.isEmpty()) { return error("invalid_scope", "Missing scopes"); diff --git a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfiguration.java b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfiguration.java index 7ad607843..8ebdea5b1 100644 --- a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfiguration.java +++ b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfiguration.java @@ -28,6 +28,12 @@ public class FederatedOpConfiguration { private final String userInfoEndpoint; private final String discoveryEndpoint; private final String authorizeCallback; + private final String jwksEndpoint; + private final String issuer; + private final String signatureAlgorithm; + + // Default signature algorithm expected for the OP's id_token when not explicitly configured. + static final String DEFAULT_SIGNATURE_ALGORITHM = "RS256"; public FederatedOpConfiguration(final ServletContext servletContext, final String opName) { this.name = opName; @@ -40,6 +46,12 @@ public class FederatedOpConfiguration { this.authorizeCallback = servletContext.getInitParameter(prefix + "authorize.callback"); this.userInfoEndpoint = servletContext.getInitParameter(prefix + "userinfo.endpoint"); this.discoveryEndpoint = servletContext.getInitParameter(prefix + "discovery.endpoint"); + // Used to validate the OP's id_token (signature via JWKS, expected issuer). See + // AuthorizeResource#validateFederatedIdToken - federated login fails closed without these. + this.jwksEndpoint = servletContext.getInitParameter(prefix + "jwks.endpoint"); + this.issuer = servletContext.getInitParameter(prefix + "issuer"); + final String configuredAlg = servletContext.getInitParameter(prefix + "signature.algorithm"); + this.signatureAlgorithm = configuredAlg == null || configuredAlg.isEmpty() ? DEFAULT_SIGNATURE_ALGORITHM : configuredAlg; } public String getName() { @@ -78,4 +90,16 @@ public class FederatedOpConfiguration { return discoveryEndpoint; } + public String getJwksEndpoint() { + return jwksEndpoint; + } + + public String getIssuer() { + return issuer; + } + + public String getSignatureAlgorithm() { + return signatureAlgorithm; + } + } diff --git a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFConstants.java b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFConstants.java index d757b573f..aabb85684 100644 --- a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFConstants.java +++ b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFConstants.java @@ -59,6 +59,11 @@ public interface KnoxIDFConstants { String TOKEN_EXCHANGE_TOPOLOGY_NAME = "token.exchange.topology.name"; + // When false (the default), the dynamic client-registration endpoint refuses anonymous callers + // even if the topology wires it as 'anon'. Deployments that intend open, unauthenticated + // registration must explicitly set this to true (see the sample knoxidf topologies). + String CLIENT_REGISTRATION_ANONYMOUS_ALLOWED = "knoxidf.client.registration.anonymous.allowed"; + // TrustedOidcIssuerService gateway-level params (read from GatewayConfig / gateway-site.xml) String TRUSTED_OIDC_ISSUER_DISCOVERY_CACHE_TTL_SECS = "gateway.trustedoidcissuer.discovery.cache.ttl.secs"; diff --git a/gateway-util-common/src/test/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfigurationTest.java b/gateway-util-common/src/test/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfigurationTest.java new file mode 100644 index 000000000..cfe74d8d9 --- /dev/null +++ b/gateway-util-common/src/test/java/org/apache/knox/gateway/util/knoxidf/FederatedOpConfigurationTest.java @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.knox.gateway.util.knoxidf; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import javax.servlet.ServletContext; + +import org.easymock.EasyMock; +import org.junit.Test; + +public class FederatedOpConfigurationTest { + + private static final String OP = "keycloak"; + private static final String PREFIX = KnoxIDFConstants.FEDERATED_OP_CONFIG_PREFIX + OP + "."; + + @Test + public void testIdTokenVerificationParamsAreRead() { + final ServletContext context = EasyMock.createNiceMock(ServletContext.class); + EasyMock.expect(context.getInitParameter(PREFIX + "jwks.endpoint")).andReturn("https://op.example/jwks";).anyTimes(); + EasyMock.expect(context.getInitParameter(PREFIX + "issuer")).andReturn("https://op.example/realms/knox";).anyTimes(); + EasyMock.expect(context.getInitParameter(PREFIX + "signature.algorithm")).andReturn("RS512").anyTimes(); + EasyMock.expect(context.getInitParameter(PREFIX + "clientId")).andReturn("knox-client").anyTimes(); + EasyMock.replay(context); + + final FederatedOpConfiguration config = new FederatedOpConfiguration(context, OP); + + assertEquals("https://op.example/jwks";, config.getJwksEndpoint()); + assertEquals("https://op.example/realms/knox";, config.getIssuer()); + assertEquals("RS512", config.getSignatureAlgorithm()); + assertEquals("knox-client", config.getClientId()); + } + + @Test + public void testSignatureAlgorithmDefaultsToRS256() { + final ServletContext context = EasyMock.createNiceMock(ServletContext.class); + // No signature.algorithm configured -> the default (RS256) must be used. + EasyMock.expect(context.getInitParameter(PREFIX + "signature.algorithm")).andReturn(null).anyTimes(); + EasyMock.replay(context); + + final FederatedOpConfiguration config = new FederatedOpConfiguration(context, OP); + + assertEquals(FederatedOpConfiguration.DEFAULT_SIGNATURE_ALGORITHM, config.getSignatureAlgorithm()); + assertEquals("RS256", config.getSignatureAlgorithm()); + } + + @Test + public void testVerificationParamsAbsentByDefault() { + final ServletContext context = EasyMock.createNiceMock(ServletContext.class); + EasyMock.replay(context); + + final FederatedOpConfiguration config = new FederatedOpConfiguration(context, OP); + + // When nothing is configured the id_token verification inputs are null, which makes the + // federated login fail closed in AuthorizeResource#validateFederatedIdToken. + assertNull(config.getJwksEndpoint()); + assertNull(config.getIssuer()); + } +}
