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.

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

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.

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?

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?

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.

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.

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?

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?

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

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


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

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?

szymon-miezal reviewed May 20, 2026

View reviewed changes

szymon-miezal reviewed May 21, 2026

View reviewed changes

		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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

piyushk010 commented May 18, 2026 •

edited

Loading