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

Conversation

amanenk
Copy link
Contributor

@amanenk amanenk commented Aug 12, 2022

Summary

closes cloudquery/cloudquery#1221


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines πŸ§‘β€πŸŽ“
  • Run go fmt to format your code πŸ–Š
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests. Learn more about testing here πŸ§ͺ
  • Update the docs by running go run ./docs/docs.go and committing the changes πŸ“ƒ
  • If adding a new resource, add relevant Terraform files in a separate PR πŸ“‚
  • Ensure the status checks below are successful βœ…

@amanenk amanenk marked this pull request as ready for review August 12, 2022 16:38
@amanenk amanenk requested review from a team and hermanschaaf and removed request for a team August 12, 2022 16:38
@@ -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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure Service Principal Auth failing for accounts, subscriptions
2 participants