Fix the review findings in connect-tailscale

tailscaled is waited for before tailscale up, a tailscaled that already runs on
the runner is not taken over and down leaves a daemon it did not start, sudo is
used only when the job is not root, an exact version is required to install
(with an optional sha256 of the archive), and the guide states when the node is
ephemeral and calls @main until the release.

Refs #185
Task: MIL-012#4

actions/connect-tailscale/action.yml

@@ -28,9 +28,13 @@ inputs:
     required: false
     default: ""
   version:
-    description: "Tailscale version to install when tailscale is not on the runner, e.g. 1.80.2. Pin it; latest is allowed"
+    description: "Exact Tailscale version to install when tailscale is not on the runner, e.g. 1.80.2. Empty uses the installed tailscale and fails when there is none"
     required: false
-    default: "latest"
+    default: ""
+  sha256:
+    description: "Optional SHA-256 of the downloaded archive tailscale_<version>_<arch>.tgz; the install fails when it differs"
+    required: false
+    default: ""
   userspace-networking:
     description: >-
       true runs tailscaled without a TUN device (no NET_ADMIN needed); tailnet
@@ -70,6 +74,7 @@ runs:
         TS_TAGS: ${{ inputs.tags }}
         TS_HOSTNAME: ${{ inputs.hostname }}
         TS_VERSION: ${{ inputs.version }}
+        TS_SHA256: ${{ inputs.sha256 }}
         TS_USERSPACE: ${{ inputs.userspace-networking }}
         TS_ARGS: ${{ inputs.args }}
         TS_TIMEOUT: ${{ inputs.timeout }}

actions/connect-tailscale/connect-tailscale.sh

@@ -7,14 +7,24 @@
 #
 # Settings come from the environment, never from interpolated expressions:
 #   TS_AUTHKEY, TS_OAUTH_SECRET, TS_TAGS, TS_HOSTNAME, TS_VERSION, TS_USERSPACE,
-#   TS_ARGS, TS_TIMEOUT, TS_HEALTH_CHECK_HOST. TS_TAILSCALE, TS_TAILSCALED,
+#   TS_SHA256, TS_ARGS, TS_TIMEOUT, TS_HEALTH_CHECK_HOST. TS_TAILSCALE,
-#   TS_SUDO and TS_STATE_DIR name the commands and the work folder, for the tests.
+#   TS_TAILSCALED, TS_SUDO, TS_STATE_DIR, TS_START_WAIT (tries) and
+#   TS_WAIT_INTERVAL (seconds) name the commands, the work folder and the waits,
+#   for the tests.
 # The auth key is passed to tailscale in a file of mode 600, never on a command
 # line. Every failure is printed as ::error::[category] message.
 
 TS_TAILSCALE="${TS_TAILSCALE:-tailscale}"
 TS_TAILSCALED="${TS_TAILSCALED:-tailscaled}"
-SUDO="${TS_SUDO-sudo}"
+# sudo is used only when the job is not root: a job container runs as root and
+# often has no sudo. TS_SUDO overrides it (empty for none).
+if [ -n "${TS_SUDO+x}" ]; then
+  SUDO="$TS_SUDO"
+elif [ "$(id -u)" = "0" ]; then
+  SUDO=""
+else
+  SUDO="sudo"
+fi
 STATE_DIR="${TS_STATE_DIR:-${RUNNER_TEMP:-/tmp}/tailscale}"
 
 fail() {
@@ -46,11 +56,13 @@ validate_inputs() {
   fi
   [[ "$TS_TIMEOUT" =~ ^[0-9]+$ ]] ||
     fail configuration "timeout must be a number of seconds."
-  [[ "$TS_VERSION" =~ ^(latest|[0-9]+\.[0-9]+\.[0-9]+)$ ]] ||
+  [[ "$TS_VERSION" =~ ^([0-9]+\.[0-9]+\.[0-9]+)?$ ]] ||
-    fail configuration "version must be latest or like 1.80.2."
+    fail configuration "version must be an exact version like 1.80.2."
+  [[ "${TS_SHA256:-}" =~ ^([0-9a-fA-F]{64})?$ ]] ||
+    fail configuration "sha256 must be 64 hexadecimal digits."
   case "$TS_USERSPACE" in
-  true | false) ;;
+    true | false) ;;
-  *) fail configuration "userspace-networking must be true or false." ;;
+    *) fail configuration "userspace-networking must be true or false." ;;
   esac
   [[ "${TS_HOSTNAME:-}" =~ ^[a-zA-Z0-9-]*$ ]] ||
     fail configuration "hostname may hold letters, digits and hyphens only."
