auth: manual token expiration, better auxiliary token handling, improve tests #665

manicminer · 2023-09-26T15:55:46Z

Better Handling of Auxiliary Tokens

Adds the AuthorizeRequest field to client.Client which API clients can define for customized authoriation behavior. This is then utilized by resourcemanager.Client to decorate requests with both Authorization and x-ms-authorization-auxiliary headers.

Adds a helper function SetAuthHeader() which decorates an *http.Request with a bearer token, e.g. adds the Authorization header. This is called by base.Client when AuthorizeRequest is nil.

Manual Token Expiration

This provides the ability to intentionally expire a cached access token, which causes a new token to be automatically acquired the next time the Token() or AuxiliaryTokens() methods are called.

Downstream implementers can test whether an auth.Authorizer implements auth.CachingAuthorizer, in case they receive a standard auth.Authorizer, and then call InvalidateTokenCache() prior to making a new request. This requires that the method is called as-needed when an action is taken that is likely to necessitate a new access token, for example with management groups which make use of token claims for access control.

Closes: #459

Whilst the new ExpireToken() method only has any meaning or effect when a token is being cached, in practise this all the existing Authorizers wrap themselves with a CachedAuthorizer. For consistency this is added to the Authorizer interface to make downstream implementation simpler and this should not cause any incompatibility or errors where one of the provided constructors is used.

…th both standard and auxiliary access tokens

manicminer · 2023-09-27T12:05:28Z

Unit test results

industrialzombie · 2023-09-27T14:58:36Z

Hey,

The client will need to be updated to use this new SetAuthHeaders() function when it executes a request.
Should that be included in the scope of this change?

https://github.com/hashicorp/go-azure-sdk/blob/main/sdk/client/client.go#L337

manicminer · 2023-09-27T15:58:35Z

@industrialzombie Yes it will, I had thought to add that separately for additional safety downstream, but on balance it's probably fine to add it here and merge it all together 👍

industrialzombie · 2023-10-10T14:34:19Z

Is it possible to get an estimate on when this might be reviewed?

manicminer · 2023-10-12T16:15:23Z

@industrialzombie We'll look at this after today's release, so that we have a few days to catch any potential bugs before shipping.

industrialzombie · 2023-10-12T16:51:15Z

Thanks, I appreciate the feedback!

tombuildsstuff

Left some comments inline but otherwise 👍

sdk/auth/cached_authorizer.go

tombuildsstuff · 2023-10-17T06:41:20Z

sdk/auth/interface.go

 	Token(ctx context.Context, request *http.Request) (*oauth2.Token, error)
-
 	AuxiliaryTokens(ctx context.Context, request *http.Request) ([]*oauth2.Token, error)
+	ExpireTokens() error


for this one (but also in retrospect) is it worth adding some documentation for this one? As it stands the "will do nothing" comments on each implementation will show as needed, but don't strictly explain the purpose for this one?

good shout, will add 👍

tombuildsstuff · 2023-10-17T06:42:38Z

sdk/auth/token.go

 )

