feat(cryptostorm): add WireGuard and OpenVPN provider support - #3164
mpatton125 wants to merge 5 commits into
Conversation
c911ea8 to
7a9856f
Compare
qdm12
left a comment
There was a problem hiding this comment.
- Big hallucination on the updating servers code. this needs to be fixed somehow, or this PR won't be accepted
- Port forwarding: I need a human to check the information returned from the port forwarding endpoint when connected
- When above is done, I need a human to verify openvpn tcp, udp, wireguard and port forwarding all work fine.
| // their publicly available node list. If the upstream format changes, | ||
| // update the nodeData struct and parsing logic accordingly. | ||
| func fetchAPI(ctx context.Context, client *http.Client) (data []nodeData, err error) { | ||
| const url = "https://cryptostorm.is/wireguard/nodes.json" |
There was a problem hiding this comment.
there is no json API, this url is the same as https://cryptostorm.is/wireguard. This returns html, not json and this will fail
There was a problem hiding this comment.
Now scraping endpoint data from https://cryptostorm.is/wireguard.
| func (p *Provider) GetConnection(selection settings.ServerSelection, ipv6Supported bool) ( | ||
| connection models.Connection, err error, | ||
| ) { | ||
| defaults := utils.NewConnectionDefaults(443, 443, 443) //nolint:mnd |
There was a problem hiding this comment.
please double check openvpn udp tcp and wireguard all work with those 443 ports.
There was a problem hiding this comment.
Definitely both work with 443 - provider says they accept OpenVPN and Wireguard connections on any port (1-65535).
There was a problem hiding this comment.
Really ? Can they accept both openvpn udp and wireguard (udp) on the same port ?? That seems odd 🤔 Or do you pick the port on their web ui for example ?
| } | ||
| ] | ||
| }, | ||
| "cryptostorm": { |
There was a problem hiding this comment.
that was not obtained from the code in the updater, so it most likely pure hallucination; fix the updater and revert these changes.
| // If the port is already forwarded (e.g. from a previous session) it will appear in | ||
| // the list regardless of whether the POST succeeded, so we treat that as success. | ||
| // Valid port range is 30000–65535. | ||
| // See: https://cryptostorm.is/port_forwarding |
There was a problem hiding this comment.
404 not found, possible hallucination? please fix
| postBody = "port=" + strconv.FormatUint(uint64(objects.ListeningPort), 10) | ||
| } | ||
|
|
||
| pfURL := "http://" + cryptostormPFServer + "/fwd" |
There was a problem hiding this comment.
add a comment for IPv6 it's http://[2001:db8::7]/fwd - that will be used later.
| postBody = "port=" + strconv.FormatUint(uint64(objects.ListeningPort), 10) | ||
| } | ||
|
|
||
| pfURL := "http://" + cryptostormPFServer + "/fwd" |
There was a problem hiding this comment.
@mpatton125 can you check what http://10.31.33.7/fwd returns when you are connected to the VPN? To confirm what the AI wrote is correct
|
|
||
| const base, bitSize = 10, 16 | ||
| for _, match := range matches { | ||
| portUint64, err := strconv.ParseUint(match[1], base, bitSize) |
There was a problem hiding this comment.
Yes this provider allows multiple port forwards, per endpoint.
There was a problem hiding this comment.
Is there a maximum limit of ports forwarded??
Let's wait for #3208 to be merged, then this can use the new setting field PortsCount
There was a problem hiding this comment.
rebase on upstream master, which supports multiple ports. What's the number of ports limit?
| // ListeningPort is the port to request from the provider, used by Cryptostorm. | ||
| // A value of 0 means let the provider assign a port. | ||
| ListeningPort uint16 |
There was a problem hiding this comment.
@mpatton125 please confirm if http://10.31.33.7/fwd tells you you can specify a port field, and if that port field can be set to 0 to let cryptostorm assign you a random port.
If this is the case:
- use the
VPN_PORT_FORWARDING_LISTENING_PORTvalue to set this. If this one is not set, try to read it from/gluetun/portforward/cryptostorm.json. If this file does not exist, leave the ListeningPort field to 0 to let cryptostorm decide. - in all cases, persist the resulting port(s) to
/gluetun/portforward/cryptostorm.json
And also:
- add a comment in internal/provider/privateinternetaccess/provider.go next to
const jsonPortForwardPath = "/gluetun/piaportforward.json"sayingTODO v4: move to /gluetun/portforward/privateinternetaccess.json - update the comment for the settings field
VPN_PORT_FORWARDING_LISTENING_PORTand make sure it runs a no-op in case the forwarded port is already the value specified inVPN_PORT_FORWARDING_LISTENING_PORT
There was a problem hiding this comment.
Yes you can specify a port at that URL, from 30000-65535. No, it can't be 0 for a random port - user must specify.
There was a problem hiding this comment.
Got it, let's wait for #3208
note for future:
we'll then use VPN_PORT_FORWARDING_LISTENING_PORTS=8000,8001 to specify ports you want. We should then change func (p *PortForwarding) setDefaults() to func (p *PortForwarding) setDefaults(vpnProvider string) and set, if the vpnProvider is cryptostorm only, p.PortsCount = gosettings.DefaultComparable(p.PortsCount, len(p.ListeningPorts))
2c06921 to
9a5995f
Compare
cd21991 to
e79b2df
Compare
40f126b to
44d5104
Compare
| } | ||
|
|
||
| err = s.onNewPorts(ctx, ports) | ||
| if s.settings.ListeningPort != 0 && port != s.settings.ListeningPort { |
There was a problem hiding this comment.
(no action for now) to be updated after #3208 is merged - update to check by index ports[i] != s.settings.ListeningPorts[i] and also we should ensure both slices are sorted
|
|
||
| const base, bitSize = 10, 16 | ||
| for _, match := range matches { | ||
| portUint64, err := strconv.ParseUint(match[1], base, bitSize) |
There was a problem hiding this comment.
rebase on upstream master, which supports multiple ports. What's the number of ports limit?
| client *http.Client, updaterWarner common.Warner, | ||
| parallelResolver common.ParallelResolver, | ||
| ) *Provider { | ||
| const jsonPortForwardPath = "/gluetun/portforward/cryptostorm.json" |
| PortsCount uint16 | ||
| // ListeningPorts are the user-specified ports. For Cryptostorm, ListeningPorts[0] | ||
| // is the port to request from the forwarding server (must be 30000–65535). | ||
| // A nil or [0] value means fall back to the persisted port. |
There was a problem hiding this comment.
can there be multiple ports requested?
There was a problem hiding this comment.
From some preliminary testing, I don't think you can request multiple ports in one POST request, but you can have multiple ports open if you request them one by one. You can close them all at once though:
➜ curl --request POST --data 'port=80808,80809' http://10.31.33.7/fwd
INVALID_PARAM_RECEIVED
Add a port:
wget -qO- http://10.31.33.7/fwd --post-data="port=31337"
curl -sS http://10.31.33.7/fwd -d port=31337
Delete one port:
wget -qO- http://10.31.33.7/fwd --post-data="delfwd=31337"
curl -sS http://10.31.33.7/fwd -d delfwd=31337
Delete all ports:
wget -qO- http://10.31.33.7/fwd --post-data="delallfwd=1"
curl -sS http://10.31.33.7/fwd -d delallfwd=1Leaving the parameter empty, setting it to 0, or trying ports instead of port are all invalid. It seems you have to explicitly request one specific port at a time.
I'd like to see this merged, so let me know if there's anything else you'd like me to test.
There was a problem hiding this comment.
Interesting, something we can add later (or now up to you) for multiple ports
…and IPv6 Implements Cryptostorm as a gluetun VPN provider supporting WireGuard and OpenVPN protocols, dual-stack IPv4/IPv6 port forwarding via HTTP to the Cryptostorm forwarding servers, and a server updater that fetches node data from cryptostorm.is/wireguard. Key changes: - Provider struct using ConnectionPicker (aligned with master's pattern) - Port forwarding with IPv4 (10.31.33.7) and IPv6 (2001:db8::7) endpoints - Updater with simplified client-only struct (no parallelResolver/warner) - DNS resolution via net.DefaultResolver in the updater - setDefaults(vpnProvider) signature to set PortsCount default for Cryptostorm - Self-redirect guard in portforward service onNewPorts() - ListeningPorts support in PortForwardObjects Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s module When a provider's JSON file is not yet in the embedded gluetun-servers module (e.g. newly added providers like Cryptostorm), start with an empty server list instead of panicking. The updater will populate servers at runtime. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
c9f07b2 to
33410b3
Compare
…errors common.ErrPortForwardNotSupported and common.ErrHTTPStatusCodeNotOK were removed from master. Replace with inline fmt.Errorf calls matching the pattern used by other providers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without a version, checkVersions() discards any on-disk data for the provider because version 0 never matches the file's version 1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
What work remains before this can be merged? Is it's just linting and git tree management or are code changes needed before it can be merged? |
|
I'm not sure what needs to be done for it to be merged. It works - I've been using it for months. |
|
I'll review this post v3.42.0 release which I'm trying to get out soon - I'm still playing catchup with PRs these days, but I'll get to it ! Edit : I'll try to review it tomorrow fingers crossed ! |
| // the corresponding listening port. For Cryptostorm, ListeningPorts[0] | ||
| // also specifies the port to request from the forwarding server (30000–65535). |
There was a problem hiding this comment.
since cryptostorm appears to support multiple ports, I think it would be nice to support it here. That way that comment can be changed to:
| // the corresponding listening port. For Cryptostorm, ListeningPorts[0] | |
| // also specifies the port to request from the forwarding server (30000–65535). | |
| // the corresponding listening port. For VPN providers supporting requesting | |
| // specific ports (Cryptostorm), this is used to request ports. |
| // For Cryptostorm, default PortsCount to the number of listening ports | ||
| // specified, since each listening port maps directly to a requested port. | ||
| defaultPortsCount := uint16(1) | ||
| if vpnProvider == providers.Cryptostorm && | ||
| len(p.ListeningPorts) > 0 && p.ListeningPorts[0] != 0 { | ||
| defaultPortsCount = uint16(len(p.ListeningPorts)) | ||
| } | ||
| p.PortsCount = gosettings.DefaultComparable(p.PortsCount, defaultPortsCount) |
There was a problem hiding this comment.
since the Validate method already takes the vpn provider string as argument, let's just validate that the user sets the ports count to the same number of listening ports, and revert the changes to setDefaults here.
By the way, can a user request a random port instead of a specific port?
| seen := make(map[string]struct{}, len(hts)) | ||
| hosts = make([]string, 0, len(hts)) | ||
| for key := range hts { | ||
| host := extractHost(key) | ||
| if _, ok := seen[host]; !ok { | ||
| seen[host] = struct{}{} | ||
| hosts = append(hosts, host) | ||
| } | ||
| } | ||
| return hosts |
There was a problem hiding this comment.
nit a tiny bit shorter
| seen := make(map[string]struct{}, len(hts)) | |
| hosts = make([]string, 0, len(hts)) | |
| for key := range hts { | |
| host := extractHost(key) | |
| if _, ok := seen[host]; !ok { | |
| seen[host] = struct{}{} | |
| hosts = append(hosts, host) | |
| } | |
| } | |
| return hosts | |
| hosts := slices.Collect(maps.Keys(hts)) | |
| for i, h := range hosts { | |
| hosts[i] = extractHost(h) | |
| } | |
| slices.Sort(hosts) | |
| return slices.Compact(hosts) |
| for _, url := range portForwardURLs { | ||
| found, err := p.registerPort(ctx, objects.Client, url, postBody, listeningPort, objects) | ||
| if err != nil { | ||
| if url == portForwardURLs[0] { | ||
| // IPv4 failure is fatal. | ||
| return nil, err | ||
| } | ||
| // IPv6 failure is a warning only. | ||
| objects.Logger.Warn("IPv6 port forward registration failed (" + url + "): " + err.Error()) | ||
| continue | ||
| } |
There was a problem hiding this comment.
nit do it outside a for loop, this is a bit confusing that index 0 is ipv4 and index 1 is ipv6. Just have two code blocks following each-other, and ideally enable the ipv6 block in an if block if ipv6 is in fact supported.
| matches = regexForwardHTML.FindAllStringSubmatch(bodyStr, -1) | ||
| } | ||
| if len(matches) == 0 { | ||
| return false, fmt.Errorf("port forwarding not supported: no active port forwards found in response") |
There was a problem hiding this comment.
| return false, fmt.Errorf("port forwarding not supported: no active port forwards found in response") | |
| return false, fmt.Errorf("port forwarding not supported: no active port forwards found in response: %s", bodyStr) |
| // registerPort POSTs the port to a single forwarding endpoint and returns | ||
| // whether the requested port was confirmed in the response. | ||
| func (p *Provider) registerPort(ctx context.Context, client *http.Client, |
There was a problem hiding this comment.
nit move this below KeepPortForward and above deletePortFromURL
|
|
||
| // Best-effort deregister on teardown. Use a fresh context since the | ||
| // original is already cancelled. | ||
| if p.forwardedPort != 0 { |
There was a problem hiding this comment.
can forwardedPort be ever 0 when we call .KeepPortForward? I don't think so 🤔
| // Provider not yet in the gluetun-servers module; start with an empty list. | ||
| // The updater can still populate servers at runtime. | ||
| const serversPath = "/gluetun/servers/" | ||
| allServers.ProviderToServers[provider] = models.Servers{ | ||
| Version: 1, | ||
| Filepath: filepath.Join(serversPath, filename), | ||
| } | ||
| continue |
There was a problem hiding this comment.
Remove this and in go.mod, add a replace github.com/qdm12/gluetun-servers github.com/mpatton125/gluetun-servers#main (or something of that nature) to get cryptostorm data from the PR? Typing from my phone but I'm curious if this works, since that would be useful for future VPN provider PRs!
Description
Issue
Fix #3112
🤖 Generated with Claude Code <--- Full disclosure