Skip to content

fix: only generate LoginSet SSH host keys on the create path - #247

Open
giuliocalzo wants to merge 1 commit into
SlinkyProject:mainfrom
giuliocalzo:fix/loginset-ssh-hostkeys-lazy-keygen
Open

fix: only generate LoginSet SSH host keys on the create path#247
giuliocalzo wants to merge 1 commit into
SlinkyProject:mainfrom
giuliocalzo:fix/loginset-ssh-hostkeys-lazy-keygen

Conversation

@giuliocalzo

Copy link
Copy Markdown
Contributor

Summary

The SSH Host Keys sync step builds the Secret — generating an RSA, an Ed25519 and an ECDSA keypair — before anything checks whether the Secret already exists, then hands it to SyncObject with shouldUpdate=true. Two problems follow:

  • Wasted work on every reconcile. The Secret is created Immutable: true and SyncObject skips immutable Secrets, so the three keypairs are generated and immediately discarded on every single LoginSet reconcile. RSA keygen defaults to 4096 bits, measured at 0.12–1.05 s per keypair on an M-series laptop, and worse again in a CPU-limited container.
  • Silent host key rotation. If the live Secret is not marked immutable — one restored from a backup or re-applied from a manifest that dropped the field — SyncObject takes the patch path and overwrites Data with the freshly generated keys on every reconcile. That rotates the SSH host keys out from under clients that have already accepted them, producing host key mismatch warnings on login nodes.

This adds objectutils.CreateObjectIfNotExists, which defers construction to a build closure that only runs on the create path, and uses it for the SSH host keys step. Net effect: keygen happens exactly once per LoginSet, and an existing host keys Secret is never rewritten regardless of its immutable field.

CreateObjectIfNotExists follows the existing SyncObject conventions — same argument order, same ReasonCreateSucceeded / ReasonCreateFailed events — and additionally treats a losing AlreadyExists create race as success.

Test plan

  • TestLoginSetReconciler_syncSshHostKeys — new; the keeps the keys of a mutable existing Secret case was verified to fail against the pre-change code (the RSA key was replaced with a freshly generated one) and to pass after.
  • TestCreateObjectIfNotExists — covers absent, already-exists (asserting the build closure is not invoked), build error, failing get, and losing create race.
  • go test ./internal/... ./api/... green, including the loginset envtest suite.
  • golangci-lint run ./internal/... introduces no new findings.

@giuliocalzo
giuliocalzo force-pushed the fix/loginset-ssh-hostkeys-lazy-keygen branch 3 times, most recently from 1200a0e to 4ee1203 Compare September 4, 2026 17:33
BuildLoginSshHostKeys generated an RSA, an Ed25519 and an ECDSA keypair
before anything checked whether the Secret already existed, so every
LoginSet reconcile paid for three keypairs that SyncObject then discarded
because the Secret is immutable.

Worse, a live Secret that is not marked immutable, one restored from a
backup that dropped the field for instance, was patched with the freshly
generated keys on every reconcile, rotating the host keys clients had
already accepted.

Look the Secret up and reuse the keys it holds, generating a set only when
there are none. The build then sync pattern of the reconcile step is
unchanged.

Changelog: Fixed - LoginSet SSH host keys are reused instead of regenerated on every reconcile
Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
@giuliocalzo
giuliocalzo force-pushed the fix/loginset-ssh-hostkeys-lazy-keygen branch from 4ee1203 to a043c2d Compare September 8, 2026 08:24
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.

1 participant