From c7993fe0f3f002b734a3b2f6b6ad86f9d42b57f9 Mon Sep 17 00:00:00 2001 From: Paul Millar Date: Thu, 4 Jan 2024 13:52:06 +0100 Subject: [PATCH] gplazma: fix NPE if gPlazma is rejecting all logins 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 --- .../main/java/org/dcache/gplazma/GPlazma.java | 19 ++++++++++++++++++- .../dcache/gplazma/monitor/LoginResult.java | 15 ++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/modules/gplazma2/src/main/java/org/dcache/gplazma/GPlazma.java b/modules/gplazma2/src/main/java/org/dcache/gplazma/GPlazma.java index 61080da7677..d01ede32fe4 100644 --- a/modules/gplazma2/src/main/java/org/dcache/gplazma/GPlazma.java +++ b/modules/gplazma2/src/main/java/org/dcache/gplazma/GPlazma.java @@ -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; } @@ -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(); diff --git a/modules/gplazma2/src/main/java/org/dcache/gplazma/monitor/LoginResult.java b/modules/gplazma2/src/main/java/org/dcache/gplazma/monitor/LoginResult.java index a3fe653eb7a..e408704c263 100644 --- a/modules/gplazma2/src/main/java/org/dcache/gplazma/monitor/LoginResult.java +++ b/modules/gplazma2/src/main/java/org/dcache/gplazma/monitor/LoginResult.java @@ -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; @@ -16,6 +17,9 @@ */ public class LoginResult { + private static final SetDiff 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(); @@ -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 initial) { + _authPhase.setPrincipals(initial, initial); + } + /** * Returns whether gPlazma finished all four phases of the login process. *

@@ -102,7 +115,7 @@ public static class PhaseResult { private final List _plugins = new ArrayList<>(); private final String _name; - private SetDiff _principals; + private SetDiff _principals = EMPTY_TO_EMPTY; private Result _result; private boolean _hasHappened;