Support jwt client authentication for Spring Cloud Azure AD Starter #29471

moarychan · 2022-06-15T02:12:54Z

Description

Fixes #29137 #27032

Changelog:

Deprecated ~~AadAuthorizationGrantType~~, use AuthorizationGrantType instead.
Deprecated ~~AadOAuth2AuthenticatedPrincipal~~, ~~AadJwtBearerTokenAuthenticationConverter~~, use the default converter JwtAuthenticationConverter instead in AadResourceServerWebSecurityConfigurerAdapter, it's required when the authorization grant type is JWT_BEARER.
Deprecated ~~AadOboOAuth2AuthorizedClientProvider~~, use JwtBearerOAuth2AuthorizedClientProvider instead.
Update validation to allow to override the attributes for azure client registration, such as only override the client authentication method for azure, spring.cloud.azure.active-directory.authorization-clients.azure.client-authentication-method=private_key_jwt.
Add default implementation OAuth2ClientAuthenticationJWKResolver to support loading a local certificate to create the client assertion.
NOTE:
The AadJwtClientAuthenticationParametersConverter implementation is similar to NimbusJwtClientAuthenticationParametersConverter, but it's not suitable for our scenario, we can not custom the JwsHeader; we can deprecate it when upgrading the spring-security-oauth2-client version to 5.7.0, it can be done by the Consumer function to custom the jws header and claims.

https://github.com/spring-projects/spring-security/blob/428216b322e1591357d8f7450d6f1045cd7c48d5/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/endpoint/NimbusJwtClientAuthenticationParametersConverter.java#L150-L152
Replace the authorization grant type to AuthorizationGrantType in AuthorizationClientProperties class.
The authorization grant type ON_BEHALF_OF will be replaced with JWT_BEARER type automatically.

How to make it still available to use AadOboOAuth2AuthorizedClientProvider flow for developers?

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

…/support-jwt-client-authentication

ghost · 2022-06-15T02:13:03Z

Thank you for your contribution moarychan! We will review the pull request and get back to you soon.

….6.X

…is more readable

chenrujun · 2022-06-24T10:20:06Z

eng/code-quality-reports/src/main/resources/revapi/revapi.json

