Skip to content

Verify the connector server certificate against the host over the legacy SSL connection - #122

Merged
vharseko merged 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:unsafe-cert-trust-hostname-verification
Sep 30, 2026
Merged

vharseko merged 4 commits into
OpenIdentityPlatform:masterfrom
vharseko:unsafe-cert-trust-hostname-verification

Conversation

@vharseko

@vharseko vharseko commented Sep 18, 2026 •

Copy link
Copy Markdown
Member

Resolves CodeQL alert https://github.com/OpenIdentityPlatform/OpenICF/security/code-scanning/24 (java/unsafe-cert-trust, critical).

Problem

RemoteFrameworkConnection (the legacy binary-protocol client for the connector server, port 8759) wraps the socket in an SSLSocket when useSSL is on. SSLSocket validates the certificate chain but never checks the certificate against the host it connects to, so anyone holding a certificate the client trusts could sit in the middle of the connection. The connector server key is written to that connection as a GuardedString encrypted with the framework's fixed default key, i.e. readable by such an attacker, and all connector operation data flows through it as well.

With the typical setup (no custom trustManagers, the server's self-signed certificate imported into the JVM truststore next to the public CAs) any publicly trusted certificate was enough.

Change

  • RemoteFrameworkConnection: enable JSSE "HTTPS" endpoint identification (SSLParameters.setEndpointIdentificationAlgorithm) before the handshake. The certificate must now name the configured host: an IP address as a subjectAltName iPAddress entry, a host name as a subjectAltName dNSName entry, or as the subject CN when the certificate has no dNSName entry (RFC 2818 rules, as HttpsURLConnection does). The check happens inside the handshake, before the key is sent. It covers the default trust store, trust managers from a TrustManagerFactory, and plain X509TrustManager implementations, which JSSE wraps. A custom X509ExtendedTrustManager is not wrapped: it is responsible for the identity check itself, as JSSE's own implementation does.
  • Opt-out for deployments that need time to fix their certificates: -Dorg.identityconnectors.framework.remote.hostnameVerification=false. Only the literal false disables the check, so a typo cannot weaken it. With the check off the client still compares the certificate names with the host and logs a WARN once per server naming the certificate's subject and subjectAltName entries, so the certificate can be fixed and the check re-enabled.
  • CertificateHostnameMatcher (package-private): the matcher behind that diagnostic, approximating JSSE — iPAddress entries for IP literals, dNSName entries with JSSE's wildcard rules for private CAs (a * within the leftmost label only, e.g. *.example.com or w*.example.com), IDN-normalized, CN only when no dNSName is present. As in JSSE, one trailing dot of the host is ignored, and a host that is not a valid DNS name (e.g. my_host.example) matches nothing; for such a host the WARN says to configure the server by a valid name or by IP, since no certificate can fix it. JSSE's public-suffix check for public CAs is not replicated, so the WARN says the certificate "may not match".
  • RemoteFrameworkConnectionInfo javadoc for useSSL documents the requirement and the property.
  • openicf-maven-plugin: the converter's trust-all trust manager is now an X509ExtendedTrustManager (every method marked @Override), so JSSE does not wrap it: neither the hostname check nor the jdk.certpath.disabledAlgorithms check on the server's chain applies. With every certificate trusted neither check gives security, and plugin goals with useSSL=true keep working against connector servers whose certificate does not name the configured host.

Tests

  • RemoteFrameworkConnectionSSLTests: a CN-only certificate is rejected when connecting by IP (with a TrustManagerFactory trust manager and with a plain X509TrustManager), a SAN certificate is accepted, CN fallback works when there is no SAN, the property disables the check and logs exactly one WARN per server, only for a non-matching certificate (captured through a test LogSpi, CapturingLogSpi, which the module's surefire configuration selects), a host with a trailing dot connects and is not reported, off and FALSE do not disable it, and the check applies with the default trust store (javax.net.ssl.trustStore).
  • CertificateHostnameMatcherTests: SAN/CN/wildcard/IP matching rules, including private-CA wildcards, the most specific of several CNs, a certificate without names, out-of-range IPv4 octets, the trailing dot, and host names that are not valid DNS names (invalid-host-san.pem names them exactly and still does not match). RemoteFrameworkConnectionSSLTests.describesWhyVerificationWouldFail pins the WARN text, including the one for such a host.
  • RemoteFrameworkConnectionInfoConverterTests (plugin): the plugin's trust manager connects by IP to a CN-only certificate with verification on.
  • RemoteConnectorInfoManagerSSLTests now uses KeyStore-san.jks (SAN localhost, 127.0.0.1, ::1); with the old CN-only KeyStore.jks it fails as expected, since it connects to 127.0.0.1. KeyStore.jks stays for the negative tests.

Compatibility

Deployments whose connector server certificate does not name the host used in the client configuration (self-signed certificates with an unrelated CN, connections by IP without an iPAddress SAN) will fail the handshake with SSLHandshakeException: No subject alternative names present / No name matching <host> found. The fix is a certificate that names the host; the system property above is the temporary escape hatch. A host configured by a name that is not a valid DNS name (an underscore as in connector_server, an empty label, a leading or trailing hyphen) fails with Illegal given domain name whatever the certificate says: JSSE rejects the name itself, so the fix there is to configure the server by a valid name or by its IP address. A trailing dot (host.example.com.) is fine. Worth a release-notes entry. openicf-maven-plugin is not affected (see above).

Not covered here: the Grizzly WebSocket client (connector-framework-server ConnectionManager) has the same class of problem outside the CodeQL model; separate PR.

…acy SSL connection

RemoteFrameworkConnection wrapped the socket in an SSLSocket, which does not
check the server certificate against the host it connects to, so anyone with
a certificate the client trusts could sit in the middle and read the
connector server key. Enable JSSE "HTTPS" endpoint identification for the
handshake (CodeQL java/unsafe-cert-trust, alert OpenIdentityPlatform#24).

The check can be switched off with
-Dorg.identityconnectors.framework.remote.hostnameVerification=false; the
client then still compares the certificate names with the host and logs a
warning naming the certificate's subject and subjectAltName entries, so the
certificate can be fixed and the check re-enabled.

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The check sits exactly where the hole was, and the tests cover the tricky trust-manager case.

  • RemoteFrameworkConnection.init sets setEndpointIdentificationAlgorithm("HTTPS") before startHandshake() (RemoteFrameworkConnection.java:127-133), so the key is never written to a server whose certificate does not name the host. The same lines apply to both the default factory and custom trust managers.
  • verifiesHostnameEvenWithPlainX509TrustManager pins the fact that JSSE wraps a legacy X509TrustManager and still checks the name. CI is green on JDK 11/17/21/25/26 on Linux, macOS and Windows.

question (non-blocking): Should the openicf-maven-plugin's useSSL path keep working against connector servers whose certificate does not name the configured host?

OpenICF-maven-plugin/src/main/java/org/forgerock/openicf/maven/RemoteFrameworkConnectionInfoConverter.java:128-129, :136-152

The converter passes getTrustManager(), an anonymous X509TrustManager that accepts every certificate. JSSE wraps it and now runs the HTTPS identity check. Plugin goals with useSSL=true that worked against a self-signed certificate with another CN, or when connecting by IP, now fail with No subject alternative names present (reproduced on JDK 26). For a trust-all caller the check adds no security, because an attacker can mint a certificate with the right SAN. The only opt-out is the JVM-global property via MAVEN_OPTS, and the plugin does not mention it. If the answer is no, a line in the release-notes entry you already plan is enough: "openicf-maven-plugin with useSSL=true: the connector server certificate must name the configured host; until it does, run Maven with MAVEN_OPTS=-Dorg.identityconnectors.framework.remote.hostnameVerification=false." If the answer is yes, this is a Major: the plugin needs its own switch in this PR.


issue (non-blocking): The diagnostic WARN tells admins to replace certificates that JSSE would accept.

OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/remote/CertificateHostnameMatcher.java:123-136, :33-37

The class javadoc claims to match names "the way ... JSSE's "HTTPS" endpoint identification do". But matchesDnsName rejects every wildcard over a single-label suffix (:130-133), treats partial leftmost wildcards as literals (:126-127), and compares IDN hosts as raw strings. With verification off, host icf.local against SAN dns:*.local, or www.example.com against dns:w*.example.com from a private CA, gives matcher false and JSSE MATCH (measured against HostnameChecker TYPE_TLS, JDK 26). reportCertificateMismatch then logs "fix the server certificate and re-enable verification" for a certificate that would pass once verification is re-enabled. Enforcement is not affected.

 * Approximates RFC 6125 / RFC 2818 name matching; JSSE's "HTTPS" endpoint
 * identification accepts more wildcard forms (e.g. {@code *.local} or
 * {@code w*.example.com} from a private CA), so a mismatch reported here is advisory.

Or: relax :130-133 and accept partial leftmost wildcards. Either way, in RemoteFrameworkConnection.reportCertificateMismatch the WARN should say "presented a certificate that may not match the host: ".


issue (non-blocking): FALSE and False also disable hostname verification, though the docs say only the literal false does.

OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/remote/RemoteFrameworkConnection.java:155, :60-61, :151

isHostnameVerificationEnabled uses equalsIgnoreCase. The field javadoc ("exactly {@code false}"), the method javadoc ("Only the literal {@code false}"), the PR description and the test name keepsVerificationUnlessPropertyIsExactlyFalse all promise an exact match. Setting the property to FALSE turns the check off (checked at runtime).

return !"false".equals(System.getProperty(HOSTNAME_VERIFICATION_PROPERTY));

Or: keep equalsIgnoreCase and remove "exactly"/"literal" from both javadocs and the test name.


issue (non-blocking): The public useSSL javadoc gets the CN rule wrong.

OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/framework/api/RemoteFrameworkConnectionInfo.java:73-78, :111-116

Both constructors say the host may appear "as CN when it has no subjectAltName". JSSE never uses the CN for an IP host: CN=10.0.0.5 with no SAN, connecting to 10.0.0.5, fails with No subject alternative names present. For a DNS host it does use the CN when the SAN has only iPAddress entries. An administrator who follows this doc and connects by IP to a CN-only certificate gets a handshake failure. The package-private CertificateHostnameMatcher javadoc already states the rule correctly.

     *            Set to true if we are to connect via SSL. The server
     *            certificate is then verified against {@code host}: an IP
     *            address must be a subjectAltName iPAddress entry, a host name
     *            a subjectAltName dNSName entry (or the subject CN when the
     *            certificate has no dNSName entry). Setting the system property
     *            {@code org.identityconnectors.framework.remote.hostnameVerification}
     *            to {@code false} disables this check (not recommended).

suggestion (non-blocking): No test checks the WARN emitted when verification is off.

OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/remote/RemoteFrameworkConnectionSSLTests.java:104-111

connectsWhenHostnameVerificationIsDisabled only connects and closes. No test captures the log or references REPORTED_MISMATCHES. If the reportCertificateMismatch call is deleted, all 6 tests stay green (measured). The same holds if the matches() early return is inverted or the once-per-server dedup is dropped.

    // with REPORTED_MISMATCHES made package-private; import static org.testng.Assert.assertFalse
    @Test
    public void reportsMismatchOnlyForNonMatchingCertificateWhenVerificationIsDisabled() throws Exception {
        KeyStore cnOnly = loadKeyStore("KeyStore.jks");
        try (TlsServer server = new TlsServer(cnOnly, InetAddress.getByName("127.0.0.1"))) {
            withHostnameVerificationProperty("false", () ->
                    connect("127.0.0.1", server.getPort(), trustManagers(cnOnly)).close());
            assertTrue(RemoteFrameworkConnection.REPORTED_MISMATCHES.contains("127.0.0.1:" + server.getPort()));
        }
        KeyStore san = loadKeyStore("KeyStore-san.jks");
        try (TlsServer server = new TlsServer(san, InetAddress.getByName("127.0.0.1"))) {
            withHostnameVerificationProperty("false", () ->
                    connect("127.0.0.1", server.getPort(), trustManagers(san)).close());
            assertFalse(RemoteFrameworkConnection.REPORTED_MISMATCHES.contains("127.0.0.1:" + server.getPort()));
        }
    }

Pin: kills the call-deletion mutant and the inverted-matches() mutant.


suggestion (non-blocking): keepsVerificationUnlessPropertyIsExactlyFalse cannot tell equals from equalsIgnoreCase.

OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/remote/RemoteFrameworkConnectionSSLTests.java:113-126

The test uses only "off", and connectsWhenHostnameVerificationIsDisabled uses only "false". Both behave the same under either comparison. So whichever contract you settle on for FALSE, a mutant going the other way survives.

            for (String value : new String[] { "off", "FALSE" }) {
                withHostnameVerificationProperty(value, () -> {
                    try {
                        connect("127.0.0.1", server.getPort(), trustManagers(store)).close();
                        fail("'" + value + "' must not disable hostname verification");
                    } catch (ConnectorException e) {
                        assertHandshakeFailure(e);
                    }
                });
            }

Pin: use this as the try-with-resources body. It turns red at the head until :155 uses equals. If case-insensitivity stays, run connectsWhenHostnameVerificationIsDisabled with "FALSE" as well.


suggestion (non-blocking): The default-truststore path, which the PR names as the main exposure, is never exercised.

OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/remote/RemoteFrameworkConnectionSSLTests.java:161-165, OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/remote/RemoteFrameworkConnection.java:114-115

Every test passes explicit trust managers, so the SSLSocketFactory.getDefault() branch never runs with useSSL on. The check at :127-132 covers both branches today. But a refactor that moved it into the SSLContext.init branch (:116-120) would keep every test green.

    @Test
    public void verifiesHostnameWithDefaultTrustStore() throws Exception {
        KeyStore store = loadKeyStore("KeyStore.jks");
        SSLContext previous = SSLContext.getDefault();
        SSLContext trusting = SSLContext.getInstance("TLS");
        trusting.init(null, trustManagers(store).toArray(new TrustManager[0]), null);
        SSLContext.setDefault(trusting);
        try (TlsServer server = new TlsServer(store, InetAddress.getByName("127.0.0.1"))) {
            connect("127.0.0.1", server.getPort(), null).close();
            fail("Expected the handshake to fail: certificate has no name matching 127.0.0.1");
        } catch (ConnectorException e) {
            assertHandshakeFailure(e);
        } finally {
            SSLContext.setDefault(previous);
        }
    }

Pin: SSLSocketFactory.getDefault() reads SSLContext.getDefault() on every call unless ssl.SocketFactory.provider is set, so no forked JVM is needed.


suggestion (non-blocking): No test exercises the matcher's guard branches: the TLD-wildcard guard, the null CN, and picking the most specific CN.

OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/remote/CertificateHostnameMatcherTests.java:79-87, CertificateHostnameMatcher.java:68, :130-133, :164

The only wildcard fixture is *.example.com, and every fixture without a SAN has a single string CN. So deleting the *.com guard, dropping commonName != null, or iterating the RDNs forward all leave the tests green.

    // openssl req -x509 -newkey rsa:2048 -nodes -keyout /dev/null -days 3650 \
    //     -subj "/CN=wrong.example/OU=x/CN=right.example" -out multi-cn.pem
    @Test
    public void usesMostSpecificCommonName() throws Exception {
        X509Certificate cert = pemCertificate("multi-cn.pem");
        assertTrue(CertificateHostnameMatcher.matches("right.example", cert));
        assertFalse(CertificateHostnameMatcher.matches("wrong.example", cert));
    }

    // openssl req -x509 -newkey rsa:2048 -nodes -keyout /dev/null -days 3650 \
    //     -subj "/O=OpenICF test" -addext "subjectAltName=DNS:*.com" -out tld-wildcard.pem
    @Test
    public void rejectsWildcardOverTopLevelDomain() throws Exception {
        X509Certificate cert = pemCertificate("tld-wildcard.pem");
        assertFalse(CertificateHostnameMatcher.matches("example.com", cert));
    }

Pin: if you relax the TLD guard instead (see the matcher issue above), pin the relaxed rule with the same fixture.


nitpick (non-blocking): IPV4_LITERAL accepts octets above 255, so a non-literal can reach InetAddress.getByName.

OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/remote/CertificateHostnameMatcher.java:47, :112-113

For 300.1.1.1, sameAddress performs a real DNS lookup (observed), although the comment says "no name resolution happens here". In the other direction, 127.1, which JSSE treats as an IPv4 literal, takes the DNS path, and the WARN fires for a matching ip:127.0.0.1 SAN.

    private static final Pattern IPV4_LITERAL =
            Pattern.compile("((25[0-5]|2[0-4]\\d|1\\d\\d|[1-9]?\\d)\\.){3}(25[0-5]|2[0-4]\\d|1\\d\\d|[1-9]?\\d)");

vharseko added a commit to vharseko/OpenICF that referenced this pull request Sep 29, 2026
- ConnectionManagerConfig follows org.identityconnectors.framework.remote.hostnameVerification
  on every connection unless set explicitly, like the legacy client in OpenIdentityPlatform#122, so a later
  change of the property reaches the WebSocket client too. Javadoc: `false` in any letter case.
- SwitchingSSLFilter: reads bypass Grizzly's SSL transport wrapper while the connection is
  plain. The wrapper gave the connection a server-mode SSLEngine on its first read, which
  through a proxy is the answer to CONNECT, so every wss connection through a proxy hung.
- ConnectionManagerConfigTest pins the property default, its letter case and live changes.
- ClientHostnameVerificationTest connects through a CONNECT proxy to wss://localhost against
  the CN-only certificate: accepted for the URI host, rejected if the engine were keyed on
  the proxy's 127.0.0.1.
…ith JSSE

- openicf-maven-plugin: the trust-all trust manager of
  RemoteFrameworkConnectionInfoConverter is now an X509ExtendedTrustManager.
  JSSE does not wrap it, so it does not add the "HTTPS" hostname check, which
  gives no security when every certificate is trusted and would only reject
  connector servers whose certificate does not name the configured host.
- CertificateHostnameMatcher follows JSSE's wildcard rules for private CAs
  (partial leftmost wildcards such as w*.example.com, wildcards over a
  single-label suffix such as *.local), compares IDN-normalized names and
  accepts only valid IPv4 literals, so 300.1.1.1 is no longer resolved. The
  WARN now says the certificate "may not match" the host.
- Only the exact value "false" disables hostname verification, as documented.
- useSSL javadoc: the subject CN is never used for an IP address.
- Tests: the mismatch WARN, "FALSE" keeping verification on, the default trust
  store (javax.net.ssl.trustStore) path, multiple CNs, a certificate without
  names, private-CA wildcards, and the plugin connecting by IP to a CN-only
  certificate.
@vharseko

Copy link
Copy Markdown
Member Author

All nine points are addressed in 181e698. I checked the JSSE claims against sun.security.util.HostnameChecker (TYPE_TLS, JDK 26), and every one of them holds.

question: openicf-maven-plugin useSSL. Yes, it should keep working, and it now does without any property. RemoteFrameworkConnectionInfoConverter.getTrustManager() now returns an X509ExtendedTrustManager. SSLContextImpl.chooseTrustManager wraps only a plain X509TrustManager, so JSSE no longer adds the "HTTPS" identity check to the plugin's trust-all manager. As you say, that check adds no security there. RemoteFrameworkConnectionInfoConverterTests connects by 127.0.0.1 to the CN-only KeyStore.jks with verification on. The test goes red with the old plain X509TrustManager.

issue: the matcher is stricter than JSSE. I relaxed the matcher to JSSE's rules for private-CA certificates rather than only documenting the gap:

  • partial leftmost wildcards (w*.example.com, a*b.example.com);
  • wildcards over a single-label suffix (*.local, *.com);
  • *, *. and *com rejected;
  • a * outside the leftmost label compared literally;
  • IDN normalization as in HostnameChecker.isMatched.

I checked 28 name/host pairs against HostnameChecker. The only difference left is 127.1, see the last point. The public-suffix check JSSE adds for public CAs is not replicated, and the class javadoc now says so and calls a reported mismatch advisory. The WARN says "may not match".

issue: FALSE disables verification. isHostnameVerificationEnabled now uses equals, as the javadocs, the description and the test name say.

issue: useSSL javadoc CN rule. I took your wording, in both constructors.

suggestion: the WARN is not tested. Added reportsMismatchOnlyForNonMatchingCertificateWhenVerificationIsDisabled, and REPORTED_MISMATCHES is now package-private.

suggestion: equals vs equalsIgnoreCase. keepsVerificationUnlessPropertyIsExactlyFalse now loops over "off" and "FALSE".

suggestion: default trust store path. The test is added, but in a different form. The SSLSocketFactory.getDefault() branch cannot be reached. RemoteFrameworkConnectionInfo stores the list through CollectionUtil.newReadOnlyList, which turns null into an empty list, so getTrustManagers() is never null. With no trust managers the connection goes through SSLContext.init(null, null, null), which uses the default TrustManagerFactory over javax.net.ssl.trustStore. The SSLContext.setDefault variant therefore fails with PKIX instead of the hostname error. verifiesHostnameWithDefaultTrustStore points javax.net.ssl.trustStore at KeyStore.jks and expects the hostname failure. That is the typical deployment the description names. The dead branch predates this PR, and I left it alone.

suggestion: matcher guard branches. Added three fixtures and tests: multi-cn.pem (the most specific CN wins), no-names.pem (no CN, no SAN) and private-wildcard-san.pem. The last one pins the relaxed rule: *.com matches example.com but not com or www.example.com, and w*.example.org matches www/w but not x.

nitpick: IPV4_LITERAL. I switched to your pattern, and 300.1.1.1 is now matched as a DNS name without a lookup; a new test pins that. 127.1 still takes the DNS-name path, so for that spelling the WARN can fire against a matching ip:127.0.0.1. Enforcement is unaffected, and I left it as is.

Each new test was checked against the reverted fix, and each of them fails there: the old matcher, equalsIgnoreCase, the removed reportCertificateMismatch call, and the plain plugin trust manager.

@vharseko vharseko added the maven-plugin OpenICF-maven-plugin label Sep 29, 2026
@vharseko

Copy link
Copy Markdown
Member Author

d35f8e4 fixes the four CodeQL java/missing-override-annotation notes on RemoteFrameworkConnectionInfoConverter.java:160-172. Every method of the plugin's trust-all X509ExtendedTrustManager now has @Override. That includes the three methods that were there before, so the class stays consistent. Behavior is unchanged, and RemoteFrameworkConnectionInfoConverterTests passes locally.

@maximthomas, could you take another look?

@vharseko
vharseko requested review from maximthomas and removed request for maximthomas September 29, 2026 17:29

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The check sits where the bug is, and the round-1 points are closed with pins.

  • RemoteFrameworkConnection.init sets setEndpointIdentificationAlgorithm("HTTPS") (:130) before startHandshake() (:133), so a certificate that does not name the host fails the handshake before the key is written.
  • isHostnameVerificationEnabled() (:155) is !"false".equals(...), and keepsVerificationUnlessPropertyIsExactlyFalse now loops over "off" and "FALSE", which turns red under equalsIgnoreCase.

issue (non-blocking): CertificateHostnameMatcher reports a match for host names that JSSE rejects before it looks at the certificate.

OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/remote/CertificateHostnameMatcher.java:61-83, :129, :164-171

HostnameChecker.matchDNS first runs new SNIHostName(host) and fails with Illegal given domain name: <host> when the host has an underscore, an empty label, a leading or trailing hyphen, or a trailing dot. normalize() uses IDN.toASCII without STD3 rules and then compares with equals, so for such a host the matcher returns true. Checked on JDK 11.0.25 and JDK 26: my_host.example against SAN or CN my_host.example, www.example.com. against *.example.com., a..example.com and -bad-.example.com give jsse=false, matcher=true. A server configured as my_host.example fails with verification on. With hostnameVerification=false no WARN is logged, and no certificate can make re-enabling work. The Compatibility section should list this case too ("the fix is a certificate that names the host" does not hold for it).

// CertificateHostnameMatcher
static boolean isVerifiableHost(String host) {
    if (isIpLiteral(host)) {
        return true;
    }
    try {
        new SNIHostName(host); // the check HostnameChecker.matchDNS runs first
        return true;
    } catch (IllegalArgumentException e) {
        return false;
    }
}

// RemoteFrameworkConnection.reportCertificateMismatch, before the certificate is inspected
if (!CertificateHostnameMatcher.isVerifiableHost(connectionInfo.getHost())) {
    problem = "is configured by a name that is not a valid DNS host name,"
            + " so hostname verification cannot succeed for it; connect by a valid name or by IP";
}

suggestion (non-blocking): The plugin's unwrapped X509ExtendedTrustManager also skips JSSE's algorithm-constraints check, and neither the javadoc nor the description says so.

OpenICF-maven-plugin/src/main/java/org/forgerock/openicf/maven/RemoteFrameworkConnectionInfoConverter.java:140-147

A plain X509TrustManager is wrapped in AbstractTrustManagerWrapper, which adds endpoint identification and the jdk.certpath.disabledAlgorithms check on the peer chain. The extended manager is not wrapped, so both checks are gone. Measured with an MD5withRSA certificate on a JDK 11 server: the base-style manager fails with "Certificates do not conform to algorithm constraints" on JDK 11 and JDK 26 clients, and the head's manager connects. This loses no security, since the manager trusts everything anyway. It does contradict "openicf-maven-plugin is not affected".

 * It is an {@link X509ExtendedTrustManager} so that JSSE does not wrap it:
 * neither the "HTTPS" hostname check nor the jdk.certpath.disabledAlgorithms
 * check applies, which adds no risk with every certificate trusted.

suggestion (non-blocking): reportsMismatchOnlyForNonMatchingCertificateWhenVerificationIsDisabled pins the dedup set, not the WARN.

OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/remote/RemoteFrameworkConnectionSSLTests.java:133-148, RemoteFrameworkConnection.java:163-190

REPORTED_MISMATCHES.add(server) runs in the if condition whatever the body does. A mutant that deletes the LOG.warn at :185 therefore keeps both asserts green. So does one that drops the contains() early return and the add() guard and warns on every connection, because the test connects to each server only once. The description says the test shows the mismatch is logged once, but it only shows that the set is filled once.

// test-scope LogSpi, selected by surefire systemPropertyVariables:
// org.identityconnectors.common.logging.class = ...remote.CapturingLogSpi
public final class CapturingLogSpi implements LogSpi {
    static final List<String> WARNINGS = new CopyOnWriteArrayList<>();
    @Override public void log(Class<?> c, String m, Log.Level l, String msg, Throwable e) {
        if (l == Log.Level.WARN) { WARNINGS.add(msg); }
    }
    @Override public void log(Class<?> c, StackTraceElement s, Log.Level l, String msg, Throwable e) {
        log(c, (String) null, l, msg, e);
    }
    @Override public boolean isLoggable(Class<?> c, Log.Level l) { return true; }
    @Override public boolean needToInferCaller(Class<?> c, Log.Level l) { return false; }
}

Pin: connect twice to the CN-only server with the property false, then assert exactly one captured WARN containing 127.0.0.1:<port>, and none for the SAN server.


suggestion (non-blocking): The same test can go red on an ephemeral-port repeat, because REPORTED_MISMATCHES is static and never cleared.

OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/remote/RemoteFrameworkConnectionSSLTests.java:139, :146, RemoteFrameworkConnection.java:69

By :146 the set already holds the ports of the CN-only server that was just closed and of connectsWhenHostnameVerificationIsDisabled. If bind(0) gives the SAN server one of those ports again, assertFalse fails even though the code is right. The same stale entry can also let :139 pass without a report. macOS allocates ports sequentially and showed 0 repeats in 300000 binds. The Linux rate, which is what CI sees, was not measured.

RemoteFrameworkConnection.REPORTED_MISMATCHES.clear();
try (TlsServer cnOnlyServer = new TlsServer(cnOnly, loopback);
        TlsServer sanServer = new TlsServer(san, loopback)) {
    // same connects and asserts; both ports are bound at once, so they differ
}

- CertificateHostnameMatcher: a host that is not a valid DNS name
  (underscore, empty label, leading/trailing hyphen) matches nothing, as
  JSSE rejects it before looking at the certificate; one trailing dot is
  ignored, as JSSE does. The WARN tells to configure such a server by a
  valid name or by IP (RemoteFrameworkConnection.describeMismatch).
- Plugin trust manager javadoc: the disabledAlgorithms check is skipped too.
- Tests: CapturingLogSpi (selected by surefire) pins exactly one WARN per
  server; the test clears its state and binds both servers at once.
vharseko added a commit to vharseko/OpenICF that referenced this pull request Sep 30, 2026
…, pin it end to end

- ConnectionManagerConfig: only exactly `false` disables hostname
  verification, as in the legacy client of OpenIdentityPlatform#122 (`equals`, not
  `equalsIgnoreCase`); Javadoc and the `FALSE` row of
  ConnectionManagerConfigTest follow.
- ClientHostnameVerificationTest:
  - followsLaterChangeOfTheSystemProperty: a client whose filter chain
    was built with verification on connects to the CN-only certificate
    once the property is `false`; red if SwitchingSSLFilter snapshots it.
  - rejectsServerCertificateWithoutMatchingNameThroughProxy: wss://127.0.0.1
    through a CONNECT proxy to the CN-only certificate is rejected; red if
    endpoint identification is skipped behind a proxy.
  - Client.open only waits for the WebSocket: a second connection to the
    same server session joins the first one's connection group and gets
    no connector lookup of its own.
@vharseko

Copy link
Copy Markdown
Member Author

All four points are addressed in 1100178.

issue: the matcher reports a match for host names JSSE rejects. Confirmed, with one correction. HostnameChecker.matchDNS does reject my_host.example, a..example.com and -bad-.example.com through new SNIHostName(host), and a full handshake fails with Illegal given domain name: <host> whatever the certificate says (checked end to end on JDK 11.0.32 and JDK 26). The trailing dot is different. X509TrustManagerImpl.checkIdentity drops one trailing dot of the peer host before it calls HostnameChecker, and Utilities.rawToSNIHostName drops it for SNI. So a handshake to localhost. passes against dns:localhost on both JDKs, and www.example.com. is not a failure case in practice. The changes:

  • CertificateHostnameMatcher.isVerifiableHost is your check, applied after one trailing dot is removed. matches() now returns false for such a host, and it also ignores one trailing dot, as JSSE does.
  • The WARN text is built in a new package-private RemoteFrameworkConnection.describeMismatch. For such a host it says the name is not a valid DNS host name, so no certificate can pass, and that the server should be configured by a valid host name or by its IP address.
  • Tests: verifiesOnlyIpLiteralsAndValidDnsNames and neverMatchesHostThatIsNotAValidDnsName, with a new fixture invalid-host-san.pem that names my_host.example exactly. ignoresTrailingDotOfHost, and an end-to-end acceptsFullyQualifiedHostWithTrailingDot, which is skipped where localhost. does not resolve. describesWhyVerificationWouldFail pins the WARN text for the invalid host too. An unresolvable name cannot reach the WARN end to end, so that case is tested one level down.
  • The Compatibility section now lists the case.

suggestion: the plugin's manager also skips jdk.certpath.disabledAlgorithms. I took your javadoc wording and fixed the description, which no longer says the manager only avoids the hostname check.

suggestion: the test pins the set, not the WARN. I added the LogSpi you sketched as a test class, org.identityconnectors.common.logging.CapturingLogSpi. It extends the package-private StdOutLogger, so the module's test output does not change. The module's surefire systemPropertyVariables selects it, and the tests assert that it is active, so a run without that configuration fails with a clear message instead of passing vacuously. reportsMismatchOnceAndOnlyForNonMatchingCertificateWhenVerificationIsDisabled connects twice to each server and asserts exactly one WARN for the CN-only server and none for the SAN server.

suggestion: the ephemeral-port repeat. The same test now clears REPORTED_MISMATCHES and the captured log first, and binds both servers at once in one try-with-resources.

Each new test fails against its mutant: LOG.warn deleted, dedup removed (a WARN on every connection), the invalid-host check removed from matches(), the trailing-dot strip removed, and the invalid-host branch removed from describeMismatch. Locally connector-framework-internal (493 tests) and the maven plugin pass on JDK 26, and the two changed test classes also pass on JDK 11.

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The delta fixes the round-2 findings where they live and pins the WARN for real.

  • RemoteFrameworkConnection.describeMismatch checks CertificateHostnameMatcher.isVerifiableHost before looking at the certificate, so a host JSSE rejects outright gets a remedy that names the configured host instead of telling the admin to replace a certificate.
  • reportsMismatchOnceAndOnlyForNonMatchingCertificateWhenVerificationIsDisabled clears REPORTED_MISMATCHES and CapturingLogSpi, binds both servers at once and asserts the WARN text; assertCapturingLog() fails loudly if the surefire wiring is lost.

@vharseko
vharseko merged commit b70b75e into OpenIdentityPlatform:master Sep 30, 2026
10 of 14 checks passed
@vharseko
vharseko deleted the unsafe-cert-trust-hostname-verification branch September 30, 2026 12:25
vharseko added a commit that referenced this pull request Oct 4, 2026
Closes the remaining 1320 open `java/missing-override-annotation` CodeQL
alerts on `master` — the last large `note`-severity category from the
ongoing CodeQL cleanup series (#122, #123, #126, #128, #130–#134).

## Change

Every location CodeQL's `java/missing-override-annotation` flagged gets
an `@Override` line inserted immediately above the method signature, at
the same indentation, at the exact line the alert points to. 225 files,
purely additive (`+1488 / −0`):

- 1320 `@Override` insertions;
- 164 `Portions Copyrighted 2026 3A Systems, LLC` header lines: 162
appended to an existing header, and 2 in a new `/* */` block for files
that had no header at all (`AttributeMappingConfig`, `ContainsQuery`).
The other 61 files already carried a 3A line covering 2026, so their
headers are left alone.

No other code changes: no renames, no logic touched, no reordering.

## Verification

`@Override` is compiler-checked — `javac` fails hard when it is placed
on a method that does not actually override or implement anything from a
supertype ("method does not override a method from its superclass").
Rather than review 1320 insertions by hand, the real proof is a clean
build: the entire reactor was built from the root `pom.xml` (`mvn
install`, all 28 modules) after the insertions, with no manual fixes
needed afterward.

Result on the original head: **BUILD SUCCESS**, all 28 modules, **1442
tests, 0 failures, 0 errors** (164 skipped, same skips as on `master`) —
including `connector-framework-internal` (469 tests) and
`ldap-connector` with its embedded OpenDJ (159 tests). Also spot-checked
~30 random insertions by hand across different files and annotation
styles (interface method declarations, anonymous inner classes, nested
static classes) before the build, all correctly placed. After the rebase
below, the whole reactor was rebuilt (`mvn install -DskipTests`, main
and test sources) and `build.yml` runs on the new head.

## Rebase onto master

The branch was rebased onto `master` at c7648cf. Two files conflicted
with PRs merged in the meantime:

- `RemoteFrameworkConnectionInfoConverter` (#122): #122 had already
added its own 3A header line and replaced the anonymous
`X509TrustManager` with an `X509ExtendedTrustManager` whose seven
methods are annotated. Master's side is kept for both hunks; this PR now
only adds `@Override` to `enableLogging`, `canConvert` and
`fromConfiguration`.
- `RemoteRequest` (#127): #127 rewrote the anonymous classes and already
annotates all four methods, so the file is taken from master unchanged
and drops out of this PR.

In six files — the converter above plus `EncryptorImpl`,
`LocalConnectorInfoManagerImpl`, `RemoteFrameworkConnection`,
`CCLWatchThreadFactory` and `ConnectionManager` — other PRs of the
series had already added a 3A header line; that line now comes from
master, and no file carries two 3A lines.

The remaining open PRs of the series may still touch files changed here;
whichever merges second needs a rebase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework OpenICF-java-framework java Pull requests that update java code maven-plugin OpenICF-maven-plugin security Security fix / CVE remediation tests Test additions or fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants