Thanks Enrico, we've made a mistake though: discussed that fips-mode will be enabled by default on master branch and disabled by default on branch-3.8.
Let me create a separate pull request for that. Andor On Thu, 2023-06-15 at 14:39 +0200, Enrico Olivelli wrote: > Il giorno mer 14 giu 2023 alle ore 13:43 Andor Molnar > <an...@apache.org> ha scritto: > > PR has been created with the proposed resolution: > > > > https://github.com/apache/zookeeper/pull/2008 > > Committed to master and branch-3.8 > > Thank you > Enrico > > > Please review. > > > > Thanks, > > Andor > > > > > > > > On Sat, 2023-06-10 at 11:25 +0200, Andor Molnar wrote: > > > "we use this method dozens of other places in the code" > > > > > > Checked. Mostly logging and output formatting like 4lws, etc. > > > > > > > > > > > > On Sat, 2023-06-10 at 11:18 +0200, Andor Molnar wrote: > > > > 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("HTT > > > > > > > > > PS") > > > > > > > > > ; > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > >
