xo / usql

Universal command-line interface for SQL databases
MIT License
8.82k stars 346 forks source link

Can't connect via Azure Active Directory authentication #380

Closed reubano closed 11 months ago

reubano commented 1 year ago

Following go-mssqldb

user@server:~$ go install -tags sqlserver github.com/xo/usql@8a0cccf
user@server:~$ usql --version
usql 0.0.0-dev
user@server:~$ usql 'sqlserver://xxx.database.windows.net?database=xxx&user id=xxx&password=xxx'
Connected with driver sqlserver (Microsoft SQL Server x.x, RTM, SQL Azure)
Type "help" for help.

ms:xxx=>
user@server:~$ usql 'azuresql://xxx.database.windows.net?database=xxx&fedauth=ActiveDirectoryMSI'
error: sqlserver: 18456: login error: Login failed for user ''.
Enter password:
user@server:~$ usql 'azuresql://xxx.database.windows.net?database=xxx&fedauth=ActiveDirectoryMSI&user id=xxx'
error: sqlserver: 18456: login error: Login failed for user 'xxx'.
Enter password:
kenshaw commented 1 year ago

I have not actually had an opportunity to test usql directly with Azure's SQL Server stuff. I'm reading through the docs now, as I'm working off the assumption that something is missing/incomplete. Bare with me, please.

kenshaw commented 1 year ago

@reubano I just pushed a commit that should fix this issue. Could you checkout this latest revision and see if you're able to connect? I don't have a Active Directory (nor a Azure account) to work with to test this myself. Thanks in advance.

reubano commented 1 year ago
user@server:~$ usql 'azuresql://xxx.database.windows.net?database=xxx&fedauth=ActiveDirectoryMSI'
error: sqlserver: 18456: login error: Login failed for user '<token-identified principal>'.
Enter password:
user@server:~$ usql 'azuresql://xxx.database.windows.net?database=xxx&fedauth=ActiveDirectoryMSI&user id=xxx'
error: sqlserver: ManagedIdentityCredential authentication failed
GET http://xxx.xxx.xxx.xxx/metadata/identity/oauth2/token
--------------------------------------------------------------------------------
RESPONSE 400 Bad Request
--------------------------------------------------------------------------------
{
  "error": "invalid_request",
  "error_description": "Identity not found"
}
--------------------------------------------------------------------------------

2nd error seems plausible since the user id is system-assigned but the docs say it expects user-assigned.

kenshaw commented 1 year ago

@reubano please report if you're able to actually get this working. I'd like to tag a new version of usql if possible.

reubano commented 1 year ago

@kenshaw I tried your new commit and got the errors listed above.

kenshaw commented 1 year ago

@reubano yes, thank you for that. Are you expecting the credentials there to work? Would it be possible for you to try to write some Go code to actually connect? As I'm unable to test, I'm hesitant on tagging a new version until I can at least get a report that there is a successful connection using the new code.

reubano commented 1 year ago

Yes I expect the first command to connect since when a user id isnt specified, it should use the system assigned managed identity. Ill try to find some go code that works in this case. Im not a go dev so if you have any recommendations I’ll happily give it a shot.

jdstone commented 1 year ago

@reubano @kenshaw,

Did this issue ever get resolved?

@kenshaw I was able to write some Go code. This produces the same error I got when using usql, which is the same error @reubano described. Here is the exact error I got with some sensitive values scrubbed.

$ usql "azuresql://xxx.database.windows.net:1433?database=xxx&fedauth=ActiveDirectoryManagedIdentity&user id=name_of_user-assigned_managed_identity"
error: sqlserver: ManagedIdentityCredential authentication failed
GET http://169.254.169.254/metadata/identity/oauth2/token
--------------------------------------------------------------------------------
RESPONSE 400 Bad Request
--------------------------------------------------------------------------------
{
  "error": "invalid_request",
  "error_description": "Identity not found"
}
--------------------------------------------------------------------------------
To troubleshoot, visit https://aka.ms/azsdk/go/identity/troubleshoot#managed-id

When I run my code, which is attached, I receive the following error. mssqltest.go.txt

2023/01/25 20:08:51 ManagedIdentityCredential authentication failed
GET http://169.254.169.254/metadata/identity/oauth2/token
--------------------------------------------------------------------------------
RESPONSE 400 Bad Request
--------------------------------------------------------------------------------
{
  "error": "invalid_request",
  "error_description": "Identity not found"
}
--------------------------------------------------------------------------------
exit status 1

Running the same usql command or the same Go code with this connection URL succeeds.

sqlserver://username:password@xxx.database.windows.net:1433?database=db_name
kenshaw commented 1 year ago

