After discussion with @bassosimone, we decided to reorganize the structure of ths module:

• move vpn to the top of the module, since it contains the most useful public exports.
• create different cmd entrypoints.

this issue might belongs to probe-cli, but I'm keeping track of it here since that functionality is not yet merged.

it's not clear to me if this is bad error handling on the side of the nettest, or an issue with how pinger handles context cancellation.

in minivpn/vpn, at some point I made an attempt at passing a logger from options to the client.
that attempt is not completed (and Options should perhaps not be polluted with this); the interface is there, but right now all the logs are using the default (global) logger.

should either normalize use of client's logger (which means passing it down the hierarchy)... or perhaps setup the global logger at the beginning of Start() based on any non-nil implementation in Client.

to prevent a potential LFI, the path that is parsed for certificate material in the config file should:

• be sanitized (use filepath.Clean())
• is not a symlink
• check that the absolute path is below the base dir (ie, the dir in which the config file is).

Overall, there're not so many things that can be exploited via this path, but it's good hygiene to sanitize our inputs.

From a review in #23:

I was thinking you could perhaps just call server.ListendAndServe with addr = {ip}:0 so as to make the OS pick the next available port for you.

I then saw, though, that the sock5 library you are using doesn't return the net.listener (https://github.com/armon/go-socks5/blob/master/socks5.go#L100) so if you actually need to know the picked port you have no way of extracting it :(

I guess we can leave this as-is for the time being, but it might be worth opening an issue as future work to open PR on https://github.com/armon/go-socks5 to return the listener (or set it as an attribute to the server struct).

• MIV-01-007 Possible DoS via Predictable Port Usage

Originally posted by @hellais in #23 (comment)

Right now, the case for certificate-based authentication for clients does not capture properly an expired certificate. This is specially problematic because it confounds bad credentials with other kind of handshake errors.

besides cli arguments it would be desirable to have compatibility with a minimal subset of openvpn configuration options. minimally:

cipher
auth
tls-cipher
remote
auth-user-pass

as noted in #11, I probably should be smarter about retries in the handshake phase if in UDP mode. the current implementation is too optimistic in this sense - we should check what does the ref implementation do.

seems bad ux to demand proto to be an integer

For convenience, we want to add a minimal form of support for the "inline file support" that openvpn accepts for the configuration format.

ca, cert and key are the minimal subset that we need to support inline right now. this makes much more convenient to generate a single config file assembled from other parts (i.e, a template filled with options from a oonirun descriptor).

I am not sure about the usability consequences of this, but right now the device that connects both halves of a tunnel only has an Up() method. It's probably a good idea to provide a way of stopping the loop too.

During discussion of #35 we considered the constrains that led to having two different parsing implementations. The gist of it was:

switch p.Raw {
case false:
	pkt = p.parseEchoReplyFromICMP(recv.bytes, from)
case true:
	pkt = p.parseEchoReplyFromIP(recv.bytes)
}

The reason is that in minivpn's client we're writing raw IP packets, while in the case of wireguard we're using a ICMP Dialer that does not allow us to parse the IP packet. We decided to unify the design and let gvisor handle the icmp packets in both cases, but to merge this PR and defer the refactor.

With @bassosimone we also decided to make the needed modifications to intercept the incoming packets in gvisor's netstack, so that we can track the TTL value for the incoming replies (as that seems to be the main advantage of parsing the IP packets in the case of the pinger usage for minivpn).

See also #40, since this refactor can probably be made while splitting the package into its own repo & reusable module.

Originally reported by 7asecurity during their security audit:

During TLS/VPN negotiation, OpenVPN clients send a Change Cipher Spec P_CONTROL_V1 packet, which contains an embedded P_ACK_V1 packet. OpenVPN in that way uses the ability to combine both P_CONTROL and P_ACK payloads1 inside a single packet. However, minivpn sends two packets (see figure)

I think this is due to the naive implementation of the ack mechanism in the current state, that doesn't allow to send ACKs for several packet ids within a single control packet. I think this can be better handled after landing #32

after last refactor, Dialer is not set on the TunDialer anymore, so any dialer needs to be passed to the client (on the fields).

should test dialers that raise errors.

extras/ping was forked after https://github.com/go-ping/ping, and modified to be able to send and receive IP packets over the "raw" vpn.Client (an openvpn connection).

