Added support for federated authentication to enable Azure AD authentication

[wrosenuance]

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.

[codecov]

Codecov Report

Merging #547 (df5b6c5) into master (045585d) will decrease coverage by 0.13%.
The diff coverage is 79.36%.

@@            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     

Impacted Files	Coverage Δ
conn_str.go	99.18% <25.00%> (-0.82%)	⬇️
tds.go	65.41% <33.33%> (-2.19%)	⬇️
fedauth.go	52.00% <100.00%> (ø)
log_conn.go	100.00% <100.00%> (ø)
token.go	61.08% <0.00%> (+0.15%)	⬆️

---

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 045585d...df5b6c5. Read the comment docs.

[wrosenuance]

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.

[kardianos]

@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:

• If possible, inject the adal functionality if possible (so this module doesn't direclty depend on it. I like how AAD Authentication using AccessToken #546 does it. If not possible, that might be fine, but I'll need to understand why. It looks like adal package is only used in a single function, so it may be possible to inject that in the connector or similar.
• Please replace if/else chains with either early returns or switch statements.
• We will want to add a go.mod file under the example folder, AAD Authentication using AccessToken #546 may do that.

[wrosenuance]

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?

[wrosenuance]

• If possible, inject the adal functionality if possible (so this module doesn't direclty depend on it. I like how AAD Authentication using AccessToken #546 does it. If not possible, that might be fine, but I'll need to understand why. It looks like adal package is only used in a single function, so it may be possible to inject that in the connector or similar.

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.

• Please replace if/else chains with either early returns or switch statements.

I will look into this - I think you pointed out a few in your review of #546 that are pertinent here as well.

• We will want to add a go.mod file under the example folder, AAD Authentication using AccessToken #546 may do that.

Ok I will check!

[kardianos]

I've merged #546, please rebase on master and I'll review this closely.

[kardianos]

This isn't comprehensive, but just a first pass review. Please address initial notes and rebase on recent commit. Thanks.

[wrosenuance]

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.

[kardianos]

@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.

[wrosenuance]

@kardianos, did you get a chance to review this yet? Please let me know if there is any way to help!

[serbrech]

Hi, anyone is still working on this?

[wrosenuance]

@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?

[kardianos]

Sorry for the delay. This year has been a blast. I've stared this change, will try get to it.

[serbrech]

if it's easier, could we reduce the scope of the PR to add the different options one by one?
for example, I'm only interesting in the MSI one. Happy to help land it if needed.

[wrosenuance]

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.

[arvindshmicrosoft]

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?

[wrosenuance]

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

Awesome!!

Do you have any plans to address the open comments? Is there anything I can do to help?

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.

[wrosenuance]

Oh! I see some recent changes from @denisenkom have introduced conflicts so I should also adapt the PR to match. 😅

[wrosenuance]

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?

[kardianos]

@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?

[wrosenuance]

Thanks @kardianos! I will split it into a few PRs and link them here. I am thinking I can make the following large chunks:

• add the connection logger code that is used to dump the packet-level activity
• add the new token types and structure definitions - I think these can exist before being used
• add the main functional changes to the mssql package
• add the ADAL token routines from the azuread package
• add the documentation

Please let me know if that doesn't work!

[kardianos]

If you could start with just the changes to mssql package, both tokens and function changes, that would be great.

[shueybubbles]

I think we should revise this plan a bit and move to "github.com/Azure/azure-sdk-for-go/sdk/azidentity"
It provides a broad set of credential types with little extra code needed in the driver.
Here's a piece of code from an app I'm building which uses azidentity on top of go-mssqldb
The big missing piece for me is the lack of server-provided tenant and resource url.
Also, we need to support an application-provided client id.

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
}