@jdstone I have no way of testing this, and @reubano has not reported back here.

reubano commented 1 year ago

@kenshaw what do I need to report on now? @jdstone has provided go code above.

kenshaw commented 1 year ago

@reubano I had made a change to the usql code. It's been long released. Would be great to know if it actually fixed the issue for you.

reubano commented 1 year ago

@kenshaw I tested https://github.com/xo/usql/commit/e10b9e8f0f663c83ccce662efd61ec1bd24bbd06 (this comment). Are you referring to another commit I'm not aware of?

kenshaw commented 1 year ago

@reubano ok, thanks. If it doesn't work for you, and the code that @jdstone shared doesn't work, then its an issue with the github.com/microsoft/go-mssqldb/azuread driver, and needs to be opened as an issue on that project. Assuming they fix it, it'll get picked up / put into usql whenever I do the next dependency update.

reubano commented 1 year ago

@jdstone are you able to verify if the bug lies in https://github.com/microsoft/go-mssqldb/tree/main/azuread? I only know enough go to install and configure Prometheus plugins.

kenshaw commented 1 year ago

@reubano I don't have access to an AzureSQL instance + Active Directory to test this with, so I cannot say that I can verify that. If the simple code that was provided by @jdstone doesn't work, then yes, this is an issue with the github.com/microsoft/go-mssqldb driver.

Based on the driver's documentation, I believe the implementation in usql is correct, and a superficial reading of @jdstone's code would suggest that if this is not some other issue, there is either a) bad/missing documentation for go-mssqldb, or b) some other underlying issue with the go-mssqldb driver.

That said, I can't further this without access to AzureSQL + AD. As such, I believe the proper place for any further discussion should be on the github.com/microsoft/go-mssqldb project/repository.

I would suggest that @jdstone open an issue on that project, and share with them the same code snippet, and see if the devs on that project are able to help.

If they do, please report/share back here, and I'll either update the usql or dburl code accordingly (if, for example, the go-mssqldb package needs some kind of logic change in usql or dburl), or if it's a logic issue in the go-mssqldb itself, the change will get picked up/released after the go-mssqldb project tags a new release.

jdstone commented 1 year ago

@kenshaw

SUCCESS!!!

Shortly after you posted your response, I actually did get it working with usql. So the problem does not lie therein. The reason I had encountered those errors was because the Virtual Machine (in Azure) in which I was running the usql command from, did not have the user-assigned managed identity properly setup on the backend. Once I set that up properly, I was able to successfully run the usql command as well as my Go code.

But it was still failing inside my sql_exporter Kubernetes deployment (aka pod, aka container), which I thought was properly setup on the backside to work with the user-assigned managed identity I had created. So I created a usql Docker container and ran that inside a Kubernetes deployment with the connection to my SQL server in Azure using the fedauth=ActiveDirectoryManagedIdentity parameter and it successfully connected.

So this brings me back to my problem with sql_exporter, where I believe the problem lies: https://github.com/burningalchemist/sql_exporter/issues/169

CC @reubano

@kenshaw, TLDR: Your code has been successfully tested with an Azure SQL Server.

kenshaw commented 1 year ago

@jdstone thank you, greatly appreciate the update. @reubano you have your answer! Thanks JD

reubano commented 1 year ago

@jdstone hmmm, well that's good news at least. However, I'm using a system-assigned identity (not user-assigned). I wonder if that makes a difference.

jdstone commented 1 year ago

You're very welcome @kenshaw! I'm glad I could help out.

@reubano You may have an Azure misconfiguration. However, Azure is quite new to me and I'm still figuring things out. It took me 1-2 weeks to finally get user-assigned managed identities working correctly (Azure setup/configuration -- lots of reading and trial and error).

As to not clutter this GitHub issue with discussion that may not actually be related to a problem with usql, feel free to contact me via email and I will help you to the best of my abilities. You can find my email on my profile.

reubano commented 1 year ago

It was Azure configs. IT enabled the proper settings and my system-assigned identity worked with usql 'azuresql://xxx.database.windows.net?database=xxx&fedauth=ActiveDirectoryManagedIdentity'.

reubano commented 11 months ago

FYI, it seems that the most recent working version is usql@v0.13.12. The other versions either fail to install with the following error:

        The go.mod file for the module providing named packages contains one or
        more replace directives. It must not contain directives that would cause
        it to be interpreted differently than if it were the main module.

or fail to connect to the db with the error error: sqlserver: unable to open tcp connection with host 'localhost:1433': dial tcp 127.0.0.1:1433: connect: connection refused.

kenshaw commented 11 months ago

@reubano there are no replace directives in v0.14.12, which I was able to install, just now, using: go install github.com/xo/usql@latest -- are you saying that v0.14.x does not work with Azure?

