Skip to content
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

fix: do not default a missing secret to an empty one #2455

Merged
merged 7 commits into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,14 @@
package org.cloudfoundry.identity.uaa.authentication;

import org.cloudfoundry.identity.uaa.client.UaaClient;
import org.cloudfoundry.identity.uaa.oauth.client.ClientConstants;
import org.cloudfoundry.identity.uaa.oauth.pkce.PkceValidationService;
import org.cloudfoundry.identity.uaa.oauth.token.ClaimConstants;
import org.cloudfoundry.identity.uaa.oauth.token.TokenConstants;
import org.cloudfoundry.identity.uaa.util.UaaStringUtils;
import org.springframework.security.authentication.BadCredentialsException;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.authentication.dao.DaoAuthenticationProvider;
import org.springframework.security.core.AuthenticationException;
import org.springframework.security.core.userdetails.User;
import org.springframework.security.core.userdetails.UserDetails;
import org.springframework.security.core.userdetails.UserDetailsService;
import org.springframework.security.crypto.password.PasswordEncoder;
Expand Down Expand Up @@ -56,18 +55,19 @@ protected void additionalAuthenticationChecks(UserDetails userDetails, UsernameP
AuthenticationException error = null;
for(String pwd: passwordList) {
try {
User user = new User(userDetails.getUsername(), pwd, userDetails.isEnabled(), userDetails.isAccountNonExpired(), userDetails.isCredentialsNonExpired(), userDetails.isAccountNonLocked(), userDetails.getAuthorities());
if (authentication.getCredentials() == null && isPublicGrantTypeUsageAllowed(authentication.getDetails()) && userDetails instanceof UaaClient) {
// in case of grant_type=authorization_code and code_verifier passed (PKCE) we check if client has option allowpublic with true and proceed even if no secret is provided
UaaClient uaaClient = (UaaClient) userDetails;
Object allowPublic = uaaClient.getAdditionalInformation().get(ClientConstants.ALLOW_PUBLIC);
if ((allowPublic instanceof String && Boolean.TRUE.toString().equalsIgnoreCase((String)allowPublic)) ||
(allowPublic instanceof Boolean && Boolean.TRUE.equals(allowPublic))) {
UaaClient uaaClient = new UaaClient(userDetails, pwd);
if (authentication.getCredentials() == null) {
if (isPublicGrantTypeUsageAllowed(authentication.getDetails()) && uaaClient.isAllowPublic()) {
// in case of grant_type=authorization_code and code_verifier passed (PKCE) we check if client has option allowpublic with true and continue even if no secret is in request
((UaaAuthenticationDetails) authentication.getDetails()).setAuthenticationMethod(CLIENT_AUTH_NONE);
break;
}
}
super.additionalAuthenticationChecks(user, authentication);
if (uaaClient.getPassword() == null) {
error = new BadCredentialsException("Missing credentials");
break;
}
super.additionalAuthenticationChecks(uaaClient, authentication);
error = null;
break;
} catch (AuthenticationException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,16 +166,14 @@ private void addNewClients() {
String secondSecret = null;
if (map.get("secret") instanceof List) {
List<String> secrets = (List<String>) map.get("secret");
if (secrets.isEmpty()) {
client.setClientSecret("");
} else {
if (!secrets.isEmpty()) {
client.setClientSecret(secrets.get(0) == null ? "" : secrets.get(0));
if (secrets.size() > 1) {
secondSecret = secrets.get(1) == null ? "" : secrets.get(1);
}
}
} else {
client.setClientSecret(map.get("secret") == null ? "" : (String) map.get("secret"));
client.setClientSecret((String) map.get("secret"));
}

Integer validity = (Integer) map.get("access-token-validity");
Expand Down Expand Up @@ -272,8 +270,10 @@ private void updatePasswordsIfChanged(String clientId, String rawPassword1, Stri
// check if both passwords are still up to date
// 1st line: client already has 2 passwords: check if both are still correct
// 2nd line: client has only 1 pasword: check if password is correct and second password is null
if ( (existingPasswordHash.length > 1 && passwordEncoder.matches(rawPassword1, existingPasswordHash[0]) && passwordEncoder.matches(rawPassword2, existingPasswordHash[1]) )
|| (passwordEncoder.matches(rawPassword1, existingPasswordHash[0]) && rawPassword2 == null) ) {
if ( (existingPasswordHash.length > 1 && rawPassword1 != null
&& passwordEncoder.matches(rawPassword1, existingPasswordHash[0])
&& rawPassword2 != null && passwordEncoder.matches(rawPassword2, existingPasswordHash[1]) )
|| (rawPassword1 != null && (passwordEncoder.matches(rawPassword1, existingPasswordHash[0]) && rawPassword2 == null)) ) {
// no changes to passwords: nothing to do here
return;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,58 @@
package org.cloudfoundry.identity.uaa.client;

import org.cloudfoundry.identity.uaa.oauth.client.ClientConstants;
import org.springframework.security.core.GrantedAuthority;
import org.springframework.security.core.SpringSecurityCoreVersion;
import org.springframework.security.core.userdetails.User;
import org.springframework.security.core.userdetails.UserDetails;

import java.util.Collection;
import java.util.Collections;
import java.util.Map;
import java.util.Optional;

public class UaaClient extends User {

private static final long serialVersionUID = SpringSecurityCoreVersion.SERIAL_VERSION_UID;
private transient Map<String, Object> additionalInformation;

private final String secret;

public UaaClient(String username, String password, Collection<? extends GrantedAuthority> authorities, Map<String, Object> additionalInformation) {
super(username, password, authorities);
super(username, password == null ? "" : password, authorities);
this.additionalInformation = additionalInformation;
this.secret = password;
}

public UaaClient(UserDetails userDetails, String secret) {
super(userDetails.getUsername(), secret == null ? "" : secret, userDetails.isEnabled(), userDetails.isAccountNonExpired(),
userDetails.isCredentialsNonExpired(), userDetails.isAccountNonLocked(), userDetails.getAuthorities());
if (userDetails instanceof UaaClient) {
this.additionalInformation = ((UaaClient) userDetails).getAdditionalInformation();
}
this.secret = secret;
}

public Map<String, Object> getAdditionalInformation() {
public boolean isAllowPublic() {
Object allowPublic = Optional.ofNullable(additionalInformation).map(e -> e.get(ClientConstants.ALLOW_PUBLIC)).orElse(Collections.emptyMap());
if ((allowPublic instanceof String && Boolean.TRUE.toString().equalsIgnoreCase((String) allowPublic)) || (allowPublic instanceof Boolean && Boolean.TRUE.equals(allowPublic))) {
return true;
} else {
return false;
}
}

private Map<String, Object> getAdditionalInformation() {
return this.additionalInformation;
}

/**
* Allow to return a null password. Super class does not allow to omit a password, therefore use own method
*
* @return The password of the client, can be null if no secret is set
*/
@Override
strehle marked this conversation as resolved.
Show resolved Hide resolved
public String getPassword() {
return this.secret;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,25 @@
import org.springframework.security.core.userdetails.UserDetails;
import org.springframework.security.core.userdetails.UserDetailsService;
import org.springframework.security.core.userdetails.UsernameNotFoundException;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.security.oauth2.provider.ClientDetailsService;
import org.springframework.security.oauth2.provider.NoSuchClientException;

public class UaaClientDetailsUserDetailsService implements UserDetailsService {

private final ClientDetailsService clientDetailsService;
private String emptyPassword = "";

public UaaClientDetailsUserDetailsService(final ClientDetailsService clientDetailsService) {
this.clientDetailsService = clientDetailsService;
}

/**
* @param passwordEncoder the password encoder to set
*/
public void setPasswordEncoder(PasswordEncoder passwordEncoder) {
this.emptyPassword = passwordEncoder.encode("");
}

public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException {
UaaClientDetails clientDetails;
try {
clientDetails = (UaaClientDetails) clientDetailsService.loadClientByClientId(username);
} catch (NoSuchClientException e) {
throw new UsernameNotFoundException(e.getMessage(), e);
}
String clientSecret = clientDetails.getClientSecret();
if (clientSecret== null || clientSecret.trim().length()==0) {
clientSecret = emptyPassword;
}
return new UaaClient(username, clientSecret, clientDetails.getAuthorities(), clientDetails.getAdditionalInformation());
return new UaaClient(username, clientDetails.getClientSecret(), clientDetails.getAuthorities(), clientDetails.getAdditionalInformation());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public void updateClientDetails(ClientDetails clientDetails, String zoneId) thro

@Override
public void updateClientSecret(String clientId, String secret, String zoneId) throws NoSuchClientException {
int count = jdbcTemplate.update(DEFAULT_UPDATE_SECRET_STATEMENT, passwordEncoder.encode(secret), clientId, zoneId);
int count = jdbcTemplate.update(DEFAULT_UPDATE_SECRET_STATEMENT, secret != null ? passwordEncoder.encode(secret) : null, clientId, zoneId);
if (count != 1) {
throw new NoSuchClientException("No client found with id = " + clientId);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.Map;

import static org.cloudfoundry.identity.uaa.oauth.client.ClientDetailsModification.SECRET;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand Down Expand Up @@ -58,7 +59,6 @@ void setUpForClientTests() {

jdbcClientDetailsService = new MultitenantJdbcClientDetailsService(jdbcTemplate, mockIdentityZoneManager, passwordEncoder);
UaaClientDetailsUserDetailsService clientDetailsService = new UaaClientDetailsUserDetailsService(jdbcClientDetailsService);
clientDetailsService.setPasswordEncoder(passwordEncoder);
client = createClient();
authenticationProvider = new ClientDetailsAuthenticationProvider(clientDetailsService, passwordEncoder);
}
Expand Down Expand Up @@ -195,6 +195,19 @@ void provider_authenticate_client_without_password_public_missing_code() {
assertThrows(BadCredentialsException.class, () -> authenticationProvider.additionalAuthenticationChecks(new UaaClient("client", "secret", Collections.emptyList(), client.getAdditionalInformation()), a));
}

@Test
void provider_authenticate_client_without_secret_user_without_secret() {
client = new BaseClientDetails(generator.generate(), "", "", "client_credentials", "uaa.resource");
jdbcClientDetailsService.addClientDetails(client);
UsernamePasswordAuthenticationToken a = mock(UsernamePasswordAuthenticationToken.class);
UaaAuthenticationDetails uaaAuthenticationDetails = mock(UaaAuthenticationDetails.class);
when(a.getDetails()).thenReturn(uaaAuthenticationDetails);
Map<String, String[]> requestParameters = new HashMap<>();
when(uaaAuthenticationDetails.getParameterMap()).thenReturn(requestParameters);
Exception e = assertThrows(BadCredentialsException.class, () -> authenticationProvider.additionalAuthenticationChecks(new UaaClient("client", null, Collections.emptyList(), client.getAdditionalInformation()), a));
assertEquals("Missing credentials", e.getMessage());
}

@Test
void provider_authenticate_client_without_password_public_false() {
client = createClient(ClientConstants.ALLOW_PUBLIC, false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ public void newClientEmptyPasswordList() throws Exception {
buildClient(null);
clients.get(clientId).put("secret", new LinkedList<>());
clientAdminBootstrap.afterPropertiesSet();
assertClient("");
assertClient(null);
}

@Test
Expand Down Expand Up @@ -280,7 +280,7 @@ public void updateTwoSecretClientSingletonNullList() throws Exception {

private void assertClient(String password) {
Assert.assertEquals(clientId, verifyClient.getClientId());
Assert.assertEquals(password == null ? "" : password, verifyClient.getClientSecret());
Assert.assertEquals(password, verifyClient.getClientSecret());
}

private void buildClientSingletonList(String password1) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,31 @@ void overrideClientWithEmptySecret() {

reset(multitenantJdbcClientDetailsService);

Map<String, Object> map = new HashMap<>();
map.put("secret", "");
map.put("override", true);
map.put("authorized-grant-types", "client_credentials");
clients.put(clientId, map);

doThrow(new ClientAlreadyExistsException("Planned"))
.when(multitenantJdbcClientDetailsService).addClientDetails(any(ClientDetails.class), anyString());
clientAdminBootstrap.afterPropertiesSet();
verify(multitenantJdbcClientDetailsService, times(1)).addClientDetails(any(ClientDetails.class), anyString());
ArgumentCaptor<ClientDetails> captor = ArgumentCaptor.forClass(ClientDetails.class);
verify(multitenantJdbcClientDetailsService, times(1)).updateClientDetails(captor.capture(), anyString());
verify(multitenantJdbcClientDetailsService, times(1)).updateClientSecret(clientId, "", "uaa");
assertEquals(new HashSet(Collections.singletonList("client_credentials")), captor.getValue().getAuthorizedGrantTypes());
}

@Test
void doNotOverrideClientWithNullSecret() {
String clientId = randomValueStringGenerator.generate();
BaseClientDetails foo = new BaseClientDetails(clientId, "", "openid", "client_credentials,password", "uaa.none");
foo.setClientSecret("secret");
multitenantJdbcClientDetailsService.addClientDetails(foo);

reset(multitenantJdbcClientDetailsService);

Map<String, Object> map = new HashMap<>();
map.put("secret", null);
map.put("override", true);
Expand All @@ -455,7 +480,7 @@ void overrideClientWithEmptySecret() {
verify(multitenantJdbcClientDetailsService, times(1)).addClientDetails(any(ClientDetails.class), anyString());
ArgumentCaptor<ClientDetails> captor = ArgumentCaptor.forClass(ClientDetails.class);
verify(multitenantJdbcClientDetailsService, times(1)).updateClientDetails(captor.capture(), anyString());
verify(multitenantJdbcClientDetailsService, times(1)).updateClientSecret(clientId, "", "uaa");
verify(multitenantJdbcClientDetailsService, times(1)).updateClientSecret(clientId, null, "uaa");
assertEquals(new HashSet(Collections.singletonList("client_credentials")), captor.getValue().getAuthorizedGrantTypes());
}

Expand Down
2 changes: 2 additions & 0 deletions uaa/src/main/webapp/WEB-INF/spring/oauth-clients.xml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
</entry>
<entry key="cf">
<map>
<entry key="secret" value=""/>
<entry key="authorized-grant-types" value="implicit,password,refresh_token"/>
<entry key="scope"
value="uaa.user,cloud_controller.read,cloud_controller.write,openid,password.write,scim.userids,cloud_controller.admin,scim.read,scim.write"/>
Expand Down Expand Up @@ -166,6 +167,7 @@
</entry>
<entry key="oauth_showcase_user_token_public">
<map>
<entry key="secret" value=""/>
<entry key="authorized-grant-types" value="user_token,password,authorization_code"/>
<entry key="scope" value="openid,uaa.user"/>
<entry key="redirect-uri" value="http://localhost:8080/uaa/"/>
Expand Down
1 change: 0 additions & 1 deletion uaa/src/main/webapp/WEB-INF/spring/oauth-endpoints.xml
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,6 @@
<bean id="clientDetailsUserService"
class="org.cloudfoundry.identity.uaa.client.UaaClientDetailsUserDetailsService">
<constructor-arg ref="jdbcClientDetailsService"/>
<property name="passwordEncoder" ref="cachingPasswordEncoder"/>
</bean>

<bean id="oauthAuthorizeRequestMatcher" class="org.cloudfoundry.identity.uaa.security.web.UaaRequestMatcher">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1008,6 +1008,10 @@ public static BaseClientDetails createClient(MockMvc mockMvc, String accessToken
public static BaseClientDetails createClient(ApplicationContext context, BaseClientDetails clientDetails, IdentityZone zone) {

MultitenantJdbcClientDetailsService service = context.getBean(MultitenantJdbcClientDetailsService.class);
if (clientDetails.getClientSecret() == null) {
// provide for tests the empty secret behavior
clientDetails.setClientSecret("");
}
service.addClientDetails(clientDetails, zone.getId());
return (BaseClientDetails) service.loadClientByClientId(clientDetails.getClientId(), zone.getId());
}
Expand Down
Loading