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

Jump to bottom

[fix] Enable Kafka TLS when TLS auth is specified #2107

Merged
merged 7 commits into from
Mar 5, 2020
Merged

[fix] Enable Kafka TLS when TLS auth is specified #2107

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0042cd3
[fix] Enable Kafka TLS when TLS auth is specified
pavolloffay Mar 2, 2020
5b48422
Use TLS when tls.enabled=true
pavolloffay Mar 2, 2020
4e170e9
Log deprecation message
pavolloffay Mar 3, 2020
5dc88cb
Fix fmt and lint
pavolloffay Mar 3, 2020
4c760ad
Remove deprecated TLS enabled flag
pavolloffay Mar 3, 2020
7e8c615
Review comments
pavolloffay Mar 4, 2020
1f1a6f6
Add tls case back
pavolloffay Mar 5, 2020
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
43 changes: 43 additions & 0 deletions cmd/ingester/app/flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,16 @@
package app

import (
"fmt"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/config/tlscfg"
"github.com/jaegertracing/jaeger/pkg/kafka/auth"
"github.com/jaegertracing/jaeger/plugin/storage/kafka"
)

Expand Down Expand Up @@ -49,6 +53,45 @@ func TestOptionsWithFlags(t *testing.T) {
assert.Equal(t, kafka.EncodingJSON, o.Encoding)
}

func TestTLSFlags(t *testing.T) {
tests := []struct {
flags []string
expected auth.AuthenticationConfig
}{
{
flags: []string{},
expected: auth.AuthenticationConfig{Authentication: "none", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}},
pavolloffay marked this conversation as resolved.
Show resolved Hide resolved
},
{
flags: []string{"--kafka.consumer.authentication=foo"},
expected: auth.AuthenticationConfig{Authentication: "foo", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}},
},
{
flags: []string{"--kafka.consumer.authentication=kerberos", "--kafka.consumer.tls.enabled=true"},
expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}, TLS: tlscfg.Options{Enabled: true}},
Copy link
Member

Choose a reason for hiding this comment

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

why is Authentication: "tls" here?

},
{
flags: []string{"--kafka.consumer.authentication=tls"},
expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}, TLS: tlscfg.Options{Enabled: true}},
},
{
flags: []string{"--kafka.consumer.authentication=tls", "--kafka.consumer.tls.enabled=false"},
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that if someone explicitly set tls.enabled to false, this will be silently changed to true`. I have nothing against that, but perhaps there's a way to detect whether this value was explicitly provided and keep whatever the user set?

Copy link
Member Author

Choose a reason for hiding this comment

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

it can be done by changing the enabled property to a pointer. I am not sure if there is much value in it. Some signatures will have to be also changed to accept the logger.

More controversial is this setting

// --kafka.consumer.authentication=kerberos",
--kafka.consumer.tls.enabled=true"

Somebody omitting the auth type and specifying tls.enabled=true.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yurishkuro any thoughts on this? I don't have a strong opinion whether kafka.consumer.tls.enabled=true should set kafka.consumer.authentication=tls.

Copy link
Contributor

@jpkrohling jpkrohling Mar 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

From Gitter, we just saw a case where a user wants TLS encryption between the agent and collector without the auth parts. kafka.producer.tls.enabled=true + kafka.producer.authentication=none is a valid combination.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, it's confusing for people to have both kafka.producer.tls.enabled and kafka.producer.authentication they will probably tend to forget the second one.

I have updated the PR to set tls auth when .tls.enabled is true.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have updated the PR to set tls auth when .tls.enabled is true.

It's the opposite. tls.enabled does not imply authentication=tls. It's perfectly possible to have authentication=none but the traffic be encrypted using TLS.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jpkrohling I am not sure if I understand you...

It's perfectly possible to have authentication=none but the traffic be encrypted using TLS.

This is exactly what the PR does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I misunderstood you, you are absolutely correct :-)

expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}, TLS: tlscfg.Options{Enabled: true}},
},
}

for _, test := range tests {
t.Run(fmt.Sprintf("%s", test.flags), func(t *testing.T) {
o := &Options{}
v, command := config.Viperize(AddFlags)
err := command.ParseFlags(test.flags)
require.NoError(t, err)
o.InitFromViper(v)
assert.Equal(t, test.expected, o.AuthenticationConfig)
})
}
}

func TestFlagDefaults(t *testing.T) {
o := &Options{}
v, command := config.Viperize(AddFlags)
Expand Down
7 changes: 7 additions & 0 deletions pkg/kafka/auth/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ func (config *AuthenticationConfig) InitFromViper(configPrefix string, v *viper.

config.TLS = tlsClientConfig.InitFromViper(v)

if config.Authentication == tls {
config.TLS.Enabled = true
}
if config.TLS.Enabled == true {
Copy link
Member

Choose a reason for hiding this comment

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

in L83, can we change ShowEnabled to false?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will break jaeger-operator and clients which adopted 1.17

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, but do we really want to keep this duality? We can extend the config to render tls.enabled as deprecated, and add warnings when used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can try to do that, I don't like the current design either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought @jpkrohling had mentioned a required use case where TLS could be enabled, but authentication set to none or something other than tls?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have created an issue to remove them in the next release #2111

Copy link
Member Author

@pavolloffay pavolloffay Mar 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@objectiser yes, the current flags are misleading for people because there is overlap betwen:

--kafka.consumer.authentication
--kafka.consumer.tls.enabled

The goal is to remove the second one.

config.Authentication = tls
}

config.PlainText.UserName = v.GetString(configPrefix + plainTextPrefix + suffixPlainTextUserName)
config.PlainText.Password = v.GetString(configPrefix + plainTextPrefix + suffixPlainTextPassword)
}
43 changes: 43 additions & 0 deletions plugin/storage/kafka/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package kafka

import (
"fmt"
"testing"
"time"

Expand All @@ -23,6 +24,8 @@ import (
"github.com/stretchr/testify/require"

"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/config/tlscfg"
"github.com/jaegertracing/jaeger/pkg/kafka/auth"
)

func TestOptionsWithFlags(t *testing.T) {
Expand Down Expand Up @@ -164,3 +167,43 @@ func TestRequiredAcksFailures(t *testing.T) {
_, err := getRequiredAcks("test")
assert.Error(t, err)
}

func TestTLSFlags(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

These tests are duplicated. Don't we have Kafka flags parsed by a packaged shared between ingester and storage?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both tests are in different packages, there is no shared test class.

tests := []struct {
flags []string
expected auth.AuthenticationConfig
}{
{
flags: []string{},
expected: auth.AuthenticationConfig{Authentication: "none", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}},
},
{
flags: []string{"--kafka.producer.authentication=foo"},
expected: auth.AuthenticationConfig{Authentication: "foo", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}},
},
{
flags: []string{"--kafka.producer.authentication=kerberos", "--kafka.producer.tls.enabled=true"},
expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}, TLS: tlscfg.Options{Enabled: true}},
},
{
flags: []string{"--kafka.producer.authentication=tls"},
expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}, TLS: tlscfg.Options{Enabled: true}},
},
{
flags: []string{"--kafka.producer.authentication=tls", "--kafka.producer.tls.enabled=false"},
expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: auth.KerberosConfig{ServiceName: "kafka", ConfigPath: "/etc/krb5.conf", KeyTabPath: "/etc/security/kafka.keytab"}, TLS: tlscfg.Options{Enabled: true}},
},
}

for _, test := range tests {
t.Run(fmt.Sprintf("%s", test.flags), func(t *testing.T) {
o := &Options{}
v, command := config.Viperize(o.AddFlags)
err := command.ParseFlags(test.flags)
require.NoError(t, err)
o.InitFromViper(v)
assert.Equal(t, test.expected, o.config.AuthenticationConfig)

})
}
}