WIP: basic support for SASL/PLAIN #923

thomaslee · 2018-04-11T08:59:40Z

Still plenty more to do here (honestly, it's a bit of a mess in places), but thought I might open this up for comment.

We have a pressing need for dumb authentication in our Kafka cluster(s) to work around some infrastructure issues and kafka-node's support for TLS client certs isn't an option in the short term. Our immediate needs would be met with basic support for SASL/PLAIN, but obviously kafka-node doesn't support SASL -- just wanted to prove to myself that it was fairly trivial to hook up something simple. So this is my whirlwind prototype.

Very, very briefly verified against a local Kafka instance configured with a SASL_PLAINTEXT listener, seems to work -- just barely. 😅

Support for SASL/GSSAPI & SASL/SCRAM-SHA-{256,512} are lower on my list of priorities, but would look pretty similar.

Previous discussion by others back in 2016 over in #511 and #482. Hopefully SASL support is still something you're open to @hyperlink.

hyperlink · 2018-04-11T15:06:51Z

Thanks for your contribution @thomaslee ! I am absolutely open to whatever the community puts out there. All these new contributions are very encouraging to see. I will go through these PRs as soon as I can.

Can you share details on how you configured kafka to work with SASL? Ideally create another Kafka container with SASL enabled and add test to verify this works. 👍

thomaslee · 2018-04-12T03:26:40Z

Sure -- there are a few steps, hopefully I haven't missed anything:

In config/server.properties, add a SASL_PLAINTEXT listener and set sasl.enabled.mechanisms:

listeners=PLAINTEXT://:9092,SASL_PLAINTEXT://:9192
sasl.enabled.mechanisms=PLAIN

then create config/kafka_server_jaas.conf (you can add more user_XXX lines for additional users):

KafkaServer {
    org.apache.kafka.common.security.plain.PlainLoginModule required
    user_myuser="mypassword";
};

lastly, when launching Kafka, set kava.security.auth.login.config via KAFKA_OPTS:

KAFKA_OPTS="-Djava.security.auth.login.config=config/kafka_server_jaas.conf" bin/kafka-server-start.sh config/server.properties

Might be an easier way, but this seems to work for me.

thomaslee · 2018-04-23T06:56:52Z

@hyperlink I've added a positive & a negative test for SASL/PLAIN auth (including broker config for an extra SASL_PLAINTEXT listener -- the final config ended up being a bit more involved than what my earlier config might have implied). Both are passing for me locally against Kafka 1.0.1.

That said, I've had a hard time getting the other tests to pass locally even without my changes -- leaning on Travis for broader verification.

thomaslee · 2018-04-23T07:34:12Z

Ugh, digging around in Kafka source code the test failures around the SASL stuff are legitimate: as I mentioned, I've been testing against Kafka 1.0.1, which supports SaslHandshake v1. Apparently Kafka versions 0.10 & 0.11 use v0 of the SaslHandshake API, which triggers a raw SASL exchange without the use of SaslAuthenticate{Request,Response}. As a result, I'll need to revisit this & port to SaslHandshake v0.

EDIT: modified to clarify 0.10 & 0.11 rather than 0.8-0.11: I'm not sure when the first support for SASL was introduced, but as far as I can tell SASL/PLAIN was only introduced in 0.10. So these tests will need to be skipped for anything <0.10

thomaslee · 2018-04-24T02:15:19Z

@hyperlink phew -- tests finally went green, then a build failed after a no-op change modifying commit history. 😢 I'm assuming that's an intermittent error.

Okay, so the nasty details:

As I mentioned: SaslHandshake v1 (used in 1.0.0+) is pretty straightforward: no surprises, easy to implement.

SaslHandshake v0 (used in 0.10/0.11) on the other hand gets a bit messy because the step where we send credentials doesn't actually use normal Kafka request/response headers. As a result, we can't rely on correlation IDs for SASL authentication responses -- so we really don't want non-SASL responses coming back while we're in the middle of a SASL handshake. See the override of KafkaClient.handleReceivedData for the nastiest part of this change.

Anyway, because of this we now wait for the API versions response to come back before proceeding with the SASL exchange + metadata load and (ultimately) allowing the client to become "ready". I think this is probably better behavior anyway: pretty sure before this change we'd fire off the API versions request in parallel with the initial metadata load, which works but probably isn't what was originally intended. (This explains some strange log messages I saw a while back too.)

Lastly, the broker seems to just close the connection if authentication fails when using SaslHandshake v0 -- so this change assumes that authentication has failed if you've sent a SASL authentication packet & the connection suddenly closes before you get a reply.

If the backward compatibility stuff is too nasty, I wouldn't shed a tear if we only supported SASL for 1.0.0+. Certainly an easier, smaller patch -- but I'm sure there would be folks out there who might want to use SASL with 0.10 or 0.11. Let me know either way.

I think that's everything. Seem reasonable? And should I set up Travis builders for 1.0.0 and 1.1.0? That will exercise the SaslHandshake v1 code path, which is not yet exercised by the PR builds.

hyperlink · 2018-04-24T14:54:13Z

😃 good work @thomaslee !

I will look at the PR in more detail and give some feedback soon.

Regarding the intermittent failures just ignore them for now I've been trying to improve the stability of the tests but they are hard to trackdown because they pass locally. I don't know why the ConsumerGroupStream tests fail. I have a branch where those tests pass everytime the suite runs. So it appears the failure is caused by some side effect of other tests.

Please setup travis building for kafka version 1 if you can and thanks for your contribution!

thomaslee · 2018-04-24T17:02:21Z

Cheers! Just added 1.0 and 1.1 to the Travis matrix. Something's up with the start-kafka.sh script when using KAFKA_OPTS in the 1.x images -- something to do with sed. Had to hack around that for now, holler out if you see a better solution.

thomaslee · 2018-04-24T22:50:54Z

Testing today revealed issues in ConsumerGroup with this change in place (using the SaslHandshake v1 code path): https://gist.githubusercontent.com/thomaslee/dfd4479a439c583e4dde490736c0d335/raw/5787ade7c1abcbfc8f06423ed935ddb4a429afb3/gistfile1.txt

The error on the broker side seems to be:

org.apache.kafka.common.errors.IllegalSaslStateException: Unexpected Kafka request of type JOIN_GROUP during SASL handshake.

Trying to figure out what's going on, but appreciate any insight.

thomaslee · 2018-04-25T02:26:05Z

Okay, seems like Client.sendGroupRequest doesn't have any smarts wrt waiting for for the usual broker connection initialization path, so it's possible (even likely) for consumer group requests to sneak in while we're still trying to authenticate a connection if out bootstrap server list differs from the advertised server list. This can occur irrespective of whether we're using SaslHandshake v0 or v1.

Seems like a proper fix might involve modifying brokerForLeader (& related methods like setupBroker etc.) to take a callback which gets invoked with a fully authenticated BrokerWrapper. Then we could ensure that any callers to brokerForLeader get a fully initialized, authenticated connection before they try to send anything.

That's a little more than I'd like to bite off here, so trying to figure out a work around by modifying waitUntilReady to take the authenticated state of a connection into account & working that into sendGroupRequest. Easy change, seems to work fine at a glance -- just ironing out a few quirks.

thomaslee · 2018-04-25T05:39:36Z

Alright, 11a66df introduced the new waitUntilReady logic that works around the issue where group requests were sneaking in before authentication.

thomaslee · 2018-04-25T17:19:44Z

Ugh, there's still more issues with this when we get disconnected from a broker. I think I need to go back to the drawing board on some of this & lean more heavily on waitUntilReady in general instead of trying to get the chain of callbacks just right.

hyperlink · 2018-04-25T18:07:41Z

@thomaslee probably outside the scope of this PR but I am thinking we should start to abstract the nitty gritty of broker initialization/authentication into the BrokerWrapper. The client makes requests normally as soon as it's instantiated and the BrokerWrapper will queue them up and process them once it's really ready. What do you think?

thomaslee · 2018-04-25T18:55:57Z

@hyperlink yeah, I think I could see that working: so basically a call to write would just queue up until isReady() === true? I'm doing something similar in an as-yet unpushed set of changes: all writes to a broker go via a sendWhenReady method in KafkaClient/Client. This model is looking significantly simpler / more reliable than trying to get those callbacks just right.

I think I like your proposal better, but given all the thrash on this PR I'm tempted to leave it to a subsequent PR if you can forgive the warts for now. 😃 Will push the new set of changes shortly.

thomaslee · 2018-04-25T23:04:13Z

@hyperlink alrighty, looks like tests are green again using the new approach that makes more liberal use of waitUntilReady. As expected, it also seems far more stable wrt existing consumer & producer logic.

Once you're happy with this & we land it, I'll follow up with a PR to rework the sendWhenReady logic for BrokerWrapper, as you hinted earlier -- unless you beat me to it, of course. 😄

thomaslee · 2018-05-03T21:24:53Z

@hyperlink I'll resolve that conflict this evening, but have you had a chance to take a look at this? Wondering how far away we realistically are from landing this (obviously non-trivial and possibly risky) change. We've got it monkey-patched into 2.6.0 internally, but as you can imagine it ain't pretty. 😄

hyperlink · 2018-05-04T13:24:54Z

@thomaslee reviewing this PR is next on my list.

thomaslee · 2018-05-05T00:37:26Z

Rebased & fixed up that conflict 😄

hyperlink

Do you mind adding documentation options.sasl in the readme?

hyperlink · 2018-05-11T17:53:12Z

lib/client.js

+  var handlers = this.unqueueCallback(socket, correlationId);
+
+  if (handlers) {
+    var decoder = handlers[0];


we're dropping node 4 so feel free to destructure to your heart's content.

hyperlink · 2018-05-11T18:29:15Z

lib/kafkaClient.js

-          logger.error('error initialize broker after connect', error);
+          logger.error('error initializing broker after connect', error);
+          if (error instanceof errors.SaslAuthenticationError) {
+            self.emit('auth_error', error);


why not use just emit on error?

Great question, I think I was concerned about raising an error here when we don't do so for other types of error (and presumably raising other types of error here would have knock-on implications for tests, client code, etc.)

To put it another way, I wanted to bubble up this error for test purposes but wasn't convinced this PR warranted changing error semantics & potentially breaking stuff. If that makes any sense at all. It makes a potentially large/risky change even larger and riskier. 😅

I'm leaning towards emitting an error. I would expect consumers/producers would want to be notified of this failure since a failure in authentication means messages will not be received or delivered.

hyperlink · 2018-05-14T17:06:16Z

lib/wrapper/BrokerWrapper.js

@@ -41,7 +43,7 @@ BrokerWrapper.prototype.isConnected = function () {
 };

 BrokerWrapper.prototype.isReady = function () {
-  return this.apiSupport != null;
+  return this.apiSupport != null && (!this.needAuthentication || this.authenticated);


could you add authenticated to the toString override?

hyperlink · 2018-05-14T17:07:36Z

lib/kafkaClient.js

+  async.waterfall(
+    [
+      callback => {
+        logger.debug(`Sending SASL/${mechanism} handshake request to ${broker.socket.addr}`);


can use ${broker} since the overridden toString should print the socket.addr along with other useful information.

Ah of course 👍

hyperlink · 2018-05-14T17:15:32Z

lib/kafkaClient.js

-      );
+      let error = this.error;
+      if (!error) {
+        if (socket.saslAuthCorrelationId !== undefined) {


should we also check this.options.sasl?

Can't hurt!

…n up some details

thomaslee · 2018-07-07T02:15:53Z

@hyperlink finally got around to applying your review feedback + rebased. Let me know if anything else needs work. FWIW we've been running the previous version of this code in production environments for a month or two without incident.

prasadkashyap · 2018-07-19T19:06:24Z

Thanks for this PR @thomaslee.

@hyperlink when do you plan on merging this please?

lordgraysith · 2018-07-19T22:57:56Z

@hyperlink we played with this at work. It seems functional and we are anxious to use it. We'd love to see it merged as well.
@thomaslee nice work!

sureshappana · 2018-07-20T14:18:54Z

@hyperlink We'd love to use this at work!!

Could you please give some timeline after merge to release?

thomaslee · 2018-07-21T01:40:36Z

Appreciate the votes of confidence folks & nobody's keener to see this land than I am -- but some patience is warranted here. It's not a totally trivial change, the project lead has a day job and I was very slow in following up on his PR feedback (because I too have a day job 😉).

That said, appreciate any feedback in terms of testing/code review/etc. in the interim (looking at you @lordgraysith -- thanks for giving it a shot!). If we can get more confidence that this stuff is solid & ready to go I'm sure it'll make Xiaoxin's job easier when he does get a chance to take a look. Cheers!

lordgraysith · 2018-08-13T15:32:57Z

Thank you so much @hyperlink and @thomaslee! We're excited to see this published out on npm.

hyperlink · 2018-09-05T18:34:46Z

Published as 3.0.0

gshakya30 · 2018-10-26T07:04:33Z

@thomaslee , Is there any plan to provide support for SASL/GSSAPI??

thomaslee force-pushed the tom_sasl_plain branch from 4980737 to fe28d91 Compare April 11, 2018 09:02

thomaslee force-pushed the tom_sasl_plain branch 2 times, most recently from 5370c11 to 1996f4a Compare April 23, 2018 06:51

thomaslee force-pushed the tom_sasl_plain branch 3 times, most recently from 4eb6b09 to b42cfbe Compare April 24, 2018 01:43

hyperlink self-requested a review April 24, 2018 14:33

thomaslee force-pushed the tom_sasl_plain branch from 0a39941 to a04d91e Compare April 25, 2018 03:22

thomaslee force-pushed the tom_sasl_plain branch 2 times, most recently from e53b393 to 318306c Compare April 25, 2018 22:18

thomaslee force-pushed the tom_sasl_plain branch 2 times, most recently from e6e1bbc to 82ac886 Compare May 5, 2018 00:36

hyperlink reviewed May 14, 2018

View reviewed changes

thomaslee and others added 18 commits July 6, 2018 16:41

Extremely rough first pass at SASL/PLAIN

b8b025c

Don't use SASL unless it's been configured

eab81f1

Add SASL_PLAINTEXT to docker-compose + necessary config

e3b7e4e

Add SASL/PLAIN test, rework config a little

1e70e3e

Add a negative test for SASL/PLAIN, add SaslAuthenticationError, clea…

31b5bd2

…n up some details

Fix broken test

df54f1d

Fix another test

de52d63

Ensure all broker writes are predicated on readiness

1d957f3

Incorporate authentication status into broker readiness

b1f6f7f

Implementing SaslHandshake v0 for Kafka 0.10, 0.11

86ba57c

Group SASL tests

c018ce4

Skip SASL tests on Kafka 0.8 and 0.9

4c6804d

Fixing tests

4b93985

Add Kafka 1.0 and 1.1 to the Travis CI matrix

55d239f

Add auth_error signal for tests

9c9d8ac

Fix eslint issues

d60a908

Review feedback

a39e3f1

Document 'sasl' option for KafkaClient

1585eeb

thomaslee force-pushed the tom_sasl_plain branch from 82ac886 to 1585eeb Compare July 7, 2018 01:30

hyperlink approved these changes Aug 8, 2018

View reviewed changes

hyperlink merged commit ee5e0c6 into SOHU-Co:master Aug 8, 2018

vsnine mentioned this pull request Apr 25, 2019

Update Kafka consumer to support SASL authentication F5Networks/f5-telemetry-streaming#10

Closed

WIP: basic support for SASL/PLAIN #923

WIP: basic support for SASL/PLAIN #923

Conversation

thomaslee commented Apr 11, 2018

hyperlink commented Apr 11, 2018

thomaslee commented Apr 12, 2018

thomaslee commented Apr 23, 2018

thomaslee commented Apr 23, 2018 • edited Loading

thomaslee commented Apr 24, 2018

hyperlink commented Apr 24, 2018

thomaslee commented Apr 24, 2018

thomaslee commented Apr 24, 2018

thomaslee commented Apr 25, 2018

thomaslee commented Apr 25, 2018

thomaslee commented Apr 25, 2018

hyperlink commented Apr 25, 2018 • edited Loading

thomaslee commented Apr 25, 2018

thomaslee commented Apr 25, 2018

thomaslee commented May 3, 2018

hyperlink commented May 4, 2018

thomaslee commented May 5, 2018

hyperlink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaslee commented Jul 7, 2018

prasadkashyap commented Jul 19, 2018

lordgraysith commented Jul 19, 2018

sureshappana commented Jul 20, 2018

thomaslee commented Jul 21, 2018

lordgraysith commented Aug 13, 2018

hyperlink commented Sep 5, 2018

gshakya30 commented Oct 26, 2018

thomaslee commented Apr 23, 2018 •

edited

Loading

hyperlink commented Apr 25, 2018 •

edited

Loading