Use azidentity instead of ADAL #2748

r4f4 · 2022-10-21T23:30:46Z

ADAL is being deprecated, so use azidentity with an adapter so the new authentication can work with v1 SDK clients. The only impact for users is to update the environment variables used for certificate authentication, from AZURE_CERTIFICATE_ to AZURE_CLIENT_CERTIFICATE_.

Signed-off-by: Rafael Fonseca [email protected]

What type of PR is this?
/kind other

What this PR does / why we need it: ADAL is being deprecated, so use azidentity with an adapter so the new authentication can work with v1 SDK clients.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Replace deprecated ADAL authentication with MSAL.
action required: for users relying on certificate authentication, the environment variables used by the libraries changed from AZURE_CERTIFICATE_ to AZURE_CLIENT_CERTIFICATE_

k8s-ci-robot · 2022-10-21T23:30:48Z

@r4f4: The label(s) kind/other cannot be applied, because the repository doesn't have them.

In response to this:

ADAL is being deprecated, so use azidentity with an adapter so the new authentication can work with v1 SDK clients.

Signed-off-by: Rafael Fonseca [email protected]

What type of PR is this?
/kind other

What this PR does / why we need it: ADAL is being deprecated, so use azidentity with an adapter so the new authentication can work with v1 SDK clients.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits

includes documentation

adds unit tests

Release note:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-10-21T23:30:54Z

Welcome @r4f4!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-10-21T23:30:55Z

Hi @r4f4. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

r4f4 · 2022-10-24T19:56:48Z

/release-note-none

mboersma · 2022-10-28T19:07:20Z

/ok-to-test

CecileRobertMichon · 2022-11-02T05:27:19Z

azure/scope/identity.go

 	"github.com/Azure/go-autorest/autorest"
-	"github.com/Azure/go-autorest/autorest/adal"
+	"github.com/jongio/azidext/go/azidext"


what is this repo? it seems that it's in someone's personal GitHub org?

should we be following https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azidentity/MIGRATION.md instead?

It's what I did. See the section https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azidentity/MIGRATION.md#use-azidentity-credentials-with-older-packages where they recommend jongio's repo for getting an adapter so the new authorizer can work with older SDK clients.

Got it, thanks for clarifying

sonasingh46 · 2022-11-17T12:26:14Z

This would be nice to get in. I have mentioned about this in workload identity proposal as a future work -- but prior work is fine.
#2814

With azure workload identity, it seem we have to have use azidentity.

azure/scope/identity.go

sonasingh46 · 2022-11-17T12:30:29Z

azure/scope/identity.go

