Skip to content

Commit

Permalink
[8096] Bug in AudienceAccessTokenValidator
Browse files Browse the repository at this point in the history
  • Loading branch information
@david-blasby
david-blasby committed Jun 25, 2024
1 parent 4c63865 commit f567a89
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 30 deletions.
58 changes: 37 additions & 21 deletions ...ava/org/fao/geonet/kernel/security/openidconnect/bearer/AudienceAccessTokenValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,45 +53,61 @@ public class AudienceAccessTokenValidator implements AccessTokenValidator {
@Autowired
OIDCConfiguration oidcConfiguration;

/**
* If "claim" is a String, then this will return true if claim == expected.
* If "claim" is a List, then it will return true if any of the list items == expected
*
* @param claim - could be a String or a List<String>
* @param expected
* @return
*/
public static boolean contains(Object claim, String expected) {
if (claim == null || expected == null) {
return false;
}
if (claim instanceof String) {
var claimStr = (String) claim;
return expected.equals(claimStr);
}
if (claim instanceof List) {
List claims = (List) claim;
for (Object o : claims) {
if ((o instanceof String) && (o.equals(expected))) {
return true;
}
}
}
return false;
}

/**
* "aud" must be our client id
* OR "azp" must be our client id (or, if its a list, contain our client id)
* OR "appid" must be our client id.
* <p>
* Otherwise, its a token not for us...
*
* This checks that the audience of the JWT access token is us.
* The main attack this tries to prevent is someone getting an access token (i.e. from keycloak or azure) that
* was meant for another application (say a silly calendar app), and then using that token here. The IDP provider
* (keycloak/azure) will validate the token as "good", but it wasn't generated for us. This does a check of the
* token that OUR client ID is mentioned (not another app).
* <p>
* This checks that the audience of the JWT access token is us.
* The main attack this tries to prevent is someone getting an access token (i.e. from keycloak or azure) that
* was meant for another application (say a silly calendar app), and then using that token here. The IDP provider
* (keycloak/azure) will validate the token as "good", but it wasn't generated for us. This does a check of the
* token that OUR client ID is mentioned (not another app).
*/
@Override
public void verifyToken(Map claimsJWT, Map userInfoClaims) throws Exception {
if ((claimsJWT.get(AUDIENCE_CLAIM_NAME) != null)
&& claimsJWT.get(AUDIENCE_CLAIM_NAME).equals(oidcConfiguration.getClientId())) {
&& contains(claimsJWT.get(AUDIENCE_CLAIM_NAME), oidcConfiguration.getClientId())) {
return;
}

if ((claimsJWT.get(APPID_CLAIM_NAME) != null)
&& claimsJWT.get(APPID_CLAIM_NAME).equals(oidcConfiguration.getClientId())) {
&& contains(claimsJWT.get(APPID_CLAIM_NAME), oidcConfiguration.getClientId())) {
return; //azure specific
}

//azp - keycloak
Object azp = claimsJWT.get(KEYCLOAK_AUDIENCE_CLAIM_NAME);
if (azp != null) {
if (azp instanceof String) {
if (((String) azp).equals(oidcConfiguration.getClientId()))
return;
} else if (azp instanceof List) {
List azps = (List) azp;
for (Object o : azps) {
if ((o instanceof String) && (o.equals(oidcConfiguration.getClientId()))) {
return;
}
}
}
if (contains(claimsJWT.get(KEYCLOAK_AUDIENCE_CLAIM_NAME), oidcConfiguration.getClientId())) {
return;
}
throw new Exception("JWT Bearer token - probably not meant for this application");
}
Expand Down
38 changes: 29 additions & 9 deletions ...org/fao/geonet/kernel/security/openidconnect/bearer/AudienceAccessTokenValidatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,29 +26,49 @@
import org.fao.geonet.kernel.security.openidconnect.OIDCConfiguration;
import org.junit.Test;

import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

public class AudienceAccessTokenValidatorTest {


String clientId = "MYCLIENTID";




public AudienceAccessTokenValidator getValidator(){
public AudienceAccessTokenValidator getValidator() {
AudienceAccessTokenValidator validator = new AudienceAccessTokenValidator();
validator.oidcConfiguration = new OIDCConfiguration();
validator.oidcConfiguration.setClientId(clientId);
return validator;
}

@Test
public void testContainsGood() {
assertTrue(AudienceAccessTokenValidator.contains("dave", "dave"));
assertTrue(AudienceAccessTokenValidator.contains(Arrays.asList("dave"), "dave"));
assertTrue(AudienceAccessTokenValidator.contains(Arrays.asList("dave", "ted"), "dave"));
assertTrue(AudienceAccessTokenValidator.contains(Arrays.asList("ted", "dave"), "dave"));
}


@Test
public void testContainsBad() {
assertFalse(AudienceAccessTokenValidator.contains("ted", "dave"));
assertFalse(AudienceAccessTokenValidator.contains("", "dave"));
assertFalse(AudienceAccessTokenValidator.contains(Arrays.asList(), "dave"));
assertFalse(AudienceAccessTokenValidator.contains(Arrays.asList("ted"), "dave"));
assertFalse(AudienceAccessTokenValidator.contains(Arrays.asList("pam", "ted"), "dave"));
}


@Test
public void testAzureGood() throws Exception {
Map claims = new HashMap();
claims.put("appid",clientId);
claims.put("appid", clientId);

AudienceAccessTokenValidator validator = getValidator();
validator.verifyToken(claims, null);
Expand All @@ -57,7 +77,7 @@ public void testAzureGood() throws Exception {
@Test
public void testKeyCloakGood() throws Exception {
Map claims = new HashMap();
claims.put("azp",clientId);
claims.put("azp", clientId);

AudienceAccessTokenValidator validator = getValidator();
validator.verifyToken(claims, null);
Expand All @@ -66,7 +86,7 @@ public void testKeyCloakGood() throws Exception {
@Test
public void testSelfCreatedGood() throws Exception {
Map claims = new HashMap();
claims.put("aud",clientId);
claims.put("aud", clientId);

AudienceAccessTokenValidator validator = getValidator();
validator.verifyToken(claims, null);
Expand All @@ -75,7 +95,7 @@ public void testSelfCreatedGood() throws Exception {
@Test(expected = Exception.class)
public void testBad1() throws Exception {
Map claims = new HashMap();
claims.put("aud","badaud");
claims.put("aud", "badaud");

AudienceAccessTokenValidator validator = getValidator();
validator.verifyToken(claims, null);
Expand All @@ -84,7 +104,7 @@ public void testBad1() throws Exception {
@Test(expected = Exception.class)
public void testBad2() throws Exception {
Map claims = new HashMap();
claims.put("azp","badaud");
claims.put("azp", "badaud");

AudienceAccessTokenValidator validator = getValidator();
validator.verifyToken(claims, null);
Expand All @@ -94,7 +114,7 @@ public void testBad2() throws Exception {
@Test(expected = Exception.class)
public void testBad3() throws Exception {
Map claims = new HashMap();
claims.put("appid","badaud");
claims.put("appid", "badaud");

AudienceAccessTokenValidator validator = getValidator();
validator.verifyToken(claims, null);
Expand Down

0 comments on commit f567a89

Please sign in to comment.