WIP - Adding scram authentication for kafka #2110

TheDavidKruse · 2020-03-02T20:37:05Z

Co-authored by Zenobius Selvadhason [email protected]

Signed-off-by: david kruse [email protected]

Which problem is this PR solving?

Resolves #2046 SCRAM authentication between jaeger and kafka

Short description of the changes

Added CLI flags to pass in algorithm (SHA), tls config, username, password, etc
Added package xdg-go/scram package to manage handshake
Added scram.go to the kafka auth directory

This is a near drop-in from the sarama scram configuration they listed in their examples.

yurishkuro

please make sure to add unit tests

pkg/kafka/auth/scram.go

pkg/kafka/auth/options.go

pkg/kafka/auth/scram.go

yurishkuro · 2020-04-09T22:39:21Z

pkg/kafka/auth/scram.go

+		saramaConfig.Net.SASL.SCRAMClientGeneratorFunc = func() sarama.SCRAMClient { return &SCRAMClient{HashGeneratorFcn: SHA256} }
+		saramaConfig.Net.SASL.Mechanism = sarama.SASLMechanism(sarama.SASLTypeSCRAMSHA256)
+	} else {
+		errors.Errorf("invalid SHA algorithm '%s': can be either 'sha256' or 'sha512'", config.algorithm)


missing return? and consequently, missing unit test for it?

I added a unit test with instantiating the scramConfig itself but adding initFromViper test soon if I can get it working.

pkg/kafka/auth/options.go

codecov · 2020-04-10T21:48:43Z

Codecov Report

Merging #2110 (8b2ef89) into master (2f8ffa9) will increase coverage by 0.40%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2110      +/-   ##
==========================================
+ Coverage   95.73%   96.14%   +0.40%     
==========================================
  Files         216      219       +3     
  Lines        9599    10585     +986     
==========================================
+ Hits         9190    10177     +987     
- Misses        336      351      +15     
+ Partials       73       57      -16

Impacted Files	Coverage Δ
cmd/collector/app/server/http.go	`0.00% <0.00%> (-88.00%)`	⬇️
cmd/collector/app/server/zipkin.go	`0.00% <0.00%> (-76.93%)`	⬇️
pkg/netutils/port.go	`50.00% <0.00%> (-50.00%)`	⬇️
cmd/query/app/static_handler.go	`86.84% <0.00%> (-7.95%)`	⬇️
cmd/collector/app/collector.go	`69.11% <0.00%> (-5.54%)`	⬇️
plugin/storage/es/dependencystore/storage.go	`85.71% <0.00%> (-5.40%)`	⬇️
plugin/storage/grpc/shared/grpc_server.go	`76.47% <0.00%> (-4.98%)`	⬇️
cmd/flags/flags.go	`43.47% <0.00%> (-3.90%)`	⬇️
plugin/storage/grpc/shared/grpc_client.go	`85.96% <0.00%> (-0.65%)`	⬇️
plugin/storage/kafka/options.go	`93.68% <0.00%> (-0.07%)`	⬇️
... and 224 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc091c5...48a9ead. Read the comment docs.

pkg/kafka/auth/scram.go

yurishkuro · 2020-04-10T23:46:39Z

pkg/kafka/auth/scram.go

+
+	"github.com/Shopify/sarama"
+	"github.com/pkg/errors"
+	SCRAM "github.com/xdg/scram"


Package name should be in lower-case. There's no need to rename it.

"scram" is being used in the config file as a string constant for the AuthenticationConfig, I would need to change the constant name or change the name of the import of the package.

but it's in a different file, why is it a problem?

I believe because the constant is package scoped and so it conflicts with the standard import name of "scram". Would something like "scrampkg" be better?

maybe scrumclient

I believe that scrampkg may be better to define that it's a package, or something like xdgscram as it's the name of the import of the package github.com/xdg/scram.

I feel as though scrumclient or scramclient would be confusing as there's a struct named scramClient and scrum just seems mispelled. Those are my thoughts about those.

pkg/kafka/auth/scram.go

pkg/kafka/auth/options.go

pkg/kafka/auth/scram.go

yurishkuro · 2020-04-11T00:12:40Z

pkg/kafka/auth/scram.go

+type SCRAMClient struct {
+	*SCRAM.Client
+	*SCRAM.ClientConversation
+	SCRAM.HashGeneratorFcn


Is there any need for embedding these? If not, it's better to give them private field names, because otherwise all their methods are leaking into the scramClient API.

Afaik they're based in the interface for the sarama config to begin using Scram and need to be embedded for the Begin, Step and Done. Let me check and I'll reply to this comment.

yurishkuro

please make sure that all commits are signed (see https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md#missing-sign-offs)

pkg/kafka/auth/scram.go

go.mod

Signed-off-by: david kruse <[email protected]>

yurishkuro · 2020-05-11T21:08:48Z

go.mod

@@ -52,7 +52,7 @@ require (
 	github.com/opentracing/opentracing-go v1.1.0
 	github.com/pelletier/go-toml v1.6.0 // indirect
 	github.com/pierrec/lz4 v2.4.1+incompatible // indirect
-	github.com/pkg/errors v0.9.1 // indirect
+	github.com/pkg/errors v0.9.1


you still have places where errors pkg is introduced, that's why it's moving to direct dependency. Please use fmt.Errorf

Signed-off-by: david kruse <[email protected]>

yurishkuro · 2020-05-11T21:37:19Z

Please make sure to run make test, several tests are currently failing with your change.

jpkrohling · 2020-09-10T10:44:40Z

What's the status of this PR, other than the conflicts?

Signed-off-by: Yuri Shkuro <[email protected]>

jpkrohling

Is this related to the similar PR from OpenTelemetry Collector? open-telemetry/opentelemetry-collector#2322

jpkrohling · 2021-01-12T10:02:25Z

pkg/kafka/auth/options.go

@@ -49,6 +49,14 @@ const (

 	defaultPlainTextUserName = ""
 	defaultPlainTextPassword = ""
+
+	// Scram configuration options


Not a huge fan of the convention, but Go rules state that acronyms should be all in caps, so, SCRAM everywhere in the next few lines.

jpkrohling · 2021-01-12T10:02:44Z

pkg/kafka/auth/options.go

+	flagSet.String(
+		configPrefix+scramPrefix+suffixScramUserName,
+		"",
+		"Scram username used to authenticate with the client")


Here as well

jpkrohling · 2021-01-12T10:03:14Z

pkg/kafka/auth/scram.go

+	Algorithm string `mapstructure:"algorithm"`
+}
+
+// SetSCRAMConfiguration ...


jpkrohling · 2021-01-12T10:04:50Z

pkg/kafka/auth/scram.go

+
+	var mechanism sarama.SASLMechanism
+
+	switch config.Algorithm {


Matter of taste I guess, but I think the version from this PR more readable: open-telemetry/opentelemetry-collector#2322

jpkrohling · 2021-01-12T10:06:03Z

pkg/kafka/auth/scram.go

+	return nil
+}
+
+type scramClient struct {


Same comment I had in the similar PR from OpenTelemetry: you should probably do a type assert here, ensuring that this is a sarama.SCRAMClient

jpkrohling · 2021-01-12T10:06:54Z

pkg/kafka/auth/scram_test.go

+	}, authCfg.SCRAM)
+}
+
+// testing Begin, Step, and Done require a network connection to test,


Is it hard to mock this?

Neustradamus · 2022-01-11T04:39:19Z

The merged PR is:

Add support for Kafka SASL/PLAIN authentication via SCRAM-SHA-256 or SCRAM-SHA-512 mechanism #2724

TheDavidKruse requested a review from a team as a code owner March 2, 2020 20:37

yurishkuro reviewed Mar 2, 2020

View reviewed changes

TheDavidKruse changed the title ~~Adding scram authentication for kafka~~ WIP - Adding scram authentication for kafka Apr 9, 2020

yurishkuro reviewed Apr 9, 2020

View reviewed changes

pkg/kafka/auth/options.go Outdated Show resolved Hide resolved

TheDavidKruse force-pushed the kafka-scram branch from 8b2ef89 to 17b5348 Compare April 10, 2020 21:34

yurishkuro reviewed Apr 11, 2020

View reviewed changes

yurishkuro reviewed May 10, 2020

View reviewed changes

pkg/kafka/auth/scram.go Outdated Show resolved Hide resolved

pkg/kafka/auth/scram.go Outdated Show resolved Hide resolved

pkg/kafka/auth/scram.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

moved SHA512 func to inline. Changed func based on feedback

bc9e1b1

Signed-off-by: david kruse <[email protected]>

TheDavidKruse force-pushed the kafka-scram branch from 4101e43 to bc9e1b1 Compare May 11, 2020 19:55

changes based on feedback

8c7033d

Signed-off-by: david kruse <[email protected]>

yurishkuro reviewed May 11, 2020

View reviewed changes

removing errors package

5ec140a

Signed-off-by: david kruse <[email protected]>

yurishkuro and others added 2 commits January 12, 2021 02:01

Merge branch 'master' into kafka-scram

4cb4263

go mod tidy

48a9ead

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro mentioned this pull request Jan 12, 2021

Not support Kafka SASL/PLAIN authentication of SCRAM-SHA-256 or SCRAM-SHA-512 mechanism #2722

Closed

WalkerWang731 mentioned this pull request Jan 12, 2021

[Closed & Outmoded]Add that support Kafka SASL/PLAIN authentication of SCRAM-SHA-256 or SCRAM-SHA-512 mechanism #2723

Closed

jpkrohling reviewed Jan 12, 2021

View reviewed changes

WalkerWang731 mentioned this pull request Jan 12, 2021

Add support for Kafka SASL/PLAIN authentication via SCRAM-SHA-256 or SCRAM-SHA-512 mechanism #2724

Merged

yurishkuro closed this in #2724 Jan 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP - Adding scram authentication for kafka #2110

WIP - Adding scram authentication for kafka #2110

TheDavidKruse commented Mar 2, 2020

yurishkuro left a comment

yurishkuro Apr 9, 2020

TheDavidKruse May 8, 2020

codecov bot commented Apr 10, 2020 •

edited

Loading

yurishkuro Apr 10, 2020

TheDavidKruse May 8, 2020

yurishkuro May 8, 2020

TheDavidKruse May 8, 2020

yurishkuro May 8, 2020

TheDavidKruse May 8, 2020

yurishkuro Apr 11, 2020

TheDavidKruse May 8, 2020

yurishkuro left a comment

yurishkuro May 11, 2020

TheDavidKruse May 11, 2020

yurishkuro commented May 11, 2020

jpkrohling commented Sep 10, 2020

jpkrohling left a comment

jpkrohling Jan 12, 2021

jpkrohling Jan 12, 2021

jpkrohling Jan 12, 2021

jpkrohling Jan 12, 2021

jpkrohling Jan 12, 2021

jpkrohling Jan 12, 2021

Neustradamus commented Jan 11, 2022

WIP - Adding scram authentication for kafka #2110

WIP - Adding scram authentication for kafka #2110

Conversation

TheDavidKruse commented Mar 2, 2020

Which problem is this PR solving?

Short description of the changes

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 10, 2020 • edited Loading

Codecov Report

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

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented May 11, 2020

jpkrohling commented Sep 10, 2020

jpkrohling 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

Neustradamus commented Jan 11, 2022

codecov bot commented Apr 10, 2020 •

edited

Loading