You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tomee.apache.org by rz...@apache.org on 2026/04/23 13:50:59 UTC
(tomee) 21/23: security/openid: honour discovered token_endpoint_auth_methods, harden in-memory store
This is an automated email from the ASF dual-hosted git repository.
rzo1 pushed a commit to branch security-4.0
in repository https://gitbox.apache.org/repos/asf/tomee.git
commit e1366586429c1868f715fe9d2f31c273fa934df3
Author: Richard Zowalla <rz...@apache.org>
AuthorDate: Tue Apr 21 08:24:21 2026 +0200
security/openid: honour discovered token_endpoint_auth_methods, harden in-memory store
preferBasicAuth() previously reflected on tokenEndpointAuthMethodsSupported() which
does not exist on jakarta.security.enterprise-api 4.0 OpenIdProviderMetadata; the
NoSuchMethodException path always fired, making the form-fallback branch unreachable.
Move the decision onto CompositeOpenIdProviderMetadata (which already owns the
discovery JsonObject) via a new tokenEndpointAuthMethodsSupported() accessor, and
replace the reflection with an instanceof check. Plain-annotation deployments still
fall through to the OIDC Core §9 default of client_secret_basic.
Also:
- TomEEInMemoryIdentityStore: encode the supplied password char[] to UTF-8 via
StandardCharsets.UTF_8.encode(CharBuffer.wrap(...)) instead of new String(chars),
so plaintext no longer lands in the String pool; scrub the temporary byte[] in a
finally block. Caller-name comparison now uses MessageDigest.isEqual on UTF-8
bytes (constant-time in the supplied length) instead of String.equals.
- BeanContext.securityRoleReferences: HashMap -> LinkedHashMap so
getSecurityRoleReferences() iterates in registration order.
- OpenIdAuthenticationMechanismUnitTest: rewrite the client_secret_post-only test
against a real CompositeOpenIdProviderMetadata built from a discovery JSON so it
exercises the production preferBasicAuth() path (dropping the subclass override);
add tokenEndpointUsesClientSecretBasicWhenDiscoveryAdvertisesIt for the positive
discovery case.
---
.../main/java/org/apache/openejb/BeanContext.java | 4 +-
.../cdi/OpenIdAuthenticationMechanism.java | 55 ++++-------
.../openid/CompositeOpenIdProviderMetadata.java | 25 +++++
.../identitystore/TomEEInMemoryIdentityStore.java | 66 ++++++++-----
.../cdi/OpenIdAuthenticationMechanismUnitTest.java | 103 +++++++++++++++++----
5 files changed, 176 insertions(+), 77 deletions(-)
diff --git a/container/openejb-core/src/main/java/org/apache/openejb/BeanContext.java b/container/openejb-core/src/main/java/org/apache/openejb/BeanContext.java
index a2b03b90b2..e86eba5aff 100644
--- a/container/openejb-core/src/main/java/org/apache/openejb/BeanContext.java
+++ b/container/openejb-core/src/main/java/org/apache/openejb/BeanContext.java
@@ -366,7 +366,9 @@ public class BeanContext extends DeploymentContext {
private Cmp cmp;
private LegacyView legacyView;
- private final Map<String, String> securityRoleReferences = new HashMap<>();
+ // LinkedHashMap so getSecurityRoleReferences() returns role-refs in the
+ // order the container registered them, not the non-deterministic HashMap order.
+ private final Map<String, String> securityRoleReferences = new LinkedHashMap<>();
/**
* TODO: Move to MethodContext
diff --git a/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanism.java b/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanism.java
index 9b4afb088b..504009575d 100644
--- a/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanism.java
+++ b/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanism.java
@@ -45,13 +45,13 @@ import org.apache.openejb.util.Logger;
import org.apache.tomee.security.cdi.openid.TomEEOpenIdContext;
import org.apache.tomee.security.cdi.openid.storage.OpenIdStorageHandler;
import org.apache.tomee.security.http.SavedRequest;
+import org.apache.tomee.security.http.openid.CompositeOpenIdProviderMetadata;
import org.apache.tomee.security.http.openid.model.TokenResponse;
import org.apache.tomee.security.http.openid.model.TomEEOpenIdCredential;
import javax.security.auth.callback.Callback;
import javax.security.auth.callback.UnsupportedCallbackException;
import java.io.IOException;
-import java.lang.reflect.Method;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
@@ -439,52 +439,33 @@ public class OpenIdAuthenticationMechanism implements HttpAuthenticationMechanis
* when talking to the token endpoint.
*
* <p>Per OIDC Core 1.0 §9 both methods are legal and {@code client_secret_basic} is the
- * default. When the {@link jakarta.security.enterprise.authentication.mechanism.http.openid.OpenIdProviderMetadata
- * OpenIdProviderMetadata} annotation exposes a {@code tokenEndpointAuthMethodsSupported()}
- * accessor (reflectively detected so we don't couple to a specific spec revision), we honour
- * it: if {@code client_secret_basic} is in the advertised list we prefer Basic; if only
- * {@code client_secret_post} is advertised we fall back to form parameters. An empty or
- * {@code null} array is treated as "not advertised" and we fall back to the OIDC default
- * of Basic.
- *
- * <p>When the accessor does not exist on the bundled API (Jakarta Security 4.0 currently
- * omits it), we unconditionally prefer Basic — the spec-mandated default — which also keeps
- * the client secret out of HTTP access logs / form bodies.</p>
+ * default OP-mandated behaviour. The spec-level {@link jakarta.security.enterprise.authentication.mechanism.http.openid.OpenIdProviderMetadata
+ * OpenIdProviderMetadata} annotation does not expose {@code token_endpoint_auth_methods_supported},
+ * but when TomEE discovers the OP via {@code providerURI} the raw JSON document is available
+ * on {@link CompositeOpenIdProviderMetadata}. If the discovered list advertises
+ * {@code client_secret_post} without {@code client_secret_basic} we fall back to form parameters;
+ * otherwise (list absent, empty, or contains Basic) we prefer Basic.</p>
*/
protected boolean preferBasicAuth() {
final Object providerMetadata = getDefinition().providerMetadata();
- if (providerMetadata == null) {
+ if (!(providerMetadata instanceof CompositeOpenIdProviderMetadata)) {
+ // No discovery document to consult: default to Basic (OIDC default).
return true;
}
- try {
- final Method accessor = providerMetadata.getClass().getMethod("tokenEndpointAuthMethodsSupported");
- final Object value = accessor.invoke(providerMetadata);
- if (!(value instanceof String[])) {
- return true;
- }
+ final String[] methods = ((CompositeOpenIdProviderMetadata) providerMetadata).tokenEndpointAuthMethodsSupported();
+ if (methods == null || methods.length == 0) {
+ return true;
+ }
- final String[] methods = (String[]) value;
- if (methods.length == 0) {
- // Missing / empty => OIDC default; prefer Basic.
+ for (final String method : methods) {
+ if ("client_secret_basic".equalsIgnoreCase(method)) {
return true;
}
-
- for (String method : methods) {
- if ("client_secret_basic".equalsIgnoreCase(method)) {
- return true;
- }
- }
-
- // Advertised list exists and does not include client_secret_basic => must use form.
- return false;
- } catch (NoSuchMethodException e) {
- // Bundled API doesn't expose the accessor; default to Basic (OIDC default).
- return true;
- } catch (ReflectiveOperationException e) {
- LOGGER.debug("Unable to read tokenEndpointAuthMethodsSupported(), defaulting to client_secret_basic: " + e.getMessage());
- return true;
}
+
+ // Advertised list exists and does not include client_secret_basic => must use form.
+ return false;
}
protected String basicAuthHeader() {
diff --git a/tomee/tomee-security/src/main/java/org/apache/tomee/security/http/openid/CompositeOpenIdProviderMetadata.java b/tomee/tomee-security/src/main/java/org/apache/tomee/security/http/openid/CompositeOpenIdProviderMetadata.java
index a39a69f6c3..7f7f22646d 100644
--- a/tomee/tomee-security/src/main/java/org/apache/tomee/security/http/openid/CompositeOpenIdProviderMetadata.java
+++ b/tomee/tomee-security/src/main/java/org/apache/tomee/security/http/openid/CompositeOpenIdProviderMetadata.java
@@ -16,6 +16,7 @@
*/
package org.apache.tomee.security.http.openid;
+import jakarta.json.JsonArray;
import jakarta.json.JsonObject;
import jakarta.json.JsonString;
import jakarta.security.enterprise.authentication.mechanism.http.openid.OpenIdConstant;
@@ -131,6 +132,30 @@ public class CompositeOpenIdProviderMetadata implements OpenIdProviderMetadata {
}
+ /**
+ * OIDC Discovery 1.0 §3 {@code token_endpoint_auth_methods_supported}: the client authentication
+ * methods supported by the OP's token endpoint (e.g. {@code client_secret_basic},
+ * {@code client_secret_post}). Not on the spec-level {@link OpenIdProviderMetadata} annotation,
+ * so this is exposed as a regular method and read directly from the discovery JSON.
+ *
+ * @return the advertised methods, or an empty array if the OP did not advertise any.
+ */
+ public String[] tokenEndpointAuthMethodsSupported() {
+ if (!openidProviderMetadata.containsKey(OpenIdConstant.TOKEN_ENDPOINT_AUTH_METHODS_SUPPORTED)) {
+ return new String[0];
+ }
+
+ final JsonArray array = openidProviderMetadata.getJsonArray(OpenIdConstant.TOKEN_ENDPOINT_AUTH_METHODS_SUPPORTED);
+ if (array == null) {
+ return new String[0];
+ }
+
+ return array.stream()
+ .map(JsonString.class::cast)
+ .map(JsonString::getString)
+ .toArray(String[]::new);
+ }
+
@Override
public Class<? extends Annotation> annotationType() {
return OpenIdProviderMetadata.class;
diff --git a/tomee/tomee-security/src/main/java/org/apache/tomee/security/identitystore/TomEEInMemoryIdentityStore.java b/tomee/tomee-security/src/main/java/org/apache/tomee/security/identitystore/TomEEInMemoryIdentityStore.java
index fc9556f212..406c18fd1a 100644
--- a/tomee/tomee-security/src/main/java/org/apache/tomee/security/identitystore/TomEEInMemoryIdentityStore.java
+++ b/tomee/tomee-security/src/main/java/org/apache/tomee/security/identitystore/TomEEInMemoryIdentityStore.java
@@ -25,6 +25,8 @@ import jakarta.security.enterprise.identitystore.CredentialValidationResult;
import jakarta.security.enterprise.identitystore.IdentityStore;
import jakarta.security.enterprise.identitystore.InMemoryIdentityStoreDefinition;
+import java.nio.ByteBuffer;
+import java.nio.CharBuffer;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.util.Arrays;
@@ -36,8 +38,9 @@ import static java.util.Collections.emptySet;
/**
* Jakarta Security 4.0 {@link InMemoryIdentityStoreDefinition}
- * implementation. Credentials are declared directly on the annotation and matched with a constant-time byte comparison
- * to avoid leaking timing information about which callers are present.
+ * implementation. Credentials are declared directly on the annotation and matched with constant-time byte
+ * comparison (both password and caller name) to avoid leaking timing information about which callers are
+ * present.
*/
@ApplicationScoped
public class TomEEInMemoryIdentityStore implements IdentityStore {
@@ -65,28 +68,37 @@ public class TomEEInMemoryIdentityStore implements IdentityStore {
return CredentialValidationResult.NOT_VALIDATED_RESULT;
}
- final String caller = usernamePasswordCredential.getCaller();
final char[] suppliedPasswordChars = usernamePasswordCredential.getPassword().getValue();
+ // Encode char[] -> UTF-8 bytes without going through String, so the plaintext
+ // password never gets interned/retained outside of a scrubbable buffer.
final byte[] suppliedPassword = toUtf8Bytes(suppliedPasswordChars);
+ final byte[] suppliedCaller = toUtf8Bytes(usernamePasswordCredential.getCaller());
+
+ try {
+ InMemoryIdentityStoreDefinition.Credentials match = null;
+ // Walk every entry once, regardless of a hit, so runtime stays roughly independent
+ // of which (or how many) callers are declared and where the match occurs.
+ for (final InMemoryIdentityStoreDefinition.Credentials entry : definition.value()) {
+ final byte[] declaredPassword = entry.password().getBytes(StandardCharsets.UTF_8);
+ final byte[] declaredCaller = entry.callerName().getBytes(StandardCharsets.UTF_8);
+ // MessageDigest.isEqual is time-constant in the length of its second argument, so
+ // both comparisons leak at most the declared (public, annotation-known) lengths.
+ final boolean passwordMatches = MessageDigest.isEqual(declaredPassword, suppliedPassword);
+ final boolean callerMatches = MessageDigest.isEqual(declaredCaller, suppliedCaller);
+ if (passwordMatches && callerMatches && match == null) {
+ match = entry;
+ }
+ }
- InMemoryIdentityStoreDefinition.Credentials match = null;
- // Walk every entry once, regardless of a hit, so runtime stays roughly independent
- // of which (or how many) callers are declared and where the match occurs.
- for (final InMemoryIdentityStoreDefinition.Credentials entry : definition.value()) {
- final byte[] declaredPassword = entry.password().getBytes(StandardCharsets.UTF_8);
- final boolean passwordMatches = MessageDigest.isEqual(declaredPassword, suppliedPassword);
- // Compare caller after password so we still compute the digest work for unknown callers.
- if (passwordMatches && entry.callerName().equals(caller) && match == null) {
- match = entry;
+ if (match == null) {
+ return CredentialValidationResult.INVALID_RESULT;
}
- }
- if (match == null) {
- return CredentialValidationResult.INVALID_RESULT;
+ final Set<String> groups = new HashSet<>(Arrays.asList(match.groups()));
+ return new CredentialValidationResult(match.callerName(), groups);
+ } finally {
+ Arrays.fill(suppliedPassword, (byte) 0);
}
-
- final Set<String> groups = new HashSet<>(Arrays.asList(match.groups()));
- return new CredentialValidationResult(match.callerName(), groups);
}
@Override
@@ -108,11 +120,21 @@ public class TomEEInMemoryIdentityStore implements IdentityStore {
}
private static byte[] toUtf8Bytes(final char[] chars) {
- if (chars == null) {
+ if (chars == null || chars.length == 0) {
+ return new byte[0];
+ }
+ // Direct char[] -> UTF-8 bytes via the charset encoder, bypassing String so the
+ // plaintext never lands in the String pool or a long-lived heap slot.
+ final ByteBuffer buffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(chars));
+ final byte[] bytes = new byte[buffer.remaining()];
+ buffer.get(bytes);
+ return bytes;
+ }
+
+ private static byte[] toUtf8Bytes(final String value) {
+ if (value == null) {
return new byte[0];
}
- // Route through a String to stay aligned with how annotation-declared passwords are encoded;
- // the spec stores passwords as plain String and we compare against their UTF-8 byte form.
- return new String(chars).getBytes(StandardCharsets.UTF_8);
+ return value.getBytes(StandardCharsets.UTF_8);
}
}
diff --git a/tomee/tomee-security/src/test/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanismUnitTest.java b/tomee/tomee-security/src/test/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanismUnitTest.java
index 9adfe8c294..19cb6c2b44 100644
--- a/tomee/tomee-security/src/test/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanismUnitTest.java
+++ b/tomee/tomee-security/src/test/java/org/apache/tomee/security/cdi/OpenIdAuthenticationMechanismUnitTest.java
@@ -21,14 +21,18 @@ import com.sun.net.httpserver.HttpServer;
import org.apache.openejb.util.reflection.Reflections;
import org.apache.tomee.security.cdi.openid.TomEEOpenIdContext;
import org.apache.tomee.security.cdi.openid.storage.OpenIdStorageHandler;
+import org.apache.tomee.security.http.openid.CompositeOpenIdProviderMetadata;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Answers;
+import jakarta.json.Json;
+import jakarta.json.JsonObject;
import jakarta.security.enterprise.AuthenticationStatus;
import jakarta.security.enterprise.authentication.mechanism.http.HttpMessageContext;
import jakarta.security.enterprise.authentication.mechanism.http.OpenIdAuthenticationMechanismDefinition;
import jakarta.security.enterprise.authentication.mechanism.http.openid.OpenIdConstant;
+import jakarta.security.enterprise.authentication.mechanism.http.openid.OpenIdProviderMetadata;
import jakarta.security.enterprise.identitystore.CredentialValidationResult;
import jakarta.security.enterprise.identitystore.IdentityStoreHandler;
import jakarta.servlet.http.HttpServletRequest;
@@ -291,23 +295,22 @@ public class OpenIdAuthenticationMechanismUnitTest {
});
server.start();
- // Simulate an OP that advertises ONLY client_secret_post by subclassing the mechanism
- // and overriding the preference decision. We keep the RETURNS_DEEP_STUBS wiring so the
- // rest of the flow still runs against the shared `definition` mock.
- OpenIdAuthenticationMechanism postOnlyMechanism = new OpenIdAuthenticationMechanism() {
- @Override
- protected boolean preferBasicAuth() {
- return false;
- }
- };
- postOnlyMechanism.setDefinitionSupplier(() -> definition);
- Reflections.set(postOnlyMechanism, "identityStoreHandler", identityStoreHandler);
- Reflections.set(postOnlyMechanism, "openIdContext", new TomEEOpenIdContext());
- Reflections.set(postOnlyMechanism, "storageHandler", storageHandler);
-
try {
- when(definition.providerMetadata().tokenEndpoint())
- .thenReturn("http://127.0.0.1:" + server.getAddress().getPort() + "/token");
+ // Real CompositeOpenIdProviderMetadata built from a discovery JSON that advertises ONLY
+ // client_secret_post. Exercises the production preferBasicAuth() code path end-to-end
+ // instead of a subclass override.
+ final JsonObject discovery = Json.createObjectBuilder()
+ .add(OpenIdConstant.TOKEN_ENDPOINT,
+ "http://127.0.0.1:" + server.getAddress().getPort() + "/token")
+ .add(OpenIdConstant.TOKEN_ENDPOINT_AUTH_METHODS_SUPPORTED,
+ Json.createArrayBuilder().add("client_secret_post"))
+ .build();
+ final OpenIdProviderMetadata overrideDefaults =
+ DefaultOverrideHolder.class.getAnnotation(OpenIdProviderMetadata.class);
+ final CompositeOpenIdProviderMetadata composite =
+ new CompositeOpenIdProviderMetadata(discovery, overrideDefaults);
+
+ when(definition.providerMetadata()).thenReturn(composite);
when(identityStoreHandler.validate(any()))
.thenReturn(new CredentialValidationResult("caller", Set.of("users")));
@@ -324,7 +327,7 @@ public class OpenIdAuthenticationMechanismUnitTest {
storageHandler.set(request, response, OpenIdStorageHandler.STATE_KEY, "STATE");
- AuthenticationStatus status = postOnlyMechanism.performAuthentication(request, response, messageContext);
+ AuthenticationStatus status = authenticationMechanism.performAuthentication(request, response, messageContext);
assertEquals(AuthenticationStatus.SUCCESS, status);
@@ -343,6 +346,72 @@ public class OpenIdAuthenticationMechanismUnitTest {
}
}
+ @Test
+ public void tokenEndpointUsesClientSecretBasicWhenDiscoveryAdvertisesIt() throws Exception {
+ final AtomicReference<String> capturedAuthHeader = new AtomicReference<>();
+ final AtomicReference<String> capturedBody = new AtomicReference<>();
+
+ HttpServer server = HttpServer.create(new InetSocketAddress(0), 0);
+ server.createContext("/token", exchange -> {
+ capturedAuthHeader.set(exchange.getRequestHeaders().getFirst("Authorization"));
+ capturedBody.set(readRequestBody(exchange));
+
+ byte[] body = "{\"token_type\":\"Bearer\",\"access_token\":\"ACCESS\",\"id_token\":\"ID\",\"expires_in\":3600}"
+ .getBytes(StandardCharsets.UTF_8);
+
+ exchange.getResponseHeaders().set("Content-Type", "application/json");
+ exchange.sendResponseHeaders(200, body.length);
+ exchange.getResponseBody().write(body);
+ exchange.close();
+ });
+ server.start();
+
+ try {
+ // Discovery advertises both methods; Basic must win because OIDC Core §9 names it as the default.
+ final JsonObject discovery = Json.createObjectBuilder()
+ .add(OpenIdConstant.TOKEN_ENDPOINT,
+ "http://127.0.0.1:" + server.getAddress().getPort() + "/token")
+ .add(OpenIdConstant.TOKEN_ENDPOINT_AUTH_METHODS_SUPPORTED,
+ Json.createArrayBuilder().add("client_secret_post").add("client_secret_basic"))
+ .build();
+ final OpenIdProviderMetadata overrideDefaults =
+ DefaultOverrideHolder.class.getAnnotation(OpenIdProviderMetadata.class);
+ final CompositeOpenIdProviderMetadata composite =
+ new CompositeOpenIdProviderMetadata(discovery, overrideDefaults);
+
+ when(definition.providerMetadata()).thenReturn(composite);
+ when(identityStoreHandler.validate(any()))
+ .thenReturn(new CredentialValidationResult("caller", Set.of("users")));
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ HttpMessageContext messageContext = mock(HttpMessageContext.class, Answers.RETURNS_DEEP_STUBS);
+
+ when(request.getParameter(OpenIdConstant.STATE)).thenReturn("STATE");
+ when(request.getParameter(OpenIdConstant.CODE)).thenReturn("CODE");
+ when(request.getRequestURL()).thenReturn(new StringBuffer("https://example.com/redirect"));
+ when(request.getRequestURI()).thenReturn("/redirect");
+ when(messageContext.notifyContainerAboutLogin(any(CredentialValidationResult.class)))
+ .thenReturn(AuthenticationStatus.SUCCESS);
+
+ storageHandler.set(request, response, OpenIdStorageHandler.STATE_KEY, "STATE");
+
+ AuthenticationStatus status = authenticationMechanism.performAuthentication(request, response, messageContext);
+
+ assertEquals(AuthenticationStatus.SUCCESS, status);
+ assertNotNull(capturedAuthHeader.get());
+ assertTrue(capturedAuthHeader.get().startsWith("Basic "));
+
+ Map<String, String> formParams = parseFormBody(capturedBody.get());
+ assertFalse(formParams.containsKey(OpenIdConstant.CLIENT_SECRET));
+ } finally {
+ server.stop(0);
+ }
+ }
+
+ @OpenIdProviderMetadata
+ private static class DefaultOverrideHolder { }
+
private static String readRequestBody(HttpExchange exchange) throws java.io.IOException {
ByteArrayOutputStream out = new ByteArrayOutputStream();
try (InputStream in = exchange.getRequestBody()) {