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 1473102923fa25c21294c75e4938d8c0c83ff93a Author: Sandor Molnar <[email protected]> AuthorDate: Tue Aug 11 23:28:13 2026 +0200 KNOX-3414: fail closed when a federated-OP client-secret alias is unresolvable (review finding M4) resolveClientSecret returned null when a configured client-secret alias could not be resolved. That null flowed into the token-request form as a BasicNameValuePair, which the URL encoder serialized to a literal client_secret=null and sent to the OP -- the opposite of fail-closed: instead of refusing, Knox made a back-channel call with a bogus secret. Make it truly fail closed: requireResolvedAliasSecret throws ClientSecretResolutionException when a declared alias resolves to nothing (null or empty), and authCallback catches it and returns a clear server_error before any HTTP request to the OP. The plaintext clientSecret fallback (no alias configured) is unchanged for backward compatibility. Covered by AuthorizeResourceClientSecretResolutionTest. Co-Authored-By: Claude Opus 4.8 <[email protected]> --- .../gateway/service/knoxidf/AuthorizeResource.java | 46 ++++++++++++++--- ...uthorizeResourceClientSecretResolutionTest.java | 59 ++++++++++++++++++++++ 2 files changed, 98 insertions(+), 7 deletions(-) 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 9747d3c63..4b6d7ec7a 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 @@ -300,7 +300,14 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { federatedOpConfigurationStore.remove(state); final String expectedNonce = federatedNonceStore.get(state); federatedNonceStore.remove(state); - final Pair<String, String> federatedTokens = exchangeFederatedAuthCodeToTokens(federatedAuthCode, federatedOpConfiguration); + final Pair<String, String> federatedTokens; + try { + federatedTokens = exchangeFederatedAuthCodeToTokens(federatedAuthCode, federatedOpConfiguration); + } catch (ClientSecretResolutionException e) { + // A configured client-secret alias could not be resolved. This is a server-side + // misconfiguration, not a client error, and we deliberately never made the OP call. + return error("server_error", e.getMessage()); + } if (StringUtils.isBlank(federatedTokens.getLeft())) { return error("invalid_request", "Federated OP did not return an id_token"); } @@ -490,25 +497,50 @@ public class AuthorizeResource extends PasscodeTokenResourceBase { * preferred, secure source and takes precedence: when it resolves to a value, that value is * used and the plaintext {@code clientSecret} topology param is never consulted. The plaintext * param remains supported as a fallback only when no alias is configured, so existing - * deployments keep working. If an alias is configured but cannot be resolved we fail closed - * (return {@code null}) rather than silently leaking through to the plaintext param, so a - * misconfigured alias surfaces as an auth failure instead of masking the intended secure source. + * deployments keep working. If an alias is configured but cannot be resolved we fail closed by + * throwing {@link ClientSecretResolutionException} rather than returning {@code null} (which the + * form encoder would have serialized to a literal {@code client_secret=null} sent to the OP) or + * silently leaking through to the plaintext param. The caller aborts the exchange before any + * HTTP request, so a misconfigured alias surfaces as a clear error instead of a bogus OP call. */ private String resolveClientSecret(final FederatedOpConfiguration opConfig) { final String alias = opConfig.getClientSecretAlias(); if (StringUtils.isBlank(alias)) { return opConfig.getClientSecret(); } + char[] secret = null; try { final AliasService aliasService = getGatewayServices().getService(ServiceType.ALIAS_SERVICE); String clusterName = (String) servletContext.getAttribute(GatewayServices.GATEWAY_CLUSTER_ATTRIBUTE); if (StringUtils.isBlank(clusterName)) { clusterName = AliasService.NO_CLUSTER_NAME; } - final char[] secret = aliasService.getPasswordFromAliasForCluster(clusterName, alias, false); - return secret == null ? null : new String(secret); + secret = aliasService.getPasswordFromAliasForCluster(clusterName, alias, false); } catch (AliasServiceException e) { - return null; + // Fall through to the fail-closed check below; an alias was configured but its lookup failed. + secret = null; + } + return requireResolvedAliasSecret(alias, secret); + } + + /** + * Fail-closed guard for an explicitly configured client-secret alias: returns the resolved secret + * or throws {@link ClientSecretResolutionException} when it is absent/empty. Package-private and + * pure so the fail-closed decision is unit-testable without a live {@code AliasService}. + */ + static String requireResolvedAliasSecret(final String alias, final char[] secret) { + if (secret == null || secret.length == 0) { + throw new ClientSecretResolutionException( + "Federated OP client secret alias '" + alias + "' is configured but could not be resolved; " + + "refusing to contact the OP without the intended secret"); + } + return new String(secret); + } + + /** Signals that a configured client-secret alias could not be resolved; the exchange must abort. */ + static final class ClientSecretResolutionException extends RuntimeException { + ClientSecretResolutionException(final String message) { + super(message); } } diff --git a/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResourceClientSecretResolutionTest.java b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResourceClientSecretResolutionTest.java new file mode 100644 index 000000000..f67937c61 --- /dev/null +++ b/gateway-service-knoxidf/src/test/java/org/apache/knox/gateway/service/knoxidf/AuthorizeResourceClientSecretResolutionTest.java @@ -0,0 +1,59 @@ +/* + * 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 static org.junit.Assert.fail; + +import org.apache.knox.gateway.service.knoxidf.AuthorizeResource.ClientSecretResolutionException; +import org.junit.Test; + +/** + * Verifies the fail-closed handling of a configured federated-OP client-secret alias (review + * finding M4). When an alias is declared but resolves to nothing, the token exchange must abort + * with a clear error and never contact the OP -- previously an unresolvable alias yielded a null + * secret that the form encoder serialized to a literal {@code client_secret=null} sent to the OP. + */ +public class AuthorizeResourceClientSecretResolutionTest { + + @Test + public void testResolvedSecretIsReturned() { + final String secret = AuthorizeResource.requireResolvedAliasSecret("op.secret.alias", "s3cr3t".toCharArray()); + assertEquals("A resolved alias must yield its secret value.", "s3cr3t", secret); + } + + @Test + public void testUnresolvableAliasFailsClosed() { + try { + AuthorizeResource.requireResolvedAliasSecret("op.secret.alias", null); + fail("A configured-but-unresolvable alias must fail closed, not return null."); + } catch (ClientSecretResolutionException e) { + assertTrue("The error should name the offending alias.", e.getMessage().contains("op.secret.alias")); + } + } + + @Test + public void testEmptyResolvedSecretFailsClosed() { + try { + AuthorizeResource.requireResolvedAliasSecret("op.secret.alias", new char[0]); + fail("An alias resolving to an empty secret must fail closed."); + } catch (ClientSecretResolutionException e) { + // expected + } + } +}
