feat: kafka sasl auth #1186

joshuajorel · 2021-04-22T13:34:21Z

Checklist:

Fixes SASL authentication to Kafka #703

Test Cases:

Test Invalid Credentials (EventSource and Trigger)
Test Invalid SASL Mechanism (EventSource and Trigger)
Test SASL PLAIN Mechanism

Notes:

sarama library says that it only supports SASL/PLAIN, but has different mechanisms available in its codebase. I added the other authentication mechanisms just in case
This PR was only tested with the SASL PLAIN mechanism

Signed-off-by: Joshua Jorel Lee <[email protected]>

whynowy · 2021-04-22T20:58:50Z

pkg/apis/common/common.go

+	// SASLMechanism is the name of the enabled SASL mechanism.
+	// Possible values: OAUTHBEARER, PLAIN (defaults to PLAIN).
+	Mechanism string `json:"mechanism,omitempty" protobuf:"bytes,1,opt,name=mechanism"`
+	// Version is the SASL Protocol Version to use


Comments do not match the fields...

Removed comments.

whynowy · 2021-04-22T20:59:12Z

pkg/apis/common/common.go

+	User *corev1.SecretKeySelector `json:"user,omitempty" protobuf:"bytes,2,opt,name=user"`
+	// Password for SASL/PLAIN authentication
+	Password *corev1.SecretKeySelector `json:"password,omitempty" protobuf:"bytes,3,opt,name=password"`
+	// authz id used for SASL/SCRAM authentication


Where is authz id?

Remove authz id. It wasn't supposed to be in there. Comments were lifted from the sarama library.

whynowy · 2021-04-22T21:00:29Z

pkg/apis/common/validate.go

