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

Support for ipv6 addresses (with or without port specified) #536

Merged
merged 3 commits into from
Oct 12, 2022

Conversation

jshort
Copy link
Collaborator

@jshort jshort commented Oct 7, 2022

Currently et does not support passing an ipv6 address as the host positional argument is assumed to be an ipv4 address or hostname with a single colon with the port after it and the rest is ignored, eg:

$ et 2620:10d:c083:1418:14ac:6bbc:4737:56ee                                                                                                                                     15:19:52  10.07.22  100% █
Could not reach the ET server: 2620:10

This change will support ipv4 address or hostname with port specified or ipv6 address with or without port specified. Currently, if a port is specified in the positional arg, it overwrites what was specified (explicitly or by default) with the -p,--port option. I think this is backwards and may offer a PR to invert this logic.

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022

Codecov Report

Base: 71.21% // Head: 70.91% // Decreases project coverage by -0.29% ⚠️

Coverage data is based on head (e8d2636) compared to base (ccc3435).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #536      +/-   ##
==========================================
- Coverage   71.21%   70.91%   -0.30%     
==========================================
  Files          49       49              
  Lines        3071     3074       +3     
==========================================
- Hits         2187     2180       -7     
- Misses        884      894      +10     
Impacted Files Coverage Δ
src/base/ServerConnection.cpp 69.81% <0.00%> (-7.55%) ⬇️
src/base/ClientConnection.cpp 72.72% <0.00%> (-4.55%) ⬇️
src/base/ServerClientConnection.cpp 92.85% <0.00%> (-3.58%) ⬇️
src/terminal/UserJumphostHandler.cpp 61.60% <0.00%> (-3.20%) ⬇️
src/terminal/ServerFifoPath.cpp 57.89% <0.00%> (+1.37%) ⬆️
src/base/BackedReader.cpp 84.41% <0.00%> (+2.59%) ⬆️
src/base/Connection.cpp 89.56% <0.00%> (+5.21%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@MisterTea
Copy link
Owner

This PR is in great condition. Can you modify the connect_to_initial_version circleci test to use ipv6 when connecting to the initial version? Don't modify the address going the other way because version 6.0.0 won't have your commit.

@jshort
Copy link
Collaborator Author

jshort commented Oct 11, 2022

Sure! Note that this does not support ipv6 zero reduction (i.e. 0:0:0:0:0:0:0:1 -> ::1. I'll file a task to implement that.

.circleci/config.yml Outdated Show resolved Hide resolved
@jshort jshort force-pushed the master branch 3 times, most recently from 26c9b06 to e77190d Compare October 11, 2022 18:17
@jshort
Copy link
Collaborator Author

jshort commented Oct 11, 2022

Pushed a temp debug config for circle ci to see if ListenAddress :: is in the sshd config(s).

EDIT:

Need to add inet6 to this:

AddressFamily inet\

EDIT2: That didn't work. Uncommented both ListenAddress 0.0.0.0 and ListenAddress :: to no avail:

Output of ss

Starting ssh (via systemctl): ssh.service.
Netid State  Recv-Q Send-Q       Local Address:Port   Peer Address:Port Process 
icmp6 UNCONN 0      0                   *%ens5:58                *:*            
udp   UNCONN 0      0            127.0.0.53%lo:53          0.0.0.0:*            
udp   UNCONN 0      0        172.28.15.34%ens5:68          0.0.0.0:*            
tcp   LISTEN 0      4096         127.0.0.53%lo:53          0.0.0.0:*            
tcp   LISTEN 0      128                0.0.0.0:22          0.0.0.0:*          

Looks like ssh isn't running for ipv6.

@jshort jshort force-pushed the master branch 17 times, most recently from 69ee277 to 160fec9 Compare October 11, 2022 20:57
@jshort
Copy link
Collaborator Author

jshort commented Oct 11, 2022

@MisterTea

Ok I got it working. Sorry for those 16 force pushes! I was running every tool I could think of to see why sshd was rejecting the ipv6 connection: netstat (gone in this version of Ubuntu), ss, ufw, ip(6)tables, lsof, ip addr, etc. Turns out that sshd was running by default so the /etc/init.d/ssh start was a no-op so I had to change to restart to pick up my updated sshd_config 😭

Currently et does not support passing an ipv6 address as the host
positional argument is assumed to be an ipv4 address or hostname with a
single colon with the port after it and the rest is ignored, eg:

```
$ et 2620:10d:c083:1418:14ac:6bbc:4737:56ee                                                                                                                                     15:19:52  10.07.22  100% █
Could not reach the ET server: 2620:10
```

This change will support ipv4 address or hostname with port specified or
ipv6 address with or without port specified.  Currently, if a port is
specified in the positional arg, it overwrites what was specified
(explicitly or by default) with the -p,--port option. I think this is
backwards and may offer a PR to invert this logic.
No-op changes:
* TerminalMain: change binary name/description
* TerminalMain: get rid of Daemon log file (does nothing now)
* SubprocessToString: reorganize forking logic and remove irrelevant
  comments and cleanup trailing whitespace
* PsuedoUserTerminal: breaks in forking switch statement
The prefix option to the terminal client is misleading as it isn't a
prefix (or search path) and is treated as an absolute path to the
etterminal executable.  This renames it as such and removes the
superfluous positional arg in the command string building for the ssh
command (i.e. /usr/local/bin/etterminal etterminal -v 0)
@MisterTea
Copy link
Owner

Great! Thanks so much for this high quality PR.

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.

None yet

3 participants