From a7004e3a5a80592c6ef0976fd48b3f1ed950186c Mon Sep 17 00:00:00 2001 From: Charles Lowell Date: Mon, 25 Oct 2021 14:54:54 -0700 Subject: [PATCH 1/4] always invoke CLI from /bin or %SYSTEMROOT% --- sdk/azidentity/azure_cli_credential.go | 41 ++++++++++---------------- sdk/azidentity/logging.go | 3 -- 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/sdk/azidentity/azure_cli_credential.go b/sdk/azidentity/azure_cli_credential.go index 044ad9017166..bca58650289e 100644 --- a/sdk/azidentity/azure_cli_credential.go +++ b/sdk/azidentity/azure_cli_credential.go @@ -7,6 +7,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "os" "os/exec" @@ -74,7 +75,7 @@ func (c *AzureCLICredential) GetToken(ctx context.Context, opts policy.TokenRequ return at, nil } -const timeoutCLIRequest = 10000 * time.Millisecond +const timeoutCLIRequest = 10 * time.Second // authenticate creates a client secret authentication request and returns the resulting Access Token or // an error in case of authentication failure. @@ -91,44 +92,34 @@ func (c *AzureCLICredential) authenticate(ctx context.Context, resource string) func defaultTokenProvider() func(ctx context.Context, resource string, tenantID string) ([]byte, error) { return func(ctx context.Context, resource string, tenantID string) ([]byte, error) { - // This is the path that a developer can set to tell this class what the install path for Azure CLI is. - const azureCLIPath = "AZURE_CLI_PATH" - - // The default install paths are used to find Azure CLI. This is for security, so that any path in the calling program's Path environment is not used to execute Azure CLI. - azureCLIDefaultPathWindows := fmt.Sprintf("%s\\Microsoft SDKs\\Azure\\CLI2\\wbin; %s\\Microsoft SDKs\\Azure\\CLI2\\wbin", os.Getenv("ProgramFiles(x86)"), os.Getenv("ProgramFiles")) - - // Default path for non-Windows. - const azureCLIDefaultPath = "/bin:/sbin:/usr/bin:/usr/local/bin" - - // Validate resource, since it gets sent as a command line argument to Azure CLI - const invalidResourceErrorTemplate = "resource %s is not in expected format. Only alphanumeric characters, [dot], [colon], [hyphen], and [forward slash] are allowed" match, err := regexp.MatchString("^[0-9a-zA-Z-.:/]+$", resource) if err != nil { return nil, err } if !match { - return nil, fmt.Errorf(invalidResourceErrorTemplate, resource) + return nil, fmt.Errorf(`unexpected scope "%s". Only alphanumeric characters and ".", ";", "-", and "/" are allowed`, resource) } ctx, cancel := context.WithTimeout(ctx, timeoutCLIRequest) defer cancel() - // Execute Azure CLI to get token + commandLine := "az account get-access-token -o json --resource " + resource + if tenantID != "" { + commandLine += " --tenant " + tenantID + } var cliCmd *exec.Cmd if runtime.GOOS == "windows" { - cliCmd = exec.CommandContext(ctx, fmt.Sprintf("%s\\system32\\cmd.exe", os.Getenv("windir"))) - cliCmd.Env = os.Environ() - cliCmd.Env = append(cliCmd.Env, fmt.Sprintf("PATH=%s;%s", os.Getenv(azureCLIPath), azureCLIDefaultPathWindows)) - cliCmd.Args = append(cliCmd.Args, "/c", "az") + dir := os.Getenv("SYSTEMROOT") + if dir == "" { + return nil, errors.New("environment variable 'SYSTEMROOT' has no value") + } + cliCmd = exec.CommandContext(ctx, "cmd.exe", "/c", commandLine) + cliCmd.Dir = dir } else { - cliCmd = exec.CommandContext(ctx, "az") - cliCmd.Env = os.Environ() - cliCmd.Env = append(cliCmd.Env, fmt.Sprintf("PATH=%s:%s", os.Getenv(azureCLIPath), azureCLIDefaultPath)) - } - cliCmd.Args = append(cliCmd.Args, "account", "get-access-token", "-o", "json", "--resource", resource) - if tenantID != "" { - cliCmd.Args = append(cliCmd.Args, "--tenant", tenantID) + cliCmd = exec.CommandContext(ctx, "/bin/sh", "-c", commandLine) + cliCmd.Dir = "/bin" } + cliCmd.Env = os.Environ() var stderr bytes.Buffer cliCmd.Stderr = &stderr diff --git a/sdk/azidentity/logging.go b/sdk/azidentity/logging.go index 2a2613ac79fd..0003a20d53ca 100644 --- a/sdk/azidentity/logging.go +++ b/sdk/azidentity/logging.go @@ -38,9 +38,6 @@ func logEnvVars() { if envCheck := os.Getenv("AZURE_AUTHORITY_HOST"); len(envCheck) > 0 { envVars = append(envVars, "AZURE_AUTHORITY_HOST") } - if envCheck := os.Getenv("AZURE_CLI_PATH"); len(envCheck) > 0 { - envVars = append(envVars, "AZURE_CLI_PATH") - } if len(envVars) > 0 { log.Writef(EventCredential, "Azure Identity => Found the following environment variables:\n\t%s", strings.Join(envVars, ", ")) } From 67cd4512a17fa3f405b005deda89f3aee911ebb1 Mon Sep 17 00:00:00 2001 From: Charles Lowell Date: Mon, 25 Oct 2021 15:05:17 -0700 Subject: [PATCH 2/4] prevent CLI credential mutating input scopes --- sdk/azidentity/azure_cli_credential.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sdk/azidentity/azure_cli_credential.go b/sdk/azidentity/azure_cli_credential.go index bca58650289e..6947bc7468c0 100644 --- a/sdk/azidentity/azure_cli_credential.go +++ b/sdk/azidentity/azure_cli_credential.go @@ -64,9 +64,12 @@ func NewAzureCLICredential(options *AzureCLICredentialOptions) (*AzureCLICredent // opts: TokenRequestOptions contains the list of scopes for which the token will have access. // Returns an AccessToken which can be used to authenticate service client calls. func (c *AzureCLICredential) GetToken(ctx context.Context, opts policy.TokenRequestOptions) (*azcore.AccessToken, error) { - // The following code will remove the /.default suffix from the scope passed into the method since AzureCLI expect a resource string instead of a scope string - opts.Scopes[0] = strings.TrimSuffix(opts.Scopes[0], defaultSuffix) - at, err := c.authenticate(ctx, opts.Scopes[0]) + if len(opts.Scopes) != 1 { + return nil, errors.New("this credential requires exactly one scope per token request") + } + // CLI expects an AAD v1 resource, not a v2 scope + scope := strings.TrimSuffix(opts.Scopes[0], defaultSuffix) + at, err := c.authenticate(ctx, scope) if err != nil { addGetTokenFailureLogs("Azure CLI Credential", err, true) return nil, err From fab7a66bf8d6fe4b1fa55a92f90735bbe798e757 Mon Sep 17 00:00:00 2001 From: Charles Lowell Date: Mon, 25 Oct 2021 15:07:12 -0700 Subject: [PATCH 3/4] stop reading AZURE_CLIENT_ID, _RESOURCE_ID for managed identities --- sdk/azidentity/managed_identity_credential.go | 16 +--------- .../managed_identity_credential_test.go | 31 ------------------- 2 files changed, 1 insertion(+), 46 deletions(-) diff --git a/sdk/azidentity/managed_identity_credential.go b/sdk/azidentity/managed_identity_credential.go index ad4159f616ea..fc9e2a5f17d7 100644 --- a/sdk/azidentity/managed_identity_credential.go +++ b/sdk/azidentity/managed_identity_credential.go @@ -6,7 +6,6 @@ package azidentity import ( "context" "fmt" - "os" "strings" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -89,20 +88,7 @@ func NewManagedIdentityCredential(options *ManagedIdentityCredentialOptions) (*M } // Assign the msiType discovered onto the client client.msiType = msiType - // check if no clientID is specified then check if it exists in an environment variable - id := cp.ID - if id == nil { - cID := os.Getenv("AZURE_CLIENT_ID") - if cID != "" { - id = ClientID(cID) - } else { - rID := os.Getenv("AZURE_RESOURCE_ID") - if rID != "" { - id = ResourceID(rID) - } - } - } - return &ManagedIdentityCredential{id: id, client: client}, nil + return &ManagedIdentityCredential{id: cp.ID, client: client}, nil } // GetToken obtains an AccessToken from the Managed Identity service if available. diff --git a/sdk/azidentity/managed_identity_credential_test.go b/sdk/azidentity/managed_identity_credential_test.go index 01a9cbb07bdd..4257cc37f6f7 100644 --- a/sdk/azidentity/managed_identity_credential_test.go +++ b/sdk/azidentity/managed_identity_credential_test.go @@ -704,34 +704,3 @@ func TestManagedIdentityCredential_CreateAccessTokenExpiresOnFail(t *testing.T) t.Fatalf("expected to receive an error but received none") } } - -func TestManagedIdentityCredential_ResourceID_envVar(t *testing.T) { - // setting a dummy value for IDENTITY_ENDPOINT in order to be able to get a ManagedIdentityCredential type - _ = os.Setenv("IDENTITY_ENDPOINT", "somevalue") - _ = os.Setenv("IDENTITY_HEADER", "header") - _ = os.Setenv("AZURE_RESOURCE_ID", "resource_id") - defer clearEnvVars("IDENTITY_ENDPOINT", "IDENTITY_HEADER", "AZURE_CLIENT_ID", "AZURE_RESOURCE_ID") - cred, err := NewManagedIdentityCredential(nil) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if cred.id != ResourceID("resource_id") { - t.Fatal("unexpected id value stored") - } - _ = os.Setenv("AZURE_RESOURCE_ID", "") - _ = os.Setenv("AZURE_CLIENT_ID", "client_id") - cred, err = NewManagedIdentityCredential(nil) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if cred.id != ClientID("client_id") { - t.Fatal("unexpected id value stored") - } - cred, err = NewManagedIdentityCredential(nil) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if cred.id != ClientID("client_id") { - t.Fatal("unexpected id value stored") - } -} From f2bbb8c81073cbc78f10b5d9ea97c1a9943025a3 Mon Sep 17 00:00:00 2001 From: Charles Lowell Date: Mon, 25 Oct 2021 15:10:03 -0700 Subject: [PATCH 4/4] changelog --- sdk/azidentity/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sdk/azidentity/CHANGELOG.md b/sdk/azidentity/CHANGELOG.md index ec382cd83165..dcde20193d9d 100644 --- a/sdk/azidentity/CHANGELOG.md +++ b/sdk/azidentity/CHANGELOG.md @@ -51,6 +51,12 @@ * Credential options structs now embed `azcore.ClientOptions`. In addition to changing literal initialization syntax, this change renames `HTTPClient` fields to `Transport`. * Renamed `LogCredential` to `EventCredential` +* `AzureCLICredential` no longer reads the environment variable `AZURE_CLI_PATH` +* `NewManagedIdentityCredential` no longer reads environment variables `AZURE_CLIENT_ID` and + `AZURE_RESOURCE_ID`. Use `ManagedIdentityCredentialOptions.ID` instead. + +### Bugs Fixed +* `AzureCLICredential.GetToken` no longer mutates its `opts.Scopes` ### Features Added * Added connection configuration options to `DefaultAzureCredentialOptions`