ZOOKEEPER-1112: Add support for C client for SASL authentication #1054

ztzg · 2019-08-12T12:49:03Z

This is a forward-port of Tom Klonikowski's (@kloni) ZOOKEEPER-1112 patches on top of the current master branch.

It stays close to the original patches, and only adds a number of commits on top:

A simple way to pass passwords to the console client via files;
Fixes and cleanups in the C library;
An Apache Rat exclusion rule for the test configuration file.

Open questions:

The patches add an optional dependency on the Cyrus SASL library.
Eric Yang wrote:

Apache Web Server has a module for authenticate sasl using Cyrus SASL library. I think the license should be compatible with written agreement from CMU.

Who should take care of this? Is there an established procedure?
Tom Klonikowski wrote:

Maybe part Reconnect #2 (sasl auth implementation via cyrus sasl) and fix typo: resourse -> resource #3 (cli using Reconnect #2) should be moved to the recipes section (recipes/sasl). What do you think?

What do you think?
Should cli_sasl_{st,mt} be folded back into cli_{st,mt}?

The existing tests as well as the added SASL tests pass in st and mt modes. Both cli_sasl_* utilities have been tested against a DIGEST-MD5 server; I intend to test them against a GSSAPI one in the near future.

…apache#1 This first patch implements the zookeeper sasl operations, extends the zkServer.sh test script with the ability to start a sasl enabled server and adds a test that checks the initial DIGEST-MD5 response. It introduces no external requirements but provides zoo_sasl/zoo_asasl functions that allow applications (or the addon from patch apache#2) to communicate with the sasl server backend. Patch apache#2 will add a simple api for sasl authentication, patch apache#3 includes a sasl enabled command line client. (Forward-ported from https://reviews.apache.org/r/2252/ by Damien Diederen.)

eolivelli

This is something we should have soon!
Thank you

I am not a C expert so I can't tell much about that part.
I left one comment for the java part and for the zkServer.sh script

bin/zkServer.sh

zookeeper-server/src/main/java/org/apache/zookeeper/util/SecurityUtils.java

The explicit QOP setting had been added with a comment specifying that Sasl.QOP="auth" was not set by the 1.6 JRE. More recent JREs, such as 1.8, properly set QOP to "auth" by default, and both Cyrus SASL and Perl's Authen::SASL have been verified to be okay with it. The patch also included 'auth-conf' and 'auth-int' in the preferences list; the reason for that is unclear. It also seems incorrect, as the wire protocol does not provide checksums nor encryption. (The plan is to carry everything over TLS anyway; perhaps QOP should be set to that triple when TLS is active?) apache#1054 (comment)

…apache#2 2nd patch * provides a simple api for sasl authentication (zoo_sasl_init, zoo_sasl_connect, zoo_sasl_authenticate) * requires libsasl2 (and plugins) * autoconf/make configuration * test for digest-md5 authentication * extended configuration for digest-md5 sasl server required by sasl2 (Forward-ported from https://reviews.apache.org/r/2315/ by Damien Diederen.)

…apache#3 3rd patch * includes a sasl-enabled cli as additional make artifacts When using the test configuration as in tests/jaas.digest.server.conf you can login via: cli_sasl_st -u super -h zk-sasl-md5 -m DIGEST-MD5 hostlist and password 'test'. If you set up Kerberos 5 on the server side and have a valid ticket its just: cli_sasl_st -m GSSAPI hostlist (Forward-ported from ticket attachment ZOOKEEPER-1112_3.patch by Damien Diederen.)

The explicit QOP setting had been added with a comment specifying that Sasl.QOP="auth" was not set by the 1.6 JRE. More recent JREs, such as 1.8, properly set QOP to "auth" by default, and both Cyrus SASL and Perl's Authen::SASL have been verified to be okay with it. The patch also included 'auth-conf' and 'auth-int' in the preferences list; the reason for that is unclear. It also seems incorrect, as the wire protocol does not provide checksums nor encryption. (The plan is to carry everything over TLS anyway; perhaps QOP should be set to that triple when TLS is active?) apache#1054 (comment)

