Skip to content

Include ServerName in tls transport cache key #56415

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

Merged
k8s-github-robot merged 2 commits into from
Nov 28, 2017
Merged

Include ServerName in tls transport cache key #56415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
195a4d6
Include ServerName in tls transport cache key
liggitt Nov 27, 2017
18d0b70
Fix TestAggregatedAPIServer setup
liggitt Nov 27, 2017
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
2 changes: 1 addition & 1 deletion staging/src/k8s.io/client-go/transport/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,5 +88,5 @@ func tlsConfigKey(c *Config) (string, error) {
return "", err
}
// Only include the things that actually affect the tls.Config
return fmt.Sprintf("%v/%x/%x/%x", c.TLS.Insecure, c.TLS.CAData, c.TLS.CertData, c.TLS.KeyData), nil
return fmt.Sprintf("%v/%x/%x/%x/%v", c.TLS.Insecure, c.TLS.CAData, c.TLS.CertData, c.TLS.KeyData, c.TLS.ServerName), nil
Copy link
Contributor

@lavalamp lavalamp Nov 30, 2017

Choose a reason for hiding this comment

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

Uh, are we printing very sensitive key data into the cache key? That seems like a terrible idea, doesn't it?

Copy link
Contributor

@lavalamp lavalamp Nov 30, 2017

Choose a reason for hiding this comment

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

It's safer to put things into a key struct, that way you don't have to worry about collisions (things maybe having / in them that shouldn't, say).

Copy link
Member Author

@liggitt liggitt Nov 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Uh, are we printing very sensitive key data into the cache key? That seems like a terrible idea, doesn't it?

If you have memory access to the cache keys, you also have it to the cache data, which has the same information in a different form

It's safer to put things into a key struct, that way you don't have to worry about collisions (things maybe having / in them that shouldn't, say).

/ isn't an issue in this particular case with the current data (%x hex-encodes, and the first value is a bool), but agree a key struct would be better here

Copy link
Contributor

@lavalamp lavalamp Dec 4, 2017

Choose a reason for hiding this comment

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

@liggitt

If you have memory access to the cache keys, you also have it to the cache data, which has the same information in a different form

I can easily see someone printing the keys to a debug log or something, which is readable by more people than they expect. People don't usually expect cache keys to be super sensitive.

I agree the separator is likely not an actual bug in this particular case, but if you use the struct approach everywhere you prevent an entire class of really hard to find bugs, some of which are serious security problems.

I will likely file an issue about these items.

Copy link
Member Author

@liggitt liggitt Dec 4, 2017

Choose a reason for hiding this comment

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

fair enough. opened #56811 to switch to struct key with its own String() impl that omits logging the key data

Copy link
Contributor

@lavalamp lavalamp Dec 4, 2017

Choose a reason for hiding this comment

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

Thanks!

}
14 changes: 14 additions & 0 deletions staging/src/k8s.io/client-go/transport/cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,20 @@ func TestTLSConfigKey(t *testing.T) {
KeyData: []byte{1},
},
},
"cert 1, key 1, servername 1": {
TLS: TLSConfig{
CertData: []byte{1},
KeyData: []byte{1},
ServerName: "1",
},
},
"cert 1, key 1, servername 2": {
TLS: TLSConfig{
CertData: []byte{1},
KeyData: []byte{1},
ServerName: "2",
},
},
"cert 1, key 2": {
TLS: TLSConfig{
CertData: []byte{1},
Expand Down
7 changes: 6 additions & 1 deletion test/integration/examples/apiserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,12 @@ func TestAggregatedAPIServer(t *testing.T) {
if err != nil {
t.Fatal(err)
}
kubeClientConfigValue.Store(kubeAPIServerConfig.GenericConfig.LoopbackClientConfig)
// Adjust the loopback config for external use (external server name and CA)
kubeAPIServerClientConfig := rest.CopyConfig(kubeAPIServerConfig.GenericConfig.LoopbackClientConfig)
kubeAPIServerClientConfig.CAFile = path.Join(certDir, "apiserver.crt")
kubeAPIServerClientConfig.CAData = nil
kubeAPIServerClientConfig.ServerName = ""
kubeClientConfigValue.Store(kubeAPIServerClientConfig)

kubeAPIServer, err := app.CreateKubeAPIServer(kubeAPIServerConfig, genericapiserver.EmptyDelegate, sharedInformers, versionedInformers)
if err != nil {
Expand Down