@@ -58,14 +70,16 @@ validate_inputs() {
 
 machine_arch() {
   case "$(uname -m)" in
-  x86_64 | amd64) echo amd64 ;;
+    x86_64 | amd64) echo amd64 ;;
-  aarch64 | arm64) echo arm64 ;;
+    aarch64 | arm64) echo arm64 ;;
-  *) fail environment "no Tailscale build for $(uname -m)." ;;
+    *) fail environment "no Tailscale build for $(uname -m)." ;;
   esac
 }
 
 install_tools() {
   command -v "$TS_TAILSCALE" >/dev/null 2>&1 && command -v "$TS_TAILSCALED" >/dev/null 2>&1 && return 0
+  [ -n "$TS_VERSION" ] ||
+    fail configuration "tailscale is not on the runner; set version (for example 1.80.2) so it can be installed."
   local arch url tmp
   arch="$(machine_arch)"
   url="https://pkgs.tailscale.com/stable/tailscale_${TS_VERSION}_${arch}.tgz"
@@ -73,6 +87,10 @@ install_tools() {
   tmp="$(mktemp -d)"
   curl -fsSL "$url" -o "$tmp/tailscale.tgz" ||
     fail environment "could not download $url."
+  if [ -n "${TS_SHA256:-}" ]; then
+    echo "${TS_SHA256}  $tmp/tailscale.tgz" | sha256sum -c --quiet - ||
+      fail environment "the downloaded archive does not match sha256."
+  fi
   tar -xzf "$tmp/tailscale.tgz" -C "$tmp" --strip-components=1 ||
     fail environment "could not unpack the Tailscale archive."
   $SUDO install -m 755 "$tmp/tailscale" "$tmp/tailscaled" /usr/local/bin/ ||
@@ -80,13 +98,34 @@ install_tools() {
   rm -rf "$tmp"
 }
 
+daemon_answers() {
+  "$TS_TAILSCALE" version --daemon >/dev/null 2>&1
+}
+
 start_daemon() {
+  # a daemon that already runs belongs to the host; this action would log it out
+  if daemon_answers; then
+    fail environment "a tailscaled already runs on this runner; the action starts its own and must not take over the host's. Use it on a job container."
+  fi
   local args=(--state=mem:)
   if [ "$TS_USERSPACE" = "true" ]; then
     args+=(--tun=userspace-networking --socks5-server=localhost:1055)
   fi
   mkdir -p "$STATE_DIR"
   $SUDO nohup "$TS_TAILSCALED" "${args[@]}" >"$STATE_DIR/tailscaled.log" 2>&1 &
+  : >"$STATE_DIR/started-by-action"
+  wait_for_daemon
+}
+
+# wait_for_daemon: tailscaled needs a moment to create its socket
+wait_for_daemon() {
+  local tries="${TS_START_WAIT:-30}" i
+  for ((i = 0; i < tries; i++)); do
+    daemon_answers && return 0
+    sleep "${TS_WAIT_INTERVAL:-1}"
+  done
+  fail connection "tailscaled did not answer in $tries tries; the last lines of its log: $(tail -n 5 "$STATE_DIR/tailscaled.log" 2>/dev/null | tr '
+' ' ')"
 }
 
 # write_key_file: the auth key in a file of mode 600; prints its path
@@ -127,6 +166,11 @@ tailnet_up() {
 }
 
 tailnet_down() {
+  # only a daemon this action started is logged out and stopped
+  if [ ! -f "$STATE_DIR/started-by-action" ]; then
+    echo "This action did not start tailscaled on this runner; nothing to do."
+    return 0
+  fi
   $SUDO "$TS_TAILSCALE" logout 2>/dev/null || echo "The node was not logged in."
   $SUDO "$TS_TAILSCALE" down 2>/dev/null || true
   $SUDO pkill -x "$(basename "$TS_TAILSCALED")" 2>/dev/null || true
@@ -138,12 +182,12 @@ main() {
   set -uo pipefail
   TS_USERSPACE="${TS_USERSPACE:-false}"
   TS_TIMEOUT="${TS_TIMEOUT:-60}"
-  TS_VERSION="${TS_VERSION:-latest}"
+  TS_VERSION="${TS_VERSION:-}"
   case "${1:-}" in
-  mask) mask_secrets ;;
+    mask) mask_secrets ;;
-  up) tailnet_up ;;
+    up) tailnet_up ;;
-  down) tailnet_down ;;
+    down) tailnet_down ;;
-  *) fail configuration "unknown command '${1:-}'; use mask, up or down." ;;
+    *) fail configuration "unknown command '${1:-}'; use mask, up or down." ;;
   esac
 }
 

