-
Notifications
You must be signed in to change notification settings - Fork 51
Add support for specifying IP version #332
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
base: main
Are you sure you want to change the base?
Conversation
|
Shall I submit the connectbot change that depends on this right away or wait until this change is accepted and integrated? I don't want to submit a change to connectbot that will cause the builds to fail because and updated version of sshlib is not yet available... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for restricting SSH connections to specific IP versions (IPv4-only, IPv6-only, or both) by introducing an IpVersion enum and new connect() method signatures.
Key Changes:
- Added IpVersion enum to Connection and TransportManager classes with three options: IPV4_AND_IPV6 (default), IPV4_ONLY, and IPV6_ONLY
- Extended connect() methods in Connection class with overloads accepting an ipVersion parameter
- Implemented IP address filtering logic in TransportManager to enforce the IP version restriction
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/main/java/com/trilead/ssh2/Connection.java | Added IpVersion enum and four new connect() method overloads with ipVersion parameter; updated existing methods to delegate with default IPV4_AND_IPV6 value; added enum mapping logic to convert Connection.IpVersion to TransportManager.IpVersion |
| src/main/java/com/trilead/ssh2/transport/TransportManager.java | Added IpVersion enum and helper methods getIPv4Address/getIPv6Address to filter addresses; modified connectDirect() and establishConnection() to accept and use ipVersion parameter; updated initialize() signature to include ipVersion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public synchronized ConnectionInfo connect(IpVersion ipVersion) throws IOException | ||
| { | ||
| return connect(null, 0, 0, ipVersion); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Same as | ||
| * {@link #connect(ServerHostKeyVerifier, int, int) connect(verifier, 0, 0)}. | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(verifier, 0, 0, IpVersion.IPV4_AND_IPV6)}. | ||
| * | ||
| * @return see comments for the | ||
| * {@link #connect(ServerHostKeyVerifier, int, int) connect(ServerHostKeyVerifier, int, int)} | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(ServerHostKeyVerifier, int, int, IpVersion)} | ||
| * method. | ||
| * @throws IOException | ||
| */ | ||
| public synchronized ConnectionInfo connect(ServerHostKeyVerifier verifier) throws IOException | ||
| { | ||
| return connect(verifier, 0, 0); | ||
| return connect(verifier, 0, 0, IpVersion.IPV4_AND_IPV6); | ||
| } | ||
|
|
||
| /** | ||
| * Same as | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(verifier, 0, 0, ipVersion)}. | ||
| * | ||
| * @return see comments for the | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(ServerHostKeyVerifier, int, int, IpVersion)} | ||
| * method. | ||
| * @throws IOException | ||
| */ | ||
| public synchronized ConnectionInfo connect(ServerHostKeyVerifier verifier, IpVersion ipVersion) throws IOException | ||
| { | ||
| return connect(verifier, 0, 0, ipVersion); | ||
| } | ||
|
|
||
| /** | ||
| * Same as | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(verifier, connectTimeout, kexTimeout, IpVersion.IPV4_AND_IPV6)}. | ||
| * | ||
| * @return see comments for the | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(ServerHostKeyVerifier, int, int, IpVersion)} | ||
| * method. | ||
| * @throws IOException | ||
| */ | ||
| public synchronized ConnectionInfo connect(ServerHostKeyVerifier verifier, int connectTimeout, int kexTimeout) | ||
| throws IOException | ||
| { | ||
| return connect(verifier, connectTimeout, kexTimeout, IpVersion.IPV4_AND_IPV6); | ||
| } |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing API documentation: The IpVersion parameter is not documented in overload methods. The new connect overloads at lines 598, 627, and 641 don't document the ipVersion parameter even though they accept it. Only the main connect method at line 734 has documentation for this parameter. For consistency and API clarity, all public methods that accept the ipVersion parameter should document it.
| * @param ipVersion | ||
| * Specify whether the connection should be restricted to one of | ||
| * IPv4 or IPv6, with a default of allowing both. See | ||
| * {@link IpVersion}. |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing feature documentation: The IP version restriction feature is not documented in the main method Javadoc. The parameter is documented, but there's no mention of limitations (e.g., that ipVersion is ignored when using a proxy connection). Consider adding a note in the documentation explaining when ipVersion applies and when it doesn't.
| * {@link IpVersion}. | |
| * {@link IpVersion}. <b>Note:</b> This parameter is ignored if a proxy connection is used | |
| * (i.e., if {@link #setProxyData(ProxyData)} has been called); in that case, the proxy | |
| * determines the IP version used for the connection. |
| else // Assume (ipVersion == IpVersion.IPV4_AND_IPV6), the default. | ||
| { | ||
| addr = InetAddress.getByName(hostname); | ||
| } |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null pointer exception risk: When ipVersion is IPV4_ONLY or IPV6_ONLY, getIPv4Address or getIPv6Address may return null if the hostname doesn't resolve to that IP version. The null address will cause a NullPointerException when passed to InetSocketAddress constructor. Add null check and throw an IOException with a clear error message explaining that the hostname doesn't resolve to the requested IP version.
| } | |
| } | |
| if (addr == null) { | |
| throw new IOException("The hostname '" + hostname + "' does not resolve to the requested IP version: " + ipVersion); | |
| } |
| IPV4_AND_IPV6, ///< Allow both IPV4 and IPv6, the default. | ||
| IPV4_ONLY, ///< Require that the connection be over IPv4 only. | ||
| IPV6_ONLY ///< Require that the connection be over IPv6 only. |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C-style comments are not idiomatic in Java. The codebase uses Javadoc-style comments (/** ... */) for documentation. These inline comments after enum values should either be removed or converted to proper Javadoc comments above each enum value, following the pattern used elsewhere in the codebase for documenting class members.
| IPV4_AND_IPV6, ///< Allow both IPV4 and IPv6, the default. | |
| IPV4_ONLY, ///< Require that the connection be over IPv4 only. | |
| IPV6_ONLY ///< Require that the connection be over IPv6 only. | |
| /** | |
| * Allow both IPV4 and IPv6, the default. | |
| */ | |
| IPV4_AND_IPV6, | |
| /** | |
| * Require that the connection be over IPv4 only. | |
| */ | |
| IPV4_ONLY, | |
| /** | |
| * Require that the connection be over IPv6 only. | |
| */ | |
| IPV6_ONLY |
| TransportManager.IpVersion tmIpVersion; | ||
| if (ipVersion == IpVersion.IPV4_ONLY) | ||
| { | ||
| tmIpVersion = TransportManager.IpVersion.IPV4_ONLY; | ||
| } | ||
| else if (ipVersion == IpVersion.IPV6_ONLY) { | ||
| tmIpVersion = TransportManager.IpVersion.IPV6_ONLY; | ||
| } | ||
| else // Assume (ipVersion == IpVersion.IPV4_AND_IPV6), the default. | ||
| { | ||
| tmIpVersion = TransportManager.IpVersion.IPV4_AND_IPV6; | ||
| } |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate enum definition: The IpVersion enum is defined identically in both Connection and TransportManager classes. This violates the DRY principle and creates a maintenance burden. Consider defining the enum once in a shared location (e.g., in Connection as the public API) and having TransportManager reference it directly, or create a shared constants class. The current approach requires maintaining two identical enums and manual conversion code (lines 807-818).
| token = TimeoutService.addTimeoutHandler(timeoutHorizont, timeoutHandler); | ||
| } | ||
|
|
||
| TransportManager.IpVersion tmIpVersion; |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing input validation: The ipVersion parameter is not validated for null. If a caller passes null for ipVersion, the code will fail with a NullPointerException at line 808. Consider adding a null check and either throwing an IllegalArgumentException with a clear message or defaulting to IPV4_AND_IPV6.
| TransportManager.IpVersion tmIpVersion; | |
| TransportManager.IpVersion tmIpVersion; | |
| // Default to IPV4_AND_IPV6 if ipVersion is null | |
| if (ipVersion == null) { | |
| ipVersion = IpVersion.IPV4_AND_IPV6; | |
| } |
| private static Socket connectDirect(String hostname, int port, int connectTimeout) | ||
| private static Socket connectDirect(String hostname, int port, int connectTimeout, IpVersion ipVersion) | ||
| throws IOException | ||
| { |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing input validation: The ipVersion parameter is not validated for null. If a caller passes null for ipVersion, the code will fail with a NullPointerException at line 309. Consider adding a null check and either throwing an IllegalArgumentException with a clear message or defaulting to IPV4_AND_IPV6.
| { | |
| { | |
| if (ipVersion == null) { | |
| ipVersion = IpVersion.IPV4_AND_IPV6; | |
| } |
| public synchronized ConnectionInfo connect(IpVersion ipVersion) throws IOException | ||
| { | ||
| return connect(null, 0, 0, ipVersion); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Same as | ||
| * {@link #connect(ServerHostKeyVerifier, int, int) connect(verifier, 0, 0)}. | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(verifier, 0, 0, IpVersion.IPV4_AND_IPV6)}. | ||
| * | ||
| * @return see comments for the | ||
| * {@link #connect(ServerHostKeyVerifier, int, int) connect(ServerHostKeyVerifier, int, int)} | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(ServerHostKeyVerifier, int, int, IpVersion)} | ||
| * method. | ||
| * @throws IOException | ||
| */ | ||
| public synchronized ConnectionInfo connect(ServerHostKeyVerifier verifier) throws IOException | ||
| { | ||
| return connect(verifier, 0, 0); | ||
| return connect(verifier, 0, 0, IpVersion.IPV4_AND_IPV6); | ||
| } | ||
|
|
||
| /** | ||
| * Same as | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(verifier, 0, 0, ipVersion)}. | ||
| * | ||
| * @return see comments for the | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(ServerHostKeyVerifier, int, int, IpVersion)} | ||
| * method. | ||
| * @throws IOException | ||
| */ | ||
| public synchronized ConnectionInfo connect(ServerHostKeyVerifier verifier, IpVersion ipVersion) throws IOException | ||
| { | ||
| return connect(verifier, 0, 0, ipVersion); | ||
| } | ||
|
|
||
| /** | ||
| * Same as | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(verifier, connectTimeout, kexTimeout, IpVersion.IPV4_AND_IPV6)}. | ||
| * | ||
| * @return see comments for the | ||
| * {@link #connect(ServerHostKeyVerifier, int, int, IpVersion) connect(ServerHostKeyVerifier, int, int, IpVersion)} | ||
| * method. | ||
| * @throws IOException | ||
| */ | ||
| public synchronized ConnectionInfo connect(ServerHostKeyVerifier verifier, int connectTimeout, int kexTimeout) | ||
| throws IOException | ||
| { | ||
| return connect(verifier, connectTimeout, kexTimeout, IpVersion.IPV4_AND_IPV6); | ||
| } |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage: The new IpVersion feature and all the new connect() method overloads lack test coverage. The repository has comprehensive integration tests (OpenSSHCompatibilityTest, DropbearCompatibilityTest) that test connection functionality, but none test the IPv4-only, IPv6-only, or default behavior of the new ipVersion parameter. Consider adding tests to verify: 1) connections work correctly with each IpVersion value, 2) appropriate errors are thrown when a hostname doesn't resolve to the requested IP version, and 3) backwards compatibility with existing code that doesn't specify ipVersion.
| IPV4_AND_IPV6, ///< Allow both IPV4 and IPv6, the default. | ||
| IPV4_ONLY, ///< Require that the connection be over IPv4 only. | ||
| IPV6_ONLY ///< Require that the connection be over IPv6 only. |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C-style comments are not idiomatic in Java. The codebase uses Javadoc-style comments (/** ... */) for documentation. These inline comments after enum values should either be removed or converted to proper Javadoc comments above each enum value, following the pattern used elsewhere in the codebase for documenting class members.
| IPV4_AND_IPV6, ///< Allow both IPV4 and IPv6, the default. | |
| IPV4_ONLY, ///< Require that the connection be over IPv4 only. | |
| IPV6_ONLY ///< Require that the connection be over IPv6 only. | |
| /** | |
| * Allow both IPV4 and IPv6, the default. | |
| */ | |
| IPV4_AND_IPV6, | |
| /** | |
| * Require that the connection be over IPv4 only. | |
| */ | |
| IPV4_ONLY, | |
| /** | |
| * Require that the connection be over IPv6 only. | |
| */ | |
| IPV6_ONLY |
Added new signatures to the connect method of the Connection class to allow the caller to restrict the IP version of the connection to be established. The new signatures duplicate existing signatures and add one parameter. For backwards compatibility, the default for the new parameter is IPv4 and IPv6, while the new values are "IPv4 only" and "IPv6 only".