@@ -136,44 +139,50 @@ func (p *ManagedControlPlaneCredentialsProvider) GetAuthorizer(ctx context.Conte

 // GetAuthorizer returns an Azure authorizer based on the provided azure identity and cluster metadata.
 func (p *AzureCredentialsProvider) GetAuthorizer(ctx context.Context, resourceManagerEndpoint, activeDirectoryEndpoint string, clusterMeta metav1.ObjectMeta) (autorest.Authorizer, error) {
-	var spt *adal.ServicePrincipalToken
+	var authErr error
+	var cred azcore.TokenCredential


Any particular reason for declaring these variables explicitly vs using shorthand in the switch case?

e.g

cred, err := azidentity.NewManagedIdentityCredential(&options)

Yes. If I don't do this then I can't use them on lines https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2748/files#diff-b6ab207506064afcbd1d777c1c7296e2953d2653a8843bfb39107af5cf5b0a87R174-R177.

The alternative would be to duplicate those lines in each case branch, which I tried to avoid.

aramase · 2022-11-17T19:00:24Z

azure/scope/identity.go

+	if len(endpoint) > 0 && endpoint[len(endpoint)-1] != '/' {
+		endpoint += "/"
+	}


Why is this additional / added here? resourceManagerEndpoint in the GetAuthorizer call should already have the exact url that the token needs to be requested for? FYI, adding an extra slash or removing an existing one can render the token unusable because the resource identifier has to be an exact match.

xref: https://github.com/Azure/azure-workload-identity/blob/main/pkg/proxy/proxy.go#L194-L203

I was just looking at #2814 and found a ref to this, so figured i'll review it. I didn't mean to lurk 😆

Thanks for the review. I've changed the code to be similar to the one you linked.

sonasingh46 · 2022-11-23T08:53:55Z

/retest

sonasingh46 · 2022-11-23T09:02:16Z

@r4f4 -- The PR is still titled WIP. Do you see any more work that needs to be done? Also, let me know if you need some help on getting this to a merge state.
Thanks for doing this. It will be really helpful in getting the azwi integration.

azure/scope/identity.go

r4f4 · 2022-11-23T09:24:36Z

@r4f4 -- The PR is still titled WIP. Do you see any more work that needs to be done? Also, let me know if you need some help on getting this to a merge state. Thanks for doing this. It will be really helpful in getting the azwi integration.

I just noticed that this code https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/azure/scope/clients.go#L96-L98 needs to be changed as well.

auth.EnvironmentSettings.GetAuthorizer docs says it creates an authorizer in the order "Client credentials, Client certificate, Username password, MSI". That's very similar to the DefaultAzureCredential [1] which tries even more auth options: "Environment (client secret, certificate, user/pwd), ManagedIdentity, AzureCli". Here we can already notice a change in the username and MSI orders, not sure if that's a problem.

Now what is a problem is that autorest uses different env vars from MSAL for client certificate authentication. autorest uses AZURE_CERTIFICATE_ [2] whereas MSAL uses AZURE_CLIENT_CERTIFICATE_ [3]. I'm still not sure what to do about that.

[1] https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azidentity#DefaultAzureCredential
[2] https://github.com/azure/go-autorest/blob/v12.0.0/autorest/azure/auth/auth.go#L45-L46
[3] https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azidentity#hdr-Service_principal_with_certificate

sonasingh46 · 2022-11-23T10:09:36Z

[1] Re the env variables -- if MSAL uses a different env var name that will mean to communicate this to users to set up a env variable with the new name if a capz version containing this change is used or upgraded to.
I don't think this is something that can be or should be controlled from within capz.

[2] I do not see ordering as a problem too.

But it is worth to put these in release note and as well as in PR description.

@aramase @CecileRobertMichon Any thoughts?

r4f4 · 2022-12-14T11:19:32Z

Rebased and added the comment as requested.

sonasingh46 · 2022-12-14T13:19:50Z

/retest

sonasingh46 · 2022-12-14T17:06:11Z

/retest

sonasingh46 · 2022-12-14T18:51:06Z

/lgtm
/approve

CecileRobertMichon · 2022-12-14T18:58:29Z

Are we keeping the 3 distinct commits intentionally or would it make sense to squash those?

sonasingh46 · 2022-12-14T19:04:19Z

I think that should be squashed and @r4f4 kept it for easy reviews. @CecileRobertMichon

@r4f4 -- Please squash the commits.

r4f4 · 2022-12-14T19:10:05Z

Squashed commits.

sonasingh46 · 2022-12-14T21:03:47Z

/retest

test/e2e/common.go

CecileRobertMichon

just a minor code cleanup comment from me, otherwise LGTM

ADAL is being deprecated, so use azidentity with an adapter so the new authentication can work with v1 SDK clients. Note that for certificate authentication, autorest uses the `AZURE_CERTIFICATE_` prefix for environment variables whereas azidentity uses `AZURE_CLIENT_CERTIFICATE_`. We make sure the latter is set to the value of the former so as not to break upgrades. Signed-off-by: Rafael Fonseca <[email protected]>

CecileRobertMichon · 2022-12-14T23:42:14Z

/test pull-cluster-api-provider-azure-e2e-optional

CecileRobertMichon · 2022-12-14T23:42:20Z

/lgtm

sonasingh46 · 2022-12-15T16:18:18Z

/lgtm
/approve

mboersma

/lgtm
/approve

k8s-ci-robot · 2022-12-15T16:28:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma, sonasingh46

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mboersma]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

CecileRobertMichon · 2022-12-15T18:45:26Z

/retest

CecileRobertMichon · 2022-12-15T20:21:03Z

another flake?

/retest

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2022

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 21, 2022

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2022

k8s-ci-robot requested review from jackfrancis and sonasingh46 October 21, 2022 23:31

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 24, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 28, 2022

CecileRobertMichon reviewed Nov 2, 2022

View reviewed changes

sonasingh46 reviewed Nov 17, 2022

View reviewed changes

azure/scope/identity.go Outdated Show resolved Hide resolved

sonasingh46 reviewed Nov 17, 2022

View reviewed changes

aramase reviewed Nov 17, 2022

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 17, 2022

sonasingh46 reviewed Nov 23, 2022

View reviewed changes

azure/scope/identity.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 23, 2022

k8s-ci-robot assigned mboersma Dec 14, 2022

r4f4 force-pushed the kube-adal-azid branch from 598d788 to da321af Compare December 14, 2022 11:18

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 14, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2022

r4f4 force-pushed the kube-adal-azid branch from da321af to 0ee717e Compare December 14, 2022 19:09

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2022

CecileRobertMichon reviewed Dec 14, 2022

View reviewed changes

test/e2e/common.go Outdated Show resolved Hide resolved

CecileRobertMichon reviewed Dec 14, 2022

View reviewed changes

r4f4 force-pushed the kube-adal-azid branch from 0ee717e to 73ac818 Compare December 14, 2022 23:39

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2022

mboersma approved these changes Dec 15, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2022

jackfrancis added this to the v1.7 milestone Dec 15, 2022

k8s-ci-robot merged commit d20fe73 into kubernetes-sigs:main Dec 15, 2022

Use azidentity instead of ADAL #2748

Use azidentity instead of ADAL #2748

Conversation

r4f4 commented Oct 21, 2022 • edited Loading

k8s-ci-robot commented Oct 21, 2022

k8s-ci-robot commented Oct 21, 2022

k8s-ci-robot commented Oct 21, 2022

r4f4 commented Oct 24, 2022

mboersma commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonasingh46 commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonasingh46 commented Nov 23, 2022

sonasingh46 commented Nov 23, 2022

r4f4 commented Nov 23, 2022

sonasingh46 commented Nov 23, 2022

r4f4 commented Dec 14, 2022

sonasingh46 commented Dec 14, 2022

sonasingh46 commented Dec 14, 2022

sonasingh46 commented Dec 14, 2022

CecileRobertMichon commented Dec 14, 2022

sonasingh46 commented Dec 14, 2022

r4f4 commented Dec 14, 2022

sonasingh46 commented Dec 14, 2022

CecileRobertMichon left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented Dec 14, 2022

CecileRobertMichon commented Dec 14, 2022

sonasingh46 commented Dec 15, 2022

mboersma left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 15, 2022

CecileRobertMichon commented Dec 15, 2022

CecileRobertMichon commented Dec 15, 2022

r4f4 commented Oct 21, 2022 •

edited

Loading