From 0cee5ee7991a915995ece160dd2d8cfaddd55b77 Mon Sep 17 00:00:00 2001 From: Jeremy Coffield Date: Mon, 1 May 2017 10:03:21 -0700 Subject: [PATCH] Added support for ignoring all unknown key types [#143216503] https://www.pivotaltracker.com/story/show/143216503 Signed-off-by: Bharath Sekar --- .../uaa/oauth/jwk/JsonWebKeyDeserializer.java | 5 ++ .../identity/uaa/oauth/jwk/JsonWebKeySet.java | 10 ++-- .../oauth/jwt/ChainedSignatureVerifier.java | 2 +- .../uaa/oauth/jwk/JsonWebKeySetTests.java | 52 +++++++++++++++++++ .../uaa/oauth/jwk/RsaJsonWebKeyTests.java | 4 +- .../jwt/ChainedSignatureVerifierTests.java | 30 ++++++++--- 6 files changed, 85 insertions(+), 18 deletions(-) diff --git a/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeyDeserializer.java b/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeyDeserializer.java index c44d171f67e..cd29cd4c7ad 100644 --- a/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeyDeserializer.java +++ b/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeyDeserializer.java @@ -23,6 +23,7 @@ import org.cloudfoundry.identity.uaa.util.JsonUtils; import java.io.IOException; +import java.util.Arrays; /** * See https://tools.ietf.org/html/rfc7517 @@ -31,6 +32,10 @@ public class JsonWebKeyDeserializer extends JsonDeserializer { @Override public JsonWebKey deserialize(JsonParser p, DeserializationContext ctxt) throws IOException, JsonProcessingException { JsonNode node = JsonUtils.readTree(p); + String kty = node.get("kty").asText("Unknown"); + if(Arrays.stream(JsonWebKey.KeyType.values()).noneMatch(knownKeyType -> knownKeyType.name().equals(kty))) { + return null; + } return new JsonWebKey(JsonUtils.getNodeAsMap(node)); } } diff --git a/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeySet.java b/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeySet.java index 7a6ae991a81..7bf774c053e 100644 --- a/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeySet.java +++ b/model/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeySet.java @@ -16,10 +16,7 @@ package org.cloudfoundry.identity.uaa.oauth.jwk; import com.fasterxml.jackson.annotation.JsonProperty; -import com.fasterxml.jackson.core.type.TypeReference; -import org.cloudfoundry.identity.uaa.util.JsonUtils; -import java.util.Arrays; import java.util.Collections; import java.util.LinkedHashSet; import java.util.LinkedList; @@ -36,9 +33,10 @@ public class JsonWebKeySet { public JsonWebKeySet(@JsonProperty("keys") List keys) { Set set = new LinkedHashSet<>(); //rules for how to override duplicates - for (T t : keys) { - set.remove(t); - set.add(t); + for (T key : keys) { + if(key == null) continue; + set.remove(key); + set.add(key); } this.keys = new LinkedList(set); } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwt/ChainedSignatureVerifier.java b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwt/ChainedSignatureVerifier.java index 06ad4ab99aa..1287ba61cea 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwt/ChainedSignatureVerifier.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/oauth/jwt/ChainedSignatureVerifier.java @@ -28,7 +28,7 @@ public class ChainedSignatureVerifier implements SignatureVerifier { private final List delegates; public ChainedSignatureVerifier(JsonWebKeySet keys) { - if(keys == null || keys.getKeys() == null) { + if(keys == null || keys.getKeys() == null || keys.getKeys().isEmpty()) { throw new IllegalArgumentException("keys cannot be null or empty"); } List ds = new ArrayList<>(keys.getKeys().size()); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeySetTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeySetTests.java index 240e74387a8..7c8eb633454 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeySetTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwk/JsonWebKeySetTests.java @@ -37,6 +37,16 @@ public class JsonWebKeySetTests { " \"value\": \"-----BEGIN PUBLIC KEY-----\\nMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDHFr+KICms+tuT1OXJwhCUmR2d\\nKVy7psa8xzElSyzqx7oJyfJ1JZyOzToj9T5SfTIq396agbHJWVfYphNahvZ/7uMX\\nqHxf+ZH9BL1gk9Y6kCnbM5R60gfwjyW1/dQPjOzn9N394zd2FJoFHwdq9Qs0wBug\\nspULZVNRxq7veq/fzwIDAQAB\\n-----END PUBLIC KEY-----\"\n" + "}"; + public static final String unknownKeyJson = "{\n" + + " \"alg\": \"RS256\",\n" + + " \"e\": \"AQAB\",\n" + + " \"kid\": \"legacy\",\n" + + " \"kty\": \"GARBAGE\",\n" + + " \"n\": \"AMcWv4ogKaz625PU5cnCEJSZHZ0pXLumxrzHMSVLLOrHugnJ8nUlnI7NOiP1PlJ9Mirf3pqBsclZV9imE1qG9n_u4xeofF_5kf0EvWCT1jqQKdszlHrSB_CPJbX91A-M7Of03f3jN3YUmgUfB2r1CzTAG6CylQtlU1HGru96r9_P\",\n" + + " \"use\": \"sig\",\n" + + " \"value\": \"-----BEGIN PUBLIC KEY-----\\nMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDHFr+KICms+tuT1OXJwhCUmR2d\\nKVy7psa8xzElSyzqx7oJyfJ1JZyOzToj9T5SfTIq396agbHJWVfYphNahvZ/7uMX\\nqHxf+ZH9BL1gk9Y6kCnbM5R60gfwjyW1/dQPjOzn9N394zd2FJoFHwdq9Qs0wBug\\nspULZVNRxq7veq/fzwIDAQAB\\n-----END PUBLIC KEY-----\"\n" + + "}"; + public static final String multiKeyJson = "{\n" + " \"keys\": [\n" + " {\n" + @@ -67,6 +77,36 @@ public class JsonWebKeySetTests { " ]\n" + "}"; + public static final String someUnknownKeysJson = "{\n" + + " \"keys\": [\n" + + " {\n" + + " \"alg\": \"RS256\",\n" + + " \"e\": \"AQAB\",\n" + + " \"kid\": \"legacy\",\n" + + " \"kty\": \"RSA\",\n" + + " \"n\": \"AMcWv4ogKaz625PU5cnCEJSZHZ0pXLumxrzHMSVLLOrHugnJ8nUlnI7NOiP1PlJ9Mirf3pqBsclZV9imE1qG9n_u4xeofF_5kf0EvWCT1jqQKdszlHrSB_CPJbX91A-M7Of03f3jN3YUmgUfB2r1CzTAG6CylQtlU1HGru96r9_P\",\n" + + " \"use\": \"sig\",\n" + + " \"value\": \"-----BEGIN PUBLIC KEY-----\\nMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDHFr+KICms+tuT1OXJwhCUmR2d\\nKVy7psa8xzElSyzqx7oJyfJ1JZyOzToj9T5SfTIq396agbHJWVfYphNahvZ/7uMX\\nqHxf+ZH9BL1gk9Y6kCnbM5R60gfwjyW1/dQPjOzn9N394zd2FJoFHwdq9Qs0wBug\\nspULZVNRxq7veq/fzwIDAQAB\\n-----END PUBLIC KEY-----\"\n" + + " },\n" + + " {\n" + + " \"alg\": \"RS256\",\n" + + " \"e\": \"AQAB\",\n" + + " \"kid\": \"legacy\",\n" + + " \"kty\": \"UNKNOWN1\",\n" + + " \"n\": \"AMcWv4ogKaz625PU5cnCEJSZHZ0pXLumxrzHMSVLLOrHugnJ8nUlnI7NOiP1PlJ9Mirf3pqBsclZV9imE1qG9n_u4xeofF_5kf0EvWCT1jqQKdszlHrSB_CPJbX91A-M7Of03f3jN3YUmgUfB2r1CzTAG6CylQtlU1HGru96r9_P\",\n" + + " \"use\": \"sig\",\n" + + " \"value\": \"-----BEGIN PUBLIC KEY-----\\nMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDHFr+KICms+tuT1OXJwhCUmR2d\\nKVy7psa8xzElSyzqx7oJyfJ1JZyOzToj9T5SfTIq396agbHJWVfYphNahvZ/7uMX\\nqHxf+ZH9BL1gk9Y6kCnbM5R60gfwjyW1/dQPjOzn9N394zd2FJoFHwdq9Qs0wBug\\nspULZVNRxq7veq/fzwIDAQAB\\n-----END PUBLIC KEY-----\"\n" + + " },\n" + + " {\n" + + " \"alg\": \"HMACSHA256\",\n" + + " \"k\": \"test-mac-key\",\n" + + " \"kid\": \"mac-id\",\n" + + " \"kty\": \"UNKNOWN2\",\n" + + " \"key_ops\": [\"sign\",\"verify\"]\n" + + " }\n" + + " ]\n" + + "}"; + @Test public void test_multi_key() { @@ -108,4 +148,16 @@ public JsonWebKeySet test_key(String json) { return keys; } + @Test + public void testUnknownKeyType() { + JsonWebKeySet keys = JsonWebKeyHelper.deserialize(unknownKeyJson); + assertEquals(0, keys.getKeys().size()); + } + + @Test + public void testIgnoreUnknownKeyTypes() { + JsonWebKeySet keys = JsonWebKeyHelper.deserialize(someUnknownKeysJson); + assertEquals(1, keys.getKeys().size()); + } + } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwk/RsaJsonWebKeyTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwk/RsaJsonWebKeyTests.java index 84380e826c2..46f854eb288 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwk/RsaJsonWebKeyTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwk/RsaJsonWebKeyTests.java @@ -127,8 +127,6 @@ public void ensure_that_duplicates_get_the_last_object() { public void test_required_properties() { Map map = new HashMap(); test_create_with_error(map); - map.put("kty", "invalid"); - test_create_with_error(map); map.put("kty", "RSA"); new VerificationKeyResponse(map); } @@ -231,4 +229,4 @@ public JsonWebKeySet deserialize_azure_keys(String json) { " ]\n" + "}"; -} \ No newline at end of file +} diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwt/ChainedSignatureVerifierTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwt/ChainedSignatureVerifierTests.java index 1d68aa5c1ea..459e8067bbc 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwt/ChainedSignatureVerifierTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/oauth/jwt/ChainedSignatureVerifierTests.java @@ -18,6 +18,7 @@ import org.cloudfoundry.identity.uaa.oauth.jwk.JsonWebKey; import org.cloudfoundry.identity.uaa.oauth.jwk.JsonWebKeyHelper; import org.cloudfoundry.identity.uaa.oauth.jwk.JsonWebKeySet; +import org.cloudfoundry.identity.uaa.util.JsonUtils; import org.junit.Before; import org.junit.Test; import org.mockito.Mockito; @@ -29,18 +30,16 @@ import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import static org.cloudfoundry.identity.uaa.oauth.jwk.JsonWebKey.KeyType.*; +import static java.util.Collections.singletonMap; +import static org.cloudfoundry.identity.uaa.oauth.jwk.JsonWebKey.KeyType.MAC; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; -import static org.mockito.Matchers.anyString; public class ChainedSignatureVerifierTests { private Signer signer; @@ -147,12 +146,11 @@ public void unsupported_key_types_are_ignored() { p.put("y", "test-ec-key-y"); p.put("use", "sig"); p.put("crv", "test-crv"); - JsonWebKey ecKey = new JsonWebKey(p); Map q = new HashMap<>(); q.put("kty","oct"); q.put("k", "octkeyvalue"); - JsonWebKey octKey = new JsonWebKey(q); - verifier = new ChainedSignatureVerifier(new JsonWebKeySet<>(Arrays.asList(validKey, ecKey, octKey))); + JsonWebKeySet keySet = JsonUtils.convertValue(singletonMap("keys", Arrays.asList(validKey, p, q)), JsonWebKeySet.class); + verifier = new ChainedSignatureVerifier(keySet); List delegates = new ArrayList((List) ReflectionTestUtils.getField(verifier, verifier.getClass(), "delegates")); assertNotNull(delegates); assertEquals(1, delegates.size()); @@ -162,6 +160,22 @@ public void unsupported_key_types_are_ignored() { } } + @Test(expected = IllegalArgumentException.class) + public void no_supported_key_types_causes_error() { + Map p = new HashMap<>(); + p.put("kty","EC"); + p.put("kid", "ecid"); + p.put("x", "test-ec-key-x"); + p.put("y", "test-ec-key-y"); + p.put("use", "sig"); + p.put("crv", "test-crv"); + Map q = new HashMap<>(); + q.put("kty","oct"); + q.put("k", "octkeyvalue"); + JsonWebKeySet keySet = JsonUtils.convertValue(singletonMap("keys", Arrays.asList(p, q)), JsonWebKeySet.class); + verifier = new ChainedSignatureVerifier(keySet); + } + @Test public void test_multi_key_both_valid() { Map p = new HashMap<>(); @@ -185,4 +199,4 @@ public void test_multi_key_both_valid() { Mockito.verifyZeroInteractions(macSigner); } -} \ No newline at end of file +}