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

AAD Authentication using AccessToken #546

Merged
merged 20 commits into from Jan 31, 2020
Merged

Conversation

paulmey
Copy link
Contributor

@paulmey paulmey commented Jan 21, 2020

Although it leaves room for improvement with respect to convenience from the DSN, this PR implements AAD authentication through access tokens.
To test this implementation, create a SQL Azure database and set yourself as the AAD admin for the database. Then run

az login # if not already logged in
at=$(az account get-access-token --resource https://database.windows.net/ --q accessToken -o tsv)
export SQLSERVER_DSN="sqlserver://srv.database.windows.net?database=db&accesstoken=$at"
go test -v

To use a service principal, grant the service principal access to the database as described in the docs.

At least partially fixes #446

@codecov
Copy link

codecov bot commented Jan 21, 2020

Codecov Report

Merging #546 into master will decrease coverage by 0.19%.
The diff coverage is 61.29%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #546     +/-   ##
=========================================
- Coverage    68.9%   68.71%   -0.2%     
=========================================
  Files          22       23      +1     
  Lines        5068     5187    +119     
=========================================
+ Hits         3492     3564     +72     
- Misses       1370     1410     +40     
- Partials      206      213      +7
Impacted Files Coverage Δ
conn_str.go 100% <ø> (ø) ⬆️
token.go 54.54% <0%> (-1.37%) ⬇️
tds.go 61.03% <66.66%> (+0.69%) ⬆️
accesstokenconnector.go 80% <80%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d7a30a...6d60088. Read the comment docs.

@paulmey paulmey changed the title Authenticate to AAD using AccessToken AAD Authentication using AccessToken Jan 21, 2020
@paulmey
Copy link
Contributor Author

paulmey commented Jan 21, 2020

I'll try to raise code coverage a bit

Copy link
Collaborator

@kardianos kardianos left a comment

Choose a reason for hiding this comment

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

This looks really good. I only have nits.

The only real change I really want is to put a go.mod file in the example folder or at a min in the example/azure-managed-identity folder, so the mssql module doesn't depend on adal.

README.md Outdated Show resolved Hide resolved
accesstokenconnector_test.go Outdated Show resolved Hide resolved
examples/azure-managed-identity/managed_identity.go Outdated Show resolved Hide resolved
tds.go Outdated Show resolved Hide resolved
tds.go Outdated Show resolved Hide resolved
tds_test.go Outdated Show resolved Hide resolved
tds_test.go Show resolved Hide resolved
tds_test.go Outdated Show resolved Hide resolved
token.go Outdated Show resolved Hide resolved
token.go Outdated Show resolved Hide resolved
@paulmey
Copy link
Contributor Author

paulmey commented Jan 31, 2020

I added a go.mod in the specific example folder and fixed most of the nits. The go.mod in the example need to be removed to make the example work until this gets merged, but hopefully that's only going to be temporary.

@kardianos kardianos merged commit 0f454e2 into denisenkom:master Jan 31, 2020
rHermes pushed a commit to vippsas/go-mssqldb that referenced this pull request Mar 30, 2020
mssql: add Azure Active Directory token based login

Create a new connector that allows refreshing the access token
based on an external identifier.
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.

Azure Active Directory Authentication is not supported?
2 participants