Federated authentication protocol support

[wrosenuance]

This is a pull request to add support for the federated authentication and security token library login protocols. 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.

It is a subset of the changes initially proposed in #547 that does not include the additional connection logging and documentation changes from that PR, which can be merged subsequently. It does not include the code to obtain the authentication tokens from Azure AD, which will be suggested separately.

[wrosenuance]

Hi @kardianos this is the first PR split out from #547 that focuses on just the mssql package.

[codecov]

Codecov Report

Merging #625 (5766e2d) into master (095ece7) will increase coverage by 0.37%.
The diff coverage is 69.15%.

@@            Coverage Diff             @@
##           master     #625      +/-   ##
==========================================
+ Coverage   71.55%   71.93%   +0.37%     
==========================================
  Files          23       24       +1     
  Lines        5280     5469     +189     
==========================================
+ Hits         3778     3934     +156     
- Misses       1292     1311      +19     
- Partials      210      224      +14     

Impacted Files	Coverage Δ
fedauth.go	52.00% <52.00%> (ø)
token_string.go	86.36% <62.50%> (-4.95%)	⬇️
tds.go	67.21% <64.92%> (+1.37%)	⬆️
token.go	60.92% <80.45%> (+4.80%)	⬆️
accesstokenconnector.go	76.92% <100.00%> (-3.08%)	⬇️
conn_str.go	100.00% <100.00%> (ø)
mssql.go	87.42% <100.00%> (+0.47%)	⬆️
... and 1 more

---

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 095ece7...5766e2d. Read the comment docs.

[kardianos]

This is more manageable.

I'm still looking at the general picture, please push back or question these requests:

• I'd like to combine the FederatedAuthenticationConfigurer with the SecurityTokenProvider and ActiveDirectoryTokenProvider functions. Then set that single interface directly on the connector. Would that be possible? I realize this may prevent a pure connection string connect from being used. That's okay for now.
• Remove SetFederatedAuthenticationConfigurer function as this is going through the connector now.
• Rather then parse information in fedauth.go, create an exported struct that the new combined interface (above) requests that contains all of this parsed information. Remove the remainder of fedauth.go.
• Is it possible to remove the special case within parseFeatureExtAck?
• The connect method is messy. I'd like to understand what's going on there better in the change, and maybe refactor the current code to better accommodate this new behavior. Even maybe have two paths, federated -> go here, otherwise, go there. Unsure.

[kardianos]

