From 79cc91ba1476d39afbd788757c7d2401b798ec90 Mon Sep 17 00:00:00 2001 From: Anish Ramasekar Date: Wed, 9 Nov 2022 18:01:00 -0800 Subject: [PATCH] feat: Fallback to `AZURE_CLIENT_ID` env var if no `client_id` query param 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 * test: add e2e test for validating proxy fallback to AZURE_CLIENT_ID Signed-off-by: Anish Ramasekar Signed-off-by: Anish Ramasekar --- pkg/proxy/proxy.go | 13 +++++--- pkg/proxy/proxy_test.go | 2 +- test/e2e/proxy_test.go | 73 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index c7db4af5b..0b0a93eef 100644 --- 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 index 51eab6e55..9ca5d1a42 100644 --- 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 index 86f53ad78..5d703ef17 100644 --- 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()) + } + }) })