login: XDG config paths and non-clobbering multi-identity key files - #612
Open
sini wants to merge 13 commits into
Open
login: XDG config paths and non-clobbering multi-identity key files#612sini wants to merge 13 commits into
sini wants to merge 13 commits into
Conversation
…time When SSH_AUTH_SOCK points at a running ssh-agent, opkssh login now also adds the freshly minted certificate and its private key to the agent, so the key is usable immediately without pointing ssh at the key files. Best-effort and non-fatal: the keys are still written to disk first, and any agent problem is reported as a warning rather than failing the login. The key is always added with a lifetime, since ssh-agent has no replace operation and keys would otherwise accumulate with every login. The lifetime is resolved as: --lifetime flag, then agent_lifetime in the client config (~/.opk/config.yml), then a default of 24h matching the server's default certificate expiration policy. Both duration strings (12h, 45m) and raw seconds (28800) are accepted, and an invalid value fails before the browser dance rather than after authentication. Relates to openpubkey#6 and openpubkey#96; first of the two PRs agreed in openpubkey#606 (the second adds a refresh daemon that rotates agent keys as tokens are refreshed).
Review findings on the agent-lifetime feature: - Reject lifetimes under one second: uint32(d / time.Second) truncated sub-second durations (500ms, 900ms) to LifetimeSecs 0, which the agent protocol treats as no lifetime at all — an immortal key, the exact state the lifetime exists to prevent. Bound raw seconds values before multiplying, since time.Duration(sec) * time.Second wraps int64 for large inputs and could land on a small bogus duration. - Set a 5s deadline on the ssh-agent exchange so a dead socket (e.g. a forwarded agent whose upstream connection is gone) degrades to a warning instead of hanging the login after authentication succeeded. - Unset SSH_AUTH_SOCK in the integration suite's TestMain so its seven login flows can never inject test keys into a developer's real agent. - Document that the agent is not currently reachable on Windows.
… in CLI docs Post-review cleanup, behavior-preserving: - The in-process test-agent scaffolding is extracted to startTestAgent (windows skip, socket lifecycle via t.Cleanup, SSH_AUTH_SOCK wiring in one place). - The SSH_AUTH_SOCK isolation guard moves into the shared Mocks fixture, so every mock login flow is isolated from the developer's real agent by construction instead of by three pasted guards; a test that needs an agent points SSH_AUTH_SOCK at its own after calling Mocks. - TestParseConfigWithAgentLifetime collapses its two verbatim copies into a loop over both accepted value shapes. - Two comment blocks that restated nearby doc comments are trimmed. - docs/cli/opkssh_login.md is regenerated to document the new --lifetime flag (only this file has content changes from the flag addition).
sini
force-pushed
the
feat/storage-layout
branch
from
August 21, 2026 01:44
f0df813 to
06de598
Compare
sini
force-pushed
the
feat/storage-layout
branch
from
August 21, 2026 02:00
997f8a8 to
d74e70d
Compare
Two storage-layout changes, predicates for upcoming refresh-daemon work: Client config resolution now follows the XDG Base Directory spec: the first existing of $XDG_CONFIG_HOME/opk/config.yml (when set and absolute; ~/.config/opk/config.yml otherwise, %AppData%\opk\config.yml on Windows) and the legacy ~/.opk/config.yml. An existing legacy config keeps working untouched and always wins when it is the only one; fresh --create-config writes land at the XDG location with 0700/0600 (the config can carry client_secret values). Login logs which config file is in use at -v. The old --config-path help text claiming %APPDATA%\.opk on Windows was wrong (the code always used ~/.opk everywhere) and is fixed. Key files no longer clobber across identities: the default ~/.ssh slot rule compares the (iss, aud, sub) identity parsed from the existing certificate's embedded PK token, failing closed, instead of trusting the "openpubkey" comment alone — a certificate whose PK token no longer parses but whose comment is exactly "openpubkey" remains reusable, so every legacy file behaves exactly as before. When all default slots belong to other identities, keys are written to the opkssh identity directory (~/.ssh/opkssh, created on demand with its IdentityFile fragment — inert without the --configure Include) named <issuer>-<client_id> or, when a different account at the same provider collides, suffixed with an 8-hex sha256(iss|aud|sub) tag; a foreign occupant at the tagged path is an error, never an overwrite. A warning naming the written path fires only when the fallback was used and no ssh-agent ended up holding the key. Intentional behavior changes, called out explicitly: a login whose default slots are all taken now succeeds into the identity directory instead of failing with "no default ssh key file free"; a parseable certificate belonging to a different identity is never overwritten (previously any opkssh cert could clobber any other); a missing opkssh directory or config fragment is created on demand rather than erroring. Single-identity users keep byte-identical behavior throughout.
Post-review cleanup of the storage-layout change; behavior-preserving: - One slot classifier (classifySlot) now answers "who holds this key-file slot" for every tier. The default tier and the opkssh identity directory keep their distinct write policies as explicit mappings over the classified states; previously two similar loops re-implemented the same pipeline and their one behavioral divergence (an absent private key makes a default slot writable even over an orphaned certificate, while the identity directory fails closed) was undocumented. - keyIdentity now carries its own validity, and equality is only reachable through sameAs, which requires both sides valid — the fail-closed rule is enforced by construction instead of at each comparison site. The identity is extracted once per login and passed down. - The IdentityFile fragment format is one contract shared by login (writer) and logout (remover) via identityFileLine/fragmentLines; the two inline copies could drift, and the writer's matcher did not tolerate CRLF while the remover did. Re-logins no longer rewrite an unchanged fragment. - ResolveClientConfigPath reports whether a config was found, removing the caller's redundant re-stat; the --config-path help text is a shared constant across both commands that carry the flag; candidate enumeration is unexported to keep the module's public API flat. - The fallback landing is returned through the write-path call chain instead of being smuggled through a LoginCmd field. - Tests share fixtures (opkDirFixture, startTestAgent, mustReadFile) and gain found-flag expectations; 13 docs/cli files whose only change was the generated date stamp are dropped from the change.
OpenSSH mis-parses an unquoted ssh_config path with a space (common with Windows home directories). identityFileLine now double-quotes a path containing blanks, per ssh_config token rules; since login's writer and logout's remover both render lines through this one function, matching and removal stay consistent by construction. Paths without blanks stay unquoted so fragment entries written by earlier versions keep matching exactly.
t.Setenv(HOME) does not drive os.UserHomeDir on Windows (it reads USERPROFILE), so the round-trip's blank-bearing home premise cannot hold there; the identityFileLine rendering assertions still run on every platform. Same guard pattern as TestResolveClientConfigPath.
Every shorthand term is now defined where it lives: the config fragment is named as ~/.ssh/opkssh/config with its Include relationship at identityFileLine, key-file slots are defined at certState, and the two key-writing destinations are named instead of called tiers. Subjectless and provenance sentences are rewritten to name their objects.
sini
force-pushed
the
feat/storage-layout
branch
from
August 21, 2026 02:06
1bdb72c to
4bd8391
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two storage-layout changes, predicates for the refresh-daemon work discussed in #606 (stacked on #611):
1. XDG config-path support
The client config now resolves through the first existing of:
$XDG_CONFIG_HOME/opk/config.ymlwhen the variable is set and absolute (the XDG Base Directory spec requires relative values to be ignored), otherwise~/.config/opk/config.ymlon Linux/macOS or%AppData%\opk\config.ymlon Windows — replacement semantics per the spec, matching git/gh.~/.opk/config.yml(all platforms).An existing
~/.opk/config.ymlkeeps working untouched and always wins when it is the only config. Fresh--create-configruns write the XDG location with 0700/0600 (the config can carryclient_secretvalues).opkssh login -vnames the config file in use. The old--config-pathhelp text claiming%APPDATA%\.opkon Windows was incorrect (the code always used~/.opkon every platform) and is fixed on both commands that carry the flag.2. Non-clobbering multi-identity key files
Today a second identity's login either overwrites the first's
~/.ssh/id_ecdsa(any opkssh cert may clobber any other — the check is theopenpubkeycomment alone) or fails with "no default ssh key file free". Now:openpubkey(so every legacy file behaves exactly as before).~/.ssh/opkssh/, created on demand together with itsIdentityFileconfig fragment — inert without the explicit--configureInclude, which this PR never touches), named<issuer>-<client_id>or, when a different account at the same provider collides, suffixed with an 8-hexsha256(iss|aud|sub)tag (hashed — some OPs use emails as subjects, which don't belong in file names). A foreign occupant at the tagged path is an error, never an overwrite.Single-identity users keep byte-identical behavior throughout — the fallback machinery only engages when a second identity actually appears.
Intentional behavior changes
~/.ssh/opkssh/directory or config fragment is created on demand rather than being an error.Notes
IdentityFilepaths containing blanks are now double-quoted per ssh_config token rules (an unquoted path with a space is mis-parsed by OpenSSH — common with Windows home directories). Login's writer and logout's remover render lines through one shared function, so matching and removal cannot drift; blank-free paths stay unquoted so existing fragment entries keep matching exactly.docs/cli/is regenerated (opkssh gendocs) for the changed help text.Design spec (with the review history behind these decisions): https://gist.github.com/sini/c32d1b73adbb56f1bdbbe620b10dc80f