From 2a5500eff5cefa0c2676acb9b4e5ffe7e1a0daf0 Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Tue, 28 Nov 2023 16:30:28 +0000 Subject: [PATCH 1/7] auth: support caching of values retrieves from Azure CLI to avoid unnecessary repeated invocations --- sdk/auth/auth.go | 7 ++- sdk/auth/azure_cli_authorizer.go | 101 +++++++++++++++++++++++++----- sdk/auth/config.go | 2 + sdk/internal/azurecli/azcli.go | 62 ++++++++++-------- sdk/internal/azurecli/versions.go | 7 +-- 5 files changed, 127 insertions(+), 52 deletions(-) diff --git a/sdk/auth/auth.go b/sdk/auth/auth.go index 722e76171f8..67abb6b3f54 100644 --- a/sdk/auth/auth.go +++ b/sdk/auth/auth.go @@ -126,9 +126,10 @@ func NewAuthorizerFromCredentials(ctx context.Context, c Credentials, api enviro if c.EnableAuthenticatingUsingAzureCLI { opts := AzureCliAuthorizerOptions{ - Api: api, - TenantId: c.TenantID, - AuxTenantIds: c.AuxiliaryTenantIDs, + Api: api, + TenantId: c.TenantID, + AuxTenantIds: c.AuxiliaryTenantIDs, + EnableCaching: c.AzureCLIUseCache, } a, err := NewAzureCliAuthorizer(ctx, opts) if err != nil { diff --git a/sdk/auth/azure_cli_authorizer.go b/sdk/auth/azure_cli_authorizer.go index 21de3aaa457..1a73d12e40f 100644 --- a/sdk/auth/azure_cli_authorizer.go +++ b/sdk/auth/azure_cli_authorizer.go @@ -17,6 +17,14 @@ import ( "golang.org/x/oauth2" ) +type cachedCliData struct { + AzVersion *string + DefaultSubscriptionId *string + DefaultTenantId *string +} + +var cliCache cachedCliData + type AzureCliAuthorizerOptions struct { // Api describes the Azure API being used Api environments.Api @@ -28,11 +36,15 @@ type AzureCliAuthorizerOptions struct { // used for Resource Manager when auxiliary tenants are needed. // e.g. https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/authenticate-multi-tenant AuxTenantIds []string + + // EnableCaching permits module-scoped caching of the parsed results of `az` command invocations, and can be set to improve + // performance when instantiating multiple authorizers, at the cost of detectability during long-running applications. + EnableCaching bool } // NewAzureCliAuthorizer returns an Authorizer which authenticates using the Azure CLI. func NewAzureCliAuthorizer(ctx context.Context, options AzureCliAuthorizerOptions) (Authorizer, error) { - conf, err := newAzureCliConfig(options.Api, options.TenantId, options.AuxTenantIds) + conf, err := newAzureCliConfig(options.Api, options.TenantId, options.AuxTenantIds, options.EnableCaching) if err != nil { return nil, err } @@ -58,13 +70,23 @@ func (a *AzureCliAuthorizer) Token(_ context.Context, _ *http.Request) (*oauth2. return nil, fmt.Errorf("could not request token: conf is nil") } - azArgs := []string{"account", "get-access-token"} + var err error // verify that the Azure CLI supports MSAL - ADAL is no longer supported - err := azurecli.CheckAzVersion(azurecli.MsalVersion, nil) - if err != nil { + var azVersion *string + if a.conf.CacheEnabled && cliCache.AzVersion != nil { + azVersion = cliCache.AzVersion + } else { + if azVersion, err = azurecli.GetAzVersion(); err != nil { + return nil, fmt.Errorf("%s. Please ensure you have installed Azure CLI version %s or newer", err, AzureCliMinimumVersion) + } + } + if err = azurecli.CheckAzVersion(*azVersion, azurecli.MsalVersion, nil); err != nil { return nil, fmt.Errorf("checking the version of the Azure CLI: %+v", err) } + + azArgs := []string{"account", "get-access-token"} + scope, err := environments.Scope(a.conf.Api) if err != nil { return nil, fmt.Errorf("determining scope for %q: %+v", a.conf.Api.Name(), err) @@ -113,12 +135,21 @@ func (a *AzureCliAuthorizer) AuxiliaryTokens(_ context.Context, _ *http.Request) } azArgs := []string{"account", "get-access-token"} + var err error // verify that the Azure CLI supports MSAL - ADAL is no longer supported - err := azurecli.CheckAzVersion(AzureCliMsalVersion, nil) - if err != nil { + var azVersion *string + if a.conf.CacheEnabled && cliCache.AzVersion != nil { + azVersion = cliCache.AzVersion + } else { + if azVersion, err = azurecli.GetAzVersion(); err != nil { + return nil, fmt.Errorf("%s. Please ensure you have installed Azure CLI version %s or newer", err, AzureCliMinimumVersion) + } + } + if err = azurecli.CheckAzVersion(*azVersion, azurecli.MsalVersion, nil); err != nil { return nil, fmt.Errorf("checking the version of the Azure CLI: %+v", err) } + scope, err := environments.Scope(a.conf.Api) if err != nil { return nil, fmt.Errorf("determining scope for %q: %+v", a.conf.Api.Name(), err) @@ -145,7 +176,6 @@ func (a *AzureCliAuthorizer) AuxiliaryTokens(_ context.Context, _ *http.Request) const ( AzureCliMinimumVersion = "2.0.81" - AzureCliMsalVersion = "2.30.0" AzureCliNextMajorVersion = "3.0.0" ) @@ -161,31 +191,67 @@ type azureCliConfig struct { // DefaultSubscriptionID is the optional default subscription ID DefaultSubscriptionID string + + // CacheEnabled specifies whether to use module-level caching to speed up idempotent az-cli operations. + CacheEnabled bool } // newAzureCliConfig validates the supplied tenant ID and returns a new azureCliConfig. -func newAzureCliConfig(api environments.Api, tenantId string, auxiliaryTenantIds []string) (*azureCliConfig, error) { +func newAzureCliConfig(api environments.Api, tenantId string, auxiliaryTenantIds []string, cacheEnabled bool) (*azureCliConfig, error) { var err error // check az-cli version - nextMajor := azurecli.NextMajorVersion - if err = azurecli.CheckAzVersion(azurecli.MinimumVersion, &nextMajor); err != nil { + var azVersion *string + if cacheEnabled && cliCache.AzVersion != nil { + azVersion = cliCache.AzVersion + } else { + azVersion, err = azurecli.GetAzVersion() + if err != nil { + return nil, fmt.Errorf("%s. Please ensure you have installed Azure CLI version %s or newer", err, AzureCliMinimumVersion) + } + + if cacheEnabled { + cliCache.AzVersion = azVersion + } + } + nextMajor := AzureCliNextMajorVersion + if err = azurecli.CheckAzVersion(*azVersion, azurecli.MsalVersion, &nextMajor); err != nil { return nil, err } // check tenant ID - tenantId, err = azurecli.CheckTenantID(tenantId) - if err != nil { - return nil, err + var defaultTenantId *string + if cacheEnabled && cliCache.DefaultTenantId != nil { + defaultTenantId = cliCache.DefaultTenantId + } else { + if defaultTenantId, err = azurecli.CheckTenantID(tenantId); err != nil { + return nil, err + } + if cacheEnabled { + cliCache.DefaultTenantId = defaultTenantId + } } - if tenantId == "" { + if defaultTenantId == nil { return nil, errors.New("invalid tenantId or unable to determine tenantId") } + tenantId = *defaultTenantId // get the default subscription ID - subscriptionId, err := azurecli.GetDefaultSubscriptionID() - if err != nil { - return nil, err + var defaultSubscriptionId *string + if cacheEnabled && cliCache.DefaultSubscriptionId != nil { + defaultSubscriptionId = cliCache.DefaultSubscriptionId + } else { + if defaultSubscriptionId, err = azurecli.GetDefaultSubscriptionID(); err != nil { + return nil, err + } + if cacheEnabled { + cliCache.DefaultSubscriptionId = defaultSubscriptionId + } + } + + var subscriptionId string + if defaultSubscriptionId != nil { + subscriptionId = *defaultSubscriptionId } return &azureCliConfig{ @@ -193,6 +259,7 @@ func newAzureCliConfig(api environments.Api, tenantId string, auxiliaryTenantIds TenantID: tenantId, AuxiliaryTenantIDs: auxiliaryTenantIds, DefaultSubscriptionID: subscriptionId, + CacheEnabled: cacheEnabled, }, nil } diff --git a/sdk/auth/config.go b/sdk/auth/config.go index 7be93f4fcb3..3f7e5ac552e 100644 --- a/sdk/auth/config.go +++ b/sdk/auth/config.go @@ -21,6 +21,8 @@ type Credentials struct { // EnableAuthenticatingUsingAzureCLI specifies whether Azure CLI authentication should be checked. EnableAuthenticatingUsingAzureCLI bool + // AzureCLIUseCache specifies whether caching should be enabled for idempotent operation results + AzureCLIUseCache bool // EnableAuthenticatingUsingClientCertificate specifies whether Client Certificate authentication should be checked. EnableAuthenticatingUsingClientCertificate bool diff --git a/sdk/internal/azurecli/azcli.go b/sdk/internal/azurecli/azcli.go index 09896637dc3..6976e98fd8a 100644 --- a/sdk/internal/azurecli/azcli.go +++ b/sdk/internal/azurecli/azcli.go @@ -7,6 +7,7 @@ import ( "bytes" "encoding/json" "fmt" + "log" "os/exec" "regexp" "strings" @@ -15,25 +16,10 @@ import ( ) // CheckAzVersion tries to determine the version of Azure CLI in the path and checks for a compatible version -func CheckAzVersion(minVersion string, nextMajorVersion *string) error { - var cliVersion *struct { - AzureCli *string `json:"azure-cli,omitempty"` - AzureCliCore *string `json:"azure-cli-core,omitempty"` - AzureCliTelemetry *string `json:"azure-cli-telemetry,omitempty"` - Extensions *interface{} `json:"extensions,omitempty"` - } - err := JSONUnmarshalAzCmd(&cliVersion, "version") - if err != nil { - return fmt.Errorf("could not parse Azure CLI version: %v", err) - } - - if cliVersion.AzureCli == nil { - return fmt.Errorf("could not detect Azure CLI version. Please ensure you have installed Azure CLI version %s or newer", minVersion) - } - - actual, err := version.NewVersion(*cliVersion.AzureCli) +func CheckAzVersion(currentVersion string, minVersion string, nextMajorVersion *string) error { + actual, err := version.NewVersion(currentVersion) if err != nil { - return fmt.Errorf("could not parse detected Azure CLI version %q: %+v", *cliVersion.AzureCli, err) + return fmt.Errorf("could not parse detected Azure CLI version %q: %+v", currentVersion, err) } supported, err := version.NewVersion(minVersion) @@ -59,24 +45,44 @@ func CheckAzVersion(minVersion string, nextMajorVersion *string) error { return nil } +// GetAzVersion tries to determine the version of Azure CLI in the path. +func GetAzVersion() (*string, error) { + var cliVersion *struct { + AzureCli *string `json:"azure-cli,omitempty"` + AzureCliCore *string `json:"azure-cli-core,omitempty"` + AzureCliTelemetry *string `json:"azure-cli-telemetry,omitempty"` + Extensions *interface{} `json:"extensions,omitempty"` + } + err := JSONUnmarshalAzCmd(&cliVersion, "version") + if err != nil { + return nil, fmt.Errorf("could not parse Azure CLI version: %v", err) + } + + if cliVersion.AzureCli == nil { + return nil, fmt.Errorf("could not detect Azure CLI version") + } + + return cliVersion.AzureCli, nil +} + // GetDefaultSubscriptionID tries to determine the default subscription -func GetDefaultSubscriptionID() (string, error) { +func GetDefaultSubscriptionID() (*string, error) { var account struct { SubscriptionID string `json:"id"` } err := JSONUnmarshalAzCmd(&account, "account", "show") if err != nil { - return "", fmt.Errorf("obtaining subscription ID: %s", err) + return nil, fmt.Errorf("obtaining subscription ID: %s", err) } - return account.SubscriptionID, nil + return &account.SubscriptionID, nil } // CheckTenantID validates the supplied tenant ID, and tries to determine the default tenant if a valid one is not supplied. -func CheckTenantID(tenantId string) (string, error) { +func CheckTenantID(tenantId string) (*string, error) { validTenantId, err := regexp.MatchString("^[a-zA-Z0-9._-]+$", tenantId) if err != nil { - return "", fmt.Errorf("could not parse tenant ID %q: %s", tenantId, err) + return nil, fmt.Errorf("could not parse tenant ID %q: %s", tenantId, err) } if !validTenantId { @@ -84,14 +90,13 @@ func CheckTenantID(tenantId string) (string, error) { ID string `json:"id"` TenantID string `json:"tenantId"` } - err := JSONUnmarshalAzCmd(&account, "account", "show") - if err != nil { - return "", fmt.Errorf("obtaining tenant ID: %s", err) + if err = JSONUnmarshalAzCmd(&account, "account", "show"); err != nil { + return nil, fmt.Errorf("obtaining tenant ID: %s", err) } tenantId = account.TenantID } - return tenantId, nil + return &tenantId, nil } // JSONUnmarshalAzCmd executes an Azure CLI command and unmarshalls the JSON output. @@ -100,6 +105,9 @@ func JSONUnmarshalAzCmd(i interface{}, arg ...string) error { var stdout bytes.Buffer arg = append(arg, "-o=json") + + log.Printf("[DEBUG] az-cli invocation: az %s", strings.Join(arg, " ")) + cmd := exec.Command("az", arg...) cmd.Stderr = &stderr cmd.Stdout = &stdout diff --git a/sdk/internal/azurecli/versions.go b/sdk/internal/azurecli/versions.go index 6fb0fac8bc8..ac6faa79d58 100644 --- a/sdk/internal/azurecli/versions.go +++ b/sdk/internal/azurecli/versions.go @@ -3,8 +3,5 @@ package azurecli -const ( - MinimumVersion = "2.0.81" - MsalVersion = "2.30.0" - NextMajorVersion = "3.0.0" -) +// MsalVersion is the first known version of Azure CLI to support MSAL / v2 tokens +const MsalVersion = "2.30.0" From 1b60c2018d2872cfd30b5cce9914ffa8b80fe7d1 Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Wed, 29 Nov 2023 19:41:01 +0000 Subject: [PATCH 2/7] auth: reimplement azure-cli caching at lower level, remove toggle so caching is always used --- sdk/auth/auth.go | 7 +- sdk/auth/azure_cli_authorizer.go | 100 +++------------------- sdk/auth/config.go | 2 - sdk/internal/azurecli/azcli.go | 133 +++++++++++++++++------------- sdk/internal/azurecli/cache.go | 24 ++++++ sdk/internal/azurecli/versions.go | 12 ++- 6 files changed, 126 insertions(+), 152 deletions(-) create mode 100644 sdk/internal/azurecli/cache.go diff --git a/sdk/auth/auth.go b/sdk/auth/auth.go index 67abb6b3f54..722e76171f8 100644 --- a/sdk/auth/auth.go +++ b/sdk/auth/auth.go @@ -126,10 +126,9 @@ func NewAuthorizerFromCredentials(ctx context.Context, c Credentials, api enviro if c.EnableAuthenticatingUsingAzureCLI { opts := AzureCliAuthorizerOptions{ - Api: api, - TenantId: c.TenantID, - AuxTenantIds: c.AuxiliaryTenantIDs, - EnableCaching: c.AzureCLIUseCache, + Api: api, + TenantId: c.TenantID, + AuxTenantIds: c.AuxiliaryTenantIDs, } a, err := NewAzureCliAuthorizer(ctx, opts) if err != nil { diff --git a/sdk/auth/azure_cli_authorizer.go b/sdk/auth/azure_cli_authorizer.go index 1a73d12e40f..e82b8040d41 100644 --- a/sdk/auth/azure_cli_authorizer.go +++ b/sdk/auth/azure_cli_authorizer.go @@ -5,7 +5,6 @@ package auth import ( "context" - "errors" "fmt" "net/http" "os" @@ -17,14 +16,6 @@ import ( "golang.org/x/oauth2" ) -type cachedCliData struct { - AzVersion *string - DefaultSubscriptionId *string - DefaultTenantId *string -} - -var cliCache cachedCliData - type AzureCliAuthorizerOptions struct { // Api describes the Azure API being used Api environments.Api @@ -36,15 +27,11 @@ type AzureCliAuthorizerOptions struct { // used for Resource Manager when auxiliary tenants are needed. // e.g. https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/authenticate-multi-tenant AuxTenantIds []string - - // EnableCaching permits module-scoped caching of the parsed results of `az` command invocations, and can be set to improve - // performance when instantiating multiple authorizers, at the cost of detectability during long-running applications. - EnableCaching bool } // NewAzureCliAuthorizer returns an Authorizer which authenticates using the Azure CLI. func NewAzureCliAuthorizer(ctx context.Context, options AzureCliAuthorizerOptions) (Authorizer, error) { - conf, err := newAzureCliConfig(options.Api, options.TenantId, options.AuxTenantIds, options.EnableCaching) + conf, err := newAzureCliConfig(options.Api, options.TenantId, options.AuxTenantIds) if err != nil { return nil, err } @@ -72,19 +59,6 @@ func (a *AzureCliAuthorizer) Token(_ context.Context, _ *http.Request) (*oauth2. var err error - // verify that the Azure CLI supports MSAL - ADAL is no longer supported - var azVersion *string - if a.conf.CacheEnabled && cliCache.AzVersion != nil { - azVersion = cliCache.AzVersion - } else { - if azVersion, err = azurecli.GetAzVersion(); err != nil { - return nil, fmt.Errorf("%s. Please ensure you have installed Azure CLI version %s or newer", err, AzureCliMinimumVersion) - } - } - if err = azurecli.CheckAzVersion(*azVersion, azurecli.MsalVersion, nil); err != nil { - return nil, fmt.Errorf("checking the version of the Azure CLI: %+v", err) - } - azArgs := []string{"account", "get-access-token"} scope, err := environments.Scope(a.conf.Api) @@ -100,7 +74,7 @@ func (a *AzureCliAuthorizer) Token(_ context.Context, _ *http.Request) (*oauth2. } var token azureCliToken - if err := azurecli.JSONUnmarshalAzCmd(&token, azArgs...); err != nil { + if err = azurecli.JSONUnmarshalAzCmd(false, &token, azArgs...); err != nil { return nil, err } @@ -137,19 +111,6 @@ func (a *AzureCliAuthorizer) AuxiliaryTokens(_ context.Context, _ *http.Request) azArgs := []string{"account", "get-access-token"} var err error - // verify that the Azure CLI supports MSAL - ADAL is no longer supported - var azVersion *string - if a.conf.CacheEnabled && cliCache.AzVersion != nil { - azVersion = cliCache.AzVersion - } else { - if azVersion, err = azurecli.GetAzVersion(); err != nil { - return nil, fmt.Errorf("%s. Please ensure you have installed Azure CLI version %s or newer", err, AzureCliMinimumVersion) - } - } - if err = azurecli.CheckAzVersion(*azVersion, azurecli.MsalVersion, nil); err != nil { - return nil, fmt.Errorf("checking the version of the Azure CLI: %+v", err) - } - scope, err := environments.Scope(a.conf.Api) if err != nil { return nil, fmt.Errorf("determining scope for %q: %+v", a.conf.Api.Name(), err) @@ -161,7 +122,7 @@ func (a *AzureCliAuthorizer) AuxiliaryTokens(_ context.Context, _ *http.Request) argsWithTenant := append(azArgs, "--tenant", tenantId) var token azureCliToken - if err := azurecli.JSONUnmarshalAzCmd(&token, argsWithTenant...); err != nil { + if err = azurecli.JSONUnmarshalAzCmd(false, &token, argsWithTenant...); err != nil { return nil, err } @@ -197,60 +158,26 @@ type azureCliConfig struct { } // newAzureCliConfig validates the supplied tenant ID and returns a new azureCliConfig. -func newAzureCliConfig(api environments.Api, tenantId string, auxiliaryTenantIds []string, cacheEnabled bool) (*azureCliConfig, error) { +func newAzureCliConfig(api environments.Api, tenantId string, auxiliaryTenantIds []string) (*azureCliConfig, error) { var err error - // check az-cli version - var azVersion *string - if cacheEnabled && cliCache.AzVersion != nil { - azVersion = cliCache.AzVersion - } else { - azVersion, err = azurecli.GetAzVersion() - if err != nil { - return nil, fmt.Errorf("%s. Please ensure you have installed Azure CLI version %s or newer", err, AzureCliMinimumVersion) - } - - if cacheEnabled { - cliCache.AzVersion = azVersion - } - } - nextMajor := AzureCliNextMajorVersion - if err = azurecli.CheckAzVersion(*azVersion, azurecli.MsalVersion, &nextMajor); err != nil { + // check az-cli version, ensure that MSAL is supported + if err = azurecli.CheckAzVersion(); err != nil { return nil, err } // check tenant ID - var defaultTenantId *string - if cacheEnabled && cliCache.DefaultTenantId != nil { - defaultTenantId = cliCache.DefaultTenantId - } else { - if defaultTenantId, err = azurecli.CheckTenantID(tenantId); err != nil { - return nil, err - } - if cacheEnabled { - cliCache.DefaultTenantId = defaultTenantId - } - } - if defaultTenantId == nil { - return nil, errors.New("invalid tenantId or unable to determine tenantId") + if defaultTenantId, err := azurecli.CheckTenantID(tenantId); err != nil { + return nil, err + } else if defaultTenantId != nil { + tenantId = *defaultTenantId } - tenantId = *defaultTenantId // get the default subscription ID - var defaultSubscriptionId *string - if cacheEnabled && cliCache.DefaultSubscriptionId != nil { - defaultSubscriptionId = cliCache.DefaultSubscriptionId - } else { - if defaultSubscriptionId, err = azurecli.GetDefaultSubscriptionID(); err != nil { - return nil, err - } - if cacheEnabled { - cliCache.DefaultSubscriptionId = defaultSubscriptionId - } - } - var subscriptionId string - if defaultSubscriptionId != nil { + if defaultSubscriptionId, err := azurecli.GetDefaultSubscriptionID(); err != nil { + return nil, err + } else if defaultSubscriptionId != nil { subscriptionId = *defaultSubscriptionId } @@ -259,7 +186,6 @@ func newAzureCliConfig(api environments.Api, tenantId string, auxiliaryTenantIds TenantID: tenantId, AuxiliaryTenantIDs: auxiliaryTenantIds, DefaultSubscriptionID: subscriptionId, - CacheEnabled: cacheEnabled, }, nil } diff --git a/sdk/auth/config.go b/sdk/auth/config.go index 3f7e5ac552e..7be93f4fcb3 100644 --- a/sdk/auth/config.go +++ b/sdk/auth/config.go @@ -21,8 +21,6 @@ type Credentials struct { // EnableAuthenticatingUsingAzureCLI specifies whether Azure CLI authentication should be checked. EnableAuthenticatingUsingAzureCLI bool - // AzureCLIUseCache specifies whether caching should be enabled for idempotent operation results - AzureCLIUseCache bool // EnableAuthenticatingUsingClientCertificate specifies whether Client Certificate authentication should be checked. EnableAuthenticatingUsingClientCertificate bool diff --git a/sdk/internal/azurecli/azcli.go b/sdk/internal/azurecli/azcli.go index 6976e98fd8a..e5e59cb8572 100644 --- a/sdk/internal/azurecli/azcli.go +++ b/sdk/internal/azurecli/azcli.go @@ -16,26 +16,29 @@ import ( ) // CheckAzVersion tries to determine the version of Azure CLI in the path and checks for a compatible version -func CheckAzVersion(currentVersion string, minVersion string, nextMajorVersion *string) error { - actual, err := version.NewVersion(currentVersion) +func CheckAzVersion() error { + currentVersion, err := getAzVersion() if err != nil { - return fmt.Errorf("could not parse detected Azure CLI version %q: %+v", currentVersion, err) + return err + } + + actual, err := version.NewVersion(*currentVersion) + if err != nil { + return fmt.Errorf("could not parse detected Azure CLI version %q: %+v", *currentVersion, err) } - supported, err := version.NewVersion(minVersion) + supported, err := version.NewVersion(MinimumVersion) if err != nil { return fmt.Errorf("could not parse supported Azure CLI version: %+v", err) } - if nextMajorVersion != nil { - nextMajor, err := version.NewVersion(*nextMajorVersion) - if err != nil { - return fmt.Errorf("could not parse next major Azure CLI version: %+v", err) - } + nextMajor, err := version.NewVersion(NextMajorVersion) + if err != nil { + return fmt.Errorf("could not parse next major Azure CLI version: %+v", err) + } - if nextMajor.LessThanOrEqual(actual) { - return fmt.Errorf("unsupported Azure CLI version %q detected, please install a version newer than %s but older than %s", actual, supported, nextMajor) - } + if nextMajor.LessThanOrEqual(actual) { + return fmt.Errorf("unsupported Azure CLI version %q detected, please install a version newer than %s but older than %s", actual, supported, nextMajor) } if actual.LessThan(supported) { @@ -45,24 +48,25 @@ func CheckAzVersion(currentVersion string, minVersion string, nextMajorVersion * return nil } -// GetAzVersion tries to determine the version of Azure CLI in the path. -func GetAzVersion() (*string, error) { - var cliVersion *struct { - AzureCli *string `json:"azure-cli,omitempty"` - AzureCliCore *string `json:"azure-cli-core,omitempty"` - AzureCliTelemetry *string `json:"azure-cli-telemetry,omitempty"` - Extensions *interface{} `json:"extensions,omitempty"` - } - err := JSONUnmarshalAzCmd(&cliVersion, "version") +// CheckTenantID validates the supplied tenant ID, and tries to determine the default tenant if a valid one is not supplied. +func CheckTenantID(tenantId string) (*string, error) { + validTenantId, err := regexp.MatchString("^[a-zA-Z0-9._-]+$", tenantId) if err != nil { - return nil, fmt.Errorf("could not parse Azure CLI version: %v", err) + return nil, fmt.Errorf("could not parse tenant ID %q: %s", tenantId, err) } - if cliVersion.AzureCli == nil { - return nil, fmt.Errorf("could not detect Azure CLI version") + if !validTenantId { + var account struct { + ID string `json:"id"` + TenantID string `json:"tenantId"` + } + if err = JSONUnmarshalAzCmd(true, &account, "account", "show"); err != nil { + return nil, fmt.Errorf("obtaining tenant ID: %s", err) + } + tenantId = account.TenantID } - return cliVersion.AzureCli, nil + return &tenantId, nil } // GetDefaultSubscriptionID tries to determine the default subscription @@ -70,7 +74,7 @@ func GetDefaultSubscriptionID() (*string, error) { var account struct { SubscriptionID string `json:"id"` } - err := JSONUnmarshalAzCmd(&account, "account", "show") + err := JSONUnmarshalAzCmd(true, &account, "account", "show") if err != nil { return nil, fmt.Errorf("obtaining subscription ID: %s", err) } @@ -78,59 +82,74 @@ func GetDefaultSubscriptionID() (*string, error) { return &account.SubscriptionID, nil } -// CheckTenantID validates the supplied tenant ID, and tries to determine the default tenant if a valid one is not supplied. -func CheckTenantID(tenantId string) (*string, error) { - validTenantId, err := regexp.MatchString("^[a-zA-Z0-9._-]+$", tenantId) +// getAzVersion tries to determine the version of Azure CLI in the path. +func getAzVersion() (*string, error) { + var cliVersion *struct { + AzureCli *string `json:"azure-cli,omitempty"` + AzureCliCore *string `json:"azure-cli-core,omitempty"` + AzureCliTelemetry *string `json:"azure-cli-telemetry,omitempty"` + Extensions *interface{} `json:"extensions,omitempty"` + } + err := JSONUnmarshalAzCmd(true, &cliVersion, "version") if err != nil { - return nil, fmt.Errorf("could not parse tenant ID %q: %s", tenantId, err) + return nil, fmt.Errorf("could not parse Azure CLI version: %v", err) } - if !validTenantId { - var account struct { - ID string `json:"id"` - TenantID string `json:"tenantId"` - } - if err = JSONUnmarshalAzCmd(&account, "account", "show"); err != nil { - return nil, fmt.Errorf("obtaining tenant ID: %s", err) - } - tenantId = account.TenantID + if cliVersion.AzureCli == nil { + return nil, fmt.Errorf("could not detect Azure CLI version") } - return &tenantId, nil + return cliVersion.AzureCli, nil } // JSONUnmarshalAzCmd executes an Azure CLI command and unmarshalls the JSON output. -func JSONUnmarshalAzCmd(i interface{}, arg ...string) error { +func JSONUnmarshalAzCmd(cacheable bool, i interface{}, arg ...string) error { var stderr bytes.Buffer var stdout bytes.Buffer arg = append(arg, "-o=json") + argstring := strings.Join(arg, " ") - log.Printf("[DEBUG] az-cli invocation: az %s", strings.Join(arg, " ")) + var result []byte + if cacheable { + if cachedResult, ok := cache.Get(argstring); ok { + result = cachedResult + } + } - cmd := exec.Command("az", arg...) - cmd.Stderr = &stderr - cmd.Stdout = &stdout + if result == nil { + log.Printf("[DEBUG] az-cli invocation: az %s", argstring) - if err := cmd.Start(); err != nil { - err := fmt.Errorf("launching Azure CLI: %+v", err) - if stdErrStr := stderr.String(); stdErrStr != "" { - err = fmt.Errorf("%s: %s", err, strings.TrimSpace(stdErrStr)) + cmd := exec.Command("az", arg...) + cmd.Stderr = &stderr + cmd.Stdout = &stdout + + if err := cmd.Start(); err != nil { + err := fmt.Errorf("launching Azure CLI: %+v", err) + if stdErrStr := stderr.String(); stdErrStr != "" { + err = fmt.Errorf("%s: %s", err, strings.TrimSpace(stdErrStr)) + } + return err } - return err - } - if err := cmd.Wait(); err != nil { - err := fmt.Errorf("running Azure CLI: %+v", err) - if stdErrStr := stderr.String(); stdErrStr != "" { - err = fmt.Errorf("%s: %s", err, strings.TrimSpace(stdErrStr)) + if err := cmd.Wait(); err != nil { + err := fmt.Errorf("running Azure CLI: %+v", err) + if stdErrStr := stderr.String(); stdErrStr != "" { + err = fmt.Errorf("%s: %s", err, strings.TrimSpace(stdErrStr)) + } + return err } - return err + + result = stdout.Bytes() } - if err := json.Unmarshal(stdout.Bytes(), &i); err != nil { + if err := json.Unmarshal(result, &i); err != nil { return fmt.Errorf("unmarshaling the output of Azure CLI: %v", err) } + if cacheable { + cache.Set(argstring, result) + } + return nil } diff --git a/sdk/internal/azurecli/cache.go b/sdk/internal/azurecli/cache.go new file mode 100644 index 00000000000..93e645d460b --- /dev/null +++ b/sdk/internal/azurecli/cache.go @@ -0,0 +1,24 @@ +package azurecli + +var cache *cachedCliData + +type cachedCliData struct { + data map[string][]byte +} + +func (c *cachedCliData) Set(index string, data []byte) { + c.data[index] = data +} + +func (c *cachedCliData) Get(index string) ([]byte, bool) { + if data, ok := c.data[index]; ok { + return data, true + } + return nil, false +} + +func init() { + cache = &cachedCliData{ + data: make(map[string][]byte), + } +} diff --git a/sdk/internal/azurecli/versions.go b/sdk/internal/azurecli/versions.go index ac6faa79d58..ccbe084a8ff 100644 --- a/sdk/internal/azurecli/versions.go +++ b/sdk/internal/azurecli/versions.go @@ -3,5 +3,13 @@ package azurecli -// MsalVersion is the first known version of Azure CLI to support MSAL / v2 tokens -const MsalVersion = "2.30.0" +const ( + // MsalVersion is the first known version of Azure CLI to support MSAL / v2 tokens + MsalVersion = "2.30.0" + + // MinimumVersion is the oldest supported version of Azure CLI by this package + MinimumVersion = "2.0.81" + + // NextMajorVersion is the next (possibly upcoming) major version that is not yet supported by this package + NextMajorVersion = "3.0.0" +) From 6b76b0010ecfa63794ba13abc1462f610da76a8b Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Wed, 29 Nov 2023 21:38:13 +0000 Subject: [PATCH 3/7] azurecli: docstring and only cache command result when a new command is invoked --- sdk/internal/azurecli/azcli.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sdk/internal/azurecli/azcli.go b/sdk/internal/azurecli/azcli.go index e5e59cb8572..3b66a014beb 100644 --- a/sdk/internal/azurecli/azcli.go +++ b/sdk/internal/azurecli/azcli.go @@ -102,7 +102,8 @@ func getAzVersion() (*string, error) { return cliVersion.AzureCli, nil } -// JSONUnmarshalAzCmd executes an Azure CLI command and unmarshalls the JSON output. +// JSONUnmarshalAzCmd executes an Azure CLI command and unmarshalls the JSON output, optionally retrieving from and +// populating the command result cache, to avoid unnecessary repeated invocations of Azure CLI. func JSONUnmarshalAzCmd(cacheable bool, i interface{}, arg ...string) error { var stderr bytes.Buffer var stdout bytes.Buffer @@ -141,15 +142,15 @@ func JSONUnmarshalAzCmd(cacheable bool, i interface{}, arg ...string) error { } result = stdout.Bytes() + + if cacheable { + cache.Set(argstring, result) + } } if err := json.Unmarshal(result, &i); err != nil { return fmt.Errorf("unmarshaling the output of Azure CLI: %v", err) } - if cacheable { - cache.Set(argstring, result) - } - return nil } From 97e1d77c88369faebdc6b58a65c508c1250ceabf Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Thu, 30 Nov 2023 14:29:02 +0000 Subject: [PATCH 4/7] remove extraneous var declarations --- sdk/auth/azure_cli_authorizer.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/sdk/auth/azure_cli_authorizer.go b/sdk/auth/azure_cli_authorizer.go index e82b8040d41..4f44c3cec91 100644 --- a/sdk/auth/azure_cli_authorizer.go +++ b/sdk/auth/azure_cli_authorizer.go @@ -57,8 +57,6 @@ func (a *AzureCliAuthorizer) Token(_ context.Context, _ *http.Request) (*oauth2. return nil, fmt.Errorf("could not request token: conf is nil") } - var err error - azArgs := []string{"account", "get-access-token"} scope, err := environments.Scope(a.conf.Api) @@ -109,7 +107,6 @@ func (a *AzureCliAuthorizer) AuxiliaryTokens(_ context.Context, _ *http.Request) } azArgs := []string{"account", "get-access-token"} - var err error scope, err := environments.Scope(a.conf.Api) if err != nil { @@ -159,10 +156,8 @@ type azureCliConfig struct { // newAzureCliConfig validates the supplied tenant ID and returns a new azureCliConfig. func newAzureCliConfig(api environments.Api, tenantId string, auxiliaryTenantIds []string) (*azureCliConfig, error) { - var err error - // check az-cli version, ensure that MSAL is supported - if err = azurecli.CheckAzVersion(); err != nil { + if err := azurecli.CheckAzVersion(); err != nil { return nil, err } From 4063677a86af3ffd05b35b3dfd14ed726e69998d Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Thu, 30 Nov 2023 14:29:41 +0000 Subject: [PATCH 5/7] remove unused constants --- sdk/auth/azure_cli_authorizer.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/sdk/auth/azure_cli_authorizer.go b/sdk/auth/azure_cli_authorizer.go index 4f44c3cec91..de1c5d84a54 100644 --- a/sdk/auth/azure_cli_authorizer.go +++ b/sdk/auth/azure_cli_authorizer.go @@ -132,11 +132,6 @@ func (a *AzureCliAuthorizer) AuxiliaryTokens(_ context.Context, _ *http.Request) return tokens, nil } -const ( - AzureCliMinimumVersion = "2.0.81" - AzureCliNextMajorVersion = "3.0.0" -) - // azureCliConfig configures an AzureCliAuthorizer. type azureCliConfig struct { Api environments.Api From 349bd97a91e6949e546063ee421f90e00c09a56f Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Thu, 30 Nov 2023 14:30:24 +0000 Subject: [PATCH 6/7] remove unused field --- sdk/auth/azure_cli_authorizer.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/sdk/auth/azure_cli_authorizer.go b/sdk/auth/azure_cli_authorizer.go index de1c5d84a54..4f2930cb45f 100644 --- a/sdk/auth/azure_cli_authorizer.go +++ b/sdk/auth/azure_cli_authorizer.go @@ -144,9 +144,6 @@ type azureCliConfig struct { // DefaultSubscriptionID is the optional default subscription ID DefaultSubscriptionID string - - // CacheEnabled specifies whether to use module-level caching to speed up idempotent az-cli operations. - CacheEnabled bool } // newAzureCliConfig validates the supplied tenant ID and returns a new azureCliConfig. From 4c32852ac0804698d171f2e6bd555a9f47178af8 Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Tue, 12 Dec 2023 20:00:23 +0000 Subject: [PATCH 7/7] auth: split `azurecli.CheckTenantID` into `GetDefaultTenantID` and `ValidateTenantID` functions --- sdk/auth/azure_cli_authorizer.go | 19 +++++++++++++++---- sdk/internal/azurecli/azcli.go | 27 ++++++++++++++------------- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/sdk/auth/azure_cli_authorizer.go b/sdk/auth/azure_cli_authorizer.go index 4f2930cb45f..ac697ea301d 100644 --- a/sdk/auth/azure_cli_authorizer.go +++ b/sdk/auth/azure_cli_authorizer.go @@ -153,11 +153,22 @@ func newAzureCliConfig(api environments.Api, tenantId string, auxiliaryTenantIds return nil, err } - // check tenant ID - if defaultTenantId, err := azurecli.CheckTenantID(tenantId); err != nil { + // obtain default tenant ID if no tenant ID was provided + if strings.TrimSpace(tenantId) == "" { + if defaultTenantId, err := azurecli.GetDefaultTenantID(); err != nil { + return nil, fmt.Errorf("tenant ID was not specified and the default tenant ID could not be determined: %v", err) + } else if defaultTenantId == nil { + return nil, fmt.Errorf("tenant ID was not specified and the default tenant ID could not be determined") + } else { + tenantId = *defaultTenantId + } + } + + // validate tenant ID + if valid, err := azurecli.ValidateTenantID(tenantId); err != nil { return nil, err - } else if defaultTenantId != nil { - tenantId = *defaultTenantId + } else if !valid { + return nil, fmt.Errorf("invalid tenant ID was provided") } // get the default subscription ID diff --git a/sdk/internal/azurecli/azcli.go b/sdk/internal/azurecli/azcli.go index 3b66a014beb..caaffd8f48c 100644 --- a/sdk/internal/azurecli/azcli.go +++ b/sdk/internal/azurecli/azcli.go @@ -48,25 +48,26 @@ func CheckAzVersion() error { return nil } -// CheckTenantID validates the supplied tenant ID, and tries to determine the default tenant if a valid one is not supplied. -func CheckTenantID(tenantId string) (*string, error) { +// ValidateTenantID validates the supplied tenant ID, and tries to determine the default tenant if a valid one is not supplied. +func ValidateTenantID(tenantId string) (bool, error) { validTenantId, err := regexp.MatchString("^[a-zA-Z0-9._-]+$", tenantId) if err != nil { - return nil, fmt.Errorf("could not parse tenant ID %q: %s", tenantId, err) + return false, fmt.Errorf("could not parse tenant ID %q: %s", tenantId, err) } - if !validTenantId { - var account struct { - ID string `json:"id"` - TenantID string `json:"tenantId"` - } - if err = JSONUnmarshalAzCmd(true, &account, "account", "show"); err != nil { - return nil, fmt.Errorf("obtaining tenant ID: %s", err) - } - tenantId = account.TenantID + return validTenantId, nil +} + +// GetDefaultTenantID tries to determine the default tenant +func GetDefaultTenantID() (*string, error) { + var account struct { + TenantID string `json:"tenantId"` + } + if err := JSONUnmarshalAzCmd(true, &account, "account", "show"); err != nil { + return nil, fmt.Errorf("obtaining tenant ID: %s", err) } - return &tenantId, nil + return &account.TenantID, nil } // GetDefaultSubscriptionID tries to determine the default subscription