Repository navigation
Conversation
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Inbox
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
|
CaCertAwareLdapSocketFactory returns the raw resolveDelegate().createSocket(...) sockets, so they never go through HostnameAwareSslSocket. ProcessLdapProperties picks this factory for any zone with caCertificates, and LdapIntegrationConfig always sets the connect timeout, so those zones take the same no-arg createSocket() path this PR fixes. Should the six createSocket overloads wrap the delegate socket the same way? |
This is a valid point. I've added another commit to do the same wrapping on CA-cert LDAP sockets. |
There was a problem hiding this comment.
🟡 Changes recommended
A critical hostname-verification gap remains in the default LDAPS path, with additional reverse-DNS identity and SSLSocket ALPN forwarding issues plus incomplete provider-specific test coverage.
5 open issues
What changed in this PR
This pull request updates LDAPS socket creation so SNI and endpoint identification are configured correctly when JNDI connects sockets later, including Bouncy Castle FIPS compatibility.
Changes:
- Added
HostnameAwareSslSocketfor deferred hostname and SNI configuration. - Wrapped sockets created by the base and CA-aware LDAP socket factories.
- Preserved disabled hostname verification through
SkipSslLdapSocketFactory. - Added tests covering socket wrapping, hostname handling, and handshake behavior.
- Review findings remain in the base and CA-aware factories and socket wrapper, including hostname preservation, trust-manager endpoint verification, ALPN delegation, and SNI clearing.
This pull request requires changes.
| File | Description |
|---|---|
server/src/test/java/org/cloudfoundry/identity/uaa/security/HostnameAwareSslSocketTest.java |
Tests hostname application, SNI handling, connection behavior, and delegation. |
server/src/test/java/org/cloudfoundry/identity/uaa/security/CaCertAwareLdapSocketFactoryTest.java |
Tests CA-aware socket wrapping, hostname mismatch behavior, and no-argument connection handshakes. |
server/src/test/java/org/cloudfoundry/identity/uaa/security/BaseLdapSocketFactoryTest.java |
Tests wrapping across LDAP socket factory overloads and endpoint algorithm configuration. |
server/src/main/java/org/cloudfoundry/identity/uaa/security/SkipSslLdapSocketFactory.java |
Disables endpoint identification when SSL verification is explicitly skipped. |
server/src/main/java/org/cloudfoundry/identity/uaa/security/HostnameAwareSslSocket.java |
Provides the delegating SSL socket wrapper for endpoint identification and SNI configuration. |
server/src/main/java/org/cloudfoundry/identity/uaa/security/CaCertAwareLdapSocketFactory.java |
Applies hostname-aware wrapping to dynamically trusted, zone-specific sockets. |
server/src/main/java/org/cloudfoundry/identity/uaa/security/BaseLdapSocketFactory.java |
Wraps created LDAP sockets and provides the default LDAPS endpoint identification algorithm. |
Review effort: Lite
| * "No hostname specified for HTTPS endpoint ID check". | ||
| */ | ||
| private Socket wrap(Socket socket, String knownHost) { | ||
| return new HostnameAwareSslSocket((SSLSocket) socket, getEndpointIdentificationAlgorithm(), knownHost); |
| @Override | ||
| public Socket createSocket(InetAddress inetAddress, int i) throws IOException { | ||
| return delegate.createSocket(inetAddress, i); | ||
| return wrap(delegate.createSocket(inetAddress, i), inetAddress.getHostName()); |
| @Override | ||
| public Socket createSocket(InetAddress address, int port) throws IOException { | ||
| return resolveDelegate().createSocket(address, port); | ||
| return wrap(resolveDelegate().createSocket(address, port), address.getHostName()); |
| // --- SSLSocket-specific methods: delegate --- | ||
|
|
||
| @Override | ||
| public String[] getSupportedCipherSuites() { | ||
| return delegate.getSupportedCipherSuites(); | ||
| } |
| @Test | ||
| void createSocket_noArg_returnsHostnameAwareSocketThatHandshakesAfterConnect() throws Exception { | ||
| TestCert cert = newTestCert(); | ||
| try (ServerHandle server = startTlsServer(cert.keyPair(), cert.certificate())) { | ||
| IdentityZoneHolder.set(zone("zone-noarg")); | ||
| CaCertAwareLdapSocketFactory.registerZoneTrust("zone-noarg", List.of(pemEncode(cert.certificate())), false); |
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Problem: with com.sun.jndi.ldap.connect.timeout set, JNDI calls the no-arg
createSocket() and only supplies the hostname later through `connect(...)`.
Bouncy Castle FIPS (bctls-fips) doesn't pick the hostname up from that later
call. The TLS handshake then fails with "No hostname specified for HTTPS endpoint
ID check".
Changes:
- HostnameAwareSslSocket (new): a delegating SSLSocket wrapper.
- It sets the endpoint identification algorithm and the SNI server name on the
socket, either at construction or at connect() time, whichever comes first.
- It skips SNI for IP literals, empty hosts and invalid hostnames.
- BaseLdapSocketFactory:
- Every createSocket overload now returns the wrapped socket.
- The new overridable getEndpointIdentificationAlgorithm() defaults to "LDAPS".
- SkipSslLdapSocketFactory: overrides that method to return null, so hostname
verification stays off when disableSslVerification is set.
CaCertAwareLdapSocketFactory returned raw delegate sockets, so zones with caCertificates bypassed the hostname/SNI fix. Since LdapIntegrationConfig always sets a connect timeout, JNDI uses the no-arg createSocket() path and supplies the host only at connect time, which BC-FIPS does not infer. Wrap all six createSocket overloads, skipping sockets from the default-trust fallback that BaseLdapSocketFactory already wraps.
- X509ExpiryCheckingTrustManager now extends X509ExtendedTrustManager and forwards the Socket/SSLEngine checks, so the JDK trust manager performs LDAPS hostname verification instead of skipping it - HostnameAwareSslSocket calls BCSSLSocket.setHost() so BCJSSE gets a peer host after createSocket() + connect() (BCJSSE ignores SNI for this check and doesn't take the host from connect()) - Use the hostname an InetAddress was created with (or its IP literal) instead of InetAddress.getHostName(), avoiding reverse DNS lookups - Forward the ALPN methods in HostnameAwareSslSocket - Add LdapSocketFactoryHandshakeTest: real handshakes on SunJSSE and BCJSSE (hostname match, mismatch, skip-SSL) - Extract shared TlsTestServer test helper; test certs now include a SAN (BCJSSE doesn't fall back to the CN by default) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Inline the TLS test server into LdapSocketFactoryHandshakeTest and drop TlsTestServer - Remove unit tests that duplicate the real-handshake coverage or only check pass-through methods - Revert the CaCertAwareLdapSocketFactoryTest refactor - Shorten Javadoc; drop the copied licence header from HostnameAwareSslSocket Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> If the maintainers squash on merge, the number of commits doesn't matter anyway.
9696752 to
fda03d2
Compare
| if (tm instanceof X509ExtendedTrustManager extended) { | ||
| extended.checkServerTrusted(x509Certificates, s, socket); | ||
| } else { | ||
| tm.checkServerTrusted(x509Certificates, s); |
There was a problem hiding this comment.
Please preserve hostname verification when the delegate is not an X509ExtendedTrustManager. The fallback checks the certificate chain but not the hostname, so a trusted certificate for a different host is accepted. Please add a handshake test for this case.
-
Before: X509ExpiryCheckingTrustManager implemented the older X509TrustManager interface. SunJSSE wrapped it and performed an additional hostname check after the delegate returned.
-
After: it extends X509ExtendedTrustManager. SunJSSE expects it to handle checks that need the socket context. If its delegate is an older X509TrustManager, the fallback calls the same two-argument method, but the outer manager does not perform the additional hostname check.
There was a problem hiding this comment.
The JDK's own HostnameChecker isn't a public API so the hostname check had to be duplicated here. Let me know if you think of another option or if it isn't worth it.
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
…legates X509ExpiryCheckingTrustManager now extends X509ExtendedTrustManager, so SunJSSE no longer wraps it and performs the hostname check it previously did for plain X509TrustManagers. When the delegate is not an X509ExtendedTrustManager, the socket/engine checkServerTrusted fallbacks validated only the chain, accepting a trusted cert for a different host. Verify the peer hostname in the fallback when endpoint identification (HTTPS/LDAPS) is requested: match DNS/IP SANs (with left-most-label wildcards), falling back to CN only when no SAN of the relevant type is present. Reject unsupported algorithms or an unknown peer host. Add TLS handshake tests covering mismatched host (rejected), matching IP SAN (accepted), and no endpoint identification (unchanged). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7e2d9cb to
0fb1bee
Compare
Problem: with com.sun.jndi.ldap.connect.timeout set, JNDI calls the no-arg createSocket() and only supplies the hostname later through
connect(...). Bouncy Castle FIPS (bctls-fips) doesn't pick the hostname up from that later call. The TLS handshake then fails with "No hostname specified for HTTPS endpoint ID check".Changes: