Default to https scheme for --proxy argument in tctl auth sign (#…

…10844) (#10911) Before this commit, if `--proxy` was set, it would be passed as it to the kubeconfig file. Due to this, if the `--proxy` URL did not have a scheme, it would default to `http`, leading to the issue reported in https://github.com/gravitational/cloud/issues/1358. With this commit, we now try to parse the `--proxy` URL and set its scheme to `https` in case it's not set. In case it's set, we only allow `--proxy` URLs with the `http` and `https` schemes.
gravitational · Mar 8, 2022 · d257f56 · d257f56
1 parent d4b7512
commit d257f56
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 15 deletions.
diff --git a/tool/tctl/common/auth_command.go b/tool/tctl/common/auth_command.go
@@ -737,7 +737,21 @@ func (a *AuthCommand) checkProxyAddr(clusterAPI auth.ClientI) error {
 		return nil
 	}
 	if a.proxyAddr != "" {
-		return nil
+		// User set --proxy. Validate it and set its scheme to https in case it was omitted.
+		u, err := url.Parse(a.proxyAddr)
+		if err != nil {
+			return trace.WrapWithMessage(err, "specified --proxy URL is invalid")
+		}
+		switch u.Scheme {
+		case "":
+			u.Scheme = "https"
+			a.proxyAddr = u.String()
+			return nil
+		case "http", "https":
+			return nil
+		default:
+			return trace.BadParameter("expected --proxy URL with http or https scheme")
+		}
 	}
 
 	// User didn't specify --proxy for kubeconfig. Let's try to guess it.

diff --git a/tool/tctl/common/auth_command_test.go b/tool/tctl/common/auth_command_test.go
@@ -104,17 +104,55 @@ func TestAuthSignKubeconfig(t *testing.T) {
 		ac          AuthCommand
 		wantAddr    string
 		wantCluster string
-		wantError   string
+		assertErr   require.ErrorAssertionFunc
 	}{
 		{
-			desc: "--proxy specified",
+			desc: "valid --proxy URL with valid URL scheme",
+			ac: AuthCommand{
+				output:        filepath.Join(tmpDir, "kubeconfig"),
+				outputFormat:  identityfile.FormatKubernetes,
+				signOverwrite: true,
+				proxyAddr:     "https://proxy-from-flag.example.com",
+			},
+			wantAddr:  "https://proxy-from-flag.example.com",
+			assertErr: require.NoError,
+		},
+		{
+			desc: "valid --proxy URL with invalid URL scheme",
+			ac: AuthCommand{
+				output:        filepath.Join(tmpDir, "kubeconfig"),
+				outputFormat:  identityfile.FormatKubernetes,
+				signOverwrite: true,
+				proxyAddr:     "file://proxy-from-flag.example.com",
+			},
+			assertErr: func(t require.TestingT, err error, _ ...interface{}) {
+				require.Error(t, err)
+				require.Equal(t, err.Error(), "expected --proxy URL with http or https scheme")
+			},
+		},
+		{
+			desc: "valid --proxy URL without URL scheme",
 			ac: AuthCommand{
 				output:        filepath.Join(tmpDir, "kubeconfig"),
 				outputFormat:  identityfile.FormatKubernetes,
 				signOverwrite: true,
 				proxyAddr:     "proxy-from-flag.example.com",
 			},
-			wantAddr: "proxy-from-flag.example.com",
+			wantAddr:  "https://proxy-from-flag.example.com",
+			assertErr: require.NoError,
+		},
+		{
+			desc: "invalid --proxy URL",
+			ac: AuthCommand{
+				output:        filepath.Join(tmpDir, "kubeconfig"),
+				outputFormat:  identityfile.FormatKubernetes,
+				signOverwrite: true,
+				proxyAddr:     "1https://proxy-from-flag.example.com",
+			},
+			assertErr: func(t require.TestingT, err error, _ ...interface{}) {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), "specified --proxy URL is invalid")
+			},
 		},
 		{
 			desc: "k8s proxy running locally with public_addr",
@@ -127,7 +165,8 @@ func TestAuthSignKubeconfig(t *testing.T) {
 					PublicAddrs: []utils.NetAddr{{Addr: "proxy-from-config.example.com:3026"}},
 				}}},
 			},
-			wantAddr: "https://proxy-from-config.example.com:3026",
+			wantAddr:  "https://proxy-from-config.example.com:3026",
+			assertErr: require.NoError,
 		},
 		{
 			desc: "k8s proxy running locally without public_addr",
@@ -142,7 +181,8 @@ func TestAuthSignKubeconfig(t *testing.T) {
 					PublicAddrs: []utils.NetAddr{{Addr: "proxy-from-config.example.com:3080"}},
 				}},
 			},
-			wantAddr: "https://proxy-from-config.example.com:3026",
+			wantAddr:  "https://proxy-from-config.example.com:3026",
+			assertErr: require.NoError,
 		},
 		{
 			desc: "k8s proxy from cluster info",
@@ -156,7 +196,8 @@ func TestAuthSignKubeconfig(t *testing.T) {
 					},
 				}},
 			},
-			wantAddr: "https://proxy-from-api.example.com:3026",
+			wantAddr:  "https://proxy-from-api.example.com:3026",
+			assertErr: require.NoError,
 		},
 		{
 			desc: "--kube-cluster specified with valid cluster",
@@ -172,6 +213,7 @@ func TestAuthSignKubeconfig(t *testing.T) {
 				}},
 			},
 			wantCluster: remoteCluster.GetMetadata().Name,
+			assertErr:   require.NoError,
 		},
 		{
 			desc: "--kube-cluster specified with invalid cluster",
@@ -186,19 +228,17 @@ func TestAuthSignKubeconfig(t *testing.T) {
 					},
 				}},
 			},
-			wantError: "couldn't find leaf cluster named \"doesnotexist.example.com\"",
+			assertErr: func(t require.TestingT, err error, _ ...interface{}) {
+				require.Error(t, err)
+				require.Equal(t, err.Error(), "couldn't find leaf cluster named \"doesnotexist.example.com\"")
+			},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
 			// Generate kubeconfig.
-			if err = tt.ac.generateUserKeys(client); err != nil && tt.wantError == "" {
-				t.Fatalf("generating KubeProxyConfig: %v", err)
-			}
-
-			if tt.wantError != "" && (err == nil || err.Error() != tt.wantError) {
-				t.Errorf("got error %v, want %v", err, tt.wantError)
-			}
+			err := tt.ac.generateUserKeys(client)
+			tt.assertErr(t, err)
 
 			// Validate kubeconfig contents.
 			kc, err := kubeconfig.Load(tt.ac.output)