Skip to content
This repository has been archived by the owner on Aug 16, 2022. It is now read-only.

fix: Fixed azure cli authorization #461

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: Fixed azure cli authorization #461

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
242c61f
fix: Fixed azure cli authorization
amanenk Aug 12, 2022
97826d6
lint
amanenk Aug 12, 2022
1136b7f
lint
amanenk Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"

"github.com/Azure/azure-sdk-for-go/sdk/azcore"
"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
"github.com/Azure/azure-sdk-for-go/services/subscription/mgmt/2020-09-01/subscription"
// Import all autorest modules
Expand Down Expand Up @@ -72,6 +73,10 @@ func Configure(logger hclog.Logger, config interface{}) (schema.ClientMeta, diag
logger.Info("Trying to authenticate via CLI (azidentity)")
var azCred azcore.TokenCredential
azCred, err = azidentity.NewAzureCLICredential(nil)
if err == nil {
// check token generation
_, err = azCred.GetToken(context.Background(), policy.TokenRequestOptions{Scopes: []string{"https://management.core.windows.net//.default"}})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amanenk Does this fix the principal auth issue, or only report the error to the user earlier? Looking at this doc, it looks like we may want to try using DefaultAzureCredential. Maybe we should even put it in a ChainedTokenCredential to fit with Azure's SDK conventions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I'm reading the error message from the original issue more carefully now, and it suggests that the issue was that the az CLI wasn't found on the path. So what you're doing here is checking that we can auth using that method, and if not, falling through to environment variables. I see! I still think we may want to use a ChainedTokenCredential instead, so that the Azure SDK will go through all the auth options for us if one fails.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amanenk I also made an attempted fix here, using the default credential chain: cloudquery/cloudquery#1283

Let's chat about how we want to proceed: perhaps we can release your fix as a release here (the final one to be released on this repo), and the default credential one only on the monorepo, since that's also a breaking change.

}
if err != nil {
logger.Info("Trying to authenticate via environment variables (azidentity)")
azCred, err = azidentity.NewEnvironmentCredential(nil)
Expand Down