Skip to content

Commit

Permalink
gplazma: fix NPE if gPlazma is rejecting all logins
Browse files Browse the repository at this point in the history
Motivation:

If gPlazma configuration is broken or a plugin has rejected its
configuration then gPlazma will NOT attempt to log in a user; in
particular, the AUTH phase is not attempted.

There is a bug when handling such requests.  The code that logs
unsuccessful login attempts needs to know the door-supplied information
(credentials and principals, if any).  This information is used to
suppress logging additional failed attempts from that user.

The door-supplied principals are currently only available if the AUTH
phase is attempted.  In the above case, this does not happen, resulting
in a NPE, as described in issue #7490.

This bug hides a further bug, where the code for logging failed login
attempts assumes that the validation error is available, even when the
validation phase has not taken place.

Modification:

Minor improvements to the error message describing how a plugin failured
to initialise.

As a general safety point, ensure that the principals SetDiff values are
non-null by initialising them to a safe default value.

Provide an alternative route to inject the door-supplied information
into the AUTH phase result.  This is a temporary solution that is
low-risk for back-porting.  A better solution would involve more
code-refactoring.

In addition, the failure to configure gPlazma is recorded as a
validation error for the login.  Again, this is an approach to reduce
the risk for back-porting.  A more general (or "correct") approach would
introduce unnecessary risk when back-porting.

Result:

A bug is fixed in gPlazma that is triggered when users attempt to
authenticate with broken gPlazma configuration or a gPlazma plugin's
configuration is broken.

Target: master
Fixes: #7490
Requires-notes: yes
Requires-book: no
Request: 9.2
Patch: https://rb.dcache.org/r/14198/
Acked-by: Tigran Mkrtchyan
  • Loading branch information
Paul Millar authored and mksahakyan committed Jan 9, 2024
1 parent 42d497e commit c7993fe
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 2 deletions.
19 changes: 18 additions & 1 deletion modules/gplazma2/src/main/java/org/dcache/gplazma/GPlazma.java
Expand Up @@ -139,6 +139,23 @@ public LoginReply login(Subject subject) throws AuthenticationException {
loginObservers.forEach(o -> o.accept(result));
return reply;
} catch (AuthenticationException e) {
/* REVISIT The following is a work-around to ensure the set of
* door-supplied information is available. This is only done if
* the AUTH phase is not attempted to avoid creating extra work
* for the common case (AUTH phase is attempted).
*/
if (!result.hasStarted()) {
var authPhase = result.getAuthPhase();
authPhase.setPublicCredentials(subject.getPublicCredentials());
authPhase.setPrivateCredentials(subject.getPrivateCredentials());
result.setInitialPrincipals(subject.getPrincipals());
/* FIXME currently need to treat a failure to initialise
* gPlazma as a validation error in order to inject the error
* message in the logged response.
*/
result.setValidationResult(Result.FAIL);
result.setValidationError(e.getMessage());
}
loginObservers.forEach(o -> o.accept(result));
throw e;
}
Expand Down Expand Up @@ -290,7 +307,7 @@ private Setup buildSetup() throws GPlazmaInternalException {
plugin = pluginLoader.newPluginByName(pluginName, combinedProperties);
} catch (PluginLoadingException e) {
throw new PluginLoadingException(
"failed to create " + pluginName + ": " + e.getMessage(), e);
"failed to create plugin \"" + pluginName + "\": " + e.getMessage(), e);
}

ConfigurationItemControl control = item.getControl();
Expand Down
Expand Up @@ -5,6 +5,7 @@
import com.google.common.collect.Sets;
import java.security.Principal;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
Expand All @@ -16,6 +17,9 @@
*/
public class LoginResult {

private static final SetDiff<Principal> EMPTY_TO_EMPTY =
new SetDiff(Collections.emptySet(), Collections.emptySet());

private final AuthPhaseResult _authPhase = new AuthPhaseResult();
private final MapPhaseResult _mapPhase = new MapPhaseResult();
private final AccountPhaseResult _accountPhase = new AccountPhaseResult();
Expand Down Expand Up @@ -83,6 +87,15 @@ public boolean hasStarted() {
return getAuthPhase().hasHappened();
}

/**
* Store the initial set of principals, as received by gPlazma from the
* door.
* @param initial The door-supplied set of principals.
*/
public void setInitialPrincipals(Set<Principal> initial) {
_authPhase.setPrincipals(initial, initial);
}

/**
* Returns whether gPlazma finished all four phases of the login process.
* <p>
Expand All @@ -102,7 +115,7 @@ public static class PhaseResult<T extends PAMPluginResult> {

private final List<T> _plugins = new ArrayList<>();
private final String _name;
private SetDiff<Principal> _principals;
private SetDiff<Principal> _principals = EMPTY_TO_EMPTY;
private Result _result;
private boolean _hasHappened;

Expand Down

0 comments on commit c7993fe

Please sign in to comment.