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

Add SASL handshake negotiation #748

Closed
wants to merge 1 commit into from

Conversation

guillaumebreton
Copy link
Contributor

@guillaumebreton guillaumebreton commented Sep 22, 2016

Here is a description of the problem I found :

Versions

Sarama Version: 482c471
Kafka Version: 0.10.0.1
Go Version: go1.7 darwin/amd64

Configuration

Kafka configuration

listeners=SASL_PLAINTEXT://:9091,SASL_SSL://:9093
advertised.listeners=SASL_PLAINTEXT://:9091,SASL_SSL://:9093
ssl.keystore.location=/opt/kafka/ssl/keystore/kafka.keystore.jks
ssl.keystore.password=XXXXXXXXXXX
ssl.key.password=XXXXXXXXXXX
ssl.truststore.location=/opt/kafka/ssl/truststore/kafka.truststore.jks
ssl.truststore.password=XXXXXXXXXXX
ssl.client.auth=required
security.inter.broker.protocol=SASL_SSL                                           
sasl.enabled.mechanisms=PLAIN                                                     
sasl.mechanism.inter.broker.protocol=PLAIN

Sarama configuration

config.Net.SASL.Enable = true
config.Net.SASL.User = username
config.Net.SASL.Password = password
Problem Description

Using tcp-proxy ang golang I have the following frames :

Connection #001 Opened :9091 >>> 167.114.245.179:9091
Connection #001 >>> 34 bytes sent
Connection #001 0000001e69616d746865636c69656e740069616d746865636c69656e7400746f746f
Connection #001 Closed (34 bytes sent, 0 bytes recieved)

Using tcp-proxy and python we have the following frames :

Connection #005 >>> 44 bytes sent
Connection #005 00000028001100000000000100176b61666b612d707974686f6e2d70726f64756365722d310005504c41494e
Connection #005 <<< 21 bytes received
Connection #005 00000011000000010000000000010005504c41494e
Connection #005 >>> 34 bytes sent
Connection #005 0000001e69616d746865636c69656e740069616d746865636c69656e7400746f746f
Connection #005 <<< 4 bytes received
Connection #005 00000000

With SASL plain authentication activated, sarama sends the username/password but does not send the SASL handshake to start the SASL negotiation. As a result, Kafka closes the connection.
This patch add the SASL handshake negotiation.

@sebgl
Copy link

sebgl commented Oct 17, 2016

Any news on that issue? We're not able to make SASL work with Sarama without this patch.
Thanks.

@wvanbergen
Copy link
Contributor

@sebgl are you running this patch in production?

@sebgl
Copy link

sebgl commented Oct 17, 2016

@wvanbergen We are. Since we recently switched our Kafka clusters to SASL_SSL, this patch is the only way we can use Sarama (and we deeply rely on it :) ).

Copy link
Contributor

@wvanbergen wvanbergen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know too much about SASL, but the go code looks pretty good except for one remark. @eapache ?

b.connErr = b.sendAndReceiveSASLPlainHandshake()
if b.connErr != nil {
Logger.Printf("Error while performing SASL handshake %s\n", b.addr)
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What error are we returning in this case? Should we set the err variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing it @wvanbergen

I might have miss something here. I guess that the connection should also be closed and the "b.opened" flag set to 0.

However as we are in a goroutine, I don't understand why the err variable should be set. We just stop the connection routine and the b.connErr is used to detect and error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be simpler just to call sendAndReceiveSASLPlainHandshake from sendAndReceiveSASLPlainAuth and re-use the existing error handling for that method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eapache indeed, it seems to be a good way of doing it 👍

@guillaumebreton guillaumebreton force-pushed the master branch 2 times, most recently from a0ddbc8 to 8561c1d Compare October 20, 2016 15:51
@Alkorin Alkorin mentioned this pull request Nov 7, 2016
@Alkorin
Copy link
Contributor

Alkorin commented Nov 7, 2016

This PR have a bug, more info in #781

This patch adds the mandatory SASL handshake for SASL negotiation.
@guillaumebreton
Copy link
Contributor Author

@Alkorin thanks for the heads up. Just patched the PR 👍

@eapache
Copy link
Contributor

eapache commented Nov 22, 2016

Sorry for the delay on this. I manually rebased, tweaked and merged this.

@eapache eapache closed this Nov 22, 2016
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.

5 participants