Add SqlServer MSI Support #2

samfoxcode · 2021-06-25T18:31:51Z

Add the option for MSI Auth to Azure SQL Server. Expects a param to be passed to enable MSI Auth, useMsi=true.

update go-mssqldb module to pull in msi auth support
make code change in database/sqlserver/sqlserver.go to check for useMsi param
dynamically get database resource uri based on the server uri
adds adal module which is needed for fetching the msi token

PR from denisenkom/go-mssqldb that added msi support: #546

Add MSI Auth option to SQL Server connection. Default if no password is provided in connection string.

update host name parsing to get just the resource endpoint for msi

update go-mssqldb to resolve panic issue referenced here: denisenkom/go-mssqldb#642

switch from deprecated methods. NewServicePrincipalTokenFromManagedIdentity calls the two methods that are deprecated for us

add useMsi param instead of looking for nil password

Update sqlserver readme for msi auth. make useMsi a bit safer

remove comment

kfcampbell · 2021-06-28T17:30:06Z

database/sqlserver/sqlserver.go

+
+	var db *sql.DB
+	if useMsi {
+		tokenProvider, err := getMSITokenProvider(fmt.Sprintf("%s%s", "https://", strings.Join(strings.Split(purl.Hostname(), ".")[1:], ".")))


Could this line be refactored into a helper in order to make its purpose more clear and maybe make it testable? I'm a little worried about some of these string operations being brittle with unexpected input.

yeah, I agree. The main purpose here is to get the resource uri which could change across clouds. I'll see what I can do. Currently, if the uri is incorrect, I believe it would result in a 4xx (invalid resource uri for tenant or something similar). Will validate that behavior and update

Sounds good, thank you! The cloud change use case is not something I had picked up from reading the code but that's super important.

database/sqlserver/README.md

kfcampbell · 2021-06-28T17:31:37Z

database/sqlserver/sqlserver.go

+	var db *sql.DB
+	if useMsi {
+		tokenProvider, err := getMSITokenProvider(fmt.Sprintf("%s%s", "https://", strings.Join(strings.Split(purl.Hostname(), ".")[1:], ".")))
+		if err != nil {


Out of curiosity, what happens if I pass useMsi as true but there's no provider available? Is it a timeout or an immediate failure?

if the resource uri is invalid it'll timeout, but if there isn't an identity available it seems to just hang - looking into why

Sounds good! I'm trying to think like the maintainer of the upstream package might and try to identify any failure modes introduced. I don't think there's necessarily a specific action item here and we shouldn't block merging on this comment, but I am certainly curious.

kfcampbell · 2021-06-28T17:33:09Z

database/sqlserver/sqlserver.go

+			return nil, err
+		}
+
+		db = sql.OpenDB(connector)


What's the difference between this sql.OpenDB() call and the below sql.Open() call? What's the reason for using two different methods to open the database?

sql.Open will actually end up calling sql.OpenDB behind the scenes, but it builds the connector first based on what's passed in/the db context. sql.OpenDB just expects a connector, which is this case we're building ourselves, so we can call sql.OpenDB directly in that case. This follows the mssql guidance for using msi connections: mssql msi

source code which has sql.Open and sql.OpenDB (sorry, can't permalink to sql.Open): https://golang.org/src/database/sql/sql.go

Awesome, thanks for the explanation!

kfcampbell · 2021-06-28T17:34:54Z

database/sqlserver/sqlserver.go

@@ -344,3 +374,16 @@ func (ss *SQLServer) ensureVersionTable() (err error) {

 	return nil
 }
+
+func getMSITokenProvider(resource string) (func() (string, error), error) {


Do you mind briefly walking me through the decision to use a higher order function here? What is it buying us?

Ohh, I see in this PR it's due to the short-lived nature of AAD tokens and this function ensures a refresh.

mssql.NewAccessTokenConnector expects a callback which is used to build the connection and that will be used to retrieve fresh tokens. msi.EnsureFresh() will update the token if it is about to expire, and then msi.OAuthToken() gets the token to be used for the connection

kfcampbell · 2021-06-28T17:46:17Z

I like these changes and I'm looking forward to being able to use them in the usage service! Managed identity SQL auth will save us a lot of headaches around rolling passwords and disaster recovery.

Do you have thoughts on the strategy to merge with the upstream repo after this merges in your branch? Some idle thoughts:

Their contributing docs aren't suuuper helpful.
Maybe it's best to merge here and then open up a PR in their repo.
@taywrobel works here and has committed to the project in the past. He might be able to help us or tell us what he did to get his CockroachDB changes in.

Co-authored-by: Keegan Campbell <[email protected]>

samfoxcode · 2021-06-28T20:12:46Z

I like these changes and I'm looking forward to being able to use them in the usage service! Managed identity SQL auth will save us a lot of headaches around rolling passwords and disaster recovery.

Do you have thoughts on the strategy to merge with the upstream repo after this merges in your branch? Some idle thoughts:

Their contributing docs aren't suuuper helpful.

Maybe it's best to merge here and then open up a PR in their repo.

@taywrobel works here and has committed to the project in the past. He might be able to help us or tell us what he did to get his CockroachDB changes in.

that was my initial thought too, have it reviewed here first then merge into the fork's main, then open a PR against the upstream

kfcampbell · 2021-06-28T21:28:41Z

That sounds good to me! I'm looking forward to seeing this PR on the main module. I won't "approve" this for now since I'm not quite sure of the etiquette in this fork scenario, but I'd definitely be interested in getting the maintainers' feedback soon.

refactor resource uri logic into its own method

add tests for msi connection. can only test whether it fails with useMsi= true, or succeeds with useMsi=false

samfoxcode added 7 commits June 16, 2021 14:42

MSI Auth for SQL Server

710e0ce

Add MSI Auth option to SQL Server connection. Default if no password is provided in connection string.

Parse resource endpoint from server for msi

04c3f27

update host name parsing to get just the resource endpoint for msi

Update go-mssqldb

2ff2841

update go-mssqldb to resolve panic issue referenced here: denisenkom/go-mssqldb#642

Update sqlserver.go

dba53bb

switch from deprecated methods. NewServicePrincipalTokenFromManagedIdentity calls the two methods that are deprecated for us

Update sqlserver.go

737f026

add useMsi param instead of looking for nil password

Update sqlserver readme

0c7286c

Update sqlserver readme for msi auth. make useMsi a bit safer

Update sqlserver.go

78f2d54

remove comment

kfcampbell reviewed Jun 28, 2021

View reviewed changes

database/sqlserver/README.md Outdated Show resolved Hide resolved

kfcampbell reviewed Jun 28, 2021

View reviewed changes

Update database/sqlserver/README.md

f7dc122

Co-authored-by: Keegan Campbell <[email protected]>

samfoxcode added 2 commits June 29, 2021 11:01

Update sqlserver.go

138aa4d

refactor resource uri logic into its own method

Update sqlserver_test.go

c5844f8

add tests for msi connection. can only test whether it fails with useMsi= true, or succeeds with useMsi=false

samfoxcode merged commit e898ae1 into master Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SqlServer MSI Support #2

Add SqlServer MSI Support #2

samfoxcode commented Jun 25, 2021 •

edited

Loading

kfcampbell Jun 28, 2021

samfoxcode Jun 28, 2021

kfcampbell Jun 28, 2021

kfcampbell Jun 28, 2021

samfoxcode Jun 28, 2021

kfcampbell Jun 28, 2021

kfcampbell Jun 28, 2021

samfoxcode Jun 28, 2021

kfcampbell Jun 28, 2021

kfcampbell Jun 28, 2021

kfcampbell Jun 28, 2021

samfoxcode Jun 28, 2021

kfcampbell commented Jun 28, 2021

samfoxcode commented Jun 28, 2021

kfcampbell commented Jun 28, 2021

Add SqlServer MSI Support #2

Add SqlServer MSI Support #2

Conversation

samfoxcode commented Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfcampbell commented Jun 28, 2021

samfoxcode commented Jun 28, 2021

kfcampbell commented Jun 28, 2021

samfoxcode commented Jun 25, 2021 •

edited

Loading