-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat(AzureDNS): Add support for Workload Identity #5570
Changes from 3 commits
741fa3c
efae037
964f4bb
df20fcd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ package azuredns | |
import ( | ||
"context" | ||
"fmt" | ||
"os" | ||
"strings" | ||
|
||
"github.com/go-logr/logr" | ||
|
@@ -71,6 +72,35 @@ func NewDNSProviderCredentials(environment, clientID, clientSecret, subscription | |
}, nil | ||
} | ||
|
||
// getFederatedSPT prepares an SPT for a Workload Identity-enabled setup | ||
func getFederatedSPT(env azure.Environment, options adal.ManagedIdentityOptions) (*adal.ServicePrincipalToken, error) { | ||
// NOTE: all related environment variables are described here: https://azure.github.io/azure-workload-identity/docs/installation/mutating-admission-webhook.html | ||
oauthConfig, err := adal.NewOAuthConfig(env.ActiveDirectoryEndpoint, os.Getenv("AZURE_TENANT_ID")) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to retrieve OAuth config: %v", err) | ||
} | ||
|
||
jwt, err := os.ReadFile(os.Getenv("AZURE_FEDERATED_TOKEN_FILE")) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to read a file with a federated token: %v", err) | ||
} | ||
|
||
// AZURE_CLIENT_ID will be empty in case azure.workload.identity/client-id annotation is not set | ||
// Also, some users might want to use a different MSI for a particular DNS zone | ||
// Thus, it's important to offer optional ClientID overrides | ||
clientID := os.Getenv("AZURE_CLIENT_ID") | ||
if options.ClientID != "" { | ||
clientID = options.ClientID | ||
} | ||
|
||
token, err := adal.NewServicePrincipalTokenFromFederatedToken(*oauthConfig, clientID, string(jwt), env.ResourceManagerEndpoint) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to create a workload identity token: %v", err) | ||
} | ||
|
||
return token, nil | ||
} | ||
|
||
func getAuthorization(env azure.Environment, clientID, clientSecret, subscriptionID, tenantID string, ambient bool, managedIdentity *cmacme.AzureManagedIdentity) (*adal.ServicePrincipalToken, error) { | ||
if clientID != "" { | ||
logf.Log.V(logf.InfoLevel).Info("azuredns authenticating with clientID and secret key") | ||
|
@@ -84,7 +114,7 @@ func getAuthorization(env azure.Environment, clientID, clientSecret, subscriptio | |
} | ||
return spt, nil | ||
} | ||
logf.Log.V(logf.InfoLevel).Info("No ClientID found: authenticating azuredns with managed identity (MSI)") | ||
logf.Log.V(logf.InfoLevel).Info("No ClientID found: attempting to authenticate with ambient credentials (Azure Workload Identity or Azure Managed Service Identity, in that order)") | ||
if !ambient { | ||
return nil, fmt.Errorf("ClientID is not set but neither `--cluster-issuer-ambient-credentials` nor `--issuer-ambient-credentials` are set. These are necessary to enable Azure Managed Identities") | ||
} | ||
|
@@ -96,6 +126,39 @@ func getAuthorization(env azure.Environment, clientID, clientSecret, subscriptio | |
opt.IdentityResourceID = managedIdentity.ResourceID | ||
} | ||
|
||
// Use Workload Identity if present | ||
if os.Getenv("AZURE_FEDERATED_TOKEN_FILE") != "" { | ||
spt, err := getFederatedSPT(env, opt) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
// adal does not offer methods to dynamically replace a federated token, thus we need to have a wrapper to make sure | ||
// we're using up-to-date secret while requesting an access token | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please elaborate on this? I also went looking at the azure-workload-identity msal-go example for inspiration and noticed a coupe of things:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, thanks for the review! :) Please elaborate on this? 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 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added more comments as requested. |
||
var refreshFunc adal.TokenRefresh = func(context context.Context, resource string) (*adal.Token, error) { | ||
newSPT, err := getFederatedSPT(env, opt) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
// Need to call Refresh(), otherwise .Token() will be empty | ||
err = newSPT.Refresh() | ||
if err != nil { | ||
return nil, err | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling Maybe add an additional comment explaining that this results in a roundtrip to the Azure auth API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When there's a special label in place (e.g. on a
https://github.com/Azure/azure-workload-identity/blob/231c6be82969efb51f0539397e6ab268c1eb8eca/pkg/webhook/consts.go#L54 This token is sent to the Oauth endpoint. Response contains only an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added more comments as requested. |
||
|
||
accessToken := newSPT.Token() | ||
|
||
return &accessToken, nil | ||
} | ||
|
||
spt.SetCustomRefreshFunc(refreshFunc) | ||
|
||
return spt, nil | ||
} | ||
|
||
logf.Log.V(logf.InfoLevel).Info("No Azure Workload Identity found: attempting to authenticate with an Azure Managed Service Identity (MSI)") | ||
|
||
spt, err := adal.NewServicePrincipalTokenFromManagedIdentity(env.ServiceManagementEndpoint, &opt) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to create the managed service identity token: %v", err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 themanagedIdentity
parameter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just the same process as for pod managed identities :)