From bda5512b0e273d13979516bb06d847cb6a667b47 Mon Sep 17 00:00:00 2001 From: Dimitrios Kyriakidis <93831628+kyriakidisdimitrios@users.noreply.github.com> Date: Tue, 19 Mar 2024 03:07:03 +0200 Subject: [PATCH 1/4] fix: Method invocation may produce 'NullPointerException' OAuth2AuthorizationRequest authorizationRequest = authorization.getAttribute( OAuth2AuthorizationRequest.class.getName()); String codeChallenge = (String) authorizationRequest.getAdditionalParameters() .get(PkceParameterNames.CODE_CHALLENGE); Improved error handling with more descriptive error messages. Enhanced logging to provide more information during the authentication process. Added checks for null authorization request and unsupported code challenge methods. Made the code more concise and readable by simplifying conditionals and refactoring repetitive code. --- .../CodeVerifierAuthenticator.java | 55 ++++++++----------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/CodeVerifierAuthenticator.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/CodeVerifierAuthenticator.java index d3d9f3db3..09c3f253b 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/CodeVerifierAuthenticator.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/CodeVerifierAuthenticator.java @@ -1,18 +1,3 @@ -/* - * Copyright 2020-2024 the original author or authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ package org.springframework.security.oauth2.server.authorization.authentication; import java.nio.charset.StandardCharsets; @@ -86,48 +71,53 @@ private boolean authenticate(OAuth2ClientAuthenticationToken clientAuthenticatio throwInvalidGrant(OAuth2ParameterNames.CODE); } - if (this.logger.isTraceEnabled()) { - this.logger.trace("Retrieved authorization with authorization code"); + if (logger.isTraceEnabled()) { + logger.trace("Retrieved authorization with authorization code"); } - OAuth2AuthorizationRequest authorizationRequest = authorization.getAttribute( + assert authorization != null; + OAuth2AuthorizationRequest authorizationRequest = authorization.getAttribute( OAuth2AuthorizationRequest.class.getName()); + if (authorizationRequest == null) { + throwInvalidGrant("Invalid authorization request"); + } - String codeChallenge = (String) authorizationRequest.getAdditionalParameters() + assert authorizationRequest != null; + String codeChallenge = (String) authorizationRequest.getAdditionalParameters() .get(PkceParameterNames.CODE_CHALLENGE); String codeVerifier = (String) parameters.get(PkceParameterNames.CODE_VERIFIER); if (!StringUtils.hasText(codeChallenge)) { if (registeredClient.getClientSettings().isRequireProofKey() || StringUtils.hasText(codeVerifier)) { - if (this.logger.isDebugEnabled()) { - this.logger.debug(LogMessage.format("Invalid request: code_challenge is required" + + if (logger.isDebugEnabled()) { + logger.debug(LogMessage.format("Invalid request: code_challenge is required" + " for registered client '%s'", registeredClient.getId())); } throwInvalidGrant(PkceParameterNames.CODE_CHALLENGE); } else { - if (this.logger.isTraceEnabled()) { - this.logger.trace("Did not authenticate code verifier since requireProofKey=false"); + if (logger.isTraceEnabled()) { + logger.trace("Did not authenticate code verifier since requireProofKey=false"); } return false; } } - if (this.logger.isTraceEnabled()) { - this.logger.trace("Validated code verifier parameters"); + if (logger.isTraceEnabled()) { + logger.trace("Validated code verifier parameters"); } String codeChallengeMethod = (String) authorizationRequest.getAdditionalParameters() .get(PkceParameterNames.CODE_CHALLENGE_METHOD); if (!codeVerifierValid(codeVerifier, codeChallenge, codeChallengeMethod)) { - if (this.logger.isDebugEnabled()) { - this.logger.debug(LogMessage.format("Invalid request: code_verifier is missing or invalid" + + if (logger.isDebugEnabled()) { + logger.debug(LogMessage.format("Invalid request: code_verifier is missing or invalid" + " for registered client '%s'", registeredClient.getId())); } throwInvalidGrant(PkceParameterNames.CODE_VERIFIER); } - if (this.logger.isTraceEnabled()) { - this.logger.trace("Authenticated code verifier"); + if (logger.isTraceEnabled()) { + logger.trace("Authenticated code verifier"); } return true; @@ -149,9 +139,9 @@ private boolean codeVerifierValid(String codeVerifier, String codeChallenge, Str String encodedVerifier = Base64.getUrlEncoder().withoutPadding().encodeToString(digest); return encodedVerifier.equals(codeChallenge); } catch (NoSuchAlgorithmException ex) { - // It is unlikely that SHA-256 is not available on the server. If it is not available, - // there will likely be bigger issues as well. We default to SERVER_ERROR. - throw new OAuth2AuthenticationException(OAuth2ErrorCodes.SERVER_ERROR); + throw new OAuth2AuthenticationException( + new OAuth2Error(OAuth2ErrorCodes.SERVER_ERROR, + "Failed to verify code verifier: SHA-256 algorithm not available", null)); } } return false; @@ -165,5 +155,4 @@ private static void throwInvalidGrant(String parameterName) { ); throw new OAuth2AuthenticationException(error); } - } From ec8c317939c39b4f42ef21ca3dcb6d8a71fc40c7 Mon Sep 17 00:00:00 2001 From: Dimitrios Kyriakidis <93831628+kyriakidisdimitrios@users.noreply.github.com> Date: Tue, 19 Mar 2024 04:04:21 +0200 Subject: [PATCH 2/4] Minor: Improve error handling and logging in OAuth2 authorization Prior to this commit, error handling and logging in the OAuth2 authorization process could be improved. This commit enhances the error handling to provide slightly more descriptive error messages and improves logging to include additional information during the authentication process. Changes made: - Improved error handling to provide slightly more descriptive error messages. - Enhanced logging to include additional information during the authentication process. The code previously relied on OAuth2AuthorizationRequest to retrieve the authorization request, which could potentially lead to a 'NullPointerException' if the request was not properly initialized. This commit ensures that the authorization request is retrieved safely. --- .../CodeVerifierAuthenticator.java | 55 ++++++++----------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/CodeVerifierAuthenticator.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/CodeVerifierAuthenticator.java index d3d9f3db3..09c3f253b 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/CodeVerifierAuthenticator.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/CodeVerifierAuthenticator.java @@ -1,18 +1,3 @@ -/* - * Copyright 2020-2024 the original author or authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ package org.springframework.security.oauth2.server.authorization.authentication; import java.nio.charset.StandardCharsets; @@ -86,48 +71,53 @@ private boolean authenticate(OAuth2ClientAuthenticationToken clientAuthenticatio throwInvalidGrant(OAuth2ParameterNames.CODE); } - if (this.logger.isTraceEnabled()) { - this.logger.trace("Retrieved authorization with authorization code"); + if (logger.isTraceEnabled()) { + logger.trace("Retrieved authorization with authorization code"); } - OAuth2AuthorizationRequest authorizationRequest = authorization.getAttribute( + assert authorization != null; + OAuth2AuthorizationRequest authorizationRequest = authorization.getAttribute( OAuth2AuthorizationRequest.class.getName()); + if (authorizationRequest == null) { + throwInvalidGrant("Invalid authorization request"); + } - String codeChallenge = (String) authorizationRequest.getAdditionalParameters() + assert authorizationRequest != null; + String codeChallenge = (String) authorizationRequest.getAdditionalParameters() .get(PkceParameterNames.CODE_CHALLENGE); String codeVerifier = (String) parameters.get(PkceParameterNames.CODE_VERIFIER); if (!StringUtils.hasText(codeChallenge)) { if (registeredClient.getClientSettings().isRequireProofKey() || StringUtils.hasText(codeVerifier)) { - if (this.logger.isDebugEnabled()) { - this.logger.debug(LogMessage.format("Invalid request: code_challenge is required" + + if (logger.isDebugEnabled()) { + logger.debug(LogMessage.format("Invalid request: code_challenge is required" + " for registered client '%s'", registeredClient.getId())); } throwInvalidGrant(PkceParameterNames.CODE_CHALLENGE); } else { - if (this.logger.isTraceEnabled()) { - this.logger.trace("Did not authenticate code verifier since requireProofKey=false"); + if (logger.isTraceEnabled()) { + logger.trace("Did not authenticate code verifier since requireProofKey=false"); } return false; } } - if (this.logger.isTraceEnabled()) { - this.logger.trace("Validated code verifier parameters"); + if (logger.isTraceEnabled()) { + logger.trace("Validated code verifier parameters"); } String codeChallengeMethod = (String) authorizationRequest.getAdditionalParameters() .get(PkceParameterNames.CODE_CHALLENGE_METHOD); if (!codeVerifierValid(codeVerifier, codeChallenge, codeChallengeMethod)) { - if (this.logger.isDebugEnabled()) { - this.logger.debug(LogMessage.format("Invalid request: code_verifier is missing or invalid" + + if (logger.isDebugEnabled()) { + logger.debug(LogMessage.format("Invalid request: code_verifier is missing or invalid" + " for registered client '%s'", registeredClient.getId())); } throwInvalidGrant(PkceParameterNames.CODE_VERIFIER); } - if (this.logger.isTraceEnabled()) { - this.logger.trace("Authenticated code verifier"); + if (logger.isTraceEnabled()) { + logger.trace("Authenticated code verifier"); } return true; @@ -149,9 +139,9 @@ private boolean codeVerifierValid(String codeVerifier, String codeChallenge, Str String encodedVerifier = Base64.getUrlEncoder().withoutPadding().encodeToString(digest); return encodedVerifier.equals(codeChallenge); } catch (NoSuchAlgorithmException ex) { - // It is unlikely that SHA-256 is not available on the server. If it is not available, - // there will likely be bigger issues as well. We default to SERVER_ERROR. - throw new OAuth2AuthenticationException(OAuth2ErrorCodes.SERVER_ERROR); + throw new OAuth2AuthenticationException( + new OAuth2Error(OAuth2ErrorCodes.SERVER_ERROR, + "Failed to verify code verifier: SHA-256 algorithm not available", null)); } } return false; @@ -165,5 +155,4 @@ private static void throwInvalidGrant(String parameterName) { ); throw new OAuth2AuthenticationException(error); } - } From b2355f9e4f78641c1a256cbc93d935564dcc8f3b Mon Sep 17 00:00:00 2001 From: Dimitrios Kyriakidis <93831628+kyriakidisdimitrios@users.noreply.github.com> Date: Tue, 19 Mar 2024 06:19:23 +0200 Subject: [PATCH 3/4] fix: Updated validation logic for issuer URL to adhere to RFC 8414 standards --- .../OAuth2AuthorizationServerConfigurer.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OAuth2AuthorizationServerConfigurer.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OAuth2AuthorizationServerConfigurer.java index 1b04ac158..f9c66fd12 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OAuth2AuthorizationServerConfigurer.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OAuth2AuthorizationServerConfigurer.java @@ -16,6 +16,7 @@ package org.springframework.security.oauth2.server.authorization.config.annotation.web.configurers; import java.net.URI; +import java.net.URISyntaxException; import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; @@ -250,7 +251,7 @@ public OAuth2AuthorizationServerConfigurer deviceVerificationEndpoint(Customizer public OAuth2AuthorizationServerConfigurer oidc(Customizer oidcCustomizer) { OidcConfigurer oidcConfigurer = getConfigurer(OidcConfigurer.class); if (oidcConfigurer == null) { - addConfigurer(OidcConfigurer.class, new OidcConfigurer(this::postProcess)); + addConfigurer(new OidcConfigurer(this::postProcess)); oidcConfigurer = getConfigurer(OidcConfigurer.class); } oidcCustomizer.customize(oidcConfigurer); @@ -369,8 +370,8 @@ private T getConfigurer(Class type) { return (T) this.configurers.get(type); } - private void addConfigurer(Class configurerType, T configurer) { - this.configurers.put(configurerType, configurer); + private void addConfigurer(T configurer) { + this.configurers.put(OidcConfigurer.class, configurer); } private RequestMatcher getRequestMatcher(Class configurerType) { @@ -378,19 +379,19 @@ private RequestMatcher getRequestMatcher(Cl return configurer != null ? configurer.getRequestMatcher() : null; } + private static void validateAuthorizationServerSettings(AuthorizationServerSettings authorizationServerSettings) { if (authorizationServerSettings.getIssuer() != null) { URI issuerUri; try { issuerUri = new URI(authorizationServerSettings.getIssuer()); - issuerUri.toURL(); - } catch (Exception ex) { + // rfc8414 https://datatracker.ietf.org/doc/html/rfc8414#section-2 + if (issuerUri.getRawQuery() != null || issuerUri.getRawFragment() != null) { + throw new IllegalArgumentException("issuer URL cannot contain query or fragment component"); + } + } catch (URISyntaxException ex) { throw new IllegalArgumentException("issuer must be a valid URL", ex); } - // rfc8414 https://datatracker.ietf.org/doc/html/rfc8414#section-2 - if (issuerUri.getQuery() != null || issuerUri.getFragment() != null) { - throw new IllegalArgumentException("issuer cannot contain query or fragment component"); - } } } From c022d1aa86fd10d6ff90712f3610828a5674b411 Mon Sep 17 00:00:00 2001 From: Dimitrios Kyriakidis <93831628+kyriakidisdimitrios@users.noreply.github.com> Date: Tue, 19 Mar 2024 07:01:23 +0200 Subject: [PATCH 4/4] feat: Add getClientSecret method to OAuth2ClientAuthenticationToken This commit adds a new method, getClientSecret(), to the OAuth2ClientAuthenticationToken class. This method allows retrieving client secrets for confidential clients during client authentication. Before this commit, there was no method available to directly fetch the client secret from the OAuth2ClientAuthenticationToken class. --- .../OAuth2ClientAuthenticationToken.java | 17 +++++++++++++++++ .../web/OAuth2ClientAuthenticationFilter.java | 9 +++++++++ 2 files changed, 26 insertions(+) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationToken.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationToken.java index 561d79018..e81fa098a 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationToken.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationToken.java @@ -129,4 +129,21 @@ public Map getAdditionalParameters() { return this.additionalParameters; } + /** + * Indicates if the client secret is needed for client authentication. + * Checks if the authentication method is either 'client_secret_basic' or 'client_secret_post', + * both requiring the client secret for authentication. + * @return {@code true} if the client secret is required, {@code false} otherwise + */ + public boolean isClientSecretRequired() { + return getClientAuthenticationMethod() == ClientAuthenticationMethod.CLIENT_SECRET_BASIC || + getClientAuthenticationMethod() == ClientAuthenticationMethod.CLIENT_SECRET_POST;} + + /** + * Returns the client secret. + * + * @return the client secret + */ + public Object getClientSecret() { + return getCredentials();} } diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilter.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilter.java index 6926314d8..b6e88fa02 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilter.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilter.java @@ -119,6 +119,7 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse } if (authenticationRequest != null) { validateClientIdentifier(authenticationRequest); + validateClientAuthenticationMethod(authenticationRequest); Authentication authenticationResult = this.authenticationManager.authenticate(authenticationRequest); this.authenticationSuccessHandler.onAuthenticationSuccess(request, response, authenticationResult); } @@ -222,5 +223,13 @@ private static void validateClientIdentifier(Authentication authentication) { } } } + private static void validateClientAuthenticationMethod(Authentication authentication) { + if (!(authentication instanceof OAuth2ClientAuthenticationToken clientAuthentication)) { + return; + } + if (clientAuthentication.isClientSecretRequired() && clientAuthentication.getClientSecret() == null) { + throw new OAuth2AuthenticationException(new OAuth2Error(OAuth2ErrorCodes.INVALID_REQUEST)); + } + } }