From f743c4758195719f986740b837007dcd92855ef6 Mon Sep 17 00:00:00 2001 From: strehle Date: Tue, 8 Aug 2023 15:32:05 +0200 Subject: [PATCH] feature: add client_auth_method=none into tokens for clients with empty secret e.g. cf client. This allows clients to see, whats really used. According to https://oauth.net/2.1/ the empty secret can be treated as public usage --- scripts/cargo/uaa.yml | 2 +- ...ibleTokenEndpointAuthenticationFilter.java | 7 +++++ .../ClientDetailsAuthenticationProvider.java | 25 +++++++++++----- .../oauth/UaaAuthorizationRequestManager.java | 7 ++++- .../PasswordGrantIntegrationTests.java | 29 +++++++++++++++++++ 5 files changed, 61 insertions(+), 9 deletions(-) diff --git a/scripts/cargo/uaa.yml b/scripts/cargo/uaa.yml index 9219d171c8e..c9de0fb9cda 100644 --- a/scripts/cargo/uaa.yml +++ b/scripts/cargo/uaa.yml @@ -51,7 +51,7 @@ jwt: revocable: false refresh: format: opaque - rotate: false + rotate: true unique: false login: diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/BackwardsCompatibleTokenEndpointAuthenticationFilter.java b/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/BackwardsCompatibleTokenEndpointAuthenticationFilter.java index 6b2db872771..9aa2671d26d 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/BackwardsCompatibleTokenEndpointAuthenticationFilter.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/BackwardsCompatibleTokenEndpointAuthenticationFilter.java @@ -13,6 +13,7 @@ package org.cloudfoundry.identity.uaa.authentication; +import org.cloudfoundry.identity.uaa.oauth.token.ClaimConstants; import org.cloudfoundry.identity.uaa.provider.oauth.ExternalOAuthAuthenticationManager; import org.cloudfoundry.identity.uaa.util.SessionUtils; import org.cloudfoundry.identity.uaa.util.UaaStringUtils; @@ -130,6 +131,12 @@ public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) if (clientAuth.isAuthenticated()) { // Ensure the OAuth2Authentication is authenticated authorizationRequest.setApproved(true); + if (clientAuth.getDetails() instanceof UaaAuthenticationDetails) { + String clientAuthentication = ((UaaAuthenticationDetails) clientAuth.getDetails()).getAuthenticationMethod(); + if (clientAuthentication != null) { + authorizationRequest.getExtensions().put(ClaimConstants.CLIENT_AUTH_METHOD, clientAuthentication); + } + } } OAuth2Request storedOAuth2Request = oAuth2RequestFactory.createOAuth2Request(authorizationRequest); diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ClientDetailsAuthenticationProvider.java b/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ClientDetailsAuthenticationProvider.java index b7c551aa564..76d8c905b30 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ClientDetailsAuthenticationProvider.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/ClientDetailsAuthenticationProvider.java @@ -18,6 +18,7 @@ import org.cloudfoundry.identity.uaa.oauth.token.ClaimConstants; import org.cloudfoundry.identity.uaa.oauth.token.TokenConstants; import org.cloudfoundry.identity.uaa.util.UaaStringUtils; +import org.springframework.security.authentication.AbstractAuthenticationToken; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.authentication.dao.DaoAuthenticationProvider; import org.springframework.security.core.AuthenticationException; @@ -25,6 +26,7 @@ import org.springframework.security.core.userdetails.UserDetails; import org.springframework.security.core.userdetails.UserDetailsService; import org.springframework.security.crypto.password.PasswordEncoder; +import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; import java.util.Collections; @@ -57,14 +59,19 @@ protected void additionalAuthenticationChecks(UserDetails userDetails, UsernameP for(String pwd: passwordList) { try { User user = new User(userDetails.getUsername(), pwd, userDetails.isEnabled(), userDetails.isAccountNonExpired(), userDetails.isCredentialsNonExpired(), userDetails.isAccountNonLocked(), userDetails.getAuthorities()); - if (authentication.getCredentials() == null && isPublicGrantTypeUsageAllowed(authentication.getDetails()) && userDetails instanceof UaaClient) { - // in case of grant_type=authorization_code and code_verifier passed (PKCE) we check if client has option allowpublic with true and proceed even if no secret is provided + if (userDetails instanceof UaaClient) { UaaClient uaaClient = (UaaClient) userDetails; - Object allowPublic = uaaClient.getAdditionalInformation().get(ClientConstants.ALLOW_PUBLIC); - if ((allowPublic instanceof String && Boolean.TRUE.toString().equalsIgnoreCase((String)allowPublic)) || - (allowPublic instanceof Boolean && Boolean.TRUE.equals(allowPublic))) { - ((UaaAuthenticationDetails) authentication.getDetails()).setAuthenticationMethod(CLIENT_AUTH_NONE); - break; + if (authentication.getCredentials() == null && isPublicGrantTypeUsageAllowed(authentication.getDetails())) { + // in case of grant_type=authorization_code and code_verifier passed (PKCE) we check if client has option allowpublic with true and proceed even if no secret is provided + Object allowPublic = uaaClient.getAdditionalInformation().get(ClientConstants.ALLOW_PUBLIC); + if ((allowPublic instanceof String && Boolean.TRUE.toString().equalsIgnoreCase((String) allowPublic)) || + (allowPublic instanceof Boolean && Boolean.TRUE.equals(allowPublic))) { + setAuthenticationMethodNone(authentication); + break; + } + } else if (ObjectUtils.isEmpty(authentication.getCredentials())) { + // set none as client_auth_method for all usage of empty secrets, e.g. cf client + setAuthenticationMethodNone(authentication); } } super.additionalAuthenticationChecks(user, authentication); @@ -79,6 +86,10 @@ protected void additionalAuthenticationChecks(UserDetails userDetails, UsernameP } } + private static void setAuthenticationMethodNone(AbstractAuthenticationToken authentication) { + ((UaaAuthenticationDetails) authentication.getDetails()).setAuthenticationMethod(CLIENT_AUTH_NONE); + } + private boolean isPublicGrantTypeUsageAllowed(Object uaaAuthenticationDetails) { UaaAuthenticationDetails authenticationDetails = uaaAuthenticationDetails instanceof UaaAuthenticationDetails ? (UaaAuthenticationDetails) uaaAuthenticationDetails : new UaaAuthenticationDetails(); diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationRequestManager.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationRequestManager.java index 604f530b47d..0859400ca64 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationRequestManager.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationRequestManager.java @@ -13,13 +13,14 @@ package org.cloudfoundry.identity.uaa.oauth; import org.cloudfoundry.identity.uaa.oauth.client.ClientConstants; +import org.cloudfoundry.identity.uaa.oauth.token.ClaimConstants; import org.cloudfoundry.identity.uaa.oauth.token.TokenConstants; import org.cloudfoundry.identity.uaa.provider.IdentityProvider; import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning; -import org.cloudfoundry.identity.uaa.provider.JdbcIdentityProviderProvisioning; import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor; import org.cloudfoundry.identity.uaa.user.UaaUser; import org.cloudfoundry.identity.uaa.user.UaaUserDatabase; +import org.cloudfoundry.identity.uaa.util.UaaSecurityContextUtils; import org.cloudfoundry.identity.uaa.util.UaaStringUtils; import org.cloudfoundry.identity.uaa.util.UaaTokenUtils; import org.cloudfoundry.identity.uaa.zone.MultitenantClientServices; @@ -397,6 +398,10 @@ public UaaTokenRequest(Map requestParameters, String clientId, C @Override public OAuth2Request createOAuth2Request(ClientDetails client) { OAuth2Request request = super.createOAuth2Request(client); + String clientAuthentication = UaaSecurityContextUtils.getClientAuthenticationMethod(); + if (clientAuthentication != null) { + request.getExtensions().put(ClaimConstants.CLIENT_AUTH_METHOD, clientAuthentication); + } return new OAuth2Request( request.getRequestParameters(), client.getClientId(), diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/PasswordGrantIntegrationTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/PasswordGrantIntegrationTests.java index 5f25766a8d8..2afa20fbf7a 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/PasswordGrantIntegrationTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/PasswordGrantIntegrationTests.java @@ -4,8 +4,10 @@ import org.cloudfoundry.identity.uaa.ServerRunning; import org.cloudfoundry.identity.uaa.integration.util.IntegrationTestUtils; import org.cloudfoundry.identity.uaa.oauth.client.ClientConstants; +import org.cloudfoundry.identity.uaa.oauth.token.ClaimConstants; import org.cloudfoundry.identity.uaa.test.UaaTestAccounts; import org.cloudfoundry.identity.uaa.util.JsonUtils; +import org.cloudfoundry.identity.uaa.util.UaaTokenUtils; import org.junit.Rule; import org.junit.Test; import org.springframework.http.HttpEntity; @@ -25,7 +27,14 @@ import java.util.HashMap; import java.util.Map; +import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.CLIENT_AUTH_NONE; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.Matchers.hasEntry; +import static org.hamcrest.Matchers.hasKey; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; import static org.springframework.http.MediaType.APPLICATION_JSON; import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; @@ -42,6 +51,14 @@ public class PasswordGrantIntegrationTests { public void testUserLoginViaPasswordGrant() { ResponseEntity responseEntity = makePasswordGrantRequest(testAccounts.getUserName(), testAccounts.getPassword(), "cf", "", serverRunning.getAccessTokenUri()); assertEquals(HttpStatus.OK, responseEntity.getStatusCode()); + validateClientAuthenticationMethod(responseEntity, true); + } + + @Test + public void testUserLoginViaPasswordGrantButOwnClient() { + ResponseEntity responseEntity = makePasswordGrantRequest(testAccounts.getUserName(), testAccounts.getPassword(), "app", "appclientsecret", serverRunning.getAccessTokenUri()); + assertEquals(HttpStatus.OK, responseEntity.getStatusCode()); + validateClientAuthenticationMethod(responseEntity, false); } @Test @@ -135,4 +152,16 @@ public void handleError(ClientHttpResponse response) { }); return template; } + + private void validateClientAuthenticationMethod(ResponseEntity responseEntity, boolean isNone) { + Map jsonBody = JsonUtils.readValue(responseEntity.getBody(), new TypeReference>() {}); + String accessToken = (String) jsonBody.get("access_token"); + assertThat(accessToken, is(notNullValue())); + Map claims = UaaTokenUtils.getClaims(accessToken); + if (isNone) { + assertThat(claims, hasEntry(ClaimConstants.CLIENT_AUTH_METHOD, CLIENT_AUTH_NONE)); + } else { + assertThat(claims, not(hasKey(ClaimConstants.CLIENT_AUTH_METHOD))); + } + } }