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

Added support for federated authentication to enable Azure AD authentication #547

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

wrosenuance
Copy link
Contributor

@wrosenuance wrosenuance commented Jan 21, 2020

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
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

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

Impacted file tree graph

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

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 wrosenuance requested review from kardianos and removed request for denisenkom January 30, 2020 19:40
@kardianos
Copy link
Collaborator

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

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
Copy link
Contributor Author

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

Ok I will check!

@kardianos
Copy link
Collaborator

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

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 isn't comprehensive, but just a first pass review. Please address initial notes and rebase on recent commit. Thanks.

tds.go Outdated Show resolved Hide resolved
fedauth.go Outdated Show resolved Hide resolved
fedauth.go Outdated Show resolved Hide resolved
fedauth.go Outdated Show resolved Hide resolved
@wrosenuance wrosenuance force-pushed the azure-auth branch 2 times, most recently from d3532fe to 948841d Compare February 3, 2020 06:55
@wrosenuance
Copy link
Contributor Author

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 wrosenuance requested a review from kardianos February 3, 2020 07:24
@kardianos
Copy link
Collaborator

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

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

@serbrech
Copy link

serbrech commented Sep 2, 2020

Hi, anyone is still working on this?

@wrosenuance
Copy link
Contributor Author

@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
Copy link
Collaborator

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

@serbrech
Copy link

serbrech commented Sep 2, 2020

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
Copy link
Contributor Author

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.

README.md Outdated Show resolved Hide resolved
azuread/fedauth_adal.go Outdated Show resolved Hide resolved
mssql.go Outdated Show resolved Hide resolved
azuread/fedauth_adal.go Outdated Show resolved Hide resolved
@arvindshmicrosoft
Copy link

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@wrosenuance wrosenuance force-pushed the azure-auth branch 5 times, most recently from 8cc9b11 to 218b70d Compare December 15, 2020 23:28
@wrosenuance
Copy link
Contributor Author

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
Copy link
Collaborator

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

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
Copy link
Collaborator

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

@shueybubbles
Copy link
Contributor

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
}

ninjadq pushed a commit to ninjadq/go-mssqldb that referenced this pull request May 28, 2024
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.

5 participants