Sure, makes perfect sense. Balazs, what's the reason for doing reverse DNS lookup when setting up the SSL context?
TrustManager will do it anyway during hostname verification, but the current implementation does a lookup for every single connection even if hostname verification is disabled. Andor On Fri, 2024-08-09 at 10:55 -0400, Bryan Beaudreault wrote: > I think OptionalSslHandler is very simple. Let's just create our own > which > applies the same fix that Balazs did for the non-optional case. > > On Fri, Aug 9, 2024 at 10:04 AM Andor Molnar <an...@apache.org> > wrote: > > > Thanks Balazs, I've found your fix since then and this is the > > OptionalSslHandler case, because I have plaintext mode enabled. > > > > Looks like there's no way to pass peer host/port to SslEngine, so I > > still think that explicitly disabling client hostname verification > > would > > be beneficial in this case. We could emit a warning message in the > > logs > > saying, hey, client hostname verification is disabled, because > > plaintext > > mode is on. > > > > The other way I'm thinking about is to implement our own version of > > OptionalSslHandler, but haven't verified if it works yet. > > > > Andor > > > > > > > > On 8/9/24 1:13 AM, Balazs Meszaros wrote: > > > Hi Andor! > > > > > > SSLEngine does not know anything about the socket that is used > > > for the > > > communication. It just encrypt/decrypt byte streams that you pass > > > through > > > it. If you check its source code, it just returns that it got in > > > the > > > constructor: > > > > > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/javax/net/ssl/SSLEngine.html#%3Cinit%3E(java.lang.String,int) > > > SSLEngine is not constructed directly in our code, it is wrapped > > > by > > Netty. > > > Netty creates it when you call SslContext.newHandler. There are > > > multiple > > > newHandler functions which accept the peer information: > > > > > https://netty.io/4.1/api/io/netty/handler/ssl/SslContext.html#newHandler-io.netty.buffer.ByteBufAllocator-java.lang.String-int- > > > I modified the handler creation in some places in HBASE-27673. > > > You have > > to > > > check the other code paths as well to see how it is created. For > > > example > > we > > > are using OptionalSsl handler but it does not pass peer > > > information when > > > creating the handler. We construct it here: > > > > > https://github.com/apache/hbase/blob/master/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcServer.java#L390 > > > Best regards, > > > Balazs > > > > > > On Fri, Aug 9, 2024 at 3:52 AM Andor Molnar <an...@apache.org> > > > wrote: > > > > > > > Hi, > > > > > > > > I'm debugging a strange error in our mTLS setup where I had to > > > > explicitly disable client hostname verification, because HBase > > > > keeps > > > > trying to validate 127.0.0.1/localhost as the peer host. > > > > > > > > ------------------------------------------ > > > > 2024-08-09 01:32:21,486 ERROR > > > > org.apache.hadoop.hbase.io.crypto.tls.HBaseTrustManager: Failed > > > > to > > > > verify host address: 127.0.0.1 > > > > javax.net.ssl.SSLPeerUnverifiedException: Certificate for > > > > <127.0.0.1> > > > > doesn't match common name of the certificate subject: my- > > > > perfect- > > > > hostname > > > > ... > > > > 2024-08-09 01:32:21,486 ERROR > > > > org.apache.hadoop.hbase.io.crypto.tls.HBaseTrustManager: Failed > > > > to > > > > verify hostname: localhost > > > > javax.net.ssl.SSLPeerUnverifiedException: Certificate for > > > > <localhost> > > > > doesn't match common name of the certificate subject: my- > > > > perfect- > > > > hostname > > > > ------------------------------------------ > > > > > > > > First it tries 127.0.0.1 and localhost after, because reverse > > > > lookup is > > > > enabled, but why is that localhost? > > > > > > > > Check is here: > > > > > > > > > > https://github.com/apache/hbase/blob/master/hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java#L97 > > > > The version of checkClientTrusted with chain, authType and > > > > engine. > > > > Based on SSLEngine javadoc: > > > > ------------------------------------------ > > > > /** > > > > * Returns the host name of the peer. > > > > * <P> > > > > * Note that the value is not authenticated, and should > > > > not be > > > > * relied upon. > > > > * > > > > * @return the host name of the peer, or null if nothing > > > > is > > > > * available. > > > > */ > > > > public String getPeerHost() { > > > > return peerHost; > > > > } > > > > ------------------------------------------ > > > > > > > > That explains it, because if the peerHost is null, > > > > InetAddress.getByName() doesn't fail and it returns the > > > > localhost. I > > > > have no idea under what circumstances can the peerHost be > > > > unknown, but > > > > would like to add a null check and skip hostname verification > > > > with a > > > > warning message in such case. > > > > > > > > It would be nice to get the peerHost from somewhere else as a > > > > fallback. > > > > > > > > Regards, > > > > Andor > > > > > > > > > > > > > > > >
