From 69c3a3614a8a9cca41d57d7673753b64dd4b0216 Mon Sep 17 00:00:00 2001 From: Paul Millar Date: Sun, 21 Apr 2024 09:20:20 +0200 Subject: [PATCH] gplazma: oidc more descriptive offline verification failures Motivation: OIDC uses caches to record the OIDC discover document and the issuer's public keys (for offline varification). Currently, if there is a problem with the discovery document or with the JWKS document then those problems are logged when the document is loaded, but subsequent attempts for offline verification of a token fail with an opaque and cryptic message. Modification: Update Result to support additional (Option-like) methods: `map` and `flatMap`. Introduce a new MissingNode class (ReasonBearingMissingNode) that can carry the reason why a JSON node is missing. Update discovery document fetching to take advantage of this new class. Update MemoriseMapWithExpiry to work on Results rather than Maps. The class is renamed to reflect this change. Issuer is updated to work with Result objects rather than Optional objects. These Result objects, if representing a failure, describe why the operation failed. This is used to generate a more descriptive AuthenticationException message. Suppress hamcrest-core from junit as this brings in an ancient version. Result: During offline verification, the details regarding any failure to fetch documents from the issuer or to parse the issuer-supplied documents are now used to build the error message when the plugin rejects a token. Target: master Requires-notes: yes Requires-book: no Closes: #7553 --- .../src/main/java/org/dcache/util/Result.java | 45 ++- .../test/java/org/dcache/util/ResultTest.java | 144 ++++++- .../dcache/gplazma/oidc/IdentityProvider.java | 5 +- .../helpers/ReasonBearingMissingNode.java | 103 +++++ .../org/dcache/gplazma/oidc/jwt/Issuer.java | 121 ++++-- ...piry.java => MemoizeResultWithExpiry.java} | 46 +-- .../oidc/MockIdentityProviderBuilder.java | 14 + .../helpers/ReasonBearingMissingNodeTest.java | 133 +++++++ .../dcache/gplazma/oidc/jwt/IssuerTest.java | 372 ++++++++++++++++++ pom.xml | 6 + 10 files changed, 924 insertions(+), 65 deletions(-) create mode 100644 modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/helpers/ReasonBearingMissingNode.java rename modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/{MemoizeMapWithExpiry.java => MemoizeResultWithExpiry.java} (55%) create mode 100644 modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/helpers/ReasonBearingMissingNodeTest.java diff --git a/modules/common/src/main/java/org/dcache/util/Result.java b/modules/common/src/main/java/org/dcache/util/Result.java index 905bd204410..f3d547bf335 100644 --- a/modules/common/src/main/java/org/dcache/util/Result.java +++ b/modules/common/src/main/java/org/dcache/util/Result.java @@ -1,6 +1,6 @@ /* dCache - http://www.dcache.org/ * - * Copyright (C) 2022 Deutsches Elektronen-Synchrotron + * Copyright (C) 2022-2024 Deutsches Elektronen-Synchrotron * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as @@ -48,6 +48,49 @@ public abstract class Result { */ public abstract C map(Function mapFromSuccess, Function mapFromFailure); + /** + * Convert a successful result to another type. This is equivalent to + * {@link Optional#map(java.util.function.Function)}. If this Result is + * a failure then that failure value is returned, otherwise this Result's + * success value is passed to mapSuccess and a new successful + * Result is returned with the value returned by mapSuccess. + *

+ * In effect, this processes a successful result using a procedure that + * cannot fail. + *