+        {
+          "code": "java.annotation.attributeValueChanged",
+          "old": "class com.azure.spring.cloud.autoconfigure.aad.AadAutoConfiguration",
+          "new": "class com.azure.spring.cloud.autoconfigure.aad.AadAutoConfiguration",


Is "new" necessary here?

Is it possible to "support JWT Client Authentication" without this change?

Same to all other changes in current file.
If breaking change is necessary, please add more description in "justification".

saragluna · 2022-06-27T07:26:49Z

.../com/azure/spring/cloud/autoconfigure/aad/AadResourceServerWebSecurityConfigurerAdapter.java


+    private Converter<Jwt, AbstractAuthenticationToken> jwtAuthenticationConverter() {
+        JwtAuthenticationConverter converter = new JwtAuthenticationConverter();
+        if (StringUtils.hasText(properties.getPrincipalClaimName())) {
+            converter.setPrincipalClaimName(properties.getPrincipalClaimName());
+        }
+        converter.setJwtGrantedAuthoritiesConverter(
+            new AadJwtGrantedAuthoritiesConverter(properties.getClaimToAuthorityPrefixMap()));
+        return converter;
+    }


spring-projects/spring-security#7834

saragluna · 2022-06-27T09:27:01Z

...ain/java/com/azure/spring/cloud/autoconfigure/aad/OAuth2ClientAuthenticationJwkResolver.java

+@FunctionalInterface
+public interface OAuth2ClientAuthenticationJwkResolver {
+
+    /**
+     * @param clientRegistration the client registration.
+     * @return a a {@link JWK}.
+     */
+    JWK resolve(ClientRegistration clientRegistration);
+}


Do we expect users to provide this bean? If not, can we move it to the impl package?

Moved to impl.oauth2 pkg.

...in/java/com/azure/spring/cloud/autoconfigure/aad/properties/AadAuthenticationProperties.java

jialigit · 2022-06-28T02:09:36Z

eng/code-quality-reports/src/main/resources/revapi/revapi.json

+        {
+          "code": "java.annotation.attributeValueChanged",
+          "new": "class com.azure.spring.cloud.autoconfigure.aad.AadAutoConfiguration",
+          "justification": "The use of AadOboOAuth2AuthorizedClientProvider is not recommended, and there is no need to retain consideration of the different situations brought by this OBO provider."


Do you the not recommended but still supported? Is this an API exposed to users or not?

Yes, users can override the bean OAuth2AuthorizedClientManager to take the OBO provider ability.

jialigit · 2022-06-28T02:14:12Z

eng/code-quality-reports/src/main/resources/revapi/revapi.json

+        {
+          "code": "java.annotation.added",
+          "new": "class com.azure.spring.cloud.autoconfigure.aad.configuration.AadOAuth2ClientConfiguration",
+          "justification": "The use of AadOboOAuth2AuthorizedClientProvider is not recommended, and there is no need to retain consideration of the different situations brought by this OBO provider."


How about the "justification" focus on the "new code" instead of "xx is no need"?

The old code is to meet the different situations of OBO provider usage, as this is deprecated, then it can be removed no matter which grant type is used.

jialigit · 2022-06-28T02:24:02Z

sdk/spring/CHANGELOG.md

+
+#### Breaking Changes
+ Deprecated classes and properties type changed [#29471](https://github.com/Azure/azure-sdk-for-java/pull/29471).
+    + Deprecated ~~AadAuthorizationGrantType~~, use `AuthorizationGrantType` instead.


Are we just @deprecate an interface or changed the implementation? If we use deprecate, we mean the api is @deprecated and add a new one.

It seems this place should be "removed" and replaced with a new one.
Do we mean this?

This @deprecated is to tell users not to use the AadAuthorizationGrantType, and still can be used for some users that depend on this type when upgrading the AAD starter to 4.3.0. It will be kept until 5.0.

jialigit · 2022-06-28T02:25:49Z

sdk/spring/CHANGELOG.md

+#### Breaking Changes
+ Deprecated classes and properties type changed [#29471](https://github.com/Azure/azure-sdk-for-java/pull/29471).
+    + Deprecated *~~AadAuthorizationGrantType~~*, use `AuthorizationGrantType` instead.
+    + The type of property *authorizationGrantType* is changed to `AuthorizationGrantType` in `AuthorizationClientProperties` class.


Is this duplicate with the above one? Or they sit in different packages?

One is for AAD starter, another is for AAD B2C starter.

jialigit · 2022-06-28T05:52:16Z

.../src/main/java/com/azure/spring/cloud/autoconfigure/aad/AadClientRegistrationRepository.java

-import static com.azure.spring.cloud.autoconfigure.aad.properties.AadAuthorizationGrantType.AUTHORIZATION_CODE;
-import static com.azure.spring.cloud.autoconfigure.aad.properties.AadAuthorizationGrantType.AZURE_DELEGATED;
+import static com.azure.spring.cloud.autoconfigure.aad.implementation.constants.Constants.AZURE_DELEGATED;
+import static com.azure.spring.cloud.autoconfigure.aad.implementation.constants.Constants.ON_BEHALF_OF;


why do we use this const instead of the original enum one?

They are all the same type AuthorizationGrantType.

jialigit · 2022-06-28T05:59:23Z

.../src/main/java/com/azure/spring/cloud/autoconfigure/aad/AadClientRegistrationRepository.java

        allClients.put(AZURE_CLIENT_REGISTRATION_ID, azureClient);
    }

+    private ClientAuthenticationMethod getAzureDefaultClientAuthenticationMethod() {
+        if (this.allClients.containsKey(AZURE_CLIENT_REGISTRATION_ID)) {


getOrDefault?

This means getting the client authentication method of the default client(azure)， there might be some ambiguity.

jialigit · 2022-06-28T06:21:12Z

.../src/main/java/com/azure/spring/cloud/autoconfigure/aad/AadClientRegistrationRepository.java

+
+        if (ON_BEHALF_OF.equals(authorizationGrantType)) {
+            authorizationGrantType = JWT_BEARER;
+            LOGGER.warn("The grant type 'on_behalf_of' of the client '{}' is not recommended, it will be "


why do we warn it here?

It means the type value is changed by our logic, I will improve the log content.

jialigit · 2022-06-28T06:26:59Z

.../java/com/azure/spring/cloud/autoconfigure/aad/AadOAuth2ClientAuthenticationJwkResolver.java

+                LOGGER.error("Resolve RSAKey exception.", e);
+            }
+        }
+        return null;


should we return null?

Currently, only supports the PRIVATE_KEY_JWT.

jialigit · 2022-06-28T06:27:39Z

.../java/com/azure/spring/cloud/autoconfigure/aad/AadOAuth2ClientAuthenticationJwkResolver.java

+                    .build();
+            } catch (KeyStoreException | IOException | NoSuchAlgorithmException
+                     | CertificateException | UnrecoverableKeyException e) {
+                LOGGER.error("Resolve RSAKey exception.", e);


Should we only log this error?

Yes, the null will be cached in com.azure.spring.cloud.autoconfigure.aad.implementation.jwt.AadJwtClientAuthenticationParametersConverter#convert.

JWK jwk = this.jwkResolver.apply(clientRegistration); if (jwk == null) { OAuth2Error oauth2Error = new OAuth2Error(INVALID_KEY_ERROR_CODE, "Failed to resolve JWK signing key for client registration '" + clientRegistration.getRegistrationId() + "'.", null); throw new OAuth2AuthorizationException(oauth2Error); }

jialigit · 2022-06-28T06:30:22Z

.../java/com/azure/spring/cloud/autoconfigure/aad/AadOAuth2ClientAuthenticationJwkResolver.java

+    }
+
+    private byte[] getX5t(X509Certificate cert)
+        throws NoSuchAlgorithmException, CertificateEncodingException {


should we throw this EX?

same as above

jialigit · 2022-06-28T06:30:56Z

.../java/com/azure/spring/cloud/autoconfigure/aad/AadOAuth2ClientAuthenticationJwkResolver.java

+        return getSHA1Byte(cert.getEncoded());
+    }
+
+    private byte[] getSHA1Byte(byte[] data) throws NoSuchAlgorithmException {


How about encodeWithSha1 or the like?

jialigit · 2022-06-28T06:37:09Z

.../com/azure/spring/cloud/autoconfigure/aad/AadResourceServerWebSecurityConfigurerAdapter.java

        // @formatter:off
    }

+    private Converter<Jwt, AbstractAuthenticationToken> jwtAuthenticationConverter() {


Can we define a bean of this object?

It's not necessary, this adapter is built-in in our starter, users can re-define a new one to custom the adapter without this converter.

jialigit · 2022-06-28T06:44:54Z

.../src/main/java/com/azure/spring/cloud/autoconfigure/aad/AadWebSecurityConfigurerAdapter.java

@@ -114,9 +122,14 @@ protected LogoutSuccessHandler oidcLogoutSuccessHandler() {
    protected OAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> accessTokenResponseClient() {
        DefaultAuthorizationCodeTokenResponseClient result = new DefaultAuthorizationCodeTokenResponseClient();
        if (repo instanceof AadClientRegistrationRepository) {
-            result.setRequestEntityConverter(
+            AadOAuth2AuthorizationCodeGrantRequestEntityConverter converter =


Can we use a more OO way in this place?

Please provide your proposal for reference.

...ava/com/azure/spring/cloud/autoconfigure/aad/configuration/AadOAuth2ClientConfiguration.java

moarychan added 2 commits June 15, 2022 09:38

Support jwt client authentication for Azure AD starter

ac52663

Merge branch 'main' of github.com:Azure/azure-sdk-for-java into moary…

16f79c8

…/support-jwt-client-authentication

ghost added azure-spring All azure-spring related issues customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Jun 15, 2022

moarychan marked this pull request as ready for review June 15, 2022 02:59

moarychan requested review from backwind1233, chenrujun, hui1110, jialigit, Netyyyy, saragluna, stliu and yiliuTo as code owners June 15, 2022 02:59

saragluna closed this Jun 15, 2022

saragluna reopened this Jun 15, 2022

moarychan added 2 commits June 15, 2022 13:56

Update CHANGELOG.md

be9dd91

Update jwt encoder for compatibility with spring-security 5.5.X and 5…

cb1561f

….6.X

moarychan marked this pull request as draft June 20, 2022 08:39

moarychan added 10 commits June 20, 2022 16:40

Remove unused lines

e1da7e1

Add Java doc

519142e

Update ignore revapi json

e974b51

Update ignore revapi json

9967623

Update ignore revapi json

6dafa18

Fix revapi json configuration

a7bb791

Fix revapi json configuration

8aba3a1

Fix revapi json and deprecated obo provider

6de492f

Fix revapi json and make the grant type on_behalf_of available as it …

2b81a26

…is more readable

Update java doc

b47d1a9

moarychan marked this pull request as ready for review June 21, 2022 07:00

This was referenced Jun 23, 2022

Add JWT Client Authentication usage microsoft/spring-cloud-azure#971

Merged

Add note for conditional access filter usage Azure-Samples/azure-spring-boot-samples#401

Closed

moarychan added 2 commits June 24, 2022 15:25

Fix comments and add more unit tests

e3f283c

Fix code smells

97a0e78

moarychan mentioned this pull request Jun 24, 2022

Delete AADOBOOAuth2AuthorizedClientProvider after spring-boot 2.7.0 GA (2022-5-19) #27032

Closed

chenrujun reviewed Jun 24, 2022

View reviewed changes

Update justification

1949deb

saragluna requested changes Jun 27, 2022

View reviewed changes

jialigit reviewed Jun 28, 2022

View reviewed changes

moarychan added 2 commits June 28, 2022 15:25

Fix comments

e700da0

Revert change

c594437

saragluna requested changes Jun 28, 2022

View reviewed changes

...ava/com/azure/spring/cloud/autoconfigure/aad/configuration/AadOAuth2ClientConfiguration.java Outdated Show resolved Hide resolved

...ava/com/azure/spring/cloud/autoconfigure/aad/configuration/AadOAuth2ClientConfiguration.java Outdated Show resolved Hide resolved

Fix comments

ac72afa

saragluna approved these changes Jun 28, 2022

View reviewed changes

saragluna added this to the 2022-07 milestone Jun 28, 2022

saragluna merged commit 316164c into Azure:main Jun 28, 2022

chenrujun mentioned this pull request Aug 10, 2022

[BUG] Cannot configure "azure" authorization client #30354

Closed

3 tasks

moarychan mentioned this pull request Oct 24, 2022

Upgrade the deprecated apis for spring cloud azure 5.0 #31543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support jwt client authentication for Spring Cloud Azure AD Starter #29471

Support jwt client authentication for Spring Cloud Azure AD Starter #29471

moarychan commented Jun 15, 2022 •

edited

Loading

ghost commented Jun 15, 2022

chenrujun Jun 24, 2022

chenrujun Jun 24, 2022 •

edited

Loading

chenrujun Jun 24, 2022

saragluna Jun 27, 2022

saragluna Jun 27, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022

moarychan Jun 28, 2022 •

edited

Loading

jialigit Jun 28, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022 •

edited

Loading

jialigit Jun 28, 2022

moarychan Jun 28, 2022

jialigit Jun 28, 2022

moarychan Jun 28, 2022

Support jwt client authentication for Spring Cloud Azure AD Starter #29471

Support jwt client authentication for Spring Cloud Azure AD Starter #29471

Conversation

moarychan commented Jun 15, 2022 • edited Loading

Description

All SDK Contribution checklist:

ghost commented Jun 15, 2022

Choose a reason for hiding this comment

chenrujun Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moarychan Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jialigit Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moarychan commented Jun 15, 2022 •

edited

Loading

chenrujun Jun 24, 2022 •

edited

Loading

moarychan Jun 28, 2022 •

edited

Loading

jialigit Jun 28, 2022 •

edited

Loading