diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..7927ff3 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,142 @@ +# Changelog + +## Unreleased + +### Added + +- **`close` keeps a link another session still holds.** One master and one tunnel serve + every process on the machine that uses the profile, and `close` tore both down + unconditionally — so two sessions sharing the link (an agent and a wrapper script, two + agents) each cut the other's connection with its own close, and under 2FA the reopen + cost a code. `open`, and the implicit open under `run`, `push`, `pull` and `local`, now + record a holder; `close` removes its own and tears down only when no live holder remains. + A holder is the calling process by default, or `HS_LEASE=` for a session whose + commands each run in a fresh shell; it lapses after `HS_LEASE_TTL` seconds idle (default + 14400); `HS_CLOSE_FORCE=1` closes regardless and clears them. `status` lists the holders. + +### Fixed + +- **A seed read that fails now says why.** The backend's message used to go to + `/dev/null`, so `open`, `doctor` and `status` said "no seed — run store-seed" even when + the seed was stored and the keychain had only refused one request. They now repeat the + backend's own reason. Every failed read is appended to `HS_TOTP_ERROR_LOG` (default + `$HS_CONFIG_DIR/.totp-errors.log`) with its exit status and, for the keychain, + the macOS session it ran in. Only the error is written; the seed never is. +- **A failed read is tried once more** after `HS_TOTP_READ_RETRY_DELAY` seconds (default + 2), which absorbs a refusal that clears by itself. Not after a cancelled prompt, which + would ask again someone who just said no, and not for a missing seed file. + +## 0.1.0 — 2026-08-21 + +First tagged release. The repository has been public since late July 2026 and there was +no tag before this one, so anyone who cloned in between holds a copy with the defects +listed under **Fixed** — several of which are silent. Upgrading is worth it. + +Two audits, in July and August, produced the work below. Every fix carries a test, and +each test was checked against the specific defect it guards rather than only against the +previous state of `main`. The suite went from 37 assertions to 194. + +### Changed — read before upgrading + +- **`submit`'s extra arguments are quoted** and no longer re-parsed by the remote shell. + `README` always promised they reached `sbatch` unchanged; they now do. An argument that + relied on remote expansion (a `$VAR`, a backtick) will stop expanding. +- **A remote script path given to `submit` is literal.** `HS_REMOTE_WORKDIR` is still + expanded by the cluster, deliberately — it is where a profile's `$USER` has to resolve. +- **`watch` clamps its interval** to `HS_WATCH_MIN_INTERVAL` (30 s) and stops at + `HS_WATCH_MAX_SECONDS` (one hour), saying so in both cases. +- **`watch` has a third exit status.** `0` finished, `1` unknown, **`3` stopped at the cap + with the job still queued**. Only `0` means the job is done. +- **`queue` prints the whole job id.** The column is ragged now; it used to be truncated, + which turned the array task `12345678_10` into `12345678_1` — a valid id for a + different task. +- **`watch`, `fetch`, `cancel` and `submit` reject anything that is not a job id**, before + it reaches a remote command line. +- **The no-argument subcommands reject trailing arguments.** `open -p bigiron` used to + open the *default* cluster while appearing to select a profile, and spend a code on it. +- **`local` no longer claims to help `sshfs`**, which reads neither `RSYNC_RSH` nor + `GIT_SSH_COMMAND`. Use `-o ssh_command="ssh $(hpc-session ssh-opts)"`. +- **A VPN configured with `HS_VPN_STATUS_CMD` alone is recognised**, which is the shape + `docs/vpn-hooks.md` recommends for a tunnel you raise yourself. It previously reported + as "not configured". +- **The tool's own remote commands run under `sh`**, whatever login shell the account + uses. `hpc-session run` is untouched: that is your command line. + +### Added + +- **CI.** The suite runs on Linux and on macOS's `/bin/bash` 3.2 — the real floor this + tool is written to — plus `shellcheck`. +- **`hpc-session templates`**, and `render ` resolved against `HS_TEMPLATE_DIR`, so + a skill that owns a job type can own its job shape without hardcoding a path into this + repository. `SKILL.md` states the contract across that seam. +- `HS_WATCH_INTERVAL`, `HS_WATCH_MIN_INTERVAL`, `HS_WATCH_MAX_SECONDS`, + `HS_WATCH_MAX_MISSES`, `HS_TEMPLATE_DIR`. +- `SLURM_NTASKS` as a template placeholder. It was hard-coded to 1 while `--nodes` was a + placeholder, so any `HS_SLURM_NODES` above 1 allocated nodes that then sat idle. +- **`hpc-session version`**, also reported by `doctor`. It answers before the profile + loads, so a broken setup can still say which copy it is; a test holds it equal to the + newest heading in this file. +- A second worked example profile, for the ordinary key-only cluster. + +### Fixed + +- **`watch` reported a running job as finished** whenever a remote query failed with + anything other than ssh's own 255 — an invalid job id, a `squeue` behind a module, a + restarting controller, a `csh` login shell. The output was identical to a real + completion. It now requires positive evidence that the controller was reached. +- **Configuration precedence was inverted.** The profile was sourced last, so every + documented one-off override (`HS_HOST=other hpc-session status`) was silently discarded. +- **The shipped `HS_REMOTE_WORKDIR` broke `submit` on every OpenSSH ≥ 9.0 client**, which + speaks SFTP and runs no remote shell to expand `$USER`. +- **`fetch` could deliver an unrelated file** from the remote home: `ls -1` on a matching + *directory* prints its contents as bare relative names. It also skipped a workdir whose + last component was a symlink, reporting "nothing matching" — which reads as "the job + wrote nothing". +- **`fetch` returned 0 on a partial retrieval.** Its status was the last copy's. +- **`init` could not create a profile named with `-p`**, the only selector `README` + documents, and the error's own suggested remedy failed when `HS_PROFILE` was exported. +- **The `command` TOTP backend could not open a session at all** — it was asked for a + stored seed it has none of by design. +- **A bad key under `AuthenticationMethods publickey,keyboard-interactive`** was retried + three times over ~90 s instead of failing at once. +- **A local code-generation failure waited out three full time steps** for something that + could never change, judging a stale error string. +- **A failed control-directory `mkdir` was swallowed**, so the session opened and simply + never multiplexed — which looks like a slow cluster, not a broken setup. +- **A mistyped `HS_TOTP_ALGO` or `HS_TOTP_PERIOD` was reported as an invalid seed**, + sending the user to re-enrol over a typo. +- **`--help` printed executable code**, having sliced a line range that had drifted. +- `sbatch`'s federated message form made `submit` return the **cluster name** as the job + id; `--parsable` made it fail *after* the job was queued. +- `config.example` omitted `HS_CONTROL_DIR`, `HS_CONFIG_DIR` and `HS_OTP`. A test now + holds every default to appearing there. +- Four documentation claims the code did not honour, including two in `SECURITY.md`. + +### Security + +- **The TOTP seed no longer passes through `argv`** on the keychain path, where `ps` and + any exec-logging agent could see it. It travels on stdin. +- **A live code no longer survives a signal.** A trap removes the temporary file on + `EXIT`, `INT`, `TERM` and `HUP`; previously a Ctrl-C between writing it and `ssh` + reading it left a valid second factor on disk with nothing left to delete it. +- **The keychain identifiers are refused if they contain a quote, a backslash or a + newline.** `security -i` re-tokenises the storing command and runs one command per line, + so either could inject options — or a whole second command — into something running + against the user's keychain. +- **The `file` backend enforces mode 0600** on an existing file. `umask` governs creation + only, so a seed written into a file that was already 0644 stayed world-readable. +- `SECURITY.md` now records what `-T /usr/bin/security` costs: any process running as you + can read the seed back without a prompt, which is what makes unattended `open` work. + +### Known limitations + +- `README` states OpenSSH 6.7+, which is correct for multiplexing. The unattended TOTP + path additionally needs `SSH_ASKPASS_REQUIRE`, which is newer + ([#13](https://github.com/HolobiomicsLab/hpc-session/issues/13)). A caller with a + terminal is unaffected. +- Multi-cluster (federated) submission is out of scope. `submit` says so when `sbatch` + reports another cluster. +- The test suite is entirely offline, against stubs. Separately, one end-to-end run was + made against a live SLURM controller (a TOTP-gated cluster behind a VPN): `open`, + `render`, `submit`, `queue`, `watch` to completion, `fetch`, `cancel`, `close`, all as + documented. That is one site and one SLURM version, not a compatibility claim. diff --git a/README.md b/README.md index 838110f..dda93af 100644 --- a/README.md +++ b/README.md @@ -55,12 +55,21 @@ the same connection, hand it `$(hpc-session ssh-opts)`. hpc-session open # VPN up + one authenticated master hpc-session run squeue -u \$USER # runs ON the cluster hpc-session run -- sacct -j 12345 -hpc-session close # drop master + VPN, freeing the link +hpc-session close # drop master + VPN once no one else holds them ``` `open` … `run` … `run` … `close` is the pattern that matters. Everything between the first and last call is free. +Two sessions on one machine — an agent and a wrapper script, two agents — share one +master and one tunnel. `open` records a *holder* for the caller, and `close` tears the +link down only when no live holder remains, so one session's `close` no longer cuts the +other's connection. The holder is the calling process by default; a session that runs +each command in a fresh shell should export `HS_LEASE=` +so its holder outlives the shell. `status` lists the holders, a holder idle for +`HS_LEASE_TTL` seconds (default 4 h) lapses, and `HS_CLOSE_FORCE=1 hpc-session close` +tears down regardless. + ### Running things, in the right place The distinction that bites people: @@ -285,8 +294,9 @@ in either skill. ## Contributing Issues and pull requests are welcome, particularly worked profiles for other clusters -(as `examples/.conf`, with placeholders instead of real logins) and VPN hooks for -clients not yet covered. Please keep `tests/run_tests.sh` green and add a case for any +(as `examples/.conf`, with placeholders instead of real logins — see +[`examples/`](examples/) for the two shapes already there) and VPN hooks for clients not +yet covered. Please keep `tests/run_tests.sh` green and add a case for any behaviour you change — CI runs it on Linux and on macOS's bash 3.2, plus `shellcheck`. ## Authors @@ -294,6 +304,12 @@ behaviour you change — CI runs it on Linux and on macOS's bash 3.2, plus `shel Developed at the [HolobiomicsLab](https://github.com/HolobiomicsLab) — CNRS and Université Côte d'Azur. +## Changelog + +[CHANGELOG.md](CHANGELOG.md). Behaviour changes worth knowing before upgrading are listed +first in each release. `hpc-session version` says which copy you have, and `doctor` opens +with the same line — quote it when reporting anything. + ## License MIT — see [LICENSE](LICENSE). Copyright CNRS and Université Côte d'Azur. diff --git a/SECURITY.md b/SECURITY.md index 108e033..9c4bf27 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -28,6 +28,10 @@ TOTP seed if you configure a backend that stores one. code (in which case nothing is stored here). - **The seed never appears in `argv` or the environment.** It is piped into the code generator on stdin, so `ps` cannot see it. +- **A failed seed read is logged; the seed never is.** The backend's error message, its + exit status and the time go to `HS_TOTP_ERROR_LOG`, created mode 0600 beside your + profiles. A successful read hands the seed on with a shell builtin, so it still reaches + no `argv`, no environment and no file. - **The generated code** is written to a `mktemp` file (mode 0600) and read exactly once: the askpass helper prints it and deletes it. That single-shot behaviour is deliberate — a helper that kept answering would let `ssh` retry a stale code in a loop. diff --git a/SKILL.md b/SKILL.md index eda7ae6..68a4e24 100644 --- a/SKILL.md +++ b/SKILL.md @@ -20,10 +20,13 @@ of `hpc-session`; see [README.md](README.md) for the full reference and ## Before anything else ```bash +hpc-session version # which release this is — behaviour differs between them hpc-session doctor # no network — reports what is configured and what is missing hpc-session status # is the master up? the VPN? is a TOTP seed present? ``` +`version` answers even when the profile is missing or broken, so it is safe to ask first. + If no profile exists, run `hpc-session init` — or `hpc-session -p init` for a second cluster — and tell the user which keys they must fill in (`HS_HOST` at minimum). Do not invent a hostname, account or partition — ask. @@ -142,6 +145,12 @@ never has to appear in your instructions. Then: - **open once, close once.** Everything in between is free; a connect-per-step loop costs a code each time. +- **name your holder when you share the machine.** Another agent or a wrapper script may + be on the same master and tunnel; `close` tears them down only when no live holder + remains. Your holder is the calling process by default, which for an agent is a shell + that ends with the command — so export `HS_LEASE=` on + every call (a session id does), and close with the same name. `status` lists the + holders; `HS_CLOSE_FORCE=1 hpc-session close` is for a holder you know is dead. - **capture the job id from `submit`'s stdout** — `job=$(hpc-session submit job.slurm)` — and pass that value on. Do not re-derive it from a log. - **read `watch`'s exit status** (0 finished, 1 unknown, 3 still queued). Only 0 licenses diff --git a/bin/hpc-session b/bin/hpc-session index 64ba3b3..97cc5a4 100755 --- a/bin/hpc-session +++ b/bin/hpc-session @@ -15,6 +15,7 @@ # hpc-session watch 12345 # poll until it leaves the queue # hpc-session fetch 12345 ./results # bring the job's files back # hpc-session close # drop master + VPN, freeing the link +# hpc-session version # which release this copy is # # Setup: hpc-session init [profile] -> edit the profile -> hpc-session doctor # Docs: README.md, docs/2fa-enrollment.md, docs/vpn-hooks.md @@ -82,6 +83,7 @@ hs_doctor_vpn() { # Everything checkable without touching the network — safe to run before a first login. hs_doctor() { + echo "hpc-session $HS_VERSION" echo "profile $HS_PROFILE ($(hs_profile_path "$HS_PROFILE"))" [ -n "$HS_HOST" ] && hs_report host ok "$HS_HOST" || hs_report host fail "HS_HOST is unset" hs_doctor_tool ssh "the whole tool depends on it" @@ -122,6 +124,15 @@ if [ "${1:-}" = init ]; then exit fi +# Dispatched before the loader, like `init`: a missing or broken profile is exactly when +# someone needs to say which copy of the tool they are running, and the answer does not +# depend on a profile. CHANGELOG.md is the other half of this string; a test holds them +# together so a tag cannot drift from what the tool reports. +HS_VERSION="0.1.0" +case "${1:-}" in + version|--version) printf 'hpc-session %s\n' "$HS_VERSION"; exit 0 ;; +esac + hs_load_profile case "${1:-}" in diff --git a/config.example b/config.example index f2717ff..912012b 100644 --- a/config.example +++ b/config.example @@ -44,6 +44,16 @@ HS_VPN_UP_CMD="" HS_VPN_DOWN_CMD="" HS_VPN_STATUS_CMD="" +# --- sharing the link between sessions ---------------------------------------------- +# One master and one tunnel serve every process that uses this profile. `open` records a +# holder for the caller; `close` tears the link down only when no live holder remains. +# The holder is the calling process by default. A session that runs each command in a +# fresh shell (an agent) should set HS_LEASE to a name stable for that session, usually +# from the environment: HS_LEASE="${CLAUDE_CODE_SESSION_ID:-}" or "${TMUX_PANE:-}". +HS_LEASE="" +HS_LEASE_TTL="14400" # a holder idle this many seconds lapses (4 h); every open refreshes it +HS_CLOSE_FORCE="0" # 1 = close tears down whoever holds the link, and clears the holders + # --- two-factor authentication (optional) ------------------------------------------ # none — key-only login (the common case) # keychain — macOS login Keychain (see HS_TOTP_SERVICE / HS_TOTP_ACCOUNT) @@ -67,6 +77,12 @@ HS_TOTP_DIGITS="6" HS_TOTP_PERIOD="30" HS_TOTP_ALGO="sha1" +# A seed read that fails is tried once more after this many seconds, and the backend's +# own message — never the seed — is appended to the log. Empty means the default, +# $HS_CONFIG_DIR/.totp-errors.log. open, doctor and status repeat the latest one. +HS_TOTP_READ_RETRY_DELAY="2" +HS_TOTP_ERROR_LOG="" + # --- connection tuning -------------------------------------------------------------- HS_CONTROL_PERSIST="8h" # how long an idle master survives HS_CONNECT_TIMEOUT="25" # seconds diff --git a/docs/2fa-enrollment.md b/docs/2fa-enrollment.md index d745753..79be2df 100644 --- a/docs/2fa-enrollment.md +++ b/docs/2fa-enrollment.md @@ -106,6 +106,7 @@ stall for up to 30 seconds waiting for a fresh step. |---|---|---| | `auth failed … waiting Ns for a fresh code` | the previous code was already consumed | it retries by itself; wait one step | | `could not open the master`, repeatedly | VPN down, or wrong seed | `hpc-session status`; compare `hpc-session code` with the phone | +| `the seed in 'keychain' could not be read: …` | the keychain refused this read, often because it wanted to show a prompt and the command ran outside your logged-in session | the message names the reason; `HS_TOTP_ERROR_LOG` keeps every failed read with the session it ran in. Run `doctor` from your own terminal to compare | | Commands hang instead of failing | stale socket after the tunnel dropped | `hpc-session close` then `open` (it also cleans up on its own) | | Every code rejected | clock skew — TOTP is time-based | check the machine's clock is NTP-synced | | Codes rejected only sometimes | your site may not use the default profile | set `HS_TOTP_DIGITS` / `HS_TOTP_PERIOD` / `HS_TOTP_ALGO` | diff --git a/examples/keyonly-direct.conf b/examples/keyonly-direct.conf new file mode 100644 index 0000000..c27bc49 --- /dev/null +++ b/examples/keyonly-direct.conf @@ -0,0 +1,37 @@ +# Example profile: the ordinary case — a cluster you reach directly, with an SSH key and +# no second factor. Start here; the other example adds a VPN and TOTP on top. +# +# Copy to ~/.config/hpc-session/.conf, replace every PLACEHOLDER, then: +# hpc-session -p doctor +# +# Contains no credentials, and should never contain any. + +# Requires a matching block in ~/.ssh/config, which is where your key, user and any jump +# host belong — every other tool can see them there too: +# Host mycluster +# HostName login.cluster.example.edu +# User YOUR_CLUSTER_LOGIN +HS_HOST="mycluster" + +# Job scripts are copied here and submitted from here. Escape $USER so the REMOTE shell +# expands it — since OpenSSH 9.0 scp speaks SFTP and runs no remote shell, so an +# unexpanded one would reach the copy verbatim. Prefer scratch over your home directory. +HS_REMOTE_WORKDIR="/scratch/\$USER/jobs" + +# Defaults for `hpc-session render`. Leave a value empty and its #SBATCH line is dropped, +# so an unset account does not become an `--account=` that SLURM rejects. +HS_SLURM_ACCOUNT="" +HS_SLURM_PARTITION="YOUR_PARTITION" +HS_SLURM_TIME="02:00:00" +HS_SLURM_CPUS="4" +HS_SLURM_MEM="16G" + +# No VPN and no second factor: leave every hook empty and the tool behaves as if neither +# existed. `open` is then a plain key login, and `doctor` needs no python3. +HS_VPN_UP_CMD="" +HS_VPN_STATUS_CMD="" +HS_TOTP_BACKEND="none" + +# Nothing here monopolises your link, so a long-lived master is pure win: one login covers +# a working day. Shorten it if your site drops idle connections. +HS_CONTROL_PERSIST="8h" diff --git a/lib/config.sh b/lib/config.sh index 959e919..6e8d41e 100644 --- a/lib/config.sh +++ b/lib/config.sh @@ -42,6 +42,10 @@ hs_apply_defaults() { : "${HS_HOST:=}" : "${HS_CONTROL_DIR:=$HOME/.ssh/hpc-session}" : "${HS_CONTROL_PERSIST:=8h}" + # Who holds the shared link; see "who holds the link" in lib/session.sh. + : "${HS_LEASE:=}" + : "${HS_LEASE_TTL:=14400}" + : "${HS_CLOSE_FORCE:=0}" : "${HS_CONNECT_TIMEOUT:=25}" : "${HS_AUTH_ATTEMPTS:=3}" : "${HS_VPN_UP_CMD:=}" @@ -56,6 +60,10 @@ hs_apply_defaults() { : "${HS_TOTP_DIGITS:=6}" : "${HS_TOTP_PERIOD:=30}" : "${HS_TOTP_ALGO:=sha1}" + # A failed seed read is tried once more after this many seconds, and the backend's own + # message — never the seed — goes to this log. See hs_seed_read. + : "${HS_TOTP_READ_RETRY_DELAY:=2}" + : "${HS_TOTP_ERROR_LOG:=$HS_CONFIG_DIR/$HS_PROFILE.totp-errors.log}" : "${HS_REMOTE_WORKDIR:=.}" # Where `render ` looks. Point it at another skill's assets and that skill owns its # own job shapes without owning any paths — see "Composing with other skills" in SKILL.md. diff --git a/lib/session.sh b/lib/session.sh index d6e7ae7..9f0d060 100644 --- a/lib/session.sh +++ b/lib/session.sh @@ -200,6 +200,68 @@ hs_open_with_retries() { hs_die "could not open the master. Check: VPN up? seed correct ('hpc-session code')? enrolled?" } +# --- who holds the link --------------------------------------------------------------- +# +# One master and one tunnel serve every process on the machine that uses the profile, and +# `close` used to tear both down unconditionally. Two sessions sharing the link — two +# agents, a wrapper script and a shell — therefore took each other's connection away: the +# other side's next command failed on a dead socket, or on a tunnel that was no longer up, +# and under 2FA the reopen cost a code it could not always produce. A holder file per +# opener fixes that: `open` (and the implicit open under run, push, pull and local) records +# who holds the link, `close` removes its own record and tears the link down only when no +# live holder remains. `status` lists them. +# +# A holder is named by HS_LEASE when set, otherwise by the pid of the process that invoked +# the tool — a wrapper script, an interactive shell. The file records that pid and the +# time. A pid holder is live while its process is; a named holder is live until it closes, +# because its pid is usually a transient shell (an agent runs each command in a new one). +# Both expire HS_LEASE_TTL seconds after their last open — the backstop against a session +# that ended without closing — and every open refreshes the stamp, so the expiry is an idle +# time, not a lifetime. HS_CLOSE_FORCE=1 tears down regardless and clears every holder. +hs_lease_name() { + local name="${HS_LEASE:-$PPID}" + case "$name" in + ""|*[!A-Za-z0-9._-]*) hs_die "HS_LEASE must be made of letters, digits, '.', '_' and '-' (got '$name')" ;; + esac + printf '%s' "$name" +} + +hs_pid_alive() { [ "${1:-0}" -gt 1 ] 2>/dev/null && kill -0 "$1" 2>/dev/null; } + +hs_lease_take() { + local dir="$HS_CONTROL_DIR/holders" name named=no + name=$(hs_lease_name) || exit 1 + [ -n "${HS_LEASE:-}" ] && named=yes + mkdir -p "$dir" || hs_die "cannot create $dir" + printf 'pid=%s\nnamed=%s\nsince=%s\n' "$PPID" "$named" "$(date +%s)" > "$dir/$name" \ + || hs_die "cannot record the holder $dir/$name" +} + +hs_lease_drop() { + local name + name=$(hs_lease_name) || exit 1 + rm -f "$HS_CONTROL_DIR/holders/$name" +} + +# Print the live holders, one per line, and remove the rest. +hs_lease_live() { + local dir="$HS_CONTROL_DIR/holders" f key value pid named since now + [ -d "$dir" ] || return 0 + now=$(date +%s) + for f in "$dir"/*; do + [ -f "$f" ] || continue + pid='' named='' since='' + while IFS='=' read -r key value; do + case "$key" in pid) pid=$value ;; named) named=$value ;; since) since=$value ;; esac + done < "$f" + if [ $((now - ${since:-0})) -lt "$HS_LEASE_TTL" ] && { [ "$named" = yes ] || hs_pid_alive "$pid"; }; then + printf '%s\n' "${f##*/}" + else + rm -f "$f" + fi + done +} + hs_open_session() { hs_require_host # Not swallowed: `set -uo pipefail` has no -e, so a failure here used to let the open @@ -207,7 +269,7 @@ hs_open_session() { mkdir -p "$HS_CONTROL_DIR" && chmod 700 "$HS_CONTROL_DIR" \ || hs_die "cannot create the control directory $HS_CONTROL_DIR — without it nothing multiplexes" hs_clean_stale - hs_master_up && { hs_note "master already up"; return 0; } + hs_master_up && { hs_lease_take; hs_note "master already up — joined it as holder $(hs_lease_name)"; return 0; } # Verify a code can be PRODUCED before raising the tunnel — not that it will be # accepted, which cannot be tested from here: under a full tunnel the login node is # usually unreachable until the tunnel is up. Catching the common failure early is still @@ -217,11 +279,19 @@ hs_open_session() { hs_die "cannot produce a TOTP code: $(hs_seed_hint)" fi hs_vpn_connect - hs_open_with_retries + hs_open_with_retries && hs_lease_take } hs_close_session() { hs_require_host + local others + hs_lease_drop + others=$(hs_lease_live | tr '\n' ' ') + if [ -n "$others" ] && [ "$HS_CLOSE_FORCE" != 1 ]; then + hs_note "link kept — still held by: ${others}(HS_CLOSE_FORCE=1 closes it anyway)" + return 0 + fi + [ "$HS_CLOSE_FORCE" = 1 ] && rm -rf "$HS_CONTROL_DIR/holders" hs_ssh -O exit "$HS_HOST" >/dev/null 2>&1 && hs_note "master closed" || hs_note "no master to close" if hs_uses_vpn && hs_vpn_up && [ -n "$HS_VPN_DOWN_CMD" ]; then eval "$HS_VPN_DOWN_CMD" >/dev/null 2>&1 && hs_note "VPN disconnected (link freed)" @@ -229,7 +299,11 @@ hs_close_session() { return 0 } -hs_ensure_open() { hs_master_up || hs_open_session || exit 1; } +# An implicit open joins the link as a holder too: a session that only ever runs commands +# over a master someone else raised is still a user of it. +hs_ensure_open() { + if hs_master_up; then hs_lease_take; else hs_open_session || exit 1; fi +} # Run a command ON THE CLUSTER. BatchMode means a dead master fails fast instead of # hanging on a prompt nothing can answer. @@ -288,7 +362,10 @@ hs_pull() { } hs_status() { + local holders hs_master_up && echo "master: UP" || echo "master: down" + holders=$(hs_lease_live | tr '\n' ' ') + echo "holders: ${holders:-none}" if hs_uses_vpn; then hs_vpn_up && echo "vpn: connected" || echo "vpn: disconnected" else diff --git a/lib/totp.sh b/lib/totp.sh index 57123a8..d010a6b 100644 --- a/lib/totp.sh +++ b/lib/totp.sh @@ -39,16 +39,84 @@ print(str(truncated % (10 ** digits)).zfill(digits)) # Seconds remaining in the current TOTP step. hs_step_left() { echo $(( HS_TOTP_PERIOD - ($(date +%s) % HS_TOTP_PERIOD) )); } -# Print the stored base32 seed. Fails if the backend holds none. -hs_seed_read() { +# One read from the backend, with nothing hidden: the seed on stdout, the backend's own +# complaint, if any, on stderr. +hs_seed_fetch() { case "$HS_TOTP_BACKEND" in - keychain) security find-generic-password -s "$HS_TOTP_SERVICE" -a "$HS_TOTP_ACCOUNT" -w 2>/dev/null ;; - pass) pass show "$HS_TOTP_PASS_ENTRY" 2>/dev/null | head -1 ;; - file) [ -f "$HS_TOTP_FILE" ] && head -1 "$HS_TOTP_FILE" ;; - *) return 1 ;; + keychain) security find-generic-password -s "$HS_TOTP_SERVICE" -a "$HS_TOTP_ACCOUNT" -w ;; + pass) pass show "$HS_TOTP_PASS_ENTRY" | head -1 ;; + file) [ -f "$HS_TOTP_FILE" ] || { echo "no file at $HS_TOTP_FILE" >&2; return 1; } + head -1 "$HS_TOTP_FILE" ;; + *) echo "backend '$HS_TOTP_BACKEND' stores no seed" >&2; return 1 ;; esac } +# Print the stored base32 seed. Fails if the backend holds none, or will not hand it over. +# +# A failed read used to vanish: the backend's stderr went to /dev/null, and every caller then +# said "no seed — run store-seed", even when the seed was stored and the keychain had merely +# refused this one request. That pointed the user at re-enrolling a second factor that was +# fine, and left a refusal that came and went with nothing to diagnose it from. Now the +# backend's message is appended to HS_TOTP_ERROR_LOG, and the read is tried once more after +# HS_TOTP_READ_RETRY_DELAY seconds, which absorbs a refusal that clears by itself. +# +# Only stderr is ever written down. stdout IS the seed: it is held in a local variable and +# handed on with `printf`, a builtin, so it still reaches no argv, no environment and no file. +# +# No second attempt after a cancelled prompt, which would ask a user who just said no again, +# nor for a file that does not exist, which a few seconds will not create. +hs_seed_read() { + local attempt seed rc err_file delay="${HS_TOTP_READ_RETRY_DELAY:-2}" + case "$delay" in ''|*[!0-9]*) delay=2 ;; esac + for attempt in 1 2; do + err_file=$(mktemp "${TMPDIR:-/tmp}/hsseed.XXXXXX") || return 1 + seed=$(hs_seed_fetch 2>"$err_file"); rc=$? + if [ "$rc" -eq 0 ] && [ -n "$seed" ]; then + rm -f "$err_file" "$(hs_seed_error_log).last" + printf '%s\n' "$seed" + return 0 + fi + seed="" + hs_seed_error_record "$attempt" "$rc" "$err_file" + rm -f "$err_file" + [ "$attempt" = 1 ] || return 1 + case "$HS_TOTP_BACKEND:$HS_SEED_LAST_ERROR" in + file:*|*[Cc]ancel*) return 1 ;; + esac + sleep "$delay" + done + return 1 +} + +hs_seed_error_log() { + echo "${HS_TOTP_ERROR_LOG:-${HS_CONFIG_DIR:-$HOME/.config/hpc-session}/${HS_PROFILE:-default}.totp-errors.log}" +} + +# Append one line per failed read: when, which backend, which attempt, its exit status, the +# macOS session the read ran in, and what the backend said. The latest message is also kept +# beside the log, so the hint printed by open, doctor and status can repeat it; a read that +# succeeds removes it. +# +# The session matters for the keychain. `security` started outside the logged-in GUI +# session cannot show a prompt, so a keychain that would ask the user refuses instead — one +# way a read can work from a terminal and fail from an agent at the same moment. +hs_seed_error_record() { # attempt, exit status, file holding the backend's stderr + local why log session="" + why=$(tr '\n' ' ' < "$3" | sed 's/ */ /g; s/ $//' | cut -c1-300) + [ -n "$why" ] || why="the backend printed nothing and gave no reason" + HS_SEED_LAST_ERROR="$why" + if [ "$HS_TOTP_BACKEND" = keychain ] && command -v launchctl >/dev/null 2>&1; then + session=" session=$(launchctl managername 2>/dev/null || echo unknown)" + fi + log=$(hs_seed_error_log) + ( umask 077 + mkdir -p "$(dirname "$log")" \ + && printf '%s %s attempt=%s exit=%s%s %s\n' "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ + "$HS_TOTP_BACKEND" "$1" "$2" "$session" "$why" >> "$log" \ + && printf '%s\n' "$why" > "$log.last" ) 2>/dev/null + return 0 +} + # Can this profile produce a code at all? Asked before the VPN goes up, so a failure is # cheap rather than stranding the link. # @@ -68,9 +136,17 @@ hs_have_seed() { # step of setup: telling a `command` user to run `store-seed` — for a backend that stores # nothing, and whose store-seed exits 1 saying exactly that — left them with no way forward # until they reached `open`, which is the one place the right message used to live. +# +# When the last read left a reason behind, that reason leads. "No seed" is only one of the +# ways a read fails; the keychain refusing this one request is another, and re-enrolling +# does nothing for it. hs_seed_hint() { + local last log + log=$(hs_seed_error_log) if [ "$HS_TOTP_BACKEND" = command ]; then echo "HS_TOTP_CMD is empty — set it to a command that prints one code" + elif last=$(cat "$log.last" 2>/dev/null) && [ -n "$last" ]; then + echo "the seed in '$HS_TOTP_BACKEND' could not be read: $last (history in $log; if no seed was ever stored, run: hpc-session store-seed)" else echo "no seed in '$HS_TOTP_BACKEND' and no HS_OTP — run: hpc-session store-seed" fi diff --git a/tests/run_tests.sh b/tests/run_tests.sh index 0c42484..788c76b 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -686,6 +686,61 @@ poison_probe "o'brien" me >/dev/null 2>&1 check "an apostrophe is accepted" 0 "$(status_of test -s "$keychain_log")" rm -rf "$keychain_bin" +# A failed seed read must say why, and get a second chance. It used to be silent: stderr +# went to /dev/null, and open, doctor and status all said "no seed — run store-seed" while +# the seed sat in the keychain, refused for one request only. `security` is SHADOWED here +# for the same reason as above: nothing in this block may reach a real keychain. +read_bin=$(mktemp -d "${TMPDIR:-/tmp}/hstest.XXXXXX") +read_log="$read_bin/errors.log" +cat > "$read_bin/security" </dev/null || echo 0); n=\$((n + 1)); echo "\$n" > "$read_bin/count" +case "\$FAKE_SECURITY" in + flaky) [ "\$n" -ge 2 ] && { echo GEZDGNBVGY3TQOJQ; exit 0; } ;; + cancel) echo "security: SecKeychainSearchCopyNext: User canceled the operation." >&2; exit 128 ;; +esac +echo "security: SecKeychainSearchCopyNext: User interaction is not allowed." >&2 +exit 36 +STUB +chmod +x "$read_bin/security" +seed_probe() { # stub mode; prints whatever the read handed over + rm -f "$read_bin/count" "$read_log" "$read_log.last" + ( PATH="$read_bin:$PATH"; export FAKE_SECURITY="$1" + HS_TOTP_BACKEND=keychain HS_TOTP_SERVICE=svc HS_TOTP_ACCOUNT=me \ + HS_TOTP_ERROR_LOG="$read_log" HS_TOTP_READ_RETRY_DELAY=0 hs_seed_read ) 2>/dev/null +} +seed=$(seed_probe flaky); rc=$? +check "a refusal that clears is absorbed" "0|GEZDGNBVGY3TQOJQ" "$rc|$seed" +check "and is recorded once" 1 "$(grep -c . "$read_log")" +check_contains "with the keychain's own reason" "User interaction is not allowed" "$(cat "$read_log")" +check_contains "and its exit status" "exit=36" "$(cat "$read_log")" +check "the seed itself is never written down" 1 "$(status_of grep -q GEZDGNBVGY3TQOJQ "$read_log")" +check "a good read clears the latest reason" 1 "$(status_of test -e "$read_log.last")" +seed=$(seed_probe refuse); rc=$? +check "a refusal that persists still fails" "1|" "$rc|$seed" +check "after exactly two attempts" 2 "$(grep -c . "$read_log")" +hint=$(HS_TOTP_BACKEND=keychain HS_TOTP_ERROR_LOG="$read_log" hs_seed_hint) +check_contains "the hint repeats the reason" "User interaction is not allowed" "$hint" +seed_probe cancel >/dev/null +check "a cancelled prompt is not shown twice" 1 "$(grep -c . "$read_log")" +rm -f "$read_log" "$read_log.last" +( HS_TOTP_BACKEND=file HS_TOTP_FILE=/nonexistent/seed HS_TOTP_ERROR_LOG="$read_log" \ + HS_TOTP_READ_RETRY_DELAY=0 hs_seed_read ) >/dev/null 2>&1 +check "a missing seed file is not read twice" 1 "$(grep -c . "$read_log")" +# Through the subcommand, as for the hint below: the fix is about what doctor says. +kc_dir=$(mktemp -d "${TMPDIR:-/tmp}/hstest.XXXXXX") +cat > "$kc_dir/kc.conf" <<'PROFILE' +HS_HOST="cluster.invalid" +HS_TOTP_BACKEND="keychain" +HS_TOTP_READ_RETRY_DELAY="0" +PROFILE +rm -f "$read_bin/count" +kc_out=$(PATH="$read_bin:$PATH" FAKE_SECURITY=refuse HS_CONFIG_DIR="$kc_dir" \ + "$HS_ROOT/bin/hpc-session" -p kc doctor 2>&1) +check_contains "doctor says why the keychain refused" "User interaction is not allowed" "$kc_out" +check "and keeps the history beside the profile" 0 "$(status_of test -s "$kc_dir/kc.totp-errors.log")" +rm -rf "$kc_dir" "$read_bin" + # `doctor` is documented as the last step of setup, so it must not send a `command` backend # user to store-seed — a subcommand that exits 1 saying it stores nothing. # @@ -907,6 +962,27 @@ help_text=$("$HS_ROOT/bin/hpc-session" --help) check_contains "help mentions open" "hpc-session open" "$help_text" check_contains "help mentions submit" "hpc-session submit" "$help_text" +# A release whose tool cannot say which release it is leaves a caller — an agent most of +# all — no way to know which side of a behaviour change it is on. The version has to +# survive a profile that does not load, and it has to match the newest CHANGELOG heading, +# or the tag, the notes and the tool drift apart silently. +ver_out=$("$HS_ROOT/bin/hpc-session" version 2>&1); rc=$? +check "version exits 0" 0 "$rc" +check_contains "version names the tool" "hpc-session" "$ver_out" +ver_num=${ver_out##* } +changelog_num=$(sed -n 's/^## \([0-9][0-9.]*\) .*/\1/p' "$HS_ROOT/CHANGELOG.md" | head -1) +check "version matches the changelog" "$changelog_num" "$ver_num" + +# It must answer with a profile that does not exist, which is when someone is diagnosing. +ver_broken=$(HS_CONFIG_DIR="$(mktemp -d "${TMPDIR:-/tmp}/hstest.XXXXXX")" \ + "$HS_ROOT/bin/hpc-session" -p nosuchprofile version 2>&1); rc=$? +check "version survives a missing profile" 0 "$rc" +check "and still reports the version" "hpc-session $changelog_num" "$ver_broken" + +doc_ver=$(HS_CONFIG_DIR="$(mktemp -d "${TMPDIR:-/tmp}/hstest.XXXXXX")" \ + "$HS_ROOT/bin/hpc-session" doctor 2>&1) +check_contains "doctor reports the version" "hpc-session $changelog_num" "$doc_ver" + bad=$("$HS_ROOT/bin/hpc-session" push only-one-arg 2>&1); rc=$? check "push with one argument fails" 1 "$rc" check_contains "push explains itself" "usage: push" "$bad" @@ -922,5 +998,73 @@ HS_CONFIG_DIR="$init_dir" HS_PROFILE=bigiron "$HS_ROOT/bin/hpc-session" init >/d check "exported HS_PROFILE init creates it" 0 "$(status_of test -f "$init_dir/bigiron.conf")" rm -rf "$(dirname "$init_dir")" +# --- two sessions on one link ---------------------------------------------------------- + +# `close` tore the master and the tunnel down unconditionally, so two sessions sharing the +# link — a wrapper script and an agent, two agents — each took the other's connection away +# with its own close. Holders: `open` records one, `close` removes its own and tears down +# only when none is left. Stubbed at hs_ssh (the `-O exit`), at hs_master_up and at the VPN +# hooks, so nothing leaves the host. +lease_ctl=$(mktemp -d "${TMPDIR:-/tmp}/hstest.XXXXXX") +lease_log=$(mktemp "${TMPDIR:-/tmp}/hstest.XXXXXX") +hs_real_ssh=$(declare -f hs_ssh) +hs_ssh() { case "$*" in *"-O exit"*) echo "master-exit" >> "$lease_log" ;; esac; return 0; } +hs_master_up() { return 0; } +lease_env() { + HS_HOST=cluster HS_CONTROL_DIR="$lease_ctl" HS_VPN_STATUS_CMD=true HS_VPN_UP_CMD="" \ + HS_VPN_DOWN_CMD="echo vpn-down >> $lease_log" "$@" +} +torn_down() { tr '\n' ' ' < "$lease_log" | sed 's/ $//'; } + +HS_LEASE=alpha lease_env hs_open_session >/dev/null 2>&1 +HS_LEASE=beta lease_env hs_open_session >/dev/null 2>&1 +out=$(HS_LEASE=alpha lease_env hs_close_session 2>&1) +check "a close with another holder keeps the link" 0 "$(status_of test ! -s "$lease_log")" +check_contains "and names who still holds it" "beta" "$out" +check "the closing holder is gone" 1 "$(status_of test -f "$lease_ctl/holders/alpha")" +HS_LEASE=beta lease_env hs_close_session >/dev/null 2>&1 +check "the last close tears the link down" "master-exit vpn-down" "$(torn_down)" + +# A holder named by a pid is only as live as that process. +: > "$lease_log" +sh -c 'exit 0' & dead_pid=$!; wait "$dead_pid" +printf 'pid=%s\nnamed=no\nsince=%s\n' "$dead_pid" "$(date +%s)" > "$lease_ctl/holders/$dead_pid" +HS_LEASE=gamma lease_env hs_close_session >/dev/null 2>&1 +check "a holder whose process ended does not keep the link" "master-exit vpn-down" "$(torn_down)" + +# A named holder outlives its shell, but not HS_LEASE_TTL: the backstop against a session +# that ended without closing. +: > "$lease_log" +printf 'pid=%s\nnamed=yes\nsince=0\n' "$$" > "$lease_ctl/holders/stale" +HS_LEASE=gamma lease_env hs_close_session >/dev/null 2>&1 +check "a holder past HS_LEASE_TTL does not keep the link" "master-exit vpn-down" "$(torn_down)" +: > "$lease_log" +printf 'pid=%s\nnamed=yes\nsince=%s\n' "$$" "$(date +%s)" > "$lease_ctl/holders/fresh" +HS_LEASE=gamma lease_env hs_close_session >/dev/null 2>&1 +check "a fresh named holder does" "" "$(torn_down)" + +# HS_CLOSE_FORCE=1 is the way out when a holder is known to be dead: it closes and clears. +HS_LEASE=alpha HS_CLOSE_FORCE=1 lease_env hs_close_session >/dev/null 2>&1 +check "HS_CLOSE_FORCE=1 closes over a live holder" "master-exit vpn-down" "$(torn_down)" +check "and clears every holder" 1 "$(status_of test -d "$lease_ctl/holders")" + +# A session that only runs commands over a master someone else raised holds it too, and +# status says who does. +HS_LEASE=runner lease_env hs_ensure_open >/dev/null 2>&1 +check "an implicit open registers a holder" 0 "$(status_of test -f "$lease_ctl/holders/runner")" +check_contains "status lists the holders" "holders: runner" \ + "$(HS_TOTP_BACKEND=none lease_env hs_status 2>&1)" + +# The holder's name becomes a file name under the control directory. +out=$(HS_LEASE='../escape' lease_env hs_open_session 2>&1); rc=$? +check "a holder name cannot leave the directory" 1 "$rc" +check "and nothing was written outside it" 1 "$(status_of test -e "$lease_ctl/escape")" + +rm -rf "$lease_ctl" "$lease_log" +unset -f lease_env torn_down +eval "$hs_real_ssh" +restore_lib +. "$HS_ROOT/lib/session.sh" + printf '\n%s passed, %s failed\n' "$PASSED" "$FAILED" [ "$FAILED" -eq 0 ]