Skip to content

Commit

Permalink
core: avoid sending bad macaroons to gplazma
Browse files Browse the repository at this point in the history 
Motivation:

Macaroons are bearer tokens issued by dCache: a macaroon issued by
dCache is only acceptable by the dCache instance that issued it.

UnionLoginStrategy, when used to combine multiple strategies, is
currently used only to combine the MacaroonLoginStrategy with the
RemoteLoginStrategy (i.e., gPlazma).

Currently, UnionLoginStrategy stops iterating through the list of
configured LoginStrategy objects only if the login was successful and
yielded a non-NOBODY identity.  This means that UnionLoginStrategy will
send a dCache-issued macaroon that is invalid (e.g., has expired) to
gPlazma.

As macaroons may be unique with each request, any caching (e.g., within
the door, to prevent overloading gPlazma) may be ineffective and
(potentially) all invalid macaroons are sent to gPlazma.

OpenID-Connect (OIDC) access-tokens are also bearer tokens.  OIDC access
tokens are opaque: there's no way to determine whether a bearer token is
an OIDC access token.  (An OP could issue a macaroon as an OIDC access
token, but currently no OP does this.)

This means that, if gPlazma has the OpenID-Connect (OIDC) plugin
configured, then (potentially) all invalid macaroons are sent to all
OPs.

Some OPs may implement counter-measures for such request spamming, such
as rate-limiting requests if requests are received too often.  This can
result in login requests to gPlazma timing out.

Modification:

Update UnionLoginStrategy so that any LoginStrategy that both
understands the credentials and rejects it terminates the login attempt.

Update MacaroonLoginStrategy so that unknown macaroons are treated as an
unknown credential instead of failing the login.

Result:

An invalid macaroon login no longer spams gPlazma and (consequently) OPs.

Target: master
Request: 4.2
Request: 4.1
Request: 4.0
Request: 3.2
Requires-notes: yes
Requires-book: no
Patch: https://rb.dcache.org/r/11245/
Acked-by: Tigran Mkrtchyan
  • Loading branch information
@paulmillar
paulmillar committed Oct 23, 2018
1 parent a5eb9e7 commit a586818
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 34 deletions.
66 changes: 36 additions & 30 deletions modules/dcache/src/main/java/org/dcache/auth/UnionLoginStrategy.java
View file
Expand Up @@ -19,13 +19,21 @@
import org.dcache.auth.attributes.Restrictions;

