CSHARP-734: SOCKS5 Proxy Support #1731
Conversation
| } | ||
|
|
||
| _proxyHost = value; | ||
| if (_proxyHost.Length == 0) |
There was a problem hiding this comment.
string.IsNullOrEmpty(_proxyHost)?
There was a problem hiding this comment.
If we reached here, we already validated the options with a regex, so the strings cannot be null.
They could be empty though if the connection string looks something like (with empty spaces):
....proxyHost= &proxyPort=2020.
So ideally we should only check that is not empty. We could also still use isNullOrEmpty for readability, but it's not necessary here.
This made me also realise that we should just check for null a couple of lines earlier, so I removed IsNullOrEmpty and added a test that verifies it.
There was a problem hiding this comment.
Makes sense, I would probably prefer comparing to String.Empty then, but I'm not insisting on this.
There was a problem hiding this comment.
And I would prefer if (_proxy == "") LOL
| } | ||
|
|
||
| var proxyPortValue = ParseInt32(name, value); | ||
| if (proxyPortValue is < 0 or > 65535) |
There was a problem hiding this comment.
It's a valid port number, but usually the port numbers less than 1024 are reserved. I tried to be more relaxed here with the validation, just excluding values that are definitely not valid.
There was a problem hiding this comment.
0 is probably wrong value for the port, as it means "random available port" as far as I know.
There was a problem hiding this comment.
I think you're right :)
| @@ -374,6 +374,10 @@ public void When_nothing_is_specified(string connectionString) | |||
| subject.MaxPoolSize.Should().Be(null); | |||
| subject.MinPoolSize.Should().Be(null); | |||
| subject.Password.Should().BeNull(); | |||
| subject.ProxyHost.Should().Be(null); | |||
There was a problem hiding this comment.
So... BeNull can't be used with proxyPort because of the type constraints on the fluent assertions, but I'll change the others.
src/MongoDB.Driver/Core/Connections/Socks5AuthenticationSettings.cs
Outdated
Show resolved
Hide resolved
| return new Hasher() | ||
| .Hash(Username) | ||
| .Hash(Password) | ||
| .GetHashCode(); |
There was a problem hiding this comment.
This class looks immutable, we can calculate the hashcode once.
There was a problem hiding this comment.
I agree we could, but I suppose this method is method is going be called almost never. Do you think it's worth to cache it vs keeping the code shorter?
There was a problem hiding this comment.
If this class is not used as a key (or part of the key) in some dictionaries, then probably it's OK to keep the code as is.
There was a problem hiding this comment.
Yes, it should not be used in that way.
| @@ -48,19 +48,36 @@ public TcpStreamFactory(TcpStreamSettings settings) | |||
| // methods | |||
| public Stream CreateStream(EndPoint endPoint, CancellationToken cancellationToken) | |||
| { | |||
| var socks5ProxySettings = _settings.Socks5ProxySettings; | |||
| var useProxy = socks5ProxySettings != null; | |||
| var targetEndpoint = useProxy ? new DnsEndPoint(socks5ProxySettings.Host, socks5ProxySettings.Port) : endPoint; | |||
There was a problem hiding this comment.
Am I right to think that proxy vs non-proxy scenario is different in 2 points:
- What endpoint to connect
- Do some additional logic after connecting the socket
If so - can we instead of tweaking the TcpStreamFactory class, create a new wrapper that will replace the endpoint, call the TcpStreamFactory.CreateStream as usual, and then do proxy negotiation after.
We are doing something similar in SslStreamFactory.
evergreen/evergreen.yml
Outdated
| TARGET="TestSocks5Proxy" \ | ||
| evergreen/run-tests.sh | ||
| OS=${OS} \ | ||
| evergreen/cleanup-proxy.sh |
There was a problem hiding this comment.
You probably should add this script to cleanup-test-resources.sh or call the cleanup-proxy.sh from post steps. Otherwise we could skip on cleaning proxy if some previous step failed with an error.
evergreen/cleanup-proxy.sh
Outdated
|
|
||
| echo "Attempt to kill proxy server process if present on ${OS}" | ||
| if [[ "$OS" =~ Windows|windows ]]; then | ||
| tasklist -FI "IMAGENAME eq python.exe" |
There was a problem hiding this comment.
Killing all Python processes might be too much. Let's investigate if there is anything else we can do.
There was a problem hiding this comment.
Yup, I've changed it to save the PID of the server processes, and kill them afterwards. The killing almost certainly has been already done by the evergreen process cleaning, but I left the cleanup script just in case, as they have suggested.
| var proxyPortValue = ParseInt32(name, value); | ||
| if (proxyPortValue is < 1 or > 65535) | ||
| { | ||
| throw new MongoConfigurationException("proxyPort must be between 0 and 65535."); |
There was a problem hiding this comment.
I would probably suggest to change the error message to:
Invalid proxy port: {proxyPortValue}: must be between 1 and 65535, inclusive
| } | ||
|
|
||
| _proxyHost = value; | ||
| if (_proxyHost.Length == 0) |
There was a problem hiding this comment.
Makes sense, I would probably prefer comparing to String.Empty then, but I'm not insisting on this.
| } | ||
|
|
||
| var proxyPortValue = ParseInt32(name, value); | ||
| if (proxyPortValue is < 0 or > 65535) |
There was a problem hiding this comment.
0 is probably wrong value for the port, as it means "random available port" as far as I know.
| sb.Append(Authentication switch | ||
| { | ||
| Socks5AuthenticationSettings.UsernamePasswordAuthenticationSettings up => | ||
| $"UsernamePassword (Username: {up.Username}, Password: {up.Password})", |
There was a problem hiding this comment.
MongoUrlBuilder.ToString is being used to build the Url from provided parameters, so having passwords there is probably the expected behavior (I suppose we ought to have another method for that, something like BuildUrl). However this class is user-facing settings class, there is a bigger chances it could be converted to string and logged or even worse outputted as a part of exception. I've checked MongoClientSettings - it does not look like it can leak passwords in the similar way.
| get => _proxyPort; | ||
| set | ||
| { | ||
| if (value is < 0 or > 65535) |
There was a problem hiding this comment.
We have Ensure.IsNullOrBetween if this is what you are looking for.
| return new Hasher() | ||
| .Hash(Username) | ||
| .Hash(Password) | ||
| .GetHashCode(); |
There was a problem hiding this comment.
If this class is not used as a key (or part of the key) in some dictionaries, then probably it's OK to keep the code as is.
src/MongoDB.Driver/Core/Connections/Socks5ProxySettingsBuilder.cs
Outdated
Show resolved
Hide resolved
| { | ||
| if (version != ProtocolVersion5) | ||
| { | ||
| throw new IOException("Invalid SOCKS version in response."); |
There was a problem hiding this comment.
Add actual and expected versions?
| await stream.ReadBytesAsync(buffer, 0, 2, cancellationToken).ConfigureAwait(false); | ||
| var acceptsUsernamePasswordAuth = ProcessGreetingResponse(buffer, useAuth); | ||
|
|
||
| // If we have username and password, but the proxy doesn't need them, we skip. |
There was a problem hiding this comment.
minor: Should comment say "we skip the authentication step" ?
There was a problem hiding this comment.
Need to update the comment to match the sync version.
tests/MongoDB.Driver.Tests/Specifications/socks5-support/Socks5SupportProseTests.cs
Outdated
Show resolved
Hide resolved
| AddressTypeIPv4 => 5, | ||
| AddressTypeIPv6 => 17, | ||
| AddressTypeDomain => buffer[4] + 2, | ||
| _ => throw new IOException("Unknown address type in SOCKS5 reply.") |
There was a problem hiding this comment.
It's worth adding the actual value, here and in other exceptions where applicable.
| await stream.ReadBytesAsync(buffer, 0, 2, cancellationToken).ConfigureAwait(false); | ||
| var acceptsUsernamePasswordAuth = ProcessGreetingResponse(buffer, useAuth); | ||
|
|
||
| // If we have username and password, but the proxy doesn't need them, we skip. |
There was a problem hiding this comment.
Need to update the comment to match the sync version.
tests/MongoDB.Driver.Tests/Specifications/socks5-support/Socks5SupportProseTests.cs
Show resolved
Hide resolved
src/MongoDB.Driver/Core/Configuration/Socks5ProxyStreamSettings.cs
Outdated
Show resolved
Hide resolved
src/MongoDB.Driver/Core/Configuration/Socks5ProxyStreamSettings.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| [Fact] | ||
| public void Socks5AuthenticationSettings_UsernamePassword_should_return_UsernamePasswordAuthenticationSettings_instance_with_correct_values() |
There was a problem hiding this comment.
Should this be in Socks5AuthenticationSettingsTests?
No description provided.