From 6927c1739059f94c10afbda82e3a0403cc3102a7 Mon Sep 17 00:00:00 2001 From: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> Date: Sun, 11 Oct 2026 13:00:11 +0200 Subject: [PATCH 1/9] Accept the decision on signing in without a keychain Replace the proposal with the decision: keep the keychain as the default and add an explicit opt-in token store that is a plain-text file, with no fallback in either direction. Record the file checks, the location in the state directory, the deferral of Windows and of the Linux kernel keyring, and the server-side refresh token lifetime that is still to be confirmed. The text now matches the fail-closed sign-in behaviour, where tokens that cannot be stored are revoked. Refs #39 Signed-off-by: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> --- .../0001-sign-in-without-a-keychain.md | 288 +++++++++--------- 1 file changed, 139 insertions(+), 149 deletions(-) diff --git a/docs/decisions/0001-sign-in-without-a-keychain.md b/docs/decisions/0001-sign-in-without-a-keychain.md index 6a3d9d3..6cd9146 100644 --- a/docs/decisions/0001-sign-in-without-a-keychain.md +++ b/docs/decisions/0001-sign-in-without-a-keychain.md @@ -1,156 +1,146 @@ # 0001. Signing in on a machine without a keychain -- Status: Proposed +- Status: Accepted - Issue: [#39](https://github.com/Hodeitek/hodeishield-cli/issues/39) +- Related: [#101](https://github.com/Hodeitek/hodeishield-cli/issues/101) (fail-closed sign-in), + [#111](https://github.com/Hodeitek/hodeishield-cli/issues/111) (keychain out of reach in a remote + session) - Deciders: maintainers -This is a proposal. Nothing here is decided or implemented; the recommendation is the author's. - ## Context -`hodeishield login` stores its tokens (an access token and, by default, a refresh token) in the -system keychain and nowhere else (`crates/hodeishield-cli/src/auth/store.rs:1-6`). On Linux the only -backend is the Secret Service over D-Bus, through `zbus-secret-service-keyring-store` -(`crates/hodeishield-cli/Cargo.toml:47-48`, `store.rs:127-131`). There is no kernel keyring (keyutils) -backend and no file fallback. - -On a machine with no Secret Service (an SSH server, a container, WSL without a desktop session, a CI -runner) this is what happens today: - -- The store is first touched when the token is saved (`commands/session.rs:100`), after the whole - browser or `--device` flow has finished. The person completes sign-in and consent, then gets - "the system keychain is not available" (`store.rs:17,100-110`). The token already issued is not - revoked by the CLI, so it stays valid at the app until revoked there. -- The error points to `HODEISHIELD_API_KEY` (`auth/mod.rs:87-97`); the README says the same - (`README.md:294-297`, and for containers `README.md:160-161`). -- `--device` does not help: it avoids the browser, not the storage. - -Who is affected: people who work from a jump host or a remote shell with their own identity (the -motivating case in the issue: MSSP staff), containers, and CI. A tenant API key works everywhere -(`auth/mod.rs:38-45`), but it is one more long-lived secret per tenant, it is not tied to a person, -and someone has to create it. - -What is at stake: a sign-in token reads one tenant's supply-chain risk data. The access token is -short-lived; the refresh token is long-lived and is the sensitive part. The README promises that -tokens are "never in a file" (`README.md:30`, `cli.rs:103-104`), so any option that writes a file -changes a documented guarantee. - -Existing seam: `TokenStore` (`store.rs:87-93`) has `load`, `save` and `delete` per profile, with a -test-only `MemoryStore` (`store.rs:185`). The CLI builds the store in one place -(`commands/mod.rs:83`). A new store is a new implementation of that trait plus a way to choose it. - -## Options - -### A. Keep keychain-only; document API keys for headless use - -Change nothing in behaviour. Improve the experience: refuse early (before opening a browser or -printing a device code) when no keychain is reachable, and say what to do; expand the README with a -"servers and containers" recipe. - -- Security: best. No new storage, no weaker path. API keys can be scoped to read scopes and rotated. -- Effort: small (an early check, a message, docs). -- Consequences: the issue's use case is answered with "request an API key", not with the person's own - identity. Per-tenant keys must be issued and tracked by someone. - -### B. Linux kernel session keyring (keyutils) as a fallback - -Store the payload with `keyctl` in the session or user keyring when no Secret Service is present. - -- Security: the secret is kernel memory, never on disk, and not readable by other users. But it is - scoped to a keyring, not a person's login: a user keyring is readable by every process of that user; - a session keyring is visible only to that session's processes. -- Lifetime: it does not survive a reboot. A session keyring disappears when the last process of the - session ends, so a new SSH connection would be signed out; a user keyring (`@u`) survives - across sessions until reboot or key timeout, but with defaults that vary by distribution and - container runtime. Containers often block the `keyctl` syscall under the default seccomp profile. -- Effort: medium. A new backend crate (or direct syscalls), Linux only, behind a selection rule and - tests that need a real kernel keyring. -- Consequences: helps SSH servers somewhat, containers and WSL unreliably. Behaviour (survives or not) - would be surprising to users and needs documenting per mode. - -### C. Encrypted file store with a passphrase - -Write the payload to a file in the config directory, encrypted with a key derived (for example with -Argon2id) from a passphrase. - -- Where the key comes from: a prompt on a terminal, or an environment variable / file descriptor for - non-interactive use (for example `HODEISHIELD_PASSPHRASE`). -- UX: a prompt on every command that needs the token, or a passphrase in the environment, which in - practice moves the secret from one place to another. Without a terminal and without the variable - the command must fail clearly. -- Threat model: protects a stolen disk, backup or file copy; does not protect against code running as - the same user once the passphrase is in memory or in the environment. A weak passphrase makes the - file an offline-guessing target for a long-lived refresh token. -- Effort: large. New crypto dependencies to vet (`deny.toml`), file format and versioning, prompting, - locking against concurrent refreshes, key rotation, tests. -- Consequences: works in all environments with a terminal; the largest new security surface to own. - -### D. Plaintext file with 0600 permissions behind an explicit opt-in - -Store the payload in a file in the config directory, mode 0600 in a 0700 directory, only when the -person opts in (for example `HODEISHIELD_INSECURE_STORAGE=file` or a config key), with a warning on -every sign-in. - -- Security: weakest. Anyone who can read the file (same user, root, backups, a mis-mounted volume, an - image layer) holds a refresh token valid until revoked. Permissions do not protect against malware - running as the user. This reverses the "never in a file" promise for those who opt in. -- Effort: small to medium (store, permission checks and refusal if the file is looser than 0600, - Windows ACL handling, docs, tests). -- Consequences: common practice in other CLIs, and it solves every environment, but it normalises the - exact exposure the CLI was designed to avoid. Requests to remove the opt-in guard would follow. - -### E. No storage: print a token once or keep it in memory for one command - -For example `hodeishield login --print-token` runs the flow and prints only the access token, to be -exported as an environment variable for the session; or the CLI uses the access token only within -one invocation. - -- Security: good if the refresh token is never requested or never stored. The access token is - short-lived, so exposure is limited to its lifetime; it can appear in shell history or process - environment if handled carelessly. -- Effort: small to medium. The CLI would need to request no `offline_access` scope, accept an access - token from the environment (a new credential source in `auth/mod.rs:38`), and print it safely - (stdout only, never with other text). -- Consequences: no refresh, so the person signs in again when the token expires (the lifetime is set - by the app). Fine for a short task on a jump host; poor for unattended jobs, which should use an API - key. Interactive device flow from a remote shell is natural here. - -### F. Delegate to an external credential helper - -The CLI calls a configured program (as some tools do with git or Docker credential helpers) to load -and save the payload, so people bring their own vault. - -- Security: depends on the helper; the CLI never writes a file. The helper's protocol and trust (the - program path comes from configuration) become part of the threat model. -- Effort: medium (protocol, spawn without a shell, error handling, docs). Larger audience needed to - justify it. - -## Recommendation (the author's, not a decision) - -Security first: the refresh token is the asset, and the options that never write it to disk or -plaintext are A, E, and B. In that order of preference: - -1. Do A now, regardless: fail early, before the browser or device code, and document the headless - path. This fixes the worst part of the current behaviour (completing sign-in and then failing, - leaving an unrevoked token) at almost no risk. -2. Add E for the jump-host case: a sign-in that requests no refresh token and does not store anything, - with the access token handed to the shell explicitly. It keeps personal identity without a stored - long-lived secret, and costs one new credential source. -3. Do not implement D or C unless the maintainers accept new storage risk. If a file is needed - anyway, prefer C over D, and only with a design review of the format and the passphrase handling. - B is worth a spike only if SSH-server users report that E is not enough. - -If the maintainers choose not to add any alternative store, the second acceptance criterion of #39 is -met by A: the README recommends API keys and explains why. - -## Open questions for the maintainers - -1. Is "tokens never in a file" a product guarantee to keep, or a default that an opt-in may change? -2. Can the app issue an access token without a refresh token (no `offline_access`) with a useful - lifetime for a jump-host session? What is that lifetime? -3. Should a person's own sign-in be preferred over per-tenant API keys for MSSP staff, or are API keys - an acceptable answer given they can be scoped and rotated? -4. For containers and CI, is an API key always the intended answer (so options B to D are out of scope - there)? -5. Should the CLI revoke the token it just obtained when it cannot store it (today it does not)? -6. Which platforms matter most: SSH servers, WSL, containers, CI? +`hodeishield login` keeps its tokens (an access token and, by default, a refresh token) in the system +keychain: macOS Keychain, Windows Credential Manager, or on Linux the Secret Service over D-Bus +(`crates/hodeishield-cli/src/auth/store.rs`). Until this decision there was no other place to keep +them, and the README promised that tokens are "never in a file". + +On a machine with no keychain (an SSH or jump host, a container, WSL without a desktop session, a CI +runner) this is what happens: + +- `login` checks that the keychain can keep a session **before** it opens a browser or shows a device + code, and stops with a pointer to `HODEISHIELD_API_KEY` (`commands/session.rs`). +- If saving fails after the app has issued tokens, the CLI revokes the refresh token and then the + access token, says so, and exits non-zero, so no grant is left that nothing holds. +- Other commands, with no API key set, exit 3 and say that the keychain is not available in this + session. +- `--device` removes the need for a browser, so storage is the only blocker. + +Who is affected: people who work from a jump host with their own identity (the case in the issue), +containers, and CI. A tenant API key works everywhere, but it is one more long-lived secret per +tenant, it is not tied to a person, and someone has to create and track it. + +The asset is the refresh token. It is long-lived, covers one tenant with the read scopes chosen at +consent, belongs to one person, and ends with `logout` or when it is revoked in the app. The access +token is short-lived. + +| Who can read it | Keychain | File readable only by its owner | API key in the environment | +|---|---|---|---| +| Code running as the same user | Yes, if the keychain is unlocked | Yes | Yes (and child processes inherit it) | +| Other users | No | No, if owner and mode are right | Depends on where it is kept | +| `root` | Yes | Yes | Yes | +| Backups, snapshots, disk images | Encrypted at rest | Plaintext | Plaintext if kept in a file | +| A container layer, if sign-in ran at build time or a signed-in container is published | Not applicable | Plaintext | Same through `ENV` or `ARG` | +| A network home directory (NFS) | Encrypted | Readable by the server's administrators and by `root` elsewhere | Same | + +On a machine with no keychain, a file readable only by its owner is weaker than a keychain mainly +against offline copies; against code already running as the user it is about the same. Compared with +the alternative that exists today, the refresh token has the same kind of exposure as an API key, but +it belongs to one person, can be attributed to that person, and `logout` revokes it. An API key lives +until someone revokes it by hand. + +## Options considered + +- **A. Keychain only; document API keys.** Strongest, and nothing new to protect. A person still + cannot use their own identity on a jump host. Already in place, and kept as the default. +- **B. Plain-text file, opt-in.** Small to medium effort; works wherever there is a file system. The + weakest storage, so it must be an explicit choice with checks on the file. +- **C. Encrypted file with a passphrase.** Protects copies of the file, but a prompt on every command + does not fit scripts, a passphrase in the environment is no better than B against code running as + the same user, and a weak one makes the file a target for offline guessing. Large: key derivation + and encryption dependencies to vet, a versioned format, sessions with no terminal. +- **D. Linux kernel keyring.** Secrets never reach disk, backups or container layers. But nothing + survives a reboot, a session keyring ends with its session (a new SSH login usually gets a new + one), the persistent keyring expires, and container runtimes often block the system call. +- **E. No stored refresh token** (print an access token once, to be exported). Short exposure, but no + refresh and not usable by unattended jobs, which should use an API key anyway. +- **F. An external credential helper.** Shifts the trust to a configured program; justified only with + a larger audience. + +## Decision + +Keep the keychain as the default and add **an opt-in token store that is a plain-text file** (B), with +the following rules. Containers and CI keep using API keys. + +- **Opt-in only.** `HODEISHIELD_TOKEN_STORE=plaintext-file`, or the global configuration key + `token_store = "plaintext-file"`. There is no per-profile option and no flag on `login`, because + every command must read from the same store. The environment wins over the configuration file. The + only other accepted value is `keychain`; anything else is an error (exit code 1) before any network + call. +- **No fallback in either direction.** With the default, nothing changes and no file is ever written, + also when the keychain is missing. With the opt-in, the keychain is never touched. Tokens are never + moved between stores: sign out, change the setting, sign in again. +- **Location.** The platform's state directory, not the configuration directory: + `$XDG_STATE_HOME/hodeishield/sessions/` (by default `~/.local/state/hodeishield/sessions/`) on + Linux, and the application support directory on macOS. One file per profile, named by the SHA-256 + of the profile name (names may contain `/` or `..`). The configuration file stays free of + credentials, so copying or syncing it never copies a token. +- **Checks on every read and write.** The CLI refuses, and never repairs, a file or directory that is + a symbolic link, is not owned by the current user, or has any group or other permission bit. It + creates the directory with mode 700 and the files with mode 600. The opened file is checked again + after it is opened, so a file swapped in between is refused. A corrupt file is an error that names + it; a file written for another profile is not accepted. +- **Atomic writes.** The CLI writes a new file created with mode 600 in the same directory, flushes it + to disk, renames it over the target and flushes the directory. A temporary file is removed when + anything fails, and no token is ever in a file that others can read. +- **Behaviour of #101 is unchanged.** `login` checks that the selected store is usable before it + opens a browser or shows a code. If saving fails after the exchange, the refresh token and then the + access token are revoked and the command exits non-zero. `logout` revokes, then deletes the file. + No new exit codes: `login` failures are 1, other commands that need a credential and find the file + refused are 3, a corrupt file is 1. +- **Windows is deferred.** There are no permission bits to check, and setting and checking an ACL + needs platform calls that the workspace's `unsafe_code = "forbid"` rules out without a vetted + wrapper. Selecting the file store on Windows is refused with exit code 1, before anything is asked + of the app. +- **Network home directories.** Documented, not refused. On Linux the CLI also warns on `login` when + the directory is on NFS or SMB, where that can be told cheaply from the file system type; elsewhere + it cannot be told reliably, so only the documentation covers it. +- **Messages.** When the keychain is not available, the hint keeps the API key first and offers the + opt-in second, with one line on the trade-off: tokens are stored unencrypted in a file only you can + read. The README has a section for servers, containers and remote sessions with the same order and + no container recipe for the file store. +- **No telemetry.** The choice of store never leaves the machine. + +## Consequences + +- The README promise "tokens never in a file" becomes "in the system keychain unless you opt in". + Every place that said so (README, `login --help`, contributing and security notes) is updated. +- People on a jump host can sign in as themselves. They accept the exposure in the table above for + that machine; the README says so next to the instructions. +- A new class of local failure exists (a loosened mode after a copy or restore, a symbolic link, a + file owned by another user). The CLI refuses and explains instead of repairing, so a mistake is + visible rather than silently fixed or silently tolerated. +- Deleting the file, or restoring it from a backup, does not end or restore a session; only the + revocation at the app does. `logout` therefore revokes first. +- A configuration file with `token_store` is refused by older CLIs (unknown field). The environment + variable avoids that. +- The tests run in the normal suite, including the refusals (symbolic link, wrong owner, wrong modes, + invalid setting, no fallback, Windows). + +## Deferred + +- **Windows.** Needs an ACL design and either a vetted wrapper crate or an exception to + `unsafe_code = "forbid"`. +- **Linux kernel keyring.** Not worth a spike until users on SSH servers report that the file store + does not fit, for example because data on disk is the main concern. Its lifetime rules (above) make + it a surprising default. +- **An encrypted file** (option C), **an access-token-only sign-in** (option E) and **a credential + helper** (option F), for the reasons above. +- **Migration between stores.** Deliberately absent; switching is sign out, change the setting, sign + in. + +## Open question + +To be confirmed with the API provider: the refresh token's lifetime, rotation and idle expiry on the +server side. They decide how long a copy of a token file stays useful, and whether the exposure of a +leaked file can be bounded by something other than revocation. Nothing in this decision depends on the +answer, but the README's wording about exposure should be revisited once it is known. From 960eeaa27497d20c65720cf9977208212b5d3c7f Mon Sep 17 00:00:00 2001 From: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> Date: Sun, 11 Oct 2026 13:00:11 +0200 Subject: [PATCH 2/9] Add an opt-in plain-text token store People who work from a jump host or another machine without a keychain could only use a tenant API key. Add a second token store, selected by HODEISHIELD_TOKEN_STORE=plaintext-file or the global token_store setting, that keeps one file per profile in the state directory. The keychain stays the default and the two stores never fall back to each other. Every read and write refuses, without repairing, a symbolic link, a file or directory owned by another user, or one open to group or others. Files are created with mode 600 and replaced atomically. The effective user id comes from rustix, which is already in the dependency graph on Linux, so no unsafe code is needed. Windows is refused for now. Refs #39 Signed-off-by: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> --- Cargo.lock | 1 + crates/hodeishield-cli/Cargo.toml | 3 + crates/hodeishield-cli/src/auth/file_store.rs | 718 ++++++++++++++++++ crates/hodeishield-cli/src/auth/mod.rs | 23 +- crates/hodeishield-cli/src/auth/store.rs | 77 +- crates/hodeishield-cli/src/cli.rs | 8 +- crates/hodeishield-cli/src/commands/config.rs | 9 + crates/hodeishield-cli/src/commands/mod.rs | 2 +- .../hodeishield-cli/src/commands/session.rs | 9 +- crates/hodeishield-cli/src/config.rs | 143 +++- crates/hodeishield-cli/tests/cli.rs | 478 ++++++++++++ 11 files changed, 1443 insertions(+), 28 deletions(-) create mode 100644 crates/hodeishield-cli/src/auth/file_store.rs diff --git a/Cargo.lock b/Cargo.lock index 61bf3f2..3f1eebc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -998,6 +998,7 @@ dependencies = [ "mockito", "predicates", "reqwest", + "rustix", "secrecy", "serde", "serde_json", diff --git a/crates/hodeishield-cli/Cargo.toml b/crates/hodeishield-cli/Cargo.toml index 8b825c7..d33ed82 100644 --- a/crates/hodeishield-cli/Cargo.toml +++ b/crates/hodeishield-cli/Cargo.toml @@ -41,6 +41,9 @@ webbrowser = "1" [target.'cfg(target_os = "macos")'.dependencies] apple-native-keyring-store = { version = "1.0.2", features = ["keychain"] } +[target.'cfg(unix)'.dependencies] +rustix = { version = "1.1.5", default-features = false, features = ["std", "process", "fs"] } + [target.'cfg(windows)'.dependencies] windows-native-keyring-store = { version = "1.1.0", default-features = false } diff --git a/crates/hodeishield-cli/src/auth/file_store.rs b/crates/hodeishield-cli/src/auth/file_store.rs new file mode 100644 index 0000000..554b902 --- /dev/null +++ b/crates/hodeishield-cli/src/auth/file_store.rs @@ -0,0 +1,718 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Hodeitek S.L. + +//! The opt-in plain-text token store: one JSON file per profile, readable only by its owner. +//! +//! This is the weaker of the two stores and is never chosen on its own: it is used only when +//! `HODEISHIELD_TOKEN_STORE=plaintext-file` or the global `token_store` setting says so. There is +//! no fallback in either direction (see `docs/decisions/0001-sign-in-without-a-keychain.md`). +//! +//! Files live in the platform's state directory (`$XDG_STATE_HOME/hodeishield/sessions/` on Linux), +//! not in the configuration directory, so a copied or synced configuration never carries a token. +//! The file name is a hash of the profile name, which may contain `/` or `..`. +//! +//! On every read and write the CLI refuses, and never repairs, a file or directory that is a +//! symbolic link, that belongs to another user, or that is open to group or others. Windows has no +//! mode bits to check and is not supported yet. + +use super::store::{Payload, StoredToken, TokenStore}; +use crate::failure::{Failure, Result}; +use serde::{Deserialize, Serialize}; +use std::path::{Path, PathBuf}; + +/// Start of the message when the store refuses a file or directory it cannot trust. Commands that +/// need a credential turn it into "not authenticated" (exit code 3), like an unreachable keychain. +pub const TOKEN_FILE_REFUSED: &str = "the plain-text token store cannot be used"; + +/// What is written to the file: the profile it belongs to (checked on read) and the keychain +/// payload. +#[cfg_attr(not(unix), allow(dead_code))] +#[derive(Serialize, Deserialize)] +struct Record { + profile: String, + #[serde(flatten)] + payload: Payload, +} + +/// The sessions directory of this user, or why there is none. +pub fn sessions_dir() -> Result { + let dirs = + directories::ProjectDirs::from("com", "Hodeitek", "HodeiShield").ok_or_else(|| { + Failure::general("Cannot find a state directory for this user.") + .hint("Use the system keychain or a tenant API key instead.") + })?; + // `state_dir` exists on Linux and the BSDs; macOS has only the application support directory. + Ok(dirs + .state_dir() + .unwrap_or_else(|| dirs.data_local_dir()) + .join("sessions")) +} + +/// The file name for a profile: the hex SHA-256 of its name, so no profile name can leave the +/// directory. +#[cfg_attr(not(unix), allow(dead_code))] +fn file_name(profile: &str) -> String { + use sha2::{Digest, Sha256}; + use std::fmt::Write; + let digest = Sha256::digest(profile.as_bytes()); + let mut name = digest.iter().fold(String::with_capacity(68), |mut s, b| { + let _ = write!(s, "{b:02x}"); + s + }); + name.push_str(".json"); + name +} + +/// Plain-text files in a private directory. +#[cfg_attr(not(unix), allow(dead_code))] +#[derive(Debug, Clone)] +pub struct FileStore { + dir: PathBuf, +} + +impl FileStore { + /// The store in the platform's state directory. Refused on Windows. + pub fn platform() -> Result { + if cfg!(windows) { + return Err(unsupported()); + } + Ok(Self::in_dir(sessions_dir()?)) + } + + pub(crate) fn in_dir(dir: PathBuf) -> Self { + Self { dir } + } + + #[cfg_attr(not(unix), allow(dead_code))] + fn path_for(&self, profile: &str) -> PathBuf { + self.dir.join(file_name(profile)) + } +} + +fn unsupported() -> Failure { + Failure::general( + "the plain-text token store is not supported on Windows yet; signing in needs the \ + Windows Credential Manager", + ) + .hint( + "Leave token_store unset (or `keychain`), or use a tenant API key in HODEISHIELD_API_KEY.", + ) +} + +#[cfg_attr(not(unix), allow(dead_code))] +fn refused(path: &Path, reason: &str, fix: Option) -> Failure { + let failure = Failure::general(format!("{TOKEN_FILE_REFUSED}: {} {reason}", path.display())); + match fix { + Some(fix) => failure.hint(format!( + "It was left untouched. {fix} Or use a tenant API key in HODEISHIELD_API_KEY." + )), + None => failure.hint( + "It was left untouched. Remove it yourself if you do not need it, or use a tenant API \ + key in HODEISHIELD_API_KEY.", + ), + } +} + +#[cfg_attr(not(unix), allow(dead_code))] +fn io_failure(what: &str, path: &Path, error: &std::io::Error) -> Failure { + Failure::general(format!("{what} {}: {error}", path.display())) +} + +#[cfg(not(unix))] +impl TokenStore for FileStore { + fn load(&self, _: &str) -> Result> { + Err(unsupported()) + } + fn save(&self, _: &str, _: &StoredToken) -> Result<()> { + Err(unsupported()) + } + fn delete(&self, _: &str) -> Result { + Err(unsupported()) + } + fn check_available(&self, _: &str) -> Result<()> { + Err(unsupported()) + } + fn location(&self, _: &str) -> Option { + None + } +} + +#[cfg(unix)] +impl TokenStore for FileStore { + fn load(&self, profile: &str) -> Result> { + unix::load(self, profile) + } + fn save(&self, profile: &str, token: &StoredToken) -> Result<()> { + unix::save(self, profile, token) + } + fn delete(&self, profile: &str) -> Result { + unix::delete(self, profile) + } + fn check_available(&self, profile: &str) -> Result<()> { + unix::check_available(self, profile) + } + fn location(&self, profile: &str) -> Option { + Some(self.path_for(profile)) + } +} + +#[cfg(unix)] +mod unix { + use super::{FileStore, Record, io_failure, refused}; + use crate::auth::store::StoredToken; + use crate::failure::{Failure, Result}; + use std::fs::{File, Metadata, OpenOptions}; + use std::io::{Read, Write}; + use std::os::unix::fs::{DirBuilderExt, MetadataExt, OpenOptionsExt, PermissionsExt}; + use std::path::{Path, PathBuf}; + + /// Biggest file read: a token payload is well under a kilobyte. + const MAX_BYTES: u64 = 64 * 1024; + + /// The effective user id of this process. + fn euid() -> u32 { + rustix::process::geteuid().as_raw() + } + + /// What is wrong with a file or directory, judged from its own metadata (never a followed + /// link): `None` when it can be trusted. + pub(super) fn problem(meta: &Metadata, want_dir: bool, euid: u32) -> Option<(String, String)> { + let kind = if want_dir { "directory" } else { "file" }; + let fix_mode = if want_dir { "700" } else { "600" }; + if meta.file_type().is_symlink() { + return Some(( + "is a symbolic link, which is never followed".into(), + String::new(), + )); + } + let right_type = if want_dir { + meta.is_dir() + } else { + meta.is_file() + }; + if !right_type { + return Some((format!("is not a regular {kind}"), String::new())); + } + if meta.uid() != euid { + return Some(( + format!("belongs to another user (uid {}), not to you", meta.uid()), + String::new(), + )); + } + let mode = meta.mode() & 0o777; + if mode & 0o077 != 0 { + return Some(( + format!("is accessible to other users (mode {mode:03o}); it must be {fix_mode}"), + format!( + "If you own it and nobody else read it, `chmod {fix_mode}` fixes it; a token that may have been read should be revoked from the app." + ), + )); + } + None + } + + fn check(path: &Path, meta: &Metadata, want_dir: bool) -> Result<()> { + match problem(meta, want_dir, euid()) { + None => Ok(()), + Some((reason, fix)) => Err(refused(path, &reason, Some(fix).filter(|f| !f.is_empty()))), + } + } + + /// Looks at a path without following a link at its end. `None` when it is absent. + fn look(path: &Path) -> Result> { + match std::fs::symlink_metadata(path) { + Ok(meta) => Ok(Some(meta)), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(e) => Err(io_failure("Cannot look at", path, &e)), + } + } + + /// The sessions directory, checked. `None` when absent and `create` is false; created with + /// mode 0700 otherwise. + fn dir(store: &FileStore, create: bool) -> Result> { + if let Some(meta) = look(&store.dir)? { + check(&store.dir, &meta, true)?; + return Ok(Some(())); + } + if !create { + return Ok(None); + } + std::fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(&store.dir) + .map_err(|e| io_failure("Cannot create", &store.dir, &e))?; + // Whatever the umask or a concurrent creator did, it is judged like an existing one. + let meta = look(&store.dir)?.ok_or_else(|| { + Failure::general(format!( + "{} disappeared while it was created.", + store.dir.display() + )) + })?; + check(&store.dir, &meta, true)?; + Ok(Some(())) + } + + /// Opens a file for reading and judges the opened handle, which must be the very file that + /// was looked at (a swap in between is refused). + fn open_checked(path: &Path) -> Result> { + let Some(seen) = look(path)? else { + return Ok(None); + }; + check(path, &seen, false)?; + let file = match File::open(path) { + Ok(file) => file, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(e) => return Err(io_failure("Cannot read", path, &e)), + }; + let opened = file + .metadata() + .map_err(|e| io_failure("Cannot read", path, &e))?; + if (opened.dev(), opened.ino()) != (seen.dev(), seen.ino()) { + return Err(refused(path, "changed while it was being opened", None)); + } + check(path, &opened, false)?; + Ok(Some(file)) + } + + pub(super) fn load(store: &FileStore, profile: &str) -> Result> { + if dir(store, false)?.is_none() { + return Ok(None); + } + let path = store.path_for(profile); + let Some(file) = open_checked(&path)? else { + return Ok(None); + }; + let mut text = String::new(); + file.take(MAX_BYTES) + .read_to_string(&mut text) + .map_err(|e| io_failure("Cannot read", &path, &e))?; + let corrupt = || { + Failure::general(format!( + "the sign-in stored in {} is unreadable", + path.display() + )) + .hint(format!( + "Remove {} and sign in again with `hodeishield login`.", + path.display() + )) + }; + let record: Record = serde_json::from_str(&text).map_err(|_| corrupt())?; + if record.profile != profile { + return Err(corrupt()); + } + Ok(Some(StoredToken::from_payload(record.payload))) + } + + /// Removes the temporary file unless it was renamed into place. + struct Temp<'a> { + path: &'a Path, + armed: bool, + } + + impl Drop for Temp<'_> { + fn drop(&mut self) { + if self.armed { + let _ = std::fs::remove_file(self.path); + } + } + } + + fn temp_path(store: &FileStore, profile: &str) -> Result { + let mut random = [0u8; 8]; + getrandom::fill(&mut random) + .map_err(|e| Failure::general(format!("cannot get random bytes: {e}")))?; + let suffix: String = random.iter().map(|b| format!("{b:02x}")).collect(); + Ok(store.dir.join(format!( + ".{}.{suffix}.tmp", + &super::file_name(profile)[..16] + ))) + } + + /// Writes `bytes` to a new 0600 file next to `path` and renames it over `path`. + fn write_atomically(store: &FileStore, profile: &str, path: &Path, bytes: &[u8]) -> Result<()> { + let temp = temp_path(store, profile)?; + let mut file = OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&temp) + .map_err(|e| io_failure("Cannot create", &temp, &e))?; + let mut guard = Temp { + path: &temp, + armed: true, + }; + // The umask can only remove bits from 0600; say 0600 whatever it was. + file.set_permissions(std::fs::Permissions::from_mode(0o600)) + .and_then(|()| file.write_all(bytes)) + .and_then(|()| file.sync_all()) + .map_err(|e| io_failure("Cannot write", &temp, &e))?; + drop(file); + std::fs::rename(&temp, path).map_err(|e| io_failure("Cannot replace", path, &e))?; + guard.armed = false; + sync_dir(&store.dir); + Ok(()) + } + + /// Makes a rename or removal durable. Best effort: not every file system allows it. + fn sync_dir(dir: &Path) { + if let Ok(handle) = File::open(dir) { + let _ = handle.sync_all(); + } + } + + pub(super) fn save(store: &FileStore, profile: &str, token: &StoredToken) -> Result<()> { + dir(store, true)?; + let path = store.path_for(profile); + // An existing file is judged before it is replaced, even though it will not be read. + if let Some(meta) = look(&path)? { + check(&path, &meta, false)?; + } + let record = Record { + profile: profile.to_owned(), + payload: token.to_payload(), + }; + let mut text = serde_json::to_string(&record) + .map_err(|e| Failure::general(format!("cannot encode the sign-in: {e}")))?; + text.push('\n'); + write_atomically(store, profile, &path, text.as_bytes()) + } + + pub(super) fn delete(store: &FileStore, profile: &str) -> Result { + if dir(store, false)?.is_none() { + return Ok(false); + } + let path = store.path_for(profile); + let Some(meta) = look(&path)? else { + return Ok(false); + }; + check(&path, &meta, false)?; + match std::fs::remove_file(&path) { + Ok(()) => { + sync_dir(&store.dir); + Ok(true) + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(e) => Err(io_failure("Cannot remove", &path, &e)), + } + } + + /// Proves the store can be used: the directory exists with the right owner and mode (it is + /// created if absent), the profile's file, if any, is trustworthy, and an empty file can be + /// created and removed there. No token is written. + pub(super) fn check_available(store: &FileStore, profile: &str) -> Result<()> { + dir(store, true)?; + let path = store.path_for(profile); + if let Some(meta) = look(&path)? { + check(&path, &meta, false)?; + } + let probe = temp_path(store, profile)?; + OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&probe) + .map_err(|e| { + Failure::general(format!( + "the plain-text token store cannot be used: cannot write in {}: {e}", + store.dir.display() + )) + .hint("Use the system keychain or a tenant API key in HODEISHIELD_API_KEY.") + })?; + let _ = std::fs::remove_file(&probe); + if let Some(fs) = super::network_file_system(&store.dir) { + eprintln!( + "Warning: {} is on {fs}. A file there can be read by the server's administrator \ + and by root on other machines that mount it; use a local directory \ + (XDG_STATE_HOME) if that matters.", + store.dir.display() + ); + } + Ok(()) + } +} + +/// The name of the network file system `path` is on, when it can be told cheaply and reliably. +/// Only Linux's `statfs` magic numbers are used; elsewhere nothing is detected. +#[cfg(all(unix, target_os = "linux"))] +fn network_file_system(path: &Path) -> Option<&'static str> { + let stat = rustix::fs::statfs(path).ok()?; + network_file_system_for(stat.f_type as u64 & 0xFFFF_FFFF) +} + +#[cfg(all(unix, not(target_os = "linux")))] +fn network_file_system(_: &Path) -> Option<&'static str> { + None +} + +/// Linux `statfs` `f_type` values of file systems served by another machine. +#[cfg(any(test, all(unix, target_os = "linux")))] +fn network_file_system_for(f_type: u64) -> Option<&'static str> { + match f_type { + 0x6969 => Some("NFS"), + 0xFF53_4D42 | 0xFE53_4D42 => Some("SMB/CIFS"), + _ => None, + } +} + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use secrecy::{ExposeSecret, SecretString}; + use std::os::unix::fs::{MetadataExt, PermissionsExt, symlink}; + + fn token() -> StoredToken { + StoredToken { + access_token: SecretString::from("at_secret"), + refresh_token: Some(SecretString::from("rt_secret")), + expires_at: Some(1_000), + scope: Some("read".to_owned()), + issuer: "https://app.example.test".to_owned(), + api_url: "https://api.example.test".to_owned(), + client_id: "cli".to_owned(), + } + } + + /// A store in `/sessions`, which does not exist yet. + fn store() -> (tempfile::TempDir, FileStore) { + let tmp = tempfile::tempdir().expect("tempdir"); + let store = FileStore::in_dir(tmp.path().join("sessions")); + (tmp, store) + } + + fn mode(path: &Path) -> u32 { + std::fs::metadata(path).expect("meta").mode() & 0o777 + } + + fn refused_with(result: Result, needle: &str) { + let err = result.expect_err("refused"); + assert!( + err.message.starts_with(TOKEN_FILE_REFUSED) && err.message.contains(needle), + "{}", + err.message + ); + assert!(err.hint.is_some()); + } + + fn saved() -> (tempfile::TempDir, FileStore) { + let (tmp, store) = store(); + store.save("p", &token()).expect("save"); + (tmp, store) + } + + #[test] + fn a_session_round_trips_with_private_modes() { + let (_tmp, store) = saved(); + let back = store.load("p").expect("load").expect("stored"); + assert_eq!(back.access_token.expose_secret(), "at_secret"); + assert_eq!(back.refresh_token.expect("rt").expose_secret(), "rt_secret"); + assert_eq!(mode(&store.dir), 0o700); + assert_eq!(mode(&store.path_for("p")), 0o600); + assert!(store.load("other").expect("load").is_none()); + } + + #[test] + fn an_atomic_write_leaves_no_temporary_file_and_replaces_the_old_one() { + let (_tmp, store) = saved(); + let newer = StoredToken { + access_token: SecretString::from("at_newer"), + ..token() + }; + store.save("p", &newer).expect("save again"); + let names: Vec<_> = std::fs::read_dir(&store.dir) + .expect("dir") + .map(|e| e.expect("entry").file_name()) + .collect(); + assert_eq!(names, [std::ffi::OsString::from(file_name("p"))]); + assert_eq!(mode(&store.path_for("p")), 0o600); + let back = store.load("p").expect("load").expect("stored"); + assert_eq!(back.access_token.expose_secret(), "at_newer"); + } + + #[test] + fn a_refused_target_leaves_no_temporary_file() { + let (_tmp, store) = saved(); + std::fs::create_dir(store.path_for("q")).expect("blocker"); + refused_with(store.save("q", &token()), "not a regular file"); + let leftovers: Vec<_> = std::fs::read_dir(&store.dir) + .expect("dir") + .filter_map(|e| e.ok().map(|e| e.file_name().to_string_lossy().into_owned())) + .filter(|n| n.ends_with(".tmp")) + .collect(); + assert!(leftovers.is_empty(), "{leftovers:?}"); + assert!(store.load("p").expect("load").is_some()); + } + + #[test] + fn a_symlinked_file_is_refused_on_every_operation() { + let (tmp, store) = saved(); + let real = tmp.path().join("real.json"); + std::fs::copy(store.path_for("p"), &real).expect("copy"); + std::fs::remove_file(store.path_for("p")).expect("rm"); + symlink(&real, store.path_for("p")).expect("link"); + refused_with(store.load("p"), "symbolic link"); + refused_with(store.save("p", &token()), "symbolic link"); + refused_with(store.delete("p"), "symbolic link"); + refused_with(store.check_available("p"), "symbolic link"); + // Left as it was. + assert!( + std::fs::symlink_metadata(store.path_for("p")) + .expect("meta") + .is_symlink() + ); + assert!(real.exists()); + } + + #[test] + fn a_symlinked_directory_is_refused() { + let (tmp, store) = store(); + let elsewhere = tmp.path().join("elsewhere"); + std::fs::create_dir(&elsewhere).expect("dir"); + std::fs::set_permissions(&elsewhere, std::fs::Permissions::from_mode(0o700)) + .expect("chmod"); + symlink(&elsewhere, &store.dir).expect("link"); + refused_with(store.load("p"), "symbolic link"); + refused_with(store.save("p", &token()), "symbolic link"); + refused_with(store.delete("p"), "symbolic link"); + refused_with(store.check_available("p"), "symbolic link"); + assert_eq!(std::fs::read_dir(&elsewhere).expect("dir").count(), 0); + } + + #[test] + fn a_file_open_to_group_or_others_is_refused() { + for bits in [0o644, 0o640, 0o604, 0o660] { + let (_tmp, store) = saved(); + let path = store.path_for("p"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(bits)).expect("chmod"); + refused_with(store.load("p"), "accessible to other users"); + refused_with(store.save("p", &token()), "accessible to other users"); + refused_with(store.delete("p"), "accessible to other users"); + refused_with(store.check_available("p"), "accessible to other users"); + // Never repaired. + assert_eq!(mode(&path), bits); + } + } + + #[test] + fn a_directory_open_to_group_or_others_is_refused() { + for bits in [0o755, 0o750, 0o705] { + let (_tmp, store) = saved(); + std::fs::set_permissions(&store.dir, std::fs::Permissions::from_mode(bits)) + .expect("chmod"); + refused_with(store.load("p"), "must be 700"); + refused_with(store.save("p", &token()), "must be 700"); + refused_with(store.delete("p"), "must be 700"); + refused_with(store.check_available("p"), "must be 700"); + assert_eq!(mode(&store.dir), bits); + std::fs::set_permissions(&store.dir, std::fs::Permissions::from_mode(0o700)) + .expect("chmod back"); + } + } + + #[test] + fn the_owner_is_compared_with_the_effective_user() { + // Another user's files cannot be made in a test, so the judgement is exercised with the + // effective uid replaced: the same file is then someone else's. + let (_tmp, store) = saved(); + let meta = std::fs::symlink_metadata(store.path_for("p")).expect("meta"); + let me = meta.uid(); + assert!(unix::problem(&meta, false, me).is_none()); + let (reason, _) = unix::problem(&meta, false, me.wrapping_add(1)).expect("another owner"); + assert!(reason.contains("belongs to another user"), "{reason}"); + let dir_meta = std::fs::symlink_metadata(&store.dir).expect("meta"); + assert!(unix::problem(&dir_meta, true, me.wrapping_add(1)).is_some()); + assert_eq!(rustix::process::geteuid().as_raw(), me); + } + + #[test] + fn something_that_is_not_a_regular_file_is_refused() { + let (_tmp, store) = saved(); + std::fs::remove_file(store.path_for("p")).expect("rm"); + std::fs::create_dir(store.path_for("p")).expect("dir"); + refused_with(store.load("p"), "not a regular file"); + } + + #[test] + fn a_corrupt_file_is_an_error_that_names_it() { + let (_tmp, store) = saved(); + std::fs::write(store.path_for("p"), "not json").expect("write"); + let err = store.load("p").expect_err("corrupt"); + assert!(err.message.contains("is unreadable"), "{}", err.message); + assert!(!err.message.starts_with(TOKEN_FILE_REFUSED)); + assert!(!err.message.contains("not json")); + } + + #[test] + fn a_file_for_another_profile_is_not_accepted() { + let (_tmp, store) = saved(); + std::fs::copy(store.path_for("p"), store.path_for("q")).expect("copy"); + let err = store.load("q").expect_err("wrong profile"); + assert!(err.message.contains("is unreadable"), "{}", err.message); + } + + #[test] + fn two_profiles_are_two_files_and_odd_names_stay_inside() { + let (_tmp, store) = store(); + for name in ["default", "client-b", "../../escape", "a/b", ""] { + store.save(name, &token()).expect("save"); + } + let names: Vec = std::fs::read_dir(&store.dir) + .expect("dir") + .map(|e| e.expect("entry").file_name().to_string_lossy().into_owned()) + .collect(); + assert_eq!(names.len(), 5, "{names:?}"); + assert!( + names.iter().all(|n| n.len() == 69 && n.ends_with(".json")), + "{names:?}" + ); + assert!(store.load("../../escape").expect("load").is_some()); + assert!(store.delete("client-b").expect("delete")); + assert!(store.load("default").expect("load").is_some()); + assert!(store.load("client-b").expect("load").is_none()); + } + + #[test] + fn delete_removes_the_file_and_says_whether_there_was_one() { + let (_tmp, store) = saved(); + assert!(store.delete("p").expect("delete")); + assert!(!store.path_for("p").exists()); + assert!(!store.delete("p").expect("again")); + let (_tmp, never) = self::store(); + assert!(!never.delete("p").expect("no directory")); + assert!(!never.dir.exists(), "deleting creates nothing"); + } + + #[test] + fn checking_availability_writes_no_token_and_leaves_no_file() { + let (_tmp, store) = store(); + store.check_available("p").expect("available"); + assert_eq!(mode(&store.dir), 0o700); + assert_eq!(std::fs::read_dir(&store.dir).expect("dir").count(), 0); + assert!(store.load("p").expect("load").is_none()); + } + + #[test] + fn reading_creates_nothing() { + let (_tmp, store) = store(); + assert!(store.load("p").expect("load").is_none()); + assert!(!store.dir.exists()); + } + + #[test] + fn network_file_systems_are_recognised_by_their_magic_number() { + assert_eq!(network_file_system_for(0x6969), Some("NFS")); + assert_eq!(network_file_system_for(0xFF53_4D42), Some("SMB/CIFS")); + assert_eq!(network_file_system_for(0xEF53), None); + } + + #[test] + fn the_state_directory_is_not_the_configuration_directory() { + let dir = sessions_dir().expect("dir"); + assert!(dir.ends_with("sessions")); + assert!( + !dir.to_string_lossy().contains("/.config/"), + "{}", + dir.display() + ); + } +} diff --git a/crates/hodeishield-cli/src/auth/mod.rs b/crates/hodeishield-cli/src/auth/mod.rs index b7a7acd..54dafea 100644 --- a/crates/hodeishield-cli/src/auth/mod.rs +++ b/crates/hodeishield-cli/src/auth/mod.rs @@ -3,6 +3,7 @@ //! Credentials: a tenant API key from the environment, or a sign-in token from the keychain. +pub mod file_store; pub mod loopback; pub mod oauth; pub mod store; @@ -96,6 +97,15 @@ pub fn not_signed_in(settings: &Settings) -> Failure { /// Windows): it is "not authenticated" (exit code 3), with a pointer to the API key. Any other /// store failure (a corrupt entry, say) passes through unchanged: it already says what happened. pub fn explain_store_failure(failure: Failure) -> Failure { + if failure.message.starts_with(file_store::TOKEN_FILE_REFUSED) { + // A token file that is refused is not a credential: "not authenticated", with the store's + // own hint, which already names the API key. + let refused = Failure::new(Kind::NotAuthenticated, failure.message); + return match failure.hint { + Some(own) => refused.hint(own), + None => refused, + }; + } let Some(rest) = failure.message.strip_prefix(store::KEYCHAIN_UNAVAILABLE) else { return failure; }; @@ -216,10 +226,13 @@ pub fn issuer_url(token: &StoredToken) -> Result { } /// Whether a stored token is the one that would be sent, for `whoami`. -pub fn describe_source(source: CredentialSource) -> &'static str { - match source { - CredentialSource::ApiKey => "tenant API key (HODEISHIELD_API_KEY)", - CredentialSource::OAuth => "sign-in token in the system keychain", +pub fn describe_source(source: CredentialSource, file: Option<&std::path::Path>) -> String { + match (source, file) { + (CredentialSource::ApiKey, _) => "tenant API key (HODEISHIELD_API_KEY)".to_owned(), + (CredentialSource::OAuth, None) => "sign-in token in the system keychain".to_owned(), + (CredentialSource::OAuth, Some(path)) => { + format!("sign-in token in a plain-text file ({})", path.display()) + } } } @@ -529,7 +542,7 @@ mod tests { let hint = err.hint.expect("hint"); // The platform's own advice stays, followed by the remote-session one. assert!( - hint.starts_with("Sign-in tokens are only kept in the system keychain."), + hint.starts_with("Sign-in tokens are kept in the system keychain."), "{hint}" ); assert!( diff --git a/crates/hodeishield-cli/src/auth/store.rs b/crates/hodeishield-cli/src/auth/store.rs index 78edfb6..362bc93 100644 --- a/crates/hodeishield-cli/src/auth/store.rs +++ b/crates/hodeishield-cli/src/auth/store.rs @@ -1,10 +1,14 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Hodeitek S.L. -//! Where sign-in tokens are kept: the system keychain, and nowhere else. +//! Where sign-in tokens are kept: the system keychain, or, only when the person opts in, a +//! plain-text file. //! -//! macOS Keychain, Windows Credential Manager, or the Secret Service on Linux and the BSDs (GNOME -//! Keyring, KWallet). If none is available, signing in fails: there is no fallback to a file. +//! The keychain is the default: macOS Keychain, Windows Credential Manager, or the Secret Service +//! on Linux and the BSDs (GNOME Keyring, KWallet). The other store is +//! [`file_store`](super::file_store), chosen by `HODEISHIELD_TOKEN_STORE=plaintext-file` or the +//! `token_store` setting. Neither is ever used as a fallback for the other: if the chosen one is +//! not available, signing in fails. use crate::failure::{Failure, Result}; use secrecy::{ExposeSecret, SecretString}; @@ -33,7 +37,7 @@ pub struct StoredToken { /// The keychain payload. Plain strings live only for the instant of (de)serialisation. #[derive(Serialize, Deserialize)] -struct Payload { +pub(super) struct Payload { v: u32, access_token: String, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -48,7 +52,7 @@ struct Payload { } impl StoredToken { - fn to_payload(&self) -> Payload { + pub(super) fn to_payload(&self) -> Payload { Payload { v: 1, access_token: self.access_token.expose_secret().to_owned(), @@ -64,7 +68,7 @@ impl StoredToken { } } - fn from_payload(payload: Payload) -> Self { + pub(super) fn from_payload(payload: Payload) -> Self { Self { access_token: SecretString::from(payload.access_token), refresh_token: payload.refresh_token.map(SecretString::from), @@ -92,12 +96,53 @@ pub trait TokenStore { /// Whether tokens can be kept for this profile, without writing anything. Sign-in checks it /// before it asks the person to approve anything. fn check_available(&self, profile: &str) -> Result<()>; + /// The file that holds the profile's tokens, for stores that are files. `None` for the + /// keychain. + fn location(&self, _profile: &str) -> Option { + None + } } /// The system keychain. #[derive(Debug, Default)] pub struct Keychain; +/// The store the settings choose, looked up when it is used: the environment and the +/// configuration file are read at that point, so a command that never needs a token never fails +/// on them. A value that is not a store, or a platform that cannot have it, is an error at the +/// first use, and nothing falls back to the other store. +#[derive(Debug, Default)] +pub struct Configured; + +impl Configured { + fn chosen(&self) -> Result> { + Ok(match crate::config::token_store()? { + crate::config::TokenStoreKind::Keychain => Box::new(Keychain), + crate::config::TokenStoreKind::PlaintextFile => { + Box::new(super::file_store::FileStore::platform()?) + } + }) + } +} + +impl TokenStore for Configured { + fn load(&self, profile: &str) -> Result> { + self.chosen()?.load(profile) + } + fn save(&self, profile: &str, token: &StoredToken) -> Result<()> { + self.chosen()?.save(profile, token) + } + fn delete(&self, profile: &str) -> Result { + self.chosen()?.delete(profile) + } + fn check_available(&self, profile: &str) -> Result<()> { + self.chosen()?.check_available(profile) + } + fn location(&self, profile: &str) -> Option { + self.chosen().ok()?.location(profile) + } +} + static STORE_READY: OnceLock> = OnceLock::new(); fn ensure_store() -> Result<()> { @@ -114,16 +159,20 @@ fn unavailable(reason: &str) -> Failure { /// What to do about a missing keychain, for the platform this build runs on. fn unavailable_hint() -> &'static str { if cfg!(target_os = "macos") { - "Sign-in tokens are only kept in the system keychain. Allow access when the Keychain \ - prompt appears (or in Keychain Access), or use a tenant API key in HODEISHIELD_API_KEY \ - instead." + "Sign-in tokens are kept in the system keychain. Allow access when the Keychain prompt \ + appears (or in Keychain Access), or use a tenant API key in HODEISHIELD_API_KEY \ + instead, or opt in to a plain-text token file with \ + HODEISHIELD_TOKEN_STORE=plaintext-file (tokens are then stored unencrypted in a file \ + only you can read)." } else if cfg!(windows) { - "Sign-in tokens are only kept in the system keychain. Check that Windows Credential \ - Manager is available, or use a tenant API key in HODEISHIELD_API_KEY instead." + "Sign-in tokens are kept in the system keychain. Check that Windows Credential Manager \ + is available, or use a tenant API key in HODEISHIELD_API_KEY instead." } else { - "Sign-in tokens are only kept in the system keychain. Start a Secret Service \ - (GNOME Keyring or KWallet); on a server without one, use a tenant API key in \ - HODEISHIELD_API_KEY instead." + "Sign-in tokens are kept in the system keychain. Start a Secret Service (GNOME Keyring \ + or KWallet); on a server without one, use a tenant API key in HODEISHIELD_API_KEY \ + instead, or opt in to a plain-text token file with \ + HODEISHIELD_TOKEN_STORE=plaintext-file (tokens are then stored unencrypted in a file \ + only you can read)." } } diff --git a/crates/hodeishield-cli/src/cli.rs b/crates/hodeishield-cli/src/cli.rs index 2f17fbb..da1106c 100644 --- a/crates/hodeishield-cli/src/cli.rs +++ b/crates/hodeishield-cli/src/cli.rs @@ -17,7 +17,8 @@ use std::str::FromStr; const AFTER_HELP: &str = "\ Credentials: HODEISHIELD_API_KEY A tenant API key. When set, it is used for every API call. - hodeishield login Otherwise, sign in to the app; the token is kept in the system keychain. + hodeishield login Otherwise, sign in to the app; the token is kept in the system keychain + (or, only if you opt in, in a plain-text file). Exit codes: 0 ok · 1 error · 2 usage · 3 not authenticated · 4 missing scope · 5 not found @@ -101,7 +102,10 @@ pub enum Command { const LOGIN_ABOUT: &str = "\ Sign in to the HodeiShield app. The token is kept in the system keychain (macOS Keychain, Windows \ -Credential Manager, Secret Service on Linux), never in a file, and is refreshed when it expires. +Credential Manager, Secret Service on Linux) and is refreshed when it expires. Only if you opt in \ +with HODEISHIELD_TOKEN_STORE=plaintext-file (Linux and macOS), it is kept instead in a file in your \ +state directory, unencrypted and readable only by you. The CLI never switches between the two on \ +its own. By default a browser window opens and the app redirects back to a one-shot listener on 127.0.0.1. \ With --device, you get a short code to type in on any other device instead. diff --git a/crates/hodeishield-cli/src/commands/config.rs b/crates/hodeishield-cli/src/commands/config.rs index 116e29b..e6c13de 100644 --- a/crates/hodeishield-cli/src/commands/config.rs +++ b/crates/hodeishield-cli/src/commands/config.rs @@ -29,6 +29,7 @@ pub fn run(ctx: &Context, command: ConfigCommand, out: &mut dyn Write) -> Result "oauth_client_id": settings.oauth_client_id, "oauth_scopes": settings.oauth_scopes, "update_check": update_check, + "token_store": settings.token_store.name(), }); if ctx.json { return Ok(print_json(out, &value)?); @@ -43,6 +44,7 @@ pub fn run(ctx: &Context, command: ConfigCommand, out: &mut dyn Write) -> Result writeln!(out, "oauth_client_id {}", settings.oauth_client_id)?; writeln!(out, "oauth_scopes {scopes}")?; writeln!(out, "update_check {update_check}")?; + writeln!(out, "token_store {}", settings.token_store.name())?; Ok(()) } ConfigCommand::Set { key, value } if key == config::UPDATE_CHECK => { @@ -52,6 +54,13 @@ pub fn run(ctx: &Context, command: ConfigCommand, out: &mut dyn Write) -> Result file.update_check = None; Ok(()) }), + ConfigCommand::Set { key, value } if key == config::TOKEN_STORE => { + edit_global(&path, |file| file.set_token_store(&value)) + } + ConfigCommand::Unset { key } if key == config::TOKEN_STORE => edit_global(&path, |file| { + file.token_store = None; + Ok(()) + }), ConfigCommand::Set { key, value } => edit(ctx, &path, |profile| profile.set(&key, &value)), ConfigCommand::Unset { key } => edit(ctx, &path, |profile| profile.unset(&key)), ConfigCommand::Use { name } => { diff --git a/crates/hodeishield-cli/src/commands/mod.rs b/crates/hodeishield-cli/src/commands/mod.rs index ad117b9..86e0af9 100644 --- a/crates/hodeishield-cli/src/commands/mod.rs +++ b/crates/hodeishield-cli/src/commands/mod.rs @@ -80,7 +80,7 @@ pub fn run(cli: Cli, out: &mut dyn Write) -> Result<()> { }, json: cli.json, verbose: cli.verbose, - store: Arc::new(auth::store::Keychain), + store: Arc::new(auth::store::Configured), }; match cli.command { Command::Vendors(command) => resources::vendors(&ctx, command, out), diff --git a/crates/hodeishield-cli/src/commands/session.rs b/crates/hodeishield-cli/src/commands/session.rs index f8c84e1..1303fa1 100644 --- a/crates/hodeishield-cli/src/commands/session.rs +++ b/crates/hodeishield-cli/src/commands/session.rs @@ -130,6 +130,13 @@ fn login_with( .ok() .flatten() .and_then(|info| user_label(&info)); + if let Some(path) = store.location(&settings.profile) { + eprintln!( + "The sign-in is stored unencrypted in {}, protected only by its permissions. \ + `hodeishield logout` revokes it and deletes the file.", + path.display() + ); + } eprintln!( "Signed in to {}{} (profile {}).", settings.app_url.host_str().unwrap_or("the app"), @@ -263,7 +270,7 @@ fn whoami_with( "Credential", source.map_or_else( || "none".to_owned(), - |s| auth::describe_source(s).to_owned(), + |s| auth::describe_source(s, store.location(&settings.profile).as_deref()), ), ), ]; diff --git a/crates/hodeishield-cli/src/config.rs b/crates/hodeishield-cli/src/config.rs index 71c3fb9..c5bf5a2 100644 --- a/crates/hodeishield-cli/src/config.rs +++ b/crates/hodeishield-cli/src/config.rs @@ -4,7 +4,8 @@ //! Configuration file and profiles. //! //! The file holds URLs and OAuth client settings per profile. It never holds a credential: tokens -//! live in the system keychain and API keys in the environment. +//! live in the system keychain (or, if the person opts in, in a separate token file) and API keys +//! in the environment. use crate::failure::{Failure, Result}; use serde::{Deserialize, Serialize}; @@ -28,6 +29,10 @@ pub struct ConfigFile { /// Whether to look for a newer release once a day. Global, not per profile. Unset: yes. #[serde(default, skip_serializing_if = "Option::is_none")] pub update_check: Option, + /// Where sign-in tokens are kept: `keychain` (unset means this) or `plaintext-file`. Global, + /// not per profile. `HODEISHIELD_TOKEN_STORE` takes precedence. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub token_store: Option, #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] pub profiles: BTreeMap, } @@ -49,20 +54,83 @@ pub struct Profile { pub oauth_scopes: Option>, } -/// The keys `config set` and `config unset` accept. `update_check` is global; the rest belong to a -/// profile. +/// The keys `config set` and `config unset` accept. `update_check` and `token_store` are global; +/// the rest belong to a profile. pub const KEYS: &[&str] = &[ "api_url", "app_url", "oauth_client_id", "oauth_scopes", UPDATE_CHECK, + TOKEN_STORE, ]; -/// The one global key. +/// A global key: whether to look for a newer release. pub const UPDATE_CHECK: &str = "update_check"; +/// A global key: where sign-in tokens are kept. +pub const TOKEN_STORE: &str = "token_store"; + +/// The environment variable that overrides [`TOKEN_STORE`]. +pub const TOKEN_STORE_ENV: &str = "HODEISHIELD_TOKEN_STORE"; + +/// Where sign-in tokens are kept. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub enum TokenStoreKind { + /// The system keychain. The default. + #[default] + Keychain, + /// A plain-text file only the user can read. Opt-in. + PlaintextFile, +} + +impl TokenStoreKind { + pub const fn name(self) -> &'static str { + match self { + Self::Keychain => "keychain", + Self::PlaintextFile => "plaintext-file", + } + } + + /// Parses a setting. `source` says where the value came from, for the error. + pub fn parse(value: &str, source: &str) -> Result { + match value.trim() { + "keychain" => Ok(Self::Keychain), + "plaintext-file" => Ok(Self::PlaintextFile), + other => Err(Failure::general(format!( + "{source}: `{}` is not a token store.", + crate::output::clean(other) + )) + .hint("Use `keychain` (the default) or `plaintext-file`.")), + } + } +} + +/// The token store in effect: the environment, then the configuration file, then the keychain. +pub fn token_store() -> Result { + let config = load(&path()?)?; + choose_token_store(&config, |name| std::env::var(name).ok()) +} + +fn choose_token_store( + config: &ConfigFile, + env: impl Fn(&str) -> Option, +) -> Result { + if let Some(value) = env(TOKEN_STORE_ENV).filter(|v| !v.trim().is_empty()) { + return TokenStoreKind::parse(&value, TOKEN_STORE_ENV); + } + match &config.token_store { + Some(value) => TokenStoreKind::parse(value, TOKEN_STORE), + None => Ok(TokenStoreKind::Keychain), + } +} + impl ConfigFile { + pub fn set_token_store(&mut self, value: &str) -> Result<()> { + self.token_store = Some(TokenStoreKind::parse(value, TOKEN_STORE)?.name().to_owned()); + Ok(()) + } + pub fn set_update_check(&mut self, value: &str) -> Result<()> { self.update_check = Some(match value.trim().to_ascii_lowercase().as_str() { "true" => true, @@ -179,7 +247,7 @@ pub fn save(path: &Path, config: &ConfigFile) -> Result<()> { .map_err(|e| Failure::general(format!("Cannot write the configuration: {e}.")))?; let body = format!( "# HodeiShield CLI configuration. It holds no credential: tokens live in the system\n\ - # keychain and API keys in HODEISHIELD_API_KEY.\n\n{text}" + # keychain (or the token file you opted in to) and API keys in HODEISHIELD_API_KEY.\n\n{text}" ); write_private(path, body.as_bytes()) .map_err(|e| Failure::general(format!("Cannot write {}: {e}.", path.display()))) @@ -212,6 +280,7 @@ pub struct Settings { pub app_url: Url, pub oauth_client_id: String, pub oauth_scopes: Option>, + pub token_store: TokenStoreKind, pub config_path: PathBuf, } @@ -272,6 +341,7 @@ pub fn resolve_with( app_url, oauth_client_id, oauth_scopes: profile.oauth_scopes, + token_store: choose_token_store(config, |name| env(name))?, config_path, }) } @@ -388,6 +458,69 @@ mod tests { assert_eq!(text.trim(), "update_check = false"); } + #[test] + fn the_token_store_is_the_keychain_unless_the_environment_or_the_file_says_otherwise() { + let none = |_: &str| None; + let from_env = |_: &str| Some("plaintext-file".to_owned()); + let mut config = ConfigFile::default(); + assert_eq!( + choose_token_store(&config, none).expect("default"), + TokenStoreKind::Keychain + ); + config.set_token_store(" plaintext-file ").expect("set"); + assert_eq!(config.token_store.as_deref(), Some("plaintext-file")); + assert_eq!( + choose_token_store(&config, none).expect("file"), + TokenStoreKind::PlaintextFile + ); + // The environment wins, in both directions; an empty variable does not count. + config.token_store = Some("keychain".to_owned()); + assert_eq!( + choose_token_store(&config, from_env).expect("env"), + TokenStoreKind::PlaintextFile + ); + config.token_store = Some("plaintext-file".to_owned()); + let keychain = |_: &str| Some("keychain".to_owned()); + assert_eq!( + choose_token_store(&config, keychain).expect("env"), + TokenStoreKind::Keychain + ); + let empty = |_: &str| Some(" ".to_owned()); + assert_eq!( + choose_token_store(&config, empty).expect("empty"), + TokenStoreKind::PlaintextFile + ); + } + + #[test] + fn any_other_token_store_is_an_error_naming_where_it_came_from() { + let mut config = ConfigFile::default(); + assert!(config.set_token_store("file").is_err()); + assert!(config.set_token_store("").is_err()); + assert!(config.token_store.is_none()); + config.token_store = Some("Keychain".to_owned()); + let err = choose_token_store(&config, |_| None).expect_err("not a store"); + assert!( + err.message.starts_with("token_store: `Keychain`"), + "{}", + err.message + ); + let err = choose_token_store(&ConfigFile::default(), |_| Some("file\u{1b}[2J".to_owned())) + .expect_err("not a store"); + assert!( + err.message.starts_with("HODEISHIELD_TOKEN_STORE:"), + "{}", + err.message + ); + assert!(!err.message.contains('\u{1b}')); + // Settings resolution reports it too, so no command runs on a misspelt setting. + let bad = ConfigFile { + token_store: Some("plain".to_owned()), + ..ConfigFile::default() + }; + assert!(resolve_with(&bad, PathBuf::new(), &Overrides::default(), no_env).is_err()); + } + #[test] fn unknown_keys_are_refused() { let mut profile = Profile::default(); diff --git a/crates/hodeishield-cli/tests/cli.rs b/crates/hodeishield-cli/tests/cli.rs index 18a8fe6..7b54192 100644 --- a/crates/hodeishield-cli/tests/cli.rs +++ b/crates/hodeishield-cli/tests/cli.rs @@ -33,6 +33,7 @@ impl Env { "HODEISHIELD_APP_URL", "HODEISHIELD_PROFILE", "HODEISHIELD_OAUTH_CLIENT_ID", + "HODEISHIELD_TOKEN_STORE", ] { cmd.env_remove(var); } @@ -41,10 +42,38 @@ impl Env { "DBUS_SESSION_BUS_ADDRESS", "unix:path=/nonexistent/hodeishield-test", ) + .env("XDG_STATE_HOME", self.state()) .env("NO_COLOR", "1"); cmd } + /// Where the state directory is for this test. + fn state(&self) -> PathBuf { + self._dir.path().join("state") + } + + /// The binary with the opt-in plain-text token store, and a home that is the test's own, so + /// the platform's state directory is inside the test's directory on every system. + #[cfg(unix)] + fn cmd_file(&self) -> Command { + let mut cmd = self.cmd(); + cmd.env("HODEISHIELD_TOKEN_STORE", "plaintext-file") + .env("HOME", self._dir.path().join("home")); + cmd + } + + /// The sessions directory of the opt-in store, where the platform keeps it. + #[cfg(unix)] + fn sessions(&self) -> PathBuf { + if cfg!(target_os = "macos") { + self._dir + .path() + .join("home/Library/Application Support/com.Hodeitek.HodeiShield/sessions") + } else { + self.state().join("hodeishield/sessions") + } + } + fn api(&self, server: &mockito::Server) -> Command { let mut cmd = self.cmd(); cmd.env("HODEISHIELD_API_KEY", KEY) @@ -771,3 +800,452 @@ fn free_form_filters_are_sent_as_typed_without_a_warning() { mock.assert(); } } + +/// The opt-in plain-text token store, end to end. Unix only: Windows refuses it (last test). +#[cfg(unix)] +mod file_store { + use super::*; + use serde_json::json; + use std::os::unix::fs::{PermissionsExt, symlink}; + use std::sync::{Arc, Mutex}; + + /// An app that offers the device flow and issues `at-new` / `rt-new`; revocation requests are + /// recorded in arrival order. + struct App { + api: mockito::ServerGuard, + app: mockito::ServerGuard, + revoked: Arc>>, + _mocks: Vec, + } + + fn app() -> App { + let mut api = mockito::Server::new(); + let mut app = mockito::Server::new(); + let revoked = Arc::new(Mutex::new(Vec::new())); + let seen = Arc::clone(&revoked); + let url = app.url(); + let mocks = vec![ + api.mock("GET", "/.well-known/oauth-protected-resource") + .with_status(404) + .create(), + app.mock("GET", "/.well-known/oauth-authorization-server") + .with_body( + json!({ + "issuer": url, + "token_endpoint": format!("{url}/token"), + "device_authorization_endpoint": format!("{url}/device/code"), + "revocation_endpoint": format!("{url}/revoke"), + "scopes_supported": ["offline_access"], + }) + .to_string(), + ) + .create(), + app.mock("POST", "/device/code") + .with_body( + json!({ + "device_code": "dc", "user_code": "ABCD-EFGH", + "verification_uri": format!("{url}/device"), + "expires_in": 600, "interval": 1, + }) + .to_string(), + ) + .create(), + app.mock("POST", "/token") + .with_body( + json!({ + "access_token": "at-new", "refresh_token": "rt-new", + "token_type": "Bearer", "expires_in": 3600, + }) + .to_string(), + ) + .create(), + app.mock("POST", "/revoke") + .with_body_from_request(move |request| { + let form: Vec<(String, String)> = + url::form_urlencoded::parse(request.body().expect("body")) + .into_owned() + .collect(); + let get = |key: &str| { + form.iter() + .find(|(k, _)| k == key) + .map(|(_, v)| v.clone()) + .unwrap_or_default() + }; + seen.lock().expect("lock").push(format!( + "{}={}", + get("token_type_hint"), + get("token") + )); + Vec::new() + }) + .create(), + ]; + App { + api, + app, + revoked, + _mocks: mocks, + } + } + + impl App { + fn with(&self, mut cmd: Command) -> Command { + cmd.env("HODEISHIELD_API_URL", self.api.url()) + .env("HODEISHIELD_APP_URL", self.app.url()); + cmd + } + } + + fn mode(path: &std::path::Path) -> u32 { + use std::os::unix::fs::MetadataExt; + std::fs::metadata(path).expect("metadata").mode() & 0o777 + } + + /// The files in the sessions directory. + fn kept(env: &Env) -> Vec { + let mut found: Vec = std::fs::read_dir(env.sessions()) + .map(|dir| dir.map(|e| e.expect("entry").path()).collect()) + .unwrap_or_default(); + found.sort(); + found + } + + fn signed_in(env: &Env, app: &App) -> PathBuf { + app.with(env.cmd_file()) + .args(["login", "--device"]) + .assert() + .success(); + let files = kept(env); + assert_eq!(files.len(), 1, "{files:?}"); + files[0].clone() + } + + fn all_output(output: &std::process::Output) -> String { + format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ) + } + + #[test] + fn login_whoami_and_logout_use_a_private_file_and_never_show_a_token() { + let env = Env::new(); + let app = app(); + let login = app + .with(env.cmd_file()) + .args(["login", "--device"]) + .output() + .expect("run"); + assert!(login.status.success(), "{}", all_output(&login)); + assert!(login.stdout.is_empty()); + assert!( + String::from_utf8_lossy(&login.stderr).contains("stored unencrypted"), + "{}", + all_output(&login) + ); + + let files = kept(&env); + assert_eq!(files.len(), 1, "{files:?}"); + let file = &files[0]; + assert_eq!(mode(file), 0o600); + assert_eq!(mode(&env.sessions()), 0o700); + assert!(file.to_string_lossy().ends_with(".json")); + let text = std::fs::read_to_string(file).expect("file"); + assert!(text.contains("at-new") && text.contains("rt-new")); + + let whoami = app + .with(env.cmd_file()) + .arg("whoami") + .output() + .expect("run"); + assert!(whoami.status.success(), "{}", all_output(&whoami)); + let shown = all_output(&whoami); + assert!(shown.contains("plain-text file"), "{shown}"); + assert!( + !shown.contains("at-new") && !shown.contains("rt-new"), + "{shown}" + ); + + let logout = app + .with(env.cmd_file()) + .arg("logout") + .output() + .expect("run"); + assert!(logout.status.success(), "{}", all_output(&logout)); + assert_eq!( + *app.revoked.lock().expect("lock"), + ["refresh_token=rt-new", "access_token=at-new"] + ); + assert!(kept(&env).is_empty()); + assert!(!all_output(&logout).contains("-new")); + } + + #[test] + fn two_profiles_are_kept_in_two_files() { + let env = Env::new(); + let app = app(); + env.cmd() + .args(["config", "set", "--profile", "client-b", "api_url"]) + .arg(app.api.url()) + .assert() + .success(); + signed_in(&env, &app); + app.with(env.cmd_file()) + .args(["--profile", "client-b", "login", "--device"]) + .assert() + .success(); + assert_eq!(kept(&env).len(), 2); + app.with(env.cmd_file()) + .args(["--profile", "client-b", "logout"]) + .assert() + .success(); + assert_eq!(kept(&env).len(), 1); + app.with(env.cmd_file()).arg("whoami").assert().success(); + } + + #[test] + fn a_corrupt_file_is_an_error_that_names_it() { + let env = Env::new(); + let app = app(); + let file = signed_in(&env, &app); + std::fs::write(&file, "{ not json").expect("corrupt it"); + for args in [&["whoami"][..], &["vendors", "list"][..]] { + app.with(env.cmd_file()) + .args(args) + .assert() + .code(1) + .stdout(predicate::str::is_empty()) + .stderr(predicate::str::contains("is unreadable")) + .stderr(predicate::str::contains(file.to_string_lossy().as_ref())); + } + } + + /// Every way the store refuses a file or directory: signing in is refused before the app is + /// asked for anything (exit 1), commands that need a credential are "not authenticated" + /// (exit 3), and nothing is repaired or deleted. + fn assert_refused(env: &Env, app: &App, reason: &str) { + let mut login_api = mockito::Server::new(); + let mut login_app = mockito::Server::new(); + let calls: Vec<_> = [&mut login_api, &mut login_app] + .into_iter() + .flat_map(|server| { + ["GET", "POST"].map(|method| { + server + .mock(method, mockito::Matcher::Any) + .expect(0) + .create() + }) + }) + .collect(); + env.cmd_file() + .env("HODEISHIELD_API_URL", login_api.url()) + .env("HODEISHIELD_APP_URL", login_app.url()) + .args(["login", "--device"]) + .assert() + .code(1) + .stdout(predicate::str::is_empty()) + .stderr(predicate::str::contains( + "plain-text token store cannot be used", + )) + .stderr(predicate::str::contains(reason)); + for call in &calls { + call.assert(); + } + for args in [&["whoami"][..], &["vendors", "list"][..]] { + app.with(env.cmd_file()) + .args(args) + .assert() + .code(3) + .stdout(predicate::str::is_empty()) + .stderr(predicate::str::contains(reason)); + } + // Nothing was sent to the app that signed in, and logout does not revoke what it cannot + // trust. + app.revoked.lock().expect("lock").clear(); + app.with(env.cmd_file()) + .arg("logout") + .assert() + .code(1) + .stderr(predicate::str::contains(reason)); + assert!(app.revoked.lock().expect("lock").is_empty()); + } + + #[test] + fn a_file_open_to_group_or_others_is_refused_and_left_alone() { + for bits in [0o644, 0o640] { + let env = Env::new(); + let app = app(); + let file = signed_in(&env, &app); + std::fs::set_permissions(&file, std::fs::Permissions::from_mode(bits)).expect("chmod"); + assert_refused(&env, &app, "accessible to other users"); + assert_eq!(mode(&file), bits); + } + } + + #[test] + fn a_directory_open_to_group_or_others_is_refused_and_left_alone() { + let env = Env::new(); + let app = app(); + signed_in(&env, &app); + std::fs::set_permissions(env.sessions(), std::fs::Permissions::from_mode(0o755)) + .expect("chmod"); + assert_refused(&env, &app, "accessible to other users"); + assert_eq!(mode(&env.sessions()), 0o755); + } + + #[test] + fn a_symlinked_file_is_refused_and_left_alone() { + let env = Env::new(); + let app = app(); + let file = signed_in(&env, &app); + let real = env.sessions().join("elsewhere"); + std::fs::rename(&file, &real).expect("move"); + symlink(&real, &file).expect("link"); + assert_refused(&env, &app, "symbolic link"); + assert!(std::fs::symlink_metadata(&file).expect("meta").is_symlink()); + assert!(real.exists()); + } + + #[test] + fn a_symlinked_directory_is_refused_and_left_alone() { + let env = Env::new(); + let app = app(); + signed_in(&env, &app); + let moved = env.state().join("moved"); + std::fs::rename(env.sessions(), &moved).expect("move"); + symlink(&moved, env.sessions()).expect("link"); + assert_refused(&env, &app, "symbolic link"); + assert!( + std::fs::symlink_metadata(env.sessions()) + .expect("meta") + .is_symlink() + ); + assert_eq!(std::fs::read_dir(&moved).expect("dir").count(), 1); + } + + #[test] + fn an_invalid_setting_is_an_error_before_anything_else_happens() { + let env = Env::new(); + let mut server = mockito::Server::new(); + let calls = server.mock("GET", mockito::Matcher::Any).expect(0).create(); + // Even with an API key, which would otherwise never look at the token store. + for value in ["file", "Keychain", "plaintext"] { + env.api(&server) + .env("HODEISHIELD_TOKEN_STORE", value) + .args(["vendors", "list"]) + .assert() + .code(1) + .stderr(predicate::str::contains("not a token store")) + .stderr(predicate::str::contains("plaintext-file")); + } + env.cmd() + .args(["config", "set", "token_store", "file"]) + .assert() + .code(1) + .stderr(predicate::str::contains("not a token store")); + std::fs::write(&env.config, "token_store = \"nope\"\n").expect("config"); + env.api(&server) + .args(["vendors", "list"]) + .assert() + .code(1) + .stderr(predicate::str::contains( + "token_store: `nope` is not a token store", + )); + calls.assert(); + assert!(!env.sessions().exists()); + } + + #[test] + fn the_setting_can_come_from_the_configuration_file_and_the_environment_wins() { + let env = Env::new(); + env.cmd() + .args(["config", "set", "token_store", "plaintext-file"]) + .assert() + .success(); + env.cmd() + .args(["config", "show"]) + .assert() + .success() + .stdout(predicate::str::contains("token_store plaintext-file")); + env.cmd() + .env("HODEISHIELD_TOKEN_STORE", "keychain") + .args(["config", "show"]) + .assert() + .success() + .stdout(predicate::str::contains("token_store keychain")); + let app = app(); + // From the file alone, and without the environment variable, the file store is used. + app.with(env.cmd()) + .env("HOME", env._dir.path().join("home")) + .args(["login", "--device"]) + .assert() + .success(); + assert_eq!(kept(&env).len(), 1); + env.cmd() + .args(["config", "unset", "token_store"]) + .assert() + .success(); + env.cmd() + .args(["config", "show"]) + .assert() + .success() + .stdout(predicate::str::contains("token_store keychain")); + } + + /// Only on Linux, where the tests make the Secret Service unreachable. + #[cfg(not(target_os = "macos"))] + #[test] + fn there_is_no_fallback_to_a_file_without_the_opt_in() { + let env = Env::new(); + let app = app(); + app.with(env.cmd()) + .args(["login", "--device"]) + .assert() + .code(1) + .stderr(predicate::str::contains("system keychain is not available")) + .stderr(predicate::str::contains( + "HODEISHIELD_TOKEN_STORE=plaintext-file", + )) + .stderr(predicate::str::contains("HODEISHIELD_API_KEY")); + assert!(!env.state().exists(), "no file or directory is made"); + // And the other way round: with the opt-in the keychain is never asked, which here would + // be an error. + signed_in(&env, &app); + } +} + +/// Windows has no mode bits to check, so the opt-in is refused there, before the app is asked for +/// anything. +#[cfg(windows)] +#[test] +fn the_plain_text_token_store_is_refused_on_windows() { + let env = Env::new(); + let mut api = mockito::Server::new(); + let mut app = mockito::Server::new(); + let calls: Vec<_> = [&mut api, &mut app] + .into_iter() + .flat_map(|server| { + ["GET", "POST"].map(|method| { + server + .mock(method, mockito::Matcher::Any) + .expect(0) + .create() + }) + }) + .collect(); + for args in [&["login"][..], &["login", "--device"][..]] { + env.cmd() + .env("HODEISHIELD_TOKEN_STORE", "plaintext-file") + .env("HODEISHIELD_API_URL", api.url()) + .env("HODEISHIELD_APP_URL", app.url()) + .args(args) + .assert() + .code(1) + .stdout(predicate::str::is_empty()) + .stderr(predicate::str::contains("not supported on Windows")); + } + for call in &calls { + call.assert(); + } +} From 15c481fb1a2cfe6ddaa3a34d0ecc932b594668cb Mon Sep 17 00:00:00 2001 From: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> Date: Sun, 11 Oct 2026 13:00:11 +0200 Subject: [PATCH 3/9] Document the plain-text token store Add a README section for servers, containers and remote sessions that puts API keys first and the opt-in file second with its trade-off, the new variable and setting in their tables, the network home directory note, and the exit code for a refused token file. Update the places that promised tokens are never in a file, and add the changelog entry. Refs #39 Signed-off-by: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> --- CHANGELOG.md | 12 +++++++++ CONTRIBUTING.md | 9 ++++--- README.md | 71 ++++++++++++++++++++++++++++++++++++++++++------- SECURITY.md | 3 ++- 4 files changed, 81 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e38822..044e23a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ commands, `--json` output and exit codes. Each release's notes start with its se ### Added +- An opt-in plain-text token store, for machines that have no system keychain (an SSH or jump host, + WSL without a desktop). Set `HODEISHIELD_TOKEN_STORE=plaintext-file`, or the global setting + `hodeishield config set token_store plaintext-file`, and `login --device` keeps the session in a + file only your user can read, one per profile, in the platform's state directory + (`$XDG_STATE_HOME/hodeishield/sessions/` on Linux). The keychain stays the default, nothing ever + falls back from one store to the other, and the file store is not available on Windows yet. Every + read and write refuses a file or directory that is a symbolic link, belongs to another user or is + open to group or others; files are replaced atomically with mode 600. `logout` revokes the tokens + and deletes the file. The README has a new section, "Servers, containers and remote sessions", + with the trade-off and the advice for containers and CI (a tenant API key); ADR 0001 records the + decision. A configuration file that contains `token_store` is refused by older CLIs. + ([#39](https://github.com/Hodeitek/hodeishield-cli/issues/39)) - Each release now includes a Windows on Arm build, `hodeishield--aarch64-pc-windows-msvc.zip`. It is built natively on a Windows on Arm runner, its `hodeishield.exe` is signed with Authenticode like the x64 one and checked to need no Visual C++ runtime, and the archive is listed in diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1456741..80ea4dc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -6,8 +6,9 @@ Thanks for helping. Bug reports, fixes and documentation improvements are welcom - **The CLI only reads.** Changes that write to the API will not be accepted; the `/v1` API is read-only anyway. -- **Credentials never leave the keychain or the environment.** No change may write a token or key to - a file, print it, or log it. Tests assert this; keep them passing. +- **Credentials stay in the keychain, the environment or the opt-in token file.** No change may write + a token or key anywhere else, print it, or log it, and nothing may fall back from one store to the + other. Tests assert this; keep them passing. - **Do not include real data** in issues, tests or examples: no real API keys, tokens, tenant names, hostnames other than the public `*.hodeishield.com`, or customer data. Use `example.com` / `example.test` and made-up values. @@ -105,8 +106,8 @@ implementation plugged into one. **Depth** is the behaviour available per unit o - **The interface is the test surface.** Test through the interface. A test that reaches past it pins the implementation and breaks on every refactor. - **One adapter is a hypothetical seam; two make it real.** Add a trait only when a second - implementation exists today, a test double included. `TokenStore` qualifies: the OS keychain and - the in-memory store used by tests both implement it. + implementation exists today, a test double included. `TokenStore` qualifies: the OS keychain, the + opt-in token file and the in-memory store used by tests all implement it. - **Locality.** Table, JSON and CSV rendering, including the sanitising of terminal control characters, lives in `output.rs`, so a change to it touches one module, not every command. diff --git a/README.md b/README.md index ffe8e0e..ee26b63 100644 --- a/README.md +++ b/README.md @@ -27,8 +27,8 @@ endpoint agents from a terminal or a script. - **Read-only.** Every command is a `GET` against the public `/v1` API. Nothing here can change data in your organisation. -- **Safe with credentials.** Sign-in tokens live in the system keychain, never in a file; nothing is - ever printed or logged; credentials only travel over HTTPS and are never replayed to a redirect. +- **Safe with credentials.** Sign-in tokens live in the system keychain unless you opt in to a + plain-text file for machines that have none; nothing is ever printed or logged; credentials only travel over HTTPS and are never replayed to a redirect. - **Built for scripts.** `--json` prints the API's own JSON, exit codes say what went wrong, and `HODEISHIELD_API_KEY` works without any interactive step. - **Verifiable.** Every release is signed with Sigstore (keyless, from GitHub Actions) and carries @@ -169,7 +169,8 @@ docker run --rm -e HODEISHIELD_API_KEY ghcr.io/hodeitek/hodeishield-cli:$v vendo `HODEISHIELD_API_KEY` is passed through from your environment (`-e NAME` without a value) so the key never appears on the command line. `hodeishield login` does not work in a container (there is no -browser and no system keychain): use a tenant API key, see [Authenticate](#authenticate). +browser and no system keychain): use a tenant API key, see +[Servers, containers and remote sessions](#servers-containers-and-remote-sessions). The image is signed by digest with cosign (keyless) and carries the release's CycloneDX SBOM as a signed attestation. Verify both before you rely on the image: @@ -297,8 +298,9 @@ hodeishield login --profile client-b A profile that has not been defined is refused (`Profile ... is not defined`). -`login` first checks that the system keychain is available and stops before opening the browser or -showing a code if it is not; use a tenant API key on such a machine. +`login` first checks that the selected token store is available and stops before opening the browser +or showing a code if it is not. On a machine without a keychain, see +[Servers, containers and remote sessions](#servers-containers-and-remote-sessions). With `--device`, open the address the CLI prints on any device and **type the code by hand**: the app does not accept a link with the code filled in, on purpose. @@ -309,8 +311,8 @@ framework the app offers; evidence; endpoints) and for a refresh token. To ask f The token is kept in the system keychain (macOS Keychain, Windows Credential Manager, or the Secret Service — GNOME Keyring, KWallet — on Linux) and refreshed automatically, also when the app ends an -access token early. Without a keychain, signing in is refused rather than falling back to a file: use -an API key there. +access token early. Without a keychain, signing in is refused rather than falling back to a file; +the file store below is an explicit choice, never a fallback. The keychain can also be out of reach in a session that has one on the machine, such as an SSH or other remote logon on Windows. With no API key set, commands then stop with exit code 3 and say that @@ -320,6 +322,53 @@ automated sessions. With the key set, the keychain is not read at all. To cut the CLI's access from elsewhere, revoke it in [the app](https://app.hodeishield.com) under **Account → Application access**. Signing out of the web app does not cut it. +### Servers, containers and remote sessions + +Where there is no desktop session (an SSH server, a container, WSL without a desktop, a CI runner) +there is usually no keychain either, and `hodeishield login` stops before it opens a browser or shows +a code. You have two options, in this order of preference: + +1. **A tenant API key** (see [With a tenant API key](#with-a-tenant-api-key-scripts-ci-servers)). + It works everywhere, needs no storage on the machine, can be limited to the read scopes you need + and is the right choice for containers and CI. Do not sign in during `docker build`, and never + commit or publish a container in which someone has signed in. +2. **A plain-text token file, if you want to sign in as yourself** on a machine you control, such as + a jump host. You opt in explicitly, and the trade-off is that the tokens are stored + **unencrypted** in a file that only your user can read. Anyone who can read your files, such as + `root`, a backup, a snapshot or a copy of your home directory, can use the refresh token until it + is revoked. + + ```sh + export HODEISHIELD_TOKEN_STORE=plaintext-file # or: hodeishield config set token_store plaintext-file + hodeishield login --device + hodeishield whoami + hodeishield logout # revokes the tokens, then deletes the file + ``` + + - **Where.** One file per profile in the sessions directory of your state directory: + `$XDG_STATE_HOME/hodeishield/sessions/` (by default `~/.local/state/hodeishield/sessions/`) on + Linux, and `~/Library/Application Support/com.Hodeitek.HodeiShield/sessions/` on macOS. The + configuration file never holds a token, and the file names do not reveal profile names. + - **Checks.** The directory is created with mode 700 and the files with mode 600, written by + replacing the file atomically. On every read and write the CLI refuses, and does not repair, a + file or directory that is a symbolic link, belongs to another user, or is open to group or + others. Commands then stop with exit code 3 (`login`: 1) and tell you what is wrong; fix it + yourself (`chmod`), and revoke the tokens in the app if someone else could have read them. + - **No fallback.** The setting must be the same on every command, and the CLI never switches + between the keychain and the file on its own. Tokens are not moved when you change it: sign out, + change the setting, sign in again. The values are `plaintext-file` and `keychain` (the default); + anything else is an error. + - **Network home directories.** Do not keep the state directory on NFS or another shared file + system: the file would be readable by the server's administrators and by `root` on other + machines that mount it, and its permissions are not enforced the same way. On Linux the CLI + warns on `login` when the directory is on NFS or SMB; it does not stop you. Point + `XDG_STATE_HOME` at a local directory instead. + - **Deleting is not revoking.** `logout` revokes the tokens at the app and then deletes the file, + but removing the file by hand, or restoring it from a backup, does not end the session. Only + revoking does. + - **Not on Windows yet.** Windows has no permission bits to check, so selecting the file store + there is refused with exit code 1, before anything is asked of the app. + ## Use ```sh @@ -380,7 +429,7 @@ hodeishield vendors list --all --csv > vendors.csv | 0 | Success | | 1 | Error (invalid input, configuration, unexpected answer) | | 2 | Invalid command line | -| 3 | Not authenticated: no credential (also when the system keychain cannot be reached in this session), or the API rejected it | +| 3 | Not authenticated: no credential (also when the system keychain cannot be reached in this session, or the opt-in token file is refused as unsafe), or the API rejected it | | 4 | The credential lacks the scope the command needs | | 5 | Not found (or not in your organisation) | | 6 | Rate limit exhausted (the CLI already retried short waits) | @@ -426,6 +475,7 @@ hodeishield config path # where the file is hodeishield --profile staging config set api_url https://api.example.test hodeishield config use staging # make it the default profile hodeishield config set update_check false # no new-version notice (global, not per profile) +hodeishield config set token_store plaintext-file # opt-in token file, see the section on servers hodeishield --profile default vendors list # or pick one per command ``` @@ -436,9 +486,12 @@ hodeishield --profile default vendors list # or pick one per command | `HODEISHIELD_API_URL` | API base URL | | `HODEISHIELD_APP_URL` | App base URL (sign-in) | | `HODEISHIELD_CONFIG` | Path of the configuration file | +| `HODEISHIELD_TOKEN_STORE` | Where sign-in tokens are kept: `keychain` (the default) or `plaintext-file`, an [opt-in](#servers-containers-and-remote-sessions); takes precedence over `token_store` in the configuration file | | `HODEISHIELD_NO_UPDATE_CHECK` | Any non-empty value turns off the [new-version notice](#new-version-notice) | -The configuration file holds URLs and sign-in settings only, never a credential. URLs must use HTTPS; +The configuration file holds URLs and sign-in settings only, never a credential. `update_check` and +`token_store` are global settings, not per profile. An older CLI refuses a configuration file that +contains `token_store`; use the environment variable there. URLs must use HTTPS; plain HTTP is accepted only for `localhost`. ## New-version notice diff --git a/SECURITY.md b/SECURITY.md index 4ede4b2..d15796a 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -24,7 +24,8 @@ Only the latest release receives fixes. ## What is in scope - Anything that could expose a credential: the tenant API key in `HODEISHIELD_API_KEY`, or the - sign-in tokens the CLI keeps in the system keychain. + sign-in tokens the CLI keeps in the system keychain or, if the user opted in, in its token file + (including the checks that refuse an unsafe file or directory). - Sending a credential anywhere other than the configured HodeiShield hosts (redirects, plain HTTP, crafted URLs or identifiers). - Output from the API reaching your terminal in a way that can run or hide commands. From 8a93e2092cb66f952b664c105423d790388772b6 Mon Sep 17 00:00:00 2001 From: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> Date: Sun, 11 Oct 2026 13:10:02 +0200 Subject: [PATCH 4/9] Clarify where the token files are and when commands refuse them Say that on macOS the sessions directory is a sibling of the application support folder, not inside the configuration folder, that the parents of the sessions directory must not be writable by other users, that the file names are a hash and not a way to hide a profile name, that running as root for a user's home does not work, and that a refused file makes login and logout exit 1 while other commands exit 3. Refs #39 Signed-off-by: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> --- CHANGELOG.md | 3 ++- README.md | 19 +++++++++++---- .../0001-sign-in-without-a-keychain.md | 24 ++++++++++++------- 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 044e23a..b52c98f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,8 @@ commands, `--json` output and exit codes. Each release's notes start with its se (`$XDG_STATE_HOME/hodeishield/sessions/` on Linux). The keychain stays the default, nothing ever falls back from one store to the other, and the file store is not available on Windows yet. Every read and write refuses a file or directory that is a symbolic link, belongs to another user or is - open to group or others; files are replaced atomically with mode 600. `logout` revokes the tokens + open to group or others (exit code 3 for commands that need a credential, 1 for `login` and + `logout`); files are replaced atomically with mode 600. `logout` revokes the tokens and deletes the file. The README has a new section, "Servers, containers and remote sessions", with the trade-off and the advice for containers and CI (a tenant API key); ADR 0001 records the decision. A configuration file that contains `token_store` is refused by older CLIs. diff --git a/README.md b/README.md index ee26b63..21b4040 100644 --- a/README.md +++ b/README.md @@ -347,13 +347,22 @@ a code. You have two options, in this order of preference: - **Where.** One file per profile in the sessions directory of your state directory: `$XDG_STATE_HOME/hodeishield/sessions/` (by default `~/.local/state/hodeishield/sessions/`) on - Linux, and `~/Library/Application Support/com.Hodeitek.HodeiShield/sessions/` on macOS. The - configuration file never holds a token, and the file names do not reveal profile names. + Linux, and `~/Library/Application Support/com.Hodeitek.HodeiShield.sessions/` on macOS (next to, + not inside, the folder that holds the configuration file). The configuration file never holds + a token, and the file names are a hash of the profile name, not the name itself (a hash of a + guessable name can be reversed, so do not treat it as hiding the profile). The parents of that + directory, such as your state directory, must not be writable by other users. - **Checks.** The directory is created with mode 700 and the files with mode 600, written by replacing the file atomically. On every read and write the CLI refuses, and does not repair, a file or directory that is a symbolic link, belongs to another user, or is open to group or - others. Commands then stop with exit code 3 (`login`: 1) and tell you what is wrong; fix it - yourself (`chmod`), and revoke the tokens in the app if someone else could have read them. + others. Commands that need a credential then stop with exit code 3 (`login` and `logout`: 1) and + tell you what is wrong; fix it yourself (`chmod`), and revoke the tokens in the app if someone + else could have read them. Temporary files left by a write that was interrupted are removed + the next time that profile is saved or signed out. + - **Do not run it as `root` or with `sudo` for your user.** A run as `root` that uses your home + directory can create root-owned directories there, which your own runs then refuse (and + `root` refuses yours, since they belong to another user). Sign in with the user that will run + the commands. - **No fallback.** The setting must be the same on every command, and the CLI never switches between the keychain and the file on its own. Tokens are not moved when you change it: sign out, change the setting, sign in again. The values are `plaintext-file` and `keychain` (the default); @@ -429,7 +438,7 @@ hodeishield vendors list --all --csv > vendors.csv | 0 | Success | | 1 | Error (invalid input, configuration, unexpected answer) | | 2 | Invalid command line | -| 3 | Not authenticated: no credential (also when the system keychain cannot be reached in this session, or the opt-in token file is refused as unsafe), or the API rejected it | +| 3 | Not authenticated: no credential (also when the system keychain cannot be reached in this session, or the opt-in token file is refused as unsafe; `login` and `logout` exit 1 for that), or the API rejected it | | 4 | The credential lacks the scope the command needs | | 5 | Not found (or not in your organisation) | | 6 | Rate limit exhausted (the CLI already retried short waits) | diff --git a/docs/decisions/0001-sign-in-without-a-keychain.md b/docs/decisions/0001-sign-in-without-a-keychain.md index 6cd9146..4691042 100644 --- a/docs/decisions/0001-sign-in-without-a-keychain.md +++ b/docs/decisions/0001-sign-in-without-a-keychain.md @@ -81,22 +81,30 @@ the following rules. Containers and CI keep using API keys. moved between stores: sign out, change the setting, sign in again. - **Location.** The platform's state directory, not the configuration directory: `$XDG_STATE_HOME/hodeishield/sessions/` (by default `~/.local/state/hodeishield/sessions/`) on - Linux, and the application support directory on macOS. One file per profile, named by the SHA-256 - of the profile name (names may contain `/` or `..`). The configuration file stays free of - credentials, so copying or syncing it never copies a token. + Linux, and on macOS a sibling of the application support folder + (`~/Library/Application Support/com.Hodeitek.HodeiShield.sessions/`), because there the + configuration and data directories are the same folder. One file per profile, named by the SHA-256 + of the profile name (names may contain `/` or `..`; the hash is not a way to hide a guessable + name). The configuration file stays free of credentials, so copying or syncing it never copies a + token. The parents of the sessions directory are not walked or checked, because that would refuse + legitimate setups; the README says they must not be writable by other users. - **Checks on every read and write.** The CLI refuses, and never repairs, a file or directory that is a symbolic link, is not owned by the current user, or has any group or other permission bit. It - creates the directory with mode 700 and the files with mode 600. The opened file is checked again - after it is opened, so a file swapped in between is refused. A corrupt file is an error that names + creates the directory with mode 700 and the files with mode 600. The directory is opened once + without following links and checked on the open handle; every later operation is relative to that + handle, so replacing the path afterwards cannot redirect it. Files are opened without following + links and without blocking, and checked again after they are opened. Running as `root` for a + user's home is not special-cased: a directory of another owner is refused in both directions. A corrupt file is an error that names it; a file written for another profile is not accepted. - **Atomic writes.** The CLI writes a new file created with mode 600 in the same directory, flushes it to disk, renames it over the target and flushes the directory. A temporary file is removed when - anything fails, and no token is ever in a file that others can read. + anything fails, and no token is ever in a file that others can read. Leftovers of a crash (a + temporary file of that profile older than a minute) are removed on the next save or logout. - **Behaviour of #101 is unchanged.** `login` checks that the selected store is usable before it opens a browser or shows a code. If saving fails after the exchange, the refresh token and then the access token are revoked and the command exits non-zero. `logout` revokes, then deletes the file. - No new exit codes: `login` failures are 1, other commands that need a credential and find the file - refused are 3, a corrupt file is 1. + No new exit codes: `login` and `logout` failures are 1, other commands that need a credential and + find the file refused are 3, a corrupt file is 1. - **Windows is deferred.** There are no permission bits to check, and setting and checking an ACL needs platform calls that the workspace's `unsafe_code = "forbid"` rules out without a vetted wrapper. Selecting the file store on Windows is refused with exit code 1, before anything is asked From e981c73f8626248132bab897b2c1e6d02a51f1dd Mon Sep 17 00:00:00 2001 From: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> Date: Sun, 11 Oct 2026 13:10:02 +0200 Subject: [PATCH 5/9] Harden the plain-text token store Keep the sessions outside the configuration directory on macOS, where the platform gives the same folder for both. Open the sessions directory once without following links, check it on the open handle and do every later operation relative to it, so replacing the path after the check cannot redirect a read, write or removal. Open files without following links and without blocking, remove stale temporary files that a crash could leave with live tokens, and choose the store once per process so a command checks and saves in the same one. State precisely in the message that group and others must have no access. Tests cover the temporary file guard, the leftovers, the opposite direction of the missing fallback, and that a refused file is left byte for byte as it was. Refs #39 Signed-off-by: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> --- crates/hodeishield-cli/src/auth/file_store.rs | 451 ++++++++++++------ crates/hodeishield-cli/src/auth/store.rs | 23 +- crates/hodeishield-cli/src/commands/mod.rs | 2 +- crates/hodeishield-cli/tests/cli.rs | 34 ++ 4 files changed, 356 insertions(+), 154 deletions(-) diff --git a/crates/hodeishield-cli/src/auth/file_store.rs b/crates/hodeishield-cli/src/auth/file_store.rs index 554b902..de84fd2 100644 --- a/crates/hodeishield-cli/src/auth/file_store.rs +++ b/crates/hodeishield-cli/src/auth/file_store.rs @@ -7,8 +7,8 @@ //! `HODEISHIELD_TOKEN_STORE=plaintext-file` or the global `token_store` setting says so. There is //! no fallback in either direction (see `docs/decisions/0001-sign-in-without-a-keychain.md`). //! -//! Files live in the platform's state directory (`$XDG_STATE_HOME/hodeishield/sessions/` on Linux), -//! not in the configuration directory, so a copied or synced configuration never carries a token. +//! Files live in the platform's state directory (`$XDG_STATE_HOME/hodeishield/sessions/` on Linux, a +//! sibling of the application support folder on macOS), not in the configuration directory, so a copied or synced configuration never carries a token. //! The file name is a hash of the profile name, which may contain `/` or `..`. //! //! On every read and write the CLI refuses, and never repairs, a file or directory that is a @@ -41,11 +41,20 @@ pub fn sessions_dir() -> Result { Failure::general("Cannot find a state directory for this user.") .hint("Use the system keychain or a tenant API key instead.") })?; - // `state_dir` exists on Linux and the BSDs; macOS has only the application support directory. - Ok(dirs - .state_dir() - .unwrap_or_else(|| dirs.data_local_dir()) - .join("sessions")) + if let Some(state) = dirs.state_dir() { + return Ok(state.join("sessions")); + } + // macOS has no state directory, and its configuration and data directories are the same one. + // A sibling of it, not a folder inside it, keeps the sessions out of the configuration + // directory, which people copy, sync and back up. + let data = dirs.data_local_dir(); + let name = data.file_name().map_or_else( + || "com.Hodeitek.HodeiShield".into(), + std::ffi::OsStr::to_os_string, + ); + let mut sibling = name; + sibling.push(".sessions"); + Ok(data.parent().unwrap_or(data).join(sibling)) } /// The file name for a profile: the hex SHA-256 of its name, so no profile name can leave the @@ -161,126 +170,159 @@ mod unix { use super::{FileStore, Record, io_failure, refused}; use crate::auth::store::StoredToken; use crate::failure::{Failure, Result}; - use std::fs::{File, Metadata, OpenOptions}; + use rustix::fs::{AtFlags, Dir, FileType, Mode, OFlags, Stat}; + use rustix::io::Errno; + use std::fs::File; use std::io::{Read, Write}; - use std::os::unix::fs::{DirBuilderExt, MetadataExt, OpenOptionsExt, PermissionsExt}; - use std::path::{Path, PathBuf}; + use std::os::fd::OwnedFd; + use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; + use std::path::Path; /// Biggest file read: a token payload is well under a kilobyte. const MAX_BYTES: u64 = 64 * 1024; + /// A temporary file this old is a leftover of a crashed write, not one being written. + const STALE_SECONDS: i64 = 60; + /// The effective user id of this process. fn euid() -> u32 { rustix::process::geteuid().as_raw() } - /// What is wrong with a file or directory, judged from its own metadata (never a followed - /// link): `None` when it can be trusted. - pub(super) fn problem(meta: &Metadata, want_dir: bool, euid: u32) -> Option<(String, String)> { - let kind = if want_dir { "directory" } else { "file" }; + fn io_error(errno: Errno) -> std::io::Error { + std::io::Error::from(errno) + } + + /// What is wrong with a file or directory, judged from its own `stat` (never a followed + /// link): `None` when it can be trusted. The second string says how to fix a mode. + pub(super) fn problem(stat: &Stat, want_dir: bool, euid: u32) -> Option<(String, String)> { + let kind = FileType::from_raw_mode(stat.st_mode); + let noun = if want_dir { "directory" } else { "file" }; let fix_mode = if want_dir { "700" } else { "600" }; - if meta.file_type().is_symlink() { + if kind == FileType::Symlink { return Some(( "is a symbolic link, which is never followed".into(), String::new(), )); } let right_type = if want_dir { - meta.is_dir() + kind == FileType::Directory } else { - meta.is_file() + kind == FileType::RegularFile }; if !right_type { - return Some((format!("is not a regular {kind}"), String::new())); + return Some((format!("is not a regular {noun}"), String::new())); } - if meta.uid() != euid { + if stat.st_uid != euid { return Some(( - format!("belongs to another user (uid {}), not to you", meta.uid()), + format!("belongs to another user (uid {}), not to you", stat.st_uid), String::new(), )); } - let mode = meta.mode() & 0o777; + let mode = Mode::from_raw_mode(stat.st_mode).bits() & 0o777; if mode & 0o077 != 0 { return Some(( - format!("is accessible to other users (mode {mode:03o}); it must be {fix_mode}"), format!( - "If you own it and nobody else read it, `chmod {fix_mode}` fixes it; a token that may have been read should be revoked from the app." + "is accessible to other users (mode {mode:03o}); group and others must have \ + no access" + ), + format!( + "If you own it and nobody else read it, `chmod {fix_mode}` fixes it; a token \ + that may have been read should be revoked from the app." ), )); } None } - fn check(path: &Path, meta: &Metadata, want_dir: bool) -> Result<()> { - match problem(meta, want_dir, euid()) { + fn check(path: &Path, stat: &Stat, want_dir: bool) -> Result<()> { + match problem(stat, want_dir, euid()) { None => Ok(()), Some((reason, fix)) => Err(refused(path, &reason, Some(fix).filter(|f| !f.is_empty()))), } } - /// Looks at a path without following a link at its end. `None` when it is absent. - fn look(path: &Path) -> Result> { - match std::fs::symlink_metadata(path) { - Ok(meta) => Ok(Some(meta)), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), - Err(e) => Err(io_failure("Cannot look at", path, &e)), + /// The sessions directory as an open handle, checked with `fstat` on the handle itself. Every + /// later operation is relative to it, so replacing the path afterwards cannot redirect them. + /// `None` when absent and `create` is false; created with mode 0700 otherwise. + pub(super) fn open_dir(store: &FileStore, create: bool) -> Result> { + let flags = OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC; + let mut created = false; + loop { + match rustix::fs::open(&store.dir, flags, Mode::empty()) { + Ok(fd) => { + let stat = rustix::fs::fstat(&fd) + .map_err(|e| io_failure("Cannot look at", &store.dir, &io_error(e)))?; + check(&store.dir, &stat, true)?; + return Ok(Some(fd)); + } + Err(Errno::NOENT) if create && !created => { + std::fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(&store.dir) + .map_err(|e| io_failure("Cannot create", &store.dir, &e))?; + created = true; + } + Err(Errno::NOENT) => return Ok(None), + Err(e @ (Errno::LOOP | Errno::NOTDIR)) => { + // A link or something that is not a directory: say which. + let stat = + rustix::fs::statat(rustix::fs::CWD, &store.dir, AtFlags::SYMLINK_NOFOLLOW) + .map_err(|e| io_failure("Cannot look at", &store.dir, &io_error(e)))?; + check(&store.dir, &stat, true)?; + return Err(io_failure("Cannot open", &store.dir, &io_error(e))); + } + Err(e) => return Err(io_failure("Cannot open", &store.dir, &io_error(e))), + } } } - /// The sessions directory, checked. `None` when absent and `create` is false; created with - /// mode 0700 otherwise. - fn dir(store: &FileStore, create: bool) -> Result> { - if let Some(meta) = look(&store.dir)? { - check(&store.dir, &meta, true)?; - return Ok(Some(())); - } - if !create { - return Ok(None); + /// Looks at an entry of the directory without following a link. `None` when it is absent. + fn look(dir: &OwnedFd, name: &str, path: &Path) -> Result> { + match rustix::fs::statat(dir, name, AtFlags::SYMLINK_NOFOLLOW) { + Ok(stat) => Ok(Some(stat)), + Err(Errno::NOENT) => Ok(None), + Err(e) => Err(io_failure("Cannot look at", path, &io_error(e))), } - std::fs::DirBuilder::new() - .recursive(true) - .mode(0o700) - .create(&store.dir) - .map_err(|e| io_failure("Cannot create", &store.dir, &e))?; - // Whatever the umask or a concurrent creator did, it is judged like an existing one. - let meta = look(&store.dir)?.ok_or_else(|| { - Failure::general(format!( - "{} disappeared while it was created.", - store.dir.display() - )) - })?; - check(&store.dir, &meta, true)?; - Ok(Some(())) } /// Opens a file for reading and judges the opened handle, which must be the very file that - /// was looked at (a swap in between is refused). - fn open_checked(path: &Path) -> Result> { - let Some(seen) = look(path)? else { + /// was looked at (a swap in between is refused). A link, a FIFO or a device is never opened + /// for long: `O_NOFOLLOW` refuses the first and `O_NONBLOCK` keeps the others from blocking. + fn open_checked(dir: &OwnedFd, name: &str, path: &Path) -> Result> { + let Some(seen) = look(dir, name, path)? else { return Ok(None); }; check(path, &seen, false)?; - let file = match File::open(path) { - Ok(file) => file, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), - Err(e) => return Err(io_failure("Cannot read", path, &e)), + let flags = OFlags::RDONLY | OFlags::NOFOLLOW | OFlags::NONBLOCK | OFlags::CLOEXEC; + let fd = match rustix::fs::openat(dir, name, flags, Mode::empty()) { + Ok(fd) => fd, + Err(Errno::NOENT) => return Ok(None), + Err(Errno::LOOP) => { + return Err(refused( + path, + "is a symbolic link, which is never followed", + None, + )); + } + Err(e) => return Err(io_failure("Cannot read", path, &io_error(e))), }; - let opened = file - .metadata() - .map_err(|e| io_failure("Cannot read", path, &e))?; - if (opened.dev(), opened.ino()) != (seen.dev(), seen.ino()) { + let opened = + rustix::fs::fstat(&fd).map_err(|e| io_failure("Cannot read", path, &io_error(e)))?; + if (opened.st_dev, opened.st_ino) != (seen.st_dev, seen.st_ino) { return Err(refused(path, "changed while it was being opened", None)); } check(path, &opened, false)?; - Ok(Some(file)) + Ok(Some(File::from(fd))) } pub(super) fn load(store: &FileStore, profile: &str) -> Result> { - if dir(store, false)?.is_none() { + let Some(dir) = open_dir(store, false)? else { return Ok(None); - } + }; let path = store.path_for(profile); - let Some(file) = open_checked(&path)? else { + let Some(file) = open_checked(&dir, &super::file_name(profile), &path)? else { return Ok(None); }; let mut text = String::new(); @@ -304,69 +346,120 @@ mod unix { Ok(Some(StoredToken::from_payload(record.payload))) } + /// The first 16 hex digits of the profile's hash: what its temporary files start with. + fn prefix(profile: &str) -> String { + super::file_name(profile)[..16].to_owned() + } + + fn temp_name(profile: &str) -> Result { + let mut random = [0u8; 8]; + getrandom::fill(&mut random) + .map_err(|e| Failure::general(format!("cannot get random bytes: {e}")))?; + let suffix: String = random.iter().map(|b| format!("{b:02x}")).collect(); + Ok(format!(".{}.{suffix}.tmp", prefix(profile))) + } + + /// Whether `name` is `..<16 hex digits>.tmp`, exactly what [`temp_name`] makes. + fn is_temp_of(name: &str, profile: &str) -> bool { + name.strip_prefix('.') + .and_then(|rest| rest.strip_prefix(prefix(profile).as_str())) + .and_then(|rest| rest.strip_prefix('.')) + .and_then(|rest| rest.strip_suffix(".tmp")) + .is_some_and(|random| { + random.len() == 16 + && random + .bytes() + .all(|b| matches!(b, b'0'..=b'9' | b'a'..=b'f')) + }) + } + + /// Removes the profile's temporary files left by a write that crashed between creating the + /// file and renaming it: they hold live tokens. Only regular files of this user that are old + /// enough not to be a write in progress by another process. Best effort. + fn remove_leftovers(dir: &OwnedFd, profile: &str) { + let Ok(entries) = Dir::read_from(dir) else { + return; + }; + let now = i64::try_from(crate::auth::now()).unwrap_or(i64::MAX); + let names: Vec = entries + .filter_map(|entry| entry.ok()) + .filter_map(|entry| entry.file_name().to_str().ok().map(str::to_owned)) + .filter(|name| is_temp_of(name, profile)) + .collect(); + for name in names { + let Ok(stat) = rustix::fs::statat(dir, name.as_str(), AtFlags::SYMLINK_NOFOLLOW) else { + continue; + }; + if FileType::from_raw_mode(stat.st_mode) == FileType::RegularFile + && stat.st_uid == euid() + && now.saturating_sub(stat.st_mtime) >= STALE_SECONDS + { + let _ = rustix::fs::unlinkat(dir, name.as_str(), AtFlags::empty()); + } + } + } + /// Removes the temporary file unless it was renamed into place. struct Temp<'a> { - path: &'a Path, + dir: &'a OwnedFd, + name: String, armed: bool, } impl Drop for Temp<'_> { fn drop(&mut self) { if self.armed { - let _ = std::fs::remove_file(self.path); + let _ = rustix::fs::unlinkat(self.dir, self.name.as_str(), AtFlags::empty()); } } } - fn temp_path(store: &FileStore, profile: &str) -> Result { - let mut random = [0u8; 8]; - getrandom::fill(&mut random) - .map_err(|e| Failure::general(format!("cannot get random bytes: {e}")))?; - let suffix: String = random.iter().map(|b| format!("{b:02x}")).collect(); - Ok(store.dir.join(format!( - ".{}.{suffix}.tmp", - &super::file_name(profile)[..16] - ))) - } - - /// Writes `bytes` to a new 0600 file next to `path` and renames it over `path`. - fn write_atomically(store: &FileStore, profile: &str, path: &Path, bytes: &[u8]) -> Result<()> { - let temp = temp_path(store, profile)?; - let mut file = OpenOptions::new() - .write(true) - .create_new(true) - .mode(0o600) - .open(&temp) - .map_err(|e| io_failure("Cannot create", &temp, &e))?; + fn create_new(dir: &OwnedFd, name: &str, path: &Path) -> Result { + let flags = + OFlags::WRONLY | OFlags::CREATE | OFlags::EXCL | OFlags::NOFOLLOW | OFlags::CLOEXEC; + rustix::fs::openat(dir, name, flags, Mode::RUSR | Mode::WUSR) + .map(File::from) + .map_err(|e| io_failure("Cannot create", path, &io_error(e))) + } + + /// Writes `bytes` to a new 0600 file in the directory and renames it over `name`. + pub(super) fn write_atomically( + store: &FileStore, + dir: &OwnedFd, + profile: &str, + bytes: &[u8], + ) -> Result<()> { + let temp = temp_name(profile)?; + let temp_path = store.dir.join(&temp); + let mut file = create_new(dir, &temp, &temp_path)?; let mut guard = Temp { - path: &temp, + dir, + name: temp.clone(), armed: true, }; // The umask can only remove bits from 0600; say 0600 whatever it was. file.set_permissions(std::fs::Permissions::from_mode(0o600)) .and_then(|()| file.write_all(bytes)) .and_then(|()| file.sync_all()) - .map_err(|e| io_failure("Cannot write", &temp, &e))?; + .map_err(|e| io_failure("Cannot write", &temp_path, &e))?; drop(file); - std::fs::rename(&temp, path).map_err(|e| io_failure("Cannot replace", path, &e))?; + let target = store.path_for(profile); + rustix::fs::renameat(dir, temp.as_str(), dir, super::file_name(profile).as_str()) + .map_err(|e| io_failure("Cannot replace", &target, &io_error(e)))?; guard.armed = false; - sync_dir(&store.dir); + // Makes the rename durable. Best effort: not every file system allows it. + let _ = rustix::fs::fsync(dir); Ok(()) } - /// Makes a rename or removal durable. Best effort: not every file system allows it. - fn sync_dir(dir: &Path) { - if let Ok(handle) = File::open(dir) { - let _ = handle.sync_all(); - } - } - pub(super) fn save(store: &FileStore, profile: &str, token: &StoredToken) -> Result<()> { - dir(store, true)?; + let dir = open_dir(store, true)? + .ok_or_else(|| Failure::general(format!("Cannot create {}.", store.dir.display())))?; + remove_leftovers(&dir, profile); let path = store.path_for(profile); // An existing file is judged before it is replaced, even though it will not be read. - if let Some(meta) = look(&path)? { - check(&path, &meta, false)?; + if let Some(stat) = look(&dir, &super::file_name(profile), &path)? { + check(&path, &stat, false)?; } let record = Record { profile: profile.to_owned(), @@ -375,25 +468,27 @@ mod unix { let mut text = serde_json::to_string(&record) .map_err(|e| Failure::general(format!("cannot encode the sign-in: {e}")))?; text.push('\n'); - write_atomically(store, profile, &path, text.as_bytes()) + write_atomically(store, &dir, profile, text.as_bytes()) } pub(super) fn delete(store: &FileStore, profile: &str) -> Result { - if dir(store, false)?.is_none() { + let Some(dir) = open_dir(store, false)? else { return Ok(false); - } + }; + remove_leftovers(&dir, profile); + let name = super::file_name(profile); let path = store.path_for(profile); - let Some(meta) = look(&path)? else { + let Some(stat) = look(&dir, &name, &path)? else { return Ok(false); }; - check(&path, &meta, false)?; - match std::fs::remove_file(&path) { + check(&path, &stat, false)?; + match rustix::fs::unlinkat(&dir, name.as_str(), AtFlags::empty()) { Ok(()) => { - sync_dir(&store.dir); + let _ = rustix::fs::fsync(&dir); Ok(true) } - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(false), - Err(e) => Err(io_failure("Cannot remove", &path, &e)), + Err(Errno::NOENT) => Ok(false), + Err(e) => Err(io_failure("Cannot remove", &path, &io_error(e))), } } @@ -401,25 +496,23 @@ mod unix { /// created if absent), the profile's file, if any, is trustworthy, and an empty file can be /// created and removed there. No token is written. pub(super) fn check_available(store: &FileStore, profile: &str) -> Result<()> { - dir(store, true)?; + let dir = open_dir(store, true)? + .ok_or_else(|| Failure::general(format!("Cannot create {}.", store.dir.display())))?; let path = store.path_for(profile); - if let Some(meta) = look(&path)? { - check(&path, &meta, false)?; + if let Some(stat) = look(&dir, &super::file_name(profile), &path)? { + check(&path, &stat, false)?; } - let probe = temp_path(store, profile)?; - OpenOptions::new() - .write(true) - .create_new(true) - .mode(0o600) - .open(&probe) - .map_err(|e| { - Failure::general(format!( - "the plain-text token store cannot be used: cannot write in {}: {e}", - store.dir.display() - )) - .hint("Use the system keychain or a tenant API key in HODEISHIELD_API_KEY.") - })?; - let _ = std::fs::remove_file(&probe); + let probe = temp_name(profile)?; + create_new(&dir, &probe, &store.dir.join(&probe)).map_err(|e| { + Failure::general(format!( + "{}: cannot write in {}: {}", + super::TOKEN_FILE_REFUSED, + store.dir.display(), + e.message + )) + .hint("Use the system keychain or a tenant API key in HODEISHIELD_API_KEY.") + })?; + let _ = rustix::fs::unlinkat(&dir, probe.as_str(), AtFlags::empty()); if let Some(fs) = super::network_file_system(&store.dir) { eprintln!( "Warning: {} is on {fs}. A file there can be read by the server's administrator \ @@ -599,10 +692,10 @@ mod tests { let (_tmp, store) = saved(); std::fs::set_permissions(&store.dir, std::fs::Permissions::from_mode(bits)) .expect("chmod"); - refused_with(store.load("p"), "must be 700"); - refused_with(store.save("p", &token()), "must be 700"); - refused_with(store.delete("p"), "must be 700"); - refused_with(store.check_available("p"), "must be 700"); + refused_with(store.load("p"), "no access"); + refused_with(store.save("p", &token()), "no access"); + refused_with(store.delete("p"), "no access"); + refused_with(store.check_available("p"), "no access"); assert_eq!(mode(&store.dir), bits); std::fs::set_permissions(&store.dir, std::fs::Permissions::from_mode(0o700)) .expect("chmod back"); @@ -614,16 +707,75 @@ mod tests { // Another user's files cannot be made in a test, so the judgement is exercised with the // effective uid replaced: the same file is then someone else's. let (_tmp, store) = saved(); - let meta = std::fs::symlink_metadata(store.path_for("p")).expect("meta"); - let me = meta.uid(); - assert!(unix::problem(&meta, false, me).is_none()); - let (reason, _) = unix::problem(&meta, false, me.wrapping_add(1)).expect("another owner"); + let stat = rustix::fs::lstat(store.path_for("p")).expect("stat"); + let me = stat.st_uid; + assert!(unix::problem(&stat, false, me).is_none()); + let (reason, _) = unix::problem(&stat, false, me.wrapping_add(1)).expect("another owner"); assert!(reason.contains("belongs to another user"), "{reason}"); - let dir_meta = std::fs::symlink_metadata(&store.dir).expect("meta"); - assert!(unix::problem(&dir_meta, true, me.wrapping_add(1)).is_some()); + let dir_stat = rustix::fs::lstat(&store.dir).expect("stat"); + assert!(unix::problem(&dir_stat, true, me.wrapping_add(1)).is_some()); assert_eq!(rustix::process::geteuid().as_raw(), me); } + fn tmp_files(store: &FileStore) -> Vec { + std::fs::read_dir(&store.dir) + .expect("dir") + .filter_map(|e| e.ok().map(|e| e.file_name().to_string_lossy().into_owned())) + .filter(|n| n.ends_with(".tmp")) + .collect() + } + + #[test] + fn a_failed_rename_removes_the_temporary_file() { + let (_tmp, store) = saved(); + // A non-empty directory where the file belongs makes the rename fail after the temporary + // file was written. + let target = store.path_for("q"); + std::fs::create_dir(&target).expect("blocker"); + std::fs::write(target.join("x"), "x").expect("fill"); + let dir = unix::open_dir(&store, false) + .expect("open") + .expect("exists"); + let err = unix::write_atomically(&store, &dir, "q", b"secret").expect_err("rename fails"); + assert!(err.message.starts_with("Cannot replace"), "{}", err.message); + assert!(tmp_files(&store).is_empty(), "{:?}", tmp_files(&store)); + } + + #[test] + fn leftover_temporary_files_of_the_profile_are_removed_when_stale() { + for deleting in [false, true] { + let (_tmp, store) = saved(); + let prefix = &file_name("p")[..16]; + let stale = store.dir.join(format!(".{prefix}.0123456789abcdef.tmp")); + let fresh = store.dir.join(format!(".{prefix}.fedcba9876543210.tmp")); + let other = store + .dir + .join(format!(".{}.0123456789abcdef.tmp", &file_name("q")[..16])); + let odd = store.dir.join(format!(".{prefix}.not-hex.tmp")); + for path in [&stale, &fresh, &other, &odd] { + std::fs::write(path, "token").expect("write"); + } + let old = std::time::SystemTime::now() - std::time::Duration::from_secs(600); + for path in [&stale, &other] { + std::fs::File::options() + .write(true) + .open(path) + .expect("open") + .set_modified(old) + .expect("age it"); + } + if deleting { + store.delete("p").expect("delete"); + } else { + store.save("p", &token()).expect("save"); + } + assert!(!stale.exists(), "a stale leftover of this profile goes"); + assert!(fresh.exists(), "a recent one may be a write in progress"); + assert!(other.exists(), "another profile's is not touched"); + assert!(odd.exists(), "only the exact pattern is matched"); + } + } + #[test] fn something_that_is_not_a_regular_file_is_refused() { let (_tmp, store) = saved(); @@ -706,13 +858,18 @@ mod tests { } #[test] - fn the_state_directory_is_not_the_configuration_directory() { - let dir = sessions_dir().expect("dir"); - assert!(dir.ends_with("sessions")); + fn the_sessions_directory_is_outside_the_configuration_directory() { + let sessions = sessions_dir().expect("dir"); + let config = crate::config::path().expect("config path"); + let config_dir = config.parent().expect("config dir"); assert!( - !dir.to_string_lossy().contains("/.config/"), - "{}", - dir.display() + sessions.ends_with("sessions") || sessions.to_string_lossy().ends_with(".sessions") + ); + assert!( + !sessions.starts_with(config_dir), + "{} is inside {}", + sessions.display(), + config_dir.display() ); } } diff --git a/crates/hodeishield-cli/src/auth/store.rs b/crates/hodeishield-cli/src/auth/store.rs index 362bc93..c712515 100644 --- a/crates/hodeishield-cli/src/auth/store.rs +++ b/crates/hodeishield-cli/src/auth/store.rs @@ -107,16 +107,27 @@ pub trait TokenStore { #[derive(Debug, Default)] pub struct Keychain; -/// The store the settings choose, looked up when it is used: the environment and the -/// configuration file are read at that point, so a command that never needs a token never fails -/// on them. A value that is not a store, or a platform that cannot have it, is an error at the -/// first use, and nothing falls back to the other store. +/// The store the settings choose. The choice is made at the first use and kept for the rest of +/// the process, so one command checks, reads and saves in the same store even if the environment +/// or the configuration file changed meanwhile. The environment and the configuration file are +/// read at that point, so a command that never needs a token never fails on them. A value that is +/// not a store, or a platform that cannot have it, is an error at the first use, and nothing +/// falls back to the other store. #[derive(Debug, Default)] -pub struct Configured; +pub struct Configured { + kind: OnceLock, +} impl Configured { fn chosen(&self) -> Result> { - Ok(match crate::config::token_store()? { + let kind = match self.kind.get() { + Some(kind) => *kind, + None => { + let chosen = crate::config::token_store()?; + *self.kind.get_or_init(|| chosen) + } + }; + Ok(match kind { crate::config::TokenStoreKind::Keychain => Box::new(Keychain), crate::config::TokenStoreKind::PlaintextFile => { Box::new(super::file_store::FileStore::platform()?) diff --git a/crates/hodeishield-cli/src/commands/mod.rs b/crates/hodeishield-cli/src/commands/mod.rs index 86e0af9..166057f 100644 --- a/crates/hodeishield-cli/src/commands/mod.rs +++ b/crates/hodeishield-cli/src/commands/mod.rs @@ -80,7 +80,7 @@ pub fn run(cli: Cli, out: &mut dyn Write) -> Result<()> { }, json: cli.json, verbose: cli.verbose, - store: Arc::new(auth::store::Configured), + store: Arc::new(auth::store::Configured::default()), }; match cli.command { Command::Vendors(command) => resources::vendors(&ctx, command, out), diff --git a/crates/hodeishield-cli/tests/cli.rs b/crates/hodeishield-cli/tests/cli.rs index 7b54192..5cc5e2e 100644 --- a/crates/hodeishield-cli/tests/cli.rs +++ b/crates/hodeishield-cli/tests/cli.rs @@ -1025,6 +1025,7 @@ mod file_store { /// asked for anything (exit 1), commands that need a credential are "not authenticated" /// (exit 3), and nothing is repaired or deleted. fn assert_refused(env: &Env, app: &App, reason: &str) { + let before = snapshot(env); let mut login_api = mockito::Server::new(); let mut login_app = mockito::Server::new(); let calls: Vec<_> = [&mut login_api, &mut login_app] @@ -1069,6 +1070,18 @@ mod file_store { .code(1) .stderr(predicate::str::contains(reason)); assert!(app.revoked.lock().expect("lock").is_empty()); + assert_eq!(snapshot(env), before, "a refused file is never touched"); + } + + /// Every file in the sessions directory with its bytes. + fn snapshot(env: &Env) -> Vec<(PathBuf, Vec)> { + kept(env) + .into_iter() + .map(|path| { + let bytes = std::fs::read(&path).expect("read"); + (path, bytes) + }) + .collect() } #[test] @@ -1213,6 +1226,27 @@ mod file_store { // be an error. signed_in(&env, &app); } + + /// The other direction of "no fallback": a valid session file is not used by the default store. + #[cfg(not(target_os = "macos"))] + #[test] + fn a_session_file_is_not_used_when_the_keychain_is_selected() { + let env = Env::new(); + let app = app(); + let file = signed_in(&env, &app); + let before = std::fs::read(&file).expect("read"); + app.with(env.cmd()) + .arg("whoami") + .assert() + .code(3) + .stdout(predicate::str::is_empty()) + .stderr(predicate::str::contains("keychain is not available")); + app.with(env.cmd()) + .args(["vendors", "list"]) + .assert() + .code(3); + assert_eq!(std::fs::read(&file).expect("read"), before); + } } /// Windows has no mode bits to check, so the opt-in is refused there, before the app is asked for From 0c33d49b257b4e36dca620e40aa16a2c004de513 Mon Sep 17 00:00:00 2001 From: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> Date: Sun, 11 Oct 2026 13:13:06 +0200 Subject: [PATCH 6/9] Point the macOS test at the sessions directory outside the configuration The sessions moved to a sibling of the configuration directory on macOS; the integration tests still looked inside it, so on macOS they found no file. Refs #39. Signed-off-by: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> --- crates/hodeishield-cli/tests/cli.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/hodeishield-cli/tests/cli.rs b/crates/hodeishield-cli/tests/cli.rs index 5cc5e2e..265194a 100644 --- a/crates/hodeishield-cli/tests/cli.rs +++ b/crates/hodeishield-cli/tests/cli.rs @@ -68,7 +68,7 @@ impl Env { if cfg!(target_os = "macos") { self._dir .path() - .join("home/Library/Application Support/com.Hodeitek.HodeiShield/sessions") + .join("home/Library/Application Support/com.Hodeitek.HodeiShield.sessions") } else { self.state().join("hodeishield/sessions") } From 868c0a4b7a6c851e947d21f67371dc156b489151 Mon Sep 17 00:00:00 2001 From: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> Date: Sun, 11 Oct 2026 13:13:54 +0200 Subject: [PATCH 7/9] State how long a copied token file stays useful The service's token lifetimes are now known: access tokens last 15 minutes, refresh tokens 30 days from the last refresh with reuse detection, and an absolute 90-day limit is being added. The README and the decision record state today's exposure of a leaked file and how to end it, instead of an open question. Refs #39. Signed-off-by: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> --- README.md | 12 +++++++-- .../0001-sign-in-without-a-keychain.md | 25 ++++++++++++++----- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 21b4040..f9df8cc 100644 --- a/README.md +++ b/README.md @@ -335,8 +335,16 @@ a code. You have two options, in this order of preference: 2. **A plain-text token file, if you want to sign in as yourself** on a machine you control, such as a jump host. You opt in explicitly, and the trade-off is that the tokens are stored **unencrypted** in a file that only your user can read. Anyone who can read your files, such as - `root`, a backup, a snapshot or a copy of your home directory, can use the refresh token until it - is revoked. + `root`, a backup, a snapshot or a copy of your home directory, can use the refresh token: + - **How long.** Today, up to 30 days after the CLI last refreshed the session (every refresh + starts a new 30 days), unless you end it earlier with `hodeishield logout` or by revoking the + access in the app under **Account › Applications**. The service is adding an absolute limit of + 90 days from the sign-in, and will also end these sessions when you change your password; this + section will say so once that is in place. + - **Reuse is detected.** Each refresh replaces the refresh token. If someone refreshes with a + copy of your file, the next refresh by your own CLI is seen as a reuse of an old token and + ends the session for both. An unexpected "sign in again" is a reason to revoke the access in + the app and check who could read the file. ```sh export HODEISHIELD_TOKEN_STORE=plaintext-file # or: hodeishield config set token_store plaintext-file diff --git a/docs/decisions/0001-sign-in-without-a-keychain.md b/docs/decisions/0001-sign-in-without-a-keychain.md index 4691042..1175fae 100644 --- a/docs/decisions/0001-sign-in-without-a-keychain.md +++ b/docs/decisions/0001-sign-in-without-a-keychain.md @@ -146,9 +146,22 @@ the following rules. Containers and CI keep using API keys. - **Migration between stores.** Deliberately absent; switching is sign out, change the setting, sign in. -## Open question - -To be confirmed with the API provider: the refresh token's lifetime, rotation and idle expiry on the -server side. They decide how long a copy of a token file stays useful, and whether the exposure of a -leaked file can be bounded by something other than revocation. Nothing in this decision depends on the -answer, but the README's wording about exposure should be revisited once it is known. +## Server-side lifetimes + +How long a copy of a token file stays useful is set by the service, not by the CLI: + +- The access token lives 15 minutes and is checked by the service on every use. +- The refresh token lives 30 days and is replaced on every refresh, which starts a new 30 days, so + today a session has no absolute limit while it keeps being refreshed. There is no idle expiry + beyond those 30 days. +- Replaying a refresh token that was already replaced ends the whole session. If an attacker + refreshes with a copied file, the legitimate CLI's next refresh ends both. +- A session ends with `hodeishield logout` (token revocation), with a revocation in the app under + Account › Applications, and when the account or the organisation is suspended. Today it does not + end when the password is changed. + +The service is adding an absolute limit of 90 days from the original sign-in and will end these +sessions when the password is changed or an administrator revokes them. From then on, the exposure of +a leaked file is at most 90 days, ended earlier by `hodeishield logout`, a password change or a +revocation under Account › Applications. Until then it is up to 30 days after the last refresh. The +README states the current figure and is updated when the limit is in place. From 82ad624bd1bb37ad1a394442252597ed7697cb81 Mon Sep 17 00:00:00 2001 From: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> Date: Sun, 11 Oct 2026 13:16:41 +0200 Subject: [PATCH 8/9] Build the symlinked-directory test next to the sessions directory The test moved the sessions directory into the state directory, which does not exist on macOS (the sessions directory sits under Application Support there), so the move failed before the case under test ran. Refs #39. Signed-off-by: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> --- crates/hodeishield-cli/tests/cli.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/hodeishield-cli/tests/cli.rs b/crates/hodeishield-cli/tests/cli.rs index 265194a..8334ef6 100644 --- a/crates/hodeishield-cli/tests/cli.rs +++ b/crates/hodeishield-cli/tests/cli.rs @@ -1125,7 +1125,9 @@ mod file_store { let env = Env::new(); let app = app(); signed_in(&env, &app); - let moved = env.state().join("moved"); + // Next to the sessions directory, which exists on every platform (the state directory is + // not used on macOS). + let moved = env.sessions().parent().expect("parent").join("moved"); std::fs::rename(env.sessions(), &moved).expect("move"); symlink(&moved, env.sessions()).expect("link"); assert_refused(&env, &app, "symbolic link"); From 7edc12a26ff6b7932f2273933b9e3b7cb2279845 Mon Sep 17 00:00:00 2001 From: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> Date: Sun, 11 Oct 2026 13:20:34 +0200 Subject: [PATCH 9/9] Compile the network file system table on Unix only Its only users are the Linux check and a Unix-only test, so on a Windows test build it was dead code and clippy refused it. Refs #39. Signed-off-by: d0ubl3g <48954024+d0ubl3g@users.noreply.github.com> --- crates/hodeishield-cli/src/auth/file_store.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/hodeishield-cli/src/auth/file_store.rs b/crates/hodeishield-cli/src/auth/file_store.rs index de84fd2..3bb3cb9 100644 --- a/crates/hodeishield-cli/src/auth/file_store.rs +++ b/crates/hodeishield-cli/src/auth/file_store.rs @@ -539,7 +539,7 @@ fn network_file_system(_: &Path) -> Option<&'static str> { } /// Linux `statfs` `f_type` values of file systems served by another machine. -#[cfg(any(test, all(unix, target_os = "linux")))] +#[cfg(all(unix, any(test, target_os = "linux")))] fn network_file_system_for(f_type: u64) -> Option<&'static str> { match f_type { 0x6969 => Some("NFS"),