-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support jwt client authentication for Spring Cloud Azure AD Starter #29471
Changes from 31 commits
ac52663
16f79c8
be9dd91
cb1561f
e1da7e1
519142e
e974b51
9967623
6dafa18
a7bb791
8aba3a1
6de492f
2b81a26
b47d1a9
49d20ae
078a5c0
69b2ebd
c4e9863
1d00df9
1ecf11a
8371cb2
c16615f
dbd28b5
7a0aac2
b0d118c
64b4a20
e3f283c
97a0e78
1949deb
e700da0
c594437
ac72afa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -254,6 +254,51 @@ | |
"old": "method org.springframework.boot.autoconfigure.kafka.KafkaProperties com.azure.spring.cloud.autoconfigure.eventhubs.kafka.AzureEventHubsKafkaAutoConfiguration::azureKafkaProperties(com.azure.spring.cloud.core.provider.connectionstring.ServiceConnectionStringProvider<com.azure.spring.cloud.core.service.AzureServiceType.EventHubs>)", | ||
"justification": "To move kafka properties customization to bean post processor." | ||
}, | ||
{ | ||
"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." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you the not recommended but still supported? Is this an API exposed to users or not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, users can override the bean |
||
}, | ||
{ | ||
"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." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about the "justification" focus on the "new code" instead of "xx is no need"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}, | ||
{ | ||
"regex": true, | ||
"code": "java.class.removed", | ||
"old": "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." | ||
}, | ||
{ | ||
"code": "java.method.returnTypeChanged", | ||
"old:": "method com.azure.spring.cloud.autoconfigure.aad.properties.AadAuthorizationGrantType com.azure.spring.cloud.autoconfigure.aad.properties.AuthorizationClientProperties::getAuthorizationGrantType()", | ||
"new": "method org.springframework.security.oauth2.core.AuthorizationGrantType com.azure.spring.cloud.autoconfigure.aad.properties.AuthorizationClientProperties::getAuthorizationGrantType()", | ||
"justification": "To support authorization grant type JWT_BEARER, we should use the default AuthorizationGrantType instead of AadAuthorizationGrantType." | ||
}, | ||
{ | ||
"code": "java.method.returnTypeChanged", | ||
"old": "method com.azure.spring.cloud.autoconfigure.aad.properties.AadAuthorizationGrantType com.azure.spring.cloud.autoconfigure.aadb2c.properties.AuthorizationClientProperties::getAuthorizationGrantType()", | ||
"new": "method org.springframework.security.oauth2.core.AuthorizationGrantType com.azure.spring.cloud.autoconfigure.aadb2c.properties.AuthorizationClientProperties::getAuthorizationGrantType()", | ||
saragluna marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"justification": "To support authorization grant type JWT_BEARER, we should use the default AuthorizationGrantType instead of AadAuthorizationGrantType." | ||
}, | ||
{ | ||
"code": "java.method.parameterTypeChanged", | ||
"old": "parameter void com.azure.spring.cloud.autoconfigure.aad.properties.AuthorizationClientProperties::setAuthorizationGrantType(===com.azure.spring.cloud.autoconfigure.aad.properties.AadAuthorizationGrantType===)", | ||
"new": "parameter void com.azure.spring.cloud.autoconfigure.aad.properties.AuthorizationClientProperties::setAuthorizationGrantType(===org.springframework.security.oauth2.core.AuthorizationGrantType===)", | ||
"justification": "To support authorization grant type JWT_BEARER, we should use the default AuthorizationGrantType instead of AadAuthorizationGrantType." | ||
}, | ||
{ | ||
"code": "java.method.parameterTypeChanged", | ||
"old": "parameter void com.azure.spring.cloud.autoconfigure.aadb2c.properties.AuthorizationClientProperties::setAuthorizationGrantType(===com.azure.spring.cloud.autoconfigure.aad.properties.AadAuthorizationGrantType===)", | ||
"new": "parameter void com.azure.spring.cloud.autoconfigure.aadb2c.properties.AuthorizationClientProperties::setAuthorizationGrantType(===org.springframework.security.oauth2.core.AuthorizationGrantType===)", | ||
"justification": "To support authorization grant type JWT_BEARER, we should use the default AuthorizationGrantType instead of AadAuthorizationGrantType." | ||
}, | ||
{ | ||
"code": "java.field.removedWithConstant", | ||
"old": "field com.azure.spring.cloud.autoconfigure.aad.AadAuthenticationFilterAutoConfiguration.PROPERTY_PREFIX", | ||
"justification": "Unused constant." | ||
}, | ||
{ | ||
"code": "java.method.visibilityReduced", | ||
"new": "method com.azure.spring.cloud.autoconfigure.context.AzureTokenCredentialAutoConfiguration.AzureServiceClientBuilderFactoryPostProcessor com.azure.spring.cloud.autoconfigure.context.AzureTokenCredentialAutoConfiguration::builderFactoryBeanPostProcessor()", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
### Features Added | ||
- GA the `spring-cloud-azure-starter-storage`. This starter supports all features of Azure Storage. | ||
- GA the `spring-cloud-azure-starter-keyvault`. This starter supports all features of Azure Key Vault. | ||
- Support Jwt Client authentication for Azure AD Starter. | ||
|
||
### Breaking Changes | ||
|
||
|
@@ -48,13 +49,28 @@ This section includes changes in `spring-cloud-azure-starter-active-directory` m | |
#### Dependency Updates | ||
- Upgrade spring-security to 5.6.4 to address [CVE-2022-22978](https://spring.io/blog/2022/05/15/cve-2022-22978-authorization-bypass-in-regexrequestmatcher) [#29304](https://github.com/Azure/azure-sdk-for-java/pull/29304). | ||
|
||
#### Features Added | ||
+ Support Jwt Client authentication [#29471](https://github.com/Azure/azure-sdk-for-java/pull/29471). | ||
|
||
#### Breaking Changes | ||
+ Deprecated classes and properties type changed [#29471](https://github.com/Azure/azure-sdk-for-java/pull/29471). | ||
+ Deprecated ~~AadAuthorizationGrantType~~, use `AuthorizationGrantType` instead. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we just @deprecate an interface or changed the implementation? If we use It seems this place should be "removed" and replaced with a new one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
+ Deprecated ~~AadOAuth2AuthenticatedPrincipal~~, ~~AadJwtBearerTokenAuthenticationConverter~~, use the default converter `JwtAuthenticationConverter` instead in `AadResourceServerWebSecurityConfigurerAdapter`. | ||
+ The type of property *authorizationGrantType* is changed to `AuthorizationGrantType` in `AuthorizationClientProperties` class. | ||
+ Deprecated ~~AadOboOAuth2AuthorizedClientProvider~~, use `JwtBearerOAuth2AuthorizedClientProvider` instead. | ||
|
||
### Spring Cloud Azure Starter Active Directory B2C | ||
This section includes changes in `spring-cloud-azure-starter-active-directory-b2c` module. | ||
|
||
#### Dependency Updates | ||
- Upgrade spring-security to 5.6.4 to address [CVE-2022-22978](https://spring.io/blog/2022/05/15/cve-2022-22978-authorization-bypass-in-regexrequestmatcher) [#29304](https://github.com/Azure/azure-sdk-for-java/pull/29304). | ||
|
||
|
||
#### 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this duplicate with the above one? Or they sit in different packages? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One is for AAD starter, another is for AAD B2C starter. |
||
|
||
## 4.2.0 (2022-05-26) | ||
|
||
- This release is compatible with Spring Boot 2.5.0-2.5.14, 2.6.0-2.6.8, 2.7.0. (Note: 2.5.x (x>14), 2.6.y (y>8) and 2.7.z (z>0) should be supported, but they aren't tested with this release.) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,14 @@ | |
package com.azure.spring.cloud.autoconfigure.aad; | ||
|
||
import com.azure.spring.cloud.autoconfigure.aad.properties.AadAuthenticationProperties; | ||
import com.azure.spring.cloud.autoconfigure.aad.properties.AadAuthorizationGrantType; | ||
import com.azure.spring.cloud.autoconfigure.aad.properties.AadAuthorizationServerEndpoints; | ||
import com.azure.spring.cloud.autoconfigure.aad.properties.AuthorizationClientProperties; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import org.springframework.security.oauth2.client.registration.ClientRegistration; | ||
import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository; | ||
import org.springframework.security.oauth2.core.AuthorizationGrantType; | ||
import org.springframework.security.oauth2.core.ClientAuthenticationMethod; | ||
import org.springframework.util.Assert; | ||
|
||
import java.util.Collection; | ||
|
@@ -21,8 +23,10 @@ | |
import java.util.Set; | ||
import java.util.stream.Collectors; | ||
|
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we use this const instead of the original enum one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are all the same type |
||
import static org.springframework.security.oauth2.core.AuthorizationGrantType.AUTHORIZATION_CODE; | ||
import static org.springframework.security.oauth2.core.AuthorizationGrantType.JWT_BEARER; | ||
|
||
|
||
/** | ||
|
@@ -34,6 +38,8 @@ | |
*/ | ||
public class AadClientRegistrationRepository implements ClientRegistrationRepository, Iterable<ClientRegistration> { | ||
|
||
private static final Logger LOGGER = LoggerFactory.getLogger(AadClientRegistrationRepository.class); | ||
|
||
/** | ||
* Azure client registration ID | ||
*/ | ||
|
@@ -68,13 +74,25 @@ public AadClientRegistrationRepository(AadAuthenticationProperties properties) { | |
.stream() | ||
.collect(Collectors.toMap( | ||
Map.Entry::getKey, | ||
entry -> toClientRegistration(entry.getKey(), entry.getValue().getAuthorizationGrantType(), | ||
entry.getValue().getScopes(), properties))); | ||
entry -> toClientRegistration(entry.getKey(), | ||
entry.getValue().getAuthorizationGrantType(), | ||
entry.getValue().getScopes(), | ||
entry.getValue().getClientAuthenticationMethod(), | ||
properties))); | ||
ClientAuthenticationMethod azureClientAuthMethod = getAzureDefaultClientAuthenticationMethod(); | ||
saragluna marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ClientRegistration azureClient = | ||
toClientRegistration(AZURE_CLIENT_REGISTRATION_ID, AUTHORIZATION_CODE, authorizationCodeScopes, properties); | ||
toClientRegistration(AZURE_CLIENT_REGISTRATION_ID, AUTHORIZATION_CODE, | ||
authorizationCodeScopes, azureClientAuthMethod, properties); | ||
allClients.put(AZURE_CLIENT_REGISTRATION_ID, azureClient); | ||
} | ||
|
||
private ClientAuthenticationMethod getAzureDefaultClientAuthenticationMethod() { | ||
if (this.allClients.containsKey(AZURE_CLIENT_REGISTRATION_ID)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getOrDefault? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means getting the client authentication method of the default client(azure), there might be some ambiguity. |
||
return this.allClients.get(AZURE_CLIENT_REGISTRATION_ID).getClientAuthenticationMethod(); | ||
} | ||
return ClientAuthenticationMethod.CLIENT_SECRET_BASIC; | ||
} | ||
moarychan marked this conversation as resolved.
Show resolved
Hide resolved
saragluna marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* Gets the set of Azure client access token scopes. | ||
* | ||
|
@@ -94,8 +112,7 @@ public ClientRegistration findByRegistrationId(String registrationId) { | |
public Iterator<ClientRegistration> iterator() { | ||
return allClients.values() | ||
.stream() | ||
.filter(client -> | ||
client.getAuthorizationGrantType().getValue().equals(AUTHORIZATION_CODE.getValue())) | ||
.filter(client -> AUTHORIZATION_CODE.equals(client.getAuthorizationGrantType())) | ||
.iterator(); | ||
} | ||
|
||
|
@@ -132,19 +149,28 @@ private Set<String> delegatedClientsAccessTokenScopes(AadAuthenticationPropertie | |
} | ||
|
||
private ClientRegistration toClientRegistration(String registrationId, | ||
AadAuthorizationGrantType aadAuthorizationGrantType, | ||
AuthorizationGrantType authorizationGrantType, | ||
Collection<String> scopes, | ||
ClientAuthenticationMethod clientAuthenticationMethod, | ||
AadAuthenticationProperties properties) { | ||
AadAuthorizationServerEndpoints endpoints = | ||
new AadAuthorizationServerEndpoints(properties.getProfile().getEnvironment().getActiveDirectoryEndpoint(), properties.getProfile().getTenantId()); | ||
new AadAuthorizationServerEndpoints(properties.getProfile().getEnvironment().getActiveDirectoryEndpoint(), | ||
properties.getProfile().getTenantId()); | ||
|
||
if (ON_BEHALF_OF.equals(authorizationGrantType)) { | ||
authorizationGrantType = JWT_BEARER; | ||
LOGGER.warn("The grant type 'on_behalf_of' is an alias, it will be replaced with " | ||
+ "'urn:ietf:params:oauth:grant-type:jwt-bearer' for client {}.", registrationId); | ||
} | ||
return ClientRegistration.withRegistrationId(registrationId) | ||
.clientName(registrationId) | ||
.authorizationGrantType(new AuthorizationGrantType((aadAuthorizationGrantType.getValue()))) | ||
.authorizationGrantType(authorizationGrantType) | ||
.scope(scopes) | ||
.redirectUri(properties.getRedirectUriTemplate()) | ||
.userNameAttributeName(properties.getUserNameAttribute()) | ||
.clientId(properties.getCredential().getClientId()) | ||
.clientSecret(properties.getCredential().getClientSecret()) | ||
.clientAuthenticationMethod(clientAuthenticationMethod) | ||
.authorizationUri(endpoints.getAuthorizationEndpoint()) | ||
.tokenUri(endpoints.getTokenEndpoint()) | ||
.jwkSetUri(endpoints.getJwkSetEndpoint()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,10 +3,16 @@ | |
|
||
package com.azure.spring.cloud.autoconfigure.aad; | ||
|
||
import com.azure.spring.cloud.autoconfigure.aad.implementation.jwt.AadJwtGrantedAuthoritiesConverter; | ||
import com.azure.spring.cloud.autoconfigure.aad.properties.AadResourceServerProperties; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.core.convert.converter.Converter; | ||
import org.springframework.security.authentication.AbstractAuthenticationToken; | ||
import org.springframework.security.config.annotation.web.builders.HttpSecurity; | ||
import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; | ||
import org.springframework.security.oauth2.jwt.Jwt; | ||
import org.springframework.security.oauth2.server.resource.authentication.JwtAuthenticationConverter; | ||
import org.springframework.util.StringUtils; | ||
|
||
/** | ||
* Abstract configuration class, used to make JwtConfigurer and AADJwtBearerTokenAuthenticationConverter take effect. | ||
|
@@ -30,10 +36,17 @@ protected void configure(HttpSecurity http) throws Exception { | |
// @formatter:off | ||
http.oauth2ResourceServer() | ||
.jwt() | ||
.jwtAuthenticationConverter( | ||
new AadJwtBearerTokenAuthenticationConverter( | ||
properties.getPrincipalClaimName(), properties.getClaimToAuthorityPrefixMap())); | ||
.jwtAuthenticationConverter(jwtAuthenticationConverter()); | ||
// @formatter:off | ||
} | ||
|
||
private Converter<Jwt, AbstractAuthenticationToken> jwtAuthenticationConverter() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we define a bean of this object? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
JwtAuthenticationConverter converter = new JwtAuthenticationConverter(); | ||
if (StringUtils.hasText(properties.getPrincipalClaimName())) { | ||
converter.setPrincipalClaimName(properties.getPrincipalClaimName()); | ||
} | ||
converter.setJwtGrantedAuthoritiesConverter( | ||
new AadJwtGrantedAuthoritiesConverter(properties.getClaimToAuthorityPrefixMap())); | ||
return converter; | ||
} | ||
Comment on lines
42
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the difference between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the user can choose to use either. Just if you want to use the if (!(context.getPrincipal().getPrincipal() instanceof Jwt)) {
return null;
} There is not a particularly clear statement of their difference, here is a good explanation:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will remove all the unused conditions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "new" necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to "support JWT Client Authentication" without this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same to all other changes in current file.
If breaking change is necessary, please add more description in "justification".