fix(security): load the system account from DS activation, not Shiro onInit() (GOSS-025)

pnnl.goss.core/src/pnnl/goss/core/security/impl/Activator.java

@@ -115,11 +115,34 @@ public void realmRemoved(ServiceReference<GossRealm> ref) {
         }
     }
 
+    /**
+     * Writes the collected realms onto the SecurityManager.
+     *
+     * Shiro's RealmSecurityManager.setRealms() rejects an empty collection with
+     * IllegalArgumentException, so removing the LAST realm used to throw out of
+     * realmRemoved() and DS logged "The realmRemoved method has thrown an
+     * exception" on every teardown. Shiro offers no way to install an empty realm
+     * set, so the guard declines the write and logs it.
+     *
+     * Declining does not widen the authorization surface. The realm reference is
+     * AT_LEAST_ONE, so losing the last realm makes this component unsatisfied and
+     * DS deactivates it; the SecurityManager service is unpublished with it, and
+     * GridOpticsServer's mandatory reference on that service goes with it. The
+     * throwing version left exactly the same stale realm set behind, because it
+     * threw before assigning, so this is strictly the same state without the
+     * spurious error.
+     */
     private synchronized void registerAllRealms() {
         Set<Realm> realms = new HashSet<>();
         for (GossRealm r : realmMap.values()) {
             realms.add((Realm) r);
         }
+        if (realms.isEmpty()) {
+            log.warn("No GossRealm services remain; leaving the previously registered realm set in place. "
+                    + "Shiro rejects an empty realm set, and this component is about to be deactivated by DS "
+                    + "because its realm reference is AT_LEAST_ONE.");
+            return;
+        }
         setRealms(realms);
         log.info("Registered {} realms with SecurityManager: {}", realms.size(),
                 realms.stream().map(r -> r.getClass().getSimpleName()).toList());

pnnl.goss.core/src/pnnl/goss/core/security/system/SystemBasedRealm.java

@@ -19,6 +19,9 @@
 import org.osgi.service.component.annotations.ConfigurationPolicy;
 import org.osgi.service.component.annotations.Modified;
 import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.osgi.service.component.annotations.ReferencePolicyOption;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -47,6 +50,20 @@
  * AT_LEAST_ONE realm guard let GridOpticsServer connect as system/manager
  * before this realm was wired.
  *
+ * LIFECYCLE (GOSS-025): the account map is loaded from {@link #activate}, which
+ * Declarative Services is guaranteed to call. It used to be loaded only from
+ * Shiro's {@code onInit()} hook, which is unreachable in this deployment:
+ * {@code onInit()} is protected and called only from
+ * {@code Initializable.init()}, normally driven by
+ * {@code LifecycleUtils.init()} out of an ini/Spring bootstrap. Nothing in
+ * goss-core, in Felix DS, or in shiro-core 2.0.0's own wiring calls it for a
+ * DS-managed realm: {@code RealmSecurityManager.setRealms()} reaches only
+ * {@code afterRealmsSet() -> applyCacheManagerToRealms()}. The realm therefore
+ * activated with a permanently empty map and the broker connect in
+ * GridOpticsServer.start() failed with UnknownAccountException for "system".
+ * {@code onInit()} is retained and delegates to the same idempotent loader, so
+ * the realm stays correct if any future host does drive Shiro's lifecycle.
+ *
  * @author Craig Allwardt
  *
  */
@@ -56,35 +73,152 @@ public class SystemBasedRealm extends AuthorizingRealm implements GossRealm {
 
     private static final Logger log = LoggerFactory.getLogger(SystemBasedRealm.class);
 
+    /**
+     * The permission string granted to the system principal. Consumed verbatim by
+     * the ActiveMQ Shiro authorization plugin through
+     * {@link pnnl.goss.core.security.impl.GossWildcardPermissionResolver}; changing
+     * it changes what the broker connection is allowed to do.
+     */
+    private static final String SYSTEM_PERMISSIONS = "queue:*,topic:*,temp-queue:*,fusion:*:read,fusion:*:write";
+
     private final Map<String, SimpleAccount> userMap = new ConcurrentHashMap<>();
     private final Map<String, Set<String>> userPermissions = new ConcurrentHashMap<>();
 
     @Reference
     GossPermissionResolver gossPermissionResolver;
 
-    @Reference
     private volatile SecurityConfig securityConfig;
 
+    /**
+     * The credential source, bound through methods rather than a field so this
+     * component is notified when it changes.
+     *
+     * The manager user and password do not come from this component's own PID
+     * (pnnl.goss.core.security.systemrealm carries only load=true); they come from
+     * the pnnl.goss.security PID by way of SecurityConfigImpl. A FileInstall edit
+     * to pnnl.goss.security.cfg calls SecurityConfigImpl's modified method, which
+     * leaves the same service instance registered with updated service properties.
+     * A field reference would give this component no callback for that, so the
+     * realm would keep serving a superseded credential. The updated= callback below
+     * is the DS 1.3+ hook for exactly that case, and the dynamic bind reloads on
+     * service replacement.
+     */
+    @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.DYNAMIC, policyOption = ReferencePolicyOption.GREEDY, unbind = "unsetSecurityConfig", updated = "updatedSecurityConfig")
+    public void setSecurityConfig(SecurityConfig securityConfig) {
+        this.securityConfig = securityConfig;
+        loadSystemAccount();
+    }
+
+    /**
+     * Called when the bound SecurityConfig's service properties change, which DS
+     * does after SecurityConfigImpl's modified method runs for a
+     * pnnl.goss.security.cfg edit. Reloads so a rotated manager credential takes
+     * effect without a bundle restart.
+     */
+    public void updatedSecurityConfig(SecurityConfig securityConfig) {
+        this.securityConfig = securityConfig;
+        loadSystemAccount();
+    }
+
+    /**
+     * Fails closed on unbind: the account map is emptied so this realm cannot keep
+     * authenticating against a credential source that is gone. The reference is
+     * mandatory, so DS deactivates this component (and transitively the
+     * SecurityManager and GridOpticsServer) when no replacement exists. The
+     * identity check keeps a greedy bind-then-unbind swap from wiping the account
+     * that the new service just loaded.
+     */
+    public void unsetSecurityConfig(SecurityConfig securityConfig) {
+        if (this.securityConfig == securityConfig) {
+            this.securityConfig = null;
+            clearAccounts();
+            log.warn("SecurityConfig unbound from SystemBasedRealm; system account cleared. "
+                    + "This realm cannot authenticate the system principal until a SecurityConfig rebinds.");
+        }
+    }
+
+    /**
+     * Rebuilds the system account from the bound {@link SecurityConfig}.
+     *
+     * The map is cleared before it is rebuilt, so a rename of the manager user
+     * cannot leave the previous principal behind and a failure cannot leave a
+     * half-built or stale account in place.
+     *
+     * @throws IllegalStateException
+     *             when no SecurityConfig is bound or the configured manager
+     *             credential is missing. Thrown rather than logged-and-swallowed so
+     *             the failure is fail-closed: propagated out of activate() it
+     *             aborts DS activation, this realm's service is never published,
+     *             the Activator's mandatory realm.type=system reference stays
+     *             unsatisfied, the SecurityManager service is never published, and
+     *             GridOpticsServer never opens a broker connection against an empty
+     *             realm.
+     */
+    private synchronized void loadSystemAccount() {
+        clearAccounts();
+
+        SecurityConfig config = this.securityConfig;
+        if (config == null) {
+            throw new IllegalStateException("SystemBasedRealm cannot build the system account: no SecurityConfig "
+                    + "service is bound. Check that pnnl.goss.security.cfg exists and that SecurityConfigImpl "
+                    + "activated.");
+        }
+
+        String managerUser = config.getManagerUser();
+        if (managerUser == null || managerUser.trim().isEmpty()) {
+            throw new IllegalStateException("SystemBasedRealm cannot build the system account: property "
+                    + "goss.system.manager is missing or blank in pnnl.goss.security.cfg.");
+        }
+
+        String managerPassword = config.getManagerPassword();
+        if (managerPassword == null || managerPassword.trim().isEmpty()) {
+            throw new IllegalStateException("SystemBasedRealm cannot build the system account for user '"
+                    + managerUser + "': property goss.system.manager.password is missing or blank in "
+                    + "pnnl.goss.security.cfg.");
+        }
+
+        SimpleAccount account = new SimpleAccount(managerUser, managerPassword, getName());
+        account.addStringPermission(SYSTEM_PERMISSIONS);
+
+        Set<String> perms = new HashSet<>();
+        perms.add(SYSTEM_PERMISSIONS);
+
+        userMap.put(managerUser, account);
+        userPermissions.put(managerUser, perms);
+
+        log.info("SystemBasedRealm loaded the system account for manager user '{}'", managerUser);
+    }
+
+    private void clearAccounts() {
+        userMap.clear();
+        userPermissions.clear();
+    }
+
+    /**
+     * Shiro's Initializable hook. Nothing drives it in this OSGi deployment (see
+     * the class comment), but it is kept wired to the same idempotent loader so the
+     * realm is correct under either entry point rather than silently empty under
+     * one of them.
+     */
     @Override
     protected void onInit() {
         super.onInit();
-        Set<String> perms = new HashSet<>();
-
-        SimpleAccount acnt = new SimpleAccount(securityConfig.getManagerUser(), securityConfig.getManagerPassword(),
-                getName());
-        acnt.addStringPermission("queue:*,topic:*,temp-queue:*,fusion:*:read,fusion:*:write");
-        perms.add("queue:*,topic:*,temp-queue:*,fusion:*:read,fusion:*:write");
-        userMap.put(securityConfig.getManagerUser(), acnt);
-        userPermissions.put(securityConfig.getManagerUser(), perms);
+        loadSystemAccount();
     }
 
     @Activate
     public void activate(Map<String, Object> properties) {
         log.info("Activating SystemBasedRealm");
+        // The component properties (systemrealm.cfg carries only load=true) are not
+        // the credential source; the bound SecurityConfig is. Reload anyway so
+        // activation never depends on bind-callback ordering.
+        loadSystemAccount();
     }
 
     @Modified
     public synchronized void updated(Map<String, Object> properties) {
+        log.info("Reloading SystemBasedRealm after a configuration update");
+        loadSystemAccount();
     }
 
     @Override

pnnl.goss.core/test/pnnl/goss/core/security/impl/test/ActivatorRealmRemovalTest.java

@@ -0,0 +1,126 @@
+package pnnl.goss.core.security.impl.test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.mockito.Mockito.mock;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.apache.shiro.authc.AuthenticationInfo;
+import org.apache.shiro.authc.AuthenticationToken;
+import org.apache.shiro.authc.SimpleAccount;
+import org.apache.shiro.authc.UsernamePasswordToken;
+import org.apache.shiro.authz.AuthorizationInfo;
+import org.apache.shiro.mgt.RealmSecurityManager;
+import org.apache.shiro.realm.AuthorizingRealm;
+import org.apache.shiro.subject.PrincipalCollection;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.osgi.framework.ServiceReference;
+
+import pnnl.goss.core.security.GossRealm;
+import pnnl.goss.core.security.impl.Activator;
+
+/**
+ * Covers the shutdown-path defect seen alongside GOSS-025: removing the last
+ * GossRealm made registerAllRealms() call Shiro's
+ * RealmSecurityManager.setRealms() with an empty collection, which throws
+ * IllegalArgumentException("Realms collection argument cannot be empty."). DS
+ * reported it as "The realmRemoved method has thrown an exception" on every
+ * teardown.
+ *
+ * Shiro offers no way to install an empty realm set, so the guard must decline
+ * the write and log it. The realm set the SecurityManager still holds is not a
+ * widened authorization surface: the Activator's realm reference is
+ * AT_LEAST_ONE, so DS deactivates this component when the last realm goes, and
+ * the SecurityManager service is unpublished with it.
+ */
+public class ActivatorRealmRemovalTest {
+
+    private static final String SYSTEM_USER = "system";
+    private static final String SYSTEM_PASSWORD = "manager";
+
+    private static final class FakeSystemRealm extends AuthorizingRealm implements GossRealm {
+        private final SimpleAccount account;
+
+        FakeSystemRealm() {
+            this.account = new SimpleAccount(SYSTEM_USER, SYSTEM_PASSWORD, getName());
+            this.account.addStringPermission("*");
+        }
+
+        @Override
+        protected AuthenticationInfo doGetAuthenticationInfo(AuthenticationToken token) {
+            UsernamePasswordToken upToken = (UsernamePasswordToken) token;
+            return SYSTEM_USER.equals(upToken.getUsername()) ? account : null;
+        }
+
+        @Override
+        protected AuthorizationInfo doGetAuthorizationInfo(PrincipalCollection principals) {
+            String username = (String) getAvailablePrincipal(principals);
+            return SYSTEM_USER.equals(username) ? account : null;
+        }
+
+        @Override
+        public Set<String> getPermissions(String identifier) {
+            return SYSTEM_USER.equals(identifier) ? Set.of("*") : new HashSet<>();
+        }
+
+        @Override
+        public boolean hasIdentifier(String identifier) {
+            return SYSTEM_USER.equals(identifier);
+        }
+    }
+
+    @SuppressWarnings("unchecked")
+    private static ServiceReference<GossRealm> realmRef() {
+        return mock(ServiceReference.class);
+    }
+
+    @Test
+    @DisplayName("Removing the last realm does not throw out of realmRemoved")
+    public void removingLastRealmDoesNotThrow() {
+        Activator securityManager = new Activator();
+        ServiceReference<GossRealm> ref = realmRef();
+        securityManager.realmAdded(ref, new FakeSystemRealm());
+        securityManager.activate();
+
+        assertThatCode(() -> securityManager.realmRemoved(ref))
+                .as("teardown of the final realm must not surface IllegalArgumentException")
+                .doesNotThrowAnyException();
+    }
+
+    @Test
+    @DisplayName("Removing one of several realms still narrows the registered realm set")
+    public void removingOneOfSeveralRealmsStillRewritesTheSet() {
+        Activator securityManager = new Activator();
+        ServiceReference<GossRealm> first = realmRef();
+        ServiceReference<GossRealm> second = realmRef();
+        FakeSystemRealm keep = new FakeSystemRealm();
+        securityManager.realmAdded(first, new FakeSystemRealm());
+        securityManager.realmAdded(second, keep);
+        securityManager.activate();
+
+        securityManager.realmRemoved(first);
+
+        RealmSecurityManager rsm = securityManager;
+        assertThat(rsm.getRealms())
+                .as("the removed realm must be gone and the surviving realm retained")
+                .containsExactly(keep);
+    }
+
+    @Test
+    @DisplayName("Removing an unknown realm reference is a no-op on the registered set")
+    public void removingUnknownReferenceIsANoOp() {
+        Activator securityManager = new Activator();
+        ServiceReference<GossRealm> known = realmRef();
+        FakeSystemRealm realm = new FakeSystemRealm();
+        securityManager.realmAdded(known, realm);
+        securityManager.activate();
+
+        securityManager.realmRemoved(realmRef());
+
+        RealmSecurityManager rsm = securityManager;
+        assertThat(rsm.getRealms()).containsExactly(realm);
+    }
+}