Skip to content

Keep the appliance's mandatory SSH password when a key is added - #481

Merged
onel merged 3 commits into
devfrom
fix/477-appliance-password-factor
Sep 11, 2026
Merged

onel merged 3 commits into
devfrom
fix/477-appliance-password-factor

Conversation

@onel

@onel onel commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

What & why

AUTH.md # Device access gives each profile a mandatory SSH factor and an optional second one: the appliance takes the malmo password with a key as the extra lock, hosted takes a key with the password as the extra lock. Only the hosted row was enforced.

On the appliance, an account that added a key and left require_password unset got AuthenticationMethods publickey — the mandatory factor replaced rather than supplemented. The field is omitempty, so a client sending {"enabled": true} lands there. That is the default path, not an odd one.

The fix is in the brain, and the placement is the substance of it. internal/hostagent/sshaccess renders exactly what it is handed, and given a key with RequirePassword false its methods() is correct to write publickey alone. host-agent does not know the environment profile and must not guess which factor is mandatory — docs/progress/ssh-per-account-access.md states that division of labour outright. So host-agent was not wrong; the brain was handing it a false the appliance is not allowed to ask for. Nothing under internal/hostagent/ is touched.

setMySSH now resolves the caller's value against the profile through effectiveRequirePassword before anything is written. Once, at the single entry pointsetMySSH is the only place a caller-supplied require_password enters, since reapplySSH, the deleteUser restore and both rollback paths read the stored row. So normalising before the store write makes every downstream push correct without touching them, and the stored row stops describing a posture sshd is not running. The audit record uses the effective value too, so Activity says what the box did rather than what was asked.

Second of three stacked fixes from the Greptile review on the v0.12.0 release PR (#474).

Supersedes #478. That PR was the same branch based on #476. Merging #476 with --delete-branch removed the base branch, which GitHub closes dependent PRs for and will not let you reopen. The branch is rebased onto the merged dev and targets dev directly; the two review findings it already picked up (the invented reapplySSH name, and the test that claimed to cover the omitted field) are in the history and stay fixed.

Spec(s) touched

docs/specs/AUTH.md # Device access. The profile table already said this; what is added is a sentence naming where both rows are enforced and why it cannot be host-agent, so the next reader does not have to re-derive that seam. No DECISIONS.md entry: 2026-09-09 set the rule and nothing about it flipped, the code simply did not implement half of it.

What was tested

  • Re-ran make check after the rebase onto the merged dev: exit 0.

  • Three tests in internal/api/ssh_test.go: the appliance sends require_password: true to the host and reports it in the DTO after a key is added; the appliance with no key does the same; hosted still honours a false and authenticates with the key alone.

  • The two appliance tests were run against the unfixed handler and fail there, with the host call showing RequirePassword:false sitting next to the key. The hosted test passes either way, which is its job — it exists to prove the fix did not spill across the profile seam.

  • make check green, exit 0.

Known gaps & deviations

  • Nothing is proved against a real sshd. The assertion stops at what the brain sends host-agent. That AuthenticationMethods publickey,password actually makes sshd demand both is covered by the ssh cloud boot from SSH in the images: package it, scope it, and prove the port opens and closes #470, which runs on hosted only. No lane exercises the appliance renderer on a booted box.
  • Existing rows are not migrated. An appliance account that enabled SSH before this keeps require_password = false until it next writes. There are no such boxes — SSH has never shipped in a release and has no UI — so a migration would be code with no subject.
  • require_password is still accepted and ignored on the appliance rather than rejected 422. Stricter would break a client that sets the field uniformly across profiles, and the DTO's key_required already tells a panel which factor is which.
  • Still no Device access UI, so this remains reachable only by API. Unchanged by this PR.

Platform gaps

None.

Definition of done

  • Behavior works in the inner loop; integration-tested as far as the brain-to-host seam (see gaps for what is not).
  • Tests added at the right layer; make check green.
  • No test deleted or newly skipped: the diff adds three tests and deletes none.
  • Progress entry written (docs/progress/appliance-ssh-password-factor.md); progress index updated.
  • Catalog change: n/a.
  • Catalog change, new published field: n/a.
  • Spec doc updated (AUTH.md); no DECISIONS.md entry needed.
  • No section-sign symbol, no hard-wrapped markdown, log/slog only, conventions per CLAUDE.md.
  • Branch off the stack base; PR into Serve no CORS headers from the brain #476.

Closes #477

AUTH.md # Device access gives each profile a mandatory factor and an
optional second one: the appliance takes the malmo password with a key as
the extra lock, hosted takes a key with the password as the extra lock.
Only the hosted row was enforced.

An appliance account that added a key and left require_password unset got
AuthenticationMethods publickey, so the mandatory factor was replaced
rather than supplemented. The field is omitempty, so that is the default
path a client takes, not an odd one.

The fix belongs in the brain. sshaccess renders what it is handed, and
given a key with RequirePassword false its methods() is right to write
publickey alone -- host-agent does not know the profile and must not guess
which factor is mandatory. The brain was handing it a false the appliance
is not allowed to ask for.

setMySSH is the only entry point a caller-supplied value comes in through;
reapplySSH, the deleteUser restore and both rollbacks read the stored row.
So resolving once before the store write makes every downstream push
correct and stops the row describing a posture sshd is not running.

Closes #477
Two review findings.

The placement comment named reapplySSH, which does not exist -- the
stored-row reapply path is syncSSHIfEnabled. The same invented name was in
the progress entry and the index, where it made the argument for the
placement harder to check.

The appliance test said it covered the omitted-field default and then sent
require_password explicitly. Both shapes decode to the same boolean today,
but omitting it is what a panel actually sends, so the two now run as
separate subtests and a later presence-sensitive decoder cannot break the
default path unnoticed.
@greptile-apps

greptile-apps Bot commented Sep 11, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge; both previous findings are resolved and no new actionable failures remain.

Reviews (2) · Last reviewed commit: "Audit what was asked, and give each subt..."

Comment thread internal/api/ssh.go
Comment thread internal/api/ssh_test.go Outdated
Two review findings.

The audit meta carried the resolved require_password on every path,
including the records written when the request was refused and nothing was
applied. On the appliance that describes a posture the box never took and
hides what the caller actually sent. It now carries the requested value,
which is what every record here is about, and the success record adds
require_password_applied beside it where there is a real applied state.

The two request-shape subtests shared one harness whose host-call slice
accumulates, and the helper read only the latest call -- so a shape that
stopped calling the host could pass on the other's leftover call. Each
subtest gets its own harness and asserts it made exactly one call.
@onel
onel merged commit 0f7f8da into dev Sep 11, 2026
2 checks passed
@onel onel mentioned this pull request Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Appliance SSH drops its mandatory password factor when a key is added

1 participant