Skip to content

Commit

Permalink
Avoid clients exchanging tokens using tokens issued to other clients (#…
Browse files Browse the repository at this point in the history 
…11542)
  • Loading branch information
@pedroigor
pedroigor committed Apr 20, 2022
1 parent ac79fd0 commit 76d83f4
Show file tree
Hide file tree
Showing 6 changed files with 412 additions and 56 deletions.
60 changes: 50 additions & 10 deletions services/src/main/java/org/keycloak/protocol/oidc/DefaultTokenExchangeProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ protected Response tokenExchange() {
}

String requestedSubject = formParams.getFirst(OAuth2Constants.REQUESTED_SUBJECT);
boolean disallowOnHolderOfTokenMismatch = true;

if (requestedSubject != null) {
event.detail(Details.REQUESTED_SUBJECT, requestedSubject);
UserModel requestedUser = session.users().getUserByUsername(realm, requestedSubject);
Expand All @@ -197,12 +199,11 @@ protected Response tokenExchange() {
event.detail(Details.IMPERSONATOR, tokenUser.getUsername());
// for this case, the user represented by the token, must have permission to impersonate.
AdminAuth auth = new AdminAuth(realm, token, tokenUser, client);
if (!AdminPermissions.evaluator(session, realm, auth).users().canImpersonate(requestedUser)) {
if (!AdminPermissions.evaluator(session, realm, auth).users().canImpersonate(requestedUser, client)) {
event.detail(Details.REASON, "subject not allowed to impersonate");
event.error(Errors.NOT_ALLOWED);
throw new CorsErrorResponseException(cors, OAuthErrorException.ACCESS_DENIED, "Client not allowed to exchange", Response.Status.FORBIDDEN);
}

} else {
// no token is being exchanged, this is a direct exchange. Client must be authenticated, not public, and must be allowed
// to impersonate
Expand All @@ -217,6 +218,9 @@ protected Response tokenExchange() {
event.error(Errors.NOT_ALLOWED);
throw new CorsErrorResponseException(cors, OAuthErrorException.ACCESS_DENIED, "Client not allowed to exchange", Response.Status.FORBIDDEN);
}

// see https://issues.redhat.com/browse/KEYCLOAK-5492
disallowOnHolderOfTokenMismatch = false;
}

tokenSession = session.sessions().createUserSession(realm, requestedUser, requestedUser.getUsername(), clientConnection.getRemoteAddr(), "impersonate", false, null, null);
Expand All @@ -230,7 +234,7 @@ protected Response tokenExchange() {

String requestedIssuer = formParams.getFirst(OAuth2Constants.REQUESTED_ISSUER);
if (requestedIssuer == null) {
return exchangeClientToClient(tokenUser, tokenSession);
return exchangeClientToClient(tokenUser, tokenSession, token, disallowOnHolderOfTokenMismatch);
} else {
try {
return exchangeToIdentityProvider(tokenUser, tokenSession, requestedIssuer);
Expand Down Expand Up @@ -271,7 +275,8 @@ protected Response exchangeToIdentityProvider(UserModel targetUser, UserSessionM

}

protected Response exchangeClientToClient(UserModel targetUser, UserSessionModel targetUserSession) {
protected Response exchangeClientToClient(UserModel targetUser, UserSessionModel targetUserSession,
AccessToken token, boolean disallowOnHolderOfTokenMismatch) {
String requestedTokenType = formParams.getFirst(OAuth2Constants.REQUESTED_TOKEN_TYPE);
if (requestedTokenType == null) {
requestedTokenType = OAuth2Constants.REFRESH_TOKEN_TYPE;
Expand All @@ -283,8 +288,11 @@ protected Response exchangeClientToClient(UserModel targetUser, UserSessionModel
throw new CorsErrorResponseException(cors, OAuthErrorException.INVALID_REQUEST, "requested_token_type unsupported", Response.Status.BAD_REQUEST);

}
ClientModel targetClient = client;

String audience = formParams.getFirst(OAuth2Constants.AUDIENCE);
ClientModel tokenHolder = token == null ? null : realm.getClientByClientId(token.getIssuedFor());
ClientModel targetClient = client;

if (audience != null) {
targetClient = realm.getClientByClientId(audience);
if (targetClient == null) {
Expand All @@ -301,10 +309,26 @@ protected Response exchangeClientToClient(UserModel targetUser, UserSessionModel
throw new CorsErrorResponseException(cors, OAuthErrorException.INVALID_CLIENT, "Client requires user consent", Response.Status.BAD_REQUEST);
}

if (!targetClient.equals(client) && !AdminPermissions.management(session, realm).clients().canExchangeTo(client, targetClient)) {
event.detail(Details.REASON, "client not allowed to exchange to audience");
event.error(Errors.NOT_ALLOWED);
throw new CorsErrorResponseException(cors, OAuthErrorException.ACCESS_DENIED, "Client not allowed to exchange", Response.Status.FORBIDDEN);
boolean isClientTheAudience = client.equals(targetClient);

if (isClientTheAudience) {
if (client.isPublicClient()) {
// public clients can only exchange on to themselves if they are the token holder
forbiddenIfClientIsNotTokenHolder(disallowOnHolderOfTokenMismatch, tokenHolder);
} else if (!client.equals(tokenHolder)) {
// confidential clients can only exchange to themselves if they are within the token audience
forbiddenIfClientIsNotWithinTokenAudience(token, tokenHolder);
}
} else {
if (client.isPublicClient()) {
// public clients can not exchange tokens from other client
forbiddenIfClientIsNotTokenHolder(disallowOnHolderOfTokenMismatch, tokenHolder);
}
if (!AdminPermissions.management(session, realm).clients().canExchangeTo(client, targetClient)) {
event.detail(Details.REASON, "client not allowed to exchange to audience");
event.error(Errors.NOT_ALLOWED);
throw new CorsErrorResponseException(cors, OAuthErrorException.ACCESS_DENIED, "Client not allowed to exchange", Response.Status.FORBIDDEN);
}
}

String scope = formParams.getFirst(OAuth2Constants.SCOPE);
Expand All @@ -320,6 +344,22 @@ protected Response exchangeClientToClient(UserModel targetUser, UserSessionModel
throw new CorsErrorResponseException(cors, OAuthErrorException.INVALID_REQUEST, "requested_token_type unsupported", Response.Status.BAD_REQUEST);
}

private void forbiddenIfClientIsNotWithinTokenAudience(AccessToken token, ClientModel tokenHolder) {

This comment has been minimized.

Sign in to view

Copy link
@brunomedeirosdedalus

brunomedeirosdedalus Jul 18, 2022

Contributor

The parameter tokenHolder can be removed
if (token != null && !token.hasAudience(client.getClientId())) {
event.detail(Details.REASON, "client is not within the token audience");
event.error(Errors.NOT_ALLOWED);
throw new CorsErrorResponseException(cors, OAuthErrorException.ACCESS_DENIED, "Client is not within the token audience", Response.Status.FORBIDDEN);
}
}

private void forbiddenIfClientIsNotTokenHolder(boolean disallowOnHolderOfTokenMismatch, ClientModel tokenHolder) {
if (disallowOnHolderOfTokenMismatch && !client.equals(tokenHolder)) {
event.detail(Details.REASON, "client is not the token holder");
event.error(Errors.NOT_ALLOWED);
throw new CorsErrorResponseException(cors, OAuthErrorException.ACCESS_DENIED, "Client is not the holder of the token", Response.Status.FORBIDDEN);
}
}

protected Response exchangeClientToOIDCClient(UserModel targetUser, UserSessionModel targetUserSession, String requestedTokenType,
ClientModel targetClient, String audience, String scope) {
RootAuthenticationSessionModel rootAuthSession = new AuthenticationSessionManager(session).createAuthenticationSession(realm, false);
Expand Down Expand Up @@ -457,7 +497,7 @@ protected Response exchangeExternalToken(String issuer, String subjectToken) {
userSession.setNote(IdentityProvider.EXTERNAL_IDENTITY_PROVIDER, externalIdpModel.get().getAlias());
userSession.setNote(IdentityProvider.FEDERATED_ACCESS_TOKEN, subjectToken);

return exchangeClientToClient(user, userSession);
return exchangeClientToClient(user, userSession, null, false);
}

protected UserModel importUserFromExternalIdentity(BrokeredIdentityContext context) {
Expand Down
75 changes: 36 additions & 39 deletions ...es/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,50 +306,47 @@ public Map<String, String> getPermissions(ClientModel client) {
@Override
public boolean canExchangeTo(ClientModel authorizedClient, ClientModel to) {

if (!authorizedClient.equals(to)) {
ResourceServer server = resourceServer(to);
if (server == null) {
logger.debug("No resource server set up for target client");
return false;
}
ResourceServer server = resourceServer(to);
if (server == null) {
logger.debug("No resource server set up for target client");
return false;
}

Resource resource = authz.getStoreFactory().getResourceStore().findByName(server, getResourceName(to));
if (resource == null) {
logger.debug("No resource object set up for target client");
return false;
}
Resource resource = authz.getStoreFactory().getResourceStore().findByName(server, getResourceName(to));
if (resource == null) {
logger.debug("No resource object set up for target client");
return false;
}

Policy policy = authz.getStoreFactory().getPolicyStore().findByName(server, getExchangeToPermissionName(to));
if (policy == null) {
logger.debug("No permission object set up for target client");
return false;
}
Policy policy = authz.getStoreFactory().getPolicyStore().findByName(server, getExchangeToPermissionName(to));
if (policy == null) {
logger.debug("No permission object set up for target client");
return false;
}

Set<Policy> associatedPolicies = policy.getAssociatedPolicies();
// if no policies attached to permission then just do default behavior
if (associatedPolicies == null || associatedPolicies.isEmpty()) {
logger.debug("No policies set up for permission on target client");
return false;
}
Set<Policy> associatedPolicies = policy.getAssociatedPolicies();
// if no policies attached to permission then just do default behavior
if (associatedPolicies == null || associatedPolicies.isEmpty()) {
logger.debug("No policies set up for permission on target client");
return false;
}

Scope scope = exchangeToScope(server);
if (scope == null) {
logger.debug(TOKEN_EXCHANGE + " not initialized");
return false;
Scope scope = exchangeToScope(server);
if (scope == null) {
logger.debug(TOKEN_EXCHANGE + " not initialized");
return false;
}
ClientModelIdentity identity = new ClientModelIdentity(session, authorizedClient);
EvaluationContext context = new DefaultEvaluationContext(identity, session) {
@Override
public Map<String, Collection<String>> getBaseAttributes() {
Map<String, Collection<String>> attributes = super.getBaseAttributes();
attributes.put("kc.client.id", Arrays.asList(authorizedClient.getClientId()));
return attributes;
}
ClientModelIdentity identity = new ClientModelIdentity(session, authorizedClient);
EvaluationContext context = new DefaultEvaluationContext(identity, session) {
@Override
public Map<String, Collection<String>> getBaseAttributes() {
Map<String, Collection<String>> attributes = super.getBaseAttributes();
attributes.put("kc.client.id", Arrays.asList(authorizedClient.getClientId()));
return attributes;
}

};
return root.evaluatePermission(resource, server, context, scope);
}
return true;

};
return root.evaluatePermission(resource, server, context, scope);
}


Expand Down
5 changes: 3 additions & 2 deletions .../main/java/org/keycloak/services/resources/admin/permissions/UserPermissionEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.keycloak.services.resources.admin.permissions;

import org.keycloak.models.ClientModel;
import org.keycloak.models.UserModel;

import java.util.Map;
Expand All @@ -40,8 +41,8 @@ public interface UserPermissionEvaluator {

void requireImpersonate(UserModel user);
boolean canImpersonate();
boolean canImpersonate(UserModel user);
boolean isImpersonatable(UserModel user);
boolean canImpersonate(UserModel user, ClientModel requester);
boolean isImpersonatable(UserModel user, ClientModel requester);

Map<String, Boolean> getAccess(UserModel user);

Expand Down
26 changes: 22 additions & 4 deletions ...ices/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@

import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
Expand Down Expand Up @@ -355,16 +356,20 @@ public Map<String, Collection<String>> getBaseAttributes() {
}

@Override
public boolean canImpersonate(UserModel user) {
public boolean canImpersonate(UserModel user, ClientModel requester) {
if (!canImpersonate()) {
return false;
}

return isImpersonatable(user);
return isImpersonatable(user, requester);
}

private boolean canImpersonate(UserModel user) {
return canImpersonate(user, null);
}

@Override
public boolean isImpersonatable(UserModel user) {
public boolean isImpersonatable(UserModel user, ClientModel requester) {
ResourceServer server = root.realmResourceServer();

if (server == null) {
Expand All @@ -389,7 +394,20 @@ public boolean isImpersonatable(UserModel user) {
return true;
}

return hasPermission(new DefaultEvaluationContext(new UserModelIdentity(root.realm, user), session), USER_IMPERSONATED_SCOPE);
Map<String, List<String>> additionalClaims = Collections.emptyMap();

if (requester != null) {
// make sure the requesting client id is available from the context as we are using a user identity that does not rely on token claims
additionalClaims = new HashMap<>();
additionalClaims.put("kc.client.id", Arrays.asList(requester.getClientId()));
}

return hasPermission(new DefaultEvaluationContext(new UserModelIdentity(root.realm, user), additionalClaims, session), USER_IMPERSONATED_SCOPE);
}

@Override
public boolean isImpersonatable(UserModel user) {
return isImpersonatable(user, null);
}

@Override
Expand Down
5 changes: 4 additions & 1 deletion ...egration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,10 @@ public AccessTokenResponse doTokenExchange(String realm, String token, String ta
parameters.add(new BasicNameValuePair(OAuth2Constants.GRANT_TYPE, OAuth2Constants.TOKEN_EXCHANGE_GRANT_TYPE));
parameters.add(new BasicNameValuePair(OAuth2Constants.SUBJECT_TOKEN, token));
parameters.add(new BasicNameValuePair(OAuth2Constants.SUBJECT_TOKEN_TYPE, OAuth2Constants.ACCESS_TOKEN_TYPE));
parameters.add(new BasicNameValuePair(OAuth2Constants.AUDIENCE, targetAudience));

if (targetAudience != null) {
parameters.add(new BasicNameValuePair(OAuth2Constants.AUDIENCE, targetAudience));
}

if (additionalParams != null) {
for (Map.Entry<String, String> entry : additionalParams.entrySet()) {
Expand Down
Loading

0 comments on commit 76d83f4

Please sign in to comment.