Fix socks5 proxy support

[sheldonth]

Took two small changes to get the socks5 proxy support working. Opened here for consideration.

[sheldonth]

See: https://github.com/seanmonstar/reqwest/blob/6a41459862fa9db2a903542afcc06460cf39c277/src/proxy.rs#L322

DNS support over SOCKS is needed to resolve the remote host.

[yeastplume]

Thanks for submitting this!

I'm not an expert on socks proxies, but this is what I've found with relation to socks urls:

socks5h:// and socks4a:// mean that the hostname isresolved by the SOCKS server.
socks5:// and socks4:// mean that the hostname is resolved locally.
socks4a:// means to use SOCKS4a

Does this string need to be configurable to avoid breaking existing configurations?

[sheldonth]

I can't think of a reason why you would want to explicitly disallow DNS queries running over the proxy but security configurations are complex and it's certainly not possible to guarantee a reason doesn't exist.

What I can say for certain is that I was unable to use the reqwest proxy client to transact with a Slatepack address until I adjusted the configuration as it is presented here. I got client callback errors that the http stack was unable to perform the DNS resolution for the .onion domain.

[yeastplume]

No, we definitely don't need to disallow DNS queries, all I'm trying to determine is whether making this change could potentially break anyone's configuration. So I guess the question is if we change the scheme to socks5h, could it cause issues for existing installations when they upgrade.

From what I gather socks resolves domains locally, and socks5h lets DNS resolution be done on the remote server; does that mean that local DNS resolution won't happen locally at all if using socks5h, or does it fall back to local DNS resolution?

Just trying to determine whether the socks scheme should be in the configuration file instead of hardcoded here.

[sheldonth]

does that mean that local DNS resolution won't happen locally at all

When you say "local DNS resolution" do you mean "checking the host /etc/hosts" or "asking the host operating system for it's current preferred DNS server and sending it a lookup" or both?

[yeastplume]

Either or both... I'm just asking 'does this change break existing installs in any way' so we can determine whether it can be merged as is or whether it needs to be added as a config file change.

[sheldonth]

'does this change break existing installs in any way'

It certainly could if you were relying on a certain DNS behavior in your system.

That said, DNS support over the proxy seems pretty crucial for the intended use case here, so I would vote to make h protocol specification the default.

Sorry I can't be of more help in this decision.

[yeastplume]

Agree with the need for this. So I think we should turn this into an optional config file parameter, and default it to socksh. This will enable proxy support by default and at least allow anyone relying on the the socks scheme to specify it on upgrade to the next version.

[yeastplume]

Merging this as a prerequisite for #617, (which will handle config file changes, will test everything within that PR)