Repository navigation
[#1196] Accept the DNS aliases of the realms as valid host names, so that a realm alias no longer needs its own entry in the FQDN map - #1197
Conversation
…alid host names, so that a realm alias no longer needs its own entry in the FQDN map FqdnValidator now also accepts a host name that is a DNS alias of a realm. RealmDnsAliases keeps the aliases of all realms in memory and reads them again after the realm configuration changes, so a request with an unknown Host header does not search the configuration store. The aliases are only consulted for a host missing from the FQDN map, and only once OpenAM is configured. Fixes OpenIdentityPlatform#1196
maximthomas
left a comment
There was a problem hiding this comment.
praise: The snapshot logic is careful, and the two tests that guard it really do catch regressions.
RealmDnsAliases.reload()takesconfigVersionbefore the read, so a change notified during the read forces another read.shouldReadRealmsAgainWhenRealmConfigurationChangesWhileTheyAreReadholds it: with the version taken after the read it goes red (1 of 15 tests fails).expose(RealmDnsAliases.class)is pinned too. Without it,RealmGuiceModuleTestfails withGuice/ChildBindingAlreadySet.FqdnValidator.isRealmDnsAliasruns only for a host that is not an fqdnMap value, and only afterAMSetupServlet.isCurrentConfigurationValid().
issue (blocking): init() marks itself initialised before the listener is registered. If the registration fails or comes late, the alias set stays frozen until restart.
openam-core/src/main/java/org/forgerock/openam/core/realms/RealmDnsAliases.java:82-89
compareAndSet(false, true) runs before idRepoServiceProvider.get().addListener(...), and nothing ever resets the flag. Three things can leave the instance without a listener for the life of the JVM, with no retry:
- the provider throws.
RealmGuiceModule.provideIdRepoServicewrapsSMSException/SSOExceptionin aRuntimeException, and the admin token action can also throw. addListenerreturns null.- a concurrent first caller loses the CAS. It then reads and stores a snapshot before the winner has registered, and a realm change in that gap is never counted.
From then on configVersion never moves, so the first stored snapshot is served for good. An alias the admin removes stays an accepted Host, which master rejects, and a new alias keeps getting the 302/400. The description says "a stale set is never kept".
private void init() {
if (initialised.get()) {
return;
}
synchronized (this) {
if (initialised.get()) {
return;
}
if (idRepoServiceProvider.get().addListener(new RealmsChangeListener()) == null) {
logger.warn("RealmDnsAliases: Failed to add ServiceListener, will retry on the next request");
return;
}
// Discards a snapshot read while no listener was registered
configVersion.incrementAndGet();
initialised.set(true);
}
}If the provider throws, initialised now stays false and the next call retries. The throw itself is the non-blocking issue below. shouldRegisterServiceListenerOnlyOnce stays green.
Pin: let addListener return null and then "LISTENER_ID", and in a second case let the idRepo provider throw once. In both, change aliasesByRealm between two contains() calls without notifying, and assert that the second call sees the change. Both cases are red on the head.
question (blocking): Should a host that is an fqdnMap key and also a realm alias still be redirected to its mapped FQDN?
openam-core/src/main/java/com/sun/identity/common/FqdnValidator.java:110, :126-131; openam-core/src/main/java/com/sun/identity/setup/ConfigureData.java:134-138
getFullyQualifiedHostName(h) reads fqdnMap.get(h) only when isHostnameValid(h) is false. isHostnameValid checks the map's values and, since this PR, the realm aliases, but never the map's keys. By default the root realm's aliases include the RDN of the root suffix, which is openam for dc=openam,dc=openidentityplatform,dc=org. Now take com.sun.identity.server.fqdnMap[openam]=openam.example.com, the documented short-name mapping. On master, GET http://openam:8080/openam/XUI/ gets a 302 to openam.example.com. On this head the request is served on the short host, where cookies scoped to .example.com do not apply. ConsoleServletBase.validateHost also loses the mapping. The description says that mapped hosts are not affected. That is true for map values but not for keys.
If it is intended, the description must say that such mappings stop redirecting, and the test suggested below must pin the head's result (null). If it is not intended, it is a blocking regression, and this is the fix:
return fqdnMap.values().contains(lowerCaseHostname)
|| (!fqdnMap.containsKey(lowerCaseHostname) && isRealmDnsAlias(lowerCaseHostname));fqdnMap keys are already lower-cased (FqdnValidator.java:86). getFullyQualifiedHostName and the console both go through isHostnameValid, so they follow.
issue (non-blocking): A RuntimeException from the alias lookup now escapes isHostnameValid. The result is a 500 instead of the rejection that the description promises.
openam-core/src/main/java/org/forgerock/openam/core/realms/RealmDnsAliases.java:74-80, :109; openam-core/src/main/java/com/sun/identity/common/FqdnValidator.java:113-117
reload() catches only SMSException. Neither contains, isRealmDnsAlias, AuthClientUtils.isValidFQDNRequest nor CoreWrapper.isValidFQDN catches anything else. The exception can come from the AMSecurityPropertiesException of AdminTokenAction through CoreGuiceModule.provideAdminSSOToken, or from the RuntimeException that the idRepo provider throws in init(). In both cases a request with an unmapped Host gets a container 500 from FQDNValidationFilter and an unchecked error from RealmContextFilter. Before this PR it got a 302 or a 400, because isHostnameValid could not throw. Catching in contains() covers both sources, init() included:
public boolean contains(String hostname) {
if (hostname == null) {
return false;
}
try {
init();
return getAliases().contains(hostname.toLowerCase());
} catch (RuntimeException e) {
logger.warn("RealmDnsAliases: Unable to read the DNS aliases of the realms", e);
return false;
}
}question (non-blocking): Is the admin console meant to be served on every realm-alias host?
openam-console/src/main/java/com/sun/identity/console/base/ConsoleServletBase.java:362; openam-core/src/main/java/com/sun/identity/authentication/client/AuthClientUtils.java:1585
validateHost calls getFullyQualifiedHostName, which now returns null for an alias. Take Host: acme.example.com, an alias of /acme, on /base/AMAdminFrame: master sends a 302 to the server host, while this head serves the console on the alias host. The description says "All three places that check the host are affected" and lists only the three filters. As a gap in the description this is minor. It becomes blocking if the console was meant to stay on the server host. If the change is intended, list ConsoleServletBase and AuthClientUtils.getValidFQDNResource in the description.
suggestion (non-blocking): No test pins what an fqdnMap key that is also a realm alias resolves to.
openam-core/src/test/java/com/sun/identity/common/FqdnValidatorTest.java:49-50, :77, :83
The setup maps fqdnMap[openam], but the realmDnsAliases mock answers false for openam. getFullyQualifiedHostName is called only for acme.example.com and attacker.example.org. Master's key-first order and this head's alias-first order both pass all 6 tests. The test below fails on the current head and passes with the fix above. If the head's result is intended, assert isNull() instead.
@Test
public void shouldRedirectFqdnMapKeyThatIsAlsoARealmDnsAlias() {
given(realmDnsAliases.contains("openam")).willReturn(true);
assertThat(FqdnValidator.getInstance().getFullyQualifiedHostName("openam")).isEqualTo("openam.example.com");
}suggestion (non-blocking): RealmDnsAliasesTest stubs out the whole realm read, so no test runs the code that reads aliases from the configuration.
openam-core/src/test/java/org/forgerock/openam/core/realms/RealmDnsAliasesTest.java:73-102
The anonymous subclass overrides both getSubRealms and getRealmAliases. As a result RealmDnsAliases.java:131-145 never runs and the organizationConfigManagerFactory mock is never called. All 21 tests would pass with getSubOrganizationNames("*", false), with ORGANIZATION_STATUS_ATTR read instead of ORGANIZATION_ALIAS_ATTR, or with the "/" prefix dropped. The current code is correct. Only one test is needed on a RealmDnsAliases that is not subclassed, and it fails against each of those mutants:
@Test
public void shouldReadDnsAliasesOfNestedRealmsFromTheConfiguration() throws Exception {
OrganizationConfigManager root = mock(OrganizationConfigManager.class);
OrganizationConfigManager sub = mock(OrganizationConfigManager.class);
given(organizationConfigManagerFactory.create(adminToken, "/")).willReturn(root);
given(organizationConfigManagerFactory.create(adminToken, "/acme/sub")).willReturn(sub);
given(root.getSubOrganizationNames("*", true)).willReturn(asSet("acme/sub"));
given(sub.getAttributes(IdConstants.REPO_SERVICE)).willReturn(
Collections.singletonMap(IdConstants.ORGANIZATION_ALIAS_ATTR, asSet("sub.acme.example.com")));
RealmDnsAliases aliases = new RealmDnsAliases(() -> adminToken, organizationConfigManagerFactory,
() -> idRepoService);
assertThat(aliases.contains("sub.acme.example.com")).isTrue();
}issue (non-blocking): In AMSDK coexistence mode, an alias change can leave the old aliases stored under the new version.
openam-core/src/main/java/com/sun/identity/sm/OrganizationConfigManager.java:1032-1045, :797-820
In coexistence mode, setAttributes first saves the SMS realm entry, which schedules the notification, and only then calls amsdk.setAttributes. getAttributes returns the merged AMSDK aliases. Suppose a contains() call lands between the two writes. It captures the new version, reads the old AMSDK value and stores it, so the new alias is rejected and the old one accepted until the next realm change. This contradicts "a stale set is never kept". Not run: it needs a coexistence deployment and a concurrent probe. A sentence in the Javadoc of RealmDnsAliases is enough.
Fixes #1196
Problem
A request addressed to a realm DNS alias is rejected by the FQDN check before the realm is resolved, unless the alias also has its own
com.sun.identity.server.fqdnMap[...]entry. The valid host names come only from the FQDN map, the server host and the site hosts thatNamingService.updateFqdnMappingsadds, so a realm alias is never among them. All three places that check the host are affected:FQDNValidationFilter(/XUI/*,/oauth2/authorize,/oauth2/device/user) redirects with 302 to the default host.LoginServlet(/UI/Login) redirects with 302 to the default host.RealmContextFilter(REST) answers 400FQDN "acme.example.com" is not valid.The strict check came with OPENAM-7340 to stop Host header injection, and the realm aliases were left out of it. The OpenAM guide does not ask for the extra FQDN map entry. PingAM, which continues ForgeRock AM, adds that entry automatically when an alias is saved in the admin UI.
Fix
FqdnValidator.isHostnameValidalso accepts a host name that is a DNS alias of a realm. It consults the aliases only for a host missing from the FQDN map, and only once OpenAM is configured (AMSetupServlet.isCurrentConfigurationValid(), asMonitoringConfigurationdoes). Requests to mapped hosts and the configurator are not affected.RealmDnsAliaseskeeps the aliases of all realms in memory, in lower case. A listener onsunIdentityRepositoryService, the same oneCachingRealmLookuprelies on, makes it read them again after the realm configuration changes. The aliases are read from the realms themselves, so it does not matter whether an alias was changed through the admin console, REST orssoadm, and nothing is left behind in the FQDN map when an alias is removed.RealmGuiceModulebinds and exposesRealmDnsAliases, because the@Named(REPO_SERVICE)ServiceConfigManagerit needs is bound only inside that private module.An alias is configured by the administrator, just like an FQDN map entry, so the list of accepted host names stays under the administrator's control. By default the root realm's aliases hold the server host names and the RDN value of the root suffix (for example
openam), so these become valid host names as well.Test
RealmDnsAliasesTest(15 tests): aliases of the root realm and of subrealms, letter case, an alias added or removed and a realm created after a configuration change, one read while the configuration is unchanged, a change notified during the read, and a failed read.FqdnValidatorTest(6 tests): a realm alias is accepted and not redirected, an unknown host is still redirected to the default host, and the aliases are not consulted for a mapped host or before OpenAM is configured.RealmGuiceModuleTest: the injector providesRealmDnsAliasesas a single instance.expose(RealmDnsAliases.class)fails the Guice test withChildBindingAlreadySet.mvn -pl openam-core,openam-rest -am clean test: openam-core 2236 tests and openam-rest 285 tests, no failures.The change was not tried on a running OpenAM.