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

JWT federated client authentication #3219

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
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
4 changes: 2 additions & 2 deletions docs/UAA-Client-Authentication.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ a dynamic token key URI. OIDC has defined the parameter jwks_uri for this alread
UAA provides its own jwks_uri with endpoint /token_keys. The content of this endpoint is [JWKS](https://datatracker.ietf.org/doc/html/rfc7517#section-5).

The content of the JWT (parameter client_assertion) can be different. The difference is defined by the standards. The [OIDC core standard](https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication)
simplify the structure, so that sub, iss content are the client_id of the authenticated OAuth2 client. The key rotation is supported with
simplify the structure, so that subject, issuer content are the client_id of the authenticated OAuth2 client. The key rotation is supported with
jwks_uri, which retrieves the JWK. You can only have one JWKS_URI by the client. For the [RFC 7523 from OAuth2 standard](https://www.rfc-editor.org/info/rfc7523) the
structure is more complex but with seperated iss and sub there can be more than one entry of federated credential.
structure is more complex but with seperated issuer and subject there can be more than one entry of federated credential.

The new parameters for JWKS Trust in UAA clients are:

Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,23 @@
package org.cloudfoundry.identity.uaa.oauth.client;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;

import static org.cloudfoundry.identity.uaa.oauth.client.ClientJwtChangeRequest.ChangeMode.ADD;
import static org.cloudfoundry.identity.uaa.oauth.client.ClientJwtChangeRequest.ChangeMode.DELETE;
import static org.cloudfoundry.identity.uaa.oauth.client.ClientJwtChangeRequest.ChangeMode.UPDATE;

@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonIgnoreProperties(ignoreUnknown = true)
public class ClientJwtChangeRequest {

public static final String JWKS_URI = "jwks_uri";
public static final String JWKS = "jwks";
public static final String ISS = "iss";
public static final String SUB = "sub";
public static final String AUD = "aud";

public enum ChangeMode {
UPDATE,
Expand All @@ -27,6 +32,13 @@ public enum ChangeMode {
private String jsonWebKeySet;
@JsonProperty("client_id")
private String clientId;
@JsonProperty(ISS)
private String issuer;
@JsonProperty(SUB)
private String subject;
@JsonProperty(AUD)
private String audience;

private ChangeMode changeMode = ADD;

public ClientJwtChangeRequest() {
Expand Down Expand Up @@ -78,11 +90,46 @@ public void setKeyId(String keyId) {
this.keyId = keyId;
}

public String getIssuer() {
return this.issuer;
}

public void setIssuer(final String issuer) {
this.issuer = issuer;
}

public String getSubject() {
return this.subject;
}

public void setSubject(final String subject) {
this.subject = subject;
}

public String getAudience() {
return this.audience;
}

public void setAudience(final String audience) {
this.audience = audience;
}

public String getChangeValue() {
// Depending on change mode, allow different values
if (changeMode == DELETE && keyId != null) {
return keyId;
}
return jsonWebKeyUri != null ? jsonWebKeyUri : jsonWebKeySet;
}

@JsonIgnore
public boolean isFederated() {
return ((changeMode == ADD || changeMode == UPDATE) && issuer != null && subject != null) ||
(changeMode == DELETE && (issuer != null || subject != null));
}

@JsonIgnore
public ClientJwtCredential getFederation() {
return ClientJwtCredential.builder().issuer(issuer).subject(subject).audience(audience).build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package org.cloudfoundry.identity.uaa.oauth.client;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.type.TypeReference;
import lombok.Builder;
import lombok.Data;
import org.cloudfoundry.identity.uaa.util.JsonUtils;
import org.springframework.util.StringUtils;

import java.util.List;

@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonIgnoreProperties(ignoreUnknown = true)
@Builder(toBuilder = true)
@Data
public class ClientJwtCredential {

@JsonProperty("sub")
private String subject;
@JsonProperty("iss")
private String issuer;
@JsonProperty("aud")
private String audience;

public ClientJwtCredential() {
}

public ClientJwtCredential(String subject, String issuer, String audience) {
this.subject = subject;
this.issuer = issuer;
this.audience = audience;
}

@JsonIgnore
public boolean isValid() {
return StringUtils.hasText(subject) && StringUtils.hasText(issuer);
}

public static List<ClientJwtCredential> parse(String clientJwtCredentials) {
try {
return JsonUtils.readValue(clientJwtCredentials, new TypeReference<>() {});
} catch (JsonUtils.JsonUtilException e) {
throw new IllegalArgumentException("Client jwt configuration cannot be parsed", e);
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,18 @@ void requestSerialization() {
assertThat(request).isNotEqualTo(def);
}

@Test
void requestSerializationFederated() {
ClientJwtChangeRequest def = new ClientJwtChangeRequest();
def.setKeyId("key-1");
def.setChangeMode(ClientJwtChangeRequest.ChangeMode.DELETE);
def.setIssuer("http://localhost:8080/uaa/oauth/token");
def.setSubject("admin-client");
def.setAudience("http://localhost:8080/uaa/oauth/token");
assertThat(def.isFederated()).isTrue();
String jsonRequest = JsonUtils.writeValueAsString(def);
ClientJwtChangeRequest request = JsonUtils.readValue(jsonRequest, ClientJwtChangeRequest.class);
assertThat(request).isNotEqualTo(def);
assertThat(def.getFederation()).isNotNull();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package org.cloudfoundry.identity.uaa.oauth.client;

import org.junit.jupiter.api.Test;

import java.util.List;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatNoException;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

class ClientJwtCredentialTest {

@Test
void parse() {
assertThatNoException().isThrownBy(() -> ClientJwtCredential.parse("[{\"iss\":\"http://localhost:8080/uaa\",\"sub\":\"client_with_jwks_trust\"}]"));
List<ClientJwtCredential> federationList = ClientJwtCredential.parse("[{\"iss\":\"http://localhost:8080/uaa\",\"sub\":\"client_with_jwks_trust\"},{\"iss\":\"http://localhost:8080/uaa\"}]");
assertThat(federationList).hasSize(2);
}

@Test
void constructor() {
ClientJwtCredential jwtCredential = new ClientJwtCredential("subject", "issuer", "audience");
assertThat(jwtCredential)
.returns("subject", ClientJwtCredential::getSubject)
.returns("issuer", ClientJwtCredential::getIssuer)
.returns("audience", ClientJwtCredential::getAudience)
.returns(true, ClientJwtCredential::isValid);
jwtCredential = new ClientJwtCredential();
assertThat(jwtCredential.isValid()).isFalse();
}

@Test
void testDeserializer() {
assertThat(ClientJwtCredential.parse("[{\"iss\":\"issuer\"}]").iterator().next().isValid()).isFalse();
}

@Test
void deserializerException() {
assertThatThrownBy(() -> ClientJwtCredential.parse("[\"iss\":\"issuer\"]"))
.isInstanceOf(IllegalArgumentException.class);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ public void log(AuditEvent auditEvent, String zoneId) {
logMessage = "%s, authenticationType=[%s]".formatted(logMessage, auditEvent.getAuthenticationType());
}

if (auditEvent.getDescription() != null) {
logMessage = "%s, detailedDescription=[%s]".formatted(logMessage, auditEvent.getDescription());
}

log(logMessage);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,10 @@ protected void additionalAuthenticationChecks(UserDetails userDetails, UsernameP
setAuthenticationMethod(authentication, CLIENT_AUTH_NONE);
break;
} else if (isPrivateKeyJwt(authentication.getDetails())) {
setAuthenticationMethod(authentication, CLIENT_AUTH_PRIVATE_KEY_JWT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question, non-blocking] Is the order important here? If so, why?

if (!validatePrivateKeyJwt(authentication.getDetails(), uaaClient)) {
error = new BadCredentialsException("Bad client_assertion type");
}
setAuthenticationMethod(authentication, CLIENT_AUTH_PRIVATE_KEY_JWT);
break;
}
} else if (ObjectUtils.isEmpty(authentication.getCredentials())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,27 +15,38 @@

import org.cloudfoundry.identity.uaa.audit.AuditEvent;
import org.cloudfoundry.identity.uaa.audit.AuditEventType;
import org.cloudfoundry.identity.uaa.authentication.UaaAuthenticationDetails;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.AuthenticationException;

public class ClientAuthenticationFailureEvent extends AbstractUaaAuthenticationEvent {

private final UaaAuthenticationDetails uaaAuthenticationDetails;
private final String clientId;
private final AuthenticationException ex;

public ClientAuthenticationFailureEvent(Authentication authentication, AuthenticationException ex, String zoneId) {
super(authentication, zoneId);
clientId = getAuthenticationDetails().getClientId();
uaaAuthenticationDetails = getAuthenticationDetails();
clientId = uaaAuthenticationDetails.getClientId();
this.ex = ex;
}

@Override
public AuditEvent getAuditEvent() {
return createAuditRecord(clientId, AuditEventType.ClientAuthenticationFailure,
getOrigin(getAuthenticationDetails()), ex.getMessage());
getOrigin(getAuthenticationDetails()), ex.getMessage(), getAuthenticationMethod(), getDetailedDescription());
}

public String getClientId() {
return clientId;
}

public String getAuthenticationMethod() {
return uaaAuthenticationDetails != null && uaaAuthenticationDetails.getAuthenticationMethod() != null ? uaaAuthenticationDetails.getAuthenticationMethod() : null;
}

public String getDetailedDescription() {
return ex != null && ex.getCause() != null ? ex.getCause().getMessage() : null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,32 @@

import org.cloudfoundry.identity.uaa.audit.AuditEvent;
import org.cloudfoundry.identity.uaa.audit.AuditEventType;
import org.cloudfoundry.identity.uaa.authentication.UaaAuthenticationDetails;
import org.springframework.security.core.Authentication;

public class ClientAuthenticationSuccessEvent extends AbstractUaaAuthenticationEvent {

private final UaaAuthenticationDetails uaaAuthenticationDetails;
private final String clientId;

public ClientAuthenticationSuccessEvent(Authentication authentication, String zoneId) {
super(authentication, zoneId);
clientId = getAuthenticationDetails().getClientId();
uaaAuthenticationDetails = getAuthenticationDetails();
clientId = uaaAuthenticationDetails.getClientId();
}

@Override
public AuditEvent getAuditEvent() {
return createAuditRecord(clientId, AuditEventType.ClientAuthenticationSuccess,
getOrigin(getAuthenticationDetails()), "Client authentication success");
getOrigin(getAuthenticationDetails()), "Client authentication success", getAuthenticationMethod(), null);
}

public String getClientId() {
return clientId;
}

public String getAuthenticationMethod() {
return uaaAuthenticationDetails != null && uaaAuthenticationDetails.getAuthenticationMethod() != null ? uaaAuthenticationDetails.getAuthenticationMethod() : null;
}

}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.cloudfoundry.identity.uaa.client;

import static java.util.Optional.ofNullable;
import static org.cloudfoundry.identity.uaa.client.ClientJwtConfiguration.JWT_CREDS;
import static org.cloudfoundry.identity.uaa.client.ClientJwtConfiguration.JWKS;
import static org.cloudfoundry.identity.uaa.client.ClientJwtConfiguration.JWKS_URI;
import static org.cloudfoundry.identity.uaa.oauth.token.TokenConstants.GRANT_TYPE_AUTHORIZATION_CODE;
Expand All @@ -20,6 +21,7 @@
import org.cloudfoundry.identity.uaa.audit.event.EntityDeletedEvent;
import org.cloudfoundry.identity.uaa.authentication.SystemAuthentication;
import org.cloudfoundry.identity.uaa.oauth.client.ClientConstants;
import org.cloudfoundry.identity.uaa.oauth.client.ClientJwtCredential;
import org.cloudfoundry.identity.uaa.provider.ClientAlreadyExistsException;
import org.cloudfoundry.identity.uaa.provider.NoSuchClientException;
import org.cloudfoundry.identity.uaa.user.UaaAuthority;
Expand Down Expand Up @@ -202,22 +204,27 @@ private void addNewClients() {
client.getAuthorizedGrantTypes().add(GRANT_TYPE_REFRESH_TOKEN);
}
for (String key : Arrays.asList("resource-ids", "scope", "authorized-grant-types", "authorities",
"redirect-uri", "secret", "id", "override", "access-token-validity",
"refresh-token-validity", "show-on-homepage", "app-launch-url", "app-icon", JWKS, JWKS_URI)) {
"redirect-uri", "secret", "id", "override", "access-token-validity", "refresh-token-validity",
"show-on-homepage", "app-launch-url", "app-icon", JWKS, JWKS_URI, JWT_CREDS)) {
info.remove(key);
}

client.setAdditionalInformation(info);

ClientJwtConfiguration keyConfig = null;
if (map.get(JWKS_URI) instanceof String || map.get(JWKS) instanceof String) {
String jwksUri = (String) map.get(JWKS_URI);
String jwks = (String) map.get(JWKS);
ClientJwtConfiguration keyConfig = ClientJwtConfiguration.parse(jwksUri, jwks);
if (keyConfig != null && keyConfig.getCleanString() != null) {
keyConfig.writeValue(client);
} else {
throw new InvalidClientDetailsException("Client jwt configuration invalid syntax. ClientID: " + client.getClientId());
keyConfig = ClientJwtConfiguration.parse(jwksUri, jwks);
writeJwtClientConfiguration(keyConfig, client);
}

if (map.get(JWT_CREDS) instanceof String jwtCredential) {
if (keyConfig == null) {
keyConfig = new ClientJwtConfiguration();
}
keyConfig.addJwtCredentials(ClientJwtCredential.parse(jwtCredential));
writeJwtClientConfiguration(keyConfig, client);
}

try {
Expand Down Expand Up @@ -251,6 +258,14 @@ private void addNewClients() {
}
}

private static void writeJwtClientConfiguration(ClientJwtConfiguration keyConfig, UaaClientDetails client) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion, non-blocking] I like re-using the method here, but I think in the case of JWT_CREDS, the validation is always true.

The is JWT credentials are parsed with ClientJwtCredential.parse ; and getCleanString() re-writes it as JSON with JsonUtils.writeValueAsString. So I don't think getCleanString() can ever fail in this case.

Therefore, I would not use getCleanString() when validating the JWT_CREDS case, and I would not modify the getCleanString() method at all - getCleanString() is really just a way of validating some config and should probably be called something different.

if (keyConfig != null && keyConfig.getCleanString() != null) {
keyConfig.writeValue(client);
} else {
throw new InvalidClientDetailsException("Client jwt configuration invalid syntax. ClientID: " + client.getClientId());
}
}

private boolean isMissingRedirectUris(UaaClientDetails client) {
return client.getRegisteredRedirectUri() == null || client.getRegisteredRedirectUri().isEmpty();
}
Expand Down
Loading
Loading