kedacore / keda

KEDA is a Kubernetes-based Event Driven Autoscaling component. It provides event driven scale for any container running in Kubernetes
https://keda.sh
Apache License 2.0
8.21k stars 1.04k forks source link

Provide support for using Azure AD service principal authentication with client secret/certificate #3933

Open Aashish93-stack opened 1 year ago

Aashish93-stack commented 1 year ago

Proposal

Proposing the following updates:

Use-Case

We have a use case where we need to support System Assigned Identity for accessing eventhub and since PodIdentity and Workload Identities don't support System-Assigned Identity out of box, we are looking into service-principal based authentication using certificates

Anything else?

No response

Aashish93-stack commented 1 year ago

I'd be happy to contribute this feature to the project.

JorTurFer commented 1 year ago

Hi @Aashish93-stack You can use Workload Identities with the service principal, doesn't it solve your problem? I mean, if you already have a service principal, you can federate it with the cluster and use Workload Identity using that service principal without any secret/certificate

Aashish93-stack commented 1 year ago

@JorTurFer unfortunately, there are some use cases where we don't have access to the service principal to configure federation and other cases where users don't won't to onboard the workload-identity, so we are hoping to add a certificate based authentication to cover all our bases

tomkerkhove commented 1 year ago

Yes, this is another way of authenticating.

I think service principal support makes sense but I'd suggest to post a configuration proposal here first to make sure we are all aligned first.

Is that OK for you @Aashish93-stack?

JorTurFer commented 1 year ago

Agreed with @tomkerkhove

BTW, There is also the option of using User Managed Identities if you need them as WI already supports them

tomkerkhove commented 1 year ago

BUt then you still need Workload Identity :) In some scenarios you just want to use a SP.

Aashish93-stack commented 1 year ago

@tomkerkhove sure that works for me.

Under the triggerAunthentication I was thinking of adding a provider azure-service-principal and it would have the following structure

apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: {trigger-authentication-name}
  namespace: {trigger-authentication-namespace}
spec:
  podIdentity:
     provider: azure-service-principal
     clientId: {{UUID}}
     tenantId: {{UUID}}
     audience: {{audience}}
 secretTargetRef:               #Optional: Required when using azureServicePrincipal and not Secret from Azure KeyVault
    - parameter: {{secretRef}}
      name: {{secret-name}}
      key: {secret-key-name}
 azureKeyVault:                #Optional: Required when using azureServicePrincipal and not referencing Secret from Kubernetes                 
      vaultURI: {key-vault-address}                                        
      credentials:                                                          
          clientId: {azure-ad-client-id}                                      
          clientSecret:                                                       
             valueFrom:                                                        
                secretKeyRef:                                                   
                   name: {k8s-secret-with-azure-ad-secret}                       
                   key: {key-within-the-secret}                                  
          tenantId: {azure-ad-tenant-id}   

I was thinking that the certificate can imported from a Kubernetes secret or Azure Key Vault, so when specifying the provider as azureServicePrincipal we would also need either the secretTargetRef or azureKeyVault.

Then in the triggerauthentication_types add a new const

const (
    PodIdentityProviderAzureServicePrincipal PodIdentityProvider = "azure-service-principal" // New const for service-principal
    PodIdentityProviderNone                          PodIdentityProvider = "none"
    PodIdentityProviderAzure                          PodIdentityProvider = "azure"
    PodIdentityProviderAzureWorkload          PodIdentityProvider = "azure-workload"
    PodIdentityProviderGCP                            PodIdentityProvider = "gcp"
    PodIdentityProviderSpiffe                         PodIdentityProvider = "spiffe"
    PodIdentityProviderAwsEKS                      PodIdentityProvider = "aws-eks"
    PodIdentityProviderAwsKiam                    PodIdentityProvider = "aws-kiam"
)

and update the AuthPodIdentity struct with new optional fields

