feat(core): improve SASL interface

[ceache]

feat(core): improve SASL interface

• Move SASL configuration out of auth_data into its own dictionary which
  exposes more SASL features (e.g. server service name, client principal,
  ...). Legacy syntax is still supported for backward compatibility.
• Remove SASL from auth_data and place it between 'connection' and
  'zookeeper protocol level authentication' to simplify connection logic
  and bring code in line with the protocol stack (SASL wraps Zookeeper,
  not the other way around).
• Consistent exception, AuthFailedError, raised during authentication
  failure between SASL and ZK authentication.
• New 'SASLException' exception raised in case of SASL intrisinc
  failures.
• Add support for GSSAPI (Kerberos).

Example connection using Digest-MD5:

  client = KazooClient(
      sasl_options={'mechanism': 'DIGEST-MD5',
                    'username': 'myusername',
                    'password': 'mypassword'}
  )

Example connection using GSSAPI (with some optional settings):

  client = KazooClient(
      sasl_options={'mechanism': 'GSSAPI',
                    'service': 'myzk',                  # optional
                    'principal': 'clt@EXAMPLE.COM'}     # optional
  )

I provided patches to pure-sasl to fix its DIGEST support but got pulled in by other projects before I could get around to clean it up, get legal approval, and contribute this back.
This is a rebase of code we have had in production for over a year now, with both DIGEST and GSSAPI authentications.

I have found this approach to be cleaner then trying to mix SASL data with Zookeeper's own auth_data.

Disclaimer from my employer:

THE FOLLOWING DISCLAIMER APPLIES TO ALL SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE:

THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS “AS IS” AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.

ONLY THE SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE, IF ANY, THAT ARE ATTACHED TO (OR OTHERWISE ACCOMPANY) THIS SUBMISSION (AND ORDINARY COURSE CONTRIBUTIONS OF FUTURES PATCHES THERETO) ARE TO BE CONSIDERED A CONTRIBUTION. NO OTHER SOFTWARE CODE OR MATERIALS ARE A CONTRIBUTION.

[ceache]

As for testing, the same method outlined in #533 can be used.
I am also working on adding code to the testing harness to support setting up a test Kerb environment (without root)

[anthonyliao]

This PR looks good. Was able to test this as well. See test below
@StephenSorriaux are you planning to merge this PR instead of #533 for GSSAPI authentication?

Test:

1. Create valid ticket in cache

$ kinit -l 5m anthonyliao
Password for anthonyliao:

$ klist
Ticket cache: FILE:/tmp/krb5cc_1524355
Default principal: anthonyliao

Valid starting       Expires              Service principal
12/7/2018 22:01:34  12/7/2018 22:06:32  tgt/testrealm.com@testrealm.com

2. Run the following script

from kazoo.client import KazooClient
from kazoo.security import make_acl
import time

zk = KazooClient(hosts='localhost', sasl_options={'mechanism':'GSSAPI'})
zk.start()

acl1 = make_acl('sasl', credential='anthonyliao', write=True)
acl2 = make_acl('world', credential='anyone', read=True, create=True, delete=True, admin=True)
zk.ensure_path('/TEST_NAMESPACE/testnode', acl=(acl1,acl2))
# Write some data to znode
zk.set('/TEST_NAMESPACE/testnode', 'data0')
# Update znode, will fail if not authenticated as anthonyliao
zk.set('/TEST_NAMESPACE/testnode', 'data1')

[StephenSorriaux]

Concerning connection using Kerberos, how are you managing the tickets renewal? It seems like the client must handle this, in case of re-connection

[StephenSorriaux]

