From 58772b5c7fdfb77c11def6104f86317a55ca954e Mon Sep 17 00:00:00 2001 From: strehle Date: Wed, 15 Jan 2025 17:18:42 +0100 Subject: [PATCH] Refactoring to fix issue #3232 Move the check for existing secrets to the grant flow (runtime) and remove it from REST (configuration). We support now private_key_jwt next to secret, so check all parts in REST is too less and will get complicated. Fix a potential security issue because we must not create a token based on no authentication --- .../uaa/oauth/token/TokenConstants.java | 1 + .../client/ClientAdminEndpointsValidator.java | 9 ------ .../client/ClientCredentialsTokenGranter.java | 9 ++++-- .../provider/token/AbstractTokenGranter.java | 30 +++++++++++++++++++ .../uaa/oauth/token/JwtTokenGranter.java | 9 +++++- 5 files changed, 46 insertions(+), 12 deletions(-) diff --git a/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/TokenConstants.java b/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/TokenConstants.java index e5a2ab3f819..3bbd14a20de 100644 --- a/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/TokenConstants.java +++ b/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/TokenConstants.java @@ -65,6 +65,7 @@ public static List getStringValues() { public static final String CLIENT_AUTH_NONE = ClientAuthentication.NONE; public static final String CLIENT_AUTH_EMPTY = "empty"; + public static final String CLIENT_AUTH_SECRET = "secret"; public static final String CLIENT_AUTH_PRIVATE_KEY_JWT = ClientAuthentication.PRIVATE_KEY_JWT; public static final String ID_TOKEN_HINT_PROMPT = "prompt"; diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/client/ClientAdminEndpointsValidator.java b/server/src/main/java/org/cloudfoundry/identity/uaa/client/ClientAdminEndpointsValidator.java index ec6598017d5..5cac828dfba 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/client/ClientAdminEndpointsValidator.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/client/ClientAdminEndpointsValidator.java @@ -144,10 +144,6 @@ public ClientDetails validate(ClientDetails prototype, boolean create, boolean c logger.debug("Invalid client: " + clientId + ". Scope cannot be empty for grant_type " + GRANT_TYPE_JWT_BEARER); throw new InvalidClientDetailsException("Scope cannot be empty for grant_type " + GRANT_TYPE_JWT_BEARER); } - if (create && !StringUtils.hasText(client.getClientSecret())) { - logger.debug("Invalid client: " + clientId + ". Client secret is required for grant type " + GRANT_TYPE_JWT_BEARER); - throw new InvalidClientDetailsException("Client secret is required for grant type " + GRANT_TYPE_JWT_BEARER); - } } if (checkAdmin && @@ -235,11 +231,6 @@ public ClientDetails validate(ClientDetails prototype, boolean create, boolean c } } if (create) { - // Only check for missing secret if client is being created. - if (requestedGrantTypes.contains(GRANT_TYPE_CLIENT_CREDENTIALS) && !StringUtils.hasText(client.getClientSecret())) { - logger.debug("Client secret is required for client_credentials grant type"); - throw new InvalidClientDetailsException("Client secret is required for client_credentials grant type"); - } clientSecretValidator.validate(client.getClientSecret()); if (prototype instanceof ClientDetailsCreation clientDetailsCreation) { diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/client/ClientCredentialsTokenGranter.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/client/ClientCredentialsTokenGranter.java index ef302747b37..a8ee326e167 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/client/ClientCredentialsTokenGranter.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/client/ClientCredentialsTokenGranter.java @@ -8,6 +8,10 @@ import org.cloudfoundry.identity.uaa.oauth.provider.token.AuthorizationServerTokenServices; import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetailsService; +import java.util.List; + +import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.CLIENT_AUTH_PRIVATE_KEY_JWT; +import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.CLIENT_AUTH_SECRET; import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.GRANT_TYPE_CLIENT_CREDENTIALS; /** @@ -20,6 +24,8 @@ */ public class ClientCredentialsTokenGranter extends AbstractTokenGranter { + private static final List ALLOWED_AUTH_METHODS = List.of(CLIENT_AUTH_SECRET, CLIENT_AUTH_PRIVATE_KEY_JWT); + public ClientCredentialsTokenGranter(AuthorizationServerTokenServices tokenServices, ClientDetailsService clientDetailsService, OAuth2RequestFactory requestFactory) { this(tokenServices, clientDetailsService, requestFactory, GRANT_TYPE_CLIENT_CREDENTIALS); @@ -33,7 +39,7 @@ protected ClientCredentialsTokenGranter(AuthorizationServerTokenServices tokenSe @Override public OAuth2AccessToken grant(String grantType, TokenRequest tokenRequest) { OAuth2AccessToken token = super.grant(grantType, tokenRequest); - if (token != null) { + if (token != null && isValidClientAuthentication(ALLOWED_AUTH_METHODS)) { DefaultOAuth2AccessToken norefresh = new DefaultOAuth2AccessToken(token); // The spec says that client credentials should not be allowed to get a refresh token norefresh.setRefreshToken(null); @@ -41,5 +47,4 @@ public OAuth2AccessToken grant(String grantType, TokenRequest tokenRequest) { } return token; } - } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/token/AbstractTokenGranter.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/token/AbstractTokenGranter.java index 89045aa2693..b1d1664b7b4 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/token/AbstractTokenGranter.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/provider/token/AbstractTokenGranter.java @@ -2,7 +2,9 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.cloudfoundry.identity.uaa.authentication.UaaAuthenticationDetails; import org.cloudfoundry.identity.uaa.oauth.common.OAuth2AccessToken; +import org.cloudfoundry.identity.uaa.oauth.common.exceptions.InvalidRequestException; import org.cloudfoundry.identity.uaa.oauth.provider.OAuth2Authentication; import org.cloudfoundry.identity.uaa.oauth.provider.OAuth2Request; import org.cloudfoundry.identity.uaa.oauth.provider.OAuth2RequestFactory; @@ -11,8 +13,16 @@ import org.cloudfoundry.identity.uaa.oauth.common.exceptions.InvalidClientException; import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetails; import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetailsService; +import org.cloudfoundry.identity.uaa.oauth.token.TokenConstants; +import org.cloudfoundry.identity.uaa.util.UaaSecurityContextUtils; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContext; +import org.springframework.security.core.context.SecurityContextHolder; import java.util.Collection; +import java.util.List; +import java.util.Optional; /** * Moved class AbstractTokenGranter implementation of from spring-security-oauth2 into UAA @@ -76,6 +86,26 @@ protected void validateGrantType(String grantType, ClientDetails clientDetails) } } + protected boolean isValidClientAuthentication(List allowedMethods) { + SecurityContext context = SecurityContextHolder.getContext(); + if (allowedMethods.contains(Optional.ofNullable(getUaaClientAuthenticationMethod(context.getAuthentication())) + .orElse(TokenConstants.CLIENT_AUTH_SECRET))) { + // internal null for client authentication means secret based (authorization header or post body) + return true; + } else { + throw new InvalidRequestException("Client credentials required"); + } + } + + protected String getUaaClientAuthenticationMethod(Authentication authentication) { + if (authentication instanceof UsernamePasswordAuthenticationToken && authentication.isAuthenticated() && + authentication.getDetails() instanceof UaaAuthenticationDetails) { + return ((UaaAuthenticationDetails) authentication.getDetails()).getAuthenticationMethod(); + } else { + return UaaSecurityContextUtils.getClientAuthenticationMethod(authentication); + } + } + protected AuthorizationServerTokenServices getTokenServices() { return tokenServices; } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/JwtTokenGranter.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/JwtTokenGranter.java index 4b8143863bf..0a9a49047fb 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/JwtTokenGranter.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/JwtTokenGranter.java @@ -13,11 +13,18 @@ import org.cloudfoundry.identity.uaa.oauth.common.exceptions.InvalidGrantException; import org.cloudfoundry.identity.uaa.oauth.provider.ClientDetails; +import java.util.List; + +import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.CLIENT_AUTH_PRIVATE_KEY_JWT; +import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.CLIENT_AUTH_SECRET; import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.GRANT_TYPE_JWT_BEARER; public class JwtTokenGranter extends AbstractTokenGranter { final DefaultSecurityContextAccessor defaultSecurityContextAccessor; + private static final List ALLOWED_AUTH_METHODS = List.of(CLIENT_AUTH_SECRET, CLIENT_AUTH_PRIVATE_KEY_JWT); + //comment I would add here CLIENT_AUTH_EMPTY to allow empty secret for jwt bearer, same as allowed for password + public JwtTokenGranter(AuthorizationServerTokenServices tokenServices, MultitenantClientServices clientDetailsService, OAuth2RequestFactory requestFactory) { @@ -41,7 +48,7 @@ protected Authentication validateRequest(TokenRequest request) { } else { throw new InvalidGrantException("User authentication not found"); } - return SecurityContextHolder.getContext().getAuthentication(); + return isValidClientAuthentication(ALLOWED_AUTH_METHODS) ? SecurityContextHolder.getContext().getAuthentication() : null; } @Override