Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AzureCliCredential to login with CLI #71

Merged
merged 49 commits into from
Jan 6, 2021

Conversation

JCZuurmond
Copy link
Contributor

This PR adds the option to use the authentication of the Azure CLI.

See this closed PR for context. IMHO the CLI login is the best default: you can login with your credentials wo storing them in plain text, you can login with a service principal (again wo storing credentials in plain text) and you can login with an interactive browser session. See here for all possible login options of azure.identitiy.

Copy link
Contributor Author

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

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

There are some differences because I started with dbt-synapse and then moved the code here so that it can be used in dbt-synapse once the inheritance issue is resolved.

dbt/adapters/sqlserver/connections.py Outdated Show resolved Hide resolved
dbt/adapters/sqlserver/connections.py Outdated Show resolved Hide resolved
dbt/adapters/sqlserver/connections.py Outdated Show resolved Hide resolved
@mikaelene
Copy link
Collaborator

I think this is a great add-on to the adapter, but it also makes the testing of different connections even more important.

@JCZuurmond
Copy link
Contributor Author

JCZuurmond commented Dec 29, 2020

Yes, I do not know how we can test this well. It would help to limit the options sql-server supports. With the CLI token we support multiple log in methods at once:

  1. Interactive browser
  2. User-password
  3. Service principal

@dataders
Copy link
Collaborator

for more context, the MSFT ODBC Active Directory connection types were built primarily for on-prem AD, rather than Azure AD and they aren't well-suited for using dbt on the command line. They work just fine for logging into a db w/ SSMS, but it would be best if we never had to:

  • store passwords & secrets in plaintext in ~/profiles.yml (requirement for ActiveDirectoryPassword and ActiveDirectoryServicePrincipal)
  • log-in with a pop-up every time you run a dbt command (requirement for ActiveDirectoryInteractive)
  • enable Active Directory Federation Services -- an outdated service that is way too much work for most users (requirement for ActiveDirectoryIntegrated)

Using azlogin is ideal because it handles securely caching credentials. @NandanHegde15 and I were just racking our brains on how we could do this when @JCZuurmond opened this PR and saved the day.

@mikaelene, when I'm back from PTO next week, addressing #65 will be the first PR. This PR can come after.

Copy link
Contributor Author

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

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

I'll limit the scope of this PR

dbt/adapters/sqlserver/connections.py Outdated Show resolved Hide resolved
dbt/adapters/sqlserver/connections.py Outdated Show resolved Hide resolved
dbt/adapters/sqlserver/connections.py Outdated Show resolved Hide resolved
@mikaelene
Copy link
Collaborator

@JCZuurmond, than you for limiting the scope of your PRs! Makes it much easier to approve. I have clients using this adapter for on-premise SQL Server only. So as long as that still works as before, I am happy to approve all PRs around how o connect to azure sql. As long as it's not to complicated. My only concern here is that it requires installation of the az cli client? But that's maybe not an issue?

@JCZuurmond
Copy link
Contributor Author

@mikaelene : As of now, this PR only adds an option for when type_auth is CLI, so nothing should break for your clients. The AzureCliCredential.get_token methods will raise an error when the Azure cli can not be invoked. We could explicitly add to the README that the Azure CLI is required to be installed, it is implied already.

@dataders dataders closed this Jan 6, 2021
@dataders dataders reopened this Jan 6, 2021
README.md Show resolved Hide resolved
@dataders dataders mentioned this pull request Jan 6, 2021
@dataders
Copy link
Collaborator

dataders commented Jan 6, 2021

@mikaelene this is good to go now!

Copy link
Collaborator

@mikaelene mikaelene left a comment

Choose a reason for hiding this comment

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

Great writeup in the readme! Thanks for all the great work you are putting in.

@mikaelene mikaelene merged commit 30f2e96 into dbt-msft:master Jan 6, 2021
@JCZuurmond JCZuurmond deleted the add-az-cli-auth branch January 6, 2021 14:43
@alittlesliceoftom
Copy link

Just shouting that I used this today, and it worked perfectly and easily, fantastic work @JCZuurmond , thank you!

This is a big upgrade in terms of usability and security IMO

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

Successfully merging this pull request may close these issues.

None yet

4 participants