feat: Fallback to AZURE_CLIENT_ID env var if no client_id query p…

…aram in token request (#628) * feat: Fallback to `AZURE_CLIENT_ID` env var if no `client_id` query param in token request Signed-off-by: Anish Ramasekar <[email protected]> * test: add e2e test for validating proxy fallback to AZURE_CLIENT_ID Signed-off-by: Anish Ramasekar <[email protected]> Signed-off-by: Anish Ramasekar <[email protected]>
Azure · Nov 10, 2022 · 79cc91b · 79cc91b
1 parent c4a69ec
commit 79cc91b
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 5 deletions.
diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go
@@ -107,11 +107,16 @@ func (p *proxy) msiHandler(w http.ResponseWriter, r *http.Request) {
 	p.logger.Info("received token request", "method", r.Method, "uri", r.RequestURI)
 	w.Header().Set("Server", userAgent)
 	clientID, resource := parseTokenRequest(r)
-	// TODO (aramase) should we fallback to the clientID in the annotated service account
-	// if clientID not found in request? This is to keep consistent with the current behavior
-	// in pod identity v1 where we default the client id to the one in AzureIdentity.
+	// if clientID not found in request, then we default to the AZURE_CLIENT_ID if present.
+	// This is to keep consistent with the current behavior in pod identity v1 where we
+	// default the client id to the one in AzureIdentity.
 	if clientID == "" {
-		http.Error(w, "The client_id parameter is required.", http.StatusBadRequest)
+		p.logger.Info("client_id not found in request, defaulting to AZURE_CLIENT_ID", "method", r.Method, "uri", r.RequestURI)
+		clientID = os.Getenv(webhook.AzureClientIDEnvVar)
+	}
+
+	if clientID == "" {
+		http.Error(w, "The client_id parameter or AZURE_CLIENT_ID environment variable must be set", http.StatusBadRequest)
 		return
 	}
 	if resource == "" {

diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go
@@ -46,7 +46,7 @@ func TestProxy_MSIHandler(t *testing.T) {
 			name:               "client_id is missing",
 			path:               `/metadata/identity/oauth2/token?api-version=2018-02-01&resource=https%3A%2F%2Fvault.azure.net%2F`,
 			expectedStatusCode: http.StatusBadRequest,
-			expectedBody:       "The client_id parameter is required.\n",
+			expectedBody:       "The client_id parameter or AZURE_CLIENT_ID environment variable must be set\n",
 		},
 		{
 			name:               "resource is missing",

diff --git a/test/e2e/proxy_test.go b/test/e2e/proxy_test.go
@@ -72,4 +72,77 @@ var _ = ginkgo.Describe("Proxy [LinuxOnly] [AKSSoakOnly] [Exclude:Arc]", func()
 			}, framework.PollShortTimeout, framework.Poll).Should(gomega.BeTrue())
 		}
 	})
+
+	// This test is to validate the proxy sidecar fallback behavior to AZURE_CLIENT_ID when the client_id parameter is not part of the request.
+	ginkgo.It("should get a valid AAD token after injecting proxy init container and sidecar with no client_id in request", func() {
+		clientID, ok := os.LookupEnv("APPLICATION_CLIENT_ID")
+		gomega.Expect(ok).To(gomega.BeTrue(), "APPLICATION_CLIENT_ID must be set")
+		// trust is only set up for 'proxy-test-sa' service account in the default namespace for now
+		const namespace = "default"
+		serviceAccount := createServiceAccount(f.ClientSet, namespace, "proxy-test-sa", map[string]string{webhook.UseWorkloadIdentityLabel: "true"}, map[string]string{webhook.ClientIDAnnotation: clientID})
+		defer f.ClientSet.CoreV1().ServiceAccounts(namespace).Delete(context.TODO(), serviceAccount, metav1.DeleteOptions{})
+
+		proxyAnnotations := map[string]string{
+			webhook.InjectProxySidecarAnnotation: "true",
+			webhook.ProxySidecarPortAnnotation:   "8080",
+		}
+
+		pod := generatePodWithServiceAccount(
+			f.ClientSet,
+			namespace,
+			serviceAccount,
+			"mcr.microsoft.com/azure-cli",
+			nil,
+			// no client_id in request
+			[]string{"/bin/sh", "-c", "az login -i --allow-no-subscriptions --debug; sleep 3600"},
+			nil,
+			proxyAnnotations,
+			true,
+		)
+
+		pod, err := createPod(f.ClientSet, pod)
+		framework.ExpectNoError(err, "failed to create pod %s in %s", pod.Name, namespace)
+		defer f.ClientSet.CoreV1().Pods(namespace).Delete(context.TODO(), pod.Name, metav1.DeleteOptions{})
+
+		// output proxy and proxy init logs for debugging
+		defer func() {
+			for _, container := range []string{webhook.ProxyInitContainerName, webhook.ProxySidecarContainerName} {
+				stdout, _ := e2epod.GetPodLogs(f.ClientSet, namespace, pod.Name, container)
+				framework.Logf("%s logs: %s", container, stdout)
+			}
+		}()
+
+		for _, container := range []string{busybox1, busybox2} {
+			framework.Logf("validating that %s in %s has acquired a valid AAD token via the proxy using AZURE_CLIENT_ID", container, pod.Name)
+			gomega.Eventually(func() bool {
+				stdout, err := e2epod.GetPodLogs(f.ClientSet, namespace, pod.Name, container)
+				if err != nil {
+					framework.Logf("failed to get logs from container %s in %s/%s: %v. Retrying...", container, namespace, pod.Name, err)
+					return false
+				}
+				framework.Logf("stdout: %s", stdout)
+				/*
+					[
+					  {
+					    "environmentName": "AzureCloud",
+					    "id": "72f988bf-86f1-41af-91ab-2d7cd011db47",
+					    "isDefault": true,
+					    "name": "N/A(tenant level account)",
+					    "state": "Enabled",
+					    "tenantId": "72f988bf-86f1-41af-91ab-2d7cd011db47",
+					    "user": {
+					      "assignedIdentityInfo": "MSIClient-3e532d33-08d4-4dea-868c-4c5c4318b6db",
+					      "name": "userAssignedIdentity",
+					      "type": "servicePrincipal"
+					    }
+					  }
+					]
+
+					// successful response on login will have the above output, so we are asserting that output
+					// contains the below string to validate that the login was successful with workload identity
+				*/
+				return strings.Contains(stdout, `"environmentName": "AzureCloud"`)
+			}, framework.PollShortTimeout, framework.Poll).Should(gomega.BeTrue())
+		}
+	})
 })