/**
* LoginStrategy which forms the union of allowed logins of several
* access strategies. Login will be granted by the first of a list of
* LoginStrategies which grants login. If no LoginStrategy grants
* login then the behaviour depends on whether the user supplied any credentials.
* If no credentials were presented then the login attempt is treated as
* anonymous and the correct level of access is granted. If credentials were
* presented then fallback to anonymous only if that is allowed.
* LoginStrategy which consults a list of several access strategies when
* authenticating a user. The first strategy in the list is consulted. If that
* strategy successfully authenticates the user then there is no further action
* and the LoginReply is used. If that strategy understands the supplied
* credentials and rejects the login then no further strategies are consulted.
* If the strategy does not understand the supplied credential then the the next
* strategy is tried. Once no further strategies are to be tried and the user
* has not been logged in successfully then.
* <p>
* If none of the consulted strategies has granted login then the behaviour
* depends on whether the user supplied any credentials. If no credentials were
* presented then the login attempt is treated as anonymous and the correct
* level of access is granted. If credentials were presented then it is
* configurable whether the user is treated as the anonymous user, or the login
* failure is propagated.
*/
public class UnionLoginStrategy implements LoginStrategy
{
Expand Down Expand Up @@ -86,33 +94,31 @@ public LoginReply login(Subject subject) throws CacheException
|| !subject.getPublicCredentials().isEmpty()
|| !subject.getPrincipals().stream().allMatch(Origin.class::isInstance);

for (LoginStrategy strategy: _loginStrategies) {
_log.debug( "Attempting login strategy: {}", strategy.getClass().getName());

try {
LoginReply login = strategy.login(subject);
_log.debug( "Login strategy returned {}", login.getSubject());
if (!Subjects.isNobody(login.getSubject())) {
return login;
PermissionDeniedCacheException loginFailure = null;
try {
for (LoginStrategy strategy: _loginStrategies) {
_log.debug( "Attempting login strategy: {}", strategy.getClass().getName());

try {
LoginReply login = strategy.login(subject);
_log.debug( "Login strategy returned {}", login.getSubject());
if (!Subjects.isNobody(login.getSubject())) {
return login;
}
} catch (IllegalArgumentException e) {
/* LoginStrategies throw IllegalArgumentException when
* provided with a Subject they cannot handle.
*/
_log.debug("Login failed with IllegalArgumentException for {}: {}", subject,
e.getMessage());
}
} catch (IllegalArgumentException e) {
/* LoginStrategies throw IllegalArgumentException when
* provided with a Subject they cannot handle.
*/
_log.debug("Login failed with IllegalArgumentException for {}: {}", subject,
e.getMessage());
} catch (PermissionDeniedCacheException e) {
/* As we form the union of all allowed logins of all
* strategies, we ignore the failure and try the next
* strategy.
*/
_log.debug("Permission denied for {}: {}", subject,
e.getMessage());
}
} catch (PermissionDeniedCacheException e) {
loginFailure = e;
}

if (areCredentialsSupplied && !_shouldFallback) {
throw new PermissionDeniedCacheException("Access denied");
throw loginFailure != null ? loginFailure : new PermissionDeniedCacheException("Access denied");
}

_log.debug( "Strategies failed, trying for anonymous access");
Expand All @@ -136,7 +142,7 @@ public LoginReply login(Subject subject) throws CacheException

default:
_log.debug( "Login failed");
throw new PermissionDeniedCacheException("Access denied");
throw loginFailure != null ? loginFailure : new PermissionDeniedCacheException("Access denied");
}
return reply;
}
Expand Down
8 changes: 4 additions & 4 deletions modules/dcache/src/main/java/org/dcache/macaroons/MacaroonProcessor.java
View file
Expand Up @@ -28,8 +28,8 @@
import org.dcache.auth.attributes.Activity;
import org.dcache.util.Strings;

import static com.google.common.base.Preconditions.checkArgument;
import static org.dcache.macaroons.CaveatValues.*;
import static org.dcache.macaroons.InvalidMacaroonException.checkMacaroon;

/**
* Class that acts as a facade to the Macaroon library.
Expand Down Expand Up @@ -122,6 +122,9 @@ public MacaroonContext expandMacaroon(String serialisedMacaroon, InetAddress cli
byte[] rawId = BaseEncoding.base16().lowerCase().decode(macaroon.signature.substring(0, 12));
String macaroonId = new String(Base64.getEncoder().encode(rawId), StandardCharsets.US_ASCII);

byte[] secret = _secretHandler.findSecret(macaroon.identifier);
checkArgument(secret != null, "Unable to find secret for macaroon [%s]", macaroonId);

MacaroonsVerifier verifier = new MacaroonsVerifier(macaroon);

ClientIPCaveatVerifier clientIPVerifier = new ClientIPCaveatVerifier(clientAddress);
Expand All @@ -130,9 +133,6 @@ public MacaroonContext expandMacaroon(String serialisedMacaroon, InetAddress cli
ContextExtractingCaveatVerifier contextExtractor = new ContextExtractingCaveatVerifier();
verifier.satisfyGeneral(contextExtractor);

byte[] secret = _secretHandler.findSecret(macaroon.identifier);
checkMacaroon(secret != null, "Unable to find secret for macaroon [%s]", macaroonId);

if (!verifier.isValid(secret)) {
StringBuilder error = new StringBuilder("Invalid macaroon [").append(macaroonId).append(']');

Expand Down

0 comments on commit a586818

Please sign in to comment.