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

Introduce SOCKS5 proxy support #1180

Merged
merged 27 commits into from
Sep 8, 2023
Merged

Introduce SOCKS5 proxy support #1180

merged 27 commits into from
Sep 8, 2023

Conversation

vbabanin
Copy link
Member

@vbabanin vbabanin commented Aug 16, 2023

Summary:

The MongoDB community has expressed the need for SOCKS5 proxy support. This PR introduces support for SOCKS5 proxy configuration for the synchronous version of the driver.

Changes:

Added support for configuring SOCKS5 proxy settings in MongoClientSettings, AutoEncryptionSettings and ClientEncryptionSettings.
Added SocksSocket which implements RFC19128 and RFC1929

Impact:

This feature enhances the driver's versatility and expands its usability in diverse networking scenarios. With this addition, users facing network restrictions, such as those accessing MongoDB deployments behind firewalls, can now seamlessly connect and interact with their databases.

Related links:

Specification
JAVA-4347

@vbabanin vbabanin self-assigned this Aug 16, 2023
@@ -51,6 +58,27 @@ public static void enableSni(final String host, final SSLParameters sslParameter
}
}

public static void configureSslSocket(final Socket socket, final SslSettings sslSettings, final InetSocketAddress inetSocketAddress) throws
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code of this method has been relocated without any alterations from its original location in SocketStreamHelper.java#L9

@vbabanin vbabanin changed the title Introduce SOCKS5 proxy support, allowing seamless connections to Mong… Introduce SOCKS5 proxy support Aug 16, 2023
@vbabanin vbabanin marked this pull request as ready for review August 17, 2023 01:35
@vbabanin vbabanin requested review from stIncMale and jyemin August 17, 2023 01:36
Copy link
Member

@stIncMale stIncMale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static checks fail. This can be addressed before the whole PR is reviewed.

@vbabanin vbabanin requested a review from stIncMale August 17, 2023 21:15
@jyemin
Copy link
Collaborator

jyemin commented Aug 17, 2023

All checks are now passing now. @vbabanin can you manually reconfigure to run the new socks tasks?

@vbabanin
Copy link
Member Author

All checks are now passing now. @vbabanin can you manually reconfigure to run the new socks tasks?

Done, I have added test-socks5 task with two build variants included.

Copy link
Member

@stIncMale stIncMale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not yet reviewed changes in the following files:

  • .evergreen/.evg.yml
  • .evergreen/run-socks5-tests.sh
  • driver-core/src/main/com/mongodb/internal/connection/SocketStream.java
  • driver-sync/src/test/functional/com/mongodb/client/Socks5ProseTest.java

Comment on lines 111 to 119
if (socket != null && !socket.isConnected()) {
socket.connect(proxyAddress, remainingMillis(timeout));
inputStream = socket.getInputStream();
outputStream = socket.getOutputStream();
} else {
super.connect(proxyAddress, remainingMillis(timeout));
inputStream = getInputStream();
outputStream = getOutputStream();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we call Socket.connect from SocketStream.initializeSocket for a scenario without a proxy, we try in a loop all InetSocketAddresses to which ServerAddress is resolved.

When we call socket.connect/super.connect here while connecting to a proxy, we don't do the same. Instead, we pick a single InetSocketAddress to which ProxySettings.getHost&getPort may be resolved, and do only one attempt.

It seems to me that we should try all IP addresses corresponding to the proxy host when connecting to a proxy, similarly to what we do when we connect to MongoDB directly.

Copy link
Member Author

@vbabanin vbabanin Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is been decided to handle this in a separate DRIVERS ticket because it requires spec changes. I am going to create one.

UPD: DRIVERS-2719

@stIncMale
Copy link
Member

I have now reviewed all the changes in the PR.

@vbabanin vbabanin requested a review from stIncMale August 31, 2023 06:21
Copy link
Collaborator

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review, but I found some things that may need addressing.

.evergreen/.evg.yml Outdated Show resolved Hide resolved
.evergreen/.evg.yml Outdated Show resolved Hide resolved
.evergreen/.evg.yml Outdated Show resolved Hide resolved
.evergreen/run-socks5-tests.sh Outdated Show resolved Hide resolved
driver-core/src/main/com/mongodb/ConnectionString.java Outdated Show resolved Hide resolved
- Eliminate KMIP server configuration from SOCKS5 tests.
- Organize SOCKS5 proxy tests into separate tasks for authenticated and non-authenticated scenarios.
- Enhance unit test coverage.

JAVA-4347
@vbabanin vbabanin requested a review from stIncMale September 5, 2023 19:52
@vbabanin vbabanin requested a review from jyemin September 5, 2023 21:27
Copy link
Collaborator

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for all my change requests.

matrix_spec: { os: "linux", ssl: ["nossl", "ssl"], version: [ "latest" ], topology: ["replicaset"] }
display_name: "Socks5: ${version} ${topology} ${ssl} ${jdk} ${os}"
matrix_spec: { os: "linux", ssl: ["nossl", "ssl"], version: [ "latest" ], topology: ["replicaset"], socks_auth: ["auth", "noauth"] }
display_name: "${socks_auth} SOCKS5 proxy: ${version} ${topology} ${ssl} ${jdk} ${os}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move socks_auth after SOCKS5 proxy so that they sort together in the display

@vbabanin vbabanin requested a review from stIncMale September 6, 2023 21:44
@vbabanin vbabanin merged commit d3008cc into mongodb:master Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants