[#1109] Keep a connection handler listening when a change to it is rejected

[vharseko]

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 createSSLContext as 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, because containsAtLeastOneKey() and containsKeyWithAlias() return false on any load failure. getKeyManagers() then throws, so the change is rejected and applyConfigurationChange never runs to set enabled back. The handler thread stops the listener, and the configuration still says enabled: 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:

• the LDAPS connection handler (LDAPConnectionHandler2);
• the administration connector, which is an LDAPConnectionHandler2 checked through AdministrationConnector.isConfigurationChangeAcceptable. Once port 4444 is down, dsconfig cannot reach the server any more; only ldapmodify on cn=Administration Connector,cn=config over plain LDAP, or a restart, brings it back;
• the HTTPS connection handler (HTTPConnectionHandler), whose createSSLContext sets enabled = false directly;
• the legacy LDAPConnectionHandler, which has the same code as LDAPConnectionHandler2.

All four were reproduced in Docker on openidentityplatform/opendj:latest (5.1.2); the steps are in #1109.

Change

createSSLContext in LDAPConnectionHandler2, LDAPConnectionHandler and HTTPConnectionHandler (and HTTPConnectionHandler.createSSLEngineConfigurator) takes a forUse flag:

• configureSSL, called at the start of the handler and by applyConfigurationChange, passes true: an SSL handler without a usable key is still disabled there, as before;
• isConfigurationAcceptable, reached from isConfigurationChangeAcceptable, ConnectionHandlerConfigManager and AdministrationConnector, passes false: 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 to enabled are skipped. In HTTPConnectionHandler the three identical disable blocks are folded into disableAndWarn(forUse).

Tests

New RejectedSSLConfigurationChangeTestCase, 9 cases, with its own file-based key manager provider on a temporary copy of the test key store:

• rejectedChangeKeepsTheHandlerListening, for LDAPConnectionHandler2, the legacy LDAPConnectionHandler and HTTPConnectionHandler, each with and without ssl-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 with a reason, then check the handler keeps serving TLS for 3 seconds (its thread re-reads enabled every second);
• handlerWithoutItsCertificateDoesNotListen, for the same three handlers: a handler whose key store lacks the configured ssl-cert-nickname is still disabled at its start. This pins the true passed by configureSSL.

tearDown drops the referential integrity reference that addLDAPChangeListener / addHTTPChangeListener registers from the handler to the key manager provider (it outlives the handler), so the provider entry can be deleted.

Results:

• on master, the 6 rejectedChangeKeepsTheHandlerListening cases fail (Connection refused for LDAP2 and HTTP, Read timed out for the legacy handler, which accepts one more connection before it closes); with the change, 9/9 pass;
• 9 mutants, each reverting one forUse argument (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;
• related classes pass: 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.

[maximthomas]

praise: The check of a proposed configuration no longer touches the running handler, and the new test proves it both ways.

• isConfigurationAcceptable now calls createSSLContext(config, false) (LDAPConnectionHandler2.java:582, and the same in LDAPConnectionHandler and HTTPConnectionHandler), while configureSSL keeps passing true for the start and the apply.
• RejectedSSLConfigurationChangeTestCase against the base's three handler files: 6 of 9 fail, all six rejectedChangeKeepsTheHandlerListening cases, each on "the handler does not serve TLS". The three handlerWithoutItsCertificateDoesNotListen cases pass. At the head it is 9/9 green in CI (build-maven (ubuntu-latest, 11)).
• The three identical disable blocks in HTTPConnectionHandler.createSSLContext are folded into disableAndWarn(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 is

Or: 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.