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

fix: Fixed azure cli authorization#461

Open
amanenk wants to merge 3 commits intocloudquery:mainfrom
amanenk:fix_auth
Open

fix: Fixed azure cli authorization#461
amanenk wants to merge 3 commits intocloudquery:mainfrom
amanenk:fix_auth

Conversation

@amanenk
Copy link
Copy Markdown
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
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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