-
Notifications
You must be signed in to change notification settings - Fork 502
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
Added support for federated authentication to enable Azure AD authentication #547
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #547 +/- ##
==========================================
- Coverage 71.98% 71.85% -0.14%
==========================================
Files 24 25 +1
Lines 5469 5529 +60
==========================================
+ Hits 3937 3973 +36
- Misses 1309 1325 +16
- Partials 223 231 +8
Continue to review full report at Codecov.
|
The low code coverage reported from AppVeyor is because I cannot enable AD logins and MSI features against their SQL server. Testing these features to get better coverage can be done manually - I have a guide at https://github.com/wrosenuance/go-mssqldb/blob/azure-auth/doc/how-to-test-azure-ad-authentication.md. If you would like automated testing for coverage I think this will require adding a mocking library (like counterfeiter) and revising much more of the structure to support injecting mocks. |
@wrosenuance I just sent a review #546 . I'm going to wait for that PR to merge before doing a review on this one, but doing a skim on this one I would request the following general changes:
|
Thanks @kardianos! I think @paulmey had reviewed this PR and had offered to rebase his contribution on top of this one, so that #546 is replaced by #548, with the expectation that it would merge after this one. Is it possible to apply them in that order instead? |
The key reason for integrating them is user convenience - if the modules are not integrated, driver users will have to call driver-specific constructors and possibly write code to wire in AD authentication, whereas the PR as proposed makes this configurable via connection string parameters that are available generically to users working through the driver-agnostic sql package.
I will look into this - I think you pointed out a few in your review of #546 that are pertinent here as well.
Ok I will check! |
I've merged #546, please rebase on master and I'll review this closely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't comprehensive, but just a first pass review. Please address initial notes and rebase on recent commit. Thanks.
d3532fe
to
948841d
Compare
Hi @kardianos and @paulmey! I pushed an update to the branch to rebase and merge with the PR #546 from @paulmey. I have tried to incorporate the requests above in terms of restructuring code flow and moving the ADAL code to a separate package. There are some drawbacks to this - even though it's possible to just switch the import to re-enable the ADAL authentication, it seems clumsy, and I can no longer run the existing test suite in the top-level package against an Azure SQL database authenticated via Active Directory. As this PR adds support for the ADAL workflow where the server provides the resource and STS URL, there needs to be a different function signature for providing tokens that accepts these, plus I think the context ought to be passed in to the existing access token connector. Since there was already an option to provide a custom Dialer in the existing Connector structure, I moved the security token and Active Directory providers there. Please let me know what you think of the revised code and approach - particularly whether it is okay to have the package separation as proposed here. |
@wrosenuance Thank you for the updates. There are a number of changes going on and it will take me a few days to review. Thank you for being patient with me and my requests so far. |
@kardianos, did you get a chance to review this yet? Please let me know if there is any way to help! |
Hi, anyone is still working on this? |
@serbrech I am still using my branch in my day job, I'm still waiting for a review for this. @kardianos - is this something you can pick up again? |
Sorry for the delay. This year has been a blast. I've stared this change, will try get to it. |
if it's easier, could we reduce the scope of the PR to add the different options one by one? |
I'm not sure it gets much easier: the main protocol changes are required either way, the exact method for obtaining credentials is a little more removed from the core of the change. |
7584f35
to
76b61aa
Compare
029641a
to
a3e03b9
Compare
Hello @wrosenuance - thanks for the great work on this. We have a use case where the changes enabled by this PR would greatly simplify the handling of Managed Identity (MSI) based authentication for the Telegraf sqlserver plugin. Do you have any plans to address the open comments? Is there anything I can do to help? |
Awesome!!
I have been waiting for @kardianos to take a look - after his initial review I made some changes but I have been holding off making many more as I'm waiting to get an indication of a path to merging this. For instance, if it made sense to split this up, it might have have a big effect on the way the PR proceeds. |
Oh! I see some recent changes from @denisenkom have introduced conflicts so I should also adapt the PR to match. 😅 |
8cc9b11
to
218b70d
Compare
Hi @denisenkom and @kardianos, I rebased this PR to work again after the recent commits from @denisenkom and added some more test cases to increase the coverage. Could you please review and let me know if anything else is needed for this to be merged? |
@wrosenuance Thank you for doing this. I just went through this an initial time in this form. This change is large. Would it be possible for you to open a new PR with changes just to the mssql package, with as few changes as possible to existing code? |
Thanks @kardianos! I will split it into a few PRs and link them here. I am thinking I can make the following large chunks:
Please let me know if that doesn't work! |
If you could start with just the changes to mssql package, both tokens and function changes, that would be great. |
218b70d
to
df5b6c5
Compare
I think we should revise this plan a bit and move to "github.com/Azure/azure-sdk-for-go/sdk/azidentity" func (s *Sqlcmd) GetTokenBasedConnection(connstr string, user string, password string) (driver.Connector, error) {
var cred azcore.TokenCredential
var err error
scope := ".default"
t := azureTenantId()
switch s.Connect.AuthenticationMethod {
case ActiveDirectoryDefault:
cred, err = azidentity.NewDefaultAzureCredential(nil)
case ActiveDirectoryInteractive:
cred, err = azidentity.NewInteractiveBrowserCredential(&azidentity.InteractiveBrowserCredentialOptions{TenantID: t, ClientID: getSqlClientId()})
// this scope may not be needed
scope = "user_impersonation"
case ActiveDirectoryPassword:
cred, err = azidentity.NewUsernamePasswordCredential(t, getSqlClientId(), user, password, nil)
case ActiveDirectoryManagedIdentity:
cred, err = azidentity.NewManagedIdentityCredential(user, nil)
case ActiveDirectoryServicePrincipal:
cred, err = azidentity.NewClientSecretCredential(t, user, password, nil)
default:
// no implementation of AAD Integrated yet
cred, err = azidentity.NewDefaultAzureCredential(nil)
}
if err != nil {
return nil, err
}
resourceUrl := s.getResourceUrl()
conn, err := mssql.NewAccessTokenConnector(connstr, func() (string, error) {
opts := policy.TokenRequestOptions{Scopes: []string{resourceUrl + scope}}
tk, err := cred.GetToken(context.Background(), opts)
if err != nil {
return "", err
}
return tk.Token, err
})
return conn, err
} |
Handle error returned from a.Value()
This is a pull request to add support for the federated authentication and security token library login options. Federated authentication is used for Active Directory logins in Azure, particularly for user/password and managed identity logins, while the security token library handles AD application (service principal) authentication with client secrets or client certificates.
The PR includes a Terraform environment setup that provisions a VM and Azure SQL server with the appropriate role grants to verify that managed identity login is working, which is necessary to execute the full suite of tests. Tests that rely on Azure VM infrastructure are disabled if the environment settings do not provide the right DSN, to avoid impact to existing testing.
Aims to address several scenarios for #446.