You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@knox.apache.org by sm...@apache.org on 2026/08/13 16:03:46 UTC
(knox) 17/37: KNOX-3414: Tier 3 hardening + Tier 4 polish for the OIDC provider
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 7802c3c65cf88cd0ac97c8bf96770b62e6e10e17
Author: Sandor Molnar <sm...@apache.org>
AuthorDate: Mon Aug 10 21:55:15 2026 +0200
KNOX-3414: Tier 3 hardening + Tier 4 polish for the OIDC provider
Tier 3 (hardening):
- Registration redirect_uris require HTTPS (plain HTTP only for loopback, RFC 8252)
- XSS: escape unknown scopes in AuthConsentServlet; rebuild knoxauth.js OP links
via the DOM API instead of raw HTML / inline onclick interpolation
- URL-encode clientId and federated-OP redirect params in AuthorizeResource / KnoxIDFUtils
- Single-use consent/federation state: add KnoxIDFArtifactStore.remove() and invalidate
state after use in authCallback and consentAccepted
- Reject disabled refresh tokens (isEnabled check) in TokenResource
- Remove hardcoded LDAP "admin-password" fallback; fail fast without a configured alias
- DiscoveryResource: literal String.replace instead of regex replaceAll for topology name
- error() maps OAuth error codes to correct HTTP status per RFC 6749 5.2 (no longer always 401)
- RedirectToUrlFilter: treat a blank fedOpSid the same as absent
Tier 4 (polish):
- Fix public-API typos: EmptyFederatedIdentityService, BASE_RESOURCE_PATH
- Make ALLOWED_RESPONSE_TYPES / DEFAULT_SCOPES immutable; copy at mutating call sites
- Drop nonce from the UserInfo response (id_token only)
- Document the KnoxIDFArtifactStore ttl*2 grace window
Tests: KnoxIDFUtilsErrorStatusTest, RegistrationRedirectUriPolicyTest, KnoxIDFArtifactStoreTest
(+18); TokenResourceAuthCodeReplayTest updated 401->400. All affected modules green.
Co-Authored-By: Claude Opus 4.8 <no...@anthropic.com>
---
.../applications/knoxauth/app/js/knoxauth.js | 14 ++--
.../knox/gateway/filter/RedirectToUrlFilter.java | 5 +-
.../factory/FederatedIdentityServiceFactory.java | 10 +--
...ice.java => EmptyFederatedIdentityService.java} | 2 +-
.../service/knoxidf/AuthConsentServlet.java | 6 +-
.../gateway/service/knoxidf/AuthorizeResource.java | 20 ++++--
.../gateway/service/knoxidf/DiscoveryResource.java | 10 +--
.../knox/gateway/service/knoxidf/JwksResource.java | 4 +-
.../service/knoxidf/RegistrationResource.java | 33 +++++++--
.../gateway/service/knoxidf/TokenResource.java | 14 ++--
.../gateway/service/knoxidf/UserInfoResource.java | 11 ++-
.../knoxidf/userparams/LdapUserParamsProvider.java | 11 ++-
.../knoxidf/KnoxIDFUtilsErrorStatusTest.java | 83 ++++++++++++++++++++++
.../knoxidf/RegistrationRedirectUriPolicyTest.java | 76 ++++++++++++++++++++
.../knoxidf/TokenResourceAuthCodeReplayTest.java | 2 +-
.../gateway/util/knoxidf/KnoxIDFArtifactStore.java | 9 +++
.../gateway/util/knoxidf/KnoxIDFConstants.java | 11 +--
.../knox/gateway/util/knoxidf/KnoxIDFUtils.java | 53 ++++++++++++--
.../util/knoxidf/KnoxIDFArtifactStoreTest.java | 63 ++++++++++++++++
19 files changed, 381 insertions(+), 56 deletions(-)
diff --git a/gateway-applications/src/main/resources/applications/knoxauth/app/js/knoxauth.js b/gateway-applications/src/main/resources/applications/knoxauth/app/js/knoxauth.js
index 5a304a556..329ad930c 100644
--- a/gateway-applications/src/main/resources/applications/knoxauth/app/js/knoxauth.js
+++ b/gateway-applications/src/main/resources/applications/knoxauth/app/js/knoxauth.js
@@ -74,12 +74,14 @@ var loadFederatedOpLinks = function() {
}
ops.forEach(op => {
- container.append(`
- <div class="fed-op-btn" onclick="loginWithOp('${op}')">
- <span class="fed-op-icon">๐</span>
- <span class="fed-op-label">Continue with ${op}</span>
- </div>
- `);
+ // Build the element via the DOM API and bind the handler in JS rather than interpolating
+ // the (attacker-controllable) op name into an HTML string / inline onclick attribute.
+ // .text() escapes the label; the click closure captures op without string injection.
+ const btn = $('<div class="fed-op-btn"></div>');
+ $('<span class="fed-op-icon"></span>').text("๐").appendTo(btn);
+ $('<span class="fed-op-label"></span>').text("Continue with " + op).appendTo(btn);
+ btn.on("click", function() { loginWithOp(op); });
+ container.append(btn);
});
};
diff --git a/gateway-provider-security-shiro/src/main/java/org/apache/knox/gateway/filter/RedirectToUrlFilter.java b/gateway-provider-security-shiro/src/main/java/org/apache/knox/gateway/filter/RedirectToUrlFilter.java
index 7501dbe4b..bffc5049a 100644
--- a/gateway-provider-security-shiro/src/main/java/org/apache/knox/gateway/filter/RedirectToUrlFilter.java
+++ b/gateway-provider-security-shiro/src/main/java/org/apache/knox/gateway/filter/RedirectToUrlFilter.java
@@ -27,6 +27,7 @@ import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
+import org.apache.commons.lang3.StringUtils;
import org.apache.knox.gateway.config.GatewayConfig;
public class RedirectToUrlFilter extends AbstractGatewayFilter {
@@ -47,7 +48,9 @@ public class RedirectToUrlFilter extends AbstractGatewayFilter {
@Override
protected void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws IOException, ServletException {
- if (redirectUrl != null && request.getHeader("Authorization") == null && request.getParameter("fedOpSid") == null) {
+ // Treat a blank fedOpSid the same as absent: a bare "?fedOpSid=" must not suppress the redirect.
+ // (Downstream authentication still runs; this only prevents an empty param from skipping it.)
+ if (redirectUrl != null && request.getHeader("Authorization") == null && StringUtils.isBlank(request.getParameter("fedOpSid"))) {
response.sendRedirect(redirectUrl + getOriginalQueryString(request));
}
chain.doFilter(request, response);
diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/factory/FederatedIdentityServiceFactory.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/factory/FederatedIdentityServiceFactory.java
index f78f96af6..7ffe009fa 100644
--- a/gateway-server/src/main/java/org/apache/knox/gateway/services/factory/FederatedIdentityServiceFactory.java
+++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/factory/FederatedIdentityServiceFactory.java
@@ -23,7 +23,7 @@ import org.apache.knox.gateway.services.GatewayServices;
import org.apache.knox.gateway.services.Service;
import org.apache.knox.gateway.services.ServiceLifecycleException;
import org.apache.knox.gateway.services.ServiceType;
-import org.apache.knox.gateway.services.knoxidf.federation.EmptyFederatedIdentitityService;
+import org.apache.knox.gateway.services.knoxidf.federation.EmptyFederatedIdentityService;
import org.apache.knox.gateway.services.knoxidf.federation.FederatedIdentityService;
import org.apache.knox.gateway.services.knoxidf.federation.JdbcFederatedIdentityService;
import org.apache.knox.gateway.services.topology.TopologyService;
@@ -36,7 +36,7 @@ import java.util.Map;
public class FederatedIdentityServiceFactory extends AbstractServiceFactory {
private static final GatewayMessages LOG = MessagesFactory.get(GatewayMessages.class);
- private static final String DEFAULT_IMPLEMENTATION = EmptyFederatedIdentitityService.class.getName();
+ private static final String DEFAULT_IMPLEMENTATION = EmptyFederatedIdentityService.class.getName();
@Override
protected Service createService(GatewayServices gatewayServices, ServiceType serviceType, GatewayConfig gatewayConfig, Map<String, String> options, String implementation)
@@ -52,8 +52,8 @@ public class FederatedIdentityServiceFactory extends AbstractServiceFactory {
FederatedIdentityService service = null;
if (shouldCreateService(implementationToUse)) {
- if (matchesImplementation(implementationToUse, EmptyFederatedIdentitityService.class, true)) {
- service = new EmptyFederatedIdentitityService();
+ if (matchesImplementation(implementationToUse, EmptyFederatedIdentityService.class, true)) {
+ service = new EmptyFederatedIdentityService();
} else if (matchesImplementation(implementationToUse, JdbcFederatedIdentityService.class)) {
try {
try {
@@ -62,7 +62,7 @@ public class FederatedIdentityServiceFactory extends AbstractServiceFactory {
service.init(gatewayConfig, options);
} catch (ServiceLifecycleException e) {
LOG.errorInitializingService(implementationToUse, e.getMessage(), e);
- service = new EmptyFederatedIdentitityService();
+ service = new EmptyFederatedIdentityService();
}
} catch (Exception e) {
throw new ServiceLifecycleException("Error while creating Federated Identity Service: " + e, e);
diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/EmptyFederatedIdentitityService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/EmptyFederatedIdentityService.java
similarity index 95%
rename from gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/EmptyFederatedIdentitityService.java
rename to gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/EmptyFederatedIdentityService.java
index 8ce9e550e..d73cd0c8d 100644
--- a/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/EmptyFederatedIdentitityService.java
+++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/federation/EmptyFederatedIdentityService.java
@@ -22,7 +22,7 @@ import org.apache.knox.gateway.services.ServiceLifecycleException;
import java.util.Map;
import java.util.Optional;
-public class EmptyFederatedIdentitityService implements FederatedIdentityService {
+public class EmptyFederatedIdentityService implements FederatedIdentityService {
@Override
public void addFederatedIdentity(FederatedIdentity identity) {
}
diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthConsentServlet.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthConsentServlet.java
index 6200edc83..211c9fec6 100644
--- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthConsentServlet.java
+++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/AuthConsentServlet.java
@@ -16,6 +16,8 @@
*/
package org.apache.knox.gateway.service.knoxidf;
+import org.apache.commons.text.StringEscapeUtils;
+
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
@@ -114,7 +116,9 @@ public class AuthConsentServlet extends HttpServlet {
case "calendar.write":
return "Modify your calendar events";
default:
- return scope;
+ // Unknown scopes are echoed into the HTML consent page. Escape them so an
+ // attacker-influenced scope value cannot inject markup (defense in depth).
+ return StringEscapeUtils.escapeHtml4(scope);
}
}
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 d778aa7e1..b27c7f9d8 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
@@ -87,7 +87,7 @@ import java.util.stream.Collectors;
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.ALLOWED_SCOPES;
-import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.BASE_RESORCE_PATH;
+import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.BASE_RESOURCE_PATH;
import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CLIENT_ID;
import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CODE;
import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CODE_CHALLENGE;
@@ -107,7 +107,7 @@ import static org.apache.knox.gateway.util.knoxidf.KnoxIDFUtils.error;
@Path(AuthorizeResource.RESOURCE_PATH)
public class AuthorizeResource extends PasscodeTokenResourceBase {
- static final String RESOURCE_PATH = BASE_RESORCE_PATH + "/authorize";
+ static final String RESOURCE_PATH = BASE_RESOURCE_PATH + "/authorize";
private static final UUID KNOX_NAMESPACE = UUID.fromString("6ba7b811-9dad-11d1-80b4-00c04fd430c8");
private static final NameBasedGenerator UUID_V5 = Generators.nameBasedGenerator(KNOX_NAMESPACE);
public static final Set<String> ALLOWED_CLAIMS = Set.of("preferred_username", "email", "email_verified",
@@ -166,7 +166,8 @@ public class AuthorizeResource extends PasscodeTokenResourceBase {
String codeChallenge,
String codeChallengeMethod) {
final String subject = SubjectUtils.getCurrentEffectivePrincipalName();
- final Set<String> requestedScopes = StringUtils.isBlank(scope) ? DEFAULT_SCOPES : new HashSet<>(Arrays.asList(scope.split("\\s+")));
+ // DEFAULT_SCOPES is an ImmutableSet; copy it into a mutable set so downstream mutation is safe.
+ final Set<String> requestedScopes = StringUtils.isBlank(scope) ? new HashSet<>(DEFAULT_SCOPES) : new HashSet<>(Arrays.asList(scope.split("\\s+")));
final AuthorizeRequestMetadata authorizeRequestMetadata = new AuthorizeRequestMetadata(clientId, subject, responseType, redirectUri, requestedScopes, state, nonce, codeChallenge, codeChallengeMethod);
final Response verificationErrorResponse = verifyParams(authorizeRequestMetadata);
if (verificationErrorResponse != null) {
@@ -180,8 +181,11 @@ public class AuthorizeResource extends PasscodeTokenResourceBase {
final String consentAuthState = UUID.randomUUID().toString();
authorizeRequestMetadataStore.put(consentAuthState, authorizeRequestMetadata);
final String baseUri = servletContext.getContextPath() + "/authConsent";
+ // Every value placed into the consent redirect's query string must be percent-encoded;
+ // a client_id containing '&', '=' or '#' would otherwise split or corrupt the URL.
+ final String clientIdParam = URLEncoder.encode(clientId, StandardCharsets.UTF_8);
final String scopeParam = URLEncoder.encode(authorizeRequestMetadata.getJoinedRequestedScopes(), StandardCharsets.UTF_8);
- final String redirect = String.format(Locale.US, "%s?client_id=%s&state=%s&scope=%s", baseUri, clientId, consentAuthState, scopeParam);
+ final String redirect = String.format(Locale.US, "%s?client_id=%s&state=%s&scope=%s", baseUri, clientIdParam, consentAuthState, scopeParam);
return Response.seeOther(java.net.URI.create(redirect)).build();
}
}
@@ -277,6 +281,10 @@ public class AuthorizeResource extends PasscodeTokenResourceBase {
if (federatedOpConfiguration == null) {
return error("invalid_request", "No federated OP configuration available for the request");
}
+ // The federated callback state is single-use: invalidate it in both stores now that it has
+ // been validated and captured, so a replayed callback with the same state is rejected.
+ authorizeRequestMetadataStore.remove(state);
+ federatedOpConfigurationStore.remove(state);
final Pair<String, String> federatedTokens = exchangeFederatedAuthCodeToTokens(federatedAuthCode, federatedOpConfiguration);
if (StringUtils.isBlank(federatedTokens.getLeft())) {
return error("invalid_request", "Federated OP did not return an id_token");
@@ -297,8 +305,10 @@ public class AuthorizeResource extends PasscodeTokenResourceBase {
final String state = request.getParameter(STATE);
final AuthorizeRequestMetadata authorizeRequestMetadata = authorizeRequestMetadataStore.get(state);
if (authorizeRequestMetadata == null) {
- return error("Consent cannot be accepted", "Invalid state");
+ return error("invalid_request", "Invalid state");
}
+ // Single-use consent state: invalidate it so the accepted-consent redirect cannot be replayed.
+ authorizeRequestMetadataStore.remove(state);
markConsentAccepted(authorizeRequestMetadata);
return authorize(authorizeRequestMetadata.getResponseType(),
authorizeRequestMetadata.getClientId(),
diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/DiscoveryResource.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/DiscoveryResource.java
index ce895549d..eb6584f3e 100644
--- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/DiscoveryResource.java
+++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/DiscoveryResource.java
@@ -32,10 +32,10 @@ import javax.ws.rs.core.UriInfo;
import java.util.HashMap;
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.BASE_RESOURCE_PATH;
import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.TOKEN_EXCHANGE_TOPOLOGY_NAME;
-@Path(BASE_RESORCE_PATH + "/.well-known/openid-configuration")
+@Path(BASE_RESOURCE_PATH + "/.well-known/openid-configuration")
@Produces(MediaType.APPLICATION_JSON)
public class DiscoveryResource {
private String currentTopologyName;
@@ -59,8 +59,10 @@ public class DiscoveryResource {
String tokenEndpoint = baseUrl + TokenResource.RESOURCE_PATH;
String userInfoEndpoint = baseUrl + UserInfoResource.RESOURCE_PATH;
if (tokenExchangeTopologyName != null) {
- tokenEndpoint = tokenEndpoint.replaceAll(currentTopologyName, tokenExchangeTopologyName);
- userInfoEndpoint = userInfoEndpoint.replaceAll(currentTopologyName, tokenExchangeTopologyName);
+ // Literal substitution: the topology name is data, not a regex. replaceAll would treat
+ // any regex metacharacter in the topology name as a pattern.
+ tokenEndpoint = tokenEndpoint.replace(currentTopologyName, tokenExchangeTopologyName);
+ userInfoEndpoint = userInfoEndpoint.replace(currentTopologyName, tokenExchangeTopologyName);
}
config.put("token_endpoint", tokenEndpoint);
config.put("userinfo_endpoint", userInfoEndpoint);
diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/JwksResource.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/JwksResource.java
index 47f802b04..1dbca3c8e 100644
--- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/JwksResource.java
+++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/JwksResource.java
@@ -24,12 +24,12 @@ import javax.ws.rs.Produces;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
-import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.BASE_RESORCE_PATH;
+import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.BASE_RESOURCE_PATH;
@Path(JwksResource.RESOURCE_PATH)
@Produces(MediaType.APPLICATION_JSON)
public class JwksResource extends JWKSResource {
- static final String RESOURCE_PATH = BASE_RESORCE_PATH + "/jwks";
+ static final String RESOURCE_PATH = BASE_RESOURCE_PATH + "/jwks";
@GET
public Response getKeys() {
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 ed3ad7411..bd7dedff2 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
@@ -43,7 +43,7 @@ import java.util.Arrays;
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.BASE_RESOURCE_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;
@@ -52,7 +52,7 @@ import static org.apache.knox.gateway.util.knoxidf.KnoxIDFUtils.error;
@RequestScoped //this is important because redirectUris/allowedScopes are part of the state of this class
public class RegistrationResource extends ClientCredentialsResource {
- static final String RESOURCE_PATH = BASE_RESORCE_PATH + "/client";
+ static final String RESOURCE_PATH = BASE_RESOURCE_PATH + "/client";
private static final String ANONYMOUS_PRINCIPAL = "anonymous";
private List<String> redirectUris;
@@ -125,6 +125,12 @@ public class RegistrationResource extends ClientCredentialsResource {
}
private Response verifyRedirectUris() {
+ return verifyRedirectUris(redirectUris);
+ }
+
+ // Package-private and list-parameterized so the redirect-URI policy (https-only except loopback,
+ // no wildcard host, restricted path/query/fragment wildcards) is unit-testable in isolation.
+ static Response verifyRedirectUris(List<String> redirectUris) {
if (redirectUris == null || redirectUris.isEmpty()) {
return error("invalid_request", "redirect_uris must be provided");
}
@@ -137,16 +143,20 @@ public class RegistrationResource extends ClientCredentialsResource {
return error("invalid_request", "Invalid redirect URI: " + uriStr);
}
- // Scheme check
- if (!"https".equalsIgnoreCase(uri.getScheme()) && !"http".equalsIgnoreCase(uri.getScheme())) {
- return error("invalid_request", "Redirect URI must use HTTPS or HTTP as scheme: " + uriStr);
- }
-
// Host check (no wildcard allowed)
if (uri.getHost() == null || uri.getHost().contains("*")) {
return error("invalid_request", "Wildcard not allowed in host: " + uriStr);
}
+ // Scheme check: require HTTPS per RFC 8252, allowing plain HTTP only for loopback
+ // (localhost / 127.0.0.1 / ::1) native-app dev. Any other http:// redirect is rejected.
+ final String scheme = uri.getScheme();
+ final boolean https = "https".equalsIgnoreCase(scheme);
+ final boolean loopbackHttp = "http".equalsIgnoreCase(scheme) && isLoopbackHost(uri.getHost());
+ if (!https && !loopbackHttp) {
+ return error("invalid_request", "Redirect URI must use HTTPS (plain HTTP allowed only for localhost): " + uriStr);
+ }
+
// Path wildcard check
String path = uri.getPath();
if (path != null && path.contains("*") && !path.endsWith("*")) {
@@ -162,6 +172,15 @@ public class RegistrationResource extends ClientCredentialsResource {
return null;
}
+ private static boolean isLoopbackHost(String host) {
+ if (host == null) {
+ return false;
+ }
+ // Strip brackets from an IPv6 literal (e.g. [::1]).
+ final String h = host.startsWith("[") && host.endsWith("]") ? host.substring(1, host.length() - 1) : host;
+ return "localhost".equalsIgnoreCase(h) || "127.0.0.1".equals(h) || "::1".equals(h);
+ }
+
@Override
protected void addArbitraryTokenMetadata(TokenMetadata tokenMetadata) {
tokenMetadata.add("redirect_uris", getRedirectUris());
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 540df06f6..513b275c3 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
@@ -61,7 +61,7 @@ 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;
+import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.BASE_RESOURCE_PATH;
import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CLIENT_ID;
import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CODE;
import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.CODE_CHALLENGE;
@@ -80,7 +80,7 @@ import static org.apache.knox.gateway.util.knoxidf.KnoxIDFUtils.error;
@Path(TokenResource.RESOURCE_PATH)
@Produces(MediaType.APPLICATION_JSON)
public class TokenResource extends PasscodeTokenResourceBase {
- static final String RESOURCE_PATH = BASE_RESORCE_PATH + "/token";
+ static final String RESOURCE_PATH = BASE_RESOURCE_PATH + "/token";
// Per-request stash for the auth-code TokenMetadata read during validation. The code is
// atomically consumed (deleted) BEFORE token issuance to close the replay window, so the
@@ -228,7 +228,7 @@ public class TokenResource extends PasscodeTokenResourceBase {
} catch (UnknownTokenException e) {
return error("invalid_grant", "Unknown refresh_token");
} catch (RefreshTokenValidationError e) {
- return error("Refresh token validation error", e.getMessage());
+ return error("invalid_grant", e.getMessage());
}
}
@@ -248,6 +248,12 @@ public class TokenResource extends PasscodeTokenResourceBase {
throw new RefreshTokenValidationError("Invalid grant: invalid refresh_token");
}
+ // A refresh token that has been administratively disabled (revoked) must not mint new tokens,
+ // even if it has not yet expired.
+ if (!refreshTokenMetadata.isEnabled()) {
+ throw new RefreshTokenValidationError("Invalid grant: refresh_token disabled");
+ }
+
if (tokenStateService.getTokenExpiration(refreshTokenId) <= System.currentTimeMillis()) {
throw new RefreshTokenValidationError("Invalid grant: Refresh token expired");
}
@@ -268,7 +274,7 @@ public class TokenResource extends PasscodeTokenResourceBase {
try {
authCodeMetadata = validateAuthCode(code, redirectUri);
} catch (AuthTokenValidationError e) {
- return error("Auth code validation error", e.getMessage());
+ return error("invalid_grant", e.getMessage());
}
// Enforce single-use: atomically consume the code BEFORE issuing any token. Of N concurrent
diff --git a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/UserInfoResource.java b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/UserInfoResource.java
index c837605ae..b4fccedd0 100644
--- a/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/UserInfoResource.java
+++ b/gateway-service-knoxidf/src/main/java/org/apache/knox/gateway/service/knoxidf/UserInfoResource.java
@@ -43,7 +43,7 @@ import java.util.HashMap;
import java.util.Map;
import java.util.stream.Collectors;
-import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.BASE_RESORCE_PATH;
+import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.BASE_RESOURCE_PATH;
import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.SCOPE_ATTRIBUTE;
import static org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants.TOKEN_ID_ATTRIBUTE;
import static org.apache.knox.gateway.util.knoxidf.KnoxIDFUtils.error;
@@ -53,7 +53,7 @@ import static org.apache.knox.gateway.util.knoxidf.KnoxIDFUtils.error;
@Produces(MediaType.APPLICATION_JSON)
public class UserInfoResource {
- static final String RESOURCE_PATH = BASE_RESORCE_PATH + "/userinfo";
+ static final String RESOURCE_PATH = BASE_RESOURCE_PATH + "/userinfo";
private UserParamsProvider userParamsProvider;
@Context
@@ -117,11 +117,8 @@ public class UserInfoResource {
claims.put("federated_sub", federatedIdentity.getExternalSubject());
claims.put("federated_iss", federatedIdentity.getExternalIssuer());
- // Add nonce if available
- String nonce = tokenMetadata.getMetadata("nonce");
- if (StringUtils.isNotBlank(nonce)) {
- claims.put("nonce", nonce);
- }
+ // Note: nonce is deliberately NOT returned here. Per OIDC it belongs in the id_token
+ // only; echoing it from the UserInfo endpoint is a spec violation and serves no purpose.
userInfo.putAll(claims);
} else {
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 080cc32de..10791d7be 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
@@ -52,7 +52,6 @@ public class LdapUserParamsProvider implements UserParamsProvider {
private static final String DEFAULT_BASE_DN = "dc=hadoop,dc=apache,dc=org";
private static final String DEFAULT_USER_DN_TEMPLATE = "uid=%s,ou=people," + DEFAULT_BASE_DN;
private static final String DEFAULT_SYSTEM_USER = "uid=admin,ou=people," + DEFAULT_BASE_DN;
- private static final String DEFAULT_SYSTEM_PASSWORD = "admin-password";
private static final String[] ATTRIBUTES = {"cn", "sn", "givenName", "mail"};
@@ -80,9 +79,11 @@ public class LdapUserParamsProvider implements UserParamsProvider {
final AliasService aliasService = services.getService(ServiceType.ALIAS_SERVICE);
try {
final char[] systemPassword = aliasService.getPasswordFromAliasForGateway(LDAP_SYSTEM_PASSWORD_ALIAS);
- return systemPassword == null ? DEFAULT_SYSTEM_PASSWORD : new String(systemPassword);
+ // No hardcoded fallback: if the alias is absent/unresolvable, return null so the LDAP
+ // bind fails fast rather than silently binding with a well-known demo password.
+ return systemPassword == null ? null : new String(systemPassword);
} catch (AliasServiceException e) {
- return DEFAULT_SYSTEM_PASSWORD;
+ return null;
}
}
@@ -179,6 +180,10 @@ public class LdapUserParamsProvider implements UserParamsProvider {
}
private LdapContext createSystemContext() throws Exception {
+ if (ldapSystemPassword == null) {
+ throw new IllegalStateException("No LDAP system password configured. Set the '"
+ + LDAP_SYSTEM_PASSWORD_ALIAS + "' alias; there is no default password.");
+ }
Hashtable<String, Object> env = new Hashtable<>();
env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
env.put(Context.PROVIDER_URL, ldapUrl);
diff --git a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/KnoxIDFUtilsErrorStatusTest.java b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/KnoxIDFUtilsErrorStatusTest.java
new file mode 100644
index 000000000..d7cd7fb55
--- /dev/null
+++ b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/KnoxIDFUtilsErrorStatusTest.java
@@ -0,0 +1,83 @@
+/*
+ * 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.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import javax.ws.rs.core.Response;
+
+import org.apache.knox.gateway.util.knoxidf.KnoxIDFUtils;
+import org.junit.Test;
+
+/**
+ * Verifies {@link KnoxIDFUtils#error(String, String)} maps each OAuth 2.0 error code to the HTTP
+ * status RFC 6749 ยง5.2 prescribes, rather than the previous always-401 behavior.
+ */
+public class KnoxIDFUtilsErrorStatusTest {
+
+ private static int statusOf(String error) {
+ return KnoxIDFUtils.error(error, "desc").getStatus();
+ }
+
+ @Test
+ public void testInvalidClientIsUnauthorized() {
+ assertEquals(401, statusOf("invalid_client"));
+ }
+
+ @Test
+ public void testAccessDeniedIsForbidden() {
+ assertEquals(403, statusOf("access_denied"));
+ }
+
+ @Test
+ public void testServerErrorIs500() {
+ assertEquals(500, statusOf("server_error"));
+ }
+
+ @Test
+ public void testTemporarilyUnavailableIs503() {
+ assertEquals(503, statusOf("temporarily_unavailable"));
+ }
+
+ @Test
+ public void testProtocolErrorsDefaultToBadRequest() {
+ for (final String error : new String[]{"invalid_request", "invalid_grant", "invalid_scope",
+ "unsupported_grant_type", "unsupported_response_type", "unauthorized_client"}) {
+ assertEquals("Expected 400 for " + error, 400, statusOf(error));
+ }
+ }
+
+ @Test
+ public void testUnknownAndNullErrorDefaultToBadRequest() {
+ assertEquals(400, statusOf("something_unexpected"));
+ assertEquals(400, statusOf(null));
+ }
+
+ @Test
+ public void testExplicitStatusOverloadWins() {
+ final Response response = KnoxIDFUtils.error("invalid_request", "desc", Response.Status.CONFLICT);
+ assertEquals(409, response.getStatus());
+ }
+
+ @Test
+ public void testBodyCarriesErrorCodeAndDescription() {
+ final String body = String.valueOf(KnoxIDFUtils.error("invalid_grant", "bad code").getEntity());
+ assertTrue(body.contains("invalid_grant"));
+ assertTrue(body.contains("bad code"));
+ }
+}
diff --git a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/RegistrationRedirectUriPolicyTest.java b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/RegistrationRedirectUriPolicyTest.java
new file mode 100644
index 000000000..db91f336c
--- /dev/null
+++ b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/RegistrationRedirectUriPolicyTest.java
@@ -0,0 +1,76 @@
+/*
+ * 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.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import java.util.Collections;
+import java.util.List;
+
+import javax.ws.rs.core.Response;
+
+import org.junit.Test;
+
+/**
+ * Verifies the dynamic-registration redirect-URI policy: HTTPS is required (RFC 8252), plain HTTP is
+ * tolerated only for loopback dev, and wildcard hosts are rejected.
+ */
+public class RegistrationRedirectUriPolicyTest {
+
+ private static Response verify(String... uris) {
+ return RegistrationResource.verifyRedirectUris(java.util.Arrays.asList(uris));
+ }
+
+ @Test
+ public void testHttpsAccepted() {
+ assertNull("A plain https redirect must be accepted.", verify("https://app.example.com/cb"));
+ }
+
+ @Test
+ public void testPlainHttpRejectedForNonLoopback() {
+ final Response response = verify("http://app.example.com/cb");
+ assertEquals(400, response.getStatus());
+ assertTrue(String.valueOf(response.getEntity()).contains("HTTPS"));
+ }
+
+ @Test
+ public void testPlainHttpAllowedForLoopback() {
+ assertNull(verify("http://localhost:8080/cb"));
+ assertNull(verify("http://127.0.0.1/cb"));
+ assertNull(verify("http://[::1]:9000/cb"));
+ }
+
+ @Test
+ public void testWildcardHostRejected() {
+ final Response response = verify("https://*.example.com/cb");
+ assertEquals(400, response.getStatus());
+ }
+
+ @Test
+ public void testEmptyListRejected() {
+ assertEquals(400, verify().getStatus());
+ assertEquals(400, RegistrationResource.verifyRedirectUris(Collections.<String>emptyList()).getStatus());
+ assertEquals(400, RegistrationResource.verifyRedirectUris((List<String>) null).getStatus());
+ }
+
+ @Test
+ public void testOneBadUriAmongGoodOnesRejectsWhole() {
+ assertEquals(400, verify("https://good.example.com/cb", "http://evil.example.com/cb").getStatus());
+ }
+}
diff --git a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceAuthCodeReplayTest.java b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceAuthCodeReplayTest.java
index ac0fb6cab..92e6632ed 100644
--- a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceAuthCodeReplayTest.java
+++ b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/TokenResourceAuthCodeReplayTest.java
@@ -144,7 +144,7 @@ public class TokenResourceAuthCodeReplayTest {
final Response response = resource.handleAuthorizationCodeFlow();
assertEquals("A code already consumed by a concurrent redemption must be rejected.",
- Response.Status.UNAUTHORIZED.getStatusCode(), response.getStatus());
+ Response.Status.BAD_REQUEST.getStatusCode(), response.getStatus());
assertTrue("The error body should identify the invalid_grant condition.",
String.valueOf(response.getEntity()).contains("invalid_grant"));
assertEquals("A losing redemption must not mint any token.", 0, issuedCount.get());
diff --git a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFArtifactStore.java b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFArtifactStore.java
index dbeba8141..94ab55122 100644
--- a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFArtifactStore.java
+++ b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFArtifactStore.java
@@ -26,6 +26,10 @@ public abstract class KnoxIDFArtifactStore<T> {
private final Cache<String, T> cache;
protected KnoxIDFArtifactStore(long ttl) {
+ // Entries live for twice the caller's TTL as a deliberate grace window: an artifact (e.g. an
+ // in-flight authorize/consent request) is created against the token TTL but must survive the
+ // extra round-trip through the browser/consent screen before it is consumed. Callers that
+ // finish with an entry earlier should remove() it rather than wait for expiry.
this.cache = Caffeine.newBuilder().expireAfterWrite(ttl * 2, TimeUnit.MILLISECONDS).build();
}
@@ -36,4 +40,9 @@ public abstract class KnoxIDFArtifactStore<T> {
public T get(String key) {
return cache.getIfPresent(key);
}
+
+ /** Invalidates an entry so a single-use artifact cannot be replayed within its TTL grace window. */
+ public void remove(String key) {
+ cache.invalidate(key);
+ }
}
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 57e8b3caf..8cad77b0b 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
@@ -16,22 +16,25 @@
*/
package org.apache.knox.gateway.util.knoxidf;
-import com.google.common.collect.Sets;
+import com.google.common.collect.ImmutableSet;
import java.util.Set;
public interface KnoxIDFConstants {
- String BASE_RESORCE_PATH = "knoxidf/api/v1";
+ String BASE_RESOURCE_PATH = "knoxidf/api/v1";
String AUTH_CODE = "authorization_code";
String CLIENT_ID = "client_id";
String REDIRECT_URI = "redirect_uri";
String REDIRECT_URIS = "redirect_uris";
String RESPONSE_TYPE = "response_type";
- Set<String> ALLOWED_RESPONSE_TYPES = Sets.newHashSet("code", "id_token", "code id_token");
+ // Immutable: an interface field is implicitly public static final, but a mutable HashSet would
+ // still let any caller add()/remove() on the shared instance. ImmutableSet forbids that.
+ Set<String> ALLOWED_RESPONSE_TYPES = ImmutableSet.of("code", "id_token", "code id_token");
String SCOPE = "scope";
String ALLOWED_SCOPES = "allowed_scopes";
String OFFLINE_ACCESS_SCOPE = "offline_access";
- Set<String> DEFAULT_SCOPES = Sets.newHashSet("openid", "profile", "email", OFFLINE_ACCESS_SCOPE);
+ // Immutable shared constant; callers that need a mutable working set copy it (new HashSet<>(...)).
+ Set<String> DEFAULT_SCOPES = ImmutableSet.of("openid", "profile", "email", OFFLINE_ACCESS_SCOPE);
String OPENID_SCOPE = SCOPE + "=openid";
String STATE = "state";
String CODE = "code";
diff --git a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFUtils.java b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFUtils.java
index 98aeb8520..423ad37ad 100644
--- a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFUtils.java
+++ b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFUtils.java
@@ -23,6 +23,8 @@ import org.apache.knox.gateway.util.JsonUtils;
import javax.servlet.ServletContext;
import javax.servlet.http.HttpServletRequest;
import javax.ws.rs.core.Response;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
@@ -33,11 +35,42 @@ import java.util.Set;
public class KnoxIDFUtils {
+ /**
+ * Builds an OAuth 2.0 error response, deriving the HTTP status from the error code per
+ * RFC 6749 ยง5.2 (rather than the previous always-401). Most protocol errors are client errors
+ * (400); {@code invalid_client} is an authentication failure (401), {@code access_denied} is a
+ * policy denial (403), and {@code server_error} is 500. Use the three-arg overload to override
+ * the status explicitly when a call site needs a status the code alone does not imply.
+ */
public static Response error(String error, String description) {
+ return error(error, description, statusForError(error));
+ }
+
+ public static Response error(String error, String description, Response.Status status) {
final Map<String, String> errorMap = new HashMap<>();
errorMap.put("error", error);
errorMap.put("error_description", description);
- return Response.status(Response.Status.UNAUTHORIZED).entity(JsonUtils.renderAsJsonString(errorMap)).build();
+ return Response.status(status).entity(JsonUtils.renderAsJsonString(errorMap)).build();
+ }
+
+ private static Response.Status statusForError(String error) {
+ if (error == null) {
+ return Response.Status.BAD_REQUEST;
+ }
+ switch (error) {
+ case "invalid_client":
+ return Response.Status.UNAUTHORIZED; // 401
+ case "access_denied":
+ return Response.Status.FORBIDDEN; // 403
+ case "server_error":
+ return Response.Status.INTERNAL_SERVER_ERROR; // 500
+ case "temporarily_unavailable":
+ return Response.Status.SERVICE_UNAVAILABLE; // 503
+ default:
+ // invalid_request, invalid_grant, invalid_scope, unsupported_grant_type,
+ // unsupported_response_type, unauthorized_client are all 400s.
+ return Response.Status.BAD_REQUEST; // 400
+ }
}
public static String getRequestParamSafe(final HttpServletRequest request, final String key) {
@@ -59,7 +92,9 @@ public class KnoxIDFUtils {
final String responseType = request.getParameter(KnoxIDFConstants.RESPONSE_TYPE);
final String redirectUri = request.getParameter(KnoxIDFConstants.REDIRECT_URI);
final String scope = request.getParameter(KnoxIDFConstants.SCOPE);
- final Set<String> requestedScopes = StringUtils.isBlank(scope) ? KnoxIDFConstants.DEFAULT_SCOPES : new HashSet<>(Arrays.asList(scope.split("\\s+")));
+ // Copy DEFAULT_SCOPES into a mutable set: the constant is now an ImmutableSet, and callers
+ // downstream may add/remove scopes on the returned set.
+ final Set<String> requestedScopes = StringUtils.isBlank(scope) ? new HashSet<>(KnoxIDFConstants.DEFAULT_SCOPES) : new HashSet<>(Arrays.asList(scope.split("\\s+")));
final String state = request.getParameter(KnoxIDFConstants.STATE);
final String nonce = request.getParameter(KnoxIDFConstants.NONCE);
final String codeChallenge = request.getParameter(KnoxIDFConstants.CODE_CHALLENGE);
@@ -68,12 +103,20 @@ public class KnoxIDFUtils {
}
public static String buildFederatedOpAuthRedirect(final FederatedOpConfiguration federatedOpConfiguration, final String federatedState) {
+ // URL-encode every value placed into the query string. client_id and the callback URI
+ // (which itself contains ':' '/' '?' etc.) and the state must be percent-encoded or the
+ // OP receives a malformed/parameter-split URL. CODE_RESPONSE_TYPE and OPENID_SCOPE are
+ // fixed "key=value" literals with no reserved characters, so they are appended as-is.
return federatedOpConfiguration.getAuthorizeEndpoint()
- + "?" + KnoxIDFConstants.CLIENT_ID + "=" + federatedOpConfiguration.getClientId()
- + "&" + KnoxIDFConstants.REDIRECT_URI + "=" + federatedOpConfiguration.getAuthorizeCallback()
+ + "?" + KnoxIDFConstants.CLIENT_ID + "=" + urlEncode(federatedOpConfiguration.getClientId())
+ + "&" + KnoxIDFConstants.REDIRECT_URI + "=" + urlEncode(federatedOpConfiguration.getAuthorizeCallback())
+ "&" + KnoxIDFConstants.CODE_RESPONSE_TYPE
+ "&" + KnoxIDFConstants.OPENID_SCOPE
- + "&" + KnoxIDFConstants.STATE + "=" + federatedState;
+ + "&" + KnoxIDFConstants.STATE + "=" + urlEncode(federatedState);
+ }
+
+ private static String urlEncode(final String value) {
+ return value == null ? "" : URLEncoder.encode(value, StandardCharsets.UTF_8);
}
}
diff --git a/gateway-util-common/src/test/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFArtifactStoreTest.java b/gateway-util-common/src/test/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFArtifactStoreTest.java
new file mode 100644
index 000000000..04dbb9e84
--- /dev/null
+++ b/gateway-util-common/src/test/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFArtifactStoreTest.java
@@ -0,0 +1,63 @@
+/*
+ * 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 org.junit.Test;
+
+/**
+ * Verifies the {@link KnoxIDFArtifactStore#remove(String)} single-use invalidation added so an
+ * artifact (e.g. an authorize/consent state) cannot be replayed within its TTL grace window.
+ */
+public class KnoxIDFArtifactStoreTest {
+
+ /** Minimal concrete store; the base class is abstract. TTL is large so nothing expires mid-test. */
+ private static final class TestStore extends KnoxIDFArtifactStore<String> {
+ TestStore() {
+ super(60_000L);
+ }
+ }
+
+ @Test
+ public void testPutThenGetReturnsValue() {
+ final TestStore store = new TestStore();
+ store.put("k", "v");
+ assertEquals("v", store.get("k"));
+ }
+
+ @Test
+ public void testRemoveInvalidatesEntry() {
+ final TestStore store = new TestStore();
+ store.put("state", "payload");
+ store.remove("state");
+ assertNull("A removed entry must not be retrievable (single-use replay guard).", store.get("state"));
+ }
+
+ @Test
+ public void testRemoveIsIdempotentAndSafeForUnknownKey() {
+ final TestStore store = new TestStore();
+ store.remove("never-put"); // must not throw
+ assertNull(store.get("never-put"));
+ }
+
+ @Test
+ public void testGetUnknownKeyReturnsNull() {
+ assertNull(new TestStore().get("missing"));
+ }
+}