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 

Name of sequence node is not unique

2023-06-09 Thread Li Wang
Hello,

We are running 3.7.1 in production and running into an "issue" that the
names of sequence nodes are not unique after the counter hits the max int
(i.e 2147483647) and overflows.  I would like to start a thread to discuss
the following

1. Is this a bug or "expected" behavior?
2. Is ZK supposed to support the overflow scenario and need to make sure
the name is unique when overflow happens?

The name is not unique after hitting the max int value because of we
have the following in zk  code base:

1.  The cversion of parent znode is used to build the child name in
PrepRequestProcessor

int parentCVersion = parentRecord.stat.getCversion();
if (createMode.isSequential()) {
path = path + String.format(Locale.ENGLISH, "%010d",
parentCVersion);
}


https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/
java/org/apache/zookeeper/server/PrepRequestProcessor.java#L668-L671


2. The parent znode is read from either zks.outstandingChangesForPath map
or zk database/datatree.

   lastChange = zks.outstandingChangesForPath.get(path);
if (lastChange == null) {
DataNode n = zks.getZKDatabase().getNode(path);


https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java#L168-L170



3. The cversion of the parent node in outstandingChangesForPath map is
always updated  but not in zk database as we added the following code in 3.6

if (parentCVersion > parent.stat.getCversion()) {
parent.stat.setCversion(parentCVersion);
parent.stat.setPzxid(zxid);
}

https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java#L477-L480

https://issues.apache.org/jira/browse/ZOOKEEPER-3249


When overflow happens, the new parentCversion is changed to -2147483648.
It's updated in the outstandingChangesForPath map. It's not updated in
DataTree and the value stays as 2147483647  because -2147483648 is less
than 2147483647, so the cVerson is inconsistent in  ZK.

Due to the inconsistent cVersion, when the next request comes in after
overflow, the sequence number is non-deterministic and not unique depending
on where the parent node is read from.  It can be 2147483647 if the
parent node is read from DataTree or -2147483648,  -2147483647 and so on if
it's from the outstandingChangesForPath map.

We have the following doc about unique naming but no info on  "expected"
behavior after overflow.

Sequence Nodes -- Unique Naming


When creating a znode you can also request that ZooKeeper append a
monotonically increasing counter to the end of path. This counter is unique
to the parent znode. The counter has a format of %010d -- that is 10 digits
with 0 (zero) padding (the counter is formatted in this way to simplify
sorting), i.e. "01". See Queue Recipe for an example use of this
feature. Note: the counter used to store the next sequence number is a
signed int (4bytes) maintained by the parent node, the counter will
overflow when incremented beyond 2147483647 (resulting in a name
"-2147483648").



Please let me know if you have any comments or inputs.


Thanks,


Li


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





Re: Volounteers for releases ?

2023-06-09 Thread Andor Molnar
Hi Enrico,

I can take the master cut next week, but let me put together an email
about a TLS topic first. I'd like to propose a fix to resolve the
problem of FIPS (custome trust manager in ZK) and reverse DNS lookups.
I'd like to include it in 3.9.0 and 3.8.2.

Andor

p.s. Whoever is making a change on the webpage, please remove the 3.8.0
release.



On Fri, 2023-06-09 at 09:11 +0200, Enrico Olivelli wrote:
> Hello ZooKeepers,
> I think that it is time to do a round of releases.
> 
> We should cut a release out of the master branch, 3.9.0 and main
> cutting a release out of 3.7.x and 3.8.x would be useful.
> 
> Before cutting the release please ensure that third party libraries
> are not reported against CVEs
> 
> 
> This is the list of the latest releases
> https://zookeeper.apache.org/releases.html
> 
> Would anyone volunteer ?
> 
> Enrico



Volounteers for releases ?

2023-06-09 Thread Enrico Olivelli
Hello ZooKeepers,
I think that it is time to do a round of releases.

We should cut a release out of the master branch, 3.9.0 and main
cutting a release out of 3.7.x and 3.8.x would be useful.

Before cutting the release please ensure that third party libraries
are not reported against CVEs


This is the list of the latest releases
https://zookeeper.apache.org/releases.html

Would anyone volunteer ?

Enrico