(I'm sorry for the duplicate message.)

This is more manageable.

I'm still looking at the general picture, please push back or question these requests:

• I'd like to combine the FederatedAuthenticationConfigurer with the SecurityTokenProvider and ActiveDirectoryTokenProvider functions. Then set that single interface directly on the connector. Would that be possible? I realize this may prevent a pure connection string connect from being used. That's okay for now.
• Remove SetFederatedAuthenticationConfigurer function as this is going through the connector now.
• Rather then parse information in fedauth.go, create an exported struct that the new combined interface (above) requests that contains all of this parsed information. Remove the remainder of fedauth.go.
• Is it possible to remove the special case within parseFeatureExtAck?
• The connect method is messy. I'd like to understand what's going on there better in the change, and maybe refactor the current code to better accommodate this new behavior. Even maybe have two paths, federated -> go here, otherwise, go there. Unsure.

[wrosenuance]

I'd like to combine the FederatedAuthenticationConfigurer with the SecurityTokenProvider and ActiveDirectoryTokenProvider functions. Then set that single interface directly on the connector. Would that be possible?
I realize this may prevent a pure connection string connect from being used. That's okay for now.

I am reluctant to break the pure connection string connect -- I guess it's possible though. Do you imagine everyone needs to use a special call to parse a DSN then that connects? I was hoping to support this style:

import (
  "database/sql"
  "net/url"

  // Import the Azure AD driver module (also imports the regular driver package)
  _ "github.com/denisenkom/go-mssqldb/azuread"
)

func ConnectWithMSI() (*sql.DB, error) {
  return sql.Open("sqlserver", "sqlserver://azuresql.database.windows.net?database=yourdb&fedauth=ActiveDirectoryMSI")
}

[kardianos]

I am reluctant to break the pure connection string connect -- I guess it's possible though. Do you imagine everyone needs to use a special call to parse a DSN then that connects? I was hoping to support this style:

Right now I want to focus on supporting:

sql.OpenDB(azuread.NewConnector("sqlserver://azuresql.database.windows.net?database=yourdb&fedauth=ActiveDirectoryMSI"))
or

sql.OpenDB(azuread.NewConnector(Options{
    Database: "yourdb",
    Option1: "abc",
}))

Where "azuread.NewConnector" might create a new mssql Connector and then assign it the correct interface to actuate it.

[wrosenuance]

I'd like to combine the FederatedAuthenticationConfigurer with the SecurityTokenProvider and ActiveDirectoryTokenProvider functions. Then set that single interface directly on the connector.

I think I have done this - I named the combined interface FederatedAuthenticationProvider and it has a function ConfigureProvider and then ProvideSecurityToken and ProvideActiveDirectoryToken.

Remove SetFederatedAuthenticationConfigurer function as this is going through the connector now.

Done.

Rather then parse information in fedauth.go, create an exported struct that the new combined interface (above) requests that contains all of this parsed information.

I did this by expanding the featureExtFedAuth that already kept a lot of the state for federated authentication, renaming to FederatedAuthenticationState and then using it more consistently in the login process.

Remove the remainder of fedauth.go.

I think this happened - I'll move the previous parts that were about loading certs into the azuread package, which just leaves the definitions in fedauth.go.

Is it possible to remove the special case within parseFeatureExtAck?

I'm not sure what you're referring to here!

The connect method is messy. I'd like to understand what's going on there better in the change, and maybe refactor the current code to better accommodate this new behavior. Even maybe have two paths, federated -> go here, otherwise, go there. Unsure.

I took a stab at this: I've split out more helper functions to more clearly show the sequence: send prelogin, read prelogin response, send login packet, loop waiting for login acknowledgement. Maybe send additional SSPI and federated authentication packets if needed.

Please let me know if this matches what you were expecting!

[kardianos]

This is looking great. I will go through this in much more detail now.

The only change I'll request at this time is to un-export all the constants, and just copy and paste them as needed into the other package, as they will never change. We don't need to create an explicit linkage to a package for a handful of constants.

[wrosenuance]

The only change I'll request at this time is to un-export all the constants

Ok!

[kardianos]

This looks great.

There is still an odd thing where we provide a DSN, then also need to provide a fed auth connector. I'd like to streamline this. Here are the next steps I would propose:

• In addition to the two requested changes, also temporarily un-export the struct and the new Connector field. But otherwise merge as-is.
• There was a recent request Export connection string parsing logic #624 to export connection string parsing logic. I think I would do this in a sub-package.
• Then I would expose a new function to create a connector from the parsed Params.
• After that, we streamline the process so that the fed auth package parses the DSN, and any extra parameters it wants, then passes a fully finished Conntector back. so we can run conn, err := fedauth.New("my-cool-dsn"); sql.OpenDB(conn).

If that sounds alright with you, then make the requested changes and un-export the structs for now. I'll write up a change to move the conn parameters around.

[wrosenuance]

There is still an odd thing where we provide a DSN, then also need to provide a fed auth connector.

I actually have pushed a second branch where I add in the AD token routines: so far that's not present in this PR, but it shows how I was planning to address this. Basically I added a driver in the azuread package that auto-populates the fed auth provider, so if you use that driver, then fed auth is supported without additional configuration. Do you want me to add that commit here?

You can see it at wrosenuance@1d891d2

There was a recent request #624 to export connection string parsing logic. I think I would do this in a sub-package.
Then I would expose a new function to create a connector from the parsed Params.

I saw that request recently and thought about trying to use it here, but later backtracked as it seemed that the data we needed was in the fed auth struct anyway, and we need to modify as well as read the parameters. We could do this if the parsed params were modifiable, otherwise the fed auth parsing would need to rewrite the DSN and reparse to make changes.

[kardianos]

Your approach for the fed auth package looks great.

Regarding parsing, you wouldn't need to parse it twice. The extra parameters would go into a bucket of unknown parameters that you could grab. Then as part of the setup, you can grab the parsed values, and the extra values, create the fed auth setup and return a connector.

I'm strongly in favor of this as it would remove the main driver from piping around values it itself doesn't need. If you're willing, I can merge this as is once the log is removed and the funcs are un-exported (for now), then we'll bring them back. That way we ensure the API is consistent, and we don't need to rebase the PR.

[wrosenuance]

Regarding parsing, you wouldn't need to parse it twice. The extra parameters would go into a bucket of unknown parameters that you could grab.

What I wanted to do that I'm not sure is anticipated by this is take the user / password supplied by the client and use them for other purposes - e.g. client cert password, or AD user / password - but modify them during parsing so that what was sent in the login packet would differ, since I didn't want to double-specify in the DSN.

On reflection, I think this is because I'm trying to overload the user/password fields in the DSN to avoid introducing lots of new DSN parameters. If we add more parameters then it's possible to keep the user/password for their original intended use.

I might make a few extra changes assuming that is what is going to happen, because it will affect a bit of what goes on in that fed auth struct (take out the username/password).

Instead, the ConfigureProvider() call could just return a fed auth library type, and based on that the tds.go code could further request the ADALWorkflow() from a separate call. Then there wouldn't be a need for the federated authentication state to be externally modified.

[kardianos]

That sounds great.

[wrosenuance]

The new version goes back to the earlier approach of providing the security token and AD token functions, but keeps them private rather than exported. Since we're expecting that the azuread package will call into the mssql package to create a connector, I have added some calls in fedauth.go for the two scenarios - security token and AD token:

• NewSecurityTokenConnector(dsn, tokenProvider) - configures for federated authentication with a security token
• NewActiveDirectoryTokenConnector(dsn, adalWorkflow, tokenProvider) - configures for federated authentication with AD token

This means there's no need to expose the struct anymore or to define the FederatedAuthenticationProvider. In the driver in the azuread package, I check the parameters and decide which of these to call - i.e. the parsing of fed auth related connection parameters is moved to the azuread package entirely.

For now I have the azuread package doing its own split of the connection parameters: when you have a chance to make that connect string PR, I think we can fairly easily use that structure instead. The revised azuread package is in wrosenuance@d5f257b

These new functions can be left as unexported for now if you prefer: I have exported them to be able to test the azuread package builds on them sensibly. Also if this is too different from what you were thinking I can go back to something closer to the previous iteration, but I think this is more in line with what you were aiming for in terms of having the mssql package less aware of fed auth related config stuff.

[kardianos]

Make private by renaming for this commit. We'll enable the API soon.

[wrosenuance]

Done - I initially added a commit, but I think you want this in the original commit, so I squashed and force pushed again.

[kardianos]

Make private by renaming for this commit. We'll enable the API soon.

[wrosenuance]

Done - I initially added a commit, but I think you want this in the original commit, so I squashed and force pushed again.

[wrosenuance]

Thanks! 🥳 Should I start a new PR for the Azure AD with ADAL implementation?

[kardianos]

You can start that if you want, but I'm gong to whip up a PR right now that adds in the connection string parsing. That will be required before any other change. So probably hold off.