+
+	var mechanismSet, userSet, passwordSet bool
+
+	if saslConfig.Mechanism != "" {


Does not need !="" check

Removed !="" check

whynowy · 2021-04-22T21:09:41Z

pkg/apis/common/validate.go

+	}
+
+	if !mechanismSet {
+		return errors.New("invalid sasl config, please configure Mechanism. Possible values are `OAUTHBEARER`, `PLAIN`, `SCRAM-SHA-256`, `SCRAM-SHA-512` and `GSSAPI`")


It says defaults to PLAIN above.

Renamed error message to account for the only possible values.

whynowy · 2021-04-22T21:14:15Z

pkg/apis/common/validate.go

+		passwordSet = true
+	}
+
+	if !userSet && !passwordSet {


Can either of them be missing?

This should have been an OR statement. Will fix.

whynowy · 2021-04-22T21:19:09Z

sensors/triggers/kafka/kafka.go

@@ -63,6 +63,24 @@ func NewKafkaTrigger(sensor *v1alpha1.Sensor, trigger *v1alpha1.Trigger, kafkaPr
 			config.Version = version
 		}

+		if kafkatrigger.SASL != nil {
+			config.Net.SASL.Enable = true
+			config.Net.SASL.Mechanism = sarama.SASLMechanism(kafkatrigger.SASL.Mechanism)


Validating?

Checks whether the configuration is set. Enables SASL authentication if parameters are set in the YAML configs.

Added additional validation for empty strings.

whynowy · 2021-04-22T21:54:58Z

pkg/apis/eventsource/v1alpha1/types.go

@@ -461,28 +461,31 @@ type KafkaEventSource struct {
 	Topic string `json:"topic" protobuf:"bytes,3,opt,name=topic"`
 	// Backoff holds parameters applied to connection.
 	ConnectionBackoff *apicommon.Backoff `json:"connectionBackoff,omitempty" protobuf:"bytes,4,opt,name=connectionBackoff"`
+	// SASL configuration for the kafka client


BTW - we should keep the order of existing fields, otherwise it's a breaking change for proto files, we should reduce that kind of changes.

https://developers.google.com/protocol-buffers/docs/pythontutorial#extending-a-protocol-buffer

Reverted the byte ordering and added the SASL configs to the end.

Signed-off-by: Joshua Jorel Lee <[email protected]>

…passwordset logic. Signed-off-by: Joshua Jorel Lee <[email protected]>

Signed-off-by: Joshua Jorel Lee <[email protected]>

whynowy · 2021-04-23T17:57:53Z

pkg/apis/common/validate.go

+		return nil
+	}
+
+	var mechanismSet, userSet, passwordSet bool


Kind of verbose.

switch saslConfig.GetMechanism() { case "PLAIN", xxxxxx: default: return errors.New("invalid sasl config. Possible xxxxxx") } if saslConfig.User == nil { return errors.New("xxx") } if saslConfig.Password == nil { return errors.New("xxx") } return nil

Removed xxxSet flags. merged validating user and password into a single if statemetn.

whynowy · 2021-04-23T17:59:10Z

pkg/apis/sensor/v1alpha1/types.go

@@ -499,20 +499,24 @@ type KafkaTrigger struct {
 	// Defaults to 500 milliseconds.
 	// +optional
 	FlushFrequency int32 `json:"flushFrequency,omitempty" protobuf:"varint,7,opt,name=flushFrequency"`
+	// SASL configuration for the kafka client


Same here, do not change the order.

Reverted order.

whynowy · 2021-04-23T18:07:48Z

eventsources/sources/kafka/start.go

+	if kafkaEventSource.SASL != nil {
+		config.Net.SASL.Enable = true
+
+		if kafkaEventSource.SASL.Mechanism == "" {


A function func (s SASLConfig) GetMechanism() would be helpful to save if..else everywhere.

Will refactor.

Refactored both sensor and event source kafka configs. Function GetMechanism() will by default, will set mechanism to "PLAINTEXT" unless otherwise specified.

Signed-off-by: Joshua Jorel Lee <[email protected]>

whynowy · 2021-04-30T03:00:10Z

pkg/apis/common/common.go

@@ -153,3 +167,13 @@ type Metadata struct {
 	Annotations map[string]string `json:"annotations,omitempty" protobuf:"bytes,1,rep,name=annotations"`
 	Labels      map[string]string `json:"labels,omitempty" protobuf:"bytes,2,rep,name=labels"`
 }
+
+func (s SASLConfig) GetMechanism() sarama.SASLMechanism {


Overall everything looks good to me except this part.

Returning sarama.SASLMechanism makes github.com/Shopify/sarama get introduced to the models, which is not good. Ideally the dependencies in models are either from native GoLang or k8s related. I prefer to return a string here.

Another reason is, since this struct is put in common, it is intended to be used by not only Kafka. Assume it's going to be used by something else, sarma.SASLMechanism might not be recognized by it. Also, I made a little research on SASL auth, it looks like the mechanism is not unified, different product has different support (PLAIN might be the one everyone supports). Correct me if I'm wrong.

Apologies for the oversight. Removed the type reference and refactored again to reflect on the actual sensor and eventsource.

I'm basing the authentication mechanism on the available library which is Sarama. The other authentication mechanisms might be something to look into in the future, but "PLAIN" auth is a good place to start.

Signed-off-by: Joshua Jorel Lee <[email protected]>

whynowy

LGTM, thanks!

* feat: SASL Auth for Kafka Signed-off-by: Joshua Jorel Lee <[email protected]>

joshuajorel added 6 commits April 22, 2021 21:12

feat: add sasl config definition

1f18505

Signed-off-by: Joshua Jorel Lee <[email protected]>

feat: add sasl auth to kafka eventsource

543194d

Signed-off-by: Joshua Jorel Lee <[email protected]>

test: add sasl validation tests

71776e9

Signed-off-by: Joshua Jorel Lee <[email protected]>

feat: add sasl auth to kafka sensor

244e31a

Signed-off-by: Joshua Jorel Lee <[email protected]>

test: added kafka trigger unit test

9b8ad2f

Signed-off-by: Joshua Jorel Lee <[email protected]>

chore: add autogenerated files

5566e10

Signed-off-by: Joshua Jorel Lee <[email protected]>

whynowy reviewed Apr 22, 2021

View reviewed changes

joshuajorel added 7 commits April 23, 2021 15:29

feat: add validation to sarama mechanism.

5201e2c

Signed-off-by: Joshua Jorel Lee <[email protected]>

fix: made sasl mechanism as an optional argument

9210e92

Signed-off-by: Joshua Jorel Lee <[email protected]>

fix: reverted field ordering.

0e377b9

Signed-off-by: Joshua Jorel Lee <[email protected]>

fix: removed if statement. modified error message. fixed userset and …

c02fc33

…passwordset logic. Signed-off-by: Joshua Jorel Lee <[email protected]>

chore: rebase from master.

0969441

Signed-off-by: Joshua Jorel Lee <[email protected]>

test: fixed test case for password and user in sasl config.

f83f84d

Signed-off-by: Joshua Jorel Lee <[email protected]>

chore: update codegen.

17cbfc4

Signed-off-by: Joshua Jorel Lee <[email protected]>

whynowy reviewed Apr 23, 2021

View reviewed changes

joshuajorel added 6 commits April 29, 2021 17:33

chore: merged master

571569e

Signed-off-by: Joshua Jorel Lee <[email protected]>

fix: reverted byte ordering for protobuf

b072a95

Signed-off-by: Joshua Jorel Lee <[email protected]>

refactor: added GetMechanism function for SASLConfig

f08d09f

Signed-off-by: Joshua Jorel Lee <[email protected]>

refactor: reduced lines for validating SASLConfig

4bee62d

Signed-off-by: Joshua Jorel Lee <[email protected]>

refactor: used GetMechanism function for setting SASL mechanism.

ec5b99e

Signed-off-by: Joshua Jorel Lee <[email protected]>

build: updated codegen

a3d7e3c

Signed-off-by: Joshua Jorel Lee <[email protected]>

whynowy reviewed Apr 30, 2021

View reviewed changes

joshuajorel added 2 commits April 30, 2021 18:54

refactor: GetMechanism() returns string

1c3a84c

Signed-off-by: Joshua Jorel Lee <[email protected]>

refactor: removed repeat reference to PLAIN

9e364a4

Signed-off-by: Joshua Jorel Lee <[email protected]>

whynowy approved these changes Apr 30, 2021

View reviewed changes

whynowy merged commit 41027b5 into argoproj:master Apr 30, 2021

juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022

feat: kafka sasl auth (argoproj#1186)

6c88e65

* feat: SASL Auth for Kafka Signed-off-by: Joshua Jorel Lee <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: kafka sasl auth #1186

feat: kafka sasl auth #1186

joshuajorel commented Apr 22, 2021

whynowy Apr 22, 2021

joshuajorel Apr 23, 2021

whynowy Apr 22, 2021

joshuajorel Apr 23, 2021

whynowy Apr 22, 2021

joshuajorel Apr 23, 2021

whynowy Apr 22, 2021

joshuajorel Apr 23, 2021

whynowy Apr 22, 2021

joshuajorel Apr 23, 2021

whynowy Apr 22, 2021

joshuajorel Apr 23, 2021

joshuajorel Apr 23, 2021

whynowy Apr 22, 2021

joshuajorel Apr 23, 2021

whynowy Apr 23, 2021

joshuajorel Apr 29, 2021

whynowy Apr 23, 2021

joshuajorel Apr 29, 2021

whynowy Apr 23, 2021

joshuajorel Apr 28, 2021

joshuajorel Apr 29, 2021 •

edited

Loading

whynowy Apr 30, 2021

joshuajorel Apr 30, 2021

whynowy left a comment


		var mechanismSet, userSet, passwordSet bool

		if saslConfig.Mechanism != "" {

feat: kafka sasl auth #1186

feat: kafka sasl auth #1186

Conversation

joshuajorel commented Apr 22, 2021

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

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

joshuajorel Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whynowy left a comment

Choose a reason for hiding this comment

joshuajorel Apr 29, 2021 •

edited

Loading