We have decided to temporarily merge #35, that implements a different signature to use a Ping Connection that comes from wireguard code - discussion on a separated issue.

Independently of implementation and refactors, it is now clear that it would be useful to consider the Pinger as an independently useful component, that does not depend on vpn specific code. In that way we can maintain it on its own, and import it from any experiments that need to use it.

It would be very useful to be able to validate that any authentication method has the required inputs.

For certs:

• Valid PEM files for ca, cert and key (ie, can be parsed with encoding/pem).
• Check that credentials are not expired.

For username authentication:

• Username and password (token) strings should not be null.

right now, the implementation is setting up a rough mtu for the virtual tun device, just substracting an arbitrary quantity from he mtu that the server announces. we should investigate how to properly define this MTU.

While being used from probe-cli, it would be very useful to be able to quickly see at which stage in a connection the client bootstrap has been left - either after a failure, timeout or regular operation.

@bassosimone suggested something along these lines:

• Add a public field to the Client struct, that is nil with the default constructor.
• Define an Event data structure (that maps to, say, uint16).
• The new public field is a chan(Event).
• If not nil, the client can emit a new event on that channel for each state transition that we want to register.
• The receiver should either make sure that the channel is consumed regularly, or provide a sufficiently-buffered channel.

There's a practical use case that needs to know a couple of addresses:

• The local IP on the network. I have my doubts here (privacy-wise), but in some circumstances this can be used as a proxy for the gateway load.
• The gateway IP (from the pushed routes the gateway sends us). I'd like to expose this so that probes can then send ICMP pings to the gateway (so that we can estimate the delta to other targets).

I know that supposedly the parrot is dead, but if the effort is low maybe it makes sense to address a couple of obvious divergences:

• The trailing 4 bytes in the random field for our (parroted) ClientHello look totally random - but at least by Wireshark's dissector they get recognized as a timestamp (I haven't checked the ranges extensively, at least they look to be placed between 2000-2100?). Probably openvpn source code is the quickest way to clarify this.

• The fact that I'm using DATA_V1 packets (intertwined with HARD_RESET_V2) while a recent openvpn uses V2.

Some providers I was intending to test against have an old ciphersuite that is not supported by go standard library:

golang/go#31933 (comment)

We are very unlikely to implement finite field DH in crypto/tls for the reasons mentioned by @jamie-digital. It's a bad design, very hard to implement securely and in constant time, and superseded by ECDHE. I am also unaware of any clients that support DHE but not the plain RSA ciphers, which we carry along for compatibility (and so we can avoid adding the complexity of things like DHE instead). Feature parity is explicitly a non-goal of crypto/tls.

golang/go#31933
golang/go#7758

There's a fork that adds some old ciphers https://github.com/mordyovits/golang-crypto-tls

for some coments, it looks like the quality of that code might be subpar, but perhaps worth a try

OpenVPN implements its own reliability layer on top of UDP (it's worthy to note that it's used for TCP too). We should follow the reference implementation as closely as possible.

This basically boils down to keep track of acknowledged packets on a fixed-size structure, and retry sending if acks have not been received after a given period. Retries follow an exponential back-off, and incoming packets with ids beyond a certain acknowledgment window will be dropped since sequentiality cannot be guaranteed by the data structure.

edit: for some reason I had originally written "sentimentality" in the last sentence.

Relevant pointers:

• https://build.openvpn.net/doxygen/group__reliable.html#details
• https://github.com/roburio/openvpn/blob/main/src/engine.ml
• https://github.com/glacjay/govpn/blob/master/reliable_udp.go very elegant implementation, imho.

it'd be useful if the client can report the bootstrap time

since this is public API, we should propagate parsing errors on the constructor.

See comments during #68 code review:
#68 (comment)

I'm unsure about this, but it might help with tls fingerprinting.
I do think censors have easier ways to pick an openvpn flow, but worth trying.

In the past months we have been working on a major overhaul of the existing codebase to be able to use a new layered approach.

This will allow us for better local reasoning of the different components, and to write unit tests in an isolated way.

right now (under internal/packetmuxer) we will fail if we cannot correctly parse an incoming packet (for instance, if the incoming packet has bogus bytes prepended).

we need to continue receiving, and possibly log & trace this event.

right now for testing I'm using InsecureSkipVerify=true