Draft
Conversation
OpenIdHttpClientSupport shipped a default-on X509TrustManager that accepted any cert for loopback HTTPS endpoints (CWE-295/297). Drop the helper entirely and let JAX-RS and jose4j use the JVM default SSLSocketFactory; tests that need localhost trust must configure javax.net.ssl.trustStore properly. The companion localhost metadata-rewrite fallback in AutoResolvingProviderMetadata depended on the same helper and is removed too; the simple .well-known/openid-configuration fetch is enough when the provider URI resolves normally.
ProcessBean can fire more than once for the same annotated type (e.g. when CDI synthesises observer/interceptor beans on it), so List.addAll accumulated duplicate @*AuthenticationMechanismDefinition instances. After c723b38 started registering one bean per collected definition, this produced multiple beans with identical qualifier sets and AmbiguousResolutionException on startup -- the tomee-security servlet tests failed to deploy. Switch the four *MechanismDefinitions collections to LinkedHashSet (annotations are value-equal) and snapshot them to ordered lists at AfterBeanDiscovery time so the indexed bean-id scheme still works. Also update OpenIdAuthenticationMechanismUnitTest to set the definition through the supplier hook rather than reflecting into a field that no longer exists.
The OpenID modules ship self-signed certs that expired in March
2023, which is why the TLS bypass in OpenIdHttpClientSupport was
required. Now that the bypass is gone we need JSSE to validate
these connections on its own.
In the download-tck antrun phase:
- seed ${glassfish.root}/glassfish8/.../cacerts.jks from the JDK's
default cacerts (146 trusted CAs) so Maven metadata fetches and
any other public HTTPS calls during the invoker run still
validate; the TCK's keytool-maven-plugin step appends the
localhost cert on top in pre-integration-test.
- regenerate localhost-rsa.jks + tomcat.cert per app-openid* dir
with a fresh 10-year validity, replacing the expired TCK files.
- replace the two hard-coded <alias> rewrites with a loop that
handles any app-openidN directory.
In the injected tomee-remote profile:
- point javax.net.ssl.trustStore at the seeded keystore so the
failsafe JVM (HtmlUnit test driver) trusts the localhost cert.
- add tomee.catalina_opts so arquillian-tomee-remote forwards the
same -Djavax.net.ssl.trustStore/Password to the forked TomEE
JVM (the OpenID relying party).
Full Jakarta Security 4.0 TCK is green against this configuration
without any trust-all fallback.
Jakarta Security 4.0 §2.4.4.2 says a callback whose URL matches neither the configured redirectURI nor the stored original-request URL MUST return CredentialValidationResult.NOT_VALIDATED_RESULT. Previously the check was conditional on redirectToOriginalResource, so with the default (false) any request carrying a state parameter drove a token-endpoint exchange. The comparison was also based on HttpServletRequest.getRequestURL(), which folds in the client-supplied Host header (CWE-350) -- behind a misconfigured reverse proxy this is attacker-influenced. - Drop the && redirectToOriginalResource() conjunct so the guard runs unconditionally. - Compare by path: request.getRequestURI() against the path component of the configured redirectURI (memoised via a new redirectPath() helper) and against the path of the stored ORIGINAL_REQUEST. Storage values are unchanged -- SavedRequest restoration still uses the full URL. Added three focused unit tests covering spoofed Host headers with matching / non-matching paths under both redirectToOriginalResource settings.
Jakarta Security 4.0 §4.2 adds SecurityContext#getAllDeclaredCallerRoles
returning every application-declared role the current caller has.
It was unimplemented and threw UnsupportedOperationException.
Resolve the current Catalina Request via OpenEJBSecurityListener
(same pattern as registerContainerAboutLogin) and iterate
Context#findSecurityRoles() -- the servlet-declared roles. For each
role, evaluate Jakarta Authorization 3.0's Policy with a
WebRoleRefPermission("", role) against the current Subject from
TomcatSecurityService#getCurrentSubject. Wrap the evaluation in the
same save/restore PolicyContext.getContextID() try/finally used by
hasAccessToWebResource. Returns a LinkedHashSet so iteration is
stable; empty set if the request, context, security service, or
policy is unavailable.
Added an integration test that deploys a @DeclareRoles-annotated
servlet under BASIC auth, logs in as the tomcat user, and verifies
the method returns only the roles the caller actually holds.
Scope: web-module declared roles only; EJB security-role-ref
integration is out of scope and deferred.
…allerPrincipal Two cleanups in AbstractSecurityService: 1. Drop the java.lang.reflect.Proxy branch that re-exposed DefaultPrincipalMapper across a foreign TCCL. jakarta.security.jacc is a container-delegated package via URLClassLoaderFirst, so the proxy path is dead code. getContext(PRINCIPAL_MAPPER, ...) now returns the field directly. 2. Tighten DefaultPrincipalMapper#getCallerPrincipal lookup order so a jakarta.security.enterprise.CallerPrincipal instance wins over any other non-Group principal. Previously only the TomEE-internal @org.apache.openejb.spi.CallerPrincipal annotation had precedence; a Subject with both a Jakarta CallerPrincipal and an unrelated Principal resolved non-deterministically via the first-non-Group fallback. The inner class is now a static nested class so tests can instantiate it directly, with a package-private getPrincipalMapper() accessor. New DefaultPrincipalMapperTest covers both CallerPrincipal forms, group skipping, empty/null subjects, and the Group->role mapping.
OIDC callbacks are cross-site, so state/nonce cookies need both the Secure flag and SameSite=None. The previous implementation set Secure conditionally on request.isSecure() (misbehaves behind TLS- terminating proxies), never set SameSite, and the delete() path omitted the attributes needed for the browser to overwrite the cookie. - set(): Secure is now unconditional behind tomee.security.openid.state-cookie-secure (default true); adds Path=/ and SameSite=None; Base64 encoding now uses StandardCharsets.UTF_8 explicitly on both sides. - delete(): mirrors Path, Secure, HttpOnly, SameSite so browsers accept the replacement cookie with maxAge=0. - get(): null-guards request.getCookies() so the handler tolerates requests without any cookie jar. New CookieBasedOpenIdStorageHandlerTest captures cookies via Mockito ArgumentCaptor and asserts all four attributes on set and delete, exercises the override property, covers the null-cookies path, and round-trips a unicode string through Base64 to confirm the UTF-8 encoding.
Jakarta Security 4.0 §3.4 introduces @InMemoryIdentityStoreDefinition for small apps that don't want to stand up a database or LDAP. The extension already collected TomcatUser/Database/LDAP stores but silently ignored the in-memory one, so any app declaring it got no IdentityStore registered. TomEEInMemoryIdentityStore is @ApplicationScoped, honours useFor() (NOT_VALIDATED when VALIDATE is absent; getCallerGroups returns empty when PROVIDE_GROUPS is absent), and uses MessageDigest.isEqual on UTF-8 bytes so password comparison is constant-time. The full declared-credentials array is walked once per validate() so runtime is roughly independent of caller position / presence. TomEESecurityExtension gains an inMemoryStore AtomicReference, matching detection in processBean, and Supplier + TomEEInMemoryIdentityStore bean registrations in registerAuthenticationMechanism mirroring the LDAP/Database pattern. The TomEEELInvocationHandler proxy transparently resolves the *Expression siblings. Unit test coverage (16 cases): - happy path returns VALID with declared groups - wrong password / unknown caller / non-UPC credential - useFor permutations (VALIDATE-only, PROVIDE_GROUPS-only, both) - empty store, empty password, case-sensitive caller name - caller + right-password-from-another-entry stays INVALID - priority() reflects annotation (spec default 90, custom value) - validationTypes() reflects useFor - duplicate caller entries: first match wins deterministically - Unicode caller/password/groups round-trip (UTF-8) - declared-group uniqueness across callers - 8-thread / 200-iteration concurrent validate stays consistent
Jakarta Authorization 3.0 expects the installed Policy to be the single source of truth for web-resource authorization. TomEERealm was running Policy.implies() as an additive "grant only" shortcut in front of the Catalina default check -- a JACC deny was discarded, and a Catalina grant could override a missing JACC decision. - JaccProvider#DefaultPolicy gains an isSentinel() marker and JaccProvider.isSentinelPolicy(Policy) exposes it package-publicly so TomEERealm can distinguish "no application policy installed for this context" from a real allow/deny verdict. - evaluatePolicyFactory(Request) (renamed from isGrantedByPolicyFactory) now returns Boolean tri-state: TRUE = application Policy allowed, FALSE = application Policy denied, null = sentinel / missing / runtime error. hasResourcePermission returns the Boolean directly when non-null; the super.hasResourcePermission fallback runs only for the null case. - PolicyContext.setHandlerData is saved and restored around the implies() call instead of being nulled in finally, so a nested authorization check on the same thread doesn't lose its handler data. - A previously silent catch (RuntimeException) now logs at DEBUG under OPENEJB_SECURITY and returns null (unknown) so the fallback decides, making misconfiguration diagnosable.
C1.5 made ProcessBean idempotent so IDENTICAL mechanism definitions
collapse. Two DIFFERENT @*AuthenticationMechanismDefinition
instances that share the same qualifier set (e.g. two
@BasicAuthenticationMechanismDefinition with the same default
BasicAuthenticationMechanism qualifier but different realmName) are
not equal, stay as separate entries, and both register beans with
the same qualifier set -- OWB then throws AmbiguousResolutionException
on first access at runtime.
Observe AfterDeploymentValidation and group each mechanism type's
collected definitions by their effective qualifier set (wrapped in
a LinkedHashSet so intra-array order and duplicates don't mask
clashes). Any group with size > 1 becomes one
afterDeploymentValidation.addDeploymentProblem(DeploymentException)
carrying the annotation simple name, the conflicting qualifier
classes, the offending instances' toString, and a hint pointing at
qualifiers = { ... }. All four mechanism types are checked in one
observer so the error surfaces every violation at once.
Test coverage (5 cases in TomEESecurityExtensionQualifierValidationTest)
exercises the extracted package-private helper
validateQualifierUniqueness: single def passes, two defs with same
default qualifier and different realmName fails with both realms
named, distinct qualifier sets pass, reordered qualifier arrays
still collide, empty input is a no-op.
This reverts commit 562eba6. The change rode along on the Security 4.0 branch but isn't needed for any of its TCK modules (none exercise Jakarta Faces) and it removes the explicit `delegate = false` synchronous path that the original loader used for `jakarta.faces.*`, which existed to avoid a MyFaces + OWB class-init ordering issue. If the delegation rewrite is still desired it should land under its own TOMEE ticket with a MyFaces-CDI smoke test.
OIDC Core 1.0 §9 lists client_secret_basic as the default client authentication method at the token endpoint, and both OP and RP are expected to support it. client_secret_post works too but puts the secret in the request body, which ends up in HTTP access logs and WAF / proxy trace dumps. - Factor out preferBasicAuth() and basicAuthHeader() helpers shared by refreshTokens() and performAuthentication(). - When Basic is preferred, send Authorization: Basic base64(clientId:clientSecret) and drop client_id / client_secret from the form body. - preferBasicAuth() reflectively reads the spec-pending tokenEndpointAuthMethodsSupported() accessor on OpenIdProviderMetadata (absent in the bundled jakarta.security.enterprise-api:4.0.0 jar). Missing accessor, empty/null return, or the presence of client_secret_basic all select Basic; a non-empty array advertising only client_secret_post selects form-parameter posting. Two new unit tests spin up a com.sun.net.httpserver.HttpServer, capture the /token request, and assert: - default path emits the Authorization: Basic header and omits client_id / client_secret from the form body, and - client_secret_post-only configuration omits the header and keeps client_id / client_secret in the form body.
The initial implementation only enumerated servlet <security-role>
entries via Catalina's Context.findSecurityRoles(). Spec §4.2 says
the method must return every application-declared role the caller
holds, which in an EE app also covers the current EJB's
<security-role-ref> / @DeclareRoles.
BeanContext gains getSecurityRoleReferences() returning a
defensive LinkedHashSet view of the internal map's keys (it's the
only accessor we own; the map was previously exposed only through
a per-role lookup).
TomEESecurityContext.getAllDeclaredCallerRoles now runs two
independent passes, each under its own PolicyContext.getContextID()
save/restore:
- web pass under toAppContext(servletContext, contextPath) using
WebRoleRefPermission("", role) against the current Request's
Context.findSecurityRoles().
- EJB pass under BeanContext.getModuleID() using
EJBRoleRefPermission(ejbName, role) against
BeanContext.getSecurityRoleReferences(). Runs whenever
ThreadContext.getThreadContext() is non-null.
Results merge into a LinkedHashSet (web first, then EJB). Empty
set only when both the Request and ThreadContext are null.
Test extension: a @stateless @DeclareRoles({"tomcat",
"ejb-only-role"}) bean is invoked from a servlet that reports the
merged set. The servlet declares "user"/"unassigned" so the test
proves "tomcat" came from the EJB branch and "ejb-only-role" is
not included because the caller doesn't have it.
Jakarta Security 4.0 §1.2.4 says the container must expose a java.security.Principal bean with the @default qualifier that resolves to the current caller. TomEE-security did not register one -- @Inject Principal failed with UnsatisfiedResolutionException unless the app declared its own producer. Add a small @ApplicationScoped CallerPrincipalProducer that delegates to SecurityContext.getCallerPrincipal() and is registered by TomEESecurityExtension.observeBeforeBeanDiscovery alongside the existing BaseUrlProducer. The producer method is @dependent so every injection site sees the live caller identity rather than a cached reference from whenever the owning bean was constructed. Three unit tests cover the happy path, the unauthenticated null case, and the per-call reevaluation contract.
…n 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.
TomEE plus/plume ship MyFaces in the server lib/, so the impl classes are linked against the container-loaded jakarta.faces.* API. When a WAR also bundles jakarta.faces-api the previous shouldSkipJsf size heuristic flipped delegation off, two distinct Class objects for the same FQN ended up on the path, and MyFaces' StartupServletContextListener died with a LinkageError - reproduced by the Jakarta Security 4.0 TCK app-mem-customform module which bundles jakarta.faces-api 4.1.0. Drop the heuristic from URLClassLoaderFirst.shouldSkipJsf so jakarta.faces.* is always loaded from the container exactly once. The matching prefix in TomEEWebappClassLoader.loadClass becomes redundant (the first if-branch now catches it via shouldDelegateToTheContainer); leave the org.apache.webbeans.jsf local-load path intact so the OWB JSF integration still binds to the webapp's CDI beans. Add URLClassLoaderFirstTest cases that exercise both the multi-copy and single-copy classpath shapes to lock the behaviour down.
preferBasicAuth() previously returned true as soon as client_secret_basic appeared anywhere in token_endpoint_auth_methods_supported, ignoring the OP's advertised preference order. Form-only OPs that legally list both methods with client_secret_post first - including the Jakarta Security 4.0 TCK mock provider used by app-openid - therefore got an empty form body and returned 500 invalid_client_id, propagating as HTTP 500 on /Callback. Walk the array in order and pick the first method we know how to speak; fall back to Basic only when no list is published or none of the listed methods are implemented (RFC 6749 / OIDC Core §9 default). Update the existing unit test to assert order-driven selection (post-first => Post wins, basic-first => Basic wins) so the contract is locked in. With this change Security 4.0 TCK app-openid (OpenIdDefaultIT, OpenIdWithELIT, InvalidRedirectURIIT) passes and the full security TCK reports 26/26 modules green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.