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.

Sign up for GitHub

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 basic authentication to Kafka storage #1983

Merged
merged 4 commits into from
Dec 31, 2019
Merged

Add basic authentication to Kafka storage #1983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43e8f5d
Configure Jager Collector to work with Kafka With Basic Authenticatio…
chandresh-pancholi Dec 18, 2019
9abe026
Merge remote-tracking branch 'upstream/master' into jaeger-1966
chandresh-pancholi Dec 19, 2019
6531292
Changing method name from addPlainTextFlag to addPlainTextFlags #1966
chandresh-pancholi Dec 23, 2019
73732d1
Merge remote-tracking branch 'upstream/master' into jaeger-1966
chandresh-pancholi Dec 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions pkg/kafka/auth/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ import (
)

const (
none = "none"
kerberos = "kerberos"
tls = "tls"
none = "none"
kerberos = "kerberos"
tls = "tls"
plaintext = "plaintext"
)

var authTypes = []string{
Expand All @@ -39,6 +40,7 @@ type AuthenticationConfig struct {
Authentication string
Kerberos KerberosConfig
TLS TLSConfig
PlainText PlainTextConfig
}

//SetConfiguration set configure authentication into sarama config structure
Expand All @@ -55,6 +57,9 @@ func (config *AuthenticationConfig) SetConfiguration(saramaConfig *sarama.Config
return nil
case tls:
return setTLSConfiguration(&config.TLS, saramaConfig)
case plaintext:
setPlainTextConfiguration(&config.PlainText, saramaConfig)
return nil
default:
return errors.Errorf("Unknown/Unsupported authentication method %s to kafka cluster.", config.Authentication)
}
Expand All @@ -74,4 +79,7 @@ func (config *AuthenticationConfig) InitFromViper(configPrefix string, v *viper.
config.TLS.CaPath = v.GetString(configPrefix + tlsPrefix + suffixTLSCA)
config.TLS.CertPath = v.GetString(configPrefix + tlsPrefix + suffixTLSCert)
config.TLS.KeyPath = v.GetString(configPrefix + tlsPrefix + suffixTLSKey)

config.PlainText.UserName = v.GetString(configPrefix + plainTextPrefix + suffixPlainTextUserName)
config.PlainText.Password = v.GetString(configPrefix + plainTextPrefix + suffixPlainTextPassword)
}
19 changes: 19 additions & 0 deletions pkg/kafka/auth/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ const (
defaultCAPath = ""
defaultCertPath = ""
defaultKeyPath = ""

plainTextPrefix = ".plaintext"
suffixPlainTextUserName = ".username"
suffixPlainTextPassword = ".password"

defaultPlainTextUserName = ""
defaultPlainTextPassword = ""
)

func addKerberosFlags(configPrefix string, flagSet *flag.FlagSet) {
Expand Down Expand Up @@ -99,6 +106,17 @@ func addTLSFlags(configPrefix string, flagSet *flag.FlagSet) {
"Path to the TLS Key for the Kafka connection")
}

func addPlainTextFlags(configPrefix string, flagSet *flag.FlagSet) {
flagSet.String(
configPrefix+plainTextPrefix+suffixPlainTextUserName,
defaultPlainTextUserName,
"The plaintext Username for SASL/PLAIN authentication")
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we call it basic like in other flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kafka and Sarama always mentioned it as Plaintext. Shall I change it to basic?
I can replace "plainText" to "basicAuth"

Copy link
Member

Choose a reason for hiding this comment

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

I'd keep it as plaintext to match Kafka documentation.

flagSet.String(
configPrefix+plainTextPrefix+suffixPlainTextPassword,
defaultPlainTextPassword,
"The plaintext Password for SASL/PLAIN authentication")
}

// AddFlags add configuration flags to a flagSet.
func AddFlags(configPrefix string, flagSet *flag.FlagSet) {
flagSet.String(
Expand All @@ -108,4 +126,5 @@ func AddFlags(configPrefix string, flagSet *flag.FlagSet) {
)
addKerberosFlags(configPrefix, flagSet)
addTLSFlags(configPrefix, flagSet)
addPlainTextFlags(configPrefix, flagSet)
}
31 changes: 31 additions & 0 deletions pkg/kafka/auth/plaintext.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Copyright (c) 2019 The Jaeger Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package auth

import (
"github.com/Shopify/sarama"
)

// PlainTextConfig describes the configuration properties needed for SASL/PLAIN with kafka
type PlainTextConfig struct {
UserName string
Password string
}

func setPlainTextConfiguration(config *PlainTextConfig, saramaConfig *sarama.Config) {
saramaConfig.Net.SASL.Enable = true
saramaConfig.Net.SASL.User = config.UserName
saramaConfig.Net.SASL.Password = config.Password
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't track. Kafka docs https://docs.confluent.io/current/kafka/authentication_sasl/authentication_sasl_plain.html mention "plaintext" as an alternative to SASL "plain", but here we seem to make them the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please correct me if I understood it, You are trying to say rather than using "PlainText" we shall use "Plain".

Copy link
Member

Choose a reason for hiding this comment

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

no, I am saying Kafka docs make clear distinction between those two methods, but in this code we seem to treat them as the same thing: calling it plaintext but using SASL structure to pass it. So which one is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

saramaconfig only has SASL struct. below is the code snippet.

// SASL based authentication with broker. While there are multiple SASL authentication methods
		// the current implementation is limited to plaintext (SASL/PLAIN) authentication
		SASL struct {
			// Whether or not to use SASL authentication when connecting to the broker
			// (defaults to false).
			Enable bool
			// SASLMechanism is the name of the enabled SASL mechanism.
			// Possible values: OAUTHBEARER, PLAIN (defaults to PLAIN).
			Mechanism SASLMechanism
			// Whether or not to send the Kafka SASL handshake first if enabled
			// (defaults to true). You should only set this to false if you're using
			// a non-Kafka SASL proxy.
			Handshake bool
			//username and password for SASL/PLAIN  or SASL/SCRAM authentication
			User     string
			Password string

Copy link
Member

Choose a reason for hiding this comment

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

well, then this is "plain", not "plaintext", because credentials are still sent over encrypted channel. From the docs:

PLAIN versus PLAINTEXT Do not confuse the SASL mechanism PLAIN with no SSL encryption being called PLAINTEXT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From Confluent slack channel:
None of the SASL mechanisms encrypt wire data not even GSSAPI. You have to enable TLS for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.confluent.io/current/kafka/authentication_ssl.html

By default, Apache Kafka® communicates in PLAINTEXT, which means that all data is sent in the clear. To encrypt communication, it is recommended to configure all the Confluent Platform components in your deployment to use SSL encryption.

}