login: load certificate into ssh-agent with configurable lifetime - #611
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).
EthanHeilman
left a comment
EthanHeilman
left a comment
There was a problem hiding this comment.
Looks good, I have some minor nits and questions, but nothing major. I like how this turned out
… in default config LoginCmd's zero value now leaves the ssh-agent untouched and the CLI opts in, so a LoginCmd built directly -- as tests build it -- cannot reach a developer's real agent even when SSH_AUTH_SOCK is set. The default client config is copied verbatim into a user's config, so it now sets agent_lifetime rather than commenting it out. A test pins that value to defaultAgentLifetime so the two cannot drift.
LoginWithRefresh mints a new certificate on every refresh and writes it to the filesystem, but left the agent holding the one from the initial login. That agent entry stopped being useful once its lifetime elapsed, so --auto-refresh silently dropped out of the agent while continuing to refresh on disk. A refreshed certificate now replaces the previous one. Replacement has to be explicit: the private key is unchanged but the certificate blob is not, and the agent stores a certificate as its own identity, so adding without removing would accumulate an entry per refresh until a server refuses the connection for too many authentication attempts. Also covers the agent path for ed25519, which the tests only exercised for ecdsa.
|
Looking at the post-merge test regression -- will fix it. |
The template now ships agent_lifetime commented out, so the test asserts the key is unset and a fresh config resolves to defaultAgentLifetime. docs/config.md said the default config sets 24h; it defers to the default.
| // forever. Best-effort: every failure is a printed warning, never a login | ||
| // failure. | ||
| func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { | ||
| sock := os.Getenv("SSH_AUTH_SOCK") |
There was a problem hiding this comment.
Instead of if l.AddKeyToAgent && !l.PrintKeyArg replace os.Getenv("SSH_AUTH_SOCK") with something like:
func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) {
sock := GetAgentSock()
where GetAgentSock() defaults to a mock and is overridable with the actual os.Getenv("SSH_AUTH_SOCK") for production and integration test use.
That way we can get unittest coverage of addCertToAgent which currently has no coverage.
There was a problem hiding this comment.
addCertToAgent already runs under test -- TestAddCertToAgent calls it
directly against a real in-process ssh-agent, for both key types. The happy
path was covered; the gap was its failure branches.
Those are reachable without a seam, since t.Setenv("SSH_AUTH_SOCK", ...)
already lets a test point the function at nothing, at a dead socket, or at an
agent that refuses the key. Keeping a real agent behind it means the cases
still exercise the wire protocol and the private-key marshalling, which is the
part most likely to break when someone adds a key type.
TestAddCertToAgentWarnings adds six such cases and takes addCertToAgent
from 58.1% to 93.5%, no production change. Mutating the not-a-certificate guard
and the empty-socket return turned each matching subtest red, so the cases
measure the branches rather than just passing.
That leaves the conn.SetDeadline failure, which I can't reach through
SSH_AUTH_SOCK. It's a two-line warning path so I left it -- if you'd rather
have it covered, say so and I'll add the indirection.
|
@sini Sorry the review took so long. Didn't have a chance until the weekend. Other than getting test coverage on |
addCertToAgent had its happy path covered end to end but none of its early returns. Six cases drive it through an absent socket, an unparseable lifetime, an unparseable certificate, a non-certificate public key, a socket with no listener, and an agent that refuses the key, taking statement coverage from 58.1% to 93.5%. Each case steers the function with SSH_AUTH_SOCK rather than a seam in the production path, so every one still runs against a real in-process agent. The conn.SetDeadline failure stays uncovered: reaching it needs a connection whose SetDeadline fails, which SSH_AUTH_SOCK cannot produce.
|
I'm not able to re-trigger the canceled jobs myself. I'm pretty confident their failure is transient/environmental. |
|
@sini Yeah, they aren't you, all tests pass. |
EthanHeilman
left a comment
EthanHeilman
left a comment
There was a problem hiding this comment.
LGTM
Some features that should be their own PRs are:
- Getting an integration test that only runs in github-actions but not locally which tests with SSH-agent. This way we have coverage and can capture any future changes that break this.
--no-ssh-agentflag and client-config setting to disable writing to ssh-agent- using
-iwhich specifies a filepath to write the key to should also disable ssh-agent
|
@sini Thanks for all your work on this! |
2 participants
Summary
First of the two PRs agreed in #606: when
SSH_AUTH_SOCKpoints at a running ssh-agent,opkssh loginnow also adds the freshly minted certificate and its private key to the agent, so the key is usable immediately without pointingsshat the key files. Best-effort and non-fatal — the keys are still written to disk first, and any agent problem is a warning rather than a login failure.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:
--lifetimeflag (opkssh login --lifetime 12h)agent_lifetimein the client config (~/.opk/config.yml)Both duration strings (
12h,45m) and raw seconds (28800) are accepted. An invalid value fails up front, before the browser dance.Per the discussion in #606, this PR makes no change to certificate
ValidBefore— the client cannot know the server's expiration policy (the server computes it from the ID token'siat), so the agent lifetime is a client-side bound to keep keys from accumulating, not a statement of the certificate's validity. Syncing expiries dynamically is the follow-up refresh-daemon PR's job.Relates to #6, #96. Split out of #606.
Notes
Run()(fail before the browser dance) and again inaddCertToAgent, becauseLogin/LoginWithRefreshare exported and reachable withoutRun()(the integration tests call them directly). Sub-second and overflowing values are rejected — a truncatedLifetimeSecsof 0 would mean an immortal key, the exact state the lifetime exists to prevent.--auto-refreshthe key is added to the agent once at initial login; refreshed certificates only go to disk as today. Live agent rotation on refresh lands with the refresh daemon (PR 2).--print-key(that mode's contract is stdout only).agent.ServeAgentserver over a unix socket (skipped on Windows), asserting the added key carries the certificate and the requestedLifetimeSecs. Both the unit login-flow tests and the integration suite now pinSSH_AUTH_SOCKempty sogo testnever touches the developer's real agent.SSH_AUTH_SOCKis typically unset, so the agent add is silently skipped (now noted indocs/config.md); named-pipe support is planned alongside the daemon work.