guides/vpn-tailscale.md

@@ -5,8 +5,8 @@ a Gitea Actions runner, so the next steps of the same job (a deployment over
 SSH, a migration) can reach hosts on the tailnet. It does what the GitHub
 marketplace action `tailscale/github-action` does, written for Gitea Actions:
 that action is built for GitHub and has not been verified on Gitea. It is
-**experimental**: it lives on the branch `wireguard-vpn-action` until a trial
+**experimental**: it is on `main` but not released, and it has not yet run on a
-shows it is wanted. For a plain tunnel without a third-party service, see
+runner with a real tailnet (MIL-012). For a plain tunnel without a third-party service, see
 [WireGuard](vpn-wireguard.md).
 
 A node belongs to one job: every job that deploys joins on its own and logs out
@@ -23,7 +23,7 @@ jobs:
       - uses: actions/checkout@v4.2.2
 
       - name: Connect Tailscale
-        uses: TirSystem/github-action/actions/connect-tailscale@v2
+        uses: TirSystem/github-action/actions/connect-tailscale@main
         with:
           oauth-secret: ${{ secrets.TS_OAUTH_SECRET }}
           tags: tag:ci
@@ -35,16 +35,17 @@ jobs:
 
       - name: Disconnect Tailscale
         if: always()
-        uses: TirSystem/github-action/actions/connect-tailscale@v2
+        uses: TirSystem/github-action/actions/connect-tailscale@main
         with:
           action: down
 ```
 
 If the short form does not resolve on your instance (Gitea looks for the action
 on the host set in `DEFAULT_ACTIONS_URL`), use the full address:
-`https://git.tirsystem.com/TirSystem/github-action/actions/connect-tailscale@v2`.
+`https://git.tirsystem.com/TirSystem/github-action/actions/connect-tailscale@main`.
-To try the branch before it is released, replace `@v2` with
+
-`@wireguard-vpn-action`.
+Until release 2.2.0 the actions exist only on `main`, so the examples call
+`@main`; `@v2` does not contain them yet. After the release, use `@v2`.
 
 ## Inputs
 
@@ -55,7 +56,8 @@ To try the branch before it is released, replace `@v2` with
 | `oauth-secret` | | The secret of an OAuth client with the scope `auth_keys`, as a secret. Needs `tags` |
 | `tags` | | ACL tags of the node, `tag:ci` or `tag:ci,tag:deploy`. Required with `oauth-secret` |
 | `hostname` | runner name | Name of the node in the tailnet |