+ * Note: this is really just syntactical sugar over + * {@link #map(java.util.function.Function, java.util.function.Function) }, + * but it helps maintain readability. + * @param The new successful type + * @param mapSuccess The function that maps the successful result. + */ + public Result map(Function mapSuccess) { + return map(s -> Result.success(mapSuccess.apply(s)), Result::failure); + } + + /** + * Further process a successful result with the possibility of failure. + * This is equivalent to + * {@link Optional#flatMap(java.util.function.Function)}. If this Result is + * failure then this method returns a Result with that failure. + * If this Result is a success then the Function processSuccess is + * applied to this Result's successful result and the value returned by + * processSuccess is returned. + *

+ * In effect, this processes a successful result by a procedure that could + * fail. + *

+ * Note: this is really just syntactical sugar over + * {@link #map(java.util.function.Function, java.util.function.Function) }, + * but it helps maintain readability. + * @param The new successful type + * @param processSuccess The function that processes a successful result + * @return an updated result. + */ + public Result flatMap(Function> processSuccess) { + return map(processSuccess, Result::failure); + } + /** * Consume this result. The consumers are most likely using side-effects * (e.g., mutating an object's state or creating a log entry) to propagate diff --git a/modules/common/src/test/java/org/dcache/util/ResultTest.java b/modules/common/src/test/java/org/dcache/util/ResultTest.java index 0d563fc488c..c7ab72b28e5 100644 --- a/modules/common/src/test/java/org/dcache/util/ResultTest.java +++ b/modules/common/src/test/java/org/dcache/util/ResultTest.java @@ -1,6 +1,6 @@ /* dCache - http://www.dcache.org/ * - * Copyright (C) 2022 Deutsches Elektronen-Synchrotron + * Copyright (C) 2022-2024 Deutsches Elektronen-Synchrotron * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as @@ -17,10 +17,15 @@ */ package org.dcache.util; +import static com.github.npathai.hamcrestopt.OptionalMatchers.isEmpty; +import static com.github.npathai.hamcrestopt.OptionalMatchers.isPresentAnd; +import java.util.List; import org.junit.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; public class ResultTest { @@ -46,6 +51,19 @@ public void shouldHaveSameHashCodeWhenBothResultsAreFailuresWithSameResult() // Tests focusing on Result#equals + @Test + public void shouldEqualItself() + { + var r = Result.success("the result"); + assertThat(r, equalTo(r)); + } + + @Test + public void shouldNotEqualAnotherObject() + { + assertThat(Result.success("the result"), not(equalTo(new Object()))); + } + @Test public void shouldEqualSuccessfulResultWithEqualValueWhenSuccessful() { @@ -99,4 +117,128 @@ public void shouldNotEqualSuccessfulResultWhenFailed() assertThat(r1, is(not(equalTo(r2)))); } + + @Test + public void shouldBeSuccessfulWhenSuccess() { + Result r = Result.success("success!"); + + assertTrue(r.isSuccessful()); + } + + @Test + public void shouldNotBeFailureWhenSuccess() { + Result r = Result.success("success!"); + + assertFalse(r.isFailure()); + } + + @Test + public void shouldNotBeSuccessfulWhenFailure() { + Result r = Result.failure("it went wrong"); + + assertFalse(r.isSuccessful()); + } + + @Test + public void shouldBeFailureWhenFailure() { + Result r = Result.failure("it went wrong!"); + + assertTrue(r.isFailure()); + } + + @Test + public void shouldHaveSuccessResultWhenSuccess() { + Result r = Result.success("success!"); + + assertThat(r.getSuccess(), isPresentAnd(equalTo("success!"))); + } + + @Test + public void shouldNotHaveFailureResultWhenSuccess() { + Result r = Result.success("success!"); + + assertThat(r.getFailure(), isEmpty()); + } + + @Test + public void shouldNotHaveSuccessResultWhenFailure() { + Result r = Result.failure("it went wrong"); + + assertThat(r.getSuccess(), isEmpty()); + } + + @Test + public void shouldHaveFailureResultWhenFailure() { + Result r = Result.failure("it went wrong"); + + assertThat(r.getFailure(), isPresentAnd(equalTo("it went wrong"))); + } + + @Test + public void shouldNotThrowExceptionWhenSuccess() { + Result r = Result.success("success!"); + + r.orElseThrow(IllegalArgumentException::new); + } + + @Test(expected=IllegalArgumentException.class) + public void shouldThrowExceptionWhenFailure() { + Result r = Result.failure("it went wrong"); + + r.orElseThrow(IllegalArgumentException::new); + } + + @Test + public void shouldMapSuccess() { + Result,String> input = Result.success(List.of("1")); + + Result,String> output = input.map(s -> s.stream() + .map(Integer::valueOf) + .toList()); + + assertThat(output.getFailure(), isEmpty()); + assertThat(output.getSuccess(), isPresentAnd(contains(1))); + } + + @Test + public void shouldNotMapFailure() { + Result,String> input = Result.failure("Not an integer"); + + Result,String> output = input.map(s -> s.stream() + .map(Integer::valueOf) + .toList()); + + assertThat(output.getSuccess(), isEmpty()); + assertThat(output.getFailure(), isPresentAnd(equalTo("Not an integer"))); + } + + @Test + public void shouldNotFlatMapFailure() { + Result,String> input = Result.failure("Not an integer"); + + Result,String> output = input.flatMap(s -> Result.success(List.of(1))); + + assertThat(output.getSuccess(), isEmpty()); + assertThat(output.getFailure(), isPresentAnd(equalTo("Not an integer"))); + } + + @Test + public void shouldFlatMapToSuccess() { + Result,String> input = Result.success(List.of("1")); + + Result,String> output = input.flatMap(s -> Result.success(List.of(1))); + + assertThat(output.getFailure(), isEmpty()); + assertThat(output.getSuccess(), isPresentAnd(contains(1))); + } + + @Test + public void shouldFlatMapToFailure() { + Result,String> input = Result.success(List.of("1")); + + Result,String> output = input.flatMap(s -> Result.failure("invalid value")); + + assertThat(output.getSuccess(), isEmpty()); + assertThat(output.getFailure(), isPresentAnd(equalTo("invalid value"))); + } } diff --git a/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/IdentityProvider.java b/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/IdentityProvider.java index 500520fd2c2..781adb9e39e 100644 --- a/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/IdentityProvider.java +++ b/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/IdentityProvider.java @@ -1,7 +1,7 @@ /* * dCache - http://www.dcache.org/ * - * Copyright (C) 2018 Deutsches Elektronen-Synchrotron + * Copyright (C) 2018-2024 Deutsches Elektronen-Synchrotron * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as @@ -36,6 +36,7 @@ import java.util.Map; import java.util.Optional; import org.apache.http.client.HttpClient; +import org.dcache.gplazma.oidc.helpers.ReasonBearingMissingNode; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -126,7 +127,7 @@ public synchronized JsonNode discoveryDocument() { nextDiscoveryFetch = now.plus(cacheDurationWhenSuccessful); } catch (IOException e) { LOGGER.warn("Failed to fetch discovery document for {}: {}", name, e.toString()); - discoveryDocument = MissingNode.getInstance(); + discoveryDocument = new ReasonBearingMissingNode(e.toString()); nextDiscoveryFetch = now.plus(CACHE_DURATION_WHEN_UNSUCCESSFUL); } } diff --git a/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/helpers/ReasonBearingMissingNode.java b/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/helpers/ReasonBearingMissingNode.java new file mode 100644 index 00000000000..b541d80efa7 --- /dev/null +++ b/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/helpers/ReasonBearingMissingNode.java @@ -0,0 +1,103 @@ +/* dCache - http://www.dcache.org/ + * + * Copyright (C) 2024 Deutsches Elektronen-Synchrotron + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package org.dcache.gplazma.oidc.helpers; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.core.JsonToken; +import com.fasterxml.jackson.databind.SerializerProvider; +import com.fasterxml.jackson.databind.node.JsonNodeType; +import com.fasterxml.jackson.databind.node.MissingNode; +import com.fasterxml.jackson.databind.node.ValueNode; +import java.io.IOException; +import java.util.Objects; + +/** + * This is like Jackson's MissingNode but it carries a reason why the + * node is missing. Unfortunately, MissingNode is declared final, so + * we cannot simply extend it. + */ +public class ReasonBearingMissingNode extends ValueNode { + + private final String reason; + + public ReasonBearingMissingNode(String reason) { + this.reason = Objects.requireNonNull(reason); + } + + public String getReason() { + return reason; + } + + @Override + public final void serialize(JsonGenerator g, SerializerProvider provider) + throws IOException + { + g.writeNull(); + } + + @Override + public JsonToken asToken() { + return JsonToken.NOT_AVAILABLE; + } + + @Override + public int hashCode() { + return reason.hashCode(); + } + + @Override + public JsonNodeType getNodeType() { + return JsonNodeType.MISSING; + } + + @Override + public boolean isMissingNode() { + return true; + } + + @Override + public String asText() { + return ""; + } + + @Override + public String toString() { + return ""; + } + + @Override + public String toPrettyString() { + return ""; + } + + + @Override + public String asText(String defaultValue) { + return defaultValue; + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof ReasonBearingMissingNode)) { + return false; + } + + ReasonBearingMissingNode o = (ReasonBearingMissingNode)other; + return reason.equals(o.reason); + } +} diff --git a/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/Issuer.java b/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/Issuer.java index 219bf22dee3..27528a51169 100644 --- a/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/Issuer.java +++ b/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/Issuer.java @@ -1,6 +1,6 @@ /* dCache - http://www.dcache.org/ * - * Copyright (C) 2019 - 2022 Deutsches Elektronen-Synchrotron + * Copyright (C) 2019 - 2024 Deutsches Elektronen-Synchrotron * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as @@ -33,18 +33,20 @@ import java.security.spec.RSAPublicKeySpec; import java.time.Duration; import java.util.Base64; -import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Optional; import java.util.Queue; import java.util.UUID; import java.util.function.Supplier; +import java.util.stream.Collectors; import org.apache.http.client.HttpClient; import org.dcache.gplazma.AuthenticationException; +import org.dcache.gplazma.oidc.helpers.ReasonBearingMissingNode; import org.dcache.gplazma.oidc.HttpClientUtils; import org.dcache.gplazma.oidc.IdentityProvider; import org.dcache.gplazma.util.JsonWebToken; +import org.dcache.util.Result; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -64,10 +66,16 @@ public class Issuer { // Recommendation for six hours comes from this document: // https://doi.org/10.5281/zenodo.3460258 - private final Supplier> keys = MemoizeMapWithExpiry.memorize(this::readJwksDocument) - .whenEmptyFor(Duration.ofMinutes(1)) - .whenNonEmptyFor(Duration.ofHours(6)) - .build(); + // REVISIT: the cache duration depends only whether the JWKS document may + // be fetched, is a JSON object with "keys" pointing to an array. If these + // steps are successful and one (or more) of the array items are malformed + // then we will cache the result for withSuccessFor duration. + // This behaviour may be suboptimal. + private final Supplier>,String>> keys = + MemoizeResultWithExpiry.memorize(this::readJwksDocument) + .whenFailureFor(Duration.ofMinutes(1)) + .whenSuccessFor(Duration.ofHours(6)) + .build(); public Issuer(HttpClient client, IdentityProvider provider, int tokenHistory) { this.provider = requireNonNull(provider); @@ -94,90 +102,108 @@ public String getEndpoint() { return provider.getIssuerEndpoint().toASCIIString(); } - private Optional jwksEndpoint() { + private Result jwksEndpoint() { JsonNode configuration = provider.discoveryDocument(); if (configuration.getNodeType() == JsonNodeType.MISSING) { - return Optional.empty(); + String reason = configuration instanceof ReasonBearingMissingNode + ? ((ReasonBearingMissingNode) configuration).getReason() + : "unknown problem"; + return Result.failure(reason); } JsonNode jwksNode = configuration.get("jwks_uri"); if (jwksNode == null) { LOGGER.warn("configuration does not have jwks_uri"); - return Optional.empty(); + return Result.failure("discovery document missing jwks_uri"); } if (!jwksNode.isTextual()) { LOGGER.warn("configuration has non-textual jwks_uri"); - return Optional.empty(); + return Result.failure("discovery document has non-textual jwks_uri"); } String url = jwksNode.asText(); if (url.isEmpty()) { LOGGER.warn("configuration has empty jwks_uri"); - return Optional.empty(); + return Result.failure("discovery document has empty jwks_uri"); } try { - return Optional.of(new URI(url)); + return Result.success(new URI(url)); } catch (URISyntaxException e) { LOGGER.warn("Bad jwks_uri URI \"{}\": {}", url, e.toString()); - return Optional.empty(); + return Result.failure("Bad jwks_uri URI " + url + ": " + e.toString()); } } - private Optional fetchJson(URI uri) { + private Result fetchJson(URI uri) { try { JsonNode document = HttpClientUtils.readJson(client, uri); - return Optional.of(document); + return Result.success(document); } catch (IOException e) { LOGGER.warn("Failed to fetch {}: {}", uri, e.toString()); - return Optional.empty(); + return Result.failure("Failed to fetch " + uri + ": " + e.toString()); } } - private Optional extractElement(JsonNode object, String key) { + private Result extractElement(JsonNode object, String key) { if (object.getNodeType() != JsonNodeType.OBJECT) { LOGGER.warn("Json node has wrong type: {} != OBJECT", object.getNodeType()); - return Optional.empty(); + return Result.failure("Json node has wrong type: " + object.getNodeType() + " != OBJECT"); } var element = object.get(key); if (element == null) { LOGGER.warn("JSON object is missing key \"{}\"", key); - return Optional.empty(); + return Result.failure("JSON object is missing key \"" + key + "\""); } - return Optional.of(element); + return Result.success(element); } - private Optional asArray(JsonNode node) { + private Result asArray(JsonNode node) { if (node.getNodeType() != JsonNodeType.ARRAY) { LOGGER.warn("Json node has wrong type: {} != ARRAY", node.getNodeType()); - return Optional.empty(); + return Result.failure("Json node has wrong type: " + node.getNodeType() + " != ARRAY"); } - return Optional.of(node); + return Result.success(node); } - private Map readJwksDocument() { + private Result>,String> readJwksDocument() { return jwksEndpoint() .flatMap(this::fetchJson) .flatMap(j -> this.extractElement(j, "keys")) .flatMap(this::asArray) - .map(this::parseJwksKeys) - .orElse(Collections.emptyMap()); + .map(this::parseJwksKeys); } - private Map parseJwksKeys(JsonNode keys) { - Map publicKeys = new HashMap<>(); + private Map> parseJwksKeys(JsonNode keys) { + Map> publicKeys = new HashMap<>(); for (JsonNode key : keys) { + if (!key.isObject()) { + LOGGER.debug("Ignoring JWKS \"keys\" array item that is not a JSON object."); + continue; + } + String kid; try { - String kid = getOptionalString(key, "kid").orElseGet(() -> UUID.randomUUID().toString()); - publicKeys.put(kid, buildPublicKey(key)); + kid = getOptionalString(key, "kid").orElseGet(() -> UUID.randomUUID().toString()); } catch (BadKeyDescriptionException e) { - LOGGER.warn("Bad public key: {}", e.getMessage()); + LOGGER.warn("Issuer returned a JWKS document with a bad public" + + " key identifier (\"kid\"). Using a random" + + " identifier as a work-around, but this could still" + + " lead to tokens being reject."); + kid = UUID.randomUUID().toString(); + } + try { + Result r = Result.success(buildPublicKey(key)); + publicKeys.put(kid, r); + } catch (BadKeyDescriptionException e) { + Result r = Result.failure(e.getMessage()); + publicKeys.put(kid, r); + LOGGER.debug("Bad public key: {}", e.getMessage()); } } return publicKeys; @@ -224,16 +250,35 @@ private PublicKey buildRSAPublicKey(JsonNode details) throws BadKeyDescriptionEx } public void checkIssued(JsonWebToken token) throws AuthenticationException { - Map keyMap = keys.get(); + Map> keyMap = keys.get() + .orElseThrow(msg -> new AuthenticationException( + "Problem fetching public keys: " + msg)); String kid = token.getKeyIdentifier(); if (kid != null) { - PublicKey publicKey = keyMap.get(kid); - checkAuthentication(publicKey != null, "Unknown kid \"" + kid + "\""); - checkAuthentication(token.isSignedBy(publicKey), "Invalid signature"); + var publicKeyResult = keyMap.get(kid); + checkAuthentication(publicKeyResult != null, "issuer has no public" + + " key for this token (key id \"" + kid + "\")"); + PublicKey publicKey = publicKeyResult + .orElseThrow(msg -> new AuthenticationException("issuer has" + + " malformed public key for this token: " + msg)); + checkAuthentication(token.isSignedBy(publicKey), "token not signed by issuer"); } else { - checkAuthentication(keyMap.values().stream().anyMatch(token::isSignedBy), - "Invalid signature"); + if (!keyMap.values().stream() + .map(Result::getSuccess) + .filter(Optional::isPresent) + .map(Optional::get) + .anyMatch(token::isSignedBy)) { + StringBuilder sb = new StringBuilder("issuer has no public key for this token"); + String failures = keyMap.entrySet().stream() + .filter(e -> e.getValue().isFailure()) + .map(e -> e.getKey() + "[" + e.getValue().getFailure().get() + "]") + .collect(Collectors.joining(", ")); + if (!failures.isEmpty()) { + sb.append(": ").append(failures); + } + throw new AuthenticationException(sb.toString()); + } } if (previousJtis != null) { @@ -242,7 +287,7 @@ public void checkIssued(JsonWebToken token) throws AuthenticationException { String jti = jtiClaim.get(); boolean isReplayAttack = previousJtis.contains(jti); previousJtis.add(jti); - checkAuthentication(!isReplayAttack, "token reuse"); + checkAuthentication(!isReplayAttack, "token is being reused; possible replay attack"); } } } diff --git a/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/MemoizeMapWithExpiry.java b/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/MemoizeResultWithExpiry.java similarity index 55% rename from modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/MemoizeMapWithExpiry.java rename to modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/MemoizeResultWithExpiry.java index b32ddaed790..9ff3f39243c 100644 --- a/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/MemoizeMapWithExpiry.java +++ b/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/jwt/MemoizeResultWithExpiry.java @@ -1,6 +1,6 @@ /* dCache - http://www.dcache.org/ * - * Copyright (C) 2019 - 2022 Deutsches Elektronen-Synchrotron + * Copyright (C) 2019 - 2024 Deutsches Elektronen-Synchrotron * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as @@ -19,27 +19,27 @@ import java.time.Duration; import java.time.Instant; -import java.util.Map; import java.util.function.Supplier; +import org.dcache.util.Result; /** - * Provide access to a Map, obtained from some supplier where the value is cached for a configurable - * duration. The cached duration may be different depending on whether the Map is empty. + * Provide access to a Result, obtained from some supplier where the value is cached for a configurable + * duration. The cached duration may be different depending on whether the Result is successful. */ -public class MemoizeMapWithExpiry> implements Supplier { +public class MemoizeResultWithExpiry> implements Supplier { private final Supplier supplier; - private final Duration whenNonEmpty; - private final Duration whenEmpty; + private final Duration whenSuccess; + private final Duration whenFailure; private C value; private Instant nextCheck; - public MemoizeMapWithExpiry(Supplier supplier, Duration whenNonEmpty, - Duration whenEmpty) { + public MemoizeResultWithExpiry(Supplier supplier, Duration whenSuccess, + Duration whenFailure) { this.supplier = supplier; - this.whenEmpty = whenEmpty; - this.whenNonEmpty = whenNonEmpty; + this.whenFailure = whenFailure; + this.whenSuccess = whenSuccess; } @Override @@ -47,39 +47,39 @@ public synchronized C get() { Instant now = Instant.now(); if (nextCheck == null || now.isAfter(nextCheck)) { value = supplier.get(); - Duration cacheDuration = (value == null || value.isEmpty()) - ? whenEmpty : whenNonEmpty; + Duration cacheDuration = (value == null || value.isFailure()) + ? whenFailure : whenSuccess; nextCheck = now.plus(cacheDuration); } return value; } - public static > Builder memorize(Supplier supplier) { + public static > Builder memorize(Supplier supplier) { return new Builder(supplier); } - public static class Builder> { + public static class Builder> { private final Supplier supplier; - private Duration whenNonEmpty; - private Duration whenEmpty; + private Duration whenSuccess; + private Duration whenFailure; public Builder(Supplier supplier) { this.supplier = supplier; } - public Builder whenEmptyFor(Duration duration) { - whenEmpty = duration; + public Builder whenFailureFor(Duration duration) { + whenFailure = duration; return this; } - public Builder whenNonEmptyFor(Duration duration) { - whenNonEmpty = duration; + public Builder whenSuccessFor(Duration duration) { + whenSuccess = duration; return this; } - public MemoizeMapWithExpiry build() { - return new MemoizeMapWithExpiry(supplier, whenNonEmpty, whenEmpty); + public MemoizeResultWithExpiry build() { + return new MemoizeResultWithExpiry(supplier, whenSuccess, whenFailure); } } } diff --git a/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/MockIdentityProviderBuilder.java b/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/MockIdentityProviderBuilder.java index 39366b5805e..7dd867d82d7 100644 --- a/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/MockIdentityProviderBuilder.java +++ b/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/MockIdentityProviderBuilder.java @@ -20,7 +20,9 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.MissingNode; import java.net.URI; +import org.dcache.gplazma.oidc.helpers.ReasonBearingMissingNode; import org.mockito.BDDMockito; import static org.mockito.Mockito.mock; @@ -66,6 +68,18 @@ public MockIdentityProviderBuilder withDiscovery(String json) { return this; } + public MockIdentityProviderBuilder withMissingDiscoveryWithoutReason() { + var missingNode = MissingNode.getInstance(); + BDDMockito.given(provider.discoveryDocument()).willReturn(missingNode); + return this; + } + + public MockIdentityProviderBuilder withMissingDiscoveryWithReason(String reason) { + var missingNode = new ReasonBearingMissingNode(reason); + BDDMockito.given(provider.discoveryDocument()).willReturn(missingNode); + return this; + } + public MockIdentityProviderBuilder withSuppress(String keyword) { BDDMockito.given(provider.isSuppressed(keyword)).willReturn(true); return this; diff --git a/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/helpers/ReasonBearingMissingNodeTest.java b/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/helpers/ReasonBearingMissingNodeTest.java new file mode 100644 index 00000000000..151ebc6fc3d --- /dev/null +++ b/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/helpers/ReasonBearingMissingNodeTest.java @@ -0,0 +1,133 @@ +/* dCache - http://www.dcache.org/ + * + * Copyright (C) 2024 Deutsches Elektronen-Synchrotron + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package org.dcache.gplazma.oidc.helpers; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.core.JsonToken; +import com.fasterxml.jackson.databind.SerializerProvider; +import com.fasterxml.jackson.databind.node.JsonNodeType; +import org.junit.Test; +import static org.hamcrest.Matchers.*; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertTrue; +import static org.mockito.BDDMockito.then; +import static org.mockito.Mockito.mock; + +public class ReasonBearingMissingNodeTest { + + @Test(expected=NullPointerException.class) + public void shouldRejectNullReason() { + new ReasonBearingMissingNode(null); + } + + @Test + public void shouldProvideReason() { + var node = new ReasonBearingMissingNode("What went wrong."); + assertThat(node.getReason(), is(equalTo("What went wrong."))); + } + + @Test + public void shouldHaveCorrectNodeType() { + var node = new ReasonBearingMissingNode("What went wrong."); + assertThat(node.getNodeType(), is(equalTo(JsonNodeType.MISSING))); + } + + @Test + public void shouldBeMissingNode() { + var node = new ReasonBearingMissingNode("What went wrong."); + assertTrue(node.isMissingNode()); + } + + @Test + public void shouldBeNotEqualToObject() { + var node = new ReasonBearingMissingNode("What went wrong."); + var other = new Object(); + assertThat(node, is(not(equalTo(other)))); + } + + @Test + public void shouldEqualSelf() { + var node = new ReasonBearingMissingNode("What went wrong."); + assertThat(node, is(equalTo(node))); + } + + @Test + public void shouldEqualNodeWithSameReason() { + var node = new ReasonBearingMissingNode("What went wrong."); + var otherNode = new ReasonBearingMissingNode("What went wrong."); + assertThat(node, is(equalTo(otherNode))); + } + + @Test + public void shouldNotEqualNodeWithDifferentReason() { + var node = new ReasonBearingMissingNode("What went wrong."); + var otherNode = new ReasonBearingMissingNode("Some other problem."); + assertThat(node, is(not(equalTo(otherNode)))); + } + + @Test + public void shouldHaveSameHashCodeAsAnotherNodeWithSameReason() { + var node = new ReasonBearingMissingNode("What went wrong."); + var otherNode = new ReasonBearingMissingNode("What went wrong."); + assertThat(node.hashCode(), is(equalTo(otherNode.hashCode()))); + } + + @Test + public void shouldHaveTokenNotAvailable() { + var node = new ReasonBearingMissingNode("What went wrong."); + assertThat(node.asToken(), sameInstance(JsonToken.NOT_AVAILABLE)); + } + + @Test + public void shouldAsTextAsEmptyString() { + var node = new ReasonBearingMissingNode("What went wrong."); + assertThat(node.asText(), equalTo("")); + } + + @Test + public void shouldToPrettyStringAsEmptyString() { + var node = new ReasonBearingMissingNode("What went wrong."); + assertThat(node.toPrettyString(), equalTo("")); + } + + @Test + public void shouldToStringAsEmptyString() { + var node = new ReasonBearingMissingNode("What went wrong."); + assertThat(node.toString(), equalTo("")); + } + + @Test + public void shouldAsTextWithDefaultToDefault() { + var node = new ReasonBearingMissingNode("What went wrong."); + assertThat(node.asText("default value"), equalTo("default value")); + } + + @Test + public void shouldSerialiseToNull() throws Exception { + var node = new ReasonBearingMissingNode("What went wrong."); + + var g = mock(JsonGenerator.class); + var p = mock(SerializerProvider.class); + + node.serialize(g, p); + + then(g).should().writeNull(); + then(g).shouldHaveNoMoreInteractions(); + then(p).shouldHaveNoInteractions(); + } +} diff --git a/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/jwt/IssuerTest.java b/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/jwt/IssuerTest.java index b7671738a9d..a898670aab6 100644 --- a/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/jwt/IssuerTest.java +++ b/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/jwt/IssuerTest.java @@ -19,6 +19,7 @@ import com.fasterxml.jackson.databind.node.ObjectNode; import java.security.PublicKey; +import java.util.Base64; import java.util.Optional; import org.apache.http.client.HttpClient; import org.dcache.gplazma.AuthenticationException; @@ -32,10 +33,12 @@ import org.mockito.BDDMockito; import static com.google.common.base.Preconditions.checkState; +import static org.apache.http.HttpStatus.SC_NOT_FOUND; import static org.dcache.gplazma.oidc.MockHttpClientBuilder.aClient; import static org.dcache.gplazma.oidc.MockIdentityProviderBuilder.anIp; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; +import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.*; @@ -97,6 +100,37 @@ public void shouldShowOfflineSuppressedWhenConfigured() throws Exception { assertThat(issuer.isOfflineSuppressed(), is(equalTo(true))); } + @Test + public void shouldPropagateErrorWhenCannotFetchDiscoveryDocument() throws Exception { + given(aClient().onGet("https://oidc.example.org/.well-known/openid-configuration").responds() + .withStatusCode(SC_NOT_FOUND).withoutEntity()); + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withMissingDiscoveryWithReason("fnord")); + given(aMockJwt().withoutKid().withoutJti().signedBy(jwtFactory.publicKey())); + given(anIssuer().withoutHistory()); + + try { + issuer.checkIssued(jwt); + fail("checkIssued unexpectedly passed"); + } catch (AuthenticationException e) { + String msg = e.getMessage(); + assertThat(msg, containsString("fnord")); + } + } + + // Perhaps this case (MissingDiscoveryWithoutReason) could be dropped in the future. + @Test(expected=AuthenticationException.class) + public void shouldPropagateDefaultErrorWhenCannotFetchDiscoveryDocument() throws Exception { + given(aClient().onGet("https://oidc.example.org/.well-known/openid-configuration").responds() + .withStatusCode(SC_NOT_FOUND).withoutEntity()); + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withMissingDiscoveryWithoutReason()); + given(aMockJwt().withoutKid().withoutJti().signedBy(jwtFactory.publicKey())); + given(anIssuer().withoutHistory()); + + issuer.checkIssued(jwt); + } + @Test public void shouldAcceptJwtWithoutKidWhenJkwsNoKid() throws Exception { given(aClient().onGet("https://oidc.example.org/jwks").responds() @@ -154,6 +188,301 @@ public void shouldRejectJwtWithUnknownKid() throws Exception { issuer.checkIssued(jwt); } + @Test + public void shouldRejectJwtWithDiscoveryNoJwksUri() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{}")); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withStatusCode(SC_NOT_FOUND).withoutEntity()); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("jwks_uri")); + assertThat(e.getMessage(), containsString("missing")); + } + } + + @Test + public void shouldRejectJwtWithDiscoveryNonTextualJwksUri() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": null}")); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withStatusCode(SC_NOT_FOUND).withoutEntity()); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("jwks_uri")); + assertThat(e.getMessage(), containsString("non-textual")); + } + } + + @Test + public void shouldRejectJwtWithDiscoveryEmptyJwksUri() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"\"}")); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withStatusCode(SC_NOT_FOUND).withoutEntity()); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("jwks_uri")); + assertThat(e.getMessage(), containsString("empty")); + } + } + + @Test + public void shouldRejectJwtWithDiscoveryBadUriJwksUri() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \":bad URI\"}")); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withStatusCode(SC_NOT_FOUND).withoutEntity()); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("jwks_uri")); + assertThat(e.getMessage(), containsString("URI")); + } + } + + @Test + public void shouldRejectJwtWithJwksNotValidJson() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("NOT JSON")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("public keys")); + assertThat(e.getMessage(), containsString("https://oidc.example.org/jwks")); + assertThat(e.getMessage(), containsString("Unrecognized token")); + } + } + + @Test + public void shouldRejectJwtWithJwksNotObject() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("[]")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("public keys")); + assertThat(e.getMessage(), containsString("wrong type")); + } + } + + @Test + public void shouldRejectJwtWithJwksMissingKeys() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("public keys")); + assertThat(e.getMessage(), containsString("missing")); + assertThat(e.getMessage(), containsString("\"keys\"")); + } + } + + @Test + public void shouldRejectJwtWithJwksKeysNotArray() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{\"keys\": null}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("public keys")); + assertThat(e.getMessage(), containsString("wrong type")); + } + } + + @Test + public void shouldRejectJwtWithKeySignedWithMalformedKeysArray() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{\"keys\": [ \"a random string\"]}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("no public key")); + } + } + + @Test + public void shouldRejectJwtWithKeySignedWithKeyOfUnknownKeyType() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{\"keys\": [{" + + "\"kid\": \"key-1\"," + + "\"kty\": \"UNKNOWN_KEY_TYPE\"}]}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("key type")); + assertThat(e.getMessage(), containsString("UNKNOWN_KEY_TYPE")); + } + } + + @Test + public void shouldRejectJwtWithKeySignedWithKeyWithMissingKeyType() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + var description = jwtFactory.describePublicKey().put("kid", "key-1").without("kty"); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{\"keys\": [" + description + "]}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsStringIgnoringCase("attribute")); + assertThat(e.getMessage(), containsString("kty")); //REVISIT shouldn't kty be in quotes? + } + } + + @Test + public void shouldRejectJwtWithKeySignedWithKeyWithNonTexturalKeyType() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + var description = jwtFactory.describePublicKey() + .put("kid", "key-1") + .put("kty", 42); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{\"keys\": [" + description + "]}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(jwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsStringIgnoringCase("attribute")); + assertThat(e.getMessage(), containsString("textual")); + assertThat(e.getMessage(), containsString("kty")); //REVISIT shouldn't kty be in quotes? + } + } + + @Test + public void shouldRejectJwtWithNoKidNotSignedByPublicKey() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + var description = jwtFactory.describePublicKey(); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{\"keys\": [" + description + "]}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withoutKid().withoutJti().signedBy(anotherJwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("public key")); + } + } + + @Test + public void shouldRejectJwtWithNoKidNotSignedByPublicKeyThatHasAProblem() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + var description = jwtFactory.describePublicKey().put("kty", "UNKNOWN_ALG"); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{\"keys\": [" + description + "]}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withoutKid().withoutJti().signedBy(anotherJwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsStringIgnoringCase("issuer")); + assertThat(e.getMessage(), containsString("public key")); + assertThat(e.getMessage(), containsString("UNKNOWN_ALG")); + } + } + + private ObjectNode mangle(ObjectNode description) { + String encodedN = description.get("n").asText(); + byte[] n = Base64.getUrlDecoder().decode(encodedN); + + // Mangle the modulus + byte[] mangledN = new byte[2]; + System.arraycopy( n, 0, mangledN, 0, 2); + String encodedMangledN = Base64.getUrlEncoder().encodeToString(mangledN); + + return description.deepCopy().put("n", encodedMangledN); + } + + @Test + public void shouldRejectJwtWithNoKidNotSignedByPublicKeyThatHasACryptoProblem() throws Exception { + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + var description = mangle(jwtFactory.describePublicKey()) + .put("kid", "key-1"); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{\"keys\": [" + description + "]}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("key-1").withoutJti().signedBy(anotherJwtFactory.publicKey())); + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("public key")); + assertThat(e.getMessage(), containsString("RSA")); + assertThat(e.getMessage(), containsString("512 bits long")); + } + } + @Test public void shouldAcceptJwtFromKey1FromOpWithMultipleKeys() throws Exception { ObjectNode description1 = jwtFactory.describePublicKey().put("kid", "key-1"); @@ -171,6 +500,49 @@ public void shouldAcceptJwtFromKey1FromOpWithMultipleKeys() throws Exception { verify(jwt, never()).isSignedBy(eq(anotherJwtFactory.publicKey())); } + @Test + public void shouldRejectJwtWithKidSignedByKeyWithInvalidKid() throws Exception { + // Description has invalid 'kid' value: it should be a string, not a number. + ObjectNode description = jwtFactory.describePublicKey().put("kid", 42); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{\"keys\": [" + description + "]}")); + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withKid("42").withoutJti().signedBy(jwtFactory.publicKey())); + + // NB. The current strategy rejects this JWT; however, this isn't a + // requirement. Some future patch might implement a fall-back strategy + // that would allow this case to succeed. + // + // This test exists to exercise the corresponding code-path (ensuring + // dCache handles this case robustly) and to ensure the corresponding + // error message contains the expected content. + + try { + issuer.checkIssued(jwt); + fail("checkIssued returned success"); + } catch (AuthenticationException e) { + assertThat(e.getMessage(), containsString("issuer")); + assertThat(e.getMessage(), containsString("public key")); + assertThat(e.getMessage(), containsString("\"42\"")); + } + } + + @Test + public void shouldAcceptJwtWithoutKidSignedByKeyWithInvalidKid() throws Exception { + // Description has invalid 'kid' value: it should be a string, not a number. + ObjectNode description = jwtFactory.describePublicKey().put("kid", 42); + given(aClient().onGet("https://oidc.example.org/jwks").responds() + .withEntity("{\"keys\": [" + description + "]}")); + given(anIp("EXAMPLE").withEndpoint("https://oidc.example.org/") + .withDiscovery("{\"jwks_uri\": \"https://oidc.example.org/jwks\"}")); + given(anIssuer().withoutHistory()); + given(aMockJwt().withoutKid().withoutJti().signedBy(jwtFactory.publicKey())); + + issuer.checkIssued(jwt); + } + @Test public void shouldAcceptJwtFromKey2FromOpWithMultipleKeys() throws Exception { ObjectNode description1 = jwtFactory.describePublicKey().put("kid", "key-1"); diff --git a/pom.xml b/pom.xml index 8f9150a575a..cb2e98f1546 100644 --- a/pom.xml +++ b/pom.xml @@ -834,6 +834,12 @@ junit junit 4.13.1 + + + org.hamcrest + hamcrest-core + + org.dcache