Created a PR for review folks: https://github.com/apache/hbase/pull/6149
Andor On Fri, 2024-08-09 at 14:43 -0500, Andor Molnar wrote: > 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 > > > > > > > > > > > > > > > > > > > >