-| `version` | `latest` | The version to install when the runner has no `tailscale`. Pin it |
+| `version` | | The exact version to install when the runner has no `tailscale`, for example `1.80.2`. Empty uses the installed one and fails when there is none |
+| `sha256` | | The SHA-256 of the downloaded archive `tailscale_<version>_<arch>.tgz`; the install fails when it differs. Recommended |
 | `userspace-networking` | `false` | `true` runs without a TUN device |
 | `args` | | Extra flags for `tailscale up`, for example `--accept-routes`. Never a secret |
 | `timeout` | `60` | Seconds to wait for the node |
@@ -74,15 +76,23 @@ To try the branch before it is released, replace `@v2` with
    start with `GITEA_` or `GITHUB_`.
 4. Rotate the secret when a person with access leaves.
 
-The node is ephemeral and its state is kept in memory, so it disappears from the
+The state of the node is kept in memory. With an OAuth client the key is
-tailnet when the job ends, even if the last step does not run.
+ephemeral, so the node disappears from the tailnet when the job ends, even if
+the last step does not run. With an auth key that is not ephemeral, a cancelled
+job can leave the node in the tailnet until `down` runs or it is removed in the
+admin console: create the key as ephemeral.
 
 ## The runner
 
-- The job runs as root or can use `sudo`. The action downloads the Tailscale
+- The job runs as root, or `sudo` is available (the action uses `sudo` only
-  binaries from `pkgs.tailscale.com` when `tailscale` is not on the runner.
+  when the job is not root). The action downloads the Tailscale binaries from
-  Pin `version`; a runner image that already contains Tailscale skips the
+  `pkgs.tailscale.com` when `tailscale` is not on the runner; `version` is then
-  download.
+  required, and `sha256` makes the install check the archive. A runner image
+  that already contains Tailscale skips the download.
+- The action starts its own `tailscaled`. It refuses to run when a `tailscaled`
+  already answers on the runner, because that daemon belongs to the host and
+  `down` would log it out: use the action on job containers, not on a runner
+  that runs jobs directly on a host with its own Tailscale.
 - **TUN mode** (the default) needs the capability `NET_ADMIN` and the device
   `/dev/net/tun` in the runner container: in the `act_runner` configuration set
   under `container:` `options: "--cap-add=NET_ADMIN --device=/dev/net/tun"`.
@@ -107,7 +117,9 @@ tailnet when the job ends, even if the last step does not run.
   never on a command line, and deletes the file at once;
 - prints errors as `::error::[category] message`, with the categories
   `configuration`, `environment` and `connection`;
-- on `down` logs the node out, stops `tailscaled` and deletes its work folder.
+- waits until `tailscaled` answers before `tailscale up` (up to 30 seconds);
+- on `down` logs the node out, stops `tailscaled` and deletes its work folder,
+  and only when this action started that daemon.
 
 The `file:` form of `--auth-key` needs a recent Tailscale; with an old pinned
 version `tailscale up` reports an invalid key.

tests/test_connect_tailscale.py

@@ -36,6 +36,11 @@ case "$1" in
     done
     exit "${UP_EXIT:-0}" ;;
   ping) exit "${PING_EXIT:-0}" ;;