This was a possibility I thought of when developping the initial DIGEST-MD5 SASL connection but I am not sure if, while authenticating using SASL, the zk server can not send the client any other kind of messages... I also noticed that, in the official java zk client, the choice was made to handle SASL packets in the readResponses() loop (see https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java#L876)

On your side, what do you think would be the best?

[ceache]

My understanding is that no other message can be exchanged during the SASL negotiation (and this block 879-885 seems to confirm it).
Also, while I was working on this and the pure-sasl patch for DIGEST-MD5, I modified a Zookeeper LUA dissector for Wireshark and I did not see any other messages.

That being said, Zookeeper is a little light on protocol specs, so I can double check that assumption, maybe something like Pings are still possible if I pause for long enough.

[StephenSorriaux]

I think it is safer to keep it the way it was currently done because, even if currently nothing might happen, we don't know what will happen in future release. Moreover, I am also not sure how this will behave when using watchers that are waiting for KazooState.CONNECTED state (that you are setting before the sasl authentication) to be spawned. In my development, that was the reason why I set the state after the sasl authentication.

[ceache]

I have studied the Java client code and it seems to me that no other packet is allowed while SASL negotiation is in progress. Their code goes to great length to bypass all send queue during that process and that the packets are strict XID for XID request/response exchanges:

• https://github.com/apache/zookeeper/blob/caca062767c36525e6ecead2ae0f34c447394809/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxnSocketNIO.java#L170
• https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java#L890
• https://github.com/apache/zookeeper/blob/cb9f303bda9137d1aebe8eff3eab85c8a59f3cdd/zookeeper-server/src/main/java/org/apache/zookeeper/client/ZooKeeperSaslClient.java#L296

Furthermore, the client also does check that the state is CONNECTED or CONNECTEDREADONLY before starting the SASL authentication:

• https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java#L1163
• https://github.com/apache/zookeeper/blob/22e4acc802904231d537ae18f479cfc729851d87/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java#L638

This indicate the the SASL negotiation only starts once the connection to the server is established (which means a CONNECTED event was already emitted), which my PR emulates. Note that native forms of authentication, when passed in as auth_data, are also evaluated right after the client reaches the connected state.
The only possible downside to my approach is a connection watcher would get a CONNECTED event followed by a LOST which is already possible (networks can fail) and what happens with native authentication schemes.
Looking at the links above, what do you think? Do you agree with my interpretation?

As for factoring out the SASL negotiation in its own function, since only SASL packets can be sent during the authentication process anyway, and only during connection, I do not see how this could bite us down the road (and it definitely made my PR less invasive).
If you still feel strongly about this, can you give me guidance as to how you would tackle integrating the SASL loop, with the above state conditions, in the main send/receive loop?
Also, could this be done in a different refactoring PR?

[StephenSorriaux]

Thanks for all those links, you are right, I missed the findSendablePacket method and it really seems nothing can happen during the SASL authentication. It makes totally sense to do the things the way you suggest.

[ceache]

@StephenSorriaux

Concerning connection using Kerberos, how are you managing the tickets renewal? It seems like the client must handle this, in case of re-connection

I do not try to implement a ticket cache that would try to leverage keytabs to refresh tickets. Java does this because they are going down the rabbit hole of trying to abstract the OS but this is far from trivial. You would have to consider all the options provided in the JAAS config (use cache, use keytabs etc), and have some tooling to understand the expiration and renewals options of tickets.

Full disclosure: we have a completely managed environment in this regard so we did not need this functionality at work.
If we wanted to add this though, shouldn't it be implemented in a separate library than kazoo?

[StephenSorriaux]

@StephenSorriaux

Concerning connection using Kerberos, how are you managing the tickets renewal? It seems like the client must handle this, in case of re-connection

I do not try to implement a ticket cache that would try to leverage keytabs to refresh tickets. Java does this because they are going down the rabbit hole of trying to abstract the OS but this is far from trivial. You would have to consider all the options provided in the JAAS config (use cache, use keytabs etc), and have some tooling to understand the expiration and renewals options of tickets.

Full disclosure: we have a completely managed environment in this regard so we did not need this functionality at work.
If we wanted to add this though, shouldn't it be implemented in a separate library than kazoo?

I agree with you, you certainly know better Kerberos than me. Maybe we could also add those informations in the documentation in order to help any user for the settings?

[StephenSorriaux]

Thanks a lot for this PR and for your awesome work.

[StephenSorriaux]

Merged to master, and might be a 2.7.0 Kazoo version on its own.

[anthonyliao]

Hi @StephenSorriaux, are there any plans to cut a release for this feature soon?

[StephenSorriaux]

Hi @anthonyliao, I would like to also merge #547 for the 2.7.0 version. Unfortunately, I did not find time to review / update it for now...

[ceache]

@StephenSorriaux @anthonyliao I would like to investigate the high rate of "timeouts" in the Travis integration jobs too.
This is not particularly related to SASL functions, or #547 but something we might want to check before cutting a new release.

I think I traced it to the ZkClient.stat() code assuming any early error is a "timeout" but I am still investigating.
I have been quite busy recently at work but I'll try to clear this this weekend.