From 57bbc6d517d3803a89eeb7e2d38851306eaeeae3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis-F=C3=A9lix=20Nothias?= Date: Fri, 21 Aug 2026 10:48:17 +0200 Subject: [PATCH 1/4] chore: a changelog, and the ordinary example profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing here was ever tagged, so anyone who cloned since 2026-07-29 holds a copy with defects that are mostly silent — a watch that reports running jobs as finished, a precedence rule that discards every documented override, a submit that fails on any current OpenSSH. The changelog leads with what CHANGED rather than what was added, because that is the part that can surprise someone upgrading: sbatch arguments are quoted now, a remote script path is literal, and watch has a third exit status. The examples directory had only the hardest case, a cluster behind a full-tunnel VPN that also enforces TOTP. Most people start with neither, and README solicits worked profiles from contributors — so the simple shape should be there to copy. --- CHANGELOG.md | 111 +++++++++++++++++++++++++++++++++++ README.md | 10 +++- examples/keyonly-direct.conf | 37 ++++++++++++ 3 files changed, 156 insertions(+), 2 deletions(-) create mode 100644 CHANGELOG.md create mode 100644 examples/keyonly-direct.conf diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..58e7ee0 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,111 @@ +# Changelog + +## 0.1.0 — 2026-08-21 + +First tagged release. The repository has been public since 2026-07-29 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 188. + +### 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. +- 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. +- Everything above is verified offline, against stubs. No part of this release has been + exercised against a live SLURM controller. diff --git a/README.md b/README.md index 838110f..e931132 100644 --- a/README.md +++ b/README.md @@ -285,8 +285,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 +295,11 @@ 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. + ## License MIT — see [LICENSE](LICENSE). Copyright CNRS and Université Côte d'Azur. 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" From 02265e679ce3ca6f7c6c99c2f8a0ab6a7a50762c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis-F=C3=A9lix=20Nothias?= Date: Fri, 21 Aug 2026 13:11:29 +0200 Subject: [PATCH 2/4] feat: let the tool say which release it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A first tag makes version a question with an answer, and CHANGELOG.md opens with behaviour changes — so a caller has to be able to tell which side of them it is on. Nothing in the tool could say. `version` is dispatched before the profile loads, like `init`: a setup that does not load is exactly when someone needs to report which copy they are running. `doctor` opens with the same line, since that output is what gets pasted into an issue. A test holds the string equal to the newest heading in CHANGELOG.md, so a tag cannot drift from what the tool reports. Verified by mutation: drifting the string fails three assertions, dropping the doctor line fails one, and moving the dispatch after the loader fails the two that cover a broken profile. Also corrects two claims in the changelog itself — the repository's public date, which was off by a timezone, and the assertion count. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 11 +++++++---- README.md | 3 ++- SKILL.md | 3 +++ bin/hpc-session | 11 +++++++++++ tests/run_tests.sh | 21 +++++++++++++++++++++ 5 files changed, 44 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58e7ee0..b4c0909 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,13 +2,13 @@ ## 0.1.0 — 2026-08-21 -First tagged release. The repository has been public since 2026-07-29 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. +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 188. +previous state of `main`. The suite went from 37 assertions to 194. ### Changed — read before upgrading @@ -47,6 +47,9 @@ previous state of `main`. The suite went from 37 assertions to 188. `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 diff --git a/README.md b/README.md index e931132..ca08ecc 100644 --- a/README.md +++ b/README.md @@ -298,7 +298,8 @@ Université Côte d'Azur. ## Changelog [CHANGELOG.md](CHANGELOG.md). Behaviour changes worth knowing before upgrading are listed -first in each release. +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 diff --git a/SKILL.md b/SKILL.md index eda7ae6..6bbccfc 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. 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/tests/run_tests.sh b/tests/run_tests.sh index 0c42484..618f67f 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -907,6 +907,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" From 079556ecf9df532baea5f318f93c097173f3a1b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis-F=C3=A9lix=20Nothias?= Date: Fri, 21 Aug 2026 16:05:53 +0200 Subject: [PATCH 3/4] docs: the release has now been run against a live controller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Known limitations" section said no part of this release had been exercised against a live SLURM controller. That is no longer true: one end-to-end run — open, render, submit, queue, watch to completion, fetch, cancel, close — was made against a TOTP-gated cluster behind a VPN, and behaved as documented. Stated as one site and one SLURM version, which is what it is, rather than as a compatibility claim. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4c0909..02389d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -110,5 +110,7 @@ previous state of `main`. The suite went from 37 assertions to 194. terminal is unaffected. - Multi-cluster (federated) submission is out of scope. `submit` says so when `sbatch` reports another cluster. -- Everything above is verified offline, against stubs. No part of this release has been - exercised against a live SLURM controller. +- 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. From 3d356bba5621d5d43f0400d4d3b8b4ac1d61e970 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis-F=C3=A9lix=20Nothias?= Date: Thu, 1 Oct 2026 14:55:34 +0200 Subject: [PATCH 4/4] fix(totp): say why a seed read failed, and try it once more MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed read sent the backend's stderr to /dev/null, so open, doctor and status all said "no seed — run store-seed" while the seed sat in the keychain, refused for one request only. The backend's own message, its exit status and, for the keychain, the macOS session the read ran in are now appended to HS_TOTP_ERROR_LOG, and the hint repeats the latest reason. The seed itself is never written anywhere. The read is tried once more after HS_TOTP_READ_RETRY_DELAY seconds, which absorbs a refusal that clears by itself; not after a cancelled prompt, and not for a missing seed file. Thirteen new assertions, with `security` shadowed throughout; they fail on the previous code. Suite: 207 passed. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 14 +++++++ SECURITY.md | 4 ++ config.example | 6 +++ docs/2fa-enrollment.md | 1 + lib/config.sh | 4 ++ lib/totp.sh | 88 +++++++++++++++++++++++++++++++++++++++--- tests/run_tests.sh | 55 ++++++++++++++++++++++++++ 7 files changed, 166 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 02389d3..d1d15be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # Changelog +## Unreleased + +### 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 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/config.example b/config.example index f2717ff..5a5499c 100644 --- a/config.example +++ b/config.example @@ -67,6 +67,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/lib/config.sh b/lib/config.sh index 959e919..d826b8c 100644 --- a/lib/config.sh +++ b/lib/config.sh @@ -56,6 +56,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/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 618f67f..9c04e1b 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. #