+  version)
+    # the daemon answers only after tailscaled was started, and after DAEMON_DELAY calls
+    [ -f "$STUB_DAEMON" ] || exit 1
+    n=$(( $(cat "$STUB_DAEMON") + 1 )); echo "$n" > "$STUB_DAEMON"
+    [ "$n" -gt "${DAEMON_DELAY:-0}" ] ;;
 esac
 """
 
@@ -52,13 +57,19 @@ def make_stub(path: Path, body: str) -> None:
 
 def run(tmp_path: Path, command: str, **env: str) -> subprocess.CompletedProcess:
     make_stub(tmp_path / "tailscale", TAILSCALE_STUB)
-    make_stub(tmp_path / "tailscaled", "#!/usr/bin/env bash\necho \"$*\" >> \"$STUB_LOG\"\n")
+    make_stub(
+        tmp_path / "tailscaled",
+        '#!/usr/bin/env bash\necho "$*" >> "$STUB_LOG"\necho 0 > "$STUB_DAEMON"\n',
+    )
     full = {
         "PATH": f"{tmp_path}:{os.environ['PATH']}",
         "TS_SUDO": "",
         "TS_STATE_DIR": str(tmp_path / "state"),
         "STUB_LOG": str(tmp_path / "log"),
         "STUB_KEY": str(tmp_path / "key"),
+        "STUB_DAEMON": str(tmp_path / "daemon"),
+        "TS_WAIT_INTERVAL": "0.05",
+        "TS_START_WAIT": "100",
         **env,
     }
     return subprocess.run(
@@ -77,7 +88,7 @@ class TestDefinition:
     def test_inputs_are_the_contract(self, document):
         assert set(document["inputs"]) == {
             "action", "authkey", "oauth-secret", "tags", "hostname", "version",
-            "userspace-networking", "args", "timeout", "health-check-host",
+            "sha256", "userspace-networking", "args", "timeout", "health-check-host",
         }
 
     def test_uses_gitea_contexts_only(self):
@@ -124,6 +135,8 @@ class TestUp:
             ({"TS_AUTHKEY": AUTHKEY, "TS_TAGS": "ci"}, "tags must look like"),
             ({"TS_AUTHKEY": AUTHKEY, "TS_TIMEOUT": "soon"}, "timeout"),
             ({"TS_AUTHKEY": AUTHKEY, "TS_VERSION": "1.x"}, "version"),
+            ({"TS_AUTHKEY": AUTHKEY, "TS_VERSION": "latest"}, "version"),
+            ({"TS_AUTHKEY": AUTHKEY, "TS_SHA256": "abc"}, "sha256"),
             ({"TS_AUTHKEY": AUTHKEY, "TS_USERSPACE": "yes"}, "userspace-networking"),
             ({"TS_AUTHKEY": AUTHKEY, "TS_HOSTNAME": "bad name"}, "hostname"),
         ],
@@ -148,6 +161,40 @@ class TestUp:
         assert "health-check-host" in result.stdout
 
 
+class TestDaemon:
+    def test_waits_until_the_daemon_answers(self, tmp_path):
+        result = run(tmp_path, "up", TS_AUTHKEY=AUTHKEY, DAEMON_DELAY="5")
+        assert result.returncode == 0, result.stdout
+        assert "up --auth-key" in log(tmp_path)
+
+    def test_a_daemon_that_never_answers_fails_before_up(self, tmp_path):
+        result = run(tmp_path, "up", TS_AUTHKEY=AUTHKEY, DAEMON_DELAY="1000", TS_START_WAIT="3")
+        assert result.returncode == 1
+        assert "::error::[connection] tailscaled did not answer" in result.stdout
+        assert "up --auth-key" not in log(tmp_path)
+
+    def test_a_daemon_of_the_host_is_not_taken_over(self, tmp_path):
+        (tmp_path / "daemon").write_text("0", encoding="utf-8")
+        result = run(tmp_path, "up", TS_AUTHKEY=AUTHKEY)
+        assert result.returncode == 1
+        assert "::error::[environment] a tailscaled already runs" in result.stdout
+        calls = log(tmp_path)  # only the probe ran: no second daemon, no up
+        assert "--state=mem:" not in calls
+        assert "up --auth-key" not in calls
+
+    def test_down_leaves_a_daemon_it_did_not_start(self, tmp_path):
+        (tmp_path / "daemon").write_text("0", encoding="utf-8")
+        result = run(tmp_path, "down")
+        assert result.returncode == 0
+        assert "nothing to do" in result.stdout
+        assert not (tmp_path / "log").exists()
+
+    def test_without_tailscale_a_version_is_required(self, tmp_path):
+        result = run(tmp_path, "up", TS_AUTHKEY=AUTHKEY, TS_TAILSCALE="no-such-tailscale")
+        assert result.returncode == 1
+        assert "::error::[configuration] tailscale is not on the runner; set version" in result.stdout
+
+
 class TestDownAndMask:
     def test_down_logs_out_and_removes_the_state(self, tmp_path):
         run(tmp_path, "up", TS_AUTHKEY=AUTHKEY)