type AuthPodIdentity struct {
    Provider PodIdentityProvider `json:"provider"`
    // +optional
    IdentityID string `json:"identityId"`
    // +optional
    ClientID string `json:"clientId"`
    // +optional
    Audience string `json:"audience"`
    // +optional
    TenantID string `json:"tenantId"`
}

Also, we need to create a new class (could be name azure_aad_service_principal.go) which would be responsible to for decoding the certificate and getting the servicePrincipalToken, this would be called from the individual scalers. In case of EventHubScaler, after getting the servicePrincipalToken, a new JWTProvider needs to be configured and passed in the while creating a new eventhub client

JorTurFer commented 1 year ago

I wouldn't add this as a new pod identity because it isn't. Maybe we could add another section like azureServicePrincipal or something like that. WDYT?

Aashish93-stack commented 1 year ago

Yes, that makes sense, updating trigger auth structure to this ->

apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: {trigger-authentication-name}
  namespace: {trigger-authentication-namespace}
spec:
  azureServicePrincipal:
     clientId: {{UUID}}
     tenantId: {{UUID}}
     audience: {{audience}}
 secretTargetRef:               #Optional: Required when using azureServicePrincipal and not Secret from Azure KeyVault
    - parameter: {{secretRef}}
      name: {{secret-name}}
      key: {secret-key-name}
 azureKeyVault:                #Optional: Required when using azureServicePrincipal and not referencing Secret from Kubernetes                 
      vaultURI: {key-vault-address}                                        
      credentials:                                                          
          clientId: {azure-ad-client-id}                                      
          clientSecret:                                                       
             valueFrom:                                                        
                secretKeyRef:                                                   
                   name: {k8s-secret-with-azure-ad-secret}                       
                   key: {key-within-the-secret}                                  
          tenantId: {azure-ad-tenant-id}   

and will add a new struct in the triggerauthentication_types

type AzureServicePrincipal struct {
    ClientID string `json:"clientId"`
    // +optional
    TenantID string `json:"tenantId"`
    // +optional
    Audience string `json:"audience"`
}

Since it's no longer part of the PodIdentity, was thinking of adding a AzureServicePrincipalHandler which can be used for decoding the certificate and returning the ServicePrincipalToken and this can be called by the new if block for triggerAuthSpec.AzureServicePrincipal in resolveAuthRef function under scale_resolvers.go and store the token in the authParams map, which can then be used by the individual scalers. Does this look good ?

JorTurFer commented 1 year ago

WDYT @v-shenoy ?

v-shenoy commented 1 year ago

WDYT @v-shenoy ?

Yeah, sounds good. Are we planning to add this only for Event Hub, or all Azure scalers @JorTurFer?

JorTurFer commented 1 year ago

I guess that this is interesting for all azure scalers, isn't?

v-shenoy commented 1 year ago

I guess that this is interesting for all azure scalers, isn't?

It is. I asked because the issue is titled for Event Hub.

JorTurFer commented 1 year ago

@Aashish93-stack , are you willing to contribute with other scalers too?

tomkerkhove commented 1 year ago

We should do it, I don't think this is even a question :) However, we can do it in phases if @Aashish93-stack does not have the bandwidth to update all of them.

My feedback:

Aashish93-stack commented 1 year ago

@tomkerkhove cool, the suggestions look good to me, will update it to azureActiveDirectoryServicePrincipal, support both certificate and client secret and update clientId to applicationId. For keyvault I was thinking as a 1P provider, but yeah will start out with Kubernetes secret and we can look into keyvault integration later since we may want to support 3p as well.

@JorTurFer Unfortunately, don't have much bandwidth, will try to get as many scalers I can or can do it in phases

JorTurFer commented 1 year ago

If you are willing to contribute, obviously is dependent on your availability, doing it in phases is awesome. Also, you are not forced to do in on every scaler, other contributors can take them (as you prefer 😄 )

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

Aashish93-stack commented 1 year ago

Sorry been busy with other commitments, will contribute this feature in March.

tomkerkhove commented 1 year ago

No worries!

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

radekfojtik commented 9 months ago

@Aashish93-stack are you still interested? I would possibly try to implement