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

Allow for local dns resolution with a custom dialer #1

Merged
merged 8 commits into from
Mar 30, 2023

Conversation

stephaniehingtgen
Copy link

@stephaniehingtgen stephaniehingtgen commented Mar 23, 2023

Overview
If the custom dialer updates the transport to proxy the mssql connection, and only the proxy can reach the network that mssql is running in, then any local DNS resolution will fail.

To address this, this PR adds an attempt to dial the connection via the custom dialer, if the DNS resolution fails.

Background
While implementing the secure socks proxy in Grafana for MSSQL (where we allow datasources to go through a socks5 proxy with TLS to allow users to connect to datasources that live in a different network than where Grafana is running), we noticed that it would only work with connection strings that used ip addresses, not domain names.

We found this is because the dialConnection function tries to resolve DNS without using the custom dialer (so the connection is not sent through through the proxy). Since Grafana cannot reach the network without the proxy and it's a local domain name, it fails to connect.

@CLAassistant
Copy link

CLAassistant commented Mar 23, 2023

CLA assistant check
All committers have signed the CLA.

@stephaniehingtgen
Copy link
Author

stephaniehingtgen commented Mar 23, 2023

Added @oscarkilhed and @mdvictor, since you both were listed as the code owners for the mssql code base in grafana, but let me know if someone else should take a look :)

@stephaniehingtgen stephaniehingtgen changed the title Do not resolve DNS when a custom dialer is provided Allow for local dns resolution with a custom dialer Mar 23, 2023
tds.go Show resolved Hide resolved
@mdvictor
Copy link

This looks good!

Just to mention: I recall there were discussions about deprecating this fork for the Microsoft maintained one in the future. This might be beneficial for a number of reasons. Not sure if the that one has this feature implemented though but might be worth a shot checking.

I'm also curious if there is any way we could test this easily.

@stephaniehingtgen
Copy link
Author

Thanks for taking a look at it!

Just to mention: I recall there were discussions about deprecating this fork for the Microsoft maintained one in the future. This might be beneficial for a number of reasons. Not sure if the that one has this feature implemented though but might be worth a shot checking.

Good to know! Thank you for bringing that up! It looks like microsoft uses the dialer right away: https://github.com/microsoft/go-mssqldb/blob/main/tds.go#L903, so we should be covered :)

I'm also curious if there is any way we could test this easily.

I think there may be something messed up with the fork currently. When I try to replace it in the go mod, it fails. It looks like we are running the fork before these commits were added: 80c34ad, eef9f2a. So I think it may be messed up there. I'll take a further look after merging this in (am I good to do so?). I was going to try to cut a release of the fork to see if that helped when updating the go mod in Grafana. In the meantime to test, what I did was overwrite the dependency directly in go/pkg/mod files 😅

@mdvictor
Copy link

Having the functionality already in the one that microsoft uses is great. Might be worth restarting the discussions about deprecating this one faster. I'll tag @oscarkilhed since I think he has more knowledge around this.

I'd say we are good to go with this fix in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants