From e0ec8b012826172fca655f56c273d8f53d14024c Mon Sep 17 00:00:00 2001
From: Ain Ghazal <99027643+ainghazal@users.noreply.github.com>
Date: Tue, 13 Feb 2024 19:37:24 +0100
Subject: [PATCH] tests: adapt integration tests to run against the refactored
 code (#65)

As part of a previous commit ($64), I reverted a recent "fix" that was
inverting the local-remote session id check - the error in that commit
was to change the check to match the description in the comment. In
reality, it was the comment that was wrong.

This commit enables integration tests for the new internal path to avoid
this kind of errors in the future.

Some test code was removed, because the script to parse certs etc was
written before minivpn could parse inline credentials. not needed
anymore.

For more context: the reason why we cannot check remote session id in
all packets is because remote-session-id only present if the packet has
an ack array. The handshake was broken by the previous change, but I did
not catch it because the integration tests were not running for the
refactored codebase.
---
 .github/workflows/build-refactor.yml          |   2 +-
 Makefile                                      |   5 +-
 tests/integration/extract.sh                  |  55 ---------
 .../integration/{openvpn_test.go => main.go}  | 110 +++++++-----------
 4 files changed, 48 insertions(+), 124 deletions(-)
 delete mode 100755 tests/integration/extract.sh
 rename tests/integration/{openvpn_test.go => main.go} (66%)

diff --git a/.github/workflows/build-refactor.yml b/.github/workflows/build-refactor.yml
index c3751ee9..53b284e6 100644
--- a/.github/workflows/build-refactor.yml
+++ b/.github/workflows/build-refactor.yml
@@ -62,5 +62,5 @@ jobs:
         with:
           go-version: '1.21'
       - name: run integration tests
-        run: go test -v ./tests/integration
+        run: go run ./tests/integration
 
diff --git a/Makefile b/Makefile
index 376d521f..5ad10a21 100644
--- a/Makefile
+++ b/Makefile
@@ -57,7 +57,6 @@ integration-server:
 test-fetch-config:
 	rm -rf data/tests
 	mkdir -p data/tests && curl 172.17.0.2:8080/ > data/tests/config
-	cd data/tests && ../../tests/integration/extract.sh config
 
 test-ping-local:
 	# run the integration-server first
@@ -72,10 +71,12 @@ qa:
 	@rm -rf data/tests
 	@mkdir -p data/tests && curl 172.17.0.2:8080/ > data/tests/config
 	@sleep 1
-	@cd data/tests && ../../tests/integration/extract.sh config
 	./minivpn -c data/tests/config -t 172.17.0.1 -n ${COUNT} ping
 	@docker stop ovpn1
 
+integration:
+	go run ./tests/integration
+
 filternet-qa:
 	cd tests/qa && ./run-filternet.sh remote-block-all
 
diff --git a/tests/integration/extract.sh b/tests/integration/extract.sh
deleted file mode 100755
index 9c1ec31d..00000000
--- a/tests/integration/extract.sh
+++ /dev/null
@@ -1,55 +0,0 @@
-#!/bin/sh
-# (c) Ain Ghazal 2022
-# this file converts an inline openvpn config file into
-# a standalone config plus separate files for the ca.crt,
-# cert.pem and key.pem.
-
-FILE=$1
-tail=0
-
-# first lets extract the inline blocks
-# ca block
-tag=ca
-f=ca.crt
-sed -n "/<$tag>/,/<\/$tag>/p" $FILE > $f
-n=$(wc -l $f | cut -f 1 -d ' ')
-tail=$(($tail+n))
-cat $f | tail -n $(($n-1)) | head -n $(($n-2)) | tee $f >/dev/null
-
-# key block
-tag=key
-f=key.pem
-sed -n "/<$tag>/,/<\/$tag>/p" $FILE > $f
-n=$(wc -l $f | cut -f 1 -d ' ')
-tail=$(($tail+n))
-cat $f | tail -n $(($n-1)) | head -n $(($n-2)) | tee $f >/dev/null
-
-# cert block
-tag=cert
-f=cert.pem
-sed -n "/<$tag>/,/<\/$tag>/p" $FILE > $f
-n=$(wc -l $f | cut -f 1 -d ' ')
-tail=$(($tail+n))
-cat $f | tail -n $(($n-1)) | head -n $(($n-2)) | tee $f >/dev/null
-
-# tls-auth (ignored)
-tag=tls-auth
-f=ta.pem
-sed -n "/<$tag>/,/<\/$tag>/p" $FILE > $f
-n=$(wc -l $f | cut -f 1 -d ' ')
-tail=$(($tail+n))
-cat $f | tail -n $(($n-4)) | head -n $(($n-5)) | tee $f >/dev/null
-
-all=$(wc -l $FILE | cut -f -1 -d ' ')
-cp $FILE config.bk
-
-echo "------------------------"
-echo "Config file:"
-echo "------------------------"
-head -n $(($all-$tail)) $FILE | tee config
-echo "------------------------"
-
-# now enable the paths for ca, cert and key
-sed -i "s/;ca ca.crt/ca ca.crt/g" config
-sed -i "s/;cert cert.pem/cert cert.pem/g" config
-sed -i "s/;key key.pem/key key.pem/g" config
diff --git a/tests/integration/openvpn_test.go b/tests/integration/main.go
similarity index 66%
rename from tests/integration/openvpn_test.go
rename to tests/integration/main.go
index 07060139..4b5f21f8 100644
--- a/tests/integration/openvpn_test.go
+++ b/tests/integration/main.go
@@ -1,29 +1,26 @@
-//build: +integration
 package main
 
 import (
 	"bufio"
-	"bytes"
 	"context"
 	"fmt"
-	"io/ioutil"
-	"log"
+	"io"
+	"net"
 	"net/http"
 	"os"
-	"os/exec"
 	"path/filepath"
-	"testing"
-	"time"
 
+	"github.com/apex/log"
 	"github.com/ory/dockertest/v3"
 	dc "github.com/ory/dockertest/v3/docker"
 
 	"github.com/ooni/minivpn/extras/ping"
-	"github.com/ooni/minivpn/vpn"
+	"github.com/ooni/minivpn/internal/model"
+	"github.com/ooni/minivpn/internal/networkio"
+	"github.com/ooni/minivpn/internal/tun"
 )
 
 const (
-	parseConfig = "extract.sh"
 	dockerImage = "ainghazal/openvpn"
 	dockerTag   = "latest"
 )
@@ -34,14 +31,14 @@ var (
 )
 
 func copyFile(src, dst string) error {
-	input, err := ioutil.ReadFile(src)
+	input, err := os.ReadFile(src)
 	if err != nil {
 		fmt.Println(err)
 		return nil
 	}
 
 	dstFile := filepath.Join(dst, src)
-	err = ioutil.WriteFile(dstFile, input, 0744)
+	err = os.WriteFile(dstFile, input, 0744)
 	if err != nil {
 		fmt.Println("Error creating", dstFile)
 		return err
@@ -73,7 +70,7 @@ func launchDocker(cipher, auth string) ([]byte, *dockertest.Pool, *dockertest.Re
 	// the minio client does not do service discovery for you (i.e. it does not check if connection can be established), so we have to use the health check
 	var config []byte
 	if err := pool.Retry(func() error {
-		url := fmt.Sprintf("http://localhost:8080/")
+		url := "http://localhost:8080/"
 		resp, err := http.Get(url)
 		if err != nil {
 			return err
@@ -81,7 +78,7 @@ func launchDocker(cipher, auth string) ([]byte, *dockertest.Pool, *dockertest.Re
 		if resp.StatusCode != http.StatusOK {
 			return fmt.Errorf("status code not OK")
 		}
-		config, err = ioutil.ReadAll(resp.Body)
+		config, err = io.ReadAll(resp.Body)
 		if err != nil {
 			panic(err)
 		}
@@ -94,75 +91,71 @@ func launchDocker(cipher, auth string) ([]byte, *dockertest.Pool, *dockertest.Re
 }
 
 func stopContainer(p *dockertest.Pool, res *dockertest.Resource) {
-	fmt.Println("Stopping container")
+	fmt.Println("[!] Stopping container")
 	if err := p.Purge(res); err != nil {
-		log.Printf("Could not purge resource: %s\n", err)
+		log.Warnf("Could not purge resource: %s\n", err)
 	}
 }
 
-func TestClientAES256GCM(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skipping test in short mode.")
-	}
-	tmp := t.TempDir()
-
-	copyFile(parseConfig, tmp)
-	os.Chdir(tmp)
-	err := os.Chmod(parseConfig, 0700)
+func readLines(f string) ([]string, error) {
+	var ll []string
+	rf, err := os.Open(f)
 	if err != nil {
-		log.Fatal(err)
+		return ll, err
+	}
+	defer rf.Close()
+	fs := bufio.NewScanner(rf)
+	fs.Split(bufio.ScanLines)
+	for fs.Scan() {
+		ll = append(ll, fs.Text())
 	}
+	return ll, nil
+}
 
-	config, pool, resource, err := launchDocker("AES-256-GCM", "SHA256")
+// This main function exercises AES256GCM
+func main() {
+	tmp, err := os.MkdirTemp("", "minivpn-integration-test")
+	defer os.RemoveAll(tmp) // clean up
 
+	fmt.Println("launching docker")
+	config, pool, resource, err := launchDocker("AES-256-GCM", "SHA256")
 	if err != nil {
-		log.Fatal(err)
+		log.WithError(err).Fatal("cannot start docker")
 	}
 	// when all test done, time to kill and remove the container
 	defer stopContainer(pool, resource)
 
-	cfgFile, err := ioutil.TempFile(tmp, "minivpn-e2e-")
-	defer cfgFile.Close()
+	cfgFile, err := os.CreateTemp(tmp, "minivpn-e2e-")
 	if err != nil {
-		log.Fatal("Cannot create temporary file", err)
+		log.WithError(err).Fatal("Cannot create temporary file")
 	}
+	defer cfgFile.Close()
 	fmt.Println("Config written to: " + cfgFile.Name())
 
 	if _, err = cfgFile.Write(config); err != nil {
-		log.Fatal("Failed to write config to temporary file", err)
+		log.WithError(err).Fatal("Failed to write config to temporary file")
 	}
 
-	// execute the extract.sh shell script, to process key blocks piecewise
-	cmd := exec.Command("./"+parseConfig, cfgFile.Name())
-	cmd.Dir = tmp
-	var out bytes.Buffer
-	cmd.Stdout = &out
-	cmd.Run()
+	// actual test begins
+	vpnConfig := model.NewConfig(model.WithConfigFile(cfgFile.Name()))
 
+	dialer := networkio.NewDialer(log.Log, &net.Dialer{})
+	conn, err := dialer.DialContext(context.TODO(), vpnConfig.Remote().Protocol, vpnConfig.Remote().Endpoint)
 	if err != nil {
-		log.Fatal(err)
+		log.WithError(err).Fatal("dial error")
 	}
 
-	c, err := readLines("config")
-	fmt.Println("Remote:", c[len(c)-1])
-	// can assert that this is a remote line
-
-	// actual test begins
-	opt, err := vpn.NewOptionsFromFilePath(filepath.Join(tmp, "config"))
+	tunnel, err := tun.StartTUN(context.Background(), conn, vpnConfig)
 	if err != nil {
-		log.Fatalf("Could not parse file: %s", err)
+		log.WithError(err).Fatal("cannot start tunnel")
 	}
-	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
-	defer cancel()
 
-	tunnel := vpn.NewClientFromOptions(opt)
-	tunnel.Start(ctx)
 	pinger := ping.New(target, tunnel)
 	pinger.Count = count
-	err = pinger.Run(ctx)
+	err = pinger.Run(context.Background())
 	defer pinger.Stop()
 	if err != nil {
-		log.Fatalf("VPN Error: %s", err)
+		log.WithError(err).Fatalf("VPN Error")
 	}
 	if pinger.PacketLoss() != 0 {
 		log.Fatalf("packet loss is not zero")
@@ -170,18 +163,3 @@ func TestClientAES256GCM(t *testing.T) {
 	// let's assert something wise about the pings
 	// can we parse the logs? get initialization etc
 }
-
-func readLines(f string) ([]string, error) {
-	var ll []string
-	rf, err := os.Open(f)
-	defer rf.Close()
-	if err != nil {
-		return ll, err
-	}
-	fs := bufio.NewScanner(rf)
-	fs.Split(bufio.ScanLines)
-	for fs.Scan() {
-		ll = append(ll, fs.Text())
-	}
-	return ll, nil
-}