From 06cbb29e9e9b10c371347d2457ff8708f4a9e9f5 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 17 Aug 2016 16:09:04 -0400 Subject: [PATCH] ClientConfig should not default to http://localhost:8080 This changes clientcmd to skip the default cluster, but preserves the behavior in kubectl. This prevents the possibility of an administrator misconfiguration in kubelet or other server component from allowing a third party who can bind to 8080 on that host from potentially impersonating an API server and gaining root access. --- .../unversioned/clientcmd/client_config.go | 2 +- .../clientcmd/client_config_test.go | 41 ++++++++++++++++--- pkg/client/unversioned/clientcmd/overrides.go | 10 +++-- pkg/kubectl/cmd/util/factory.go | 4 ++ .../admission/imagepolicy/admission_test.go | 4 +- .../auth/authorizer/webhook/webhook_test.go | 4 +- 6 files changed, 50 insertions(+), 15 deletions(-) diff --git a/pkg/client/unversioned/clientcmd/client_config.go b/pkg/client/unversioned/clientcmd/client_config.go index 3be1d77c66b..014c6a24069 100644 --- a/pkg/client/unversioned/clientcmd/client_config.go +++ b/pkg/client/unversioned/clientcmd/client_config.go @@ -329,7 +329,7 @@ func (config *DirectClientConfig) getCluster() clientcmdapi.Cluster { clusterInfoName := config.getClusterName() var mergedClusterInfo clientcmdapi.Cluster - mergo.Merge(&mergedClusterInfo, DefaultCluster) + mergo.Merge(&mergedClusterInfo, config.overrides.ClusterDefaults) mergo.Merge(&mergedClusterInfo, EnvVarCluster) if configClusterInfo, exists := clusterInfos[clusterInfoName]; exists { mergo.Merge(&mergedClusterInfo, configClusterInfo) diff --git a/pkg/client/unversioned/clientcmd/client_config_test.go b/pkg/client/unversioned/clientcmd/client_config_test.go index e819e72fe07..a561d05d8d8 100644 --- a/pkg/client/unversioned/clientcmd/client_config_test.go +++ b/pkg/client/unversioned/clientcmd/client_config_test.go @@ -304,11 +304,13 @@ func TestCreateCleanWithPrefix(t *testing.T) { cleanConfig.Server = tc.server config.Clusters["clean"] = cleanConfig - clientBuilder := NewNonInteractiveClientConfig(*config, "clean", &ConfigOverrides{}, nil) + clientBuilder := NewNonInteractiveClientConfig(*config, "clean", &ConfigOverrides{ + ClusterDefaults: DefaultCluster, + }, nil) clientConfig, err := clientBuilder.ClientConfig() if err != nil { - t.Errorf("Unexpected error: %v", err) + t.Fatalf("Unexpected error: %v", err) } matchStringArg(tc.host, clientConfig.Host, t) @@ -321,7 +323,7 @@ func TestCreateCleanDefault(t *testing.T) { clientConfig, err := clientBuilder.ClientConfig() if err != nil { - t.Errorf("Unexpected error: %v", err) + t.Fatalf("Unexpected error: %v", err) } matchStringArg(config.Clusters["clean"].Server, clientConfig.Host, t) @@ -329,14 +331,42 @@ func TestCreateCleanDefault(t *testing.T) { matchStringArg(config.AuthInfos["clean"].Token, clientConfig.BearerToken, t) } -func TestCreateMissingContext(t *testing.T) { +func TestCreateCleanDefaultCluster(t *testing.T) { + config := createValidTestConfig() + clientBuilder := NewDefaultClientConfig(*config, &ConfigOverrides{ + ClusterDefaults: DefaultCluster, + }) + + clientConfig, err := clientBuilder.ClientConfig() + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + matchStringArg(config.Clusters["clean"].Server, clientConfig.Host, t) + matchBoolArg(config.Clusters["clean"].InsecureSkipTLSVerify, clientConfig.Insecure, t) + matchStringArg(config.AuthInfos["clean"].Token, clientConfig.BearerToken, t) +} + +func TestCreateMissingContextNoDefault(t *testing.T) { const expectedErrorContains = "Context was not found for specified context" config := createValidTestConfig() clientBuilder := NewNonInteractiveClientConfig(*config, "not-present", &ConfigOverrides{}, nil) + _, err := clientBuilder.ClientConfig() + if err == nil { + t.Fatalf("Unexpected error: %v", err) + } +} +func TestCreateMissingContext(t *testing.T) { + const expectedErrorContains = "Context was not found for specified context" + config := createValidTestConfig() + clientBuilder := NewNonInteractiveClientConfig(*config, "not-present", &ConfigOverrides{ + ClusterDefaults: DefaultCluster, + }, nil) + clientConfig, err := clientBuilder.ClientConfig() if err != nil { - t.Errorf("Unexpected error: %v", err) + t.Fatalf("Unexpected error: %v", err) } expectedConfig := &restclient.Config{Host: clientConfig.Host} @@ -344,7 +374,6 @@ func TestCreateMissingContext(t *testing.T) { if !reflect.DeepEqual(expectedConfig, clientConfig) { t.Errorf("Expected %#v, got %#v", expectedConfig, clientConfig) } - } func matchBoolArg(expected, got bool, t *testing.T) { diff --git a/pkg/client/unversioned/clientcmd/overrides.go b/pkg/client/unversioned/clientcmd/overrides.go index afb8e716995..9c117ea3548 100644 --- a/pkg/client/unversioned/clientcmd/overrides.go +++ b/pkg/client/unversioned/clientcmd/overrides.go @@ -27,10 +27,12 @@ import ( // ConfigOverrides holds values that should override whatever information is pulled from the actual Config object. You can't // simply use an actual Config object, because Configs hold maps, but overrides are restricted to "at most one" type ConfigOverrides struct { - AuthInfo clientcmdapi.AuthInfo - ClusterInfo clientcmdapi.Cluster - Context clientcmdapi.Context - CurrentContext string + AuthInfo clientcmdapi.AuthInfo + // ClusterDefaults are applied before the configured cluster info is loaded. + ClusterDefaults clientcmdapi.Cluster + ClusterInfo clientcmdapi.Cluster + Context clientcmdapi.Context + CurrentContext string } // ConfigOverrideFlags holds the flag names to be used for binding command line flags. Notice that this structure tightly diff --git a/pkg/kubectl/cmd/util/factory.go b/pkg/kubectl/cmd/util/factory.go index 13db3b45728..9af9777dd45 100644 --- a/pkg/kubectl/cmd/util/factory.go +++ b/pkg/kubectl/cmd/util/factory.go @@ -33,6 +33,7 @@ import ( "time" "github.com/emicklei/go-restful/swagger" + "github.com/imdario/mergo" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -1219,6 +1220,9 @@ func DefaultClientConfig(flags *pflag.FlagSet) clientcmd.ClientConfig { flags.StringVar(&loadingRules.ExplicitPath, "kubeconfig", "", "Path to the kubeconfig file to use for CLI requests.") overrides := &clientcmd.ConfigOverrides{} + // use the standard defaults for this client config + mergo.Merge(&overrides.ClusterDefaults, clientcmd.DefaultCluster) + flagNames := clientcmd.RecommendedConfigOverrideFlags("") // short flagnames are disabled by default. These are here for compatibility with existing scripts flagNames.ClusterOverrideFlags.APIServer.ShortName = "s" diff --git a/plugin/pkg/admission/imagepolicy/admission_test.go b/plugin/pkg/admission/imagepolicy/admission_test.go index cb377bf6432..7c0af1b405f 100644 --- a/plugin/pkg/admission/imagepolicy/admission_test.go +++ b/plugin/pkg/admission/imagepolicy/admission_test.go @@ -115,7 +115,7 @@ users: client-certificate: {{ .Cert }} client-key: {{ .Key }} `, - wantErr: false, + wantErr: true, }, { msg: "multiple clusters with no context", @@ -135,7 +135,7 @@ users: client-certificate: {{ .Cert }} client-key: {{ .Key }} `, - wantErr: false, + wantErr: true, }, { msg: "multiple clusters with a context", diff --git a/plugin/pkg/auth/authorizer/webhook/webhook_test.go b/plugin/pkg/auth/authorizer/webhook/webhook_test.go index 613e3db2dc0..703fc02c6f7 100644 --- a/plugin/pkg/auth/authorizer/webhook/webhook_test.go +++ b/plugin/pkg/auth/authorizer/webhook/webhook_test.go @@ -89,7 +89,7 @@ users: client-certificate: {{ .Cert }} client-key: {{ .Key }} `, - wantErr: false, + wantErr: true, }, { msg: "multiple clusters with no context", @@ -109,7 +109,7 @@ users: client-certificate: {{ .Cert }} client-key: {{ .Key }} `, - wantErr: false, + wantErr: true, }, { msg: "multiple clusters with a context",