reubano commented 11 months ago

Correct.

$ go install -tags sqlserver github.com/xo/usql@v0.14.12
$ usql 'azuresql://xxx.database.windows.net?database=xxx&fedauth=ActiveDirectoryMSI'
error: sqlserver: unable to open tcp connection with host 'localhost:1433': dial tcp 127.0.0.1:1433: connect: connection refused
$ go install -tags sqlserver github.com/xo/usql@v0.13.12
$ usql 'azuresql://xxx.database.windows.net?database=xxx&fedauth=ActiveDirectoryMSI'
Connected with driver sqlserver (Microsoft SQL Server x.x, RTM, SQL Azure)
Type "help" for help.

ms:xxx=>
kenshaw commented 11 months ago

@reubano I believe I see the issue. Can you try with just sqlserver instead of azuresql as the URL? I will be able to fix this, but would like to confirm you're able to connect using this URL: sqlserver://xxx.database.windows.net?database=xxx&fedauth=ActiveDirectoryMSI

FYI the way the underlying driver works is that it seems to only recognize the sqlserver URLs, and this is being passed directly to the driver without modification. The presence of the fedauth= parameter is what causes usql to change the driver name, but it seems to not work unless the protocol portion is changed as well. I'll fix that.

kenshaw commented 11 months ago

Nevermind. I made a mistake in a previous commit.

kenshaw commented 11 months ago

@reubano I'm deeply sorry about this. Unfortunately, since I don't use Azure I made a horrible horrible mistake, that should have never happened. A fix is coming, and I'll issue a release shortly.

reubano commented 11 months ago

No worries. Thanks!

kenshaw commented 11 months ago

OK -- although I do not have Azure SQL to test against myself, the latest commit should work:

$ ./most.sh && ./usql 'azuresql://my-cool-thing.database.windows.net?database=xxx&fedauth=ActiveDirectoryMSI'
+ CGO_ENABLED=1
+ go build '-tags=most sqlite_app_armor sqlite_fts5 sqlite_introspect sqlite_json1 sqlite_math_functions sqlite_stat4 sqlite_userauth sqlite_vtable no_adodb' '-ldflags=-s -w -X github.com/xo/usql/text.CommandName=usql -X github.com/xo/usql/text.CommandVersion=23.08.16-dev'
error: sqlserver: lookup my-cool-thing.database.windows.net: no such host
ken@ken-desktop:~/src/go/src/github.com/xo/usql$ 

Could you do a go install github.com/xo/usql@master and test this before I tag the commit and do a release? Thanks

reubano commented 11 months ago

error: sqlserver: 18456: login error: Login failed for user ''.

kenshaw commented 11 months ago

@reubano and this worked previously?

reubano commented 11 months ago

Yes. v0.13.12 works.

kenshaw commented 11 months ago

@reubano I just emailed you on the email listed on your GitHub profile. Let me know if it's possible to do a screen share quickly, as I can't debug this as I have no Azure account.

kenshaw commented 11 months ago

@reubano Never mind -- I think I found the issue.

kenshaw commented 11 months ago

@reubano I just pushed a new commit. Can you try again with the go install github.com/xo/usql@master please? Thanks.

reubano commented 11 months ago

Still no luck. Same error. If it helps, I narrowed the bad version to v0.14.0 by running the static binary.

error: sqlserver: unable to open tcp connection with host 'localhost:1433': dial tcp 127.0.0.1:1433: connect: connection refused

I wasn't able to test v0.13.13 though since it doesn't have a binary.

kenshaw commented 11 months ago

Were you able to install it from the latest commit though?

reubano commented 11 months ago

Yep. Just redid it to triple check.

kenshaw commented 11 months ago

Ok, sorry. There's a lot of complex logic paths that I haven't touched in a few years that I had to unpack.

reubano commented 11 months ago

If you make a v0.13.13 static binary I can test that and narrow it down even further. Not sure how many commits you made bw it and v0.13.12, but I doubt that many which touched mssql.

kenshaw commented 11 months ago

I pushed another commit just now. Hoping this one will magically make it work. Please try doing the go install github.com/xo/usql@master again and try that one. Thanks.

reubano commented 11 months ago

Success!!! I did go install -tags sqlserver github.com/xo/usql@5844f84 to ensure it picked up the commit. Thanks for such a quick turn-a-round!

kenshaw commented 11 months ago

Ok -- well, anyone trying to use dburl with azuresql, follow the commits I made today, August 16th, 2023.

git diff 711948df9877e2e1b27178729496ac9454e0c304..5844f84442ff98e133cae4fdd2660c65e5dd8f0d

I'll tag the release shortly.