Skip to content

Conversation

@ztzg
Copy link

@ztzg ztzg commented Nov 20, 2019

Hi @ewhauser,

The attached patch allows a .NET application to automatically authenticate via SASL when (re)connecting to a ZooKeeper ensemble. The feature is enabled by passing an implementation of the ISaslClient interface.

I am able to successfully perform DIGEST-MD5 authentication by implementing that interface using a slightly modified version of S22.Sasl (see below).

I am currently looking into adding GSSAPI support, which would help validate the mechanism—but haven't found a "reasonable" way to access that API bound under .NET yet (ideas/suggestions welcome on that front!).

Assuming I make some progress on the GSSAPI front, would you be interested in integrating a polished version of this patch?

Best,
Damien Diederen


class SaslClient : ISaslClient
{
    private SaslMechanism m = null;

    public byte[] Start()
    {
        m = SaslFactory.Create("DIGEST-MD5");

        m.Properties.Add("Username", "[...]");
        m.Properties.Add("Password", "[...]");
        m.Properties.Add("Protocol", "zookeeper");
        
        return new byte[] { };
    }

    public bool IsCompleted { get { return m == null || m.IsCompleted; } }

    public byte[] EvaluateChallenge(byte[] token)
    {
        return m.GetResponse(token);
    }

    public void Finish()
    {
        m = null;
    }
}

@ztzg
Copy link
Author

ztzg commented Dec 9, 2019

Hi @ewhauser,

I have just pushed a refreshed version of this patch which makes it possible to connect to a SASL-enabled ZooKeeper using either a fixed S22.Sasl (DIGEST-MD5) or a Cyrus SASL wrapper for .NET (DIGEST-MD5 and GSSAPI).

What do you think? Is there any interest in these patches?

More generally, it would be useful to have a clarification as of the status of this project: do you still use it? Do you know of any other active users? Would you accept patches adding support for ZK 3.5 features?

@fantasy0v0: As the last active contributor, would you have any comments?

@ewhauser
Copy link
Owner

ewhauser commented Dec 9, 2019

I'm happy to merge this. Is it possible to add a test?

@ztzg
Copy link
Author

ztzg commented Dec 9, 2019

Hi @ewhauser,

Great to read that you are still interested!

Re test: sure (hence the "WIP" in the current title). I'll re-spin ASAP, but don't hesitate to review/comment on the code in the meantime.

Cheers, -D

@ztzg ztzg force-pushed the RT-46545-zookeeper-net-sasl branch from 469fdda to 8c7e069 Compare December 11, 2019 13:29
@ztzg
Copy link
Author

ztzg commented Dec 11, 2019

I'm happy to merge this. Is it possible to add a test?

Re test: sure

Well, easier said than done… I have been struggling with this a bit, as the simplest mechanism provided by ZooKeeper is DIGEST-MD5 (and not PLAIN)—and and that already requires bringing a "true" SASL implementation along as the server includes nonces in challenges.

I have a branch which includes such a test—but it requires adding my (patched!) fork of S22.Sasl as a Git submodule and bumping the *.Tests projects to .NET 4.5:

https://github.com/ztzg/ewhauser-zookeeper.net/tree/RT-46545-zookeeper-net-sasl-with-tests

It doesn't change the ZooKeeperNet project, which can still targets .NET 4.0 thanks to the ISaslClient interface, but the whole thing still seems a bit overkill.

What do you think? Ideas welcome!

@ewhauser
Copy link
Owner

ewhauser commented Dec 12, 2019 via email

ztzg added 4 commits December 15, 2019 15:21
Built from this commit of https://github.com/ztzg/S22.Sasl:

    commit b85c919b39ede184d36bc2e04f63b6e530ca3e10
    Author: Damien Diederen <dd@crosstwine.com>
    Date:   Sun Dec 15 14:59:53 2019 +0100

        S22.Sasl.csproj: Retarget to .NET v4.0

https://github.com/ztzg/S22.Sasl/tree/RT-46545-zookeeper-net-sasl-binary
Note that for this test to pass, The following must be configured in zoo.conf:

    authProvider.1=org.apache.zookeeper.server.auth.SASLAuthenticationProvider

The following in jaas.conf:

    Server {
      org.apache.zookeeper.server.auth.DigestLoginModule required
        user_super="adminsecret"
        user_bob="bobsecret";
    };

And the server must be started with:

    -Djava.security.auth.login.config=.../jaas.conf

See https://cwiki.apache.org/confluence/display/ZOOKEEPER/Client-Server+mutual+authentication#Client-Servermutualauthentication-ServerConfiguration
for additional details.
@ztzg ztzg force-pushed the RT-46545-zookeeper-net-sasl branch from 8c7e069 to 4810e41 Compare December 15, 2019 14:28
@ztzg
Copy link
Author

ztzg commented Dec 15, 2019

Hi @ewhauser,

The existing tests just run against the Zookeeper server as an integration test. Seems like a reasonable solution for a test.

Right. But the server has to be configured for SASL authentication for the test to pass! I have added a note on how to do so at the top of ZooKeeperNet.Tests/SaslTests.cs, as I did not know where else to put it:

The following must be configured in zoo.conf:

    authProvider.1=org.apache.zookeeper.server.auth.SASLAuthenticationProvider

The following in jaas.conf:

    Server {
      org.apache.zookeeper.server.auth.DigestLoginModule required
        user_super="adminsecret"
        user_bob="bobsecret";
    };

And the server must be started with:

    -Djava.security.auth.login.config=.../jaas.conf

See https://cwiki.apache.org/confluence/display/ZOOKEEPER/Client-Server+mutual+authentication#Client-Servermutualauthentication-ServerConfiguration
for additional details.

Targeting a SASL-enabled server is only half the story, though.

The patch enables SASL authentication via an ISaslClient interface, but an application (or, in this case, integration test) also needs to provide an implementation of one of the supported SASL mechanisms (DIGEST-MD5 or GSSAPI).

Unlike Java, .NET does not carry one in the standard library, and the only one I could find was developed for IMAP and required patching to work with ZooKeeper.

I have built a .NET v4.0 assembly and included it in lib/, alongside the other binaries. The corresponding tree is https://github.com/ztzg/S22.Sasl/tree/RT-46545-zookeeper-net-sasl-binary:

Built from this commit of https://github.com/ztzg/S22.Sasl:

    commit b85c919b39ede184d36bc2e04f63b6e530ca3e10
    Author: Damien Diederen <dd@crosstwine.com>
    Date:   Sun Dec 15 14:59:53 2019 +0100

        S22.Sasl.csproj: Retarget to .NET v4.0

Does that work for you?

Cheers, -D

@ztzg ztzg changed the title WIP ZooKeeperNet: Implement optional SASL authentication on connect ZooKeeperNet: Implement optional SASL authentication on connect Dec 15, 2019
@ztzg
Copy link
Author

ztzg commented Jan 13, 2020

Hi @ewhauser,

Did you see my update above? If so, what do you think?

@ewhauser
Copy link
Owner

Yes, I'm ok with that direction.

@ewhauser ewhauser merged commit 98aa4b5 into ewhauser:trunk Jan 15, 2020
@ztzg
Copy link
Author

ztzg commented Jan 15, 2020

Fantastic :) Thanks!

@ztzg ztzg deleted the RT-46545-zookeeper-net-sasl branch March 18, 2020 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants