SASL SCRAM support #465

spuun · 2017-11-02T09:51:46Z

Added a SASL SCRAM authenticator.

Instead of adding even more parameters to Client constructor it's now possible to inject an authenticator. This makes it possible to create custom authenticators without modifing ruby-kafka. To make this possible the mechanism validation in SaslHandshakeRequest had to be removed. Possible authentication mechanism is validated by the broker so this shouldn't be needed anyway.

It's also possible to inject a SSL context to the Client constructor.

dasch · 2017-11-02T10:32:40Z

I'd rather keep adding parameters to the initializer – this was a deliberate design choice that may make the code a bit harder to follow, but would be simpler for most users of the library.

dasch · 2017-11-02T10:36:16Z

lib/kafka/sasl_scram_authenticator.rb

+        raise FailedScramAuthentication, 'Invalid server signature' if response['v'] != server_signature
+      rescue FailedScramAuthentication
+        raise
+      rescue => e


I'd like to avoid catch-all rescues.

spuun · 2017-11-02T12:08:45Z

I've changed the code to use new parameters in the initializer. I also put back the validation in SaslHandshakeRequest.

dasch · 2017-11-02T12:15:15Z

README.md

+```ruby
+kafka = Kafka.new(
+  sasl_scram_username: 'username',
+  sasl_scram_password: 'password,


Missing an end quite there.

Oh! Forgot to validate docs..

dasch · 2017-11-02T12:36:06Z

README.md

+kafka = Kafka.new(
+  sasl_scram_username: 'username',
+  sasl_scram_password: 'password,
+  sasl_scram_mechanism: Kafka::SCRAM_SHA256,


I'd prefer a String name of the mechanism, e.g. "sha256", as that will make it a lot easier to integrate with config files and the like.

Ok. I can change that. Do you prefer "sha256" over "SHA-256" (the current value of the constant)?

I prefer "sha256".

dasch · 2017-11-02T12:36:28Z

lib/kafka.rb

@@ -225,6 +225,15 @@ class OffsetCommitError < Error
  class FetchError < Error
  end

+  class NoPartitionsAssignedError < Error
+  end


How did this sneak in here?

I have no idea... Maybe some merge i missed.

dasch · 2017-11-02T12:37:18Z

lib/kafka/sasl_gssapi_authenticator.rb

@@ -3,7 +3,7 @@ class SaslGssapiAuthenticator
    GSSAPI_IDENT = "GSSAPI"
    GSSAPI_CONFIDENTIALITY = false

-    def initialize(connection:, logger:, sasl_gssapi_principal:, sasl_gssapi_keytab:)
+    def initialize(conncetion:, logger:, sasl_gssapi_principal:, sasl_gssapi_keytab:)


dasch · 2017-11-02T12:38:25Z

lib/kafka/sasl_scram_authenticator.rb

+
+      begin
+        msg = first_message
+        log_debug "[scram] Sending client's first message: #{msg}"


There's currently no precedent for "tagging" log lines with e.g. [scram]. Could you instead do Sending first SCRAM client message? The same for the rest of the log lines.

dasch · 2017-11-03T13:20:17Z

Looking good! Let me know when you need another round of review.

It must be possible to implement new sasl types without having to modify ruby-kafka. Made authenticator parameter name more generic (remove sasl_)

spuun · 2017-11-03T18:39:21Z

I've changed the mechanism configuration parameters to be 'sha256' and 'sha512'.

dasch · 2017-11-06T10:20:58Z

lib/kafka/client.rb

+    #
+    # @param sasl_scram_mechanism [String, nil] Scram mechanism ("sha256", "sha512")
+    #
+    # @param use_ssl [Booleanm false] Use SSL


Why add this parameter? The decision on whether to use SSL is already done here:

ruby-kafka/lib/kafka/client.rb

Lines 528 to 529 in a0e1d14

def build_ssl_context(ca_cert_file_path, ca_cert, client_cert, client_cert_key)

return nil unless ca_cert_file_path || ca_cert || client_cert || client_cert_key

dasch · 2017-11-06T10:21:37Z

lib/kafka/client.rb


-      sasl_authenticator = SaslAuthenticator.new(
+      sasl_authenticator ||= SaslAuthenticator.new(


The || can be removed.

dasch · 2017-11-06T10:23:13Z

spec/fake_server.rb

-      client_id = request_decoder.string
+    loop do
+      request_bytes = decoder.bytes
+      request_data = Kafka::Protocol::Decoder.new(StringIO.new(request_bytes));


No reason for a semicolon here.

dasch · 2017-11-06T10:23:50Z

spec/fake_server.rb

+      api_key = request_data.int16
+      _api_version = request_data.int16
+      correlation_id = request_data.int32
+      _client_id = request_data.string


No reason for underscores here.

dasch · 2017-11-06T10:24:23Z

spec/fake_server.rb

-        @authenticating = true
-        @auth_mechanism = message
+      case api_key
+      when 17 then


No reason to switch to a case statement here.

dasch · 2017-11-08T10:53:34Z

Thanks for the hard work! ❤️

dasch · 2017-11-09T13:46:49Z

@spuun I've released v0.5.1.beta1 with this functionality – would you be able to deploy and test that version?

spuun · 2017-11-14T10:29:47Z

I've tested beta2 and it seems to work just fine! 👍

dasch · 2017-11-14T10:38:47Z

@spuun great, thanks!

dasch reviewed Nov 2, 2017

View reviewed changes

spuun added 15 commits November 3, 2017 19:29

Don't validate sasl type

caecb0d

It must be possible to implement new sasl types without having to modify ruby-kafka. Made authenticator parameter name more generic (remove sasl_)

Added sasl scram authenticator

984d451

Pass connection to authenticate! instead of constructor

a54e9bb

No need for connection to be instance variable

14749ff

Added specs for scram

1a3112c

Documented scram

0be2562

Code formatting and clean up

8b3e751

Don't catch all errors

e08cfc4

Refactorized to use parameters instead of DI

062c117

Added missing quote to code example

9fb20e1

Removed unused exception

216e5cf

Fixed typo

16138a8

Changed log message

d954935

Changed scram mechanism parameter values

4f00aa6

Updated scram code example

ce5c3bd

dasch reviewed Nov 6, 2017

View reviewed changes

spuun added 2 commits November 6, 2017 12:02

Removed unwanted and unnecessary characters

a6233f4

Changed case to if and removed underscore from unused variables

609b840

dasch merged commit 7d1618e into zendesk:master Nov 8, 2017

	def build_ssl_context(ca_cert_file_path, ca_cert, client_cert, client_cert_key)
	return nil unless ca_cert_file_path \|\| ca_cert \|\| client_cert \|\| client_cert_key


		sasl_authenticator = SaslAuthenticator.new(
		sasl_authenticator \|\|= SaslAuthenticator.new(

SASL SCRAM support #465

SASL SCRAM support #465

Uh oh!

Conversation

spuun commented Nov 2, 2017

Uh oh!

dasch commented Nov 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spuun commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dasch commented Nov 3, 2017

Uh oh!

spuun commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dasch commented Nov 8, 2017

Uh oh!

dasch commented Nov 9, 2017

Uh oh!

spuun commented Nov 14, 2017

Uh oh!

dasch commented Nov 14, 2017

Uh oh!

Uh oh!

spuun commented Nov 2, 2017 •

edited

Loading