Concerns around Keda Identity Implementation for Azure

[tshaiman]

following #4262 and the recent issues we had with workload Identity and token rotation
I would like to start a discussion around the implementation of Azure Identity In Keda.

• Disclosure : I haven't been a go developer for the last 3 years

• I download Keda code and look at the implementation of the Azure identity libraries ,and was a bit surprised to see many dedicated wrappers and implementation around the already exiting Azure. Identity library.
  (e.g azure_aad_workload_identity.go / azure_aad_podidentity.go )

• from my experience with Azure, a client code ( C# / Go / Java) should never know whether its Pod Identity /Workload identity / Service Principal etc.

• the Azure Identity lib let a developer use either ChainedCredentials or Default Credentials and knows how to detect you identity Paramars / environment variable : it is not clear to me why Keda implementation WRAPS the azure libraries ?
  https://learn.microsoft.com/en-us/azure/developer/go/azure-sdk-authentication?tabs=bash

• so the following code ( from aad_workload_identity.go) is raising questions , as why would it need to handle the token calls and saved them to files , when the Azure Identity Library does all the magic behind the scenes ?

// GetAzureADWorkloadIdentityToken returns the AADToken for resource
func GetAzureADWorkloadIdentityToken(ctx context.Context, identityID, resource string) (AADToken, error) {
	clientID := DefaultClientID
	if identityID != "" {
		clientID = identityID
	}

	signedAssertion, err := readJWTFromFileSystem(TokenFilePath)
	if err != nil {
		return AADToken{}, fmt.Errorf("error reading service account token - %w", err)
	}

	if signedAssertion == "" {
		return AADToken{}, fmt.Errorf("assertion can't be empty string")
	}

	cred := confidential.NewCredFromAssertionCallback(func(context.Context, confidential.AssertionRequestOptions) (string, error) {
		return signedAssertion, nil
	})

	authorityOption := confidential.WithAuthority(fmt.Sprintf("%s%s/oauth2/token", AuthorityHost, TenantID))
	confidentialClient, err := confidential.New(
		clientID,
		cred,
		authorityOption,
	)
	if err != nil {
		return AADToken{}, fmt.Errorf("error creating confidential client - %w", err)
	}

	result, err := confidentialClient.AcquireTokenByCredential(ctx, []string{getScopedResource(resource)})
	if err != nil {
		return AADToken{}, fmt.Errorf("error acquiring aad token - %w", err)
	}

• I took a simple go code for a test drive and followed MS guidelines on how to work with Auzre Identity library and manged to authenticate myself to Service bus using Azure CLI + authenticate a pod with Managed Identity to service bus using this snippet

func (s *azureServiceBusReader) getServiceBusAdminClient() (*admin.Client, error) {
	if s.client != nil {
		return s.client, nil
	}
	var err error
	var client *admin.Client
	cred, err := azidentity.NewDefaultAzureCredential(nil)
	if err != nil {
		s.logger.Error(err, "Could not create Azure Default Credentials")
	}
	client, err = admin.NewClient(s.metadata.fullyQualifiedNamespace, cred, nil)
	s.client = client
	return client, err
}

• I believe this is how Azure aimed for the user to use its libraries and not handle their token / file /JWT / expiration themselves.

• the only layer that should know whether this is PodIdentity or WorkloadIdentity is the pod definition , not the golang code
  so I'm not sure why there are dedicated switch case in the Keda code to check that .

• it could be that the reason was because historically not all libraries of Azure ( Storage Queue ? Service bus OLD libraries) did not have the option to use DefaultAzureCredentials (*a.k.a TokenCredential) so developer needed to first ask a token from AAD with scopes and then generate another calls with that token to the resource. this is no longer the case.

• I believe that simplifying the code not repeating/wrapping parts that are already implemented by the provider can make the code more maintainable , easy to use and read, and with less bugs.

looking forward to hear your opinion.

[JorTurFer]

Hey!
There are good questions, and I'm going to try to answer them :)

I download Keda code and look at the implementation of the Azure identity libraries ,and was a bit surprised to see many dedicated wrappers and implementation around the already exiting Azure. Identity library.
(e.g azure_aad_workload_identity.go / azure_aad_podidentity.go )

Yeah, sadly the golang SDKs iaren't like dotnet SDK. ServiceBus is the first one which uses the azidentity SDK, that's why we have multiple implementations around the same code, because we try to the maximum amount of code that we can. Azure-sdk-for-go is growing and I hope that soon more serivices will be avilable using that SDK.

from my experience with Azure, a client code ( C# / Go / Java) should never know whether its Pod Identity /Workload identity / Service Principal etc.

Yes, this is true for an app with a single authentication, but KEDA supports multiple authentication at the same time. That's why we require the knowledge about the authentication method. If we follow the approach for regular apps, a single identity can be used with a single authentication mechanism. For example, the issue about using different MSIs wouldn't have solution if KEDA hadn't known about the pod identity mechanism because that was the issue indeed, KEDA used the first successfully method.

the Azure Identity lib let a developer use either ChainedCredentials or Default Credentials and knows how to detect you identity Paramars / environment variable : it is not clear to me why Keda implementation WRAPS the azure libraries ?
learn.microsoft.com/en-us/azure/developer/go/azure-sdk-authentication?tabs=bash

The only wrap that we use is for msi and is because they recommended us to do it. We needed to reduce the authentication timeout like they do "magically" with DefaultAzureCredential, but they modify and internal property. We asked them and that's their suggestion, wrapping their code to apply the timeout. The other cases are the full implementation of the auth mechanism because it wasn't supported and we wanted to support pod identities even thought the old SDKs don't do it, eg: https://github.com/kedacore/keda/blob/main/pkg/scalers/azure/azure_aad_podidentity.go#L42

so the following code ( from aad_workload_identity.go) is raising questions , as why would it need to handle the token calls and saved them to files , when the Azure Identity Library does all the magic behind the scenes ?

This code isn't used in ServiceBus scaler, is used in other scalers which don't use azindeity SDK, so we need to handle the token. BTW, we aren't saving them into files, we are reading the projected volume with the service account token

I took a simple go code for a test drive and followed MS guidelines on how to work with Auzre Identity library and manged to authenticate myself to Service bus using Azure CLI + authenticate a pod with Managed Identity to service bus using this snippet

We cannot use NewDefaultAzureCredential because one of the included sources is az-cli, which tries to get the token from the shell. As docker images don't have shell, this generated an error. That's why we generate our own ChainedCredential adding it only if the shell is available. Apart from the shell thing, NewDefaultAzureCredential doesn't allow using multiple identities and/or multiple podidentity types, it takes the first one that works.

I believe this is how Azure aimed for the user to use its libraries and not handle their token / file /JWT / expiration themselves.

I think this is already answered, for new SDKs with azidentity support, we don't handle the token at all.

the only layer that should know whether this is PodIdentity or WorkloadIdentity is the pod definition , not the golang code
so I'm not sure why there are dedicated switch case in the Keda code to check that .

I think this is already answered, we need to do it because otherwise KEDA is attached to a single identity with a single mechanism. This means that the use case that you have with some scalers with AAD-pod-identity and some scalers with WI wouldn't be possible, the first match would be used. KEDA isn't a single app where you can limit the access easily. From administration POV, not having the option to defined identities or authentication mechanism could mean a lot of permission stacked in the same identity with arbitraty access from people in the cluster (the same identity would have to be used for all the teams because NewDefaultAzureCredential isn't configurable)

it could be that the reason was because historically not all libraries of Azure ( Storage Queue ? Service bus OLD libraries) did not have the option to use DefaultAzureCredentials (*a.k.a TokenCredential) so developer needed to first ask a token from AAD with scopes and then generate another calls with that token to the resource. this is no longer the case.

This is exactly the reason. Old SDKs don't support azidenity SDK

I believe that simplifying the code not repeating/wrapping parts that are already implemented by the provider can make the code more maintainable , easy to use and read, and with less bugs.

Agreed. That's why we check azure-sdk-for-go and we want to update all the scalers with the new SDK once it has support for them and that's why we actively check the new support. IMO, the new approach using azindentity is the correct approach. It's the same that MSFT uses in other languages like dotnet, but currently the ecosystem is too much heterogeneous, that's the reason for having all that code.

Now, my opinion about this:

The identity management in golang is a mess, in general, the support for azure in golang is really poor, luckily, it has been becoming better during the last months, but it's terrible. I'd like to unify all the scalers around azindetity SDK, but that means to remove the working code and create new code wrapping the azidentity code, which IMO doesn't add any value because I'd need to adapt the new code to the old SDKs, adding new possibilities of bugs during the process.
I hope that our azure_azidentity_chain will be the future because all the scaler will be migrated to the new SDK, so eventually we can remove all the manual handled tokens. In fact,, I created that chain for having a unified authentication code for all the scalers with azure pod identities. I have to add eventually we remove aad-pod-identity support because it's deprecated, that's why I have worked in the future of the pod identity code, not modifying the old code because my idea is to remove it once the new SDK support grows (and removing aad-pod-identity from azure_azidentity_chain is easy)

I hope this has clarified a bit the situation

[tshaiman]

Thank you for the detailed explanation.
I want to make sure the GetAzureADWorkloadIdentityToken is not called from Service Bus scaler , as this is a bit confusing.

when creating new adminClient the following code runs :

case kedav1alpha1.PodIdentityProviderAzure, kedav1alpha1.PodIdentityProviderAzureWorkload:
		creds, chainedErr := azure.NewChainedCredential(s.podIdentity.IdentityID, s.podIdentity.Provider)
		if chainedErr != nil {
			return nil, chainedErr
		}

then when looking at azure.NewChainedCredential :

	switch podIdentity {
	case v1alpha1.PodIdentityProviderAzure:
		// Used for aad-pod-identity
		msiCred, err := ManagedIdentityWrapperCredential(identityID)
		if err == nil {
			creds = append(creds, msiCred)
		}
	case v1alpha1.PodIdentityProviderAzureWorkload:
		wiCred, err := NewADWorkloadIdentityCredential(identityID)
		if err == nil {
			creds = append(creds, wiCred)
		}
	}

which is under azure_aad_workload_identity.go , and since it implements the Refresh interface :

// Refresh is for implementing the adal.Refresher interface
func (wiTokenProvider *ADWorkloadIdentityTokenProvider) Refresh() error {
	if time.Now().Before(wiTokenProvider.aadToken.ExpiresOnTimeObject) {
		return nil
	}

	aadToken, err := GetAzureADWorkloadIdentityToken(wiTokenProvider.ctx, wiTokenProvider.IdentityID, wiTokenProvider.Resource)
	if err != nil {
		return err
	}

I can call this GetAzureADWorkloadIdentityToken .

am I'm missing something ?

[JorTurFer]

If you check the function's body, you can see that NewADWorkloadIdentityCredential returns azidentity.WorkloadIdentityCredential, which is the new SDK implementation. There isn't any call to Refresh() from Service Bus because the new SDK uses the function GetToken() and that function is internally implemented.

func NewADWorkloadIdentityCredential(identityID string) (*azidentity.WorkloadIdentityCredential, error) {
	clientID := DefaultClientID
	if identityID != "" {
		clientID = identityID
	}
	return azidentity.NewWorkloadIdentityCredential(TenantID, clientID, TokenFilePath, &azidentity.WorkloadIdentityCredentialOptions{})
}

Refresh() is used in scalers like event hub, which don't use the new azidentity SDK.
Be care with the struct names, because the other function GetToken in the file is from the type ADWorkloadIdentityTokenProvider. If you want to see the code that ServiceBus SDK executes, you need to check the azidentity SDK

[tshaiman]

Got It !
yes , I've missed that indeed .thank you