From 5bab3380b2b49b4961400c02d1f70f74cd37525f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Jun 2025 22:19:20 +0000 Subject: [PATCH 1/3] Initial plan for issue From 7b961766b004a642d8dbe53fe19269ae56ee282a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Jun 2025 22:36:29 +0000 Subject: [PATCH 2/3] Add identity logging to ARM token exchange through IMDS Co-authored-by: matucker-msft <77026928+matucker-msft@users.noreply.github.com> --- internal/controller/acrpullbinding_controller.go | 2 +- internal/controller/acrpullbinding_v1beta2_controller.go | 6 +++--- .../controller/acrpullbinding_v1beta2_controller_test.go | 9 +++++---- internal/controller/generic_controller.go | 4 ++-- pkg/authorizer/token_retriever.go | 8 +++++++- 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/internal/controller/acrpullbinding_controller.go b/internal/controller/acrpullbinding_controller.go index 1b6be85..3481307 100644 --- a/internal/controller/acrpullbinding_controller.go +++ b/internal/controller/acrpullbinding_controller.go @@ -91,7 +91,7 @@ func NewV1beta1Reconciler(opts *V1beta1ReconcilerOpts) *AcrPullBindingReconciler msiClientID, msiResourceID, acrServer := specOrDefault(opts, binding.Spec) return base36sha224([]byte(msiClientID + msiResourceID + acrServer + binding.Spec.Scope)) }, - CreatePullCredential: func(ctx context.Context, binding *msiacrpullv1beta1.AcrPullBinding, serviceAccount *corev1.ServiceAccount) (string, time.Time, error) { + CreatePullCredential: func(ctx context.Context, logger logr.Logger, binding *msiacrpullv1beta1.AcrPullBinding, serviceAccount *corev1.ServiceAccount) (string, time.Time, error) { msiClientID, msiResourceID, acrServer := specOrDefault(opts, binding.Spec) acrAccessToken, err := opts.Auth.AcquireACRAccessToken(ctx, msiResourceID, msiClientID, acrServer, binding.Spec.Scope) if err != nil { diff --git a/internal/controller/acrpullbinding_v1beta2_controller.go b/internal/controller/acrpullbinding_v1beta2_controller.go index 969613a..bb100bc 100644 --- a/internal/controller/acrpullbinding_v1beta2_controller.go +++ b/internal/controller/acrpullbinding_v1beta2_controller.go @@ -31,7 +31,7 @@ type CoreOpts struct { } type ServiceAccountTokenMinter func(ctx context.Context, serviceAccountNamespace, serviceAccountName string) (*authenticationv1.TokenRequest, error) -type armTokenFetcher func(ctx context.Context, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) +type armTokenFetcher func(ctx context.Context, logger logr.Logger, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) type armAcrTokenExchanger func(ctx context.Context, armToken azcore.AccessToken, spec msiacrpullv1beta2.AcrConfiguration) (azcore.AccessToken, error) // V1beta2ReconcilerOpts configures the inputs for reconciling v1beta2 pull bindings @@ -97,7 +97,7 @@ func NewV1beta2Reconciler(opts *V1beta2ReconcilerOpts) *PullBindingReconciler { GetInputsHash: func(binding *msiacrpullv1beta2.AcrPullBinding) string { return inputsHash(binding.Spec) }, - CreatePullCredential: func(ctx context.Context, binding *msiacrpullv1beta2.AcrPullBinding, serviceAccount *corev1.ServiceAccount) (string, time.Time, error) { + CreatePullCredential: func(ctx context.Context, logger logr.Logger, binding *msiacrpullv1beta2.AcrPullBinding, serviceAccount *corev1.ServiceAccount) (string, time.Time, error) { var tenantId, clientId, token string if binding.Spec.Auth.WorkloadIdentity != nil { if binding.Spec.Auth.WorkloadIdentity.TenantID != "" { @@ -126,7 +126,7 @@ func NewV1beta2Reconciler(opts *V1beta2ReconcilerOpts) *PullBindingReconciler { token = response.Status.Token } - armToken, err := opts.fetchArmToken(ctx, binding.Spec, tenantId, clientId, token) + armToken, err := opts.fetchArmToken(ctx, logger, binding.Spec, tenantId, clientId, token) if err != nil { return "", time.Time{}, fmt.Errorf("failed to retrieve ARM token: %v", err) } diff --git a/internal/controller/acrpullbinding_v1beta2_controller_test.go b/internal/controller/acrpullbinding_v1beta2_controller_test.go index 3b0a0d5..671203e 100644 --- a/internal/controller/acrpullbinding_v1beta2_controller_test.go +++ b/internal/controller/acrpullbinding_v1beta2_controller_test.go @@ -8,6 +8,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore" msiacrpullv1beta2 "github.com/Azure/msi-acrpull/api/v1beta2" + "github.com/go-logr/logr" "github.com/go-logr/logr/testr" "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" @@ -1559,7 +1560,7 @@ func noopTokenStub() func(*testing.T, *msiacrpullv1beta2.AcrPullBinding, *corev1 return func(t *testing.T, binding *msiacrpullv1beta2.AcrPullBinding, serviceAccount *corev1.ServiceAccount) (ServiceAccountTokenMinter, armTokenFetcher, armAcrTokenExchanger) { return func(ctx context.Context, serviceAccountNamespace, serviceAccountName string) (*authenticationv1.TokenRequest, error) { return nil, errors.New("unexpected call to SA token request") - }, func(ctx context.Context, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) { + }, func(ctx context.Context, logger logr.Logger, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) { return azcore.AccessToken{}, errors.New("unexpected call to ARM token request") }, func(ctx context.Context, armToken azcore.AccessToken, spec msiacrpullv1beta2.AcrConfiguration) (azcore.AccessToken, error) { return azcore.AccessToken{}, errors.New("unexpected call to ARM ACR token exchange") @@ -1571,7 +1572,7 @@ func managedIdentityValidatingTokenStub(output azcore.AccessToken, outputError e return func(t *testing.T, binding *msiacrpullv1beta2.AcrPullBinding, serviceAccount *corev1.ServiceAccount) (ServiceAccountTokenMinter, armTokenFetcher, armAcrTokenExchanger) { return func(ctx context.Context, serviceAccountNamespace, serviceAccountName string) (*authenticationv1.TokenRequest, error) { return nil, errors.New("unexpected call to SA token request for managed identity") - }, func(ctx context.Context, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) { + }, func(ctx context.Context, logger logr.Logger, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) { assert.Empty(t, cmp.Diff(spec, binding.Spec), "arm token request binding spec mismatch") assert.Empty(t, serviceAccount.Annotations["azure.workload.identity/tenant-id"], "arm token request unexpected tenant id") assert.Empty(t, serviceAccount.Annotations["azure.workload.identity/client-id"], "arm token request unexpected client id") @@ -1597,7 +1598,7 @@ func workloadIdentityValidatingTokenStub(output azcore.AccessToken, outputError Token: "fake-sa-token", }, }, nil - }, func(ctx context.Context, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) { + }, func(ctx context.Context, logger logr.Logger, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) { assert.Empty(t, cmp.Diff(spec, binding.Spec), "arm token request binding spec mismatch") assert.Equal(t, serviceAccount.Annotations["azure.workload.identity/tenant-id"], tenantId, "arm token request tenant id mismatch") assert.Equal(t, serviceAccount.Annotations["azure.workload.identity/client-id"], clientId, "arm token request client id mismatch") @@ -1623,7 +1624,7 @@ func workloadIdentityLiteralValidatingTokenStub(output azcore.AccessToken, outpu Token: "fake-sa-token", }, }, nil - }, func(ctx context.Context, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) { + }, func(ctx context.Context, logger logr.Logger, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) { assert.Empty(t, cmp.Diff(spec, binding.Spec), "arm token request binding spec mismatch") assert.Equal(t, spec.Auth.WorkloadIdentity.TenantID, tenantId, "arm token request tenant id mismatch") assert.Equal(t, spec.Auth.WorkloadIdentity.ClientID, clientId, "arm token request client id mismatch") diff --git a/internal/controller/generic_controller.go b/internal/controller/generic_controller.go index 6ceda05..eaef2b6 100644 --- a/internal/controller/generic_controller.go +++ b/internal/controller/generic_controller.go @@ -34,7 +34,7 @@ type genericReconciler[O pullBinding] struct { GetPullSecretName func(O) string GetInputsHash func(O) string - CreatePullCredential func(context.Context, O, *corev1.ServiceAccount) (string, time.Time, error) + CreatePullCredential func(context.Context, logr.Logger, O, *corev1.ServiceAccount) (string, time.Time, error) UpdateStatusError func(O, string) O @@ -162,7 +162,7 @@ func (r *genericReconciler[O]) reconcile(ctx context.Context, logger logr.Logger if pullSecretMissing || pullSecretNeedsRefresh || pullSecretInputsChanged { logger.WithValues("pullSecretMissing", pullSecretMissing, "pullSecretNeedsRefresh", pullSecretNeedsRefresh, "pullSecretInputsChanged", pullSecretInputsChanged).Info("generating new pull credential") - dockerConfig, expiresOn, err := r.CreatePullCredential(ctx, acrBinding, serviceAccount) + dockerConfig, expiresOn, err := r.CreatePullCredential(ctx, logger, acrBinding, serviceAccount) if err != nil { logger.Info(err.Error()) return &action[O]{updatePullBindingStatus: r.UpdateStatusError(acrBinding, err.Error())} diff --git a/pkg/authorizer/token_retriever.go b/pkg/authorizer/token_retriever.go index ecad4ec..acaaf37 100644 --- a/pkg/authorizer/token_retriever.go +++ b/pkg/authorizer/token_retriever.go @@ -9,6 +9,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/go-logr/logr" msiacrpullv1beta2 "github.com/Azure/msi-acrpull/api/v1beta2" ) @@ -31,7 +32,7 @@ func AcquireARMToken(ctx context.Context, id azidentity.ManagedIDKind) (azcore.A return cred.GetToken(ctx, policy.TokenRequestOptions{Scopes: []string{customARMResource}}) } -func ARMTokenForBinding(ctx context.Context, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) { +func ARMTokenForBinding(ctx context.Context, logger logr.Logger, spec msiacrpullv1beta2.AcrPullBindingSpec, tenantId, clientId, serviceAccountToken string) (azcore.AccessToken, error) { env := environment(spec.ACR.Environment, spec.ACR.CloudConfig) var credential azcore.TokenCredential @@ -41,11 +42,16 @@ func ARMTokenForBinding(ctx context.Context, spec msiacrpullv1beta2.AcrPullBindi var id azidentity.ManagedIDKind if spec.Auth.ManagedIdentity.ClientID != "" { id = azidentity.ClientID(spec.Auth.ManagedIdentity.ClientID) + logger.Info("Attempting token exchange through IMDS using managed identity", "clientID", spec.Auth.ManagedIdentity.ClientID) } else if spec.Auth.ManagedIdentity.ResourceID != "" { id = azidentity.ResourceID(spec.Auth.ManagedIdentity.ResourceID) + logger.Info("Attempting token exchange through IMDS using managed identity", "resourceID", spec.Auth.ManagedIdentity.ResourceID) + } else { + logger.Info("Attempting token exchange through IMDS using system-assigned managed identity") } credential, err = azidentity.NewManagedIdentityCredential(&azidentity.ManagedIdentityCredentialOptions{ID: id}) case spec.Auth.WorkloadIdentity != nil: + logger.Info("Attempting token exchange using workload identity", "tenantID", tenantId, "clientID", clientId) // n.b. the built-in azidentity.WorkloadIdentityCredential assumes we're loading a service account token // from a file in a Pod, where the Kubernetes API server is rotating it, etc. Unfortunately that is not // our use-case here, and we certainly don't want to centralize every service account token we ever mint From c5c9886316ee51e701a51e37fb1965eab9d924e3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Jun 2025 22:38:02 +0000 Subject: [PATCH 3/3] Format code and finalize identity logging implementation Co-authored-by: matucker-msft <77026928+matucker-msft@users.noreply.github.com> --- internal/controller/acrpullbinding_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/acrpullbinding_controller_test.go b/internal/controller/acrpullbinding_controller_test.go index eeffb72..6aa754b 100644 --- a/internal/controller/acrpullbinding_controller_test.go +++ b/internal/controller/acrpullbinding_controller_test.go @@ -834,7 +834,7 @@ func Test_ACRPullBindingController_reconcile(t *testing.T) { referencingServiceAccounts: []corev1.ServiceAccount{ { ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "delegate"}, - ImagePullSecrets: []corev1.LocalObjectReference{{Name: "binding-msi-acrpull-secret"},{Name: "previous-binding-msi-acrpull-secret"},{Name: "extraneous"}}, + ImagePullSecrets: []corev1.LocalObjectReference{{Name: "binding-msi-acrpull-secret"}, {Name: "previous-binding-msi-acrpull-secret"}, {Name: "extraneous"}}, }, }, pullSecrets: []corev1.Secret{{