[#1109] Keep a connection handler listening when a change to it is rejected - #1110
Conversation
vharseko
added
bug
tests
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The check of a proposed configuration no longer touches the running handler, and the new test proves it both ways.
isConfigurationAcceptablenow callscreateSSLContext(config, false)(LDAPConnectionHandler2.java:582, and the same inLDAPConnectionHandlerandHTTPConnectionHandler), whileconfigureSSLkeeps passingtruefor the start and the apply.RejectedSSLConfigurationChangeTestCaseagainst the base's three handler files: 6 of 9 fail, all sixrejectedChangeKeepsTheHandlerListeningcases, each on "the handler does not serve TLS". The threehandlerWithoutItsCertificateDoesNotListencases pass. At the head it is 9/9 green in CI (build-maven (ubuntu-latest, 11)).- The three identical disable blocks in
HTTPConnectionHandler.createSSLContextare folded intodisableAndWarn(forUse).
issue (non-blocking): For HTTPConnectionHandler, the new forUse javadoc says an applied change disables a handler without a usable key, but an HTTP apply never leaves the handler disabled.
opendj-server-legacy/src/main/java/org/opends/server/protocols/http/HTTPConnectionHandler.java:825-829, :216, :245
applyConfigurationChange calls configureSSL(config) at :216, and disableAndWarn(true) there logs "Disabling …" and sets enabled = false. Then :245 runs this.enabled = this.currentConfig.isEnabled();, which undoes it. So an accepted HTTPS change that names an ssl-cert-nickname missing from the key store logs the warning while the handler keeps listening. It keeps its old certificate, because every SSL property is in anyChangeRequiresRestart (:250-266). This behaviour predates the PR. What is new is the documented contract, which is false for HTTP. The commit body makes the same claim ("Only the start of the handler and an applied change disable it"). The LDAP handlers set enabled before configureSSL (LDAPConnectionHandler2.java:293/:299, LDAPConnectionHandler.java:299/:306), so the javadoc holds for them.
* @param forUse
* {@code true} when the handler is going to use the configurator: at its start a handler
* without a usable key is disabled ({@link #applyConfigurationChange} sets {@code enabled}
* from the configuration afterwards); {@code false} when the configurator only checks a
* proposed configuration, which must leave the running handler as it isOr: move this.enabled = … above configureSSL(config) so HTTP matches the LDAP handlers. That changes behaviour and falls outside this PR.
suggestion (non-blocking): No case drives the applied-change road that the new javadocs name ("at its start or when a change is applied").
opendj-server-legacy/src/test/java/org/opends/server/protocols/RejectedSSLConfigurationChangeTestCase.java:201, opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java:293-299
The start (handlerWithoutItsCertificateDoesNotListen) and the check (rejectedChangeKeepsTheHandlerListening) are pinned, but no test calls applyConfigurationChange on a running SSL handler. So moving enabled = config.isEnabled() below configureSSL(config) in LDAPConnectionHandler2.applyConfigurationChange, as HTTP has it, keeps all 9 cases green. The check accepts a missing nickname, because wrapping an empty alias set does not throw, so dsconfig can reach this road. The gap predates the PR.
/** An applied change that leaves the handler without its certificate still disables it. */
@SuppressWarnings("unchecked")
@Test
public void appliedChangeWithoutItsCertificateStopsListening() throws Exception
{
final int port = TestCaseUtils.findFreePort();
final ConnectionHandler<?> handler = start(Kind.LDAP2, configuration(Kind.LDAP2, port, null, "5 megabytes", true));
try
{
assertServesTLS(port);
((ConfigurationChangeListener<LDAPConnectionHandlerCfg>) handler).applyConfigurationChange(
(LDAPConnectionHandlerCfg) configuration(Kind.LDAP2, port, "no-such-cert", "5 megabytes", true));
final long deadline = System.currentTimeMillis() + KEEPS_SERVING_MS;
while (true)
{
try (Socket socket = new Socket("127.0.0.1", port))
{
assertTrue(System.currentTimeMillis() < deadline, "the handler still listens after the applied change");
}
catch (ConnectException expected)
{
break;
}
Thread.sleep(250);
}
}
finally
{
((ServerShutdownListener) handler).processServerShutdown(STOP_REASON);
handler.finalizeConnectionHandler(STOP_REASON);
handler.join(10000);
assertFalse(handler.isAlive(), "the connection handler thread is still running");
}
}Pin: under the mutant the listener stays up and the connect succeeds. At the head, run() stops the listener. Not run. This pin checks the connect, which is why it covers only LDAP2. The legacy handler can't be pinned the same way: when it is disabled while running it still accepts the TCP connect and only the handshake times out (measured against the base), and after this change its SSL context holds no key, so a failed handshake shows nothing.
suggestion (non-blocking): No test sets forUse in the null-key-manager-provider branch.
opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java:966-969, opendj-server-legacy/src/main/java/org/opends/server/protocols/http/HTTPConnectionHandler.java:910-914, LDAPConnectionHandler.java:1328
configuration() always names the provider that setUp registers, so keyManagerProvider == null is never reached. Restoring true in that branch, which is the base's code there, leaves the suite green. Production can reach the branch. An enabled provider whose key store failed to load at startup is never registered (KeyManagerProviderConfigManager skips it). The aggregation check reads the config entry, not the registry. So a modify that names such a provider together with an unsupported ssl-protocol passes through this branch during the check and is then refused. Under the mutant, that refused change stops the listener, which is #1109 again.
// configuration(...) gains a trailing DN keyManagerDN parameter used in place of KEY_MANAGER_DN;
// the existing five-argument form delegates with KEY_MANAGER_DN.
/** A check that finds no registered key manager provider leaves the running handler as it is. */
@Test(dataProvider = "kinds")
public void checkWithoutKeyManagerProviderKeepsTheHandlerListening(Kind kind) throws Exception
{
final int port = TestCaseUtils.findFreePort();
final ConnectionHandler<?> handler = start(kind, configuration(kind, port, null, "5 megabytes", true));
try
{
handler.isConfigurationAcceptable(configuration(kind, port, null, "6 megabytes", true,
DN.valueOf("cn=No Such Keys,cn=Key Manager Providers,cn=config")), new ArrayList<LocalizableMessage>());
final long deadline = System.currentTimeMillis() + KEEPS_SERVING_MS;
do
{
assertServesTLS(port);
Thread.sleep(250);
}
while (System.currentTimeMillis() < deadline);
}
finally
{
((ServerShutdownListener) handler).processServerShutdown(STOP_REASON);
handler.finalizeConnectionHandler(STOP_REASON);
handler.join(10000);
assertFalse(handler.isAlive(), "the connection handler thread is still running");
}
}Pin: the key store is left intact, so the running context keeps its key, and under the mutant assertServesTLS fails within the 3 s window for all three kinds. Not run. I haven't checked whether InitializationUtils.getConfiguration decodes a reference to a missing entry.
suggestion (non-blocking): rejectedChangeKeepsTheHandlerListening accepts any rejection reason, not specifically the SSL one.
opendj-server-legacy/src/test/java/org/opends/server/protocols/RejectedSSLConfigurationChangeTestCase.java:174
The PR says the change "is still rejected with the reason it gave before", but the case only asserts !reasons.isEmpty(). A refusal that never reaches createSSLContext(config, false) also passes. One example: force the port-check guard (currentConfig == null || …) to true. The check then refuses with address-in-use on the port the running handler holds, the handler keeps serving, and the case stays green. Today every exception on the check road of all three handlers is wrapped into ERR_CONNHANDLER_SSL_CANNOT_INITIALIZE, so the reason holds, but nothing pins it.
assertFalse(reasons.isEmpty(), "the change was rejected without a reason");
assertEquals(reasons.get(0).ordinal(), ERR_CONNHANDLER_SSL_CANNOT_INITIALIZE.ordinal(), String.valueOf(reasons));Pin: with the static import from org.opends.messages.ProtocolMessages, the port-guard mutant makes the case fail. Not run.
…a change to it is rejected The check of a proposed configuration built its SSL context through the same code as the start of the handler, and that code disables the handler when its key store holds no usable key. A key store that cannot be loaded takes that branch, so the change was rejected and the running handler stopped listening, until a later change was accepted or the server restarted. The administration connector, an LDAPConnectionHandler2, and the HTTP connection handler behaved the same way; with the administration connector down, dsconfig could not reach the server any more. createSSLContext in LDAPConnectionHandler2, LDAPConnectionHandler and HTTPConnectionHandler now takes whether the handler is going to use the context. Only the start of the handler and an applied change disable it. The check still rejects the change with the reason it gave before. Fixes OpenIdentityPlatform#1109
…out a key manager provider The forUse javadoc of HTTPConnectionHandler no longer says that an applied change disables the handler: applyConfigurationChange sets enabled from the configuration after configureSSL, so only the start does. The previous commit says the same of all three handlers; that holds for the two LDAP handlers only. The HTTP behaviour predates this change and is OpenIdentityPlatform#1111. RejectedSSLConfigurationChangeTestCase gains: - appliedChangeWithoutItsCertificateStopsListening: an applied change that takes the certificate of LDAPConnectionHandler2 away is accepted by the check and stops the listener; - checkWithoutKeyManagerProviderKeepsTheHandlerListening, for the three handlers: a check that finds no key manager provider registered under the configured DN leaves the running handler serving TLS; - rejectedChangeKeepsTheHandlerListening checks that the change is rejected with ERR_CONNHANDLER_SSL_CANNOT_INITIALIZE, not with any reason.
c35c6c5 to
b4b0cad
Compare
|
Thanks. All four points are taken in b4b0cad. The branch is also rebased onto master 79980bd. issue: HTTP javadoc. Confirmed. suggestion: apply road. Added suggestion: null key manager provider. Added suggestion: rejection reason. Results: the class runs 13/13 at the head through the reactor ( |
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The second commit closes the test gaps left after round 1 and makes the HTTP javadoc match what the code does.
checkWithoutKeyManagerProviderKeepsTheHandlerListeningreaches the check-side null-provider branch of all three handlers by deregisteringKEY_MANAGER_DNafter the start.rejectedChangeKeepsTheHandlerListeningnow requiresERR_CONNHANDLER_SSL_CANNOT_INITIALIZE; before, any reason passed.- The
forUsejavadoc ofHTTPConnectionHandler.createSSLEngineConfiguratornow names only the start and points atapplyConfigurationChange. The HTTP apply behaviour is split out as #1111.
…ey the handler can present, and leave a running HTTP handler as it is when one is applied The SSL settings of the HTTP connection handler take effect only when it restarts: Grizzly takes the SSL engine configuration when the handler starts its HTTP server, and every SSL property is in anyChangeRequiresRestart. A change that left the handler without a key it can present (no key manager provider registered under the configured DN, a key store without a key, or none of the ssl-cert-nickname aliases in it) was accepted by the check, because wrapping an empty alias set does not throw. The apply then logged "Disabling ...", set enabled to false, and set it back from the configuration a few lines later, so the handler kept serving with its old certificate and was down only after the restart. A handler that its start had disabled for want of a key was even started by such a change, with an SSL context without a key. createSSLContext no longer touches enabled. It collects why the configuration leaves the handler without a key, and each caller decides what that means: - the check refuses the configuration with these reasons, for a change to a running handler and for a handler being added or enabled, which is checked on a new instance. A key store that cannot be loaded is still refused with ERR_CONNHANDLER_SSL_CANNOT_INITIALIZE, as before; - the start logs the reasons and "Disabling ...", and disables the handler, as before; - the apply, reached when the key store changed after the check, adds the reasons and the new WARN_CONNHANDLER_NO_KEY_UNTIL_RESTART to the result, asks for administrative action, and logs no "Disabling". A running handler keeps serving with the SSL settings it started with; a handler that is down stays down. A change that gives a handler disabled at its start a key still starts it, as before. The reasons name the configuration by config.name() rather than by friendlyName, which is not set on the new instance the check of an added or enabled handler runs on. A missing alias is reported by its own name rather than by the whole alias set, and while another configured alias is in the key store it is only logged, as before. The forUse flag and disableAndWarn that OpenIdentityPlatform#1110 added to the HTTP handler are replaced by the collected reasons. The LDAP handlers are left as they are: they build an SSL engine for each new connection, so their SSL changes take effect at once, and an applied change without a key disables them at once. RejectedSSLConfigurationChangeTestCase gains 11 HTTP cases: the check refusing each of the three kinds of missing key on a running handler and on a new instance, with the reason naming the handler; the check accepting a configuration with one of its aliases in the key store; the apply of a change without a key, by a changed nickname and by a key store emptied under an unchanged configuration; and a handler disabled at its start staying down on a change still without a key and starting on one that gives it a key.
2 participants
Fixes #1109
Problem
When a change to a connection handler is checked, the check builds an SSL context for the proposed configuration through the same
createSSLContextas the start of the handler. That method disables the handler (enabled = false) when its key store holds no usable key or lacks the configured alias. A key store that cannot be loaded (replaced by one with another password, truncated, missing) takes those branches, becausecontainsAtLeastOneKey()andcontainsKeyWithAlias()returnfalseon any load failure.getKeyManagers()then throws, so the change is rejected andapplyConfigurationChangenever runs to setenabledback. The handler thread stops the listener, and the configuration still saysenabled: true. The handler stays down after the key store is fixed, until a later change is accepted or the server restarts.The same happens to:
LDAPConnectionHandler2);LDAPConnectionHandler2checked throughAdministrationConnector.isConfigurationChangeAcceptable. Once port 4444 is down,dsconfigcannot reach the server any more; onlyldapmodifyoncn=Administration Connector,cn=configover plain LDAP, or a restart, brings it back;HTTPConnectionHandler), whosecreateSSLContextsetsenabled = falsedirectly;LDAPConnectionHandler, which has the same code asLDAPConnectionHandler2.All four were reproduced in Docker on
openidentityplatform/opendj:latest(5.1.2); the steps are in #1109.Change
createSSLContextinLDAPConnectionHandler2,LDAPConnectionHandlerandHTTPConnectionHandler(andHTTPConnectionHandler.createSSLEngineConfigurator) takes aforUseflag:configureSSL, called at the start of the handler and byapplyConfigurationChange, passestrue: an SSL handler without a usable key is still disabled at its start, and by an applied change to an LDAP handler, as before.HTTPConnectionHandler.applyConfigurationChangesetsenabledfrom the configuration afterconfigureSSL, so an applied change never disabled the HTTP handler; that is left as it was and filed as An applied change that leaves the HTTP connection handler without a usable key logs "Disabling" but the handler keeps serving until the restart #1111;isConfigurationAcceptable, reached fromisConfigurationChangeAcceptable,ConnectionHandlerConfigManagerandAdministrationConnector, passesfalse: the check leaves the running handler as it was.A key store that cannot be loaded still makes
getKeyManagers()throw, so the change is still rejected with the reason it gave before. The error messages about the key store are still logged during the check; only the "Disabling …" warning and the change toenabledare skipped. InHTTPConnectionHandlerthe three identical disable blocks are folded intodisableAndWarn(forUse).Tests
New
RejectedSSLConfigurationChangeTestCase, 13 cases, with its own file-based key manager provider on a temporary copy of the test key store:rejectedChangeKeepsTheHandlerListening, forLDAPConnectionHandler2, the legacyLDAPConnectionHandlerandHTTPConnectionHandler, each with and withoutssl-cert-nickname(so both the no-key branch and the alias branch are covered): start an SSL handler, check it serves TLS, overwrite the key store file with garbage, check that a change (max-request-size) is rejected withERR_CONNHANDLER_SSL_CANNOT_INITIALIZE, then check the handler keeps serving TLS for 3 seconds (its thread re-readsenabledevery second);handlerWithoutItsCertificateDoesNotListen, for the same three handlers: a handler whose key store lacks the configuredssl-cert-nicknameis still disabled at its start. This pins thetruepassed byconfigureSSL;checkWithoutKeyManagerProviderKeepsTheHandlerListening, for the same three handlers: the test's key manager provider is deregistered fromDirectoryServeraround a check (the state of an enabled provider whose key store could not be loaded at the start of the server: its entry exists, it is not registered), and the running handler keeps serving TLS for 3 seconds;appliedChangeWithoutItsCertificateStopsListening, forLDAPConnectionHandler2: a change tossl-cert-nickname: no-such-certis accepted by the check and, once applied, stops the listener. The legacy handler keeps accepting TCP connections while disabled, and the HTTP handler is not disabled by an applied change (An applied change that leaves the HTTP connection handler without a usable key logs "Disabling" but the handler keeps serving until the restart #1111), so neither can be pinned this way.tearDowndrops the referential integrity reference thataddLDAPChangeListener/addHTTPChangeListenerregisters from the handler to the key manager provider (it outlives the handler), so the provider entry can be deleted.Results:
rejectedChangeKeepsTheHandlerListeningcases fail (Connection refusedfor LDAP2 and HTTP,Read timed outfor the legacy handler, which accepts one more connection before it closes); with the change, 13/13 pass;forUseargument (the check in each handler, only the alias branch of LDAP2 and HTTP, only the no-key branch of LDAP2, the start path in each handler), are each caught by the expected cases. Seven more are each caught too: the base'struein the branch without a registered key manager provider, in each handler (bycheckWithoutKeyManagerProviderKeepsTheHandlerListeningfor its kind);enabled = config.isEnabled()moved belowconfigureSSLinLDAPConnectionHandler2.applyConfigurationChange(byappliedChangeWithoutItsCertificateStopsListening); and the port guard of the check forced totrueinLDAPConnectionHandler2andLDAPConnectionHandler(by the reason check ofrejectedChangeKeepsTheHandlerListening). The HTTP variant of that last mutant is not observable on macOS: the HTTP handler binds Grizzly to the wildcard address rather than tolisten-address, so the guard's probe on 127.0.0.1 binds next to it (The HTTP connection handler and the JMX RMI connector ignore listen-address and listen on every interface #1112);TestLDAPConnectionHandler,HTTPConnectionHandlerTestCase,StartTLSExtendedOperationTestCase,ExternalSASLMechanismHandlerTestCase, the three certificate mapper test cases,RejectUnauthReqTests,LDAPAuthenticationHandlerTestCase,LDAPConnectionTestCase,DsconfigOptionsTestCase.Related
#1095 / PR #1101 (the server loads a changed key store file without a restart): with #1101 a key store renewed together with its PIN file loads, but one that cannot be loaded at all still reaches the branches changed here. #1087 / PR #1100 holds back a new key store password until the next start of the Docker container partly because of this issue.
#1111: an applied change that leaves the HTTP connection handler without a usable key logs "Disabling", but the handler keeps serving until the restart. Found in the review of this PR; it predates it.