DSP-24887: Enable SAN-aware peer identity lookup for endpoint verification on hostname by piyushk010 · Pull Request #40 · riptano/netty

piyushk010 · 2026-05-18T10:40:27Z

No description provided.

szymon-miezal

I have completed the first pass, the direction seems ok to me, I am yet to validate it end to end though.

szymon-miezal · 2026-05-20T09:07:49Z

+    private static TrustManager[] wrapIfNeeded(TrustManager[] tms, boolean sanPeerIdentityLookup,
+                                               ResumptionController resumptionController) {


I would keep the order of existing arguments unchanged - add the boolean flag as last one.

szymon-miezal · 2026-05-20T09:23:22Z

+import java.util.Collection;
+import java.util.List;
+
+final class SanPeerIdentityTrustManager extends X509ExtendedTrustManager {


Does this class need to extend X509ExtendedTrustManager?
I see that the constructor already takes an instance of X509ExtendedTrustManager, what is the point of inheritance then?
If the goal is to declare method it should rather implement an interface, perhaps X509TrustManager.

This class intentionally extends X509ExtendedTrustManager because the SAN-aware logic depends on the engine-aware checkServerTrusted(..., SSLEngine) overload. Unlike X509TrustManager, which only validates certificate chains, X509ExtendedTrustManager provides SSLEngine/Socket-aware callbacks that expose connection context like engine.getPeerHost().

That peer-host access is required here to inspect and potentially override the identity used for endpoint verification before delegating to the underlying trust manager. Implementing only X509TrustManager would lose access to the engine-aware APIs, so the SAN-based peer identity selection logic would not be possible.

The constructor taking an X509ExtendedTrustManager defines the delegate type being wrapped (that actually performs verifications), while inheritance ensures the wrapper itself remains substitutable anywhere an X509ExtendedTrustManager is expected by the JDK/OpenSSL SSL context setup paths.

szymon-miezal · 2026-05-20T09:23:49Z

+    private static final Integer DNS_SAN_TYPE = Integer.valueOf(2);
+    private static final Integer IP_SAN_TYPE = Integer.valueOf(7);


Where are these values taken from?
Do these need to be big integers?

These values come from the X.509 GeneralName SAN type numbers returned by X509Certificate.getSubjectAlternativeNames(): 2 for DNS and 7 for IP. They do not need to be big Integers. I've changed it.

szymon-miezal · 2026-05-20T09:26:20Z

+
+        @Override
+        public SSLSession getSession() {
+            return new DelegatingPeerHostSession(super.getSession(), peerHost);


Don't we need here the same null guard as in getHandshakeSession? If not, why?

I've added it.

szymon-miezal · 2026-05-20T09:28:51Z

+            System.out.println("SanPeerIdentityTrustManager: lookup skipped: engine=" + (engine != null)
+                               + ", chainPresent=" + (chain != null)
+                               + ", leafPresent=" + (chain != null && chain.length > 0 && chain[0] != null));


Standard output loggers are a bad idea, please rework it to use the pattern used in other classes.

replaced them.

szymon-miezal · 2026-05-20T09:56:19Z

+    private String resolvePeerIdentity(SSLEngine engine, X509Certificate leafCertificate) throws CertificateException {
+        String peerHost = engine.getPeerHost();
+        System.out.println("SanPeerIdentityTrustManager: resolving peerHost=" + peerHost
+                           + ", reverseLookupHosts=" + reverseLookupHosts);
+        if (peerHost == null || peerHost.isEmpty()) {
+            System.out.println("SanPeerIdentityTrustManager: aborted due to empty peerHost");
+            return null;
+        }
+
+        SanTypes sanTypes = readSanTypes(leafCertificate);
+        System.out.println("SanPeerIdentityTrustManager: SAN types for peerHost=" + peerHost
+                           + ", hasDnsSans=" + sanTypes.hasDnsSans
+                           + ", hasIpSans=" + sanTypes.hasIpSans);
+        if (!sanTypes.hasDnsSans && !sanTypes.hasIpSans) {
+            System.out.println("SanPeerIdentityTrustManager: aborted for peerHost=" + peerHost
+                               + " because certificate has no DNS/IP SANs");
+            return null;
+        }
+
+        if (sanTypes.hasIpSans && !sanTypes.hasDnsSans) {
+            System.out.println("SanPeerIdentityTrustManager: using original peerHost=" + peerHost
+                               + " because certificate only has IP SANs");
+            return peerHost;
+        }
+
+        if (sanTypes.hasDnsSans && !isIpAddress(peerHost)) {
+            System.out.println("SanPeerIdentityTrustManager: using original peerHost=" + peerHost
+                               + " because it is already a hostname and certificate has DNS SANs");
+            return peerHost;
+        }
+
+        if (sanTypes.hasDnsSans && reverseLookupHosts) {
+            String canonicalHost = reverseLookup(peerHost);
+            System.out.println("SanPeerIdentityTrustManager: reverse lookup for peerHost=" + peerHost
+                               + " returned canonicalHost=" + canonicalHost);
+            if (canonicalHost != null && !isIpAddress(canonicalHost)) {
+                System.out.println("SanPeerIdentityTrustManager: using reverse-looked-up canonicalHost="
+                                   + canonicalHost + " for peerHost=" + peerHost);
+                return canonicalHost;
+            }
+        }
+
+        System.out.println("SanPeerIdentityTrustManager: falling back to original peerHost=" + peerHost);
+        return peerHost;
+    }


It looks like logic that could easily be unit tested

szymon-miezal · 2026-05-20T09:58:12Z

+    private final X509ExtendedTrustManager delegate;
+    private final boolean reverseLookupHosts;
+
+    SanPeerIdentityTrustManager(X509ExtendedTrustManager delegate, boolean reverseLookupHosts) {


I think that we should assume the goal of this class is performing a DNS lookup (if needed), in other words, it should be only created if the passed reverseLookupHosts argument is true and then we don't need this parameter at all.

Furthermore I don't see any executions of SanPeerIdentityTrustManager with reverseLookupHosts=false.

Agreed. In the current implementation the wrapper is only created for the reverse-lookup-enabled case, and there are no false call sites. I will simplify this by removing the reverseLookupHosts parameter and making reverse lookup part of the class’s fixed behavior.

szymon-miezal · 2026-05-20T09:58:38Z

+    }
+
+    @Override
+    public void checkClientTrusted(X509Certificate[] chain, String authType, java.net.Socket socket)


Is there a reason why Socket can't be imported?

szymon-miezal · 2026-05-20T10:01:46Z

+            if (sanPeerIdentityLookup && tms[i] instanceof X509ExtendedTrustManager) {
+                tms[i] = new SanPeerIdentityTrustManager((X509ExtendedTrustManager) tms[i], true);
+            }


Maybe we could move it to a wrapIfNeeded status method of SanPeerIdentityTrustManager?

szymon-miezal · 2026-05-21T09:29:23Z

+        return false;
+    }
+
+    @SuppressJava6Requirement(reason = "Usage guarded by java version check")


There is no java version check with wrapTrustManagerIfNeeded, what is the point of this annotation?

wrapTrustManagerIfNeeded() references X509ExtendedTrustManager, which in my understanding triggers maybe Netty’s Java compatibility/static-analysis checks because it is newer than the older Java 6 SSL APIs. The suppression is used to acknowledge that intentional usage.

In this case the safety comes from the runtime capability/type check we've done via useExtendedTrustManager(manager), which ensures the SAN-aware wrapper(SanPeerIdentityTrustManager) is only applied when the provided trust manager supports the extended engine-aware APIs required by newly implemented SanPeerIdentityTrustManager.

There are similar usages of @SuppressJava6Requirement around other X509ExtendedTrustManager integrations in the same class, such as setVerifyCallback() and ExtendedTrustManagerVerifyCallback().

szymon-miezal · 2026-05-21T09:38:13Z

+    static final SslContextOption<Boolean> SAN_PEER_IDENTITY_LOOKUP =
+            new SslContextOption<Boolean>("SAN_PEER_IDENTITY_LOOKUP");


Where are we setting these options?

This option is not enabled inside Netty itself. It is intended to be set by the downstream caller that builds the client SslContextBuilder. In our case, that caller is BDP, which enables the option when constructing the client SSL context. look here.

szymon-miezal · 2026-05-21T09:42:39Z

+        if (!sanPeerIdentityLookup) {
+            return manager;
+        }
+        if (useExtendedTrustManager(manager)) {


Please explain what's the intent behind the useExtendedTrustManager check.

useExtendedTrustManager(manager) ensures that the SAN-aware wrapper(SanPeerIdentityTrustManager) is only applied when the provided trust manager supports the engine-aware X509ExtendedTrustManager APIs. SanPeerIdentityTrustManager relies on SSLEngine-based callbacks to inspect and potentially override the peer identity used for endpoint verification, which if i understand correctly is not possible with X509TrustManager

szymon-miezal · 2026-05-21T09:49:59Z

     */
    public <T> SslContextBuilder option(SslContextOption<T> option, T value) {
+        if (SanPeerIdentityConfig.SAN_PEER_IDENTITY_LOOKUP.equals(option)) {
+            sanPeerIdentityLookup = value != null && Boolean.TRUE.equals(value);


Why does setting the sanPeerIdentityLookup happen only when the value is true?

because this behavior (SAN-aware certificate matching) is meant to be configurable by the code that builds the SSL context, and we can configure it in BDP itself.

Enable SAN-aware peer identity lookup for Netty client endpoint verification based on hostname. Added a new opt-in SAN-aware peer identity feature via `SAN_PEER_IDENTITY_LOOKUP` in Netty SSL context configuration. Implemented `SanPeerIdentityTrustManager` to dynamically select the verification identity based on certificate SAN types, including optional reverse-DNS lookup for IP-based peers with DNS SAN certs. Integrated the feature into both JDK and OpenSSL client TLS paths by wrapping `X509ExtendedTrustManager` during SSL context/session creation. Updated `SslContextBuilder`, `OpenSslClientContext`, `ReferenceCountedOpenSslClientContext`, and `JdkSslClientContext` so the SAN-aware verification behavior is propagated consistently across providers.

szymon-miezal reviewed May 20, 2026

View reviewed changes

szymon-miezal reviewed May 21, 2026

View reviewed changes

piyushk010 force-pushed the DSP-24887-4.1.128.2 branch from 82db417 to 43d26a1 Compare May 26, 2026 21:47

		private static TrustManager[] wrapIfNeeded(TrustManager[] tms, boolean sanPeerIdentityLookup,
		ResumptionController resumptionController) {

		private static final Integer DNS_SAN_TYPE = Integer.valueOf(2);
		private static final Integer IP_SAN_TYPE = Integer.valueOf(7);

		static final SslContextOption<Boolean> SAN_PEER_IDENTITY_LOOKUP =
		new SslContextOption<Boolean>("SAN_PEER_IDENTITY_LOOKUP");

Conversation

piyushk010 commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szymon-miezal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piyushk010 May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piyushk010 May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

piyushk010 commented May 18, 2026 •

edited

Loading

piyushk010 May 26, 2026 •

edited

Loading

piyushk010 May 26, 2026 •

edited

Loading