feat(lps): sealed password transport — agent seals to control X25519 key, gateway relays opaque#483
Merged
Merged
Conversation
LPS rotated passwords crossed agent→gateway→control as cleartext inside mTLS, so the gateway — the least-trusted server-side actor (no DB, no encryption key) — could read every rotated password (manchtools/power- manage-agent#62). This removes the gateway from the confidentiality boundary: the agent seals each password to a control-owned X25519 key, the gateway relays opaque bytes, and control unseals at receipt before the existing at-rest encryption path. Recovery (GetDeviceLpsPasswords) is unchanged. Spec: docs 06-specs/18-lps-sealed-transport.md. - migration 010: single-row lps_keypair table (infra state, not event- sourced). Private key stored ONLY enc:v2 (AAD-bound), never cleartext. - EnsureLpsKeypair (advisory-locked, first-writer-wins ON CONFLICT) runs at control boot; replicas converge on one key via Postgres. Fatal on failure — running without it would silently disable LPS rotation. - ProxySyncActions attaches the CA-signed public key (new lps-pubkey domain) so a relaying gateway cannot substitute its own key; the agent verifies fail-closed against its enrollment CA. - ProxyStoreLpsPasswords unseals SealedPassword (bound to device|action|username) then re-encrypts at rest. An unsealable blob is InvalidArgument (permanent — no inbox retry loop) with NO event appended; a nil keypair fails closed. Blobs/plaintext never logged. - gateway proxyLpsRotations parses base64 sealed_password and DROPS any legacy cleartext entry loudly (never proxies or enqueues cleartext) — the old-agent + new-server upgrade path. Tests: keypair idempotency + concurrency + at-rest-encryption, signed- pubkey distribution + key-swap rejection, full seal→store→recover E2E with the real SDK primitive, unseal-rejection (tampered/wrong-key/wrong- context) with no-event assertion, nil-keypair fail-closed, gateway legacy-drop. Unseal-reject guard red-checked. Closes #481 was separate; refs manchtools/power-manage-agent#62
Address local review of the sealed-LPS handler: - ProxyStoreLpsPasswords is now two-phase: unseal + re-encrypt + parse the ENTIRE batch (staging) before appending any event. A later entry's unseal failure previously returned InvalidArgument AFTER earlier entries were already appended, leaving a partial batch the permanent error would not retry. Staging makes it all-or-nothing. - BuildSignedLpsPublicKey parses the key before signing (rejects a nil/malformed key rather than distributing a CA-signed unusable one) and signs the parsed key's own bytes, so a later mutation of the caller's slice cannot desync the bytes from the signature. - Drop rotated usernames from the gateway drop-path and handler unseal/encrypt error logs — device_id + action_id locate the failure without persisting an account identifier in error logs. New test TestProxyStoreLpsPasswords_BatchIsAtomic pins the [good, bad] batch → InvalidArgument + zero events appended.
📝 WalkthroughWalkthroughThis PR implements sealed LPS password transport: agents seal rotated passwords to a control-owned X25519 public key, the gateway relays opaque sealed bytes, and control unseals/re-encrypts them at rest. Adds keypair bootstrap, signing, storage, staged batch persistence, updated agent parsing, migration, queries, tests, and an ADR. ChangesSealed LPS Password Transport
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant Gateway
participant ControlProxy as InternalHandler
participant Encryptor
participant Store
Agent->>Agent: seal password with control public key
Agent->>Gateway: sealed_password (base64)
Gateway->>ControlProxy: ProxyStoreLpsPasswords(SealedPassword)
ControlProxy->>ControlProxy: OpenLpsPassword(SealedPassword)
alt unseal fails
ControlProxy-->>Gateway: InvalidArgument
else unseal succeeds
ControlProxy->>Encryptor: EncryptWithContext(plaintext)
ControlProxy->>Store: append lps_password event
Store-->>ControlProxy: persisted / error
ControlProxy-->>Gateway: success or Internal (retry)
end
sequenceDiagram
participant Main as cmd/control/main.go
participant Api as EnsureLpsKeypair/BuildSignedLpsPublicKey
participant Store
participant Handler as InternalHandler
Main->>Api: EnsureLpsKeypair(store, encryptor)
Api->>Store: load or insert keypair
Store-->>Api: private key, public key
Main->>Api: BuildSignedLpsPublicKey(publicKey, signer)
Api-->>Main: signed LpsPublicKey
Main->>Handler: SetLpsKeypair(priv, signedPublicKey)
Handler-->>Agent: LpsPublicKey (via SYNC response)
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: security, database, needs-migration Suggested reviewers: (based on ownership of 🐰 A rabbit seals a secret tight, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/store/migrations/010_lps_keypair.sql (1)
15-23: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider a length CHECK on
public_key.
public_keyis documented as a fixed 32-byte X25519 key but has no length constraint.decodeLpsKeypair(internal/api/lps_keypair.go) already validates this at read time, so this is purely a defense-in-depth suggestion for data integrity at the DB layer.♻️ Optional constraint
public_key BYTEA NOT NULL, + -- Defense-in-depth: application already validates, but a DB-level + -- constraint catches a malformed row before it reaches decode time. + CHECK (octet_length(public_key) = 32),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/migrations/010_lps_keypair.sql` around lines 15 - 23, Add a defensive length constraint for the lps_keypair.public_key column in the CREATE TABLE statement so the database enforces the documented fixed 32-byte X25519 size. Update the migration in 010_lps_keypair.sql by adding a CHECK on public_key, and keep the existing read-time validation in decodeLpsKeypair unchanged since this is only extra data integrity protection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/store/migrations/010_lps_keypair.sql`:
- Around line 15-23: Add a defensive length constraint for the
lps_keypair.public_key column in the CREATE TABLE statement so the database
enforces the documented fixed 32-byte X25519 size. Update the migration in
010_lps_keypair.sql by adding a CHECK on public_key, and keep the existing
read-time validation in decodeLpsKeypair unchanged since this is only extra data
integrity protection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9907fb4f-4a95-4a7d-8fb8-22e7b146ebff
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.suminternal/store/generated/lps_keypair.sql.gois excluded by!**/generated/**internal/store/generated/models.gois excluded by!**/generated/**
📒 Files selected for processing (13)
cmd/control/main.godocs/adr/0028-lps-sealed-password-transport.mdgo.modinternal/api/internal_handler.gointernal/api/internal_handler_gateway_binding_test.gointernal/api/internal_handler_test.gointernal/api/lps_keypair.gointernal/api/lps_sealed_transport_test.gointernal/handler/agent.gointernal/handler/agent_action_result_test.gointernal/handler/control_proxy_test.gointernal/store/migrations/010_lps_keypair.sqlinternal/store/queries/lps_keypair.sql
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.
Server side of the LPS wire-cleartext fix (manchtools/power-manage-agent#62, option C). Spec:
docs 06-specs/18-lps-sealed-transport.md. Depends on SDKcrypto.SealLpsPassword/OpenLpsPassword+verify.LpsPublicKeySignatureDomain(#263/#264, merged; SDK pin bumped to a314641).Threat closed
LPS rotated passwords crossed agent→gateway→control as cleartext inside mTLS, so the gateway — the least-trusted server-side actor (no DB, no encryption key) — could read every rotated password. The agent now seals each password to a control-owned X25519 key; the gateway relays opaque bytes; control unseals at receipt then re-encrypts with the existing at-rest path.
GetDeviceLpsPasswordsrecovery is unchanged.Changes
lps_keypairtable (infra state, not event-sourced). Private key stored ONLYenc:v2(AAD-bound), never cleartext. New file (009 already applied in rc envs).ON CONFLICT DO NOTHING) at control boot; replicas converge on one key via Postgres. Fatal on failure.power-manage-lps-pubkeydomain) so a relaying gateway cannot substitute its own key; agent verifies fail-closed.InvalidArgument, zero events (atomic; no inbox retry loop). Nil keypair fails closed.sealed_password, DROPS legacy cleartext entries loudly (old-agent upgrade path) — never proxies/enqueues cleartext.Tests
Keypair idempotency + concurrency + at-rest-encryption; signed-pubkey distribution + key-swap rejection; full seal→store→recover E2E with the real SDK primitive; unseal rejection (tampered/wrong-key/wrong-context) with no-event assertion; batch atomicity ([good,bad] → no events); nil-keypair fail-closed; gateway legacy-drop. Unseal-reject guard red-checked.
Local CodeRabbit: 3 findings, all fixed in the second commit (batch atomicity, pubkey validate+copy, username log redaction).
Full gate green under the pinned SDK: gofmt, vet, staticcheck,
internal/api+internal/handler+cmd/control+internal/storetest suites, archtest.Summary by CodeRabbit
New Features
Bug Fixes
Documentation