+// SetAuthHeaders decorates a *http.Request with necessary authorization headers for Azure APIs. For more information about the vendor-specific
+// `x-ms-authorization-auxiliary` header, see https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/authenticate-multi-tenant
+func SetAuthHeaders(ctx context.Context, req *http.Request, authorizer Authorizer) error {


since this is specific to Resource Manager, this probably wants to go into the Resource Manager Client?

agreed, other APIs may perhaps support the concept of multiple tenants but the implementation will be different

sdk/auth/token.go

tombuildsstuff · 2023-10-17T06:47:12Z

sdk/client/client.go

@@ -327,14 +327,11 @@ func (c *Client) Execute(ctx context.Context, req *Request) (*Response, error) {
 		return nil, fmt.Errorf("req.Request was nil")
 	}

-	// at this point we're ready to send the HTTP Request, as such let's get the Authorization token
-	// and add that to the request
+	// Set Authorization and X-Ms-Authorization-Auxiliary headers


Doing this will cause issues with Storage Data Plane which is super picky about the headers it supports

Perhaps Client should have a ConfigureAuthorizerForRequest function/method defined against the struct which the implementations can pass-in?

Makes sense, I've added this and set a custom authorization function for ResourceManagerClient. Where such custom function is not specified, the base client will fall back to setting a standard Authorization header with a bearer token (i.e. its current behavior).

sdk/client/client.go

tombuildsstuff · 2023-10-17T06:51:33Z

sdk/auth/shared_key_authorizer.go

-// ---
+// ExpireToken has no effect with shared keys
+func (c *SharedKeyAuthorizer) ExpireTokens() error {
+	return fmt.Errorf("SharedKeyAuthorizer tokens cannot expire")


given the other authorizers are a noop, why not do the same here? perhaps this method wants renaming to InvalidateTokenCache or something to better reflect what it's doing?

I've done away with this ambiguity and added a CachingAuthorizer interface which can be tested for at runtime to call the InvalidateTokenCache() method.

* Add a custom authorization function for ResourceManagerClient, which sets the `Authorization` and `x-ms-authorization-auxiliary` headers. * When custom authorization function is not defined, use the auth.SetAuthHeader() function to decorate requests with a standard `Authorization` header having a bearer token.

manicminer · 2023-10-24T00:55:18Z

Thanks for the review @tombuildsstuff, I've reworked it to remove the anbiguity over invalidation by adding a CachingAuthorizer interface, and added an AuthorizeRequest property to the base client to allow API clients to supply a custom function for decorating the request. WDYT?

tombuildsstuff

A couple of minor comments but otherwise 👍

tombuildsstuff · 2023-10-24T07:18:03Z

sdk/auth/cached_authorizer.go

+// tokens to be acquired when Token() or AuxiliaryTokens() are next called
+func (c *CachedAuthorizer) InvalidateCachedTokens() error {
+	if c.token == nil {
+		return fmt.Errorf("internal-error: c.token was nil")


tbh I think it's worth this being a noop?

Suggested change

return fmt.Errorf("internal-error: c.token was nil")

return nil

Makes sense, the end result should be the same 👍

tombuildsstuff · 2023-11-06T14:54:30Z

sdk/client/resourcemanager/authorization.go

+	auxTokenValues := make([]string, 0)
+	for _, auxToken := range auxTokens {
+		auxTokenValues = append(auxTokenValues, fmt.Sprintf("%s %s", auxToken.Type(), auxToken.AccessToken))
+	}
+	req.Header.Set("X-Ms-Authorization-Auxiliary", strings.Join(auxTokenValues, ", "))


if there aren't any, should we omit this?

…o aux tokens are present

manicminer added enhancement New feature or request authentication labels Sep 26, 2023

manicminer requested a review from a team as a code owner September 26, 2023 15:55

manicminer added 5 commits September 27, 2023 01:00

auth: extend manual token expiration to include auxiliary tokens

96793a5

auth: support the exp claim

ca32de0

auth: add a SetAuthHeaders() function for decorating the Request wi…

5165d82

…th both standard and auxiliary access tokens

auth: proper unit testing for the CachedAuthorizer

1072418

auth: fix a bug with auxiliary token caching

60af3f3

manicminer changed the title ~~auth: ability to manually expire a cached token~~ auth: manual token expiration, better auxiliary token handling, improve tests Sep 27, 2023

auth: documentation and cleanup

82cc448

manicminer force-pushed the f/manual-token-expiry branch from 814e1ea to 82cc448 Compare September 27, 2023 00:24

auth: fix up test token pattern

4e0f271

manicminer added 2 commits September 27, 2023 17:22

client: use auth package helper to set authorization headers

15a78fe

auth: fix bug where empty slice of auxTokens is skipped

e2ff919

manicminer added a commit to hashicorp/terraform-provider-azurerm that referenced this pull request Sep 28, 2023

REBASE: update go-azure-sdk, see hashicorp/go-azure-sdk#665

5c922f6

manicminer mentioned this pull request Sep 28, 2023

azurerm_private_dns_zone_virtual_network_link - explicit test for cross-tenant usage hashicorp/terraform-provider-azurerm#23420

Merged

tombuildsstuff approved these changes Oct 17, 2023

View reviewed changes

manicminer added 2 commits October 23, 2023 22:26

Merge branch 'main' into f/manual-token-expiry

48750d5

manicminer requested a review from tombuildsstuff October 24, 2023 00:55

tombuildsstuff approved these changes Nov 6, 2023

View reviewed changes

industrialzombie mentioned this pull request Nov 27, 2023

Private DNS Zone Virtual Network Link - Creation fails for cross-tenant hashicorp/terraform-provider-azurerm#23238

Closed

1 task

manicminer added 3 commits November 27, 2023 18:43

Merge branch 'main' into f/manual-token-expiry

ff6a279

auth: ignore missing token when invalidating cache

55249e0

resourcemanager: don't set x-ms-authorization-auxiliary header when n…

6445851

…o aux tokens are present

manicminer added the release-once-merged The SDK should be released once this PR is merged label Nov 27, 2023

manicminer merged commit 8e462d0 into main Nov 27, 2023
5 checks passed

manicminer deleted the f/manual-token-expiry branch November 27, 2023 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth: manual token expiration, better auxiliary token handling, improve tests #665

auth: manual token expiration, better auxiliary token handling, improve tests #665

manicminer commented Sep 26, 2023 •

edited

Loading

manicminer commented Sep 27, 2023

industrialzombie commented Sep 27, 2023

manicminer commented Sep 27, 2023

industrialzombie commented Oct 10, 2023

manicminer commented Oct 12, 2023

industrialzombie commented Oct 12, 2023

tombuildsstuff left a comment

tombuildsstuff Oct 17, 2023

manicminer Oct 23, 2023

tombuildsstuff Oct 17, 2023

manicminer Oct 23, 2023 •

edited

Loading

tombuildsstuff Oct 17, 2023

manicminer Oct 24, 2023

tombuildsstuff Oct 17, 2023

manicminer Oct 24, 2023

manicminer commented Oct 24, 2023

tombuildsstuff left a comment

tombuildsstuff Oct 24, 2023

manicminer Nov 27, 2023

tombuildsstuff Nov 6, 2023

	return fmt.Errorf("internal-error: c.token was nil")
	return nil

auth: manual token expiration, better auxiliary token handling, improve tests #665

auth: manual token expiration, better auxiliary token handling, improve tests #665

Conversation

manicminer commented Sep 26, 2023 • edited Loading

Better Handling of Auxiliary Tokens

Manual Token Expiration

manicminer commented Sep 27, 2023

industrialzombie commented Sep 27, 2023

manicminer commented Sep 27, 2023

industrialzombie commented Oct 10, 2023

manicminer commented Oct 12, 2023

industrialzombie commented Oct 12, 2023

tombuildsstuff 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

manicminer Oct 23, 2023 • 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

manicminer commented Oct 24, 2023

tombuildsstuff 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

manicminer commented Sep 26, 2023 •

edited

Loading

manicminer Oct 23, 2023 •

edited

Loading