feat(ssh): restore FIDO2 security key support (#2308) - #2823
Conversation
…h GUI OpenSSH FIDO2 keys (ed25519-sk / ecdsa-sk) now authenticate through a Netcatty-owned agent and SSH_ASKPASS bridge, with Keychain generate/import UI and native PIN/touch dialogs instead of a terminal askpass.
Address review findings: serialize owned-agent acquire/release with quit hooks, pair Homebrew ssh-add with agent/keygen, drop soft-key path heuristics, force FIDO agent on ProxyJump hops, strip machine-local paths from the ssh2 patch, and clarify touch-prompt CTA.
Clear owned FIDO agents only via tracked SSH_AGENT_PID, isolate ssh-agent -k env from the ambient shell agent, and install process exit hooks so utility workers clean up daemonized agents on shutdown.
Only register process exit and Electron quit hooks for FIDO agent cleanup so SIGTERM/SIGINT keep their default process-exit behavior in terminal workers.
Share buildFidoAwareAgentPrepOptions / resolvePreparedAgentSocket so SFTP, port-forward, Mosh, and ET prepare SK handles into the Netcatty FIDO agent with resolveWebContents for PIN/touch, and native OpenSSH uses that socket instead of the ambient login agent.
OpenSSH sk private key handles only embed sk-*-@openssh.com inside the base64 body, so raw-text regex never matched real PEMs. Decode the PEM payload for bridge detection/materialize, align Keychain import with domain detectFidoSshKeyType, and add regression tests for base64-only SK PEMs.
openImport always seeds type: ED25519, so using draftKey.type first discarded detectFidoSshKeyType for paste-imported sk PEMs. Resolve type from key material via resolveImportedKeyType and use that on save.
Revive closed PR #2624 onto current main: OpenSSH sk-* auth via a Netcatty-owned ssh-agent + SSH_ASKPASS bridge, Keychain ED25519-SK/ECDSA-SK generate/import, and native PIN/touch dialogs. Also harden gaps found vs Termius/OpenSSH and prior review: - Windows askpass uses named pipes (\\.\pipe\...) - SK certificate algorithms and path-only IdentityFile detection - FIDO+certificate keeps the agent signing path - Simulated askpass PIN/touch round-trip tests (no hardware) Co-authored-by: 陈大猫 <binaricat@users.noreply.github.com>
binaricat
added
the
automation:codex-loop
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Address review findings on the restored FIDO2 path: - Keep system/owned agent for SK + OpenSSH certificates (all ssh2 bridges) - Async enhanceAuthOptionsForFido peeks IdentityFile paths on every surface - Stage companion -cert.pub beside materialized SK handles for ssh-add - Per-lease askpass resolver routing for multi-window PIN/touch - Win32 uses system openssh-ssh-agent pipe (no unsupported -a spawn) - Release owned FIDO agent refcount on SSH session close Co-authored-by: 陈大猫 <binaricat@users.noreply.github.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
bugbot run |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- Guard path-backed SK peek against password-only auth - Attach one-shot FIDO agent release on SSH/SFTP/exec/port-forward - Preserve and fire release hooks for Mosh/ET PTY sessions - Drop askpass leases when owned agent/lease is released Co-authored-by: 陈大猫 <binaricat@users.noreply.github.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
bugbot run |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66e97d0b2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| // No .pub selector — still attempt ssh-add when requested. | ||
| } | ||
| if (alreadyLoaded) { |
There was a problem hiding this comment.
When a shared agent already contains the bare SK key but not its certificate, this branch skips ssh-add solely because the blob from <identity>.pub is loaded. For a materialized vault certificate, the separate <identity>-cert.pub companion is therefore never loaded, so the strict agent wrapper can offer only the bare key and certificate-only authentication fails. Check whether the certificate blob is also present before skipping the staged identity.
Useful? React with 👍 / 👎.
| }; | ||
| } | ||
| if (options.agentForwarding) { | ||
| const forwardingSocketPath = await getAvailableForwardingAgentSocket(options.identityAgent, options); |
There was a problem hiding this comment.
For a FIDO-backed Mosh connection with agent forwarding, this await can reject after prepareSystemSshAgentForAuth has already acquired an agent reference and askpass lease—for example, on Windows when the configured forwarding agent is Pageant/Cygwin and the native OpenSSH compatibility check rejects it. Because prepareMoshSshAgentOptions throws before returning the embedded release callback, the caller cannot clean it up, retaining the agent and renderer resolver until shutdown; release the prepared agent in this failure path.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51b3b04fa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (etExitFinalized) return; | ||
| if (sessions.get(sessionId) !== session) return; | ||
| etExitFinalized = true; | ||
| releaseFidoAgents(); |
There was a problem hiding this comment.
When the user explicitly closes a FIDO-backed ET session, closeSession kills the PTY and immediately deletes the session from the shared map. The ensuing onExit callback therefore returns at the preceding map check before reaching this release, leaving the agent reference and askpass lease alive until process shutdown. Store the release on the session or invoke it from the explicit-close path.
Useful? React with 👍 / 👎.
| ...(typeof agent?._releaseNetcattyFidoAgent === "function" | ||
| ? { _releaseNetcattyFidoAgent: agent._releaseNetcattyFidoAgent } | ||
| : {}), |
There was a problem hiding this comment.
When ET uses FIDO authentication with agent forwarding and forwarding-agent resolution rejects, the agent acquired for this prepareOne call has its release callback only in the local prepared object here. Because preparedEntries is populated only after prepareOne returns, the outer catch releases earlier hops but cannot release the failing hop, retaining its agent reference and askpass resolver. Guard the rest of prepareOne and release its newly acquired agent on failure.
Useful? React with 👍 / 👎.
| if (bareAlreadyPresent) { | ||
| // Reloaded only to advertise a missing companion certificate. Join | ||
| // Netcatty-owned refcounts; never adopt a pre-existing user identity. | ||
| if (hasSharedAgentIdentity(key)) { | ||
| sharedAgentIdentities.push({ key, identityPath }); | ||
| } |
There was a problem hiding this comment.
When the shared Windows agent already contains the user's bare SK identity but lacks the staged companion certificate, this path runs ssh-add and loads the certificate, then records no cleanup unless Netcatty already owns the bare key. After the connection closes, the newly added certified identity consequently remains advertised to unrelated agent clients. Track the certificate addition separately so it can be released without treating the user's pre-existing bare identity as owned.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f84979dbf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| retainSharedAgentIdentity(entry.key, entry.identityPath, cleanupDir); | ||
| } | ||
| // Temp lifetime is owned by the shared-identity refcount now. | ||
| skTempCleanup = null; |
There was a problem hiding this comment.
When another session reuses the same inline SK identity through the shared Windows agent, retainSharedAgentIdentity() finds the existing blob entry and increments its refcount without retaining the new session's cleanupDir. This code nevertheless clears skTempCleanup, so the finally block cannot remove that second materialized private-key handle/certificate directory, while the last release only removes the first directory recorded in the map. Repeated connections therefore leave credential staging directories behind; only transfer cleanup ownership when the refcount entry actually accepts the directory, or delete redundant staging immediately after loading.
AGENTS.md reference: AGENTS.md:L143-L143
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b1e4aa4c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useSshAgent: options.useSshAgent === true || forceSystemAgentForFido, | ||
| useFidoAgent: forceSystemAgentForFido || options.useFidoAgent === true, |
There was a problem hiding this comment.
When the checked direct-SSH path uses an SK handle selected through the HostDetails identity-file picker, the resulting reference key retains the default ED25519 type and the renderer sends useSshAgent: false; this backend probe correctly discovers the SK file and creates systemAuthAgent, but sets useSshAgent: true only on the temporary preparation object. The later shouldOfferAgentForLogin(options, ...) still reads the original false value, so no agent authentication method is offered and the selected path-backed FIDO key cannot authenticate. Propagate the detected FIDO state into the login-auth decision rather than only into preparation.
Useful? React with 👍 / 👎.
| try { | ||
| return Buffer.from(match[1].replace(/\s+/g, ""), "base64").toString("binary"); | ||
| } catch { | ||
| return null; |
There was a problem hiding this comment.
When a user imports an SK private handle without also pasting its optional public key, this helper runs in the renderer, whose main window has nodeIntegration: false and no global Buffer. The resulting ReferenceError is swallowed by this catch, so every base64-only SK PEM is classified as ordinary ED25519 instead of ED25519-SK/ECDSA-SK, omitting the expected FIDO metadata and agent routing from renderer-side consumers. Use a renderer-safe decoder such as atob/Uint8Array with a guarded Node fallback.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd53b18c21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (cleanupDir) { | ||
| fs.promises.rm(cleanupDir, { recursive: true, force: true }).catch(() => {}); |
There was a problem hiding this comment.
When the Windows system-agent fallback releases the last reference to a materialized inline SK key, execFile() starts ssh-add -d asynchronously but this immediately removes the staging directory before the callback runs. Since ssh-add -d interprets its operands as identity-file paths (ssh-add(1), -d), the child can observe a missing file and fail to remove the key; the refcount entry has already been discarded, so the identity can remain available to unrelated agent clients indefinitely. Delete cleanupDir only after the removal process finishes.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
netcatty-bot
added
automation:bot-pr
|
Codex reported no major issues. This PR is marked ready for human review/merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d0a0c6c9a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const readFile = injected.readFile || fs.promises.readFile; | ||
| const expand = injected.expandIdentityFilePath || expandIdentityFilePath; | ||
| for (const rawPath of identityFilePaths) { | ||
| const identityPath = expand(rawPath); |
There was a problem hiding this comment.
When an imported SSH config IdentityFile uses tokens such as %r, %h, or %p, this probe reads the literal path, so it fails to recognize a path-backed FIDO key. The later login code only forces the FIDO agent path when this probe succeeds, so those selected SK handles fall back to the normal software-key loading path and cannot authenticate. Use the same token-aware resolution with the host/port/user context before reading the .pub or private key.
Useful? React with 👍 / 👎.
3 participants
Summary
Restores accidentally-deleted PR #2624 onto current
main, hardens gaps found by comparing Netcatty’s OpenSSH-agent path with Termius / OpenSSH / other SSH clients, and adds hardware-free FIDO askpass simulation tests.Closes #2308
Type of Change
Related Issue (optional)
Closes #2308
Changes Made
sk-ssh-ed25519@openssh.com/sk-ecdsa-sha2-nistp256@openssh.com) auth via Netcatty-ownedssh-agent+ssh-sk-helper(never softwareprivateKeysign)FidoPromptModal) driven bySSH_ASKPASSfor generate /ssh-add/ connectED25519-SK/ECDSA-SK, resident + verify-requiredssh2patch for sk public/private handle parse (based on Add FIDO/U2F Security Key (SK) support mscdex/ssh2#1496)AppSideEffects/DialogsHost/ thinApp.tsx)Gap fill / review fixes
\\.\pipe\netcatty-fido-askpass-*named pipesopenssh-ssh-agentpipe (no unsupported-a)-cert.pubforssh-addenhanceAuthOptionsForFidopeeks.pub/handle on every surfaceNETCATTY_FIDO_ASKPASS_LEASE→ resolver map + lease releaseauthMethod === "password"Client comparison (research)
ssh-keygen -t *-sk, agent,ssh-sk-helper); we wrap thisOut of scope: process-in CTAP/libfido2, Windows bundled Win32-OpenSSH packaging, YubiKey OTP/PIV/OpenPGP.
Testing
npm test/npm run linton restore + hardening commits; CI re-runs full suiteChecklist