feat(AzureDNS): Add support for Workload Identity

[weisdd]

Pull Request Motivation

This PR brings support for Workload Identity to AzureDNS issuer.

There's another PR #5308, which also introduces support for Workload Identity, though it has two drawbacks:

• It doesn't handle access token renewal:
  • A federated token is renewed every hour by default (it's supposed to be re-read from disk), whereas AAD token received in exchange - every 24h.
  • adal doesn't read federated tokens from disk itself and doesn't expose any methods to dynamically replace jwt. Thus, token renewal would fail due to stale federated token;
• It supports only one Client ID:
  • As it was explained in details by @pinkfloydx33, Workload Identity supports scenarios where you map multiple MSIs to one service account, so you could have fine-grained access per each resolver.

@karlschriek, the author of that PR, does not currently have enough time to fix those, so we agreed that I'll drive it all further (discussion).

Closes: #5085

Kind

/kind feature

Release Note

Add support for Workload Identity to AzureDNS resolver

Extra information

The feature can be tested by deploying the updated image with these parameters supplied to the official helm chart:

# Needed only if Issuer is used. ClusterIssuer has ambient flag enabled by default.
extraArgs:
  - --issuer-ambient-credentials

serviceAccount:
  annotations:
    azure.workload.identity/client-id: <YOUR-CLIENT-ID>
  labels:
    azure.workload.identity/use: "true"

ClientID can be overridden through Issuer or ClusterIssuer:

[...]
    solvers:
    - dns01:
        azureDNS:
          subscriptionID: AZURE_SUBSCRIPTION_ID
          resourceGroupName: AZURE_DNS_ZONE_RESOURCE_GROUP
          hostedZoneName: AZURE_DNS_ZONE
          environment: AzurePublicCloud
          managedIdentity:
            clientID: <YOUR-CLIENT-ID>

Running tool: /usr/local/go/bin/go test -timeout 30s -coverprofile=/var/folders/rt/68xkylrx55j9zxpvxrggn53h0000gn/T/vscode-goTJGj6K/go-code-cover github.com/cert-manager/cert-manager/pkg/issuer/acme/dns/azuredns

ok  	github.com/cert-manager/cert-manager/pkg/issuer/acme/dns/azuredns	0.295s	coverage: 20.6% of statements

[jetstack-bot]

Hi @weisdd. Thanks for your PR.

I'm waiting for a cert-manager 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.

[weisdd]

@wallrj you might want to review this PR as well since you worked on #5308
Please, let me know if I should already open a PR for the documentation in https://github.com/cert-manager/website
Thanks!

@pinkfloydx33 Not sure what's the best way to build images of cert-manager. I did the following:

1. In make/git.mk, replaced:

RELEASE_VERSION := $(shell git describe --tags --match='v*' --abbrev=14)

to:

RELEASE_VERSION := "v1.10.0-wi"

2. Built and pushed the image:

make cert-manager-controller-linux
docker tag cert-manager-controller-amd64:v1.10.0-wi quay.io/weisdd/cert-manager-controller:v1.10.0-wi
docker push quay.io/weisdd/cert-manager-controller:v1.10.0-wi

Unless you want to build your own image, you can use mine: quay.io/weisdd/cert-manager-controller:v1.10.0-wi :)

[pinkfloydx33]

I am dealing with hurricane and spotty internet. I cannot review nor test until probably Friday. I can use your image though, that will be easiest. Thanks

[pinkfloydx33]

LGTM.

Verified in my sandbox cluster:

• Token refresh over 24/48 hours
• AZWI ClientId from environment variable only
• AZWI ClientId from azureDNS.managedIdentity.clientID only
• AZWI ClientId set as environment variable but overridden via azureDNS.managedIdentity.clientID

All cases behave as expected.

[weisdd]

@pinkfloydx33 great to see that you survived the hurricane, and thanks for the tests! :)

[pinkfloydx33]

Glad to help....Now now we need to get this approved! /fingers-crossed

[weisdd]

@wallrj @inteon Please, let me know what is the current estimate on when you could potentially review the PR. - Any timeline is fine for me, it just a matter of setting some expectations.

[sathyanarays]

A general question. Don't we have to make any changes in the CRDs?

[weisdd]

@sathyanarays I think there's no reason for that as the implementation suggested in the PR fits the existing "ambient-credentials" model very well. - It's of the same nature, and Workload Identity is considered only in case a unique environment variable is present (AZURE_FEDERATED_TOKEN_FILE), which gets added to a container by Azure AD Workload Identity mutating webhook in case a special label (azure.workload.identity/use: true) is part of a service account's metadata. Thus, the process is straightforward and predictable.

[inteon]

@weisdd looks great! Would it be possible to also add a test (I would like to see the refreshFunc logic tested)?
/ok-to-test

[weisdd]

@inteon thanks :)
I think it should be feasible to mock requests to OauthConfig.TokenEndpoint with the help of httptest, I'll look into that.

[weisdd]

@inteon I've added the test. It's a bit monstrous in terms of its size, but that helped to cover token refresh, clientID overrides, and tenantID value.

/usr/local/go/bin/go test -timeout 30s -v github.com/cert-manager/cert-manager/pkg/issuer/acme/dns/azuredns 
=== RUN   TestLiveAzureDnsPresent
    azuredns_test.go:53: skipping live test
--- SKIP: TestLiveAzureDnsPresent (0.00s)
=== RUN   TestLiveAzureDnsCleanUp
    azuredns_test.go:64: skipping live test
--- SKIP: TestLiveAzureDnsCleanUp (0.00s)
=== RUN   TestInvalidAzureDns
--- PASS: TestInvalidAzureDns (0.00s)
=== RUN   TestGetAuthorizationFederatedSPT
=== RUN   TestGetAuthorizationFederatedSPT/token_refresh
=== RUN   TestGetAuthorizationFederatedSPT/clientID_overrides_through_managedIdentity_section
--- PASS: TestGetAuthorizationFederatedSPT (0.00s)
    --- PASS: TestGetAuthorizationFederatedSPT/token_refresh (0.00s)
    --- PASS: TestGetAuthorizationFederatedSPT/clientID_overrides_through_managedIdentity_section (0.00s)
PASS
ok      github.com/cert-manager/cert-manager/pkg/issuer/acme/dns/azuredns       0.341s

[wallrj]

Thanks @weisdd for picking up this work!

And apologies for the delayed review.

I've been staring at this code today and think I'm finally understanding it.
Here are a few initial questions and comments.
I'll try and test it tomorrow.

[wallrj]

Please elaborate on this?
I looked at the default refresh function and it appears to use the RefreshToken that accompanies the latest AccessToken to update the AccessToken before it expires.
Why is that not desirable? Add some further explanation to the comment.

I also went looking at the azure-workload-identity msal-go example for inspiration and noticed a coupe of things:

1. They used to use autorest.NewBearerAuthorizerCallback with a callback to load the K8S service account token from the filesystem. Does that solve the problem of missing "methods to dynamically replace a federated token" more elegantly than this?
2. That example has recently been updated to to use azidentity and azsecrets sdk : "removing the dependency on autorest and switching to use track 2 sdks". Would it make sense to do that here too? I note that the autorest library is deprecated. In fact I think it's been suggested in the previous PR that we should do that first:

• Update Azure SDK and remove deprecated autorest dependency #5452

[weisdd]

No worries, thanks for the review! :)

Please elaborate on this?
I looked at the default refresh function and it appears to use the RefreshToken that accompanies the latest AccessToken to update the AccessToken before it expires.
Why is that not desirable? Add some further explanation to the comment.
Well, as I understand, there's no concept of RefreshToken for Workload Identity. An app is always supposed to exchange a federated token for an access token. Before working on the PR for cert-manager, I prepared a similar patch for external-dns (sadly, stays unreviewed), and while doing so, I dumped both ServicePrincipalToken (spt in code) and an access token (returned by newSPT.Token()), in both cases RefreshToken is empty.

1. They used to use autorest.NewBearerAuthorizerCallback with a callback to load the K8S service account token from the filesystem. Does that solve the problem of missing "methods to dynamically replace a federated token" more elegantly than this?
Just by looking through the code, I'm not sure if it offers renewal. Something to be tested for more than 24h (AAD token will expire after a day).
I like my current code for the fact that it doesn't go too deep into implementation internals of the library, it rather acts as a simple wrapper. Though, I'm not a software engineer (SRE), so I'd trust your taste if you suggest to go into a different direction.

2. That example has recently been updated to to Azure/azure-workload-identity#639 : "removing the dependency on autorest and switching to use track 2 sdks". Would it make sense to do that here too? I note that the autorest library is deprecated. In fact I think it's been suggested in the previous PR that we should do that first:
#5452
Indeed, the library will be maintained only till April next year, so, in the long run, cert-manager should definitely migrate away from it.
And I saw the PR, but there's no estimate on when it'll get merged, so I hoped that an incremental change like in my PR will make it into the code sooner. And, actually, azure-sdk-for-go will also require a workaround for Workload Identity at the moment as the native supported will go beta only in December or January: Azure/azure-sdk-for-go#15615

[weisdd]

Added more comments as requested.

[wallrj]

Calling Refresh sends the K8S ServiceAccountToken (JWT) to Azure's Oauth endpoint and gets back an Oauth AccessToken and RefreshToken, right?

Maybe add an additional comment explaining that this results in a roundtrip to the Azure auth API.

[weisdd]

When there's a special label in place (e.g. on a ServiceAccount: azure.workload.identity/use: "true"), a mutation webhook modifies a pod's spec to add a projected volume with a federated token. The path to the token is always the same, it's just a concatenation of these two constants:

	TokenFilePathName             = "azure-identity-token"
	TokenFileMountPath            = "/var/run/secrets/azure/tokens" // #nosec

https://github.com/Azure/azure-workload-identity/blob/231c6be82969efb51f0539397e6ab268c1eb8eca/pkg/webhook/consts.go#L54
To avoid hardcoding, the apps are supposed to find it through AZURE_FEDERATED_TOKEN_FILE env.
The token gets updated once an hour by default.

This token is sent to the Oauth endpoint. Response contains only an AccessToken (valid for 24h), RefreshToken is not present.

[weisdd]

Added more comments as requested.

[wallrj]

It took me a while to realise that the clientID parameter will be empty in the case of ambient credentials, but that in that case, the clientID may be supplied via the managedIdentity parameter.

[weisdd]

It's just the same process as for pod managed identities :)

[wallrj]

Thanks @weisdd

I've finally got it working and I've been documenting the steps here:

• An Azure AKS + Let's Encrypt Tutorial website#1120

I will do some more testing before adding a lgtm

[wallrj]

Thanks everyone for your efforts.

I tested this and wrote a short tutorial explaining how I test it

• An Azure AKS + Let's Encrypt Tutorial website#1120

It works well in my simple tests and I trust that you guys have done your own testing.

Let's merge it, I'll create another cert-manager alpha release so that more people can test the feature more easily,
and in particular I'd like other to test the existing AAD ambient credentials feature,
because I couldn't get that working, probably because I don't know AKS well enough.

/lgtm
/approve

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj, weisdd

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 [wallrj]

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

[wallrj]

/retest

[wallrj]

/kind feature

[wallrj]

@weisdd @pinkfloydx33 @karlschriek FYI This feature is included in our latest pre-release. Please do some testing if you have time:

• https://github.com/cert-manager/cert-manager/releases/tag/v1.11.0-alpha.1

[pinkfloydx33]

Thanks. Wont be able to to do anything until tomorrow at the earliest, but will definitely test it out

[pinkfloydx33]

@wallrj Alpha image working great/as expected. Thanks everyone who put effort into this!