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 support for Kafka SASL/PLAIN authentication via SCRAM-SHA-256 or SCRAM-SHA-512 mechanism #2724

Merged
merged 5 commits into from
Jan 14, 2021
Merged

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

merged 5 commits into from
Jan 14, 2021

Conversation

WalkerWang731
Copy link
Contributor

@WalkerWang731 WalkerWang731 commented Jan 12, 2021

Which problem is this PR solving?

Short description of the changes

  • The smallest modification on the current version.
  • Different from the previous PR WIP - Adding scram authentication for kafka #2110, I just modified pkg/kafka/auth/plaintext.go , pkg/kafka/auth/options.go and pkg/kafka/auth/config.go, and not add any new file.
  • More clearly for option name, using option name is mechanism (reference from Kafka parameter name rules )
  • About the Prefix, I think to keep using plaintext better than scarm, because these are both belong to SASL, in the future, it can change to sasl if needed. Actually, using plaintext is ok, it will not be confusing, because it name is SASL_PLAINTEXT that from Kafka official security protocol parameter name
  • About the parameters limit of mechanism, I followed Kafka name rules (SCRAM-SHA-256, SCRAM-SHA-512, PLAIN)
  • I reserved the current version plaintext mechanism, and add to default. If users do not specify the mechanism, it not affect continued use. if users use the mechanism then just only add --kafka.ROLE.plaintext.mechanism parameter then can use it.

Changelog

Add that support Kafka SASL/PLAIN authentication of SCRAM-SHA-256 or SCRAM-SHA-512 mechanism

Parameter config of collector (SPAN_STORAGE_TYPE=kafka)

--kafka.producer.plaintext.mechanism string      The plaintext Mechanism for SASL/PLAIN authentication, e.g. 'SCRAM-SHA-256' or 'SCRAM-SHA-512' or 'PLAIN' (default "PLAIN")

Parameter config of ingester

--kafka.consumer.plaintext.mechanism string      The plaintext Mechanism for SASL/PLAIN authentication, e.g. 'SCRAM-SHA-256' or 'SCRAM-SHA-512' or 'PLAIN' (default "PLAIN")

@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #2724 (5d19011) into master (2ffdaae) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2724      +/-   ##
==========================================
+ Coverage   95.73%   95.76%   +0.03%     
==========================================
  Files         217      217              
  Lines        9619     9619              
==========================================
+ Hits         9209     9212       +3     
+ Misses        337      336       -1     
+ Partials       73       71       -2     
Impacted Files Coverage Δ
cmd/collector/app/server/zipkin.go 73.07% <0.00%> (-3.85%) ⬇️
cmd/query/app/static_handler.go 98.26% <0.00%> (+3.47%) ⬆️

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 2ffdaae...5d19011. Read the comment docs.

go.mod Show resolved Hide resolved
pkg/kafka/auth/plaintext.go Outdated Show resolved Hide resolved
plugin/storage/kafka/factory_test.go Outdated Show resolved Hide resolved
pkg/kafka/auth/plaintext.go Outdated Show resolved Hide resolved
// PlainTextConfig describes the configuration properties needed for SASL/PLAIN with kafka
type PlainTextConfig struct {
UserName string `mapstructure:"username"`
Password string `mapstructure:"password" json:"-"`
UserName string `mapstructure:"username"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
UserName string `mapstructure:"username"`
Username string `mapstructure:"username"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I will update later.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM

@yurishkuro yurishkuro changed the title Add that support Kafka SASL/PLAIN authentication of SCRAM-SHA-256 or SCRAM-SHA-512 mechanism Add support for Kafka SASL/PLAIN authentication via SCRAM-SHA-256 or SCRAM-SHA-512 mechanism Jan 14, 2021
@yurishkuro yurishkuro merged commit 8fb235a into jaegertracing:master Jan 14, 2021
bhiravabhatla pushed a commit to bhiravabhatla/jaeger that referenced this pull request Jan 25, 2021
…SCRAM-SHA-512 mechanism (jaegertracing#2724)

* add that suppot Kafka SASL/PLAIN authentication of SCRAM-SHA-256 or SCRAM-SHA-512 mechanism

Signed-off-by: WalkerWang731 <[email protected]>

* rename XDGSCRAMClient to scramClient and remove paramater that no point on factory_test.go

Signed-off-by: WalkerWang731 <[email protected]>

* add type assertion

Signed-off-by: WalkerWang731 <[email protected]>

* replacement UserName to Username

Signed-off-by: WalkerWang731 <[email protected]>
@Neustradamus
Copy link

@WalkerWang731 and all others: Thanks a lot for all about SCRAM.

Linked to:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants