Re: FIPS: removing ZKTrustManager

2023-06-15 Thread Andor Molnar
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
>  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
> > > > > >  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
> > > > > > > >  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
> > > > > > > > 

Re: FIPS: removing ZKTrustManager

2023-06-15 Thread Enrico Olivelli
Il giorno mer 14 giu 2023 alle ore 13:43 Andor Molnar
 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
> > > > >  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
> > > > > > >  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:
> > > > > 

Re: FIPS: removing ZKTrustManager

2023-06-14 Thread Andor Molnar
PR has been created with the proposed resolution:

https://github.com/apache/zookeeper/pull/2008

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
> > > >  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
> > > > > >  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
> > > > > > 

Re: FIPS: removing ZKTrustManager

2023-06-10 Thread Andor Molnar
"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  > >
> > wrote:
> > 
> > > Il giorno ven 9 giu 2023 alle ore 14:07 Andor Molnar
> > >  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
> > > > >  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
> > > > 

Re: FIPS: removing ZKTrustManager

2023-06-10 Thread Andor Molnar
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 
> wrote:
> 
> > Il giorno ven 9 giu 2023 alle ore 14:07 Andor Molnar
> >  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
> > > >  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 

Re: FIPS: removing ZKTrustManager

2023-06-09 Thread Andor Molnar
Sure, good point. I don't want to wipe it completely, just putting it
behind a feature flag.



On Fri, 2023-06-09 at 10:03 -0700, Patrick Hunt wrote:
> "remove ZKTrustManager entirely from the codebase" - what is the
> impact on
> backward compatibility if this is done? Why wouldn't we keep this as
> an
> option (not the default?) to ensure folks won't experience a "gap"
> when
> migrating to new versions. We could phase it out over time as part of
> such
> a plan (at least n-1 compatibility is something we have guaranteed in
> the
> past)
> 
> Patrick
> 
> On Fri, Jun 9, 2023 at 9:29 AM Szalay-Bekő Máté <
> szalay.beko.m...@gmail.com>
> 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  > >
> > wrote:
> > 
> > > Il giorno ven 9 giu 2023 alle ore 14:07 Andor Molnar
> > >  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
> > > > >  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 

Re: FIPS: removing ZKTrustManager

2023-06-09 Thread Patrick Hunt
"remove ZKTrustManager entirely from the codebase" - what is the impact on
backward compatibility if this is done? Why wouldn't we keep this as an
option (not the default?) to ensure folks won't experience a "gap" when
migrating to new versions. We could phase it out over time as part of such
a plan (at least n-1 compatibility is something we have guaranteed in the
past)

Patrick

On Fri, Jun 9, 2023 at 9:29 AM 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 
> wrote:
>
> > Il giorno ven 9 giu 2023 alle ore 14:07 Andor Molnar
> >  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
> > > >  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,
> > > 

Re: FIPS: removing ZKTrustManager

2023-06-09 Thread Szalay-Bekő Máté
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  wrote:

> Il giorno ven 9 giu 2023 alle ore 14:07 Andor Molnar
>  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
> > >  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
> > > >
> > > >
> > > >
> >
>


Re: FIPS: removing ZKTrustManager

2023-06-09 Thread Enrico Olivelli
Il giorno ven 9 giu 2023 alle ore 14:07 Andor Molnar
 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
> >  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
> > >
> > >
> > >
>


Re: FIPS: removing ZKTrustManager

2023-06-09 Thread Andor Molnar
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.

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
>  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
> > 
> > 
> > 



Re: FIPS: removing ZKTrustManager

2023-06-09 Thread Enrico Olivelli
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
 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
>
>
>


FIPS: removing ZKTrustManager

2023-06-09 Thread Andor Molnar
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