From c9ad1d7339b164dfba0846ec49fa4a52474d3e23 Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Fri, 2 Nov 2018 11:39:14 -0700 Subject: [PATCH] Implement fmt.Stringer on rest.Config to sanitize sensitive fields It's very easy to add glog.Info(config) calls for debugging (or actual logging). In some scenarios those configs will carry sensitive tokens and those tokens will end up in logs or response bodies. Leaking of those stringified configs compromises the cluster. Also implement fmt.GoStringer. --- .../plugin/pkg/client/auth/exec/BUILD | 1 + .../plugin/pkg/client/auth/exec/exec.go | 5 +- staging/src/k8s.io/client-go/rest/config.go | 75 ++++++++++ .../src/k8s.io/client-go/rest/config_test.go | 139 ++++++++++++++++++ .../client-go/tools/clientcmd/api/types.go | 44 ++++++ .../sample-cli-plugin/Godeps/Godeps.json | 4 + 6 files changed, 267 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/BUILD b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/BUILD index 6941559833..21a8fc0271 100644 --- a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/BUILD +++ b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/BUILD @@ -18,6 +18,7 @@ go_library( "//staging/src/k8s.io/client-go/tools/clientcmd/api:go_default_library", "//staging/src/k8s.io/client-go/transport:go_default_library", "//staging/src/k8s.io/client-go/util/connrotation:go_default_library", + "//vendor/github.com/davecgh/go-spew/spew:go_default_library", "//vendor/golang.org/x/crypto/ssh/terminal:go_default_library", "//vendor/k8s.io/klog:go_default_library", ], diff --git a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go index be4814bcc3..b88902c103 100644 --- a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go +++ b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go @@ -31,6 +31,7 @@ import ( "sync" "time" + "github.com/davecgh/go-spew/spew" "golang.org/x/crypto/ssh/terminal" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -73,8 +74,10 @@ func newCache() *cache { return &cache{m: make(map[string]*Authenticator)} } +var spewConfig = &spew.ConfigState{DisableMethods: true, Indent: " "} + func cacheKey(c *api.ExecConfig) string { - return fmt.Sprintf("%#v", c) + return spewConfig.Sprint(c) } type cache struct { diff --git a/staging/src/k8s.io/client-go/rest/config.go b/staging/src/k8s.io/client-go/rest/config.go index 271693c2c5..3f6b9bc236 100644 --- a/staging/src/k8s.io/client-go/rest/config.go +++ b/staging/src/k8s.io/client-go/rest/config.go @@ -129,6 +129,47 @@ type Config struct { // Version string } +var _ fmt.Stringer = new(Config) +var _ fmt.GoStringer = new(Config) + +type sanitizedConfig *Config + +type sanitizedAuthConfigPersister struct{ AuthProviderConfigPersister } + +func (sanitizedAuthConfigPersister) GoString() string { + return "rest.AuthProviderConfigPersister(--- REDACTED ---)" +} +func (sanitizedAuthConfigPersister) String() string { + return "rest.AuthProviderConfigPersister(--- REDACTED ---)" +} + +// GoString implements fmt.GoStringer and sanitizes sensitive fields of Config +// to prevent accidental leaking via logs. +func (c *Config) GoString() string { + return c.String() +} + +// String implements fmt.Stringer and sanitizes sensitive fields of Config to +// prevent accidental leaking via logs. +func (c *Config) String() string { + if c == nil { + return "" + } + cc := sanitizedConfig(CopyConfig(c)) + // Explicitly mark non-empty credential fields as redacted. + if cc.Password != "" { + cc.Password = "--- REDACTED ---" + } + if cc.BearerToken != "" { + cc.BearerToken = "--- REDACTED ---" + } + if cc.AuthConfigPersister != nil { + cc.AuthConfigPersister = sanitizedAuthConfigPersister{cc.AuthConfigPersister} + } + + return fmt.Sprintf("%#v", cc) +} + // ImpersonationConfig has all the available impersonation options type ImpersonationConfig struct { // UserName is the username to impersonate on each request. @@ -168,6 +209,40 @@ type TLSClientConfig struct { CAData []byte } +var _ fmt.Stringer = TLSClientConfig{} +var _ fmt.GoStringer = TLSClientConfig{} + +type sanitizedTLSClientConfig TLSClientConfig + +// GoString implements fmt.GoStringer and sanitizes sensitive fields of +// TLSClientConfig to prevent accidental leaking via logs. +func (c TLSClientConfig) GoString() string { + return c.String() +} + +// String implements fmt.Stringer and sanitizes sensitive fields of +// TLSClientConfig to prevent accidental leaking via logs. +func (c TLSClientConfig) String() string { + cc := sanitizedTLSClientConfig{ + Insecure: c.Insecure, + ServerName: c.ServerName, + CertFile: c.CertFile, + KeyFile: c.KeyFile, + CAFile: c.CAFile, + CertData: c.CertData, + KeyData: c.KeyData, + CAData: c.CAData, + } + // Explicitly mark non-empty credential fields as redacted. + if len(cc.CertData) != 0 { + cc.CertData = []byte("--- TRUNCATED ---") + } + if len(cc.KeyData) != 0 { + cc.KeyData = []byte("--- REDACTED ---") + } + return fmt.Sprintf("%#v", cc) +} + type ContentConfig struct { // AcceptContentTypes specifies the types the client will accept and is optional. // If not set, ContentType will be used to define the Accept header diff --git a/staging/src/k8s.io/client-go/rest/config_test.go b/staging/src/k8s.io/client-go/rest/config_test.go index 8f5cce674d..df4d257c8b 100644 --- a/staging/src/k8s.io/client-go/rest/config_test.go +++ b/staging/src/k8s.io/client-go/rest/config_test.go @@ -19,6 +19,7 @@ package rest import ( "context" "errors" + "fmt" "io" "net" "net/http" @@ -26,6 +27,7 @@ import ( "reflect" "strings" "testing" + "time" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" @@ -381,3 +383,140 @@ func TestCopyConfig(t *testing.T) { } } } + +func TestConfigStringer(t *testing.T) { + formatBytes := func(b []byte) string { + // %#v for []byte always pre-pends "[]byte{". + // %#v for struct with []byte field always pre-pends "[]uint8{". + return strings.Replace(fmt.Sprintf("%#v", b), "byte", "uint8", 1) + } + tests := []struct { + desc string + c *Config + expectContent []string + prohibitContent []string + }{ + { + desc: "nil config", + c: nil, + expectContent: []string{""}, + }, + { + desc: "non-sensitive config", + c: &Config{ + Host: "localhost:8080", + APIPath: "v1", + UserAgent: "gobot", + }, + expectContent: []string{"localhost:8080", "v1", "gobot"}, + }, + { + desc: "sensitive config", + c: &Config{ + Host: "localhost:8080", + Username: "gopher", + Password: "g0ph3r", + BearerToken: "1234567890", + TLSClientConfig: TLSClientConfig{ + CertFile: "a.crt", + KeyFile: "a.key", + CertData: []byte("fake cert"), + KeyData: []byte("fake key"), + }, + AuthProvider: &clientcmdapi.AuthProviderConfig{ + Config: map[string]string{"secret": "s3cr3t"}, + }, + ExecProvider: &clientcmdapi.ExecConfig{ + Args: []string{"secret"}, + Env: []clientcmdapi.ExecEnvVar{{Name: "secret", Value: "s3cr3t"}}, + }, + }, + expectContent: []string{ + "localhost:8080", + "gopher", + "a.crt", + "a.key", + "--- REDACTED ---", + formatBytes([]byte("--- REDACTED ---")), + formatBytes([]byte("--- TRUNCATED ---")), + }, + prohibitContent: []string{ + "g0ph3r", + "1234567890", + formatBytes([]byte("fake cert")), + formatBytes([]byte("fake key")), + "secret", + "s3cr3t", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + got := tt.c.String() + t.Logf("formatted config: %q", got) + + for _, expect := range tt.expectContent { + if !strings.Contains(got, expect) { + t.Errorf("missing expected string %q", expect) + } + } + for _, prohibit := range tt.prohibitContent { + if strings.Contains(got, prohibit) { + t.Errorf("found prohibited string %q", prohibit) + } + } + }) + } +} + +func TestConfigSprint(t *testing.T) { + c := &Config{ + Host: "localhost:8080", + APIPath: "v1", + ContentConfig: ContentConfig{ + AcceptContentTypes: "application/json", + ContentType: "application/json", + }, + Username: "gopher", + Password: "g0ph3r", + BearerToken: "1234567890", + Impersonate: ImpersonationConfig{ + UserName: "gopher2", + }, + AuthProvider: &clientcmdapi.AuthProviderConfig{ + Name: "gopher", + Config: map[string]string{"secret": "s3cr3t"}, + }, + AuthConfigPersister: fakeAuthProviderConfigPersister{}, + ExecProvider: &clientcmdapi.ExecConfig{ + Command: "sudo", + Args: []string{"secret"}, + Env: []clientcmdapi.ExecEnvVar{{Name: "secret", Value: "s3cr3t"}}, + }, + TLSClientConfig: TLSClientConfig{ + CertFile: "a.crt", + KeyFile: "a.key", + CertData: []byte("fake cert"), + KeyData: []byte("fake key"), + }, + UserAgent: "gobot", + Transport: &fakeRoundTripper{}, + WrapTransport: fakeWrapperFunc, + QPS: 1, + Burst: 2, + RateLimiter: &fakeLimiter{}, + Timeout: 3 * time.Second, + Dial: fakeDialFunc, + } + want := fmt.Sprintf( + `&rest.Config{Host:"localhost:8080", APIPath:"v1", ContentConfig:rest.ContentConfig{AcceptContentTypes:"application/json", ContentType:"application/json", GroupVersion:(*schema.GroupVersion)(nil), NegotiatedSerializer:runtime.NegotiatedSerializer(nil)}, Username:"gopher", Password:"--- REDACTED ---", BearerToken:"--- REDACTED ---", BearerTokenFile:"", Impersonate:rest.ImpersonationConfig{UserName:"gopher2", Groups:[]string(nil), Extra:map[string][]string(nil)}, AuthProvider:api.AuthProviderConfig{Name: "gopher", Config: map[string]string{--- REDACTED ---}}, AuthConfigPersister:rest.AuthProviderConfigPersister(--- REDACTED ---), ExecProvider:api.AuthProviderConfig{Command: "sudo", Args: []string{"--- REDACTED ---"}, Env: []ExecEnvVar{--- REDACTED ---}, APIVersion: ""}, TLSClientConfig:rest.sanitizedTLSClientConfig{Insecure:false, ServerName:"", CertFile:"a.crt", KeyFile:"a.key", CAFile:"", CertData:[]uint8{0x2d, 0x2d, 0x2d, 0x20, 0x54, 0x52, 0x55, 0x4e, 0x43, 0x41, 0x54, 0x45, 0x44, 0x20, 0x2d, 0x2d, 0x2d}, KeyData:[]uint8{0x2d, 0x2d, 0x2d, 0x20, 0x52, 0x45, 0x44, 0x41, 0x43, 0x54, 0x45, 0x44, 0x20, 0x2d, 0x2d, 0x2d}, CAData:[]uint8(nil)}, UserAgent:"gobot", Transport:(*rest.fakeRoundTripper)(%p), WrapTransport:(transport.WrapperFunc)(%p), QPS:1, Burst:2, RateLimiter:(*rest.fakeLimiter)(%p), Timeout:3000000000, Dial:(func(context.Context, string, string) (net.Conn, error))(%p)}`, + c.Transport, fakeWrapperFunc, c.RateLimiter, fakeDialFunc, + ) + + for _, f := range []string{"%s", "%v", "%+v", "%#v"} { + if got := fmt.Sprintf(f, c); want != got { + t.Errorf("fmt.Sprintf(%q, c)\ngot: %q\nwant: %q", f, got, want) + } + } +} diff --git a/staging/src/k8s.io/client-go/tools/clientcmd/api/types.go b/staging/src/k8s.io/client-go/tools/clientcmd/api/types.go index 1391df7021..990a440c66 100644 --- a/staging/src/k8s.io/client-go/tools/clientcmd/api/types.go +++ b/staging/src/k8s.io/client-go/tools/clientcmd/api/types.go @@ -17,6 +17,8 @@ limitations under the License. package api import ( + "fmt" + "k8s.io/apimachinery/pkg/runtime" ) @@ -150,6 +152,25 @@ type AuthProviderConfig struct { Config map[string]string `json:"config,omitempty"` } +var _ fmt.Stringer = new(AuthProviderConfig) +var _ fmt.GoStringer = new(AuthProviderConfig) + +// GoString implements fmt.GoStringer and sanitizes sensitive fields of +// AuthProviderConfig to prevent accidental leaking via logs. +func (c AuthProviderConfig) GoString() string { + return c.String() +} + +// String implements fmt.Stringer and sanitizes sensitive fields of +// AuthProviderConfig to prevent accidental leaking via logs. +func (c AuthProviderConfig) String() string { + cfg := "" + if c.Config != nil { + cfg = "--- REDACTED ---" + } + return fmt.Sprintf("api.AuthProviderConfig{Name: %q, Config: map[string]string{%s}}", c.Name, cfg) +} + // ExecConfig specifies a command to provide client credentials. The command is exec'd // and outputs structured stdout holding credentials. // @@ -172,6 +193,29 @@ type ExecConfig struct { APIVersion string `json:"apiVersion,omitempty"` } +var _ fmt.Stringer = new(ExecConfig) +var _ fmt.GoStringer = new(ExecConfig) + +// GoString implements fmt.GoStringer and sanitizes sensitive fields of +// ExecConfig to prevent accidental leaking via logs. +func (c ExecConfig) GoString() string { + return c.String() +} + +// String implements fmt.Stringer and sanitizes sensitive fields of ExecConfig +// to prevent accidental leaking via logs. +func (c ExecConfig) String() string { + var args []string + if len(c.Args) > 0 { + args = []string{"--- REDACTED ---"} + } + env := "[]ExecEnvVar(nil)" + if len(c.Env) > 0 { + env = "[]ExecEnvVar{--- REDACTED ---}" + } + return fmt.Sprintf("api.AuthProviderConfig{Command: %q, Args: %#v, Env: %s, APIVersion: %q}", c.Command, args, env, c.APIVersion) +} + // ExecEnvVar is used for setting environment variables when executing an exec-based // credential plugin. type ExecEnvVar struct { diff --git a/staging/src/k8s.io/sample-cli-plugin/Godeps/Godeps.json b/staging/src/k8s.io/sample-cli-plugin/Godeps/Godeps.json index 09bafc3531..2ecca328a6 100644 --- a/staging/src/k8s.io/sample-cli-plugin/Godeps/Godeps.json +++ b/staging/src/k8s.io/sample-cli-plugin/Godeps/Godeps.json @@ -6,6 +6,10 @@ "./..." ], "Deps": [ + { + "ImportPath": "github.com/davecgh/go-spew/spew", + "Rev": "782f4967f2dc4564575ca782fe2d04090b5faca8" + }, { "ImportPath": "github.com/evanphx/json-patch", "Rev": "36442dbdb585210f8d5a1b45e67aa323c197d5c4"