prelogin fields data structure breaks when using access token for federated auth with AWS RDS Proxy MS SQL support

[mattgillard]

Describe the bug

1. When i use connection string:

odbc:server=%s;password={%s};port=%d;fedauth=ActiveDirectoryServicePrincipalAccessToken;
A valid token as passed as the password, and an AWS RDS MS SQL Proxy configured as the server, I get this error:

2023/01/29 12:16:08 Error preparing SQL statement:federated authentication is not supported by the server

2. Below extract from the TDS spec v33 found here:

Says that If the server received preloginFEDAUTHREQUIRED - it MUST respond as above.

3. In the prelogin response, as there is no valid preloginFEDAUTHREQUIRED index (6) the check here passes through to the else statement. (see below for debugger screenshots)

else if fe.FedAuthLibrary != FedAuthLibraryReserved {
			return 0, fmt.Errorf("federated authentication is not supported by the server")

I worked around this issue by removing the else. I am not sure if this is a problem with this library, or if its an issue with the AWS implementation of the protocol. (see below for example dotnet program which works fine - maybe the dotnet implementation doesn't have a similar check?)

See diffs for fix:

Exception message: 2023/01/29 12:16:08 Error preparing SQL statement:federated authentication is not supported by the server

To Reproduce
Include a complete code listing that we can run to reproduce the issue.
See here for gist

Instructions as per comment in gist:

1. Create an RDS MS SQL Server (Express is fine for cheapness)
2. Create an RDS Proxy (plug in your requirements)

aws rds create-db-proxy \
    --db-proxy-name sqlproxy \
    --engine-family SQLSERVER  \
    --auth Description="MS SQL RDS Proxy",AuthScheme="SECRETS",SecretArn="arn:aws:secretsmanager:ap-southeast-2:1234567890:secret:rdsad5de9b2-a9be-4052-a448-ec5112025942-kyUTPL",IAMAuth="ENABLED",ClientPasswordAuthType="SQL_SERVER_AUTHENTICATION" \
    --role-arn "arn:aws:iam::1234567890:role/service-role/rds-proxy-role-1674792657645"\
    --vpc-subnet-ids "subnet-xxx" "subnet-yyy" \
    --vpc-security-group-ids sg-xxx

3. Register your RDS DB with the proxy:

aws rds register-db-proxy-targets \
    --db-proxy-name sqlproxy \
    --db-instance-identifiers "sqlexpress"

4. Ensure your IAM User/Role allows rds-db:connect as per AWS IAM docs
5. Enter resulting Proxy FQDN below in server variable
6. go run test.go

Expected behavior

I expect the following output (with my patch i get the correct output):

$ go run test.go
Connected!
Microsoft SQL Server 2019 (RTM-CU16-GDR) (KB5014353) - 15.0.4236.7 (X64) 
        May 29 2022 15:55:47 
        Copyright (C) 2019 Microsoft Corporation
        Express Edition (64-bit) on Windows Server 2016 Datacenter 10.0 <X64> (Build 14393: ) (Hypervisor)

Instead I get this output (without my patch):

$ go run query.go
Connected!
2023/01/29 12:04:02 Error preparing SQL statement:federated authentication is not supported by the server
exit status 1

Further technical details

SQL Server version: Any (SQL 2019 Express)
Operating system: Any (MacOS Ventura 13.1 M1)

Additional context

prelogin definitions are here:
https://github.com/microsoft/go-mssqldb/blob/main/tds.go#L104

.NET 6 works just fine with Microsoft.Data.SqlClient which implies to me its not an issue with the implementation of the protocol.
Working .NET code here.

Debugger screenshot prior to readPrelogin response:

And after readPrelogin (no key for preloginFEDAUTHREQUIRED):

[mattgillard]

Update: After looking at the dotnet SqlClient code - it makes sense why it works - the implementation logic is different for prelogin response.

I am guessing the switch statement never hits case (int)PreLoginOptions.FEDAUTHREQUIRED as that option is not returned. So technically Microsoft.Data.SqlClient is breaking the specification.

https://github.com/dotnet/SqlClient/blob/20d4c199923c9b4ea2ffd44d9304fcb306c5efb5/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs#L1054

[shueybubbles]

I think your scenario doesn't need to use the azuread package. There's a fedauth connector you can leverage that doesn't take any dependency on MSAL SDK for Go. Have you tried using NewSecurityTokenConnector directly?

[mattgillard]

sure - but I get the same problem - as I have to use the azuread driver for federation - I do not see any other option?
As soon as it sees fedauth in the connection string - it resets p.fedAuthLibrary from the default of mssql.FedAuthLibraryReserved as per:

go-mssqldb/azuread/configuration.go

Line 81 in 1576f3e

p.fedAuthLibrary = mssql.FedAuthLibraryADAL

I tried this:

connString := fmt.Sprintf("odbc:server=%s;port=%d;fedauth=FedAuthLibraryReserved;",
		server, port)
	//print(authToken)

	//conn, err := sql.Open(azuread.DriverName, connString)
	tokenProviderWithCtx := func(ctx context.Context) (string, error) {
		return authToken, nil
	}

	connector, err := mssql.NewConnectorWithAccessTokenProvider(connString, tokenProviderWithCtx)
	conn := sql.OpenDB(connector)

Maybe we need an extra fedauth option for non AAD federation which keeps p.fedAuthLibrary as default. But that would seem messy I guess as all the federation code is in the azuread package.

Technically it doesn't matter who is the token provider, azuread does the job except for this issue.

[shueybubbles]

azuread package is simply an AAD-specific federation implementation which implements the token fetch callback using MSAL SDK for Go. The federation implementation is part of the core mssql code, split between fedauth.go and tds.go. I think having a new method in fedauth.go to create a config that has both fedAuthRequired == true and fedAuthLibrary == FedAuthLibraryReserved for this scenario is probably ok.

Alternately - does it make sense for the prelogin response validation to check for both FedAuthLibrarySecurityToken and FedAuthLibraryReserved as possible inputs instead of just the one? After all, both values have the semantic that the server doesn't need to provide any information for the client to provide the token.

Pulling in azuread is just going to bloat your binary for little benefit.

[mattgillard]

I get it now. Leave it with me and I will find some time to try it. Thanks for the pointer!

[mattgillard]

How about this.

else if fe.FedAuthLibrary != FedAuthLibraryReserved && fe.ADALWorkflow > 0 {
		return 0, fmt.Errorf("federated authentication is not supported by the server")
	}

NewSecurityTokenConnector does not set the ADALWorkflow. I think you only care about the above error message if its actually going through the ADALWorkflow somehow and didn't get the preloginFEDAUTHREQUIRED response?

For a generic access token provided by the user the above should be enough as they just want to auth and we don't want to check for the support.

[shueybubbles]

sounds good to me! Thx for working it out. Now I need some other curious-and-motivated community members to add Always Encrypted support.