From f567a89c0901df20a792d628d7f591d5e7d1e3df Mon Sep 17 00:00:00 2001 From: "david.blasby" Date: Tue, 25 Jun 2024 09:02:45 -0700 Subject: [PATCH] [8096] Bug in AudienceAccessTokenValidator --- .../bearer/AudienceAccessTokenValidator.java | 58 ++++++++++++------- .../AudienceAccessTokenValidatorTest.java | 38 +++++++++--- 2 files changed, 66 insertions(+), 30 deletions(-) diff --git a/core/src/main/java/org/fao/geonet/kernel/security/openidconnect/bearer/AudienceAccessTokenValidator.java b/core/src/main/java/org/fao/geonet/kernel/security/openidconnect/bearer/AudienceAccessTokenValidator.java index bb379936b61..4db2c671975 100644 --- a/core/src/main/java/org/fao/geonet/kernel/security/openidconnect/bearer/AudienceAccessTokenValidator.java +++ b/core/src/main/java/org/fao/geonet/kernel/security/openidconnect/bearer/AudienceAccessTokenValidator.java @@ -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 + * @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. *

* 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). + *

+ * 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"); } diff --git a/core/src/test/java/org/fao/geonet/kernel/security/openidconnect/bearer/AudienceAccessTokenValidatorTest.java b/core/src/test/java/org/fao/geonet/kernel/security/openidconnect/bearer/AudienceAccessTokenValidatorTest.java index b64287c38db..bc7b7e7c21f 100644 --- a/core/src/test/java/org/fao/geonet/kernel/security/openidconnect/bearer/AudienceAccessTokenValidatorTest.java +++ b/core/src/test/java/org/fao/geonet/kernel/security/openidconnect/bearer/AudienceAccessTokenValidatorTest.java @@ -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); @@ -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); @@ -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); @@ -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); @@ -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); @@ -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);