From f696b9ab952a9658b1c40d375efb7e21fd35c281 Mon Sep 17 00:00:00 2001 From: Zoltan Maradics Date: Thu, 18 Apr 2019 17:05:01 +0200 Subject: [PATCH 001/140] Add PKCE support --- .../uaa/account/OpenIdConfiguration.java | 10 ++ .../uaa/account/OpenIdConfigurationTests.java | 1 + .../uaa/oauth/UaaAuthorizationEndpoint.java | 40 +++++ .../oauth/pkce/PkceValidationException.java | 17 ++ .../uaa/oauth/pkce/PkceValidationService.java | 167 ++++++++++++++++++ .../identity/uaa/oauth/pkce/PkceVerifier.java | 32 ++++ .../pkce/verifiers/PlainPkceVerifier.java | 27 +++ .../pkce/verifiers/S256PkceVerifier.java | 49 +++++ ...EnhancedAuthorizationCodeTokenGranter.java | 121 +++++++++++++ .../oauth/UaaAuthorizationEndpointTest.java | 43 +++++ .../oauth/pkce/PkceValidationServiceTest.java | 129 ++++++++++++++ .../pkce/verifiers/PlainPkceVerifierTest.java | 47 +++++ .../pkce/verifiers/S256PkceVerifierTest.java | 47 +++++ .../identity/uaa/test/UaaTestAccounts.java | 6 + .../webapp/WEB-INF/spring/oauth-endpoints.xml | 32 +++- ...uthorizationCodeGrantIntegrationTests.java | 128 ++++++++++++++ .../util/IntegrationTestUtils.java | 128 ++++++++++++++ .../uaa/login/AuthorizeEndpointDocs.java | 11 ++ .../identity/uaa/login/TokenEndpointDocs.java | 8 + .../endpoints/OpenIdConnectEndpointDocs.java | 1 + 20 files changed, 1043 insertions(+), 1 deletion(-) create mode 100644 server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationException.java create mode 100644 server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationService.java create mode 100644 server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceVerifier.java create mode 100644 server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/PlainPkceVerifier.java create mode 100644 server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/S256PkceVerifier.java create mode 100644 server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/PkceEnhancedAuthorizationCodeTokenGranter.java create mode 100644 server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationServiceTest.java create mode 100644 server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/PlainPkceVerifierTest.java create mode 100644 server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/S256PkceVerifierTest.java diff --git a/model/src/main/java/org/cloudfoundry/identity/uaa/account/OpenIdConfiguration.java b/model/src/main/java/org/cloudfoundry/identity/uaa/account/OpenIdConfiguration.java index 84ec3ce9a01..8c760e07faf 100644 --- a/model/src/main/java/org/cloudfoundry/identity/uaa/account/OpenIdConfiguration.java +++ b/model/src/main/java/org/cloudfoundry/identity/uaa/account/OpenIdConfiguration.java @@ -53,6 +53,8 @@ public class OpenIdConfiguration { private String serviceDocumentation = "http://docs.cloudfoundry.org/api/uaa/"; @JsonProperty("ui_locales_supported") private String[] uiLocalesSupported = new String[]{"en-US"}; + @JsonProperty("code_challenge_methods_supported") + private String[] codeChallengeMethodsSupported = new String[]{"S256", "plain"}; //For json serialization public OpenIdConfiguration() {} @@ -200,4 +202,12 @@ public String[] getUiLocalesSupported() { public void setUiLocalesSupported(String[] uiLocalesSupported) { this.uiLocalesSupported = uiLocalesSupported; } + + public String[] getCodeChallengeMethodsSupported() { + return codeChallengeMethodsSupported; + } + + public void setCodeChallengeMethodsSupported(String[] codeChallengeMethodsSupported) { + this.codeChallengeMethodsSupported = codeChallengeMethodsSupported; + } } diff --git a/model/src/test/java/org/cloudfoundry/identity/uaa/account/OpenIdConfigurationTests.java b/model/src/test/java/org/cloudfoundry/identity/uaa/account/OpenIdConfigurationTests.java index 5eccec6b825..541fca4fd31 100644 --- a/model/src/test/java/org/cloudfoundry/identity/uaa/account/OpenIdConfigurationTests.java +++ b/model/src/test/java/org/cloudfoundry/identity/uaa/account/OpenIdConfigurationTests.java @@ -57,6 +57,7 @@ public void testDefaultClaims() { assertFalse(defaultConfig.isClaimsParameterSupported()); assertEquals("http://docs.cloudfoundry.org/api/uaa/", defaultConfig.getServiceDocumentation()); assertArrayEquals(new String[]{"en-US"}, defaultConfig.getUiLocalesSupported()); + assertArrayEquals(new String[]{"S256", "plain"}, defaultConfig.getCodeChallengeMethodsSupported()); } } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationEndpoint.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationEndpoint.java index d151c3819a4..9485f0827e1 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationEndpoint.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationEndpoint.java @@ -17,6 +17,7 @@ import org.apache.http.client.utils.URIUtils; import org.cloudfoundry.identity.uaa.authentication.UaaPrincipal; import org.cloudfoundry.identity.uaa.oauth.client.ClientConstants; +import org.cloudfoundry.identity.uaa.oauth.pkce.PkceValidationService; import org.cloudfoundry.identity.uaa.oauth.token.CompositeToken; import org.cloudfoundry.identity.uaa.util.UaaHttpRequestUtils; import org.cloudfoundry.identity.uaa.zone.MultitenantClientServices; @@ -122,6 +123,8 @@ public class UaaAuthorizationEndpoint extends AbstractEndpoint implements Authen private OpenIdSessionStateCalculator openIdSessionStateCalculator; private static final List supported_response_types = Arrays.asList("code", "token", "id_token"); + + private PkceValidationService pkceValidationService; @RequestMapping(value = "/oauth/authorize") public ModelAndView authorize(Map model, @@ -159,6 +162,8 @@ public ModelAndView authorize(Map model, if (authorizationRequest.getClientId() == null) { throw new InvalidClientException("A client id must be provided"); } + + validateAuthorizationRequestPkceParameters(authorizationRequest.getRequestParameters()); String resolvedRedirect = ""; try { @@ -243,6 +248,32 @@ public ModelAndView authorize(Map model, } } + + /** + * PKCE parameters check: + * code_challenge: (Optional) Must be provided for PKCE and must not be empty. + * code_challenge_method: (Optional) Default value is "plain". See .well-known + * endpoint for supported code challenge methods list. + * @param authorizationRequestParameters Authorization request parameters. + */ + protected void validateAuthorizationRequestPkceParameters(Map authorizationRequestParameters) { + if (pkceValidationService != null) { + String codeChallenge = authorizationRequestParameters.get(PkceValidationService.CODE_CHALLENGE); + if (codeChallenge != null) { + if(!PkceValidationService.isCodeChallengeParameterValid(codeChallenge)) { + throw new InvalidRequestException("Code challenge length must between 43 and 128 and use only [A-Z],[a-z],[0-9],_,.,-,~ characters."); + } + String codeChallengeMethod = authorizationRequestParameters.get(PkceValidationService.CODE_CHALLENGE_METHOD); + if (codeChallengeMethod == null) { + codeChallengeMethod = "plain"; + } + if (!pkceValidationService.isCodeChallengeMethodSupported(codeChallengeMethod)) { + throw new InvalidRequestException("Unsupported code challenge method. Supported: " + + pkceValidationService.getSupportedCodeChallengeMethods().toString()); + } + } + } + } // This method handles /oauth/authorize calls when user is not logged in and the prompt=none param is used @Override @@ -837,4 +868,13 @@ public OpenIdSessionStateCalculator getOpenIdSessionStateCalculator() { public void setOpenIdSessionStateCalculator(OpenIdSessionStateCalculator openIdSessionStateCalculator) { this.openIdSessionStateCalculator = openIdSessionStateCalculator; } + + public PkceValidationService getPkceValidationService() { + return pkceValidationService; + } + + public void setPkceValidationService(PkceValidationService pkceValidationService) { + this.pkceValidationService = pkceValidationService; + } + } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationException.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationException.java new file mode 100644 index 00000000000..1064f638ff5 --- /dev/null +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationException.java @@ -0,0 +1,17 @@ +package org.cloudfoundry.identity.uaa.oauth.pkce; + +/** + * Universal PKCE Validation Service exception + * + * @author Zoltan Maradics + * + */ +public class PkceValidationException extends Exception{ + + private static final long serialVersionUID = 7887667018613362856L; + + public PkceValidationException(String msg) { + super(msg); + } + +} diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationService.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationService.java new file mode 100644 index 00000000000..54d072f6e85 --- /dev/null +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationService.java @@ -0,0 +1,167 @@ +package org.cloudfoundry.identity.uaa.oauth.pkce; + +import java.util.Collections; +import java.util.Map; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * PKCE Validation Service. + * - Validate Code Verifier parameter. + * - Validate Code Challenge parameter. + * - Validate Code Challenge Method parameter. + * - List supported code challenge methods. + * - Verify code verifier and code challenge based on code challenge method. + * + * @author Zoltan Maradics + */ + +public class PkceValidationService { + + /* + * Regular expression match with any string: + * - Length between 43 and 128 + * - Contains only [A-Z],[a-z],[0-9],_,.,-,~ characters + * (Note: '_' is part of the 'w' in the pattern.) + */ + private static final Pattern pattern = Pattern.compile("^[\\w\\.\\-\\~]{43,128}$"); + + public static final String CODE_CHALLENGE = "code_challenge"; + public static final String CODE_CHALLENGE_METHOD = "code_challenge_method"; + public static final String CODE_VERIFIER = "code_verifier"; + + private Map pkceVerifiers; + + public PkceValidationService() { + this(Collections.emptyMap()); + } + + public PkceValidationService(Map pkceVerifiers) { + this.pkceVerifiers = pkceVerifiers; + } + + /** + * Get all supported code challenge methods. + * @return Set of supported code challenge methods. + */ + public Set getSupportedCodeChallengeMethods() { + Set supportedCodeChallengeMethods = this.pkceVerifiers.keySet(); + return supportedCodeChallengeMethods; + } + + /** + * Check code challenge method is supported or not. + * @param codeChallengeMethod + * Code challenge method parameter. + * @return true if the code challenge method is supported. + * false otherwise. + */ + public boolean isCodeChallengeMethodSupported(String codeChallengeMethod) { + if (codeChallengeMethod == null) { + return false; + } + return this.pkceVerifiers.containsKey(codeChallengeMethod); + } + + /** + * Check presence of PKCE parameters and validate. + * @param requestParameters + * Map of query parameters of Authorization request. + * @param codeVerifier + * Code verifier. + * @return true: (1) in case of Authorization Code Grant without PKCE. + * (2) in case of Authorization Code Grant with PKCE and code verifier + * matched with code challenge based on code challenge method. + * false: in case of Authorization Code Grant with PKCE and code verifier + * does not match with code challenge based on code challenge method. + * @throws PkceValidationException + * (1) Code verifier must be provided for this authorization code. + * (2) Code verifier not required for this authorization code. + */ + public boolean checkAndValidate(Map requestParameters, String codeVerifier) throws PkceValidationException { + if (!hasPkceParameters(requestParameters, codeVerifier)) { + return true; + } + String codeChallengeMethod = extractCodeChallengeMethod(requestParameters); + return pkceVerifiers.get(codeChallengeMethod).verify(codeVerifier, + requestParameters.get(PkceValidationService.CODE_CHALLENGE)); + } + + /** + * Check if PKCE parameters are present. + * @param requestParameters + * Map of authorization request parameters. + * @param codeVerifier + * Code verifier. + * @return true: There are Code Challenge and Code Verifier parameters with not null value. + * false: There are no PKCE parameters. + * @throws PkceValidationException + * (1) Code verifier must be provided for this authorization code. + * (2) Code verifier not required for this authorization code. + */ + protected boolean hasPkceParameters(Map requestParameters, String codeVerifier) throws PkceValidationException{ + String codeChallenge = requestParameters.get(CODE_CHALLENGE); + if (codeChallenge != null) { + if (codeVerifier != null && !codeVerifier.isEmpty()) { + return true; + }else { + throw new PkceValidationException("Code verifier must be provided for this authorization code."); + } + }else if (codeVerifier != null && !codeVerifier.isEmpty()){ + throw new PkceValidationException("Code verifier not required for this authorization code."); + } + return false; + } + + /** + * Extract code challenge method from request. + * @param requestParameters: Authorization request parameters. + * @return + * If there is no code challenge method in authorization request then return: "plain" + * Otherwise return the value of code challenge method parameter. + */ + protected String extractCodeChallengeMethod(Map requestParameters) { + String codeChallengeMethod = requestParameters.get(CODE_CHALLENGE_METHOD); + if (codeChallengeMethod == null) { + return "plain"; + }else { + return codeChallengeMethod; + } + } + + /** + * Validate the code verifier parameter based on RFC 7636 recommendations. + * + * @param codeVerifier: Code Verifier parameter from token request. + * @return true or false based on evaluation. + */ + public static boolean isCodeVerifierParameterValid(String codeVerifier) { + return matchWithPattern(codeVerifier); + } + + /** + * Validate the code challenge parameter based on RFC 7636 recommendations. + * + * @param codeChallenge: Code Challenge parameter from token request. + * @return true or false based on evaluation. + */ + public static boolean isCodeChallengeParameterValid(String codeChallenge) { + return matchWithPattern(codeChallenge); + } + + /** + * Validate parameter with predefined regular expression (length and used + * character set) + * + * @param parameter: Code Verifier or Code Challenge + * @return true or false based on parameter match with regular expression + */ + protected static boolean matchWithPattern(String parameter) { + if (parameter == null) { + return false; + } + final Matcher matcher = pattern.matcher(parameter); + return matcher.matches(); + } +} diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceVerifier.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceVerifier.java new file mode 100644 index 00000000000..9977d6db46a --- /dev/null +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceVerifier.java @@ -0,0 +1,32 @@ +package org.cloudfoundry.identity.uaa.oauth.pkce; + +/** + * Each PKCE verifier MUST implement this interface to be able to be used + * in PKCE validation service. + * + * @author Zoltan Maradics + * + */ +public interface PkceVerifier { + + /** + * Verify that the code verifier matches the code challenge based on code challenge method. + * code_challenge = code_challenge_method(code_verifier) + * + * @param codeVerifier + * Code verifier parameter. + * @param codeChallenge + * Code challenge parameter. + * @return true: if code verifier transformed with code challenge method match + * with code challenge. + * false: otherwise. + */ + public boolean verify(String codeVerifier, String codeChallenge); + + /** + * Getter for Code Challenge Method name. + * + * @return + */ + public String getCodeChallengeMethod(); +} diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/PlainPkceVerifier.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/PlainPkceVerifier.java new file mode 100644 index 00000000000..d2e89da6f75 --- /dev/null +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/PlainPkceVerifier.java @@ -0,0 +1,27 @@ +package org.cloudfoundry.identity.uaa.oauth.pkce.verifiers; + +import org.cloudfoundry.identity.uaa.oauth.pkce.PkceVerifier; + +/** + * Plain code challenge method implementation. + * + * @author Zoltan Maradics + * + */ +public class PlainPkceVerifier implements PkceVerifier{ + + private final String codeChallengeMethod = "plain"; + + @Override + public boolean verify(String codeVerifier, String codeChallenge) { + if (codeVerifier == null || codeChallenge == null) { + return false; + } + return codeChallenge.equals(codeVerifier); + } + + @Override + public String getCodeChallengeMethod() { + return codeChallengeMethod; + } +} diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/S256PkceVerifier.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/S256PkceVerifier.java new file mode 100644 index 00000000000..6d3d998c595 --- /dev/null +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/S256PkceVerifier.java @@ -0,0 +1,49 @@ +package org.cloudfoundry.identity.uaa.oauth.pkce.verifiers; + +import java.io.UnsupportedEncodingException; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; + +import org.apache.commons.codec.binary.Base64; +import org.cloudfoundry.identity.uaa.oauth.pkce.PkceVerifier; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * SHA-256 code challenge method implementation. + * + * @author Zoltan Maradics + * + */ +public class S256PkceVerifier implements PkceVerifier { + + private static Logger logger = LoggerFactory.getLogger(S256PkceVerifier.class); + private final String codeChallengeMethod = "S256"; + + public S256PkceVerifier() { + } + + @Override + public boolean verify(String codeVerifier, String codeChallenge) { + if (codeVerifier == null || codeChallenge == null) { + return false; + } + try { + byte[] bytes = codeVerifier.getBytes("US-ASCII"); + MessageDigest md = MessageDigest.getInstance("SHA-256"); + md.update(bytes, 0, bytes.length); + byte[] digest = md.digest(); + return codeChallenge.contentEquals(Base64.encodeBase64URLSafeString(digest)); + } catch (UnsupportedEncodingException e) { + logger.debug(e.getMessage(),e); + } catch (NoSuchAlgorithmException e) { + logger.debug(e.getMessage(),e); + } + return false; + } + + @Override + public String getCodeChallengeMethod() { + return codeChallengeMethod; + } +} diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/PkceEnhancedAuthorizationCodeTokenGranter.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/PkceEnhancedAuthorizationCodeTokenGranter.java new file mode 100644 index 00000000000..a8bbd878ee7 --- /dev/null +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/PkceEnhancedAuthorizationCodeTokenGranter.java @@ -0,0 +1,121 @@ +package org.cloudfoundry.identity.uaa.oauth.token; + +import java.util.HashMap; +import java.util.Map; + +import org.cloudfoundry.identity.uaa.oauth.pkce.PkceValidationException; +import org.cloudfoundry.identity.uaa.oauth.pkce.PkceValidationService; +import org.springframework.security.core.Authentication; +import org.springframework.security.oauth2.common.exceptions.InvalidClientException; +import org.springframework.security.oauth2.common.exceptions.InvalidGrantException; +import org.springframework.security.oauth2.common.exceptions.InvalidRequestException; +import org.springframework.security.oauth2.common.exceptions.RedirectMismatchException; +import org.springframework.security.oauth2.common.util.OAuth2Utils; +import org.springframework.security.oauth2.provider.ClientDetails; +import org.springframework.security.oauth2.provider.ClientDetailsService; +import org.springframework.security.oauth2.provider.OAuth2Authentication; +import org.springframework.security.oauth2.provider.OAuth2Request; +import org.springframework.security.oauth2.provider.OAuth2RequestFactory; +import org.springframework.security.oauth2.provider.TokenRequest; +import org.springframework.security.oauth2.provider.code.AuthorizationCodeServices; +import org.springframework.security.oauth2.provider.code.AuthorizationCodeTokenGranter; +import org.springframework.security.oauth2.provider.token.AuthorizationServerTokenServices; + +public class PkceEnhancedAuthorizationCodeTokenGranter extends AuthorizationCodeTokenGranter { + + private final AuthorizationCodeServices authorizationCodeServices; + + private PkceValidationService pkceValidationService; + + public PkceEnhancedAuthorizationCodeTokenGranter(AuthorizationServerTokenServices tokenServices, + AuthorizationCodeServices authorizationCodeServices, ClientDetailsService clientDetailsService, + OAuth2RequestFactory requestFactory) { + super(tokenServices, authorizationCodeServices, clientDetailsService, requestFactory); + this.authorizationCodeServices = authorizationCodeServices; + } + + @Override + protected OAuth2Authentication getOAuth2Authentication(ClientDetails client, TokenRequest tokenRequest) { + + Map parameters = tokenRequest.getRequestParameters(); + String authorizationCode = parameters.get("code"); + String redirectUri = parameters.get(OAuth2Utils.REDIRECT_URI); + + if (authorizationCode == null) { + throw new InvalidRequestException("An authorization code must be supplied."); + } + + /* + * PKCE code verifier parameter length and charset validation + */ + String codeVerifier = parameters.get(PkceValidationService.CODE_VERIFIER); + if (codeVerifier != null && !PkceValidationService.isCodeVerifierParameterValid(codeVerifier)) { + throw new InvalidRequestException("Code verifier length must between 43 and 128 and use only [A-Z],[a-z],[0-9],_,.,-,~ characters."); + } + + OAuth2Authentication storedAuth = authorizationCodeServices.consumeAuthorizationCode(authorizationCode); + if (storedAuth == null) { + throw new InvalidGrantException("Invalid authorization code: " + authorizationCode); + } + + /* + * PKCE code verifier parameter verification + */ + try { + if (pkceValidationService != null && !pkceValidationService.checkAndValidate(storedAuth.getOAuth2Request().getRequestParameters(), codeVerifier)) { + // has PkceValidation service and validation failed + throw new InvalidGrantException("Invalid code verifier: " + codeVerifier); + } + } catch (PkceValidationException exception) { + // during the validation one of the PKCE parameters missing + throw new InvalidGrantException("PKCE error: "+ exception.getMessage()); + } + // No pkceValidationService defined or Pkce validation successfully passed + + OAuth2Request pendingOAuth2Request = storedAuth.getOAuth2Request(); + // https://jira.springsource.org/browse/SECOAUTH-333 + // This might be null, if the authorization was done without the redirect_uri + // parameter + String redirectUriApprovalParameter = pendingOAuth2Request.getRequestParameters().get(OAuth2Utils.REDIRECT_URI); + + if ((redirectUri != null || redirectUriApprovalParameter != null) + && !pendingOAuth2Request.getRedirectUri().equals(redirectUri)) { + throw new RedirectMismatchException("Redirect URI mismatch."); + } + + String pendingClientId = pendingOAuth2Request.getClientId(); + String clientId = tokenRequest.getClientId(); + if (clientId != null && !clientId.equals(pendingClientId)) { + // just a sanity check. + throw new InvalidClientException("Client ID mismatch"); + } + + // Secret is not required in the authorization request, so it won't be available + // in the pendingAuthorizationRequest. We do want to check that a secret is + // provided + // in the token request, but that happens elsewhere. + + Map combinedParameters = new HashMap( + pendingOAuth2Request.getRequestParameters()); + // Combine the parameters adding the new ones last so they override if there are + // any clashes + combinedParameters.putAll(parameters); + + // Make a new stored request with the combined parameters + OAuth2Request finalStoredOAuth2Request = pendingOAuth2Request.createOAuth2Request(combinedParameters); + + Authentication userAuth = storedAuth.getUserAuthentication(); + + return new OAuth2Authentication(finalStoredOAuth2Request, userAuth); + + } + + public PkceValidationService getPkceValidationService() { + return pkceValidationService; + } + + public void setPkceValidationService(PkceValidationService pkceValidationService) { + this.pkceValidationService = pkceValidationService; + } + +} diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationEndpointTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationEndpointTest.java index 454b54b24e4..6cf3d4c8657 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationEndpointTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaAuthorizationEndpointTest.java @@ -3,7 +3,12 @@ import org.cloudfoundry.identity.uaa.authentication.UaaAuthentication; import org.cloudfoundry.identity.uaa.authentication.UaaAuthenticationDetails; import org.cloudfoundry.identity.uaa.authentication.UaaPrincipal; +import org.cloudfoundry.identity.uaa.oauth.pkce.PkceValidationService; +import org.cloudfoundry.identity.uaa.oauth.pkce.PkceVerifier; +import org.cloudfoundry.identity.uaa.oauth.pkce.verifiers.PlainPkceVerifier; +import org.cloudfoundry.identity.uaa.oauth.pkce.verifiers.S256PkceVerifier; import org.cloudfoundry.identity.uaa.oauth.token.CompositeToken; +import org.cloudfoundry.identity.uaa.test.UaaTestAccounts; import org.junit.Before; import org.junit.Test; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; @@ -42,6 +47,7 @@ public class UaaAuthorizationEndpointTest { private AuthorizationCodeServices authorizationCodeServices; private Set responseTypes; private OpenIdSessionStateCalculator openIdSessionStateCalculator; + private PkceValidationService pkceValidationService; private HashMap model = new HashMap<>(); private SimpleSessionStatus sessionStatus = new SimpleSessionStatus(); @@ -58,6 +64,14 @@ public void setup() { uaaAuthorizationEndpoint.setOpenIdSessionStateCalculator(openIdSessionStateCalculator); responseTypes = new HashSet<>(); + PlainPkceVerifier plainPkceVerifier = new PlainPkceVerifier(); + S256PkceVerifier s256PkceVerifier = new S256PkceVerifier(); + Map pkceVerifiers = new HashMap(); + pkceVerifiers.put(plainPkceVerifier.getCodeChallengeMethod(), plainPkceVerifier); + pkceVerifiers.put(s256PkceVerifier.getCodeChallengeMethod(), s256PkceVerifier); + pkceValidationService = new PkceValidationService(pkceVerifiers); + uaaAuthorizationEndpoint.setPkceValidationService(pkceValidationService); + when(openIdSessionStateCalculator.calculate("userid", null, "http://example.com")).thenReturn("opbshash"); when(authorizationCodeServices.createAuthorizationCode(any(OAuth2Authentication.class))).thenReturn("code"); } @@ -305,6 +319,35 @@ public void testApproveWithModifiedApprovalParameters() { assertThat(((RedirectView)view).getUrl(), containsString("error=invalid_scope")); } + @Test(expected = InvalidRequestException.class) + public void testShortCodeChallengeParameter() throws Exception { + Map parameters = new HashMap(); + parameters.put(PkceValidationService.CODE_CHALLENGE, "ShortCodeChallenge"); + uaaAuthorizationEndpoint.validateAuthorizationRequestPkceParameters(parameters); + } + + @Test(expected = InvalidRequestException.class) + public void testLongCodeChallengeParameter() throws Exception { + Map parameters = new HashMap(); + parameters.put(PkceValidationService.CODE_CHALLENGE, "LongCodeChallenge12346574382823193700987654321326352173528351287635126532123452534234254323254325325325432342532532254325432532532"); + uaaAuthorizationEndpoint.validateAuthorizationRequestPkceParameters(parameters); + } + + @Test(expected = InvalidRequestException.class) + public void testForbiddenCodeChallengeParameter() throws Exception { + Map parameters = new HashMap(); + parameters.put(PkceValidationService.CODE_CHALLENGE, "#ForbiddenCodeChallenge098765445647544743211234657438282319370#"); + uaaAuthorizationEndpoint.validateAuthorizationRequestPkceParameters(parameters); + } + + @Test(expected = InvalidRequestException.class) + public void testUnsupportedCodeChallengeMethodParameter() throws Exception { + Map parameters = new HashMap(); + parameters.put(PkceValidationService.CODE_CHALLENGE, UaaTestAccounts.CODE_CHALLENGE); + parameters.put(PkceValidationService.CODE_CHALLENGE_METHOD, "unsupportedMethod"); + uaaAuthorizationEndpoint.validateAuthorizationRequestPkceParameters(parameters); + } + private AuthorizationRequest getAuthorizationRequest(String clientId, String redirectUri, String state, String scope, Set responseTypes) { HashMap parameters = new HashMap<>(); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationServiceTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationServiceTest.java new file mode 100644 index 00000000000..bb0a11e0e49 --- /dev/null +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/PkceValidationServiceTest.java @@ -0,0 +1,129 @@ +package org.cloudfoundry.identity.uaa.oauth.pkce; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; +import static org.hamcrest.Matchers.*; + +import java.util.Arrays; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +import org.cloudfoundry.identity.uaa.oauth.pkce.verifiers.PlainPkceVerifier; +import org.cloudfoundry.identity.uaa.oauth.pkce.verifiers.S256PkceVerifier; +import org.junit.Before; +import org.junit.Test; + +/** + * + * @author Zoltan Maradics + * + */ +public class PkceValidationServiceTest { + + private PkceValidationService pkceValidationService; + private Map authorizeRequestParameters; + + private final String longCodeChallengeOrCodeVerifierParameter = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cME9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cME9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"; + private final String shortCodeChallengeOrCodeVerifierParameter = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-c"; + private final String containsForbiddenCharactersCodeChallengeOrCodeVerifierParameter = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM%"; + private final String validPlainCodeChallengeOrCodeVerifierParameter = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"; + + private final String invalidCodeChallengeMethod = "InvalidMethod"; + + @Before + public void createPkceValidationService() throws Exception { + pkceValidationService = new PkceValidationService(createPkceVerifiers()); + authorizeRequestParameters = new HashMap(); + } + + @Test + public void testLongCodeChallengeParameter() throws Exception { + assertFalse(PkceValidationService.matchWithPattern(longCodeChallengeOrCodeVerifierParameter)); + } + + @Test + public void testShortCodeChallengeParameter() throws Exception { + assertFalse(PkceValidationService.matchWithPattern(shortCodeChallengeOrCodeVerifierParameter)); + } + + @Test + public void testContainsForbiddenCharactersCodeChallengeParameter() throws Exception { + assertFalse(PkceValidationService + .matchWithPattern(containsForbiddenCharactersCodeChallengeOrCodeVerifierParameter)); + } + + @Test + public void testNullCodeChallengeOrCodeVerifierParameters() throws Exception { + assertFalse(PkceValidationService.matchWithPattern(null)); + } + + @Test + public void testValidCodeChallengeParameter() throws Exception { + assertTrue(PkceValidationService.matchWithPattern(validPlainCodeChallengeOrCodeVerifierParameter)); + } + + @Test + public void testInvalidCodeChallengeMethodParameter() throws Exception { + assertFalse(pkceValidationService.isCodeChallengeMethodSupported(invalidCodeChallengeMethod)); + } + + @Test + public void testNullCodeChallengeMethodParameter() throws Exception { + assertFalse(pkceValidationService.isCodeChallengeMethodSupported(null)); + } + + @Test + public void testS256CodeChallengeMethodParameter() throws Exception { + assertTrue(pkceValidationService.isCodeChallengeMethodSupported("S256")); + } + + @Test + public void testPlainCodeChallengeMethodParameter() throws Exception { + assertTrue(pkceValidationService.isCodeChallengeMethodSupported("plain")); + } + + @Test + public void testNoPkceParametersForEvaluation() throws Exception { + assertTrue(pkceValidationService.checkAndValidate(authorizeRequestParameters, null)); + } + + @Test(expected = PkceValidationException.class) + public void testCodeChallengeMissingForEvaluation() throws Exception { + pkceValidationService.checkAndValidate(authorizeRequestParameters, + validPlainCodeChallengeOrCodeVerifierParameter); + } + + @Test(expected = PkceValidationException.class) + public void testCodeVerifierMissingForEvaluation() throws Exception { + authorizeRequestParameters.put(PkceValidationService.CODE_CHALLENGE, + validPlainCodeChallengeOrCodeVerifierParameter); + pkceValidationService.checkAndValidate(authorizeRequestParameters, ""); + } + + @Test + public void testNoCodeChallengeMethodForEvaluation() throws Exception { + authorizeRequestParameters.put(PkceValidationService.CODE_CHALLENGE, + validPlainCodeChallengeOrCodeVerifierParameter); + assertThat(pkceValidationService.checkAndValidate(authorizeRequestParameters, + validPlainCodeChallengeOrCodeVerifierParameter), is(true)); + } + + @Test + public void testPkceValidationServiceConstructorWithCodeChallengeMethodsMap() throws Exception { + Set testHashSet = new HashSet<>(Arrays.asList("S256", "plain")); + assertEquals(testHashSet, pkceValidationService.getSupportedCodeChallengeMethods()); + } + + private Map createPkceVerifiers() { + S256PkceVerifier s256PkceVerifier = new S256PkceVerifier(); + PlainPkceVerifier plainPkceVerifier = new PlainPkceVerifier(); + Map pkceVerifiers = new HashMap(); + pkceVerifiers.put(plainPkceVerifier.getCodeChallengeMethod(), plainPkceVerifier); + pkceVerifiers.put(s256PkceVerifier.getCodeChallengeMethod(), s256PkceVerifier); + return pkceVerifiers; + } +} diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/PlainPkceVerifierTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/PlainPkceVerifierTest.java new file mode 100644 index 00000000000..54881644af4 --- /dev/null +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/PlainPkceVerifierTest.java @@ -0,0 +1,47 @@ +package org.cloudfoundry.identity.uaa.oauth.pkce.verifiers; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.cloudfoundry.identity.uaa.oauth.pkce.verifiers.PlainPkceVerifier; +import org.junit.Before; +import org.junit.Test; + +/** + * + * @author Zoltan Maradics + * + */ +public class PlainPkceVerifierTest { + + private PlainPkceVerifier plainPkceVerifier; + + private final String matchParameter = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"; + private final String mismatchParameter = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"; + + @Before + public void createPlainCodeChallengeMethod() throws Exception { + plainPkceVerifier = new PlainPkceVerifier(); + } + + @Test + public void testCodeVerifierMethodWithMatchParameters() throws Exception { + assertTrue(plainPkceVerifier.verify(matchParameter, matchParameter)); + } + + @Test + public void testCodeVerifierMethodWithMismatchParameters() throws Exception { + assertFalse(plainPkceVerifier.verify(matchParameter, mismatchParameter)); + } + + @Test + public void testCodeChallengeIsNull() throws Exception { + assertFalse(plainPkceVerifier.verify(matchParameter, null)); + } + + @Test + public void testCodeVerifierIsNull() throws Exception { + assertFalse(plainPkceVerifier.verify(null, matchParameter)); + } + +} diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/S256PkceVerifierTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/S256PkceVerifierTest.java new file mode 100644 index 00000000000..aef11f7736f --- /dev/null +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/pkce/verifiers/S256PkceVerifierTest.java @@ -0,0 +1,47 @@ +package org.cloudfoundry.identity.uaa.oauth.pkce.verifiers; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.cloudfoundry.identity.uaa.oauth.pkce.verifiers.S256PkceVerifier; +import org.junit.Before; +import org.junit.Test; + +/** + * + * @author Zoltan Maradics + * + */ +public class S256PkceVerifierTest { + + private S256PkceVerifier s256CodeChallengeMethod; + + private final String validCodeChallenge = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"; + private final String validCodeVerifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"; + + @Before + public void createS256CodeChallengeMethod() throws Exception { + s256CodeChallengeMethod = new S256PkceVerifier(); + } + + @Test + public void testCodeVerifierMethodWithMatchParameters() throws Exception { + assertTrue(s256CodeChallengeMethod.verify(validCodeVerifier, validCodeChallenge)); + } + + @Test + public void testCodeVerifierMethodWithMismatchParameters() throws Exception { + assertFalse(s256CodeChallengeMethod.verify(validCodeVerifier, validCodeVerifier)); + } + + @Test + public void testCodeChallengeIsNull() throws Exception { + assertFalse(s256CodeChallengeMethod.verify(validCodeVerifier, null)); + } + + @Test + public void testCodeVerifierIsNull() throws Exception { + assertFalse(s256CodeChallengeMethod.verify(null, validCodeChallenge)); + } + +} diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/test/UaaTestAccounts.java b/server/src/test/java/org/cloudfoundry/identity/uaa/test/UaaTestAccounts.java index 0e8aecb00be..08755fbce4d 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/test/UaaTestAccounts.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/test/UaaTestAccounts.java @@ -52,6 +52,12 @@ public class UaaTestAccounts implements TestAccounts { public static final String DEFAULT_PASSWORD = "koala"; public static final String DEFAULT_USERNAME = "marissa"; + + public static final String CODE_CHALLENGE = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"; + + public static final String CODE_CHALLENGE_METHOD_S256 = "S256"; + + public static final String CODE_VERIFIER = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"; private static final Logger logger = LoggerFactory.getLogger(UaaTestAccounts.class); diff --git a/uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml b/uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml index eb5efb7f3a9..98839e9d598 100755 --- a/uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml +++ b/uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml @@ -34,13 +34,28 @@ client-details-service-ref="jdbcClientDetailsService" token-services-ref="tokenServices" user-approval-handler-ref="userManagedApprovalHandler" authorization-request-manager-ref="authorizationRequestManager" request-validator-ref="oauth2RequestValidator"> - + + + + + + + + + + + + + + + @@ -158,6 +173,7 @@ + @@ -454,6 +470,20 @@ + + + + + + + + + + + + + + diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/AuthorizationCodeGrantIntegrationTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/AuthorizationCodeGrantIntegrationTests.java index a87dd3946ec..ae9fad2ad74 100755 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/AuthorizationCodeGrantIntegrationTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/AuthorizationCodeGrantIntegrationTests.java @@ -32,7 +32,9 @@ import java.util.Map; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; +import static org.hamcrest.CoreMatchers.containsString; /** * @author Dave Syer @@ -55,6 +57,98 @@ public void testSuccessfulAuthorizationCodeFlow() throws Exception { } @Test + public void testSuccessfulAuthorizationCodeFlowWithPkceS256() throws Exception { + testAuthorizationCodeFlowWithPkce_Internal(UaaTestAccounts.CODE_CHALLENGE, + UaaTestAccounts.CODE_CHALLENGE_METHOD_S256, UaaTestAccounts.CODE_VERIFIER); + testAuthorizationCodeFlowWithPkce_Internal(UaaTestAccounts.CODE_CHALLENGE, + UaaTestAccounts.CODE_CHALLENGE_METHOD_S256, UaaTestAccounts.CODE_VERIFIER); + } + + @Test + public void testSuccessfulAuthorizationCodeFlowWithPkcePlain() throws Exception { + testAuthorizationCodeFlowWithPkce_Internal(UaaTestAccounts.CODE_CHALLENGE, "plain", UaaTestAccounts.CODE_CHALLENGE); + testAuthorizationCodeFlowWithPkce_Internal(UaaTestAccounts.CODE_CHALLENGE, "plain", UaaTestAccounts.CODE_CHALLENGE); + } + + @Test + public void testPkcePlainWithWrongCodeVerifier() throws Exception { + ResponseEntity tokenResponse = doAuthorizeAndTokenRequest(UaaTestAccounts.CODE_CHALLENGE, "plain", UaaTestAccounts.CODE_VERIFIER); + assertEquals(HttpStatus.BAD_REQUEST, tokenResponse.getStatusCode()); + Map body = tokenResponse.getBody(); + assertThat(body.get("error"), containsString("invalid_grant")); + assertThat(body.get("error_description"), containsString("Invalid code verifier")); + } + + @Test + public void testPkceS256WithWrongCodeVerifier() throws Exception { + ResponseEntity tokenResponse = doAuthorizeAndTokenRequest(UaaTestAccounts.CODE_CHALLENGE, UaaTestAccounts.CODE_CHALLENGE_METHOD_S256, UaaTestAccounts.CODE_CHALLENGE); + assertEquals(HttpStatus.BAD_REQUEST, tokenResponse.getStatusCode()); + Map body = tokenResponse.getBody(); + assertThat(body.get("error"), containsString("invalid_grant")); + assertThat(body.get("error_description"), containsString("Invalid code verifier")); + } + + @Test + public void testMissingCodeChallenge() throws Exception { + ResponseEntity tokenResponse = doAuthorizeAndTokenRequest("", UaaTestAccounts.CODE_CHALLENGE_METHOD_S256, UaaTestAccounts.CODE_VERIFIER); + assertEquals(HttpStatus.BAD_REQUEST, tokenResponse.getStatusCode()); + Map body = tokenResponse.getBody(); + assertThat(body.get("error"), containsString("invalid_grant")); + assertThat(body.get("error_description"), containsString("PKCE error: Code verifier not required for this authorization code.")); + } + + @Test + public void testMissingCodeVerifier() throws Exception { + ResponseEntity tokenResponse = doAuthorizeAndTokenRequest(UaaTestAccounts.CODE_CHALLENGE, UaaTestAccounts.CODE_CHALLENGE_METHOD_S256, ""); + assertEquals(HttpStatus.BAD_REQUEST, tokenResponse.getStatusCode()); + Map body = tokenResponse.getBody(); + assertThat(body.get("error"), containsString("invalid_grant")); + assertThat(body.get("error_description"), containsString("PKCE error: Code verifier must be provided for this authorization code.")); + } + + @Test + public void testInvalidCodeChallenge() throws Exception { + AuthorizationCodeResourceDetails resource = testAccounts.getDefaultAuthorizationCodeResource(); + String responseLocation = IntegrationTestUtils.getAuthorizationResponse(serverRunning, + resource.getClientId(), + testAccounts.getUserName(), + testAccounts.getPassword(), + resource.getPreEstablishedRedirectUri(), + "ShortCodeChallenge", + UaaTestAccounts.CODE_CHALLENGE_METHOD_S256); + assertThat(responseLocation, containsString("Code challenge length must between 43 and 128 and use only [A-Z],[a-z],[0-9],_,.,-,~ characters.")); + } + + @Test + public void testInvalidCodeVerifier() throws Exception { + AuthorizationCodeResourceDetails resource = testAccounts.getDefaultAuthorizationCodeResource(); + ResponseEntity tokenResponse = IntegrationTestUtils.getTokens(serverRunning, + testAccounts, + resource.getClientId(), + resource.getClientSecret(), + resource.getPreEstablishedRedirectUri(), + "invalidCodeVerifier", + "authorizationCode"); + assertEquals(HttpStatus.BAD_REQUEST, tokenResponse.getStatusCode()); + Map body = tokenResponse.getBody(); + assertThat(body.get("error"), containsString("invalid_request")); + assertThat(body.get("error_description"), containsString("Code verifier length must")); + } + + @Test + public void testUnsupportedCodeChallengeMethod() throws Exception { + AuthorizationCodeResourceDetails resource = testAccounts.getDefaultAuthorizationCodeResource(); + String responseLocation = IntegrationTestUtils.getAuthorizationResponse(serverRunning, + resource.getClientId(), + testAccounts.getUserName(), + testAccounts.getPassword(), + resource.getPreEstablishedRedirectUri(), + UaaTestAccounts.CODE_CHALLENGE, + "UnsupportedCodeChallengeMethod"); + assertThat(responseLocation, containsString("Unsupported code challenge method.")); + } + + @Test public void testZoneDoesNotExist() { ServerRunning.UriBuilder builder = serverRunning.buildUri(serverRunning.getAuthorizationUri().replace("localhost", "testzonedoesnotexist.localhost")) .queryParam("response_type", "code") @@ -111,4 +205,38 @@ public void testSuccessfulAuthorizationCodeFlow_Internal() throws Exception { assertTrue("Wrong claims: " + token.getClaims(), token.getClaims().contains("\"aud\"")); assertTrue("Wrong claims: " + token.getClaims(), token.getClaims().contains("\"user_id\"")); } + + private void testAuthorizationCodeFlowWithPkce_Internal(String codeChallenge, String codeChallengeMethod, String codeVerifier) throws Exception { + + ResponseEntity tokenResponse = doAuthorizeAndTokenRequest(codeChallenge, codeChallengeMethod, codeVerifier); + assertEquals(HttpStatus.OK, tokenResponse.getStatusCode()); + Map body = tokenResponse.getBody(); + Jwt token = JwtHelper.decode(body.get("access_token")); + assertTrue("Wrong claims: " + token.getClaims(), token.getClaims().contains("\"aud\"")); + assertTrue("Wrong claims: " + token.getClaims(), token.getClaims().contains("\"user_id\"")); + IntegrationTestUtils.callCheckToken(serverRunning, + testAccounts, + body.get("access_token"), + testAccounts.getDefaultAuthorizationCodeResource().getClientId(), + testAccounts.getDefaultAuthorizationCodeResource().getClientSecret()); + } + + private ResponseEntity doAuthorizeAndTokenRequest(String codeChallenge, String codeChallengeMethod, String codeVerifier) throws Exception { + AuthorizationCodeResourceDetails resource = testAccounts.getDefaultAuthorizationCodeResource(); + String authorizationResponse = IntegrationTestUtils.getAuthorizationResponse(serverRunning, + resource.getClientId(), + testAccounts.getUserName(), + testAccounts.getPassword(), + resource.getPreEstablishedRedirectUri(), + codeChallenge, + codeChallengeMethod); + String authorizationCode = authorizationResponse.split("code=")[1].split("&")[0]; + return IntegrationTestUtils.getTokens(serverRunning, + testAccounts, + resource.getClientId(), + resource.getClientSecret(), + resource.getPreEstablishedRedirectUri(), + codeVerifier, + authorizationCode); + } } diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/util/IntegrationTestUtils.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/util/IntegrationTestUtils.java index 26f14139e46..4962278032a 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/util/IntegrationTestUtils.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/util/IntegrationTestUtils.java @@ -1119,6 +1119,134 @@ public static HttpHeaders getHeaders(CookieStore cookies) { headers.add("Cookie", cookie.getName() + "=" + cookie.getValue()); } return headers; + } + + public static String getAuthorizationResponse(ServerRunning serverRunning, + String clientId, + String username, + String password, + String redirectUri, + String codeChallenge, + String codeChallengeMethod) throws Exception { + BasicCookieStore cookies = new BasicCookieStore(); + String mystateid = "mystateid"; + ServerRunning.UriBuilder builder = serverRunning.buildUri("/oauth/authorize") + .queryParam("response_type", "code") + .queryParam("state", mystateid) + .queryParam("client_id", clientId); + if (hasText(redirectUri)) { + builder = builder.queryParam("redirect_uri", redirectUri); + } + if (hasText(codeChallenge)) { + builder = builder.queryParam("code_challenge", codeChallenge); + } + if (hasText(codeChallengeMethod)) { + builder = builder.queryParam("code_challenge_method", codeChallengeMethod); + } + URI uri = builder.build(); + ResponseEntity result = + serverRunning.createRestTemplate().exchange( + uri.toString(), + HttpMethod.GET, + new HttpEntity<>(null, getHeaders(cookies)), + Void.class + ); + assertEquals(HttpStatus.FOUND, result.getStatusCode()); + String location = result.getHeaders().getLocation().toString(); + if (result.getHeaders().containsKey("Set-Cookie")) { + for (String header : result.getHeaders().get("Set-Cookie")) { + int nameLength = header.indexOf('='); + cookies.addCookie(new BasicClientCookie(header.substring(0, nameLength), header.substring(nameLength + 1))); + } + } + ResponseEntity response = serverRunning.getForString(location, getHeaders(cookies)); + if (response.getHeaders().containsKey("Set-Cookie")) { + for (String cookie : response.getHeaders().get("Set-Cookie")) { + int nameLength = cookie.indexOf('='); + cookies.addCookie(new BasicClientCookie(cookie.substring(0, nameLength), cookie.substring(nameLength + 1))); + } + } + MultiValueMap formData = new LinkedMultiValueMap<>(); + assertTrue(response.getBody().contains("/login.do")); + assertTrue(response.getBody().contains("username")); + assertTrue(response.getBody().contains("password")); + String csrf = IntegrationTestUtils.extractCookieCsrf(response.getBody()); + formData.add("username", username); + formData.add("password", password); + formData.add(CookieBasedCsrfTokenRepository.DEFAULT_CSRF_COOKIE_NAME, csrf); + // Should be redirected to the original URL, but now authenticated + result = serverRunning.postForResponse("/login.do", getHeaders(cookies), formData); + assertEquals(HttpStatus.FOUND, result.getStatusCode()); + cookies.clear(); + if (result.getHeaders().containsKey("Set-Cookie")) { + for (String cookie : result.getHeaders().get("Set-Cookie")) { + int nameLength = cookie.indexOf('='); + cookies.addCookie(new BasicClientCookie(cookie.substring(0, nameLength), cookie.substring(nameLength + 1))); + } + } + response = serverRunning.createRestTemplate().exchange( + result.getHeaders().getLocation().toString(), HttpMethod.GET, new HttpEntity<>(null, getHeaders(cookies)), + String.class); + if (response.getHeaders().containsKey("Set-Cookie")) { + for (String cookie : response.getHeaders().get("Set-Cookie")) { + int nameLength = cookie.indexOf('='); + cookies.addCookie(new BasicClientCookie(cookie.substring(0, nameLength), cookie.substring(nameLength + 1))); + } + } + if (response.getStatusCode() == HttpStatus.OK) { + // The grant access page should be returned + assertTrue(response.getBody().contains("

Application Authorization

")); + formData.clear(); + formData.add(USER_OAUTH_APPROVAL, "true"); + formData.add(DEFAULT_CSRF_COOKIE_NAME, IntegrationTestUtils.extractCookieCsrf(response.getBody())); + result = serverRunning.postForResponse("/oauth/authorize", getHeaders(cookies), formData); + assertEquals(HttpStatus.FOUND, result.getStatusCode()); + location = result.getHeaders().getLocation().toString(); + } else if(response.getStatusCode() == HttpStatus.BAD_REQUEST){ + return response.getBody(); + } else { + // Token cached so no need for second approval + assertEquals(HttpStatus.FOUND, response.getStatusCode()); + location = response.getHeaders().getLocation().toString(); + } + return location; + } + + public static ResponseEntity getTokens(ServerRunning serverRunning, + UaaTestAccounts testAccounts, + String clientId, + String clientSecret, + String redirectUri, + String codeVerifier, + String authorizationCode) throws Exception { + MultiValueMap formData = new LinkedMultiValueMap<>(); + formData.clear(); + formData.add("client_id", clientId); + formData.add("grant_type", GRANT_TYPE_AUTHORIZATION_CODE); + formData.add("code", authorizationCode); + if (hasText(redirectUri)) { + formData.add("redirect_uri", redirectUri); + } + if (hasText(codeVerifier)) { + formData.add("code_verifier", codeVerifier); + } + HttpHeaders tokenHeaders = new HttpHeaders(); + tokenHeaders.set("Authorization", testAccounts.getAuthorizationHeader(clientId, clientSecret)); + return serverRunning.postForMap("/oauth/token", formData, tokenHeaders); + } + + public static void callCheckToken(ServerRunning serverRunning, + UaaTestAccounts testAccounts, + String accessToken, + String clientId, + String clientSecret) { + MultiValueMap formData = new LinkedMultiValueMap<>(); + HttpHeaders headers = new HttpHeaders(); + headers.set("Authorization", testAccounts.getAuthorizationHeader(clientId, clientSecret)); + formData.add("token", accessToken); + ResponseEntity tokenResponse = serverRunning.postForMap("/check_token", formData, headers); + assertEquals(HttpStatus.OK, tokenResponse.getStatusCode()); + assertNotNull(tokenResponse.getBody().get("iss")); } public static Map getAuthorizationCodeTokenMap(ServerRunning serverRunning, diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AuthorizeEndpointDocs.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AuthorizeEndpointDocs.java index 7bfc303ed3e..006981eb774 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AuthorizeEndpointDocs.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AuthorizeEndpointDocs.java @@ -4,6 +4,7 @@ import org.cloudfoundry.identity.uaa.authentication.UaaPrincipal; import org.cloudfoundry.identity.uaa.mock.EndpointDocs; import org.cloudfoundry.identity.uaa.mock.util.MockMvcUtils; +import org.cloudfoundry.identity.uaa.oauth.pkce.PkceValidationService; import org.cloudfoundry.identity.uaa.scim.ScimUser; import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning; import org.cloudfoundry.identity.uaa.scim.jdbc.JdbcScimUserProvisioning; @@ -53,6 +54,8 @@ class AuthorizeEndpointDocs extends EndpointDocs { private final ParameterDescriptor promptParameter = parameterWithName(ID_TOKEN_HINT_PROMPT).description("specifies whether to prompt for user authentication. Only value `" + ID_TOKEN_HINT_PROMPT_NONE + "` is supported.").attributes(key("constraints").value("Optional"), key("type").value(STRING)); private final ParameterDescriptor responseTypeParameter = parameterWithName(RESPONSE_TYPE).attributes(key("constraints").value("Required"), key("type").value(STRING)); private final ParameterDescriptor loginHintParameter = parameterWithName("login_hint").optional(null).type(STRING).description("UAA 4.19.0 Indicates the identity provider to be used. The passed string has to be a URL-Encoded JSON Object, containing the field `origin` with value as `origin_key` of an identity provider."); + private final ParameterDescriptor codeChallenge = parameterWithName(PkceValidationService.CODE_CHALLENGE).description("UAA 4.27.0 [PKCE](https://tools.ietf.org/html/rfc7636) Code Challenge. When `code_challenge` is present also a `code_challenge_method` must be provided. A matching `code_verifier` parameter must be provided in the subsequent request to get an `access_token` from `/oauth/token`").attributes(key("constraints").value("Optional"), key("type").value(STRING)); + private final ParameterDescriptor codeChallengeMethod = parameterWithName(PkceValidationService.CODE_CHALLENGE_METHOD).description("UAA 4.27.0 [PKCE](https://tools.ietf.org/html/rfc7636) Code Challenge Method. `S256` and `plain` methods are supported. `S256` method creates a BASE64 URL encoded SHA256 hash of the `code_verifier`. The `plain` method is intended for constrained devices unable to calculate SHA256. In this case the `code_verifier` equals the `code_challenge`. If possible it is recommended to use `S256`.").attributes(key("constraints").value("Optional"), key("type").value(STRING)); private UaaAuthentication principal; @@ -81,6 +84,8 @@ void browserCodeRequest() throws Exception { .param(SCOPE, "openid oauth.approvals") .param(REDIRECT_URI, "http://localhost/app") .param("login_hint", URLEncoder.encode("{\"origin\":\"uaa\"}", "utf-8")) + .param(PkceValidationService.CODE_CHALLENGE, UaaTestAccounts.CODE_CHALLENGE) + .param(PkceValidationService.CODE_CHALLENGE_METHOD, UaaTestAccounts.CODE_CHALLENGE_METHOD_S256) .session(session); Snippet requestParameters = requestParameters( @@ -88,6 +93,8 @@ void browserCodeRequest() throws Exception { clientIdParameter, scopesParameter, redirectParameter, + codeChallenge, + codeChallengeMethod, loginHintParameter ); @@ -115,12 +122,16 @@ void apiCodeRequest() throws Exception { .param(RESPONSE_TYPE, "code") .param(CLIENT_ID, "login") .param(REDIRECT_URI, "http://localhost/redirect/cf") + .param(PkceValidationService.CODE_CHALLENGE, UaaTestAccounts.CODE_CHALLENGE) + .param(PkceValidationService.CODE_CHALLENGE_METHOD, UaaTestAccounts.CODE_CHALLENGE_METHOD_S256) .param(STATE, new RandomValueStringGenerator().generate()); Snippet requestParameters = requestParameters( responseTypeParameter.description("Space-delimited list of response types. Here, `code` for requesting an authorization code for an access token, as per OAuth spec"), clientIdParameter, redirectParameter, + codeChallenge, + codeChallengeMethod, parameterWithName(STATE).description("any random string to be returned in the Location header as a query parameter, used to achieve per-request customization").attributes(key("constraints").value("Required"), key("type").value(STRING)) ); diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/TokenEndpointDocs.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/TokenEndpointDocs.java index 9e06651b707..33138b4f603 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/TokenEndpointDocs.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/TokenEndpointDocs.java @@ -5,6 +5,7 @@ import org.cloudfoundry.identity.uaa.authentication.UaaPrincipal; import org.cloudfoundry.identity.uaa.mock.token.AbstractTokenMockMvcTests; import org.cloudfoundry.identity.uaa.mock.util.MockMvcUtils; +import org.cloudfoundry.identity.uaa.oauth.pkce.PkceValidationService; import org.cloudfoundry.identity.uaa.oauth.token.CompositeToken; import org.cloudfoundry.identity.uaa.oauth.token.TokenConstants; import org.cloudfoundry.identity.uaa.provider.IdentityProvider; @@ -13,6 +14,7 @@ import org.cloudfoundry.identity.uaa.scim.ScimUser; import org.cloudfoundry.identity.uaa.test.JUnitRestDocumentationExtension; import org.cloudfoundry.identity.uaa.test.SnippetUtils; +import org.cloudfoundry.identity.uaa.test.UaaTestAccounts; import org.cloudfoundry.identity.uaa.test.TestClient; import org.cloudfoundry.identity.uaa.user.UaaAuthority; import org.cloudfoundry.identity.uaa.util.JsonUtils; @@ -81,6 +83,7 @@ import static org.springframework.restdocs.payload.PayloadDocumentation.responseFields; import static org.springframework.restdocs.request.RequestDocumentation.pathParameters; import static org.springframework.restdocs.request.RequestDocumentation.requestParameters; +import static org.springframework.restdocs.snippet.Attributes.key; import static org.springframework.restdocs.templates.TemplateFormats.markdown; import static org.springframework.security.oauth2.common.util.OAuth2Utils.CLIENT_ID; import static org.springframework.security.oauth2.common.util.OAuth2Utils.GRANT_TYPE; @@ -103,6 +106,7 @@ class TokenEndpointDocs extends AbstractTokenMockMvcTests { private final ParameterDescriptor opaqueFormatParameter = parameterWithName(REQUEST_TOKEN_FORMAT).optional(null).type(STRING).description("UAA 3.3.0 Can be set to `" + OPAQUE.getStringValue() + "` to retrieve an opaque and revocable token."); private final ParameterDescriptor scopeParameter = parameterWithName(SCOPE).optional(null).type(STRING).description("The list of scopes requested for the token. Use when you wish to reduce the number of scopes the token will have."); private final ParameterDescriptor loginHintParameter = parameterWithName("login_hint").optional(null).type(STRING).description("UAA 4.19.0 Indicates the identity provider to be used. The passed string has to be a URL-Encoded JSON Object, containing the field `origin` with value as `origin_key` of an identity provider. Note that this identity provider must support the grant type `password`."); + private final ParameterDescriptor codeVerifier = parameterWithName(PkceValidationService.CODE_VERIFIER).description("UAA 4.26.0 [PKCE](https://tools.ietf.org/html/rfc7636) Code Verifier. A `code_verifier` parameter must be provided if a `code_challenge` parameter was present in the previous call to `/oauth/authorize`. The `code_verifier` must match the used `code_challenge` (according to the selected `code_challenge_method`)").attributes(key("constraints").value("Optional"), key("type").value(STRING)); private final FieldDescriptor accessTokenFieldDescriptor = fieldWithPath("access_token").description("An OAuth2 [access token](https://tools.ietf.org/html/rfc6749#section-1.4). When `token_format=opaque` is requested this value will be a random string that can only be validated using the UAA's `/check_token` or `/introspect` endpoints. When `token_format=jwt` is requested, this token will be a [JSON Web Token](https://tools.ietf.org/html/rfc7519) suitable for offline validation by OAuth2 Resource Servers."); private final FieldDescriptor idTokenFieldDescriptor = fieldWithPath("id_token").description("An OpenID Connect [ID token](http://openid.net/specs/openid-connect-core-1_0.html#IDToken). This portion of the token response is only returned when clients are configured with the scope `openid`, the `response_type` includes `id_token`, and the user has granted approval to the client for the `openid` scope."); @@ -177,6 +181,8 @@ void getTokenUsingAuthCodeGrant() throws Exception { .param(RESPONSE_TYPE, "code") .param(CLIENT_ID, "login") .param(REDIRECT_URI, redirect) + .param(PkceValidationService.CODE_CHALLENGE, UaaTestAccounts.CODE_CHALLENGE) + .param(PkceValidationService.CODE_CHALLENGE_METHOD, UaaTestAccounts.CODE_CHALLENGE_METHOD_S256) .param(STATE, new RandomValueStringGenerator().generate()); MockHttpServletResponse authCodeResponse = mockMvc.perform(getAuthCode) @@ -199,6 +205,7 @@ void getTokenUsingAuthCodeGrant() throws Exception { .param(GRANT_TYPE, GRANT_TYPE_AUTHORIZATION_CODE) .param("code", code) .param(REQUEST_TOKEN_FORMAT, OPAQUE.getStringValue()) + .param(PkceValidationService.CODE_VERIFIER, UaaTestAccounts.CODE_VERIFIER) .param(REDIRECT_URI, redirect); Snippet requestParameters = requestParameters( @@ -207,6 +214,7 @@ void getTokenUsingAuthCodeGrant() throws Exception { parameterWithName("code").description(codeDescription).attributes(SnippetUtils.constraints.value("Required"), SnippetUtils.type.value(STRING)), grantTypeParameter.description("the type of authentication being used to obtain the token, in this case `authorization_code`"), clientSecretParameter, + codeVerifier, opaqueFormatParameter ); diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/OpenIdConnectEndpointDocs.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/OpenIdConnectEndpointDocs.java index af037a35a75..c7d5db203e9 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/OpenIdConnectEndpointDocs.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/OpenIdConnectEndpointDocs.java @@ -33,6 +33,7 @@ void getWellKnownOpenidConf() throws Exception { fieldWithPath("claims_supported").description("JSON array containing a list of the Claim Names of the Claims that the OpenID Provider MAY be able to supply values for."), fieldWithPath("claims_parameter_supported").description("Boolean value specifying whether the OP supports use of the claims parameter."), fieldWithPath("service_documentation").description("URL of a page containing human-readable information that developers might want or need to know when using the OpenID Provider."), + fieldWithPath("code_challenge_methods_supported").description("UAA 4.31.0JSON array containing a list of [PKCE](https://tools.ietf.org/html/rfc7636) code challenge methods supported by this authorization endpoint."), fieldWithPath("ui_locales_supported").description("Languages and scripts supported for the user interface.") ); From ed076a6543d35f73dcb4e312fb1fdc601dad2f60 Mon Sep 17 00:00:00 2001 From: Ariel Taitz Date: Thu, 27 Jun 2019 16:01:05 +0300 Subject: [PATCH 002/140] add lombok dependency --- dependencies.gradle | 1 + model/build.gradle | 4 ++-- server/build.gradle | 6 ++++++ uaa/build.gradle | 2 ++ 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/dependencies.gradle b/dependencies.gradle index f52a847f0fb..83ebec15f22 100644 --- a/dependencies.gradle +++ b/dependencies.gradle @@ -78,6 +78,7 @@ libraries.apacheLdapApi = 'org.apache.directory.api:api-ldap-model:1.0.0' libraries.commonsLogging = 'commons-logging:commons-logging:1.2' libraries.flywayCore = 'org.flywaydb:flyway-core:5.2.4' libraries.junit = 'junit:junit:4.12' +libraries.lombok = 'org.projectlombok:lombok:1.18.8' libraries.mockito = 'org.mockito:mockito-core:2.13.0' libraries.postgresql = 'org.postgresql:postgresql:42.2.5' libraries.thymeleafDialect = 'nz.net.ultraq.thymeleaf:thymeleaf-layout-dialect:2.4.1' diff --git a/model/build.gradle b/model/build.gradle index 6b6afd77dda..f8d6faebcd3 100644 --- a/model/build.gradle +++ b/model/build.gradle @@ -23,8 +23,8 @@ dependencies { exclude(module: 'hamcrest-core') } - compileOnly 'org.projectlombok:lombok:1.18.8' - annotationProcessor 'org.projectlombok:lombok:1.18.6' + compileOnly libraries.lombok + annotationProcessor libraries.lombok testCompile 'org.skyscreamer:jsonassert:1.5.0' } diff --git a/server/build.gradle b/server/build.gradle index 859e29361d9..e6a307d7494 100644 --- a/server/build.gradle +++ b/server/build.gradle @@ -74,6 +74,9 @@ dependencies { compile group: 'org.apache.santuario', name: 'xmlsec', version: '2.1.2' + compileOnly libraries.lombok + annotationProcessor libraries.lombok + testCompile project(':cloudfoundry-identity-model').sourceSets.test.output testCompile libraries.springTest @@ -95,6 +98,9 @@ dependencies { testCompile libraries.tomcatJdbc testCompile libraries.jsonPathAssert + + testCompileOnly libraries.lombok + testAnnotationProcessor libraries.lombok } configurations.all { diff --git a/uaa/build.gradle b/uaa/build.gradle index 4a28eb6c322..5d2acc0e43a 100644 --- a/uaa/build.gradle +++ b/uaa/build.gradle @@ -60,6 +60,8 @@ dependencies { testCompile libraries.mockito testCompile libraries.tomcatJdbc testCompile 'org.springframework.restdocs:spring-restdocs-mockmvc:2.0.3.RELEASE' + testCompileOnly libraries.lombok + testAnnotationProcessor libraries.lombok } node { From 1f333a424ded663ac456a749fcf63ecab582e21f Mon Sep 17 00:00:00 2001 From: Ariel Taitz Date: Thu, 27 Jun 2019 16:30:26 +0300 Subject: [PATCH 003/140] unit tests for redirect_uri path integrity check bypass --- .../beans/LegacyRedirectResolverTest.java | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java index 536bfe2b828..fd300515ba7 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java @@ -13,6 +13,8 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.springframework.security.oauth2.common.exceptions.RedirectMismatchException; import org.springframework.security.oauth2.provider.ClientDetails; import org.springframework.security.oauth2.provider.client.BaseClientDetails; @@ -649,6 +651,37 @@ void withWildCardHostCaps() { } } + @Nested + @DisplayName("integrity check bypass") + class IntegrityCheckBypass { + + private static final String REGISTERED_REDIRECT_URI = "http://example.com/foo"; + + @ParameterizedTest(name = "{index} " + REGISTERED_REDIRECT_URI + "{0} shoud not match") + @ValueSource(strings = { + "/../bar", + "/%2e./bar", //%2e is . url encoded + "/%252e./bar", //%25 is % url encoded + "/%2525252e./bar", //path may be url decoded multiple times when passing through web servers, proxies and browser + "/%25252525252525252525252e./bar", + }) + void doubleDotTraversal(String suffix) { + assertFalse(resolver.redirectMatches(REGISTERED_REDIRECT_URI + suffix, REGISTERED_REDIRECT_URI)); + } + + @ParameterizedTest(name = "{index} " + REGISTERED_REDIRECT_URI + "{0} shoud match") + @ValueSource(strings = { + "/./bar", + "/%2e/bar", + "/%252e/bar", + "/%2525252e/bar", + }) + void singleDotTraversal(String suffix) { + assertTrue(resolver.redirectMatches(REGISTERED_REDIRECT_URI + suffix, REGISTERED_REDIRECT_URI)); + } + + } + @Nested class ResolveRedirect { private ClientDetails mockClientDetails; From d806456af43a6ecd65a66a3cacea81b81ec53a83 Mon Sep 17 00:00:00 2001 From: Ariel Taitz Date: Thu, 11 Jul 2019 14:39:43 +0300 Subject: [PATCH 004/140] redirect resolver tests should cover path traversal bypass for both registered paths containing wildcards and those that do not --- .../beans/LegacyRedirectResolverTest.java | 67 +++++++++++++------ 1 file changed, 47 insertions(+), 20 deletions(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java index fd300515ba7..10ca97ec836 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolverTest.java @@ -14,7 +14,8 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.springframework.security.oauth2.common.exceptions.RedirectMismatchException; import org.springframework.security.oauth2.provider.ClientDetails; import org.springframework.security.oauth2.provider.client.BaseClientDetails; @@ -24,6 +25,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.stream.Stream; import static org.apache.logging.log4j.Level.WARN; import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.GRANT_TYPE_AUTHORIZATION_CODE; @@ -651,33 +653,58 @@ void withWildCardHostCaps() { } } - @Nested - @DisplayName("integrity check bypass") - class IntegrityCheckBypass { - - private static final String REGISTERED_REDIRECT_URI = "http://example.com/foo"; - - @ParameterizedTest(name = "{index} " + REGISTERED_REDIRECT_URI + "{0} shoud not match") - @ValueSource(strings = { + private static Stream doubleDotTraversalArguments() { + Iterable requestedSuffix = Arrays.asList( "/../bar", "/%2e./bar", //%2e is . url encoded "/%252e./bar", //%25 is % url encoded "/%2525252e./bar", //path may be url decoded multiple times when passing through web servers, proxies and browser - "/%25252525252525252525252e./bar", - }) - void doubleDotTraversal(String suffix) { - assertFalse(resolver.redirectMatches(REGISTERED_REDIRECT_URI + suffix, REGISTERED_REDIRECT_URI)); - } + "/%25252525252525252525252e./bar" + ); + return generateDotTraversalArguments(requestedSuffix); + } - @ParameterizedTest(name = "{index} " + REGISTERED_REDIRECT_URI + "{0} shoud match") - @ValueSource(strings = { + private static Stream singleDotTraversalArguments() { + Iterable requestedSuffix = Arrays.asList( "/./bar", "/%2e/bar", "/%252e/bar", - "/%2525252e/bar", - }) - void singleDotTraversal(String suffix) { - assertTrue(resolver.redirectMatches(REGISTERED_REDIRECT_URI + suffix, REGISTERED_REDIRECT_URI)); + "/%2525252e/bar" + ); + return generateDotTraversalArguments(requestedSuffix); + } + + private static Stream generateDotTraversalArguments(Iterable requestedSuffix) { + //registered redirect uri that contains a wildcard (*) is matched using an Ant path matcher + //registered redirect uri that lacks a wildcard is matched using a different path matcher + //hence both cases must be verified for ability to withstand integrity check bypass + Iterable registeredSuffix = Arrays.asList( + "", + "/**" + ); + Stream.Builder builder = Stream.builder(); + requestedSuffix.forEach( + req -> registeredSuffix.forEach(reg -> builder.accept(Arguments.of(req, reg))) + ); + return builder.build(); + } + + @Nested + @DisplayName("integrity check bypass") + class IntegrityCheckBypass { + + private static final String REGISTERED_REDIRECT_URI = "http://example.com/foo"; + + @ParameterizedTest(name = "{index} " + REGISTERED_REDIRECT_URI + "{0} shoud not match " + REGISTERED_REDIRECT_URI + "{1}") + @MethodSource("org.cloudfoundry.identity.uaa.oauth.beans.LegacyRedirectResolverTest#doubleDotTraversalArguments") + void doubleDotTraversal(String requestedSuffix, String registeredSuffix) { + assertFalse(resolver.redirectMatches(REGISTERED_REDIRECT_URI + requestedSuffix, REGISTERED_REDIRECT_URI + registeredSuffix)); + } + + @ParameterizedTest(name = "{index} " + REGISTERED_REDIRECT_URI + "{0} shoud match " + REGISTERED_REDIRECT_URI + "{1}") + @MethodSource("org.cloudfoundry.identity.uaa.oauth.beans.LegacyRedirectResolverTest#singleDotTraversalArguments") + void singleDotTraversal(String requestedSuffix, String registeredSuffix) { + assertTrue(resolver.redirectMatches(REGISTERED_REDIRECT_URI + requestedSuffix, REGISTERED_REDIRECT_URI + registeredSuffix)); } } From 1100c3f6f241f76f73d97e5a19a840c35703666f Mon Sep 17 00:00:00 2001 From: Ariel Taitz Date: Tue, 11 Jun 2019 18:45:33 +0300 Subject: [PATCH 005/140] fix double dot path traversal integrity check bypass --- .../oauth/beans/LegacyRedirectResolver.java | 61 ++++++++++++++----- 1 file changed, 45 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java index 8424ae387e9..32d0338ef59 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java @@ -1,5 +1,6 @@ package org.cloudfoundry.identity.uaa.oauth.beans; +import lombok.SneakyThrows; import org.apache.commons.lang.ArrayUtils; import org.cloudfoundry.identity.uaa.util.UaaUrlUtils; import org.slf4j.Logger; @@ -10,13 +11,17 @@ import org.springframework.util.AntPathMatcher; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; +import org.springframework.util.StringUtils; import org.springframework.web.util.UriComponentsBuilder; import java.net.URI; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; import java.util.AbstractMap.SimpleEntry; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Predicate; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -42,21 +47,19 @@ protected boolean redirectMatches(String requestedRedirect, String clientRedirec String normalizedRequestedRedirect = normalizeUri(requestedRedirect); String normalizedClientRedirect = normalizeUri(clientRedirect); - URI requestedRedirectURI = URI.create(normalizedRequestedRedirect); ClientRedirectUriPattern clientRedirectUri = new ClientRedirectUriPattern(normalizedClientRedirect); if (!clientRedirectUri.isValidRedirect()) { logger.error(String.format("Invalid redirect uri: %s", normalizedClientRedirect)); return false; } - - if (clientRedirectUri.isWildcard(normalizedClientRedirect) && - clientRedirectUri.isSafeRedirect(requestedRedirectURI) && - clientRedirectUri.match(requestedRedirectURI)) { - return true; + Predicate matcher; + if (isWildcard(normalizedClientRedirect)) { + matcher = req -> clientRedirectUri.isSafeRedirect(req) && clientRedirectUri.match(req); + } else { + matcher = req -> super.redirectMatches(req, normalizedClientRedirect); } - - return super.redirectMatches(normalizedRequestedRedirect, normalizedClientRedirect); + return matches(matcher, normalizedRequestedRedirect); } catch (IllegalArgumentException e) { logger.error( String.format("Could not validate whether requestedRedirect (%s) matches clientRedirectUri (%s)", @@ -67,6 +70,31 @@ protected boolean redirectMatches(String requestedRedirect, String clientRedirec } } + //todo: logging? + private boolean matches(Predicate matcher, String requestedRedirect) { //todo: name + int hadEnough = 5; //todo + for (int i = 1; i <= hadEnough; i++) { + if (!matcher.test(requestedRedirect)) { + return false; + } + String decoded = urlDecodeAndNormalize(requestedRedirect); //todo: optimize by only normalizing at end? + if (decoded.equals(requestedRedirect)) { //todo: move to for loop? + return true; + } + requestedRedirect = decoded; + } + return false; + } + + private String urlDecodeAndNormalize(String url) { + return StringUtils.cleanPath(urlDecode(url)); + } + + @SneakyThrows + private String urlDecode(String url) { + return URLDecoder.decode(url, StandardCharsets.UTF_8.name()); + } + @Override public String resolveRedirect(String requestedRedirect, ClientDetails client) throws OAuth2Exception { Set registeredRedirectUris = ofNullable(client.getRegisteredRedirectUri()).orElse(emptySet()); @@ -140,6 +168,11 @@ private static void redactHashFragment(UriComponentsBuilder builder) { } } + private static boolean isWildcard(String configuredRedirectPattern) { + return configuredRedirectPattern.contains("*"); + } + + private class SpecCompliantRedirectMatcher { private final CurrentVersionOfSpringResolverWithMethodExposedAndSubdomainsOff matcher = new CurrentVersionOfSpringResolverWithMethodExposedAndSubdomainsOff(); @@ -186,10 +219,10 @@ private static class ClientRedirectUriPattern { } } - boolean isSafeRedirect(URI requestedRedirect) { + boolean isSafeRedirect(String requestedRedirect) { // We iterate backwards through the hosts to make sure the TLD and domain match String[] configuredRedirectHost = splitAndReverseHost(getHost()); - String[] requestedRedirectHost = splitAndReverseHost(requestedRedirect.getHost()); + String[] requestedRedirectHost = splitAndReverseHost(URI.create(requestedRedirect).getHost()); if (requestedRedirectHost.length < configuredRedirectHost.length) { return false; @@ -207,12 +240,8 @@ boolean isValidRedirect() { return isValidRedirect; } - boolean match(URI requestedRedirect) { - return matcher.match(redirectUri, requestedRedirect.toString()); - } - - private boolean isWildcard(String configuredRedirectPattern) { - return configuredRedirectPattern.contains("*"); + boolean match(String requestedRedirect) { + return matcher.match(redirectUri, requestedRedirect); } private String getHost() { From 6300b3972e1f39098a6e7b9788a2825355678136 Mon Sep 17 00:00:00 2001 From: Ariel Taitz Date: Wed, 12 Jun 2019 14:38:44 +0300 Subject: [PATCH 006/140] javadoc explaining integrity bypass fix --- .../oauth/beans/LegacyRedirectResolver.java | 35 +++++++++++++------ 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java index 32d0338ef59..8683528c866 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java @@ -70,26 +70,39 @@ protected boolean redirectMatches(String requestedRedirect, String clientRedirec } } - //todo: logging? - private boolean matches(Predicate matcher, String requestedRedirect) { //todo: name - int hadEnough = 5; //todo - for (int i = 1; i <= hadEnough; i++) { + /** + * Repeatedly: + *
    + *
  1. checks for a match
    2. + *
  2. url decodes the requested path
    3. + *
+ * until path cannot be url decoded any further. Then normalizes the path before the final check. + *

+ * For example, if example.com/foo is the registered url and example.com/foo/%252e./bar is the requested url, + * checks a match for: + *

    + *
  1. example.com/foo/%252e./bar
    2. + *
  2. example.com/foo/%2e./bar
    3. + *
  3. example.com/foo/../bar
    4. + *
  4. example.com/bar
    5. + *
+ *

+ */ + private boolean matches(Predicate matcher, String requestedRedirect) { + for (int i = 1; i <= 5; i++) { if (!matcher.test(requestedRedirect)) { return false; } - String decoded = urlDecodeAndNormalize(requestedRedirect); //todo: optimize by only normalizing at end? - if (decoded.equals(requestedRedirect)) { //todo: move to for loop? - return true; + String decoded = urlDecode(requestedRedirect); + if (decoded.equals(requestedRedirect)) { + return matcher.test(StringUtils.cleanPath(decoded)); } requestedRedirect = decoded; } + logger.debug("aborted url decoding loop to mitigate DOS attack that sends a repeatedly url-encoded path"); return false; } - private String urlDecodeAndNormalize(String url) { - return StringUtils.cleanPath(urlDecode(url)); - } - @SneakyThrows private String urlDecode(String url) { return URLDecoder.decode(url, StandardCharsets.UTF_8.name()); From 33688a76204aaf63ff7986f9c887bba82070f240 Mon Sep 17 00:00:00 2001 From: Ariel Taitz Date: Thu, 27 Jun 2019 17:10:26 +0300 Subject: [PATCH 007/140] meaningful name for number of url-decode attempts --- .../identity/uaa/oauth/beans/LegacyRedirectResolver.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java index 8683528c866..d2736ffc432 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java @@ -89,7 +89,8 @@ protected boolean redirectMatches(String requestedRedirect, String clientRedirec *

*/ private boolean matches(Predicate matcher, String requestedRedirect) { - for (int i = 1; i <= 5; i++) { + int maxDecodeAttempts = 5; + for (int i = 1; i <= maxDecodeAttempts; i++) { if (!matcher.test(requestedRedirect)) { return false; } From 3132bf184f0620aa0d8fa51faf48ffe0282dbd59 Mon Sep 17 00:00:00 2001 From: Ariel Taitz Date: Thu, 27 Jun 2019 17:13:36 +0300 Subject: [PATCH 008/140] capitilize first letter of log sentence --- .../identity/uaa/oauth/beans/LegacyRedirectResolver.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java index d2736ffc432..818784dcda9 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java @@ -100,7 +100,7 @@ private boolean matches(Predicate matcher, String requestedRedirect) { } requestedRedirect = decoded; } - logger.debug("aborted url decoding loop to mitigate DOS attack that sends a repeatedly url-encoded path"); + logger.debug("Aborted url decoding loop to mitigate DOS attack that sends a repeatedly url-encoded path"); return false; } From af733108498a964c21d289ccdf0360e95bc532c1 Mon Sep 17 00:00:00 2001 From: Ariel Taitz Date: Thu, 27 Jun 2019 17:30:58 +0300 Subject: [PATCH 009/140] better name for matches function --- .../identity/uaa/oauth/beans/LegacyRedirectResolver.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java index 818784dcda9..3d6e544bac6 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java @@ -59,7 +59,7 @@ protected boolean redirectMatches(String requestedRedirect, String clientRedirec } else { matcher = req -> super.redirectMatches(req, normalizedClientRedirect); } - return matches(matcher, normalizedRequestedRedirect); + return matchesAfterNormalization(matcher, normalizedRequestedRedirect); } catch (IllegalArgumentException e) { logger.error( String.format("Could not validate whether requestedRedirect (%s) matches clientRedirectUri (%s)", @@ -88,7 +88,7 @@ protected boolean redirectMatches(String requestedRedirect, String clientRedirec * *

*/ - private boolean matches(Predicate matcher, String requestedRedirect) { + private boolean matchesAfterNormalization(Predicate matcher, String requestedRedirect) { int maxDecodeAttempts = 5; for (int i = 1; i <= maxDecodeAttempts; i++) { if (!matcher.test(requestedRedirect)) { From 0775178f42ed0e9653346e1fd67b698af8cd383c Mon Sep 17 00:00:00 2001 From: Ariel Taitz Date: Thu, 11 Jul 2019 16:34:27 +0300 Subject: [PATCH 010/140] Added "final" modifier for method-local constant --- .../identity/uaa/oauth/beans/LegacyRedirectResolver.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java index 3d6e544bac6..ebfcd6a845a 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/beans/LegacyRedirectResolver.java @@ -89,7 +89,7 @@ protected boolean redirectMatches(String requestedRedirect, String clientRedirec *

*/ private boolean matchesAfterNormalization(Predicate matcher, String requestedRedirect) { - int maxDecodeAttempts = 5; + final int maxDecodeAttempts = 5; for (int i = 1; i <= maxDecodeAttempts; i++) { if (!matcher.test(requestedRedirect)) { return false; From e8a845883d1e3e311c377a6ea7861c3d8f8247c7 Mon Sep 17 00:00:00 2001 From: Cloud Foundry Identity Team Date: Thu, 28 Jan 2021 01:44:05 +0000 Subject: [PATCH 011/140] Update UAA image reference in k8s deployment template to cloudfoundry/uaa@sha256:37fcf63a156b75174fbc7be0e3809d64cda6851de0bfe6fa7f12793d919de96a --- k8s/templates/values/image.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/k8s/templates/values/image.yml b/k8s/templates/values/image.yml index 756a85434ee..54e35b92c5b 100644 --- a/k8s/templates/values/image.yml +++ b/k8s/templates/values/image.yml @@ -1,3 +1,3 @@ #@data/values --- -image: "cloudfoundry/uaa@sha256:7b76095c9848216b4871b7120fe5366ab560d20635027c17833267d5e425197f" +image: "cloudfoundry/uaa@sha256:37fcf63a156b75174fbc7be0e3809d64cda6851de0bfe6fa7f12793d919de96a" From 333954df0df9327cea2a75c906a63aa5fea3544e Mon Sep 17 00:00:00 2001 From: Florian Tack Date: Tue, 10 Nov 2020 16:42:22 +0100 Subject: [PATCH 012/140] Restructure login method to not read all IdentityProviders on login_hint --- .../identity/uaa/login/LoginInfoEndpoint.java | 70 ++++++++++++------- .../uaa/login/LoginInfoEndpointTests.java | 24 +++++-- 2 files changed, 65 insertions(+), 29 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java b/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java index 4a8042215b0..aea0b5682f2 100755 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java @@ -74,6 +74,7 @@ import java.sql.Timestamp; import java.text.SimpleDateFormat; import java.time.Duration; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Date; @@ -273,15 +274,31 @@ private String login(Model model, Principal principal, List excludedProm clientName = (String) clientInfo.get(ClientConstants.CLIENT_NAME); } - Map samlIdentityProviders = - getSamlIdentityProviderDefinitions(allowedIdentityProviderKeys); - Map oauthIdentityProviders = - getOauthIdentityProviderDefinitions(allowedIdentityProviderKeys); - Map allIdentityProviders = - new HashMap<>() {{ - putAll(samlIdentityProviders); - putAll(oauthIdentityProviders); - }}; + Map samlIdentityProviders; + Map oauthIdentityProviders; + Map allIdentityProviders = Collections.emptyMap(); + Map loginHintProviders = Collections.emptyMap(); + + String loginHintParam = extractLoginHintParam(session, request); + UaaLoginHint uaaLoginHint = UaaLoginHint.parseRequestParameter(loginHintParam); + if (uaaLoginHint != null && (allowedIdentityProviderKeys == null || allowedIdentityProviderKeys.contains(uaaLoginHint.getOrigin()))) { + if (!(OriginKeys.UAA.equals(uaaLoginHint.getOrigin()) || OriginKeys.LDAP.equals(uaaLoginHint.getOrigin()))) { + try { + IdentityProvider loginHintProvider = externalOAuthProviderConfigurator + .retrieveByOrigin(uaaLoginHint.getOrigin(), IdentityZoneHolder.get().getId()); + loginHintProviders = Collections.singletonList(loginHintProvider).stream().collect( + new MapCollector( + IdentityProvider::getOriginKey, IdentityProvider::getConfig)); + } catch (EmptyResultDataAccessException ignored) { + } + } + oauthIdentityProviders = Collections.emptyMap(); + samlIdentityProviders = Collections.emptyMap(); + } else { + samlIdentityProviders = getSamlIdentityProviderDefinitions(allowedIdentityProviderKeys); + oauthIdentityProviders = getOauthIdentityProviderDefinitions(allowedIdentityProviderKeys); + allIdentityProviders = new HashMap<>() {{putAll(samlIdentityProviders);putAll(oauthIdentityProviders);}}; + } boolean fieldUsernameShow = true; boolean returnLoginPrompts = true; @@ -311,8 +328,8 @@ private String login(Model model, Principal principal, List excludedProm } Map.Entry idpForRedirect; - idpForRedirect = evaluateLoginHint(model, session, samlIdentityProviders, - oauthIdentityProviders, allIdentityProviders, allowedIdentityProviderKeys, request); + idpForRedirect = evaluateLoginHint(model, samlIdentityProviders, + oauthIdentityProviders, allIdentityProviders, allowedIdentityProviderKeys, loginHintParam, uaaLoginHint, loginHintProviders); boolean discoveryEnabled = IdentityZoneHolder.get().getConfig().isIdpDiscoveryEnabled(); boolean discoveryPerformed = Boolean.parseBoolean(request.getParameter("discoveryPerformed")); @@ -480,27 +497,29 @@ private Map.Entry evaluateIdpDiscove return idpForRedirect; } + private String extractLoginHintParam(HttpSession session, HttpServletRequest request) { + String loginHintParam = + ofNullable(session) + .flatMap(s -> ofNullable(SessionUtils.getSavedRequestSession(s))) + .flatMap(sr -> ofNullable(sr.getParameterValues("login_hint"))) + .flatMap(lhValues -> Arrays.stream(lhValues).findFirst()) + .orElse(request.getParameter("login_hint")); + return loginHintParam; + } + private Map.Entry evaluateLoginHint( Model model, - HttpSession session, Map samlIdentityProviders, Map oauthIdentityProviders, Map allIdentityProviders, List allowedIdentityProviderKeys, - HttpServletRequest request + String loginHintParam, + UaaLoginHint uaaLoginHint, + Map loginHintProviders ) { - Map.Entry idpForRedirect = null; - String loginHintParam = - ofNullable(session) - .flatMap(s -> ofNullable(SessionUtils.getSavedRequestSession(s))) - .flatMap(sr -> ofNullable(sr.getParameterValues("login_hint"))) - .flatMap(lhValues -> Arrays.stream(lhValues).findFirst()) - .orElse(request.getParameter("login_hint")); - if (loginHintParam != null) { // parse login_hint in JSON format - UaaLoginHint uaaLoginHint = UaaLoginHint.parseRequestParameter(loginHintParam); if (uaaLoginHint != null) { logger.debug("Received login hint: " + loginHintParam); logger.debug("Received login hint with origin: " + uaaLoginHint.getOrigin()); @@ -519,12 +538,13 @@ private Map.Entry evaluateLoginHint( allIdentityProviders.entrySet().stream().filter( idp -> idp.getKey().equals(uaaLoginHint.getOrigin()) ).collect(Collectors.toList()); - if (hintIdentityProviders.size() > 1) { + if (loginHintProviders.size() > 1) { throw new IllegalStateException( "There is a misconfiguration with the identity provider(s). Please contact your system administrator." ); - } else if (hintIdentityProviders.size() == 1) { - idpForRedirect = hintIdentityProviders.get(0); + } + if (loginHintProviders.size() == 1) { + idpForRedirect = new ArrayList<>(loginHintProviders.entrySet()).get(0); logger.debug("Setting redirect from origin login_hint to: " + idpForRedirect); } else { logger.debug("Client does not allow provider for login_hint with origin key: " diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java index 9e1dbb0d570..9f6f0110237 100755 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java @@ -36,6 +36,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.springframework.dao.DataAccessException; +import org.springframework.dao.EmptyResultDataAccessException; import org.springframework.http.MediaType; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; @@ -1163,7 +1164,7 @@ void loginHintOriginOidc() throws Exception { MultitenantClientServices clientDetailsService = mockClientService(); - mockOidcProvider(mockIdentityProviderProvisioning); + mockLoginHintProvider(configurator); LoginInfoEndpoint endpoint = getEndpoint(IdentityZoneHolder.get(), clientDetailsService); @@ -1184,7 +1185,7 @@ void loginHintOriginOidcForJson() throws Exception { MultitenantClientServices clientDetailsService = mockClientService(); - mockOidcProvider(mockIdentityProviderProvisioning); + mockLoginHintProvider(configurator); LoginInfoEndpoint endpoint = getEndpoint(IdentityZoneHolder.get(), clientDetailsService); @@ -1197,7 +1198,7 @@ void loginHintOriginOidcForJson() throws Exception { assertNotNull(extendedModelMap.get("prompts")); assertTrue(extendedModelMap.get("prompts") instanceof Map); Map returnedPrompts = (Map) extendedModelMap.get("prompts"); - assertEquals(3, returnedPrompts.size()); + assertEquals(2, returnedPrompts.size()); } @Test @@ -1210,6 +1211,7 @@ void loginHintOriginInvalid() throws Exception { SavedRequest savedRequest = SessionUtils.getSavedRequestSession(mockHttpServletRequest.getSession()); when(savedRequest.getParameterValues("login_hint")).thenReturn(new String[]{"{\"origin\":\"my-OIDC-idp1\"}"}); + when(configurator.retrieveByOrigin(eq("my-OIDC-idp1"), anyString())).thenThrow(new EmptyResultDataAccessException(0)); endpoint.loginForHtml(extendedModelMap, null, mockHttpServletRequest, singletonList(MediaType.TEXT_HTML)); @@ -1405,7 +1407,7 @@ void loginHintOverridesDefaultProvider() throws Exception { MultitenantClientServices clientDetailsService = mockClientService(); - mockOidcProvider(mockIdentityProviderProvisioning); + mockLoginHintProvider(configurator); LoginInfoEndpoint endpoint = getEndpoint(IdentityZoneHolder.get(), clientDetailsService); @@ -1656,4 +1658,18 @@ private static void mockOidcProvider(IdentityProviderProvisioning mockIdentityPr when(mockOidcConfig.isShowLinkText()).thenReturn(true); when(mockIdentityProviderProvisioning.retrieveAll(anyBoolean(), any())).thenReturn(singletonList(mockProvider)); } + + private static void mockLoginHintProvider(ExternalOAuthProviderConfigurator mockIdentityProviderProvisioning) + throws MalformedURLException { + IdentityProvider mockProvider = mock(IdentityProvider.class); + when(mockProvider.getOriginKey()).thenReturn("my-OIDC-idp1"); + when(mockProvider.getType()).thenReturn(OriginKeys.OIDC10); + AbstractExternalOAuthIdentityProviderDefinition mockOidcConfig = mock(OIDCIdentityProviderDefinition.class); + when(mockOidcConfig.getAuthUrl()).thenReturn(new URL("http://localhost:8080/uaa")); + when(mockOidcConfig.getRelyingPartyId()).thenReturn("client-id"); + when(mockOidcConfig.getResponseType()).thenReturn("token"); + when(mockProvider.getConfig()).thenReturn(mockOidcConfig); + when(mockOidcConfig.isShowLinkText()).thenReturn(true); + when(mockIdentityProviderProvisioning.retrieveByOrigin(eq("my-OIDC-idp1"), any())).thenReturn(mockProvider); + } } From 0e31bb4cce3399d242b477dd2a9d992e2bcfc407 Mon Sep 17 00:00:00 2001 From: Florian Tack Date: Thu, 12 Nov 2020 11:29:25 +0100 Subject: [PATCH 013/140] Move read configurations and parameters to allow earlier decisions --- .../identity/uaa/login/LoginInfoEndpoint.java | 36 ++++++++++--------- .../uaa/login/LoginInfoEndpointTests.java | 7 ---- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java b/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java index aea0b5682f2..d9b3c8a276e 100755 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java @@ -274,14 +274,25 @@ private String login(Model model, Principal principal, List excludedProm clientName = (String) clientInfo.get(ClientConstants.CLIENT_NAME); } + //Read all configuration and parameters at the beginning to allow earlier decisions + boolean discoveryEnabled = IdentityZoneHolder.get().getConfig().isIdpDiscoveryEnabled(); + boolean discoveryPerformed = Boolean.parseBoolean(request.getParameter("discoveryPerformed")); + String defaultIdentityProviderName = IdentityZoneHolder.get().getConfig().getDefaultIdentityProvider(); + boolean accountChooserEnabled = IdentityZoneHolder.get().getConfig().isAccountChooserEnabled(); + boolean otherAccountSignIn = Boolean.parseBoolean(request.getParameter("otherAccountSignIn")); + boolean savedAccountsEmpty = getSavedAccounts(request.getCookies(), SavedAccountOption.class).isEmpty(); + boolean accountChooserNeeded = accountChooserEnabled && !(otherAccountSignIn || savedAccountsEmpty) && discoveryEnabled &&!discoveryPerformed; + + String loginHintParam = extractLoginHintParam(session, request); + UaaLoginHint uaaLoginHint = UaaLoginHint.parseRequestParameter(loginHintParam); + Map samlIdentityProviders; Map oauthIdentityProviders; Map allIdentityProviders = Collections.emptyMap(); Map loginHintProviders = Collections.emptyMap(); - String loginHintParam = extractLoginHintParam(session, request); - UaaLoginHint uaaLoginHint = UaaLoginHint.parseRequestParameter(loginHintParam); if (uaaLoginHint != null && (allowedIdentityProviderKeys == null || allowedIdentityProviderKeys.contains(uaaLoginHint.getOrigin()))) { + // Login hint: Only try to read the hinted IdP from database if (!(OriginKeys.UAA.equals(uaaLoginHint.getOrigin()) || OriginKeys.LDAP.equals(uaaLoginHint.getOrigin()))) { try { IdentityProvider loginHintProvider = externalOAuthProviderConfigurator @@ -294,6 +305,10 @@ private String login(Model model, Principal principal, List excludedProm } oauthIdentityProviders = Collections.emptyMap(); samlIdentityProviders = Collections.emptyMap(); + } else if (accountChooserNeeded || (discoveryEnabled && !discoveryPerformed)) { + //Account Chooser and discovery do not need any IdP information + oauthIdentityProviders = Collections.emptyMap(); + samlIdentityProviders = Collections.emptyMap(); } else { samlIdentityProviders = getSamlIdentityProviderDefinitions(allowedIdentityProviderKeys); oauthIdentityProviders = getOauthIdentityProviderDefinitions(allowedIdentityProviderKeys); @@ -331,10 +346,6 @@ private String login(Model model, Principal principal, List excludedProm idpForRedirect = evaluateLoginHint(model, samlIdentityProviders, oauthIdentityProviders, allIdentityProviders, allowedIdentityProviderKeys, loginHintParam, uaaLoginHint, loginHintProviders); - boolean discoveryEnabled = IdentityZoneHolder.get().getConfig().isIdpDiscoveryEnabled(); - boolean discoveryPerformed = Boolean.parseBoolean(request.getParameter("discoveryPerformed")); - String defaultIdentityProviderName = IdentityZoneHolder.get().getConfig().getDefaultIdentityProvider(); - idpForRedirect = evaluateIdpDiscovery(model, samlIdentityProviders, oauthIdentityProviders, allIdentityProviders, allowedIdentityProviderKeys, idpForRedirect, discoveryEnabled, discoveryPerformed, defaultIdentityProviderName); if (idpForRedirect == null && !jsonResponse && !fieldUsernameShow && allIdentityProviders.size() == 1) { @@ -381,7 +392,7 @@ private String login(Model model, Principal principal, List excludedProm excludedPrompts, returnLoginPrompts); if (principal == null) { - return getUnauthenticatedRedirect(model, request, discoveryEnabled, discoveryPerformed); + return getUnauthenticatedRedirect(model, request, discoveryEnabled, discoveryPerformed, accountChooserNeeded); } return "home"; } @@ -390,25 +401,18 @@ private String getUnauthenticatedRedirect( Model model, HttpServletRequest request, boolean discoveryEnabled, - boolean discoveryPerformed + boolean discoveryPerformed, + boolean accountChooserNeeded ) { String formRedirectUri = request.getParameter(UaaSavedRequestAwareAuthenticationSuccessHandler.FORM_REDIRECT_PARAMETER); if (hasText(formRedirectUri)) { model.addAttribute(UaaSavedRequestAwareAuthenticationSuccessHandler.FORM_REDIRECT_PARAMETER, formRedirectUri); } - boolean accountChooserEnabled = IdentityZoneHolder.get().getConfig().isAccountChooserEnabled(); - boolean otherAccountSignIn = Boolean.parseBoolean(request.getParameter("otherAccountSignIn")); - boolean savedAccountsEmpty = getSavedAccounts(request.getCookies(), SavedAccountOption.class).isEmpty(); - if (discoveryEnabled) { if (model.containsAttribute("login_hint")) { return goToPasswordPage(request.getParameter("email"), model); } - boolean accountChooserNeeded = accountChooserEnabled - && !(otherAccountSignIn || savedAccountsEmpty) - && !discoveryPerformed; - if (accountChooserNeeded) { return "idp_discovery/account_chooser"; } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java index 9f6f0110237..d498b30de69 100755 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java @@ -818,13 +818,6 @@ void authcodeWithAllowedProviderStillUsesAccountChooser() throws Exception { MultitenantClientServices clientDetailsService = mock(MultitenantClientServices.class); when(clientDetailsService.loadClientByClientId("client-id", "other-zone")).thenReturn(clientDetails); - // mock SamlIdentityProviderConfigurator - List clientIDPs = new LinkedList<>(); - clientIDPs.add(createIdentityProviderDefinition("my-client-awesome-idp1", "other-zone")); - clientIDPs.add(createIdentityProviderDefinition("uaa", "other-zone")); - when(mockSamlIdentityProviderConfigurator.getIdentityProviderDefinitions(eq(allowedProviders), eq(zone))).thenReturn(clientIDPs); - - LoginInfoEndpoint endpoint = getEndpoint(IdentityZoneHolder.get(), clientDetailsService); endpoint.loginForHtml(extendedModelMap, null, request, singletonList(MediaType.TEXT_HTML)); From 7e7c376cd4bc5180eb78b43bcc33b21b069bb8aa Mon Sep 17 00:00:00 2001 From: Florian Tack Date: Fri, 29 Jan 2021 13:18:49 +0100 Subject: [PATCH 014/140] Fix bug with invalid login_hint on login page --- .../identity/uaa/login/LoginInfoEndpoint.java | 12 ++++++++-- .../uaa/login/LoginInfoEndpointTests.java | 24 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java b/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java index d9b3c8a276e..9776b363adf 100755 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java @@ -303,8 +303,16 @@ private String login(Model model, Principal principal, List excludedProm } catch (EmptyResultDataAccessException ignored) { } } - oauthIdentityProviders = Collections.emptyMap(); - samlIdentityProviders = Collections.emptyMap(); + if (!loginHintProviders.isEmpty()) { + oauthIdentityProviders = Collections.emptyMap(); + samlIdentityProviders = Collections.emptyMap(); + } else { + samlIdentityProviders = getSamlIdentityProviderDefinitions(allowedIdentityProviderKeys); + oauthIdentityProviders = getOauthIdentityProviderDefinitions(allowedIdentityProviderKeys); + allIdentityProviders = new HashMap<>(); + allIdentityProviders.putAll(samlIdentityProviders); + allIdentityProviders.putAll(oauthIdentityProviders); + } } else if (accountChooserNeeded || (discoveryEnabled && !discoveryPerformed)) { //Account Chooser and discovery do not need any IdP information oauthIdentityProviders = Collections.emptyMap(); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java index d498b30de69..e410ed4aa9e 100755 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java @@ -1151,6 +1151,30 @@ void invalidLoginHintErrorOnDiscoveryPage() throws Exception { assertEquals("idp_discovery/email", redirect); } + @Test + public void testInvalidLoginHintLoginPageReturnsList() throws Exception { + MockHttpServletRequest mockHttpServletRequest = getMockHttpServletRequest(); + + BaseClientDetails clientDetails = new BaseClientDetails(); + clientDetails.setClientId("client-id"); + MultitenantClientServices clientDetailsService = mock(MultitenantClientServices.class); + when(clientDetailsService.loadClientByClientId("client-id", "uaa")).thenReturn(clientDetails); + LoginInfoEndpoint endpoint = getEndpoint(IdentityZoneHolder.get(), clientDetailsService); + + List clientAllowedIdps = new LinkedList<>(); + clientAllowedIdps.add(createOIDCIdentityProvider("my-OIDC-idp1")); + clientAllowedIdps.add(createOIDCIdentityProvider("my-OIDC-idp2")); + when(mockIdentityProviderProvisioning.retrieveAll(eq(true), anyString())).thenReturn(clientAllowedIdps); + when(mockIdentityProviderProvisioning.retrieveByOrigin(eq("invalidorigin"), anyString())).thenThrow(new EmptyResultDataAccessException(1)); + + SavedRequest savedRequest = SessionUtils.getSavedRequestSession(mockHttpServletRequest.getSession()); + when(savedRequest.getParameterValues("login_hint")).thenReturn(new String[]{"{\"origin\":\"invalidorigin\"}"}); + + endpoint.loginForHtml(extendedModelMap, null, mockHttpServletRequest, Collections.singletonList(MediaType.TEXT_HTML)); + + assertFalse(((Map)extendedModelMap.get("oauthLinks")).isEmpty()); + } + @Test void loginHintOriginOidc() throws Exception { MockHttpServletRequest mockHttpServletRequest = getMockHttpServletRequest(); From c32c34908d7e885202ef6ea86a143732c16cb121 Mon Sep 17 00:00:00 2001 From: Florian Tack Date: Fri, 29 Jan 2021 13:19:10 +0100 Subject: [PATCH 015/140] Add MockMvc Test to show performance improvement --- .../LoginPagePerformanceMockMvcTest.java | 166 ++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 uaa/src/test/java/org/cloudfoundry/identity/uaa/performance/LoginPagePerformanceMockMvcTest.java diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/performance/LoginPagePerformanceMockMvcTest.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/performance/LoginPagePerformanceMockMvcTest.java new file mode 100644 index 00000000000..3ad5ab09761 --- /dev/null +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/performance/LoginPagePerformanceMockMvcTest.java @@ -0,0 +1,166 @@ +package org.cloudfoundry.identity.uaa.performance; + +import static org.cloudfoundry.identity.uaa.mock.util.MockMvcUtils.CookieCsrfPostProcessor.cookieCsrf; +import static org.cloudfoundry.identity.uaa.mock.util.MockMvcUtils.createOtherIdentityZoneAndReturnResult; +import static org.junit.Assert.assertFalse; +import static org.springframework.http.MediaType.TEXT_HTML; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import java.io.File; +import java.net.URL; +import java.util.Collections; + +import org.cloudfoundry.identity.uaa.DefaultTestContext; +import org.cloudfoundry.identity.uaa.codestore.JdbcExpiringCodeStore; +import org.cloudfoundry.identity.uaa.constants.OriginKeys; +import org.cloudfoundry.identity.uaa.impl.config.IdentityZoneConfigurationBootstrap; +import org.cloudfoundry.identity.uaa.mock.util.MockMvcUtils; +import org.cloudfoundry.identity.uaa.provider.AbstractExternalOAuthIdentityProviderDefinition; +import org.cloudfoundry.identity.uaa.provider.IdentityProvider; +import org.cloudfoundry.identity.uaa.provider.JdbcIdentityProviderProvisioning; +import org.cloudfoundry.identity.uaa.provider.OIDCIdentityProviderDefinition; +import org.cloudfoundry.identity.uaa.provider.oauth.OidcMetadataFetcher; +import org.cloudfoundry.identity.uaa.util.SetServerNameRequestPostProcessor; +import org.cloudfoundry.identity.uaa.web.LimitedModeUaaFilter; +import org.cloudfoundry.identity.uaa.zone.IdentityZone; +import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder; +import org.cloudfoundry.identity.uaa.zone.MultitenancyFixture; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.oauth2.common.util.RandomValueStringGenerator; +import org.springframework.security.oauth2.provider.client.BaseClientDetails; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.MvcResult; +import org.springframework.util.StopWatch; +import org.springframework.util.StringUtils; +import org.springframework.web.context.WebApplicationContext; + +@DefaultTestContext +@DirtiesContext +public class LoginPagePerformanceMockMvcTest { + + private WebApplicationContext webApplicationContext; + + private RandomValueStringGenerator generator; + + private MockMvc mockMvc; + + + + private File originalLimitedModeStatusFile; + + @MockBean + OidcMetadataFetcher oidcMetadataFetcher; + + @BeforeEach + void setUpContext( + @Autowired WebApplicationContext webApplicationContext, + @Autowired MockMvc mockMvc, + @Autowired LimitedModeUaaFilter limitedModeUaaFilter + ) { + generator = new RandomValueStringGenerator(); + this.webApplicationContext = webApplicationContext; + this.mockMvc = mockMvc; + SecurityContextHolder.clearContext(); + + originalLimitedModeStatusFile = MockMvcUtils.getLimitedModeStatusFile(webApplicationContext); + MockMvcUtils.resetLimitedModeStatusFile(webApplicationContext, null); + assertFalse(limitedModeUaaFilter.isEnabled()); + } + + @AfterEach + void resetGenerator( + @Autowired JdbcExpiringCodeStore jdbcExpiringCodeStore + ) { + jdbcExpiringCodeStore.setGenerator(new RandomValueStringGenerator(24)); + } + + @AfterEach + void tearDown(@Autowired IdentityZoneConfigurationBootstrap identityZoneConfigurationBootstrap) throws Exception { + MockMvcUtils.setSelfServiceLinksEnabled(webApplicationContext, IdentityZone.getUaaZoneId(), true); + identityZoneConfigurationBootstrap.afterPropertiesSet(); + SecurityContextHolder.clearContext(); + IdentityZoneHolder.clear(); + MockMvcUtils.resetLimitedModeStatusFile(webApplicationContext, originalLimitedModeStatusFile); + } + + @Test + void idpDiscoveryRedirectsToOIDCProvider( + @Autowired JdbcIdentityProviderProvisioning jdbcIdentityProviderProvisioning + ) throws Exception { + String subdomain = "oidc-discovery-" + generator.generate().toLowerCase(); + IdentityZone zone = MultitenancyFixture.identityZone(subdomain, subdomain); + zone.getConfig().setIdpDiscoveryEnabled(true); + BaseClientDetails client = new BaseClientDetails("admin", null, null, "client_credentials", + "clients.admin,scim.read,scim.write,idps.write,uaa.admin", "http://redirect.url"); + client.setClientSecret("admin-secret"); + createOtherIdentityZoneAndReturnResult(mockMvc, webApplicationContext, client, zone, false, IdentityZoneHolder.getCurrentZoneId()); + + + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", null); + String originKey = createOIDCProvider(jdbcIdentityProviderProvisioning, generator, zone, "id_token code", "test.org"); + + StopWatch stopWatch = new StopWatch(); + stopWatch.start(); + for (int i = 0; i <1000; i++) { + MvcResult mvcResult = mockMvc.perform(get("/login") + .with(cookieCsrf()) + .header("Accept", TEXT_HTML) + .with(new SetServerNameRequestPostProcessor(zone.getSubdomain() + ".localhost"))) + .andExpect(status().isOk()) + .andReturn(); + MockHttpServletResponse response = mvcResult.getResponse(); + } + + stopWatch.stop(); + long totalTimeMillis = stopWatch.getTotalTimeMillis(); + + System.out.println(totalTimeMillis + "ms"); + } + + + private static String createOIDCProvider(JdbcIdentityProviderProvisioning jdbcIdentityProviderProvisioning, RandomValueStringGenerator generator, IdentityZone zone, String responseType, String domain) throws Exception { + String originKey = generator.generate(); + AbstractExternalOAuthIdentityProviderDefinition definition = new OIDCIdentityProviderDefinition(); + definition.setAuthUrl(new URL("http://myauthurl.com")); + definition.setTokenKey("key"); + definition.setTokenUrl(new URL("http://mytokenurl.com")); + definition.setRelyingPartyId("id"); + definition.setRelyingPartySecret("secret"); + definition.setLinkText("my oidc provider"); + if (StringUtils.hasText(responseType)) { + definition.setResponseType(responseType); + } + if (StringUtils.hasText(domain)) { + definition.setEmailDomain(Collections.singletonList(domain)); + } + + IdentityProvider identityProvider = MultitenancyFixture.identityProvider(originKey, zone.getId()); + identityProvider.setType(OriginKeys.OIDC10); + identityProvider.setConfig(definition); + createIdentityProvider(jdbcIdentityProviderProvisioning, zone, identityProvider); + return originKey; + } + + private static IdentityProvider createIdentityProvider(JdbcIdentityProviderProvisioning jdbcIdentityProviderProvisioning, IdentityZone identityZone, IdentityProvider activeIdentityProvider) { + activeIdentityProvider.setIdentityZoneId(identityZone.getId()); + return jdbcIdentityProviderProvisioning.create(activeIdentityProvider, identityZone.getId()); + } +} From e21ece5dc99c129c7616ab0322c4c803c1b18764 Mon Sep 17 00:00:00 2001 From: Cloud Foundry Identity Team Date: Wed, 3 Feb 2021 00:53:19 +0000 Subject: [PATCH 016/140] Update UAA image reference in k8s deployment template to cloudfoundry/uaa@sha256:c1d54700f1e6b8fabe49917a8e4ca59f381a11c69982439525f9af8a08f29e0c --- k8s/templates/values/image.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/k8s/templates/values/image.yml b/k8s/templates/values/image.yml index 54e35b92c5b..0ac73de2027 100644 --- a/k8s/templates/values/image.yml +++ b/k8s/templates/values/image.yml @@ -1,3 +1,3 @@ #@data/values --- -image: "cloudfoundry/uaa@sha256:37fcf63a156b75174fbc7be0e3809d64cda6851de0bfe6fa7f12793d919de96a" +image: "cloudfoundry/uaa@sha256:c1d54700f1e6b8fabe49917a8e4ca59f381a11c69982439525f9af8a08f29e0c" From ecf3734d0b8533f1571220c450979f5595d92edf Mon Sep 17 00:00:00 2001 From: Bruce Ricard Date: Thu, 4 Feb 2021 11:51:58 -0800 Subject: [PATCH 017/140] Bump dependency * the previous version of mime has a vulnerability https://nvd.nist.gov/vuln/detail/CVE-2017-16138 --- uaa/slate/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/uaa/slate/package.json b/uaa/slate/package.json index db4427c6ca5..b11f2920864 100644 --- a/uaa/slate/package.json +++ b/uaa/slate/package.json @@ -116,7 +116,7 @@ "map-obj": "^1.0.1", "media-typer": "^0.3.0", "meow": "^3.7.0", - "mime": "^1.3.4", + "mime": "^2.5.0", "mime-db": "^1.22.0", "mime-types": "^2.1.10", "minimatch": "^3.0.0", From 9834fc3405b31a0a887d22e48e4162d57364217c Mon Sep 17 00:00:00 2001 From: Cloud Foundry Identity Team Date: Sun, 14 Feb 2021 06:41:15 +0000 Subject: [PATCH 018/140] Update UAA image reference in k8s deployment template to cloudfoundry/uaa@sha256:7fd48f08134e279a4fe2b8a200ecfdca8cda847175df38f1293e9818d7dd53cc --- k8s/templates/values/image.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/k8s/templates/values/image.yml b/k8s/templates/values/image.yml index 0ac73de2027..0a6418f398e 100644 --- a/k8s/templates/values/image.yml +++ b/k8s/templates/values/image.yml @@ -1,3 +1,3 @@ #@data/values --- -image: "cloudfoundry/uaa@sha256:c1d54700f1e6b8fabe49917a8e4ca59f381a11c69982439525f9af8a08f29e0c" +image: "cloudfoundry/uaa@sha256:7fd48f08134e279a4fe2b8a200ecfdca8cda847175df38f1293e9818d7dd53cc" From fdd4e72ee103ba12f9baef19058eb0acc9edaa53 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Thu, 25 Feb 2021 11:55:01 -0800 Subject: [PATCH 019/140] feat: for unit tests, add time out to waiting for DB to start - and print out the DB boot log if fail to start [#177100664] --- scripts/start_db_helper.sh | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/scripts/start_db_helper.sh b/scripts/start_db_helper.sh index b7dcb37889a..036e9c9f3d8 100755 --- a/scripts/start_db_helper.sh +++ b/scripts/start_db_helper.sh @@ -16,7 +16,8 @@ function bootDB { db=$1 if [[ "${db}" = "postgresql" ]]; then - launchDB="(/docker-entrypoint.sh postgres -c 'max_connections=250' &> /var/log/postgres-boot.log) &" + bootLogLocation="/var/log/postgres-boot.log" + launchDB="(/docker-entrypoint.sh postgres -c 'max_connections=250' &> ${bootLogLocation}) &" testConnection="(! ps aux | grep docker-entrypoint | grep -v 'grep') && psql -h localhost -U postgres -c '\conninfo' &>/dev/null" initDB="psql -c 'drop database if exists uaa;' -U postgres; psql -c 'create database uaa;' -U postgres; psql -c 'drop user if exists root;' --dbname=uaa -U postgres; psql -c \"create user root with superuser password 'changeme';\" --dbname=uaa -U postgres; psql -c 'show max_connections;' --dbname=uaa -U postgres;" @@ -27,7 +28,8 @@ function bootDB { elif [[ "${db}" = "mysql" ]] || [[ "${db}" = "mysql-5.6" ]]; then - launchDB="(MYSQL_DATABASE=uaa MYSQL_ROOT_HOST=127.0.0.1 MYSQL_ROOT_PASSWORD='changeme' bash /entrypoint.sh mysqld &> /var/log/mysql-boot.log) &" + bootLogLocation="/var/log/mysql-boot.log" + launchDB="(MYSQL_DATABASE=uaa MYSQL_ROOT_HOST=127.0.0.1 MYSQL_ROOT_PASSWORD='changeme' bash /entrypoint.sh mysqld &> ${bootLogLocation}) &" testConnection="echo '\s;' | mysql -uroot -pchangeme &>/dev/null" initDB="mysql -uroot -pchangeme -e 'SET GLOBAL max_connections = 250; ALTER DATABASE uaa DEFAULT CHARACTER SET utf8 DEFAULT COLLATE utf8_general_ci;';" @@ -37,7 +39,8 @@ function bootDB { } elif [[ "${db}" = "percona" ]]; then - launchDB="bash /entrypoint.sh &> /var/log/mysql-boot.log" + bootLogLocation="/var/log/mysql-boot.log" + launchDB="bash /entrypoint.sh &> ${bootLogLocation}" testConnection="echo '\s;' | mysql &>/dev/null" initDB="mysql -e \"CREATE USER 'root'@'127.0.0.1' IDENTIFIED BY 'changeme' ;\"; mysql -e \"GRANT ALL ON *.* TO 'root'@'127.0.0.1' WITH GRANT OPTION ;\"; @@ -60,7 +63,9 @@ function bootDB { echo -n "Booting $db" set -x eval "$launchDB" - while true; do + + for i in {0..600} # wait at most 10 mins to the database to start + do set +ex eval "$testConnection" exitcode=$? @@ -80,4 +85,8 @@ function bootDB { echo -n "." sleep 1 done + + echo "Printing database boot logs:" + cat "$bootLogLocation" + exit 1 } From 766e981735b144aa33b674b7f3fab9d798a8f3d0 Mon Sep 17 00:00:00 2001 From: Olivier Lechevalier Date: Mon, 26 Oct 2020 17:07:48 +0900 Subject: [PATCH 020/140] Fix typo --- .../manager/DynamicZoneAwareAuthenticationManagerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/authentication/manager/DynamicZoneAwareAuthenticationManagerTest.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/authentication/manager/DynamicZoneAwareAuthenticationManagerTest.java index fc0ffc45b32..1eb215e7691 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/authentication/manager/DynamicZoneAwareAuthenticationManagerTest.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/authentication/manager/DynamicZoneAwareAuthenticationManagerTest.java @@ -142,7 +142,7 @@ void testNonUAAZoneUaaActiveAccountLocked() { } @Test - void testNonUAAZoneUaaActiveUaaAuthenticationSucccess() { + void testNonUAAZoneUaaActiveUaaAuthenticationSuccess() { IdentityZoneHolder.set(ZONE); when(providerProvisioning.retrieveByOrigin(OriginKeys.UAA, ZONE.getId())).thenReturn(uaaActive); when(providerProvisioning.retrieveByOrigin(OriginKeys.LDAP, ZONE.getId())).thenReturn(ldapActive); From c88dfff40118d395b71607b4d3a2e265168625e4 Mon Sep 17 00:00:00 2001 From: Markus Strehle Date: Fri, 19 Mar 2021 09:28:58 +0100 Subject: [PATCH 021/140] refactor: implement recommendation from thymeleaf (#1512) by change I found warnings in log. in total 3 warnings found [THYMELEAF][Test worker] Template Mode 'HTML5' is deprecated. Using Template Mode 'HTML' instead. [THYMELEAF][Test worker] Template Mode 'HTML5' is deprecated. Using Template Mode 'HTML' instead. Initializing Spring TestDispatcherServlet '' Initializing Servlet '' Completed initialization in 3 ms The layout:decorator/data-layout-decorator processor has been deprecated and will be removed in the next major version of the layout dialect. Please use layout:decorate/data-layout-decorate instead to future-proof your code. See https://github.com/ultraq/thymeleaf-layout-dialect/issues/95 for more information. Fragment expression "layouts/main" is being wrapped as a Thymeleaf 3 fragment expression (~{...}) for backwards compatibility purposes. This wrapping will be dropped in the next major version of the expression processor, so please rewrite as a Thymeleaf 3 fragment expression to future-proof your code. See https://github.com/thymeleaf/thymeleaf/issues/451 for more information. ## https://github.com/ultraq/thymeleaf-layout-dialect/issues/95 , changed layout:decorator to layout:decorate ## use template mode HTML instead of HTML5 ## swtiched back to from th:with="isLdap which was removed with spring update --- .../identity/uaa/login/ThymeleafConfig.java | 3 ++- .../resources/templates/web/access_confirmation.html | 2 +- .../templates/web/access_confirmation_error.html | 2 +- .../resources/templates/web/accounts/email_sent.html | 2 +- .../resources/templates/web/accounts/link_prompt.html | 2 +- .../templates/web/accounts/new_activation_email.html | 2 +- server/src/main/resources/templates/web/approvals.html | 2 +- .../src/main/resources/templates/web/change_email.html | 2 +- .../main/resources/templates/web/change_password.html | 2 +- .../src/main/resources/templates/web/email_sent.html | 2 +- server/src/main/resources/templates/web/error.html | 2 +- .../resources/templates/web/external_auth_error.html | 2 +- .../resources/templates/web/force_password_change.html | 2 +- .../main/resources/templates/web/forgot_password.html | 2 +- server/src/main/resources/templates/web/home.html | 2 +- .../templates/web/idp_discovery/account_chooser.html | 2 +- .../resources/templates/web/idp_discovery/email.html | 2 +- .../templates/web/idp_discovery/password.html | 2 +- .../main/resources/templates/web/invalid_request.html | 2 +- .../templates/web/invitations/accept_invite.html | 10 +++++----- server/src/main/resources/templates/web/login.html | 2 +- .../main/resources/templates/web/login_implicit.html | 2 +- .../main/resources/templates/web/mfa/enter_code.html | 2 +- .../templates/web/mfa/manual_registration.html | 2 +- .../src/main/resources/templates/web/mfa/qr_code.html | 2 +- server/src/main/resources/templates/web/passcode.html | 2 +- .../main/resources/templates/web/reset_password.html | 2 +- .../src/main/resources/templates/web/switch_idp.html | 2 +- 28 files changed, 33 insertions(+), 32 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/login/ThymeleafConfig.java b/server/src/main/java/org/cloudfoundry/identity/uaa/login/ThymeleafConfig.java index 673230e6472..7efd81c0e43 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/login/ThymeleafConfig.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/login/ThymeleafConfig.java @@ -28,6 +28,7 @@ import org.thymeleaf.spring5.SpringTemplateEngine; import org.thymeleaf.spring5.templateresolver.SpringResourceTemplateResolver; import org.thymeleaf.spring5.view.ThymeleafViewResolver; +import org.thymeleaf.templatemode.TemplateMode; import org.thymeleaf.templateresolver.ITemplateResolver; import java.nio.charset.StandardCharsets; @@ -104,7 +105,7 @@ public org.springframework.web.servlet.view.ContentNegotiatingViewResolver viewR private SpringResourceTemplateResolver baseHtmlTemplateResolver(ApplicationContext context) { SpringResourceTemplateResolver templateResolver = new SpringResourceTemplateResolver(); templateResolver.setSuffix(".html"); - templateResolver.setTemplateMode("HTML5"); + templateResolver.setTemplateMode(TemplateMode.HTML); templateResolver.setApplicationContext(context); return templateResolver; } diff --git a/server/src/main/resources/templates/web/access_confirmation.html b/server/src/main/resources/templates/web/access_confirmation.html index 99e230ec0ba..a66b703a9e3 100644 --- a/server/src/main/resources/templates/web/access_confirmation.html +++ b/server/src/main/resources/templates/web/access_confirmation.html @@ -2,7 +2,7 @@ + layout:decorate="~{layouts/main}"> diff --git a/server/src/main/resources/templates/web/access_confirmation_error.html b/server/src/main/resources/templates/web/access_confirmation_error.html index 7d454fd650c..2fbe2f85337 100644 --- a/server/src/main/resources/templates/web/access_confirmation_error.html +++ b/server/src/main/resources/templates/web/access_confirmation_error.html @@ -1,5 +1,5 @@ - +
diff --git a/server/src/main/resources/templates/web/accounts/email_sent.html b/server/src/main/resources/templates/web/accounts/email_sent.html index 5b0d5084c10..389d37b61b0 100644 --- a/server/src/main/resources/templates/web/accounts/email_sent.html +++ b/server/src/main/resources/templates/web/accounts/email_sent.html @@ -1,7 +1,7 @@ + layout:decorate="~{layouts/main}"> diff --git a/server/src/main/resources/templates/web/accounts/link_prompt.html b/server/src/main/resources/templates/web/accounts/link_prompt.html index 145b18d69fa..27d34953a52 100644 --- a/server/src/main/resources/templates/web/accounts/link_prompt.html +++ b/server/src/main/resources/templates/web/accounts/link_prompt.html @@ -1,7 +1,7 @@ + layout:decorate="~{layouts/main}">

Create your account

diff --git a/server/src/main/resources/templates/web/accounts/new_activation_email.html b/server/src/main/resources/templates/web/accounts/new_activation_email.html index a77bc756f2b..7150431ff98 100644 --- a/server/src/main/resources/templates/web/accounts/new_activation_email.html +++ b/server/src/main/resources/templates/web/accounts/new_activation_email.html @@ -1,7 +1,7 @@ + layout:decorate="~{layouts/main}">

Create your account

diff --git a/server/src/main/resources/templates/web/approvals.html b/server/src/main/resources/templates/web/approvals.html index a72c4348b7d..6d392a12450 100644 --- a/server/src/main/resources/templates/web/approvals.html +++ b/server/src/main/resources/templates/web/approvals.html @@ -2,7 +2,7 @@ + layout:decorate="~{layouts/main}">