Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 11 commits into
base: master
from

Conversation

@ztzg
Copy link
Contributor

commented Aug 12, 2019

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:

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

Open questions:

  1. 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?

  2. Tom Klonikowski wrote:

    Maybe part #2 (sasl auth implementation via cyrus sasl) and #3 (cli using #2) should be moved to the recipes section (recipes/sasl). What do you think?

    What do you think?

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

ZOOKEEPER-1112: Add support for C client for SASL authentication Patch
…#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 #2) to
communicate with the sasl server backend.

Patch #2 will add a simple api for sasl authentication, patch #3
includes a sasl enabled command line client.

(Forward-ported from https://reviews.apache.org/r/2252/ by Damien Diederen.)

@ztzg ztzg force-pushed the ztzg:ZOOKEEPER-1112-c-client-sasl-support branch from 8986320 to 4fb8f97 Aug 14, 2019

@eolivelli
Copy link
Contributor

left a comment

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 Outdated Show resolved Hide resolved

@ztzg ztzg force-pushed the ztzg:ZOOKEEPER-1112-c-client-sasl-support branch from 4fb8f97 to 8d884e4 Aug 15, 2019

ztzg added a commit to ztzg/zookeeper that referenced this pull request Aug 15, 2019
ZOOKEEPER-1112: Do not specify QOP for the SASL server
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)
Tom Klonikowski and others added 10 commits Oct 7, 2011
ZOOKEEPER-1112: Add support for C client for SASL authentication Patch
…#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.)
ZOOKEEPER-1112: Add support for C client for SASL authentication Patch
…#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.)
ZOOKEEPER-1112: C client: Allow passing a password to cli_sasl_* via …
…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
ZOOKEEPER-1112: C client: Fix use-after-free in async SASL authentica…
…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.
ZOOKEEPER-1112: C client: Silence warnings in cli_sasl_*
The Cyrus SASL library expects us to pass type-punned pointers!
ZOOKEEPER-1112: Do not specify QOP for the SASL server
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?)

#1054 (comment)
ZOOKEEPER-1112: C client: Cleanups to zk_sasl.c
(Forward-ported by Damien Diederen from a patch of Charles-Henri de
Boysson's.  All mistakes are mine.)

@ztzg ztzg force-pushed the ztzg:ZOOKEEPER-1112-c-client-sasl-support branch from 8d884e4 to 5889b65 Aug 21, 2019

@ztzg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Please @phunt @anmolnar @hanm take a look

asfgit pushed a commit that referenced this pull request Aug 23, 2019
ZOOKEEPER-3510: Make 'zkServer.sh stop' more reliable
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
asfgit pushed a commit that referenced this pull request Aug 23, 2019
ZOOKEEPER-3510: Make 'zkServer.sh stop' more reliable
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

(cherry picked from commit 942213d)
Signed-off-by: Norbert Kalmar <nkalmar@apache.org>
ztzg added a commit to ztzg/zookeeper that referenced this pull request Aug 25, 2019
ZOOKEEPER-1112: Do not specify QOP for the SASL server
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)
bringhurst pushed a commit to bringhurst/zookeeper that referenced this pull request Aug 26, 2019
ZOOKEEPER-3510: Make 'zkServer.sh stop' more reliable
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.