The Cyrus SASL library expects us to pass type-punned pointers!

…a file This patch adds a -f <FILE> flag to the SASL test client. When used, the client extracts the first line of <FILE> (without newline) when a password is requested. E.g.: cli_sasl_st -u super -f super.secret -h zk-sasl-md5 -m DIGEST-MD5 hostlist

…tion The 'sasl_completion_ctx' structure cannot be freed directly after queuing the SASL request, as the data is not copied and the callback otherwise causes an use-after-free. It turns out we don't really need to allocate a new data structure, anyway: the only pointer which has to make it through is the SASL zoo_sasl_conn_t, and it has such a lifetime which makes it okay to store in the completion's data field.

…pport

(Forward-ported by Damien Diederen from a patch of Charles-Henri de Boysson's. All mistakes are mine.)

ztzg · 2019-08-21T11:20:40Z

Hi @eolivelli,

I have respun this series once more with a number of improvements and cleanups—including a patch from @ceache. Could you suggest somebody to "ping" for a review of the C code?

(Also: don't hesitate to let me know if you would like me to split this PR in three: 1/ fundamental ZK API, 2/ Cyrus SASL binding, and 3/ SASL client example.)

eolivelli · 2019-08-21T11:56:24Z

Please @phunt @anmolnar @hanm take a look

As mentioned in #1054 (comment) : There is a `sleep 3` statement in `zkServer.sh restart`. I am unable to unearth the history of that particular line, but I believe part—if not all—of that sleep should be part of `zkServer.sh stop`. I frequently observe `FAILED TO START` errors in the C test suite; the logs consistently show that those are caused by `java.net.BindException: Address already in use`. Adding a simple `sleep 1` before `echo STOPPED` "fixes" it for me. As noted in the commit message, the `sleep` is far from optimal, an adaptive mechanism would be better—but I do not want to make the first iteration too complicated. Author: Damien Diederen <dd@crosstwine.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes #1055 from ztzg/ZOOKEEPER-3510-zkserver-stop-delay

As mentioned in #1054 (comment) : There is a `sleep 3` statement in `zkServer.sh restart`. I am unable to unearth the history of that particular line, but I believe part—if not all—of that sleep should be part of `zkServer.sh stop`. I frequently observe `FAILED TO START` errors in the C test suite; the logs consistently show that those are caused by `java.net. 8000 BindException: Address already in use`. Adding a simple `sleep 1` before `echo STOPPED` "fixes" it for me. As noted in the commit message, the `sleep` is far from optimal, an adaptive mechanism would be better—but I do not want to make the first iteration too complicated. Author: Damien Diederen <dd@crosstwine.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes #1055 from ztzg/ZOOKEEPER-3510-zkserver-stop-delay (cherry picked from commit 942213d) Signed-off-by: Norbert Kalmar <nkalmar@apache.org>

The explicit QOP setting had been added with a comment specifying that Sasl.QOP="auth" was not set by the 1.6 JRE. More recent JREs, such as 1.8, properly set QOP to "auth" by default, and both Cyrus SASL and Perl's Authen::SASL have been verified to be okay with it. The patch also included 'auth-conf' and 'auth-int' in the preferences list; the reason for that is unclear. It also seems incorrect, as the wire protocol does not provide checksums nor encryption. (The plan is to carry everything over TLS anyway; perhaps QOP should be set to that triple when TLS is active?) apache#1054 (comment)

As mentioned in apache#1054 (comment) : There is a `sleep 3` statement in `zkServer.sh restart`. I am unable to unearth the history of that particular line, but I believe part—if not all—of that sleep should be part of `zkServer.sh stop`. I frequently observe `FAILED TO START` errors in the C test suite; the logs consistently show that those are caused by `java.net.BindException: Address already in use`. Adding a simple `sleep 1` before `echo STOPPED` "fixes" it for me. As noted in the commit message, the `sleep` is far from optimal, an adaptive mechanism would be better—but I do not want to make the first iteration too complicated. Author: Damien Diederen <dd@crosstwine.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1055 from ztzg/ZOOKEEPER-3510-zkserver-stop-delay

The explicit QOP setting had been added with a comment specifying that Sasl.QOP="auth" was not set by the 1.6 JRE. More recent JREs, such as 1.8, properly set QOP to "auth" by default, and both Cyrus SASL and Perl's Authen::SASL have been verified to be okay with it. The patch also included 'auth-conf' and 'auth-int' in the preferences list; the reason for that is unclear. It also seems incorrect, as the wire protocol does not provide checksums nor encryption. (The plan is to carry everything over TLS anyway; perhaps QOP should be set to that triple when TLS is active?) apache#1054 (comment)

ztzg · 2019-09-30T06:39:03Z

Greetings, all,

I am withdrawing this pull request for now. We had to rework the mechanism to enable automatic SASL authentication on reconnect without "abusing" the global watcher. I have a proof of concept, and intend to submit a cleaned-up new patch after further testing. (No ETA, though, as I am currently focused on other topics. Holler up if there is interest and I am too slow.)

Thanks, -D

…ibrary This is a "respin" of #1054, which I withdrew due to some annoying shortcomings. This changeset allows C clients to use SASL to authenticate with the ZooKeeper server. It is loosely based on patches #1 and #2 by Tom Klonikowski, at https://reviews.apache.org/r/2252/, but the result has been extensively reworked to follow the semantics of the Java client: * No SASL operations are exposed through the API; * The configuration is provided, and stored, at "handle init time"; * SASL authentication is automatically performed after each (re)connect. It introduces an optional dependency on the Cyrus SASL library, which can either be autodetected (default) or configured using the `--without-sasl`/`--with-sasl[=DIR]` flags, or -DWITH_CYRUS_SASL for CMake/Windows. `TestServerRequireClientSASLAuth.cc` has been renamed to `TestSASLAuth.cc`, and a test has been added which successfully (re)authenticates using the `DIGEST-MD5` mechanism. The code has also been used to successfully authenticate clients via `GSSAPI`/Kerberos. This commit also adds SASL support to the `cli.c` client. Co-authored-by: Tom Klonikowski <klonik_tinformatik.haw-hamburg.de> Author: Damien Diederen <dd@crosstwine.com> Reviewers: Mate Szalay-Beko <szalay.beko.mate@gmail.com>, Norbert Kalmar <nkalmar@apache.org> Closes #1134 from ztzg/ZOOKEEPER-1112-c-client-sasl-support-v2

…ibrary This is a "respin" of apache#1054, which I withdrew due to some annoying shortcomings. This changeset allows C clients to use SASL to authenticate with the ZooKeeper server. It is loosely based on patches #1 and #2 by Tom Klonikowski, at https://reviews.apache.org/r/2252/, but the result has been extensively reworked to follow the semantics of the Java client: * No SASL operations are exposed through the API; * The configuration is provided, and stored, at "handle init time"; * SASL authentication is automatically performed after each (re)connect. It introduces an optional dependency on the Cyrus SASL library, which can either be autodetected (default) or configured using the `--without-sasl`/`--with-sasl[=DIR]` flags, or -DWITH_CYRUS_SASL for CMake/Windows. `TestServerRequireClientSASLAuth.cc` has been renamed to `TestSASLAuth.cc`, and a test has been added which successfully (re)authenticates using the `DIGEST-MD5` mechanism. The code has also been used to successfully authenticate clients via `GSSAPI`/Kerberos. This commit also adds SASL support to the `cli.c` client. Co-authored-by: Tom Klonikowski <klonik_tinformatik.haw-hamburg.de> Author: Damien Diederen <dd@crosstwine.com> Reviewers: Mate Szalay-Beko <szalay.beko.mate@gmail.com>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1134 from ztzg/ZOOKEEPER-1112-c-client-sasl-support-v2

As mentioned in apache#1054 (comment) : There is a `sleep 3` statement in `zkServer.sh restart`. I am unable to unearth the history of that particular line, but I believe part—if not all—of that sleep should be part of `zkServer.sh stop`. I frequently observe `FAILED TO START` errors in the C test suite; the logs consistently show that those are caused by `java.net.BindException: Address already in use`. Adding a simple `sleep 1` before `echo STOPPED` "fixes" it for me. As noted in the commit message, the `sleep` is far from optimal, an adaptive mechanism would be better—but I do not want to make the first iteration too complicated. Author: Damien Diederen <dd@crosstwine.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1055 from ztzg/ZOOKEEPER-3510-zkserver-stop-delay

…ibrary This is a "respin" of apache#1054, which I withdrew due to some annoying shortcomings. This changeset allows C clients to use SASL to authenticate with the ZooKeeper server. It is loosely based on patches #1 and #2 by Tom Klonikowski, at https://reviews.apache.org/r/2252/, but the result has been extensively reworked to follow the semantics of the Java client: * No SASL operations are exposed through the API; * The configuration is provided, and stored, at "handle init time"; * SASL authentication is automatically performed after each (re)connect. It introduces an optional dependency on the Cyrus SASL library, which can either be autodetected (default) or configured using the `--without-sasl`/`--with-sasl[=DIR]` flags, or -DWITH_CYRUS_SASL for CMake/Windows. `TestServerRequireClientSASLAuth.cc` has been renamed to `TestSASLAuth.cc`, and a test has been added which successfully (re)authenticates using the `DIGEST-MD5` mechanism. The code has also been used to successfully authenticate clients via `GSSAPI`/Kerberos. This commit also adds SASL support to the `cli.c` client. Co-authored-by: Tom Klonikowski <klonik_tinformatik.haw-hamburg.de> Author: Damien Diederen <dd@crosstwine.com> Reviewers: Mate Szalay-Beko <szalay.beko.mate@gmail.com>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1134 from ztzg/ZOOKEEPER-1112-c-client-sasl-support-v2

As mentioned in apache#1054 (comment) : There is a `sleep 3` statement in `zkServer.sh restart`. I am unable to unearth the history of that particular line, but I believe part—if not all—of that sleep should be part of `zkServer.sh stop`. I frequently observe `FAILED TO START` errors in the C test suite; the logs consistently show that those are caused by `java.net.BindException: Address already in use`. Adding a simple `sleep 1` before `echo STOPPED` "fixes" it for me. As noted in the commit message, the `sleep` is far from optimal, an adaptive mechanism would be better—but I do not want to make the first iteration too complicated. Author: Damien Diederen <dd@crosstwine.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1055 from ztzg/ZOOKEEPER-3510-zkserver-stop-delay

…ibrary This is a "respin" of apache#1054, which I withdrew due to some annoying shortcomings. This changeset allows C clients to use SASL to authenticate with the ZooKeeper server. It is loosely based on patches #1 and #2 by Tom Klonikowski, at https://reviews.apache.org/r/2252/, but the result has been extensively reworked to follow the semantics of the Java client: * No SASL operations are exposed through the API; * The configuration is provided, and stored, at "handle init time"; * SASL authentication is automatically performed after each (re)connect. It introduces an optional dependency on the Cyrus SASL library, which can either be autodetected (default) or configured using the `--without-sasl`/`--with-sasl[=DIR]` flags, or -DWITH_CYRUS_SASL for CMake/Windows. `TestServerRequireClientSASLAuth.cc` has been renamed to `TestSASLAuth.cc`, and a test has been added which successfully (re)authenticates using the `DIGEST-MD5` mechanism. The code has also been used to successfully authenticate clients via `GSSAPI`/Kerberos. This commit also adds SASL support to the `cli.c` client. Co-authored-by: Tom Klonikowski <klonik_tinformatik.haw-hamburg.de> Author: Damien Diederen <dd@crosstwine.com> Reviewers: Mate Szalay-Beko <szalay.beko.mate@gmail.com>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1134 from ztzg/ZOOKEEPER-1112-c-client-sasl-support-v2

As mentioned in apache#1054 (comment) : There is a `sleep 3` statement in `zkServer.sh restart`. I am unable to unearth the history of that particular line, but I believe part—if not all—of that sleep should be part of `zkServer.sh stop`. I frequently observe `FAILED TO START` errors in the C test suite; the logs consistently show that those are caused by `java.net.BindException: Address already in use`. Adding a simple `sleep 1` before `echo STOPPED` "fixes" it for me. As noted in the commit message, the `sleep` is far from optimal, an adaptive mechanism would be better—but I do not want to make the first iteration too complicated. Author: Damien Diederen <dd@crosstwine.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1055 from ztzg/ZOOKEEPER-3510-zkserver-stop-delay

…ibrary This is a " 6D40 respin" of apache#1054, which I withdrew due to some annoying shortcomings. This changeset allows C clients to use SASL to authenticate with the ZooKeeper server. It is loosely based on patches #1 and #2 by Tom Klonikowski, at https://reviews.apache.org/r/2252/, but the result has been extensively reworked to follow the semantics of the Java client: * No SASL operations are exposed through the API; * The configuration is provided, and stored, at "handle init time"; * SASL authentication is automatically performed after each (re)connect. It introduces an optional dependency on the Cyrus SASL library, which can either be autodetected (default) or configured using the `--without-sasl`/`--with-sasl[=DIR]` flags, or -DWITH_CYRUS_SASL for CMake/Windows. `TestServerRequireClientSASLAuth.cc` has been renamed to `TestSASLAuth.cc`, and a test has been added which successfully (re)authenticates using the `DIGEST-MD5` mechanism. The code has also been used to successfully authenticate clients via `GSSAPI`/Kerberos. This commit also adds SASL support to the `cli.c` client. Co-authored-by: Tom Klonikowski <klonik_tinformatik.haw-hamburg.de> Author: Damien Diederen <dd@crosstwine.com> Reviewers: Mate Szalay-Beko <szalay.beko.mate@gmail.com>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1134 from ztzg/ZOOKEEPER-1112-c-client-sasl-support-v2

ztzg force-pushed the ZOOKEEPER-1112-c-client-sasl-support branch from 8986320 to 4fb8f97 Compare August 14, 2019 16:25

eolivelli requested changes Aug 14, 2019

View reviewed changes

bin/zkServer.sh Outdated Show resolved Hide resolved

zookeeper-server/src/main/java/org/apache/zookeeper/util/SecurityUtils.java Outdated Show resolved Hide resolved

ztzg force-pushed the ZOOKEEPER-1112-c-client-sasl-support branch from 4fb8f97 to 8d884e4 Compare August 15, 2019 08:03

ztzg mentioned this pull request Aug 15, 2019

ZOOKEEPER-3510: Make 'zkServer.sh stop' more reliable #1055

Closed

Tom Klonikowski and others added 10 commits August 21, 2019 11:42

ZOOKEEPER-1112: C client: Silence warnings in cli_sasl_*

0cad82e

The Cyrus SASL library expects us to pass type-punned pointers!

ZOOKEEPER-1112: C client: Fix stack-allocated buffer usage in SASL su…

7a303be

…pport

ZOOKEEPER-1112: C client: Exclude test configuration from Rat check

88b7756

ZOOKEEPER-1112: C client: Better align with other API conventions

2d1860d

ZOOKEEPER-1112: C client: Cleanups to zk_sasl.c

5889b65

(Forward-ported by Damien Diederen from a patch of Charles-Henri de Boysson's. All mistakes are mine.)

ztzg force-pushed the ZOOKEEPER-1112-c-client-sasl-support branch from 8d884e4 to 5889b65 Compare August 21, 2019 11:16

ztzg closed this Sep 30, 2019

ztzg mentioned this pull request Nov 6, 2019

ZOOKEEPER-1112: Add (Cyrus) SASL authentication support to C client library #1134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ZOOKEEPER-1112: Add support for C client for SASL authentication #1054

ZOOKEEPER-1112: Add support for C client for SASL authentication #1054

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZOOKEEPER-1112: Add support for C client for SASL authentication #1054

ZOOKEEPER-1112: Add support for C client for SASL authentication #1054

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!