Skip to content

Commit

Permalink
gplazma: oidc add suppress for audience claim verification
Browse files Browse the repository at this point in the history
Motivation:

Earlier versions of dCache did not support audience claim verification
in the oidc plugin.  Although audience claims should be verified, doing
so risks breaking existing clients.  Suppressing audience verification
reproduces the earlier dCache behaviour, giving admins time to educate
their users on correct audience claim values.

Modification:

Add support for the `-suppress=audience` option that disables the aud
claim verification.  Log a warning on start-up if audience checking is
suppressed.  Log when a token that should be rejected is accepted due to
suppression of audience checking.

Result:

It is now possible to configure dCache so the oidc plugin does not check
audience fields.  This should be used where necessary and only for a
short transition period.

Target: master
Request: 8.2
Request: 9.0
Require-notes: yes
Require-book: no
Patch: https://rb.dcache.org/r/13903/
Acked-by: Tigran Mkrtchyan
  • Loading branch information
paulmillar authored and lemora committed Mar 15, 2023
1 parent c50771d commit f925377
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 6 deletions.
Expand Up @@ -27,6 +27,7 @@
import org.apache.http.client.HttpClient;
import org.dcache.auth.BearerTokenCredential;
import org.dcache.auth.OAuthProviderPrincipal;
import org.dcache.auth.Origin;
import org.dcache.auth.attributes.Restriction;
import org.dcache.gplazma.AuthenticationException;
import org.dcache.gplazma.oidc.helpers.JsonHttpClient;
Expand Down Expand Up @@ -58,6 +59,8 @@ public class OidcAuthPlugin implements GPlazmaAuthenticationPlugin {
private final static String OIDC_ALLOWED_AUDIENCES = "gplazma.oidc.audience-targets";
private final static String OIDC_PROVIDER_PREFIX = "gplazma.oidc.provider!";

private final static String SUPPRESS_AUDIENCE_TOKEN = "audience";

private static final String DEFAULT_PROFILE_NAME = "oidc";
private static final Map<String,ProfileFactory> PROFILES = Map.of("oidc", new OidcProfileFactory(),
"scitokens", new ScitokensProfileFactory(),
Expand Down Expand Up @@ -95,8 +98,15 @@ private static IdentityProvider createIdentityProvider(String name, String descr
.flatMap(v -> Splitter.on(',').trimResults().splitToStream(v))
.collect(Collectors.toList());

return new IdentityProvider(name, issuer, profile, client, discoveryCacheDuration,
var idp = new IdentityProvider(name, issuer, profile, client, discoveryCacheDuration,
suppress);
if (idp.isSuppressed(SUPPRESS_AUDIENCE_TOKEN)) {
LOG.warn("Audience (\"aud\") checking is suppressed for OP {}. This makes dCache "
+ "compatible with behaviour before version 8.2.0, but it also violates RFC "
+ "\"MUST\" requirements and may have security implications.",
idp.getName());
}
return idp;
} catch (URISyntaxException e) {
throw new IllegalArgumentException(
"Invalid endpoint " + endpoint + ": " + e.getMessage());
Expand Down Expand Up @@ -186,7 +196,7 @@ public void authenticate(Set<Object> publicCredentials, Set<Object> privateCrede
try {
ExtractResult result = tokenProcessor.extract(token);
checkAuthentication(!result.claims().isEmpty(), "processing token yielded no claims");
checkAudience(result.claims());
checkAudience(result, identifiedPrincipals);

var idp = result.idp();
identifiedPrincipals.add(new OAuthProviderPrincipal(idp.getName()));
Expand Down Expand Up @@ -220,22 +230,73 @@ private static void checkValid(String token) throws AuthenticationException {
}
}

private void checkAudience(Map<String,JsonNode> claims) throws AuthenticationException {
private void checkAudience(ExtractResult result, Set<Principal> principals)
throws AuthenticationException {
var claims = result.claims();
var audClaim = claims.get("aud");

if (audClaim == null) {
return;
}

var idp = result.idp();
boolean suppressAudience = idp.isSuppressed(SUPPRESS_AUDIENCE_TOKEN);

if (audClaim.isArray()) {
List<String> audClaimAsList = new ArrayList<>(audClaim.size());
audClaim.elements().forEachRemaining(e -> audClaimAsList.add(e.textValue()));
checkAuthentication(audClaimAsList.stream().anyMatch(audienceTargets::contains),
"intended for %s", audClaimAsList);

if (!audClaimAsList.stream().anyMatch(audienceTargets::contains)) {
if (suppressAudience) {
logAudienceSuppression(audClaimAsList.toString(), idp.getName(), claims,
principals, "one of these audiences");
return;
}
throw new AuthenticationException("intended for " + audClaimAsList);
}
} else {
String aud = audClaim.textValue();
checkAuthentication(audienceTargets.contains(aud), "intended for %s", aud);
if (!audienceTargets.contains(aud)) {
if (suppressAudience) {
logAudienceSuppression(aud, idp.getName(), claims, principals, "this audience");
return;
}
throw new AuthenticationException("intended for " + aud);
}
}
}

private static void logAudienceSuppression(String audience, String op,
Map<String,JsonNode> claims, Set<Principal> principals, String whatToAdd) {
Optional<String> clientIPAddress = principals.stream()
.filter(Origin.class::isInstance)
.map(Origin.class::cast)
.findFirst()
.map(Principal::getName);

LOG.warn("Accepting token with an incompatible audience \"{}\" issued by OP {} to {}{}. "
+ "To prevent similar warnings, either add {} to the 'gplazma.oidc.audience-targets' "
+ "configuration property or update the client to use an audience value already defined "
+ "in this configuration property.", audience, op,
clientId(claims).map(id -> "client \"" + id + "\"").orElse("an unknown client"),
clientIPAddress.map(addr -> " connecting from " + addr).orElse(""),
whatToAdd);
}

private static Optional<String> clientId(Map<String,JsonNode> claims) {
var clientIdClaim = claims.get("client_id");
if (clientIdClaim != null && clientIdClaim.isTextual()) {
return Optional.of(clientIdClaim.textValue());
}

// Some OPs (*cough* EGI CheckIn *cough*) violate RFC 9068 by issuing JWTs without a
// 'client_id' claim; however, they do provide an 'azp' claim.
var azpClaim = claims.get("azp");
if (azpClaim != null && azpClaim.isTextual()) {
return Optional.of(azpClaim.textValue());
}

return Optional.empty();
}

private static <T> Collection<T> nullToEmpty(final Collection<T> collection) {
Expand Down
Expand Up @@ -567,6 +567,27 @@ public void shouldRejectSingleAuthClaimWithNoConfiguredAudiences() throws Except
when(invoked().withBearerToken("FOO"));
}

@Test
public void shouldAcceptSingleAuthClaimWithNoMatchingAudiencesIfAudienceSuppressed() throws Exception {
var profile = aProfile().thatReturns(aProfileResult()
.withPrincipals(Collections.singleton(new OidcSubjectPrincipal("sub-claim-value", "MY-OP"))))
.build();
var op = anIp("MY-OP").withProfile(profile).withSuppress("audience").build();
var claims = claims()
.withStringClaim("sub", "sub-claim-value")
.withStringClaim("aud", "another-service.example.org")
.build();
given(aPlugin()
.withProperty("gplazma.oidc.audience-targets", "dcache.example.org")
.withTokenProcessor(aTokenProcessor().thatReturns(aResult().from(op).containing(claims))));

when(invoked().withBearerToken("FOO"));

verify(processor).extract(eq("FOO"));
verify(profile).processClaims(eq(op), eq(claims));
assertThat(principals, hasItem(new OidcSubjectPrincipal("sub-claim-value", "MY-OP")));
}

@Test(expected=AuthenticationException.class)
public void shouldRejectSingleAuthClaimWithDifferentConfiguredAudience() throws Exception {
given(aPlugin()
Expand Down
24 changes: 24 additions & 0 deletions skel/share/defaults/gplazma.properties
Expand Up @@ -382,6 +382,30 @@ gplazma.htpasswd.file.cache-period.unit = SECONDS
#
# The following suppress keywords are supported:
#
# audience Suppress audience ("aud") claim verification. By
# default, dCache will check that the "aud" claim, if
# present, matches one of the identities configured via
# the 'gplazma.oidc.audience-targets' configuration
# property. The token is rejected if there is no
# match. This option suppresses this check, resulting
# in dCache accepting tokens with an arbitrary "aud"
# claim value.
#
# In general, audience verification provides a form of
# "damage limitation" if a token is misappropriated.
# The stolen token may only be used against the token's
# intended target service and not against any other
# service that the user is authorised.
#
# Suppressing audience verification may be needed as
# earlier versions of dCache lacked audience
# verification. Existing clients may be obtaining
# tokens with inappropriate audience fields. It is
# recommended that this option is used sparingly and
# only for a short time; i.e., a strategy is devised
# through which clients are updated and this
# suppression may be disabled.
#
# offline Suppress offline validation. By default, dCache will
# attempt to validate a JWT using the OP's public keys.
# If the token is valid then the claims stored within
Expand Down

0 comments on commit f925377

Please sign in to comment.