feat(provision): settle + verify managed SSH key (ed25519) and persist for #71 (#74) - #82
Merged
Merged
Conversation
…74) Settles the managed-key algorithm and delivers the verified-key fact #71 needs. - Algorithm decision: keep ed25519 (grounded accepted from RouterOS 7.12+, and batch-login-verified on the 7.20.8 floor and current stable in test/lab/ssh-keys/REPORT.md). RSA-2048 is only a sub-7.12 fallback quickchr never provisions; ECDSA is rejected by RouterOS. - installSshKey() now does a real host-OpenSSH batch login (BatchMode=yes, PasswordAuthentication=no) after install — presence in /user/ssh-keys is necessary but not sufficient — and returns { privateKeyPath, algorithm, batchVerified }. Best-effort: a failed probe records batchVerified=false and never aborts provisioning. - Persist the fact as MachineState.managedSshKey (to machine.json). This is the data source #71's descriptor reads to advertise SSH private-key batch auth as usable ONLY when batchVerified is true. - Integration test (test/integration/provisioning.test.ts) asserts the persisted fact + an independent BatchMode login; verified live on 7.20.8 and 7.23.2. - Docs aligned: provisioning.instructions.md, REPORT.md §6, CHANGELOG. Closes #74. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a ChangesManaged SSH key verification and persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 7, 2026
There was a problem hiding this comment.
Pull request overview
This PR strengthens quickchr’s provisioning pipeline by turning the managed SSH key into a persisted, machine-level “fact” that downstream consumers (notably the upcoming #71 descriptor) can trust for batch-mode SSH handoff.
Changes:
- Add a
ManagedSshKeyfact (path/algorithm/batchVerified/verifiedAt) and persist it onMachineState/machine.json. - Update provisioning to verify managed-key usability via a real host OpenSSH
BatchMode=yeslogin (best-effort; does not fail provisioning). - Add an integration assertion that both persistence and independent batch login work; align lab report, provisioning instructions, and changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/provision.ts |
Adds managed-key batch verification and returns/passes the persisted managedSshKey fact. |
src/lib/quickchr.ts |
Persists managedSshKey into state and warns when verification fails. |
src/lib/types.ts |
Introduces ManagedSshKey and attaches it to MachineState. |
test/integration/provisioning.test.ts |
Verifies managedSshKey persistence and validates batch SSH login works. |
test/lab/ssh-keys/REPORT.md |
Updates lab narrative to reflect the new persisted/verified data source. |
.github/instructions/provisioning.instructions.md |
Documents the settled algorithm and the persisted verification fact. |
CHANGELOG.md |
Records the new behavior and persistence in Unreleased notes. |
Comment on lines
381
to
+383
| lastListBody = listBody; | ||
| const keys = JSON.parse(listBody) as Array<{ user?: string }>; | ||
| if (Array.isArray(keys) && keys.some((k) => k.user === username)) return; | ||
| if (Array.isArray(keys) && keys.some((k) => k.user === username)) { listed = true; break; } |
Comment on lines
+290
to
+294
| "-o", "StrictHostKeyChecking=no", | ||
| "-o", "UserKnownHostsFile=/dev/null", | ||
| "-o", "PasswordAuthentication=no", | ||
| "-o", "BatchMode=yes", | ||
| "-o", "ConnectTimeout=10", |
Comment on lines
+488
to
+492
| "-o", "StrictHostKeyChecking=no", | ||
| "-o", "UserKnownHostsFile=/dev/null", | ||
| "-o", "PasswordAuthentication=no", | ||
| "-o", "BatchMode=yes", | ||
| "-o", "ConnectTimeout=10", |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
This was referenced Jul 7, 2026
mobileskyfi
added a commit
that referenced
this pull request
Jul 7, 2026
Follow-up to the unresolved review threads on the now-merged PR #82. ## What changed - Match the generated RouterOS SSH key row by user, generated key comment (`info ?? key-owner`), key type, and fingerprint when RouterOS exposes one. - Normalize trailing SHA256 fingerprint padding because RouterOS 7.20.8 includes base64 padding while OpenSSH-style fingerprints omit it. - Run production and integration OpenSSH probes with `IdentitiesOnly=yes` and `-F` to the platform null device so agent/config identities cannot produce false-positive batch verification on Unix or Windows. - Reuse the production SSH-key matcher from the integration test so key-type and optional fingerprint behavior stay aligned. - Fail provisioning if quickchr cannot fingerprint its own generated managed SSH public key. - Update the scoped provisioning instructions, SSH key lab report, and changelog to reflect the stricter verification contract. ## Why The PR #82 review was correct: the previous check could accept any key for the same user, and `ssh -i` without isolating OpenSSH identity/config behavior could still authenticate through other configured identities. That could make `MachineState.managedSshKey.batchVerified` look stronger than it really was, which matters for the upcoming #71 descriptor. ## Validation - `bun run check` - `bun test test/unit/` — 686 pass, 18 skip, 0 fail - `QUICKCHR_INTEGRATION=1 bun test test/integration/provisioning.test.ts` — 11 pass, 0 fail
This was referenced Jul 9, 2026
This was referenced Jul 10, 2026
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
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.
Completes #74 and delivers the data source #71 was blocked on.
Decision (settled, grounded)
Keep ed25519 as the managed key. Grounded in
test/lab/ssh-keys/REPORT.md: accepted by/user/ssh-keys/add|importfrom RouterOS 7.12+, and batch-login-verified on the 7.20.8 floor and current stable. RSA-2048 is only a sub-7.12 fallback quickchr never provisions; ECDSA is rejected by RouterOS.What changed
installSshKey()now verifies with a real batch login. Presence in/user/ssh-keysis necessary but not sufficient — it follows up withssh -o BatchMode=yes -o PasswordAuthentication=noand returns{ privateKeyPath, algorithm, batchVerified }. Best-effort: a failed probe recordsbatchVerified: falseand never aborts provisioning.MachineState.managedSshKey(tomachine.json). This is exactly what API: live descriptor contract for centrs --quickchr targets #71's descriptor reads to advertiseservices.ssh.auth.privateKeyPath/ batch key auth as usable only whenbatchVerifiedis true.machine.json) and an independentBatchMode=yeslogin — verified live on 7.20.8 and 7.23.2.provisioning.instructions.md,REPORT.md §6,CHANGELOG.Acceptance (#74) → evidence
BatchMode=yes/PasswordAuthentication=nologin →test/integration/provisioning.test.ts.provision.ts(ed25519).privateKeyPath/ batch auth as usable only when known-good →managedSshKey.batchVerifiedis the gate.What's left for #71 (separate session)
This PR provides the fact; #71 implements the descriptor v1 shape (the
servicesmap,ssh.auth.{modes,batchModes,privateKeyPath}) that consumesMachineState.managedSshKey. No further #74 work blocks it.Local verification: 683 unit tests pass; integration green on 7.20.8 + 7.23.2; tsc/biome/markdownlint/cspell clean.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes