refactor: rework aged authentication config interface #366

comtalyst · 2024-05-22T19:00:47Z

Fixes #

Description
Part of #224

How was this change tested?

Unit tests
~~E2E~~ OUTDATED, TODO
E2E for NAP (using managed identity)

Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: #
No

Release Note

- Deprecate `ARM_USE_CREDENTIAL_FROM_ENVIRONMENT` and `ARM_USE_MANAGED_IDENTITY_EXTENSION` in favor of track2-based authentication: https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azidentity/README.md#defaultazurecredential
- Rename `ARM_USER_ASSIGNED_IDENTITY_ID` to `ARM_KUBELET_IDENTITY_CLIENT_ID` for accuracy to its purpose/use

comtalyst

/test

coveralls · 2024-05-22T19:09:17Z

Pull Request Test Coverage Report for Build 10869172682

Details

41 of 48 (85.42%) changed or added relevant lines in 7 files are covered.
6 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.2%) to 97.899%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/providers/instance/azure_client.go	0	2	0.0%
pkg/providers/instance/skuclient/skuclient.go	0	5	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/auth/util.go	2	0.0%
pkg/auth/config.go	4	75.86%

Totals
Change from base Build 10782405541:	0.2%
Covered Lines:	36254
Relevant Lines:	37032

💛 - Coveralls

pkg/auth/config.go

comtalyst

/test

comtalyst

/test

tallaxes

Before commenting on the appropriate configuration surface, and corresponding implementation, I would like to see a clear description of which authentication methods we want to support - and if we have a good reason to constrain them. Is there anything that prevents us from just using DefaultAzureCredential, which attempts a variety of authentication mechanisms (including the ones we need to support), using its existing/documented configuration surface? Would be good to avoid custom switches (like ARM_AUTH_METHOD) if the "standard"/OOB works well.

tallaxes · 2024-06-12T01:34:04Z

pkg/auth/cred.go

+	if cfg.ArmAuthMethod == authMethodWorkloadIdentity {
 		klog.V(2).Infoln("cred: using workload identity for new credential")
 		return azidentity.NewDefaultAzureCredential(nil)


"using workload identity" - this is not quite what NewDefaultAzureCredential does, it will attempt a variety of mechanisms. If we are Ok with this - which may well be the case - let's just say it, and we don't need to special case Managed Identity below. If we want to constrain this to Workload Identity - needs to use NewWorkloadIdentityCredential?

There is a discussion related to this with @charliedmcb in another PR. I think our understanding match yours, with the new knowledge here is that NewDefaultAzureCredential() might cover managed identity as well.

So far, I think that we should scope it down to workload identity for simplicity. This means we will support only two auth methods for now: workload identity and system assigned MSI.
Before this PR (and in the current iteration you are seeing), with this NewDefaultAzureCredential(), it looks like we are supporting a wider scope, which I don't think we need to. There are "shortcuts" where ARM_AUTH_METHOD is workload identity, but they ended up using something else that's compatible with this implementation.

So, I think I will make changes to NewWorkloadIdentityCredential(), if possible.
Maybe we could consider formally using/supporting NewDefaultAzureCredential() later on if this module grows/we don't want to contain much knowledge on authentication.

I'm not sure what you mean by below; could you provide more example?

Would be good to avoid custom switches (like ARM_AUTH_METHOD) if the "standard"/OOB works well.

What I mean is that azidentity already provides (via environment variables) an existing configuration surface, and (via DefaultAzureCredential) a reasonable sequence of auth methods to try, including the two that we need to support. We should have good reasons to a) constrain the auth methods, and b) introduce additional/custom configuration surface, such as the proposed ARM_AUTH_METHOD setting. (Which we may need to remove later if decide to use NewDefaultAzureCredential? Removal is always hard ...) There may be valid reasons for both (improved security posture, the built-in sequence not meeting our order requirements, established pattern elsewhere, etc.) - but would be good to have clarity on which ones - if any - apply here.

One more option to consider - if we decide there is a good reason to support only workload identity and MSI - is to just try them in order (similar to how DefaultAzureCredential does it), eliminating the need (and capability) for the explicit choice via ARM_AUTH_METHOD.

My preference would be for the simplest secure approach that supports the requirements, which seems to be DefaultAzureCredential. (Any changes there will likely require additional configuration, and are very unlikely to break existing auth scenarios. In NAP we control the configuration tightly enough for this not to be a concern, for self-hosted breaking changes, if any, would/should surface via release notes.) Second best would be to try a more controlled sequence of auth methods we need to support - unless you see a problem with this, and we have to specify the method explicitly via configuration.

Agreed DefaultAzureCredential already encapsulates what we need. I don't see them breaking it either as its wildly used in many projects for wrapping many types of auth based on some quick github code searching.

Any changes there will likely require additional configuration, and are very unlikely to break existing auth scenarios

I don't see them breaking it either

I doubt it will break too, plus the tests will catch it anyway in case we neglect release notes.
But my point is it might have new auth methods we are not aware of, which means more liability as I mentioned. This will not happen in the more controlled approach (i.e., NewWorkloadIdentityCredential with ARM_AUTH_METHOD)

In NAP we control the configuration tightly enough for this not to be a concern

Yep all these concerns are for self-hosted. In NAP we have static integration interface.

But maybe I am overthinking again. The extra auth method might not add that much additional cost to us compared what DefaultAzureCredential will save us.
We could try going with implicit auth method and use DefaultAzureCredential and see how it goes.

The only problem, if it is at all, is autorest_auth.go is also using it. But I just haven't take a look at that deeply yet. If we are moving in this direction then I will see the possibility.

Circling back to this.

The concern of unwanted auth method is valid, from an example in Cluster Autoscaler. "Requirement" to continue the support is an engineering burden.

Although, this DefaultAzureCredential is something standardized. If we are going to rely on this standardization (azure-sdk-for-go) for a next while, the simplicity we get might worth it, and the deprecation (if necessary) might not be too difficult.

TL;DR let's just use DefaultAzureCredential.

… middle layers to eliminate confusion

comtalyst · 2024-09-15T08:02:03Z

pkg/operator/operator.go

@@ -169,7 +170,7 @@ func getCABundle(restConfig *rest.Config) (*string, error) {
 }

 func getVnetGUID(cfg *auth.Config, subnetID string) (string, error) {
-	creds, err := auth.NewCredential(cfg)
+	creds, err := azidentity.NewDefaultAzureCredential(nil)


An alternative is to keep the abstraction, but just return azidentity.NewDefaultAzureCredential(nil). But there are only 2 uses (another in azure_client.go) at the moment, so I don't think it is significant. Could refactor later.

comtalyst · 2024-09-15T08:04:10Z

pkg/providers/instance/skuclient/skuclient.go

 		return
 	}
+	authorizer := azidext.NewTokenCredentialAdapter(cred, []string{azidext.DefaultManagementScope})


An alternative is to keep the abstraction, that returns authorizer. But this is the only use of it at the moment, so I don't think it is significant. Could refactor later.

comtalyst

/test

comtalyst

/test

Bryce-Soghigian · 2024-09-15T20:17:00Z

pkg/auth/config.go

+	cfg.TenantID = strings.TrimSpace(os.Getenv("ARM_TENANT_ID"))
+	cfg.SubscriptionID = strings.TrimSpace(os.Getenv("ARM_SUBSCRIPTION_ID"))
+	cfg.VMType = strings.ToLower(os.Getenv("ARM_VM_TYPE"))
+	cfg.ClusterName = strings.TrimSpace(os.Getenv("AZURE_CLUSTER_NAME"))


How is this used?

bootstrap variables, mostly

pkg/providers/instance/azure_client.go

Bryce-Soghigian · 2024-09-15T20:18:36Z

pkg/providers/instance/skuclient/skuclient.go

-	authorizer, err := auth.NewAuthorizer(sc.cfg, sc.env)
+	// Create a new authorizer for the sku client
+	// TODO (charliedmcb): need to get track 2 support for the skewer API
+	cred, err := azidentity.NewDefaultAzureCredential(nil)


How did you test this?

It is the code of auth.NewAuthorizer(sc.cfg, sc.env), with changes to use azidentity.NewDefaultAzureCredential(nil).
E2E for each auth method should be sufficient.

Bryce-Soghigian

Run e2e tests please before merging

comtalyst requested a review from tallaxes May 22, 2024 19:01

comtalyst force-pushed the comtalyst/auth-interface-rework branch from a56a078 to 852da05 Compare May 22, 2024 19:02

comtalyst commented May 22, 2024

View reviewed changes

Bryce-Soghigian reviewed May 22, 2024

View reviewed changes

pkg/auth/config.go Outdated Show resolved Hide resolved

comtalyst commented May 22, 2024

View reviewed changes

comtalyst force-pushed the comtalyst/auth-interface-rework branch from 214bbde to c3615b8 Compare June 7, 2024 00:14

comtalyst commented Jun 7, 2024

View reviewed changes

tallaxes reviewed Jun 12, 2024

View reviewed changes

tallaxes assigned comtalyst Jun 12, 2024

tallaxes added area/security Issues or PRs related to security area/global-config Issues or PRs related global configuration labels Jun 12, 2024

comtalyst added 7 commits September 14, 2024 15:53

refactor: rework aged authentication config flow

a86fe37

refactor: rename userAssignedIdentityID to kubeletIdentityClientID in…

c79efcb

… middle layers to eliminate confusion

chore: not supporting legacy envs

04bdaf2

chore: rename "credential-from-environment" to "workload-identity"

cd7cc96

chore: make presubmit

02bf57b

refactor: rename AuthMethod to ArmAuthMethod for clarity

f9f4e58

refactor: abandon ARM_AUTH_METHOD in favor of DefaultAzureCredentials

584d2bf

comtalyst force-pushed the comtalyst/auth-interface-rework branch from c3615b8 to 584d2bf Compare September 15, 2024 07:31

comtalyst commented Sep 15, 2024

View reviewed changes

comtalyst requested review from tallaxes and Bryce-Soghigian September 15, 2024 20:06

Bryce-Soghigian reviewed Sep 15, 2024

View reviewed changes

pkg/providers/instance/azure_client.go Show resolved Hide resolved

Bryce-Soghigian reviewed Sep 15, 2024

View reviewed changes

Bryce-Soghigian approved these changes Sep 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: rework aged authentication config interface #366

refactor: rework aged authentication config interface #366

comtalyst commented May 22, 2024 •

edited

Loading

comtalyst left a comment

coveralls commented May 22, 2024 •

edited

Loading

comtalyst left a comment

comtalyst left a comment

tallaxes left a comment •

edited

Loading

tallaxes Jun 12, 2024 •

edited

Loading

comtalyst Jun 12, 2024 •

edited

Loading

comtalyst Jun 12, 2024

tallaxes Jun 12, 2024

tallaxes Jun 12, 2024

tallaxes Jun 12, 2024

Bryce-Soghigian Jun 12, 2024

comtalyst Jun 13, 2024

comtalyst Jun 13, 2024 •

edited

Loading

comtalyst Sep 14, 2024 •

edited

Loading

comtalyst Sep 15, 2024 •

edited

Loading

comtalyst Sep 15, 2024 •

edited

Loading

comtalyst left a comment

comtalyst left a comment

Bryce-Soghigian Sep 15, 2024

comtalyst Sep 15, 2024

Bryce-Soghigian Sep 15, 2024

comtalyst Sep 15, 2024

Bryce-Soghigian left a comment

refactor: rework aged authentication config interface #366

Are you sure you want to change the base?

refactor: rework aged authentication config interface #366

Conversation

comtalyst commented May 22, 2024 • edited Loading

comtalyst left a comment

Choose a reason for hiding this comment

coveralls commented May 22, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10869172682

Details

💛 - Coveralls

comtalyst left a comment

Choose a reason for hiding this comment

comtalyst left a comment

Choose a reason for hiding this comment

tallaxes left a comment • edited Loading

Choose a reason for hiding this comment

tallaxes Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

comtalyst Jun 12, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comtalyst Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

comtalyst Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

comtalyst Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

comtalyst Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

comtalyst left a comment

Choose a reason for hiding this comment

comtalyst left a 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

Choose a reason for hiding this comment

Bryce-Soghigian left a comment

Choose a reason for hiding this comment

comtalyst commented May 22, 2024 •

edited

Loading

coveralls commented May 22, 2024 •

edited

Loading

tallaxes left a comment •

edited

Loading

tallaxes Jun 12, 2024 •

edited

Loading

comtalyst Jun 12, 2024 •

edited

Loading

comtalyst Jun 13, 2024 •

edited

Loading

comtalyst Sep 14, 2024 •

edited

Loading

comtalyst Sep 15, 2024 •

edited

Loading

comtalyst Sep 15, 2024 •

edited

Loading