Conversation
chore: back-merge main into develop
Point the ppk testdata README at a tracked regenerate.sh instead of the git-ignored plan file, and encode the puttygen 0.84 passphrase handling that actually works instead of the plan's untested invocation.
Derive the AES key, IV, and MAC key from the passphrase via Argon2id, decrypt the private blob, and verify the HMAC-SHA-256 MAC. A wrong passphrase surfaces as PpkError::WrongPassphrase; a MAC mismatch on an unencrypted key surfaces as PpkError::Damaged. Also lays the v2 (HMAC-SHA-1, double-SHA-1 cipher key) path used by the next task's fixtures.
Zeroize the MAC input buffer and pre-size it so it never reallocates, cap Argon2 passes and parallelism alongside memory before any KDF work runs, preserve a PPK comment's exact whitespace since it is part of the MAC input, enable the aes/cbc zeroize features so the cipher key does not survive in freed heap, and bring the crypto module back to rustfmt's formatting.
…ests Add a P-521 fixture and fold it into the ECDSA comparison test, since it is the curve whose private scalar routinely needs left_pad's zero-extension. Add an encrypted DSA fixture so the "refused before decrypting" test can actually distinguish that from "refused because of a wrong passphrase", which a plain DSA key cannot. Also drop the duplicate wire_string helper in favor of the existing ssh_string, add direct unit tests for left_pad, and correct a comment that misattributed a PEM-wrapping mismatch to an upstream decoder bug rather than ssh-encoding's fixed 70-column width.
Pipe converted OpenSSH key text to ssh-add over stdin so it never becomes a file, and remove keys the same way by piping the public line to ssh-add -d. Argument assembly is unit-tested; the live-agent behaviour was verified once by hand against a throwaway agent on a temporary socket: $ cat "$TMP/k" | ssh-add -t 43200 - Identity added: (stdin) (agent-probe) Lifetime set to 12:00:00 $ ssh-add -l 256 SHA256:+vv03E6DcJQGIF4WxDlombzCnNs4aXRpubBLKyQ3dwc agent-probe (ED25519) $ cat "$TMP/k.pub.line" | ssh-add -d - Identity removed: (stdin) ED25519 (agent-probe) $ ssh-add -l The agent has no identities. Also confirmed ssh-add's stderr on a malformed stdin key never echoes the key material back, only a generic parse error, so the error path is safe to surface to a caller's log.
An early return from add_from_memory or remove between spawn and wait (e.g. EPIPE when ssh-add exits before reading stdin) left the child process unwaited. std::process::Child is not reaped on drop, so each such failure left a zombie process entry until the whole app exited. Capture the write error instead of propagating it directly, wait on the child on that path too, then return the original error.
The immediate-vs-window branch compared KeyState::PuttyUnencrypted, but state() reports PuttyNoAgent for every PuTTY key on a no-agent machine regardless of encryption, so the branch never fired for the audience it exists for and could ask for a passphrase the source file did not need. Also close two gaps around the pending export: cancelling or closing the unlock window left the slot parked for a later, unrelated passphrase to be spent on, and a failed export deleted an existing destination before the passphrase was known to be right.
An unencrypted PuTTY key was reported as locked, in a state with no passphrase to type and no Unlock button on offer. Both reporting sites -- the state table in the unlock command and the offline half of the SSH gate -- now say the agent is what is missing, which is true and is what the Convert action exists for. The renderer already routes that code to a single warning instead of a futile prompt. Require the Private-MAC to be the length its version's digest actually has. A short one just failed to verify, which every caller reads as a wrong passphrase, so a structurally broken file left the user retyping a correct passphrase forever with no way to learn the file was damaged. Clear a pending export before the idempotence guard in dismiss_prompt, not after. A slot can be parked against a prompt that was answered while its window was still on screen, and behind the guard such a slot survived to spend the next passphrase on writing a key to disk -- the one act in this feature that must be asked for each time. Let a key already in the agent be converted. Refusing it reported a loaded, working key as "not a private key", and no other code would have been true either: the conversion needs no agent, the agent's copy lasts one session while the file lasts, and the state is reachable without Settings ever offering the action. Print the CLI's PuTTY advice once per process. It sits in the closure that builds each git subprocess's environment, so an update across N repositories repeated the same four lines N times.
The only place this feature puts key material on disk had no test. The three export tests call a test-only replica of the writer, so they would all still pass if finish_export were deleted: the convert-then-delete ordering, the switch to the new path, and the resolution every waiter and window listener depends on were all uncovered. Two tests through unlock itself. The first parks an export, answers it with the right passphrase over a destination that already exists, and pins the written key, the path switch, the spent slot and the single resolution -- false, because the key just written is encrypted and holds no passphrase yet. The second answers with a wrong one and pins that every byte of the pre-existing destination is still there, which is the guarantee converting before deleting exists for.
state() read the process environment inline, so the four PuTTY rows of the state machine could only be asserted as an "or" of states and would pass whether the fold worked or not. Extract the pure classify() and supply agent availability as an argument, then assert every row exactly.
The mapping from a PPK failure to what the user is told was written out three times, agreeing by hand. Add From<PpkError> for UnlockError and route the export path through it and the existing unlock_error_key, so there is one table from PpkError and one from UnlockError.
For an unencrypted key the private blob is the plaintext private key, and state() parses the file on every settings render, gate check and failed git invocation -- each one dropping that plaintext into freed heap. Make it Zeroizing and drop the Debug derive that could print it. Also correct key_format::sniff's doc, which claimed an unreadable file is Other: it never touches the filesystem.
app/ssh_key.rs carried four unrelated things in 1200 lines. Move the PEM line-wrapping codec to app/openssh_pem.rs and the pure KeyState/Gate table to app/ssh_key/gate.rs, re-exported so callers are unchanged. The key store, its file classification and its decryption stay together; they are genuinely coupled. Moves and use lines only.
Reject a base64 body with one leftover character, report a non-numeric version as malformed rather than unsupported, and refuse an unknown format version instead of routing it into v2's SHA-1 crypto. Each has a test; a duplicate header field stays last-write-wins, now with the reason written down. Wipe the v2 SHA-1 digests once copied out, assert what the decrypted blobs actually contain rather than only that they decrypted, check the OpenSSH checkints on the hand-rolled test decrypt and try ssh-key's own first, cover the loaded-agent state in the environment test, and give the CLI test a per-run temp directory.
Support PuTTY-format private keys (.ppk, versions 2 and 3) in the desktop app. A chosen .ppk is parsed and decrypted in memory, re-encoded as OpenSSH text and piped to ssh-add over stdin, so the key never becomes a file; ssh then finds it in the agent, which is why a PuTTY key's git environment is deliberately empty. Where no agent can hold it, Settings offers an explicit one-time conversion to an OpenSSH key file. The CLI does not read these keys and prints the puttygen command that converts one.
rustdoc writes browser JavaScript under target/, which fails no-undef on window by the thousand. Running cargo doc before pnpm lint therefore buried the real lint output under generated code nobody edits.
Both the CLI and desktop backend derived MCP preset group directories by splitting each resolved skill's root path and taking only its first segment, which capped presets at one group level. Replace that loop in both front ends with the core preset_group_dirs helper, which walks every ancestor directory of every resolved skill.
The app checks for a newer release on startup and once a day, offers it in the status bar, and can download the right artifact for the host, verify its SHA-256, and install it. A major or minor version opens a dialog with the cumulative release notes; a patch is offered by the badge alone. Refusing a dialog silences it until a newer minor line appears. A release candidate is offered newer candidates as well as final releases; a stable build is offered only final ones. A check can also be started by hand, from Settings, from About, or from the macOS Help menu, which ignores the once-a-day interval. Every check appears in the task list with its result, so a check that found nothing is distinguishable from one that never ran. Installing replaces the running application, so it happens from a detached helper that waits for this process to exit: macOS mounts the dmg and swaps the bundle through two atomic renames, a running AppImage is replaced by an atomic rename onto itself, a deb goes to the system installer, and Windows launches its own. If the copy is refused, a marker written by that helper is read on the next launch and the dialog reopens with the xattr command that clears the block. Each release publishes versions.json, generated from CHANGES.md and the staged artifacts, listing a hundred versions with notes on the newest ten. Existence is checked against GitHub so a deleted release stops being advertised. The updater never trusts GitHub's ordering: releases are listed lexicographically by tag, so v0.5.0-rc.10 sorts below rc.3 and v0.10.0 would sort below v0.9.0. It reads several pages and picks the greatest tag with its own version parser. `skillkeeper update` prints how to update the CLI on the current platform, without touching the network. Also here: the interface is translated into all eighteen catalogues, the Rust toolchain is pinned to an exact version so a local check and CI run the same linter, and pages leave more room below their last element. Squashed from the session that built it: the candidate releases cut to exercise the pipeline, and an abandoned experiment with release titles, carry no information worth keeping.
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.
What this is
The 0.5.0 release: the desktop app can now update itself, and the release
pipeline publishes the manifest it reads.
The app checks for a newer release on startup and once a day, offers it in the
status bar, and downloads the right artifact for the host, verifies its SHA-256,
and installs it. A major or minor version opens a dialog with the cumulative
release notes; a patch is offered by the badge alone, and refusing a dialog
silences it until a newer minor line appears. A release candidate is offered
newer candidates as well as final releases; a stable build is offered only final
ones.
A check can also be started by hand -- from the Application updates section at
the end of Settings, from About, or from Check for Updates in the macOS Help
menu -- which ignores the once-a-day interval. Every check appears in the task
list with its result, so a check that found nothing is distinguishable from one
that never ran.
Installing replaces the running application, so it happens from a detached
helper that waits for this process to exit: macOS mounts the dmg and swaps the
bundle through two atomic renames; a running AppImage is replaced by an atomic
rename onto itself; a deb goes to the system installer; Windows launches its
own. If the copy is refused, a marker the helper leaves behind is read on the
next launch and the dialog reopens with the
xattrcommand that clears theblock, and a button to copy it.
skillkeeper updateprints how to update the CLI on the current platform,without touching the network.
Also here: the whole interface is translated into all eighteen catalogues, the
Rust toolchain is pinned to an exact version so a local check and CI run the
same linter, and pages leave more room below their last element.
Two defects worth knowing about
Both were found by exercising this against real published releases, and both
would have failed silently.
GitHub orders releases lexicographically by tag name, not by date. So
v0.5.0-rc.10sorts belowv0.5.0-rc.3, and reading the first entry of therelease list answered
rc.9whilerc.10was newer by two minutes. Anrc.9build therefore read its own manifest, found nothing newer, and could never be
offered
rc.10. This is not a candidate-only quirk:v0.10.0will sort belowv0.9.0for the same reason. The updater now reads several pages and picks thegreatest tag with its own version parser.
docs/development/releasing.mdrecords this so nothing comes to depend on that order again.
The daily interval suppressed a fresh install's first check using a
conclusion the previous build had reached, so a newly installed version reported
"postponed" and showed no badge while a manual check found the update at once.
The persisted state now records which version made each attempt, and a version
change defeats the interval.
Verification
Gates on this branch: 1163 Rust tests, 872 TypeScript tests,
cargo fmt,clippy -D warnings, typecheck, coverage, the renderer build, the Storybookbuild, and the end-to-end suite against the fixture repository (43 tests).
The producer/consumer contract is pinned by a test running against a manifest a
real release published, copied verbatim, which now includes a two-digit
candidate -- the case that caused the first defect above.
Beyond that, the flow was exercised against published releases: installing one
candidate and being offered the next, on artifacts built by this pipeline.
Not covered by tests
Installing and relaunching cannot be automated: it needs a published release and
a process restart. It was checked by hand. The candidate releases cut to exercise
the pipeline have been deleted, so the release list holds only real releases.