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

Add an option to verify TCP pings #465

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bjyoungblood
Copy link

This enables user-defined captive portal detection by adding a hook
to VintageNet.Connectivity.TCPPing so that consumers can perform a
custom test against the established TCP connection (such as a TLS
handshake).

This enables user-defined captive portal detection by adding a hook
to `VintageNet.Connectivity.TCPPing` so that consumers can perform a
custom test against the established TCP connection (such as a TLS
handshake).
@@ -11,7 +11,7 @@ defmodule VintageNet.Connectivity.HostList do
@type ip_or_hostname() :: :inet.ip_address() | String.t()

@type name_port() :: {ip_or_hostname(), 1..65535}
@type ip_port() :: {:inet.ip_address(), 1..65535}
@type name_ip_port() :: {ip_or_hostname(), :inet.ip_address(), 1..65535}
Copy link
Author

Choose a reason for hiding this comment

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

This change seems kind of ugly, but it was necessary in order to preserve the original hostname for custom verify funs to use for SNI.

@bjyoungblood
Copy link
Author

bjyoungblood commented May 17, 2023

This definitely needs some more work.

Assuming that you have eth0 connected to a network that redirects the internet to a captive portal, this is the current behavior:

  1. This doesn't work on startup. The TCP stats check is probably getting in the way
  2. Manually setting eth0's connection status to :lan or :disconnected makes everything work as expected (it didn't the very first time I tried it, but that's looking like it may have been a fluke 🤷‍♂️ )
  3. Reconnecting the ethernet cable breaks everything again
  4. There's currently no way to detect if the active interface becomes bad after having been considered good (which would probably fix the above issues)

@fhunleth
Copy link
Member

I believe that the TCP stats check will miss short TCP sessions, but it seems like it could easily confound testing.

The other thing is that you really need two network interfaces active to test this. The distinction between :lan and :internet is one of interface prioritization rather than turning anything on or off. The original idea was that if you have an app that retries to connect to a server and it actually succeeds before the network is declared as :internet, then why not all it. I hadn't expected that captive portals were so sneaky. I still like these semantics, though, since if you just have one network interface, then why not try and fail. If you have two, though, then I definitely want to be more sure before switching over.

Perhaps we can brainstorm what to do and other options. I like the idea of having a better internet check. I'm also wondering if we should think about making the connection to backend servers less aggressively switch. Like perhaps not switching to the better network interface when there's a working connection unless the network interface has been good for 5 minutes (for example). This is not a fix to the general captive portal issue, but I think that it may be a fix to what we're seeing with the captive portal appearing for a short time before going away.

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

2 participants