Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Aashish93-stack opened this issue Nov 29, 2022 · 22 comments
Assignees
Labels
feature All issues for new features that have been committed to

Comments

@Aashish93-stack
Copy link

Proposal

Proposing the following updates:

  • Add a provider for azure service principal
  • Update the AuthPodIdentity, triggerAuthentication to take in a ClientID, Audience and Secret
  • Adding mechanism to load the base64 decoded Kubernetes secret
  • Adding a mechanism to decode the certificate and get the AadToken

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 Aashish93-stack added feature-request All issues for new features that have not been committed to needs-discussion labels Nov 29, 2022
@Aashish93-stack
Copy link
Author

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

@JorTurFer JorTurFer changed the title Adding support for authenticating with service principal using certificate stored as a Kubernetes secret Azure Event Hub: Adding support for authenticating with service principal using certificate stored as a Kubernetes secret Nov 29, 2022
@JorTurFer
Copy link
Member

JorTurFer commented Nov 29, 2022

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
Copy link
Author

Aashish93-stack commented Nov 30, 2022

@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
Copy link
Member

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
Copy link
Member

JorTurFer commented Dec 2, 2022

Agreed with @tomkerkhove

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

@tomkerkhove
Copy link
Member

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

@Aashish93-stack
Copy link
Author

@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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

WDYT @v-shenoy ?

@v-shenoy
Copy link
Contributor

v-shenoy commented Dec 6, 2022

WDYT @v-shenoy ?

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

@JorTurFer
Copy link
Member

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

@v-shenoy
Copy link
Contributor

v-shenoy commented Dec 6, 2022

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
Copy link
Member

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

@tomkerkhove tomkerkhove changed the title Azure Event Hub: Adding support for authenticating with service principal using certificate stored as a Kubernetes secret Provide support for using Azure AD service principal authentication with client secret/certificate Dec 6, 2022
@tomkerkhove
Copy link
Member

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:

  • azureServicePrincipal is a bit vague imo, I would rename it to azureActiveDirectoryIdentity/azureActiveDirectoryApplication/azureActiveDirectoryServicePrincipal
  • I'd like to support both certificate and client secret which could be coming from kubernetes secret or file. I don't think Azure Key Vault should be in the initial scope because we'd need to know how to authenticate to that then etc while you could argue that we support KV as a 1P provider and they should use that maybe? I don't know; I think it might be a bit much for initial scenario
  • I'd suggest to rename clientId to align with Azure AD's terminology which means appId or applicationId would be a better fit. A lot of end-users in Azure space always get confused with clientId.

@Aashish93-stack
Copy link
Author

@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
Copy link
Member

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
Copy link

stale bot commented Feb 4, 2023

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.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Feb 4, 2023
@Aashish93-stack
Copy link
Author

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

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Feb 4, 2023
@tomkerkhove
Copy link
Member

No worries!

@stale
Copy link

stale bot commented Apr 7, 2023

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.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Apr 7, 2023
@tomkerkhove tomkerkhove added feature All issues for new features that have been committed to and removed feature-request All issues for new features that have not been committed to stale All issues that are marked as stale due to inactivity labels Apr 13, 2023
@radekfojtik
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature All issues for new features that have been committed to
Projects
Status: To Do
Development

No branches or pull requests

5 participants