From 3de02a1c82160ff4783e1d91ca99e9765587b285 Mon Sep 17 00:00:00 2001 From: Luca Date: Sat, 1 Aug 2026 17:30:20 -0700 Subject: [PATCH 1/2] test(security): RED for GOSS-025 system realm bootstrap SystemBasedRealm populates its account map only from Shiro's onInit() hook. onInit() is protected and reachable only from Initializable.init(), which nothing in goss-core, Felix DS, or shiro-core 2.0.0 ever calls on a DS-managed realm: RealmSecurityManager.setRealms() reaches only afterRealmsSet() -> applyCacheManagerToRealms(), not init(). The realm therefore activates with a permanently empty userMap, and the system principal cannot authenticate against it. SystemBasedRealmActivationTest asserts the realm resolves the system account after @Activate alone, with the exact broker permission string, plus config reload and fail-closed behavior when the credential source is unusable. ActivatorRealmRemovalTest covers the shutdown defect on the same path: removing the last realm calls setRealms(emptySet), which Shiro rejects with IllegalArgumentException out of realmRemoved. RED: 8 failing across the two new classes, including "Realms collection argument cannot be empty." at Activator.java:123. Refs GOSS-025 --- .../impl/test/ActivatorRealmRemovalTest.java | 126 ++++++++ .../test/SystemBasedRealmActivationTest.java | 281 ++++++++++++++++++ 2 files changed, 407 insertions(+) create mode 100644 pnnl.goss.core/test/pnnl/goss/core/security/impl/test/ActivatorRealmRemovalTest.java create mode 100644 pnnl.goss.core/test/pnnl/goss/core/security/system/test/SystemBasedRealmActivationTest.java diff --git a/pnnl.goss.core/test/pnnl/goss/core/security/impl/test/ActivatorRealmRemovalTest.java b/pnnl.goss.core/test/pnnl/goss/core/security/impl/test/ActivatorRealmRemovalTest.java new file mode 100644 index 00000000..b4c5dfef --- /dev/null +++ b/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 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 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 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 first = realmRef(); + ServiceReference 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 known = realmRef(); + FakeSystemRealm realm = new FakeSystemRealm(); + securityManager.realmAdded(known, realm); + securityManager.activate(); + + securityManager.realmRemoved(realmRef()); + + RealmSecurityManager rsm = securityManager; + assertThat(rsm.getRealms()).containsExactly(realm); + } +} diff --git a/pnnl.goss.core/test/pnnl/goss/core/security/system/test/SystemBasedRealmActivationTest.java b/pnnl.goss.core/test/pnnl/goss/core/security/system/test/SystemBasedRealmActivationTest.java new file mode 100644 index 00000000..b7d24635 --- /dev/null +++ b/pnnl.goss.core/test/pnnl/goss/core/security/system/test/SystemBasedRealmActivationTest.java @@ -0,0 +1,281 @@ +package pnnl.goss.core.security.system.test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.lang.reflect.Field; +import java.util.Collections; +import java.util.Map; +import java.util.Set; + +import org.apache.shiro.authc.AuthenticationInfo; +import org.apache.shiro.authc.IncorrectCredentialsException; +import org.apache.shiro.authc.SimpleAccount; +import org.apache.shiro.authc.UsernamePasswordToken; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import pnnl.goss.core.security.JWTAuthenticationToken; +import pnnl.goss.core.security.SecurityConfig; +import pnnl.goss.core.security.system.SystemBasedRealm; + +/** + * Covers the GOSS-025 bootstrap failure: SystemBasedRealm populated its account + * map only from Shiro's {@code onInit()} hook, and nothing in this OSGi + * deployment ever calls {@code Initializable.init()}. Declarative Services calls + * {@code @Activate}; Shiro's RealmSecurityManager.setRealms() reaches only + * afterRealmsSet() -> applyCacheManagerToRealms(), never init(). So the realm + * activated with a permanently empty userMap and every system/manager + * authentication against it threw UnknownAccountException, which broke the + * broker connect in GridOpticsServer.start(). + * + * The load-bearing assertion in this class is that the realm resolves the system + * account after DS activation WITHOUT any call to init(). Everything else here + * guards the surrounding lifecycle: config reload, reference rebind, and fail- + * closed behavior when the credential source is unusable. + */ +public class SystemBasedRealmActivationTest { + + private static final String SYSTEM_USER = "system"; + private static final String SYSTEM_PASSWORD = "manager"; + + /** + * The exact permission string the broker's authorization plugin consumes for + * the system principal. Deliberately duplicated here as a literal rather than + * referenced from production code: this test is the guard that the string is + * not silently altered. + */ + private static final String SYSTEM_PERMISSIONS = "queue:*,topic:*,temp-queue:*,fusion:*:read,fusion:*:write"; + + /** Mutable SecurityConfig stub standing in for SecurityConfigImpl. */ + private static final class FakeSecurityConfig implements SecurityConfig { + private String managerUser; + private String managerPassword; + + FakeSecurityConfig(String managerUser, String managerPassword) { + this.managerUser = managerUser; + this.managerPassword = managerPassword; + } + + @Override + public String getManagerUser() { + return managerUser; + } + + @Override + public String getManagerPassword() { + return managerPassword; + } + + @Override + public boolean getUseToken() { + return false; + } + + @Override + public boolean validateToken(String token) { + return false; + } + + @Override + public JWTAuthenticationToken parseToken(String token) { + return null; + } + + @Override + public String createToken(Object userId, Set roles) { + return null; + } + } + + /** + * Simulates the Declarative Services reference injection for the + * securityConfig reference. The production reference is a field reference + * today, so this mirrors it exactly. + */ + private static void bindSecurityConfig(SystemBasedRealm realm, SecurityConfig config) throws Exception { + Field field = SystemBasedRealm.class.getDeclaredField("securityConfig"); + field.setAccessible(true); + field.set(realm, config); + } + + /** The DS-supplied component properties: systemrealm.cfg carries load=true. */ + private static Map componentProperties() { + return Map.of("load", "true"); + } + + @Test + @DisplayName("Realm resolves the system account after DS activation, with no call to Shiro init()") + public void systemAccountResolvesAfterActivateWithoutShiroInit() throws Exception { + SystemBasedRealm realm = new SystemBasedRealm(); + bindSecurityConfig(realm, new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD)); + + // Exactly what the container does: @Activate, and nothing else. No init(). + realm.activate(componentProperties()); + + assertThat(realm.hasIdentifier(SYSTEM_USER)) + .as("the realm must know the system principal after DS activation alone") + .isTrue(); + + AuthenticationInfo info = realm.getAuthenticationInfo( + new UsernamePasswordToken(SYSTEM_USER, SYSTEM_PASSWORD)); + + assertThat(info).as("system/manager must authenticate against the activated realm").isNotNull(); + assertThat(info.getPrincipals().getPrimaryPrincipal()).isEqualTo(SYSTEM_USER); + assertThat(info.getCredentials()).isEqualTo(SYSTEM_PASSWORD); + + assertThat(info).isInstanceOf(SimpleAccount.class); + SimpleAccount account = (SimpleAccount) info; + assertThat(account.getStringPermissions()) + .as("the system account must carry exactly the broker permission string, unaltered") + .containsExactly(SYSTEM_PERMISSIONS); + + assertThat(realm.getPermissions(SYSTEM_USER)) + .as("PermissionAdapter view must report the same single permission string") + .containsExactly(SYSTEM_PERMISSIONS); + } + + @Test + @DisplayName("Unknown principals are still rejected after activation") + public void unknownPrincipalIsRejected() throws Exception { + SystemBasedRealm realm = new SystemBasedRealm(); + bindSecurityConfig(realm, new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD)); + realm.activate(componentProperties()); + + assertThat(realm.hasIdentifier("intruder")).isFalse(); + assertThat(realm.getPermissions("intruder")).isEmpty(); + assertThat(realm.getAuthenticationInfo(new UsernamePasswordToken("intruder", "manager"))) + .as("the realm must not vend an account for an unconfigured principal") + .isNull(); + } + + @Test + @DisplayName("A wrong password for the system principal is rejected") + public void wrongSystemPasswordIsRejected() throws Exception { + SystemBasedRealm realm = new SystemBasedRealm(); + bindSecurityConfig(realm, new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD)); + realm.activate(componentProperties()); + + assertThatThrownBy(() -> realm.getAuthenticationInfo( + new UsernamePasswordToken(SYSTEM_USER, "not-the-password"))) + .isInstanceOf(IncorrectCredentialsException.class); + } + + @Test + @DisplayName("Shiro init() remains safe and idempotent if anything ever calls it") + public void shiroInitIsIdempotentAfterActivation() throws Exception { + SystemBasedRealm realm = new SystemBasedRealm(); + bindSecurityConfig(realm, new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD)); + realm.activate(componentProperties()); + + realm.init(); + + assertThat(realm.hasIdentifier(SYSTEM_USER)).isTrue(); + assertThat(realm.getPermissions(SYSTEM_USER)).containsExactly(SYSTEM_PERMISSIONS); + AuthenticationInfo info = realm.getAuthenticationInfo( + new UsernamePasswordToken(SYSTEM_USER, SYSTEM_PASSWORD)); + assertThat(info.getPrincipals().getPrimaryPrincipal()).isEqualTo(SYSTEM_USER); + assertThat(info.getCredentials()).isEqualTo(SYSTEM_PASSWORD); + } + + @Test + @DisplayName("A configuration update replaces the stored credential rather than keeping the stale one") + public void modifiedReloadsChangedCredentials() throws Exception { + SystemBasedRealm realm = new SystemBasedRealm(); + FakeSecurityConfig config = new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD); + bindSecurityConfig(realm, config); + realm.activate(componentProperties()); + + config.managerPassword = "rotated"; + realm.updated(componentProperties()); + + assertThatThrownBy(() -> realm.getAuthenticationInfo( + new UsernamePasswordToken(SYSTEM_USER, SYSTEM_PASSWORD))) + .as("the superseded password must no longer authenticate") + .isInstanceOf(IncorrectCredentialsException.class); + + AuthenticationInfo info = realm.getAuthenticationInfo( + new UsernamePasswordToken(SYSTEM_USER, "rotated")); + assertThat(info).isNotNull(); + assertThat(info.getCredentials()).isEqualTo("rotated"); + } + + @Test + @DisplayName("A renamed manager user does not leave the previous account behind") + public void modifiedDoesNotLeaveStaleAccounts() throws Exception { + SystemBasedRealm realm = new SystemBasedRealm(); + FakeSecurityConfig config = new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD); + bindSecurityConfig(realm, config); + realm.activate(componentProperties()); + + config.managerUser = "operator"; + realm.updated(componentProperties()); + + assertThat(realm.hasIdentifier(SYSTEM_USER)) + .as("the previous manager user must be evicted, not accumulated") + .isFalse(); + assertThat(realm.getPermissions(SYSTEM_USER)).isEmpty(); + assertThat(realm.hasIdentifier("operator")).isTrue(); + assertThat(realm.getPermissions("operator")).containsExactly(SYSTEM_PERMISSIONS); + } + + @Test + @DisplayName("Activation fails closed with an empty realm when the manager user is absent") + public void activationFailsClosedWithoutManagerUser() throws Exception { + SystemBasedRealm realm = new SystemBasedRealm(); + bindSecurityConfig(realm, new FakeSecurityConfig(null, SYSTEM_PASSWORD)); + + assertThatThrownBy(() -> realm.activate(componentProperties())) + .as("a missing manager user must abort activation, not publish an empty realm") + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("goss.system.manager"); + + assertThat(realm.hasIdentifier(SYSTEM_USER)).isFalse(); + assertThat(realm.getAuthenticationInfo(new UsernamePasswordToken(SYSTEM_USER, SYSTEM_PASSWORD))) + .isNull(); + } + + @Test + @DisplayName("Activation fails closed with an empty realm when the manager password is absent") + public void activationFailsClosedWithoutManagerPassword() throws Exception { + SystemBasedRealm realm = new SystemBasedRealm(); + bindSecurityConfig(realm, new FakeSecurityConfig(SYSTEM_USER, " ")); + + assertThatThrownBy(() -> realm.activate(componentProperties())) + .as("a blank manager password must abort activation, not publish an empty realm") + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("goss.system.manager.password"); + + assertThat(realm.hasIdentifier(SYSTEM_USER)).isFalse(); + assertThat(realm.getAuthenticationInfo(new UsernamePasswordToken(SYSTEM_USER, " "))) + .isNull(); + } + + @Test + @DisplayName("Activation with no bound SecurityConfig fails closed instead of NPEing into an empty realm") + public void activationFailsClosedWithoutSecurityConfig() { + SystemBasedRealm realm = new SystemBasedRealm(); + + assertThatThrownBy(() -> realm.activate(componentProperties())) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("SecurityConfig"); + + assertThat(realm.hasIdentifier(SYSTEM_USER)).isFalse(); + } + + @Test + @DisplayName("Component properties are never treated as the credential source") + public void componentPropertiesAreNotACredentialSource() throws Exception { + SystemBasedRealm realm = new SystemBasedRealm(); + bindSecurityConfig(realm, new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD)); + + // systemrealm.cfg carries only load=true; a null property map must not + // change which principal the realm serves. + assertThatCode(() -> realm.activate(Collections.emptyMap())).doesNotThrowAnyException(); + + assertThat(realm.hasIdentifier(SYSTEM_USER)).isTrue(); + assertThat(realm.hasIdentifier("load")).isFalse(); + assertThat(realm.getPermissions(SYSTEM_USER)).containsExactly(SYSTEM_PERMISSIONS); + } +} From dc789a60c64cf3449bb73431aa14216571f27cf4 Mon Sep 17 00:00:00 2001 From: Luca Date: Sat, 1 Aug 2026 17:34:48 -0700 Subject: [PATCH 2/2] fix(security): load the system account from DS activation, not Shiro onInit() SystemBasedRealm built its only account inside onInit(), Shiro's Initializable hook. In this deployment nothing calls it. onInit() is protected and reachable only from AuthenticatingRealm.init(), which is normally driven by LifecycleUtils.init() from an ini or Spring bootstrap. Verified against shiro-core 2.0.0 bytecode: RealmSecurityManager.setRealms() reaches afterRealmsSet() -> applyCacheManagerToRealms() and applyEventBusToRealms(), never init(); LifecycleUtils appears only in destroy(). goss-core has no init() or LifecycleUtils call site. The realm therefore activated with a permanently empty userMap. The failure only surfaced where SystemBasedRealm was the sole registered realm: the reported message comes from ModularRealmAuthenticator's doSingleRealmAuthentication, which runs only when getRealms().size() == 1. With PropertyBasedRealm also present, AtLeastOneSuccessfulStrategy let the system principal authenticate through that realm instead, which is why a complete bundle set masked the defect. Changes: * The account load moves into a single idempotent loadSystemAccount(), and activate(), the modified callback, the SecurityConfig bind and updated callbacks, and onInit() all delegate to it. onInit() is kept wired so the realm stays correct under a host that does drive Shiro's lifecycle. * loadSystemAccount() clears before rebuilding, so a renamed manager user cannot leave the previous principal authenticating. * Missing SecurityConfig, or a blank goss.system.manager or goss.system.manager.password, now throws IllegalStateException naming the property and file. Out of activate() that aborts DS activation, so the realm service is never published, the Activator's mandatory realm.type=system reference stays unsatisfied, the SecurityManager service is never published, and no broker connection is made against an empty realm. * The SecurityConfig reference becomes a dynamic greedy method reference with unbind and updated callbacks. The manager credential comes from the pnnl.goss.security PID via SecurityConfigImpl, not from this component's own PID, so an edit to pnnl.goss.security.cfg only updates that service's registration properties. A field reference gave no callback for that and the realm kept a superseded credential. Unbind clears the account map. The system permission string, the ConfigurationPolicy.REQUIRE contract on pnnl.goss.core.security.systemrealm (GOSS-004), and the realm.type=system marker (GADP-012) are all unchanged. Also guards Activator.registerAllRealms() against an empty realm set. Shiro rejects setRealms(emptySet) with IllegalArgumentException, so removing the last realm threw out of realmRemoved() on every teardown. The guard declines the write and logs; it does not widen the authorization surface, because the realm reference is AT_LEAST_ONE and DS deactivates the component and unpublishes the SecurityManager service along with it. GREEN: 55 tests in pnnl.goss.core, up from 36. Full gradle build passes. Refs GOSS-025 --- .../goss/core/security/impl/Activator.java | 23 +++ .../security/system/SystemBasedRealm.java | 152 ++++++++++++++++-- .../impl/test/ActivatorRealmRemovalTest.java | 6 +- .../test/SystemRealmActivationOrderTest.java | 63 ++++++++ .../test/SystemBasedRealmActivationTest.java | 127 ++++++++++++--- 5 files changed, 333 insertions(+), 38 deletions(-) diff --git a/pnnl.goss.core/src/pnnl/goss/core/security/impl/Activator.java b/pnnl.goss.core/src/pnnl/goss/core/security/impl/Activator.java index 73f0c293..133a0e25 100644 --- a/pnnl.goss.core/src/pnnl/goss/core/security/impl/Activator.java +++ b/pnnl.goss.core/src/pnnl/goss/core/security/impl/Activator.java @@ -115,11 +115,34 @@ public void realmRemoved(ServiceReference 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 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()); diff --git a/pnnl.goss.core/src/pnnl/goss/core/security/system/SystemBasedRealm.java b/pnnl.goss.core/src/pnnl/goss/core/security/system/SystemBasedRealm.java index 1aa6da87..1df5d27b 100644 --- a/pnnl.goss.core/src/pnnl/goss/core/security/system/SystemBasedRealm.java +++ b/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 userMap = new ConcurrentHashMap<>(); private final Map> 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 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 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 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 properties) { + log.info("Reloading SystemBasedRealm after a configuration update"); + loadSystemAccount(); } @Override diff --git a/pnnl.goss.core/test/pnnl/goss/core/security/impl/test/ActivatorRealmRemovalTest.java b/pnnl.goss.core/test/pnnl/goss/core/security/impl/test/ActivatorRealmRemovalTest.java index b4c5dfef..411d67e0 100644 --- a/pnnl.goss.core/test/pnnl/goss/core/security/impl/test/ActivatorRealmRemovalTest.java +++ b/pnnl.goss.core/test/pnnl/goss/core/security/impl/test/ActivatorRealmRemovalTest.java @@ -32,9 +32,9 @@ * * 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. + * 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 { diff --git a/pnnl.goss.core/test/pnnl/goss/core/security/impl/test/SystemRealmActivationOrderTest.java b/pnnl.goss.core/test/pnnl/goss/core/security/impl/test/SystemRealmActivationOrderTest.java index 0de2af90..cda86e9e 100644 --- a/pnnl.goss.core/test/pnnl/goss/core/security/impl/test/SystemRealmActivationOrderTest.java +++ b/pnnl.goss.core/test/pnnl/goss/core/security/impl/test/SystemRealmActivationOrderTest.java @@ -159,6 +159,55 @@ public void systemRealmPublishesDistinguishingProperty() throws Exception { .isTrue(); } + @Test + @DisplayName("GOSS-025: system realm hangs its account load off container-driven entry points, not Shiro init()") + public void systemRealmDeclaresContainerDrivenLifecycleEntryPoints() throws Exception { + Document doc = descriptor("pnnl.goss.core.security-system.jar", + "pnnl.goss.core.security.system.SystemBasedRealm"); + Element component = doc.getDocumentElement(); + + // The GOSS-025 defect was that the ONLY population path was Shiro's + // onInit(), which nothing in this deployment calls. These attributes are the + // paths Declarative Services is contractually guaranteed to invoke, so the + // descriptor is where that guarantee has to be asserted. + assertThat(component.getAttribute("activate")) + .as("DS must have an activate method to drive; without it the realm loads from nothing") + .isEqualTo("activate"); + assertThat(component.getAttribute("modified")) + .as("a configuration update must reach the realm rather than silently deactivating it") + .isEqualTo("updated"); + assertThat(component.getAttribute("configuration-policy")) + .as("GOSS-004 documents the required systemrealm.cfg dependency deliberately") + .isEqualTo("require"); + assertThat(component.getAttribute("configuration-pid")) + .isEqualTo("pnnl.goss.core.security.systemrealm"); + } + + @Test + @DisplayName("GOSS-025: the system realm's SecurityConfig reference is mandatory and notifies on change") + public void systemRealmSecurityConfigReferenceIsMandatoryAndNotifying() throws Exception { + Document doc = descriptor("pnnl.goss.core.security-system.jar", + "pnnl.goss.core.security.system.SystemBasedRealm"); + Element ref = referenceByInterface(doc, "pnnl.goss.core.security.SecurityConfig"); + assertNotNull(ref, "the system realm must declare a SecurityConfig reference; it is the credential source"); + + String cardinality = ref.getAttribute("cardinality"); + assertThat(cardinality.isEmpty() || "1..1".equals(cardinality)) + .as("the credential source must be mandatory so DS cannot activate an unauthenticatable realm;" + + " was '%s'", cardinality) + .isTrue(); + assertThat(ref.getAttribute("bind")) + .as("a bind callback is what lets the realm load on wiring rather than on an uncalled hook") + .isEqualTo("setSecurityConfig"); + assertThat(ref.getAttribute("unbind")) + .as("an unbind callback is what lets the realm fail closed when the credential source goes") + .isEqualTo("unsetSecurityConfig"); + assertThat(ref.getAttribute("updated")) + .as("pnnl.goss.security.cfg edits change the bound service's properties, not the instance;" + + " without this callback a rotated manager credential is never picked up") + .isEqualTo("updatedSecurityConfig"); + } + @Test @DisplayName("SecurityManager component gates activation on the system realm via a mandatory target filter") public void activatorReferencesSystemRealmWithTargetFilter() throws Exception { @@ -297,6 +346,20 @@ private JarEntry findByComponentName(JarFile jar, String componentName) throws I return null; } + private Element referenceByInterface(Document doc, String interfaceName) { + NodeList refs = doc.getElementsByTagName("reference"); + for (int i = 0; i < refs.getLength(); i++) { + Node node = refs.item(i); + if (node instanceof Element) { + Element ref = (Element) node; + if (interfaceName.equals(ref.getAttribute("interface"))) { + return ref; + } + } + } + return null; + } + private Element referenceByName(Document doc, String refName) { NodeList refs = doc.getElementsByTagName("reference"); for (int i = 0; i < refs.getLength(); i++) { diff --git a/pnnl.goss.core/test/pnnl/goss/core/security/system/test/SystemBasedRealmActivationTest.java b/pnnl.goss.core/test/pnnl/goss/core/security/system/test/SystemBasedRealmActivationTest.java index b7d24635..700b142a 100644 --- a/pnnl.goss.core/test/pnnl/goss/core/security/system/test/SystemBasedRealmActivationTest.java +++ b/pnnl.goss.core/test/pnnl/goss/core/security/system/test/SystemBasedRealmActivationTest.java @@ -4,7 +4,6 @@ import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import java.lang.reflect.Field; import java.util.Collections; import java.util.Map; import java.util.Set; @@ -23,17 +22,17 @@ /** * Covers the GOSS-025 bootstrap failure: SystemBasedRealm populated its account * map only from Shiro's {@code onInit()} hook, and nothing in this OSGi - * deployment ever calls {@code Initializable.init()}. Declarative Services calls - * {@code @Activate}; Shiro's RealmSecurityManager.setRealms() reaches only - * afterRealmsSet() -> applyCacheManagerToRealms(), never init(). So the realm - * activated with a permanently empty userMap and every system/manager + * deployment ever calls {@code Initializable.init()}. Declarative Services + * calls {@code @Activate}; Shiro's RealmSecurityManager.setRealms() reaches + * only afterRealmsSet() -> applyCacheManagerToRealms(), never init(). So the + * realm activated with a permanently empty userMap and every system/manager * authentication against it threw UnknownAccountException, which broke the * broker connect in GridOpticsServer.start(). * - * The load-bearing assertion in this class is that the realm resolves the system - * account after DS activation WITHOUT any call to init(). Everything else here - * guards the surrounding lifecycle: config reload, reference rebind, and fail- - * closed behavior when the credential source is unusable. + * The load-bearing assertion in this class is that the realm resolves the + * system account after DS activation WITHOUT any call to init(). Everything + * else here guards the surrounding lifecycle: config reload, reference rebind, + * and fail- closed behavior when the credential source is unusable. */ public class SystemBasedRealmActivationTest { @@ -90,14 +89,11 @@ public String createToken(Object userId, Set roles) { } /** - * Simulates the Declarative Services reference injection for the - * securityConfig reference. The production reference is a field reference - * today, so this mirrors it exactly. + * Simulates the Declarative Services bind callback for the securityConfig + * reference. DS invokes bind methods before @Activate. */ - private static void bindSecurityConfig(SystemBasedRealm realm, SecurityConfig config) throws Exception { - Field field = SystemBasedRealm.class.getDeclaredField("securityConfig"); - field.setAccessible(true); - field.set(realm, config); + private static void bindSecurityConfig(SystemBasedRealm realm, SecurityConfig config) { + realm.setSecurityConfig(config); } /** The DS-supplied component properties: systemrealm.cfg carries load=true. */ @@ -221,29 +217,90 @@ public void modifiedDoesNotLeaveStaleAccounts() throws Exception { } @Test - @DisplayName("Activation fails closed with an empty realm when the manager user is absent") - public void activationFailsClosedWithoutManagerUser() throws Exception { + @DisplayName("A rebound SecurityConfig replaces the account rather than adding to it") + public void rebindReplacesTheSystemAccount() { SystemBasedRealm realm = new SystemBasedRealm(); - bindSecurityConfig(realm, new FakeSecurityConfig(null, SYSTEM_PASSWORD)); + FakeSecurityConfig first = new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD); + bindSecurityConfig(realm, first); + realm.activate(componentProperties()); - assertThatThrownBy(() -> realm.activate(componentProperties())) - .as("a missing manager user must abort activation, not publish an empty realm") + realm.setSecurityConfig(new FakeSecurityConfig("operator", "rotated")); + realm.unsetSecurityConfig(first); + + assertThat(realm.hasIdentifier(SYSTEM_USER)) + .as("greedy rebind must not leave the previous credential source's account behind") + .isFalse(); + assertThat(realm.hasIdentifier("operator")).isTrue(); + assertThat(realm.getPermissions("operator")).containsExactly(SYSTEM_PERMISSIONS); + + AuthenticationInfo info = realm.getAuthenticationInfo( + new UsernamePasswordToken("operator", "rotated")); + assertThat(info.getCredentials()).isEqualTo("rotated"); + } + + @Test + @DisplayName("An updated SecurityConfig service picks up a rotated credential without reactivation") + public void updatedReferenceCallbackReloadsCredentials() { + SystemBasedRealm realm = new SystemBasedRealm(); + FakeSecurityConfig config = new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD); + bindSecurityConfig(realm, config); + realm.activate(componentProperties()); + + // pnnl.goss.security.cfg edited: SecurityConfigImpl's modified method runs + // on the same instance, DS updates its service properties, and DS calls the + // updated= callback on this reference. + config.managerPassword = "rotated"; + realm.updatedSecurityConfig(config); + + assertThatThrownBy(() -> realm.getAuthenticationInfo( + new UsernamePasswordToken(SYSTEM_USER, SYSTEM_PASSWORD))) + .isInstanceOf(IncorrectCredentialsException.class); + assertThat(realm.getAuthenticationInfo(new UsernamePasswordToken(SYSTEM_USER, "rotated")) + .getCredentials()).isEqualTo("rotated"); + } + + @Test + @DisplayName("Unbinding the credential source empties the realm rather than leaving it authenticating") + public void unbindFailsClosed() { + SystemBasedRealm realm = new SystemBasedRealm(); + FakeSecurityConfig config = new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD); + bindSecurityConfig(realm, config); + realm.activate(componentProperties()); + + realm.unsetSecurityConfig(config); + + assertThat(realm.hasIdentifier(SYSTEM_USER)).isFalse(); + assertThat(realm.getPermissions(SYSTEM_USER)).isEmpty(); + assertThat(realm.getAuthenticationInfo(new UsernamePasswordToken(SYSTEM_USER, SYSTEM_PASSWORD))) + .isNull(); + } + + @Test + @DisplayName("Wiring fails closed with an empty realm when the manager user is absent") + public void wiringFailsClosedWithoutManagerUser() { + SystemBasedRealm realm = new SystemBasedRealm(); + + assertThatThrownBy(() -> realm.setSecurityConfig(new FakeSecurityConfig(null, SYSTEM_PASSWORD))) + .as("a missing manager user must abort DS wiring, not publish an empty realm") .isInstanceOf(IllegalStateException.class) .hasMessageContaining("goss.system.manager"); assertThat(realm.hasIdentifier(SYSTEM_USER)).isFalse(); assertThat(realm.getAuthenticationInfo(new UsernamePasswordToken(SYSTEM_USER, SYSTEM_PASSWORD))) .isNull(); + assertThatThrownBy(() -> realm.activate(componentProperties())) + .as("activation must not paper over the unusable credential source either") + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("goss.system.manager"); } @Test - @DisplayName("Activation fails closed with an empty realm when the manager password is absent") - public void activationFailsClosedWithoutManagerPassword() throws Exception { + @DisplayName("Wiring fails closed with an empty realm when the manager password is blank") + public void wiringFailsClosedWithoutManagerPassword() { SystemBasedRealm realm = new SystemBasedRealm(); - bindSecurityConfig(realm, new FakeSecurityConfig(SYSTEM_USER, " ")); - assertThatThrownBy(() -> realm.activate(componentProperties())) - .as("a blank manager password must abort activation, not publish an empty realm") + assertThatThrownBy(() -> realm.setSecurityConfig(new FakeSecurityConfig(SYSTEM_USER, " "))) + .as("a blank manager password must abort DS wiring, not publish an empty realm") .isInstanceOf(IllegalStateException.class) .hasMessageContaining("goss.system.manager.password"); @@ -264,6 +321,24 @@ public void activationFailsClosedWithoutSecurityConfig() { assertThat(realm.hasIdentifier(SYSTEM_USER)).isFalse(); } + @Test + @DisplayName("A configuration update that breaks the credential source empties the realm") + public void modifiedFailsClosedOnBrokenConfig() { + SystemBasedRealm realm = new SystemBasedRealm(); + FakeSecurityConfig config = new FakeSecurityConfig(SYSTEM_USER, SYSTEM_PASSWORD); + bindSecurityConfig(realm, config); + realm.activate(componentProperties()); + + config.managerPassword = null; + assertThatThrownBy(() -> realm.updated(componentProperties())) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("goss.system.manager.password"); + + assertThat(realm.hasIdentifier(SYSTEM_USER)) + .as("a failed reload must not leave the superseded account authenticating") + .isFalse(); + } + @Test @DisplayName("Component properties are never treated as the credential source") public void componentPropertiesAreNotACredentialSource() throws Exception {