First, I've created a pull request for ZOOKEEPER-3860:
https://github.com/apache/zookeeper/pull/2005
To improve the logging in ZKTrustManager without altering the
behaviour. The patch also contains a change in
NetUtils.formatInetAddr() which, I believe, should use the hostname
when creating textual representation of an InetAddress. I'm not 100%
sure about this, because while it certainly helps in TLS cases to avoid
unnecessary reverse DNS lookups, we use this method dozens of other
places in the code. Unit tests are passsing.
ZOOKEEPER-4268
It's about reverse lookups in the client code, but I haven't found the
reported behaviour on latest master, so just closed the ticket.
Andor
On Fri, 2023-06-09 at 18:29 +0200, Szalay-Bekő Máté wrote:
> yeah, I remember these tickets, thanks for picking them up!
> I agree and like the solution you proposed, in general in the long
> term it
> is good not to use a custom trust manager, but rely on the standard
> one.
>
> Máté
>
>
> On Fri, Jun 9, 2023 at 2:08 PM Enrico Olivelli <eolive...@gmail.com>
> wrote:
>
> > Il giorno ven 9 giu 2023 alle ore 14:07 Andor Molnar
> > <an...@apache.org> ha scritto:
> > > I'd like to backport this to the 3.8 branch too.
> > >
> > > Let's say I'll add new "zookeeper.fips-mode" parameter which will
> > > be
> > > "false" by default in 3.8 and "true" for 3.9.0.
> >
> > I am +1
> > ZK 3.9 will take time to be adopted and this is an important
> > security
> > related topic
> >
> > Enrico
> >
> > > Thoughts?
> > >
> > > Andor
> > >
> > >
> > >
> > > On Fri, 2023-06-09 at 13:55 +0200, Enrico Olivelli wrote:
> > > > I think that switching to
> > > > sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); is a
> > > > good
> > > > option.
> > > > The less tweaks we have about Security code the better.
> > > >
> > > >
> > > > It would be great to see this in 3.9.0.
> > > >
> > > > Enrico
> > > >
> > > > Il giorno ven 9 giu 2023 alle ore 13:42 Andor Molnar
> > > > <an...@apache.org> ha scritto:
> > > > > Hi zk folks,
> > > > >
> > > > > Problem(s)
> > > > > ==========
> > > > >
> > > > > One problem that we're having with a custom Trust Manager in
> > > > > ZK is
> > > > > that
> > > > > FIPS doesn't allow that:
> > > > >
> > > > > https://issues.apache.org/jira/browse/ZOOKEEPER-4393
> > > > >
> > > > > In FIPS mode the only allowed TrustManager in the JDK is
> > > > > X509TrustManagerImpl which is the default implementation. The
> > > > > class
> > > > > is
> > > > > final, so extending it is not an option unfortunately.
> > > > >
> > > > > The intention behind implementing a custom trust manager in
> > > > > ZK was,
> > > > > I
> > > > > believe, the need for server and client-side hostname
> > > > > verification.
> > > > > Hostname verification officially is not part of the SSL/TLS
> > > > > protocol,
> > > > > it's the responsibility of an upper level protocol like
> > > > > HTTPS.
> > > > >
> > > > > Hacking hostname verification in the SSL handshake is nice
> > > > > and was
> > > > > working fine so far, but unfortunately breaks the FIPS
> > > > > standard.
> > > > >
> > > > > Another annoying issue with ZKTrustManager is the need for
> > > > > reverse
> > > > > DNS
> > > > > lookup. This is usually needed when the hostname of the
> > > > > certificate
> > > > > provider is not known at the time of handshake. For instance,
> > > > > when
> > > > > somebody connects the client via IP address, which is
> > > > > generally not
> > > > > recommended when TLS is active in the client-server protocol.
> > > > >
> > > > > The bigger problem I've found is in the leader election: when
> > > > > a
> > > > > peer
> > > > > connects with a smaller id, the node will close the existing
> > > > > connection
> > > > > and opens a new one in the other direction, based on the
> > > > > information
> > > > > received in the InitialMessage from the peer which only
> > > > > contains
> > > > > the IP
> > > > > address, not the hostname. Therefore TrustManager needs to
> > > > > perform
> > > > > reverse DNS lookup.
> > > > >
> > > > > Tickets about reverse DNS lookup issues:
> > > > > https://issues.apache.org/jira/browse/ZOOKEEPER-3860
> > > > > https://issues.apache.org/jira/browse/ZOOKEEPER-4268
> > > > >
> > > > > Proposal
> > > > > ========
> > > > >
> > > > > I suggest to remove ZKTrustManager entirely from the codebase
> > > > > and
> > > > > use
> > > > > the built-in, FIPS-Enabled X509TrustManagerImpl instead. It
> > > > > has the
> > > > > downside of losing hostname verification, but we have an
> > > > > option to
> > > > > re-
> > > > > enable it in client-server communication: Netty has built-in
> > > > > support
> > > > > for it, we just need to do
> > > > >
> > > > > sslParameters.setEndpointIdentificationAlgorithm("HTTPS");
> > > > >
> > > > > when creating the SSLEngine and that will result in a
> > > > > behaviour
> > > > > very
> > > > > similar to what we provide currently. I can show some
> > > > > examples.
> > > > >
> > > > > What we will truly lose is the hostname verification option
> > > > > in the
> > > > > Quorum and Leader Election protocols. Since in these
> > > > > protocols we
> > > > > manipulate the sockets directly, we would need to implement
> > > > > the
> > > > > verification manually.
> > > > >
> > > > > What do you think about this trade-off?
> > > > >
> > > > > Of course, we can put this change behind a feature flag
> > > > > "fips-
> > > > > mode",
> > > > > which will lead to a new mode in ZooKeeper that is actually
> > > > > less
> > > > > strict
> > > > > as the original behaviour.
> > > > >
> > > > > Regards,
> > > > > Andor
> > > > >
> > > > >
> > > > >
