Skip to content

feat(security): sealed secret envelope and the control instance's sealing key - #7108

Merged
rbuergi merged 19 commits into
mainfrom
feat/secret-sealing
Oct 24, 2026
Merged

rbuergi merged 19 commits into
mainfrom
feat/secret-sealing

Conversation

@rbuergi

@rbuergi rbuergi commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

What

Step 1a of the vault-only client setup (design: `Google/ConnectViaMother.md` in MeshWeaver.Plugins). A platform primitive so an installation can hand a secret to its control instance without the value ever being plaintext in a node, an event or a log.

  • `SecretEnvelope` (Mesh.Contract): ECDH P-256 + HKDF-SHA256 + AES-256-GCM, .NET built-ins only, versioned, key id, associated data binds the ciphertext to its context (`deployment|object`) so it cannot be replayed onto another deployment or object.
  • `SecretSealingKeyService` (PluginCatalog): one node `Admin/SecretSealingKey/current`, minted on first use after authentication (an anonymous request cannot provoke the write), read back so replicas agree. The private half is only `enc:` ciphertext of the platform key protector, refused without a master key; only the public half is served. Rotation (90 days or on demand) keeps the outgoing key for a 7-day window.
  • `GET /api/instances/seal-key` (Memex.Portal.Shared), mapped with `MapInstanceRegistration`: authenticated by the instance key, answers key id, public key, algorithm, version.
  • Doc: `Doc/Architecture/SecretSealing`.

Tests (`SecretSealingTest`, real mesh + test server)

Round trip; two seals differ; wrong context, key, key id, version, tampered ciphertext/tag/ephemeral key all fail to open; empty/oversized refused; anonymous request answers 401 and mints nothing; the served key seals a value only the service opens; the stored private half is `enc:` ciphertext; rotation keeps the outgoing key for the window and then only the new one opens.

Order

Platform ADDITION: nothing depends on it yet. The Hosting and Hosting.Instance halves (MeshWeaver.Plugins) follow.

🤖 Generated with Claude Code

Also in this PR

operator.selfServiceSecrets on the deployment record (HostingOperatorSpec): the vault objects the instance's own global administrators may write through the control lane. Empty by default; nothing changes for a record until it opts in. Never rendered into the portal configuration (test).

…ling key

An installation seals a secret to a public key the control instance publishes, so the value is
ciphertext in every node, event and log. ECDH P-256 + HKDF-SHA256 + AES-256-GCM (.NET built-ins),
bound to the deployment and object it is for, versioned with a key id, with bounded-window rotation.
The private half is enc: ciphertext of the platform key protector; only the public half is served.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 24, 2026 09:48
@rbuergi
rbuergi enabled auto-merge October 24, 2026 09:48
…instance's own administrators may write (default empty)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@meshweaver-cloud
meshweaver-cloud Bot disabled auto-merge October 24, 2026 09:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Rotation can invalidate still-needed keys, inbound envelopes lack size bounds, and key-resolution faults bypass the intended error response.

4 open issues
What changed in this PR

Adds the platform foundation for installations to send encrypted secrets to their control instance, ahead of the planned Hosting integration.

Changes:

  • Introduces a versioned, context-bound secret envelope using ECDH, HKDF and AES-GCM.
  • Adds protected sealing-key storage, rotation and an authenticated public-key endpoint.
  • Documents the protocol and adds cryptographic and real-mesh integration tests.
File Description
test/​Memex.Portal.Shared.Test/​SecretSealingTest.cs Tests encryption, authentication, protected storage and rotation.
src/​MeshWeaver.PluginCatalog/​SecretSealingKeyService.cs Manages sealing keys and opens encrypted envelopes.
src/​MeshWeaver.PluginCatalog/​PluginCatalogConfigurationExtensions.cs Registers the key service and infrastructure node type.
src/​MeshWeaver.Mesh.Contract/​Security/​SecretEnvelope.cs Defines encryption primitives, key records and wire contracts.
src/​MeshWeaver.Documentation/​Data/​Architecture/​SecretSealing.md Documents secret transport, key lifecycle and intended consumers.
memex/​Memex.Portal.Shared/​Api/​SealKeyEndpoints.cs Serves the public sealing key to authenticated installations.
memex/​Memex.Portal.Shared/​Api/​InstanceRegistrationEndpoints.cs Maps the sealing-key endpoint alongside registration routes.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 24, 2026 09:56
rbuergi and others added 2 commits October 24, 2026 12:03
…control plane mints the pair

An installation holds the control-inbox announcement key, not necessarily an instance key at the control
instance, so the public key must be fetchable without one. A public key is harmless to publish; the write
(minting) is never provoked by a request.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Ambiguous associated-data encoding allows the public envelope primitive to accept coordinated changes to the key id and context.

4 open issues
1 previously missed issue

In code that hasn't changed since last review

Medium severity Ambiguous envelope encoding permits key and context substitution

src/​MeshWeaver.Mesh.Contract/​Security/​SecretEnvelope.cs:134

Sealing with keyId="k1", context="a|b", then changing these to keyId="k1|a", context="b" produces identical associated-data bytes, so the same private key still opens the envelope. The service's key lookup blocks this substitution, but the public SecretEnvelope primitive does not enforce its different-context rejection contract. Encode the fields unambiguously, update the documented wire contract, and add a regression test that changes both fields together.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 24, 2026 10:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

    31 files  ± 0      31 suites  ±0   1h 7m 54s ⏱️ -8s
14 520 tests +23  14 323 ✅ +23  197 💤 ±0  0 ❌ ±0 
14 550 runs  +23  14 353 ✅ +23  197 💤 ±0  0 ❌ ±0 

Results for commit 27ae84c. ± Comparison against base commit 4287711.

This pull request removes 44 and adds 41 tests. Note that renamed tests count towards both.

   --- End of inner exception stack trace ---
   --- End of inner exception stack trace ---)
   --- End of inner exception stack trace ---, expected: True)
   --- End of inner exception stack trace ---, isDenial: True)
 ---> (Inner Exception #1) MeshWeaver.Mesh.QueryProviderStalledException: Query provider(s) [pg] did not emit an Initial within the query fan-in's 16s bound for query 'nodeType:NodeType' (user 'system'). The merged Initial gates on EVERY provider, so this query has NO snapshot to answer with — it is reported as unavailable (retryable) rather than left hanging with no error. This is an availability failure, never a permission verdict: a consumer deciding access must fail CLOSED and say it could not establish the answer. Fix the stalled provider; never bump the consumer's timeout.<---
 ---> (Inner Exception #1) MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432<---
 ---> (Inner Exception #1) System.ArgumentException: Value does not fall within the expected range.<---
 ---> (Inner Exception #1) System.InvalidOperationException: boom<---
 ---> (Inner Exception #1) System.InvalidOperationException: source B is misconfigured<---
…
Memex.Portal.Shared.Test.InstanceIdRulesMatchTheRegistryTest ‑ TheSetupHostAgreesWithTheRegistry(candidate: "f2995759-13be-4678-95fb-d9701dbbd420")
Memex.Portal.Shared.Test.PublishStorageFaultIsNotAConflictTest ‑ AStorageFault_IsA500_NamingTheError(fault: System.AggregateException: One or more errors occurred. (Access to the path '/data/modules/activation.d/MeshWeaver.Markdown.Export.json' is denied.)
 ---> System.UnauthorizedAccessException: Access to the path '/data/modules/activation.d/MeshWeaver.Markdown.Export.json' is denied.
   --- End of inner exception stack trace ---)
Memex.Portal.Shared.Test.PublishStorageFaultIsNotAConflictTest ‑ AStorageFault_IsA500_NamingTheError(fault: System.InvalidOperationException: landing faulted
 ---> System.UnauthorizedAccessException: Access to the path '/data/modules/activation.d/MeshWeaver.Markdown.Export.json' is denied.
   --- End of inner exception stack trace ---)
Memex.Portal.Shared.Test.SecretSealingAuthorizationTest ‑ AnOrdinaryCaller_CannotOpenASealedSecret_NorRotateTheKey
Memex.Portal.Shared.Test.SecretSealingAuthorizationTest ‑ TheElevatedWork_RunsOutsideThePermissionFoldsGate
Memex.Portal.Shared.Test.SecretSealingTest ‑ ADifferentContext_DifferentKey_OrTamperedBytes_NeverOpen
Memex.Portal.Shared.Test.SecretSealingTest ‑ AKeyIdItsOwnReaderWouldRefuse_IsRefusedAtSealing
Memex.Portal.Shared.Test.SecretSealingTest ‑ ALookupThatFails_IsAnswered503_NotAnUnhandledFault
Memex.Portal.Shared.Test.SecretSealingTest ‑ ANegatedEphemeralPoint_DoesNotAuthenticate
Memex.Portal.Shared.Test.SecretSealingTest ‑ AReadThatIsNotAnAnswerOfAbsence_IsAFault_NeverANullKey
…

♻️ This comment has been updated with latest results.

rbuergi and others added 3 commits October 24, 2026 12:35
…r, every outgoing key kept for its window, 503 on a failed lookup

- SecretEnvelope: TryParse/Unseal refuse oversized wire and fields before decoding.
- SecretSealingKeyService: the due-check and rotation run inside the owner's update (a lost race adopts the landed key); SecretSealingKey.Retired keeps every unexpired outgoing key.
- SealKeyEndpoints: a faulted lookup is a logged 503 inside the chain.
- Route contract documents the anonymous, read-only publication; topic map lists SecretSealing; the test's 30 s literal uses TestTimeouts.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…out keys include the second one

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 24, 2026 10:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rbuergi and others added 2 commits October 24, 2026 18:21
- Seal refuses a key id its own reader rejects (1..64 chars).
- The AAD length-prefixes the key id and the context, so moving a separator
  between them no longer authenticates.
- The sealing-key read uses GetMeshNodeOutcome: only Absent is "no key";
  a delete in flight or an unanswered read is a fault, never a mint.
- DeploymentRecordJson.ForPortal drops Operator.SelfServiceSecrets, so the
  allow-list never reaches the portal through Deployment__Record.

Each has a test that failed before the fix.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 24, 2026 16:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unchecked System elevation exposes privileged operations, and delayed cross-replica rotation patches can discard keys before their grace periods expire.

3 open issues
3 resolved since last review

Review effort: Balanced

Comment on lines +118 to +120
return hub.GetWorkspace().GetMeshNodeStream(SecretSealingKeys.Path)
.Update(node => node.ContentAs<SecretSealingKey>(hub.JsonSerializerOptions) is { } there
? node with { Content = RotateAtOwner(there, candidate, rotate, DateTimeOffset.UtcNow) }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, and fixed in fa79deb. My earlier reply on this file said the owner runs the due-check. That was inaccurate: the lambda runs on the writer's mirror (MeshNodeStreamHandle.UpdateRemote), and the owner only applies the three-way merge.

What the owner does with a delayed rotation, traced in code:

  • Base values: MeshNodePatchMerge.ExtractBaseValues skips a field whose base is null.
  • Owner merge: MeshNodePatchMerge.Apply then applies the patch value unconditionally (the "no base signal" branch).
  • Partial refusal: the refusal still commits what did not conflict (DataExtensions.cs, the "A PARTIAL refusal is still a refusal" block). The refused current-key fields NACK Conflict, and the re-run finds the key not due, so it is a no-op.

So a mint left Retired = null, and a rotation delayed behind K0 → KA → KC replaced [K0, KA] with [K0]. Exactly your scenario.

The fix is minimal and keeps the design. SecretSealingKey.Retired is never null (= []), so the field always carries a base. The owner then takes the array branch: a three-way merge by element identity that applies the writer's additions and removals onto the LIVE list. The delayed write appends and never replaces. The current-key fields were already safe, because every rotation changes all of them, so a stale writer's values conflict and are refused.

I did not move this to an owner-side watcher. That would redesign the PR, and the merge contract already gives the right answer once the base is carried.

Test: SecretSealingTest.ARotationDelayedBehindTwoOthers_KeepsEveryKeyRetiredSinceItsMirrorWasRead. It drives the REAL diff (MeshNodeStreamHandle.ComputeMergePatchDiff over ToJsonObject) and the REAL owner merge (MeshNodePatchMerge.Apply with ExtractBaseValues). The writer's mirror is K0 and overdue, while the owner holds K0 → KA → KC. The test asserts KA and K0 still open.

Negative control: with Retired defaulting to null, it failed with "Expected value to be "enc:kA" … but found ". Doc/Architecture/SecretSealing now describes this accurately.

Copilot AI balanced review requested due to automatic review settings October 24, 2026 16:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A valid modification of the ephemeral public key can still authenticate, leaving the envelope’s tamper-rejection guarantee incomplete.

3 open issues
1 resolved since last review

Review effort: Balanced

…under a stale rotation

Unseal and Rotate elevated to System with no check on the caller. The service
is a mesh singleton, so any code holding a hub could resolve it and open a
sealed secret. Both now check the caller's own right on
Admin/SecretSealingKey/current first: Execute to open, Update to rotate.
Denied throws UnauthorizedAccessException. An undetermined check fails closed.
Only then does the work run as System.

The rotation's Update lambda runs on the writer's mirror. Its merge patch
reaches the owner with the writer's base values. Retired was null on a
freshly minted key, so that field carried no base, and the owner's merge let
the patch replace the live list. A rotation delayed behind two others
(K0 -> KA -> KC) therefore dropped KA. Retired is now never null, so the owner
merges it as a list and the delayed write appends.

Tests: SecretSealingAuthorizationTest (an outsider is refused, then the
administrator's same calls work), and
ARotationDelayedBehindTwoOthers_KeepsEveryKeyRetiredSinceItsMirrorWasRead,
which runs the real merge diff and the real owner merge. Both fail with their
fix reverted.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 24, 2026 17:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Concurrent stale rotations can extend a retired key’s acceptance beyond its original grace deadline.

3 open issues
1 resolved since last review

Review effort: Balanced

rbuergi and others added 2 commits October 24, 2026 20:53
…red key keeps its first deadline

The negated ephemeral point (x, p - y) yields the same ECDH x-coordinate and so the same AES key; without the
transmitted SPKI bytes in the associated data a rewritten epk still authenticated. It is now the last
length-prefixed AAD field in Seal and Unseal, and the format doc says so.

Two rotations computed from the same key at different times each append a retirement for it, and the owner's
list merge keeps both. ProtectedPrivateKeyFor now decides on the earliest entry for the id, and RotatedTo
collapses duplicates to it, so a later writer never extends the window.

Regressions (both failed before the fix): ANegatedEphemeralPoint_DoesNotAuthenticate,
TwoRotationsOfTheSameKeyAtDifferentTimes_KeepTheFirstDeadline (real diff + real owner merge).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 24, 2026 18:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The cryptographic protocol and cross-replica key lifecycle need human security review, with retirement cleanup and unreadable-record handling still unresolved.

3 open issues
2 resolved since last review

Review effort: Balanced

rbuergi and others added 2 commits October 24, 2026 22:07
…present non-key node is a fault

A rotation's Retired list reaches the owner as a three-way merge by element, so a writer removes only the entries
its mirror held. Dropping an expired first retirement let a later entry for the same key (unseen by that writer,
or added by a rotation delayed past the cleanup) become the earliest and open the expired key again. RotatedTo now
clears the expired entry's key material and keeps it with its deadline, so the first deadline still decides.

KeyFrom throws for a Present node whose content does not read as a SecretSealingKey, reserving null for Absent,
so Resolve/Rotate never try to create a key over an existing node.

Regressions (all failed before the fix): CleaningUpAnExpiredKey_FromAPartialMirror_DoesNotReopenIt and
ARetirementDelayedPastTheCleanup_DoesNotReopenTheKey (real diff + real owner merge), the Present-but-not-a-key
case in AReadThatIsNotAnAnswerOfAbsence_IsAFault_NeverANullKey, and the tombstone expectation in
TwoRotationsInsideOneWindow_KeepEveryOutgoingKeyUntilItsOwnDeadline.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 24, 2026 20:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The cryptographic protocol and distributed key rotation warrant human security review, with an unresolved authorization concurrency concern.

2 open issues
2 resolved since last review

Review effort: Balanced

rbuergi and others added 2 commits October 24, 2026 22:33
Unseal and Rotate took the caller's decision with a bare Take(1), so on a
warm cache the System pipeline (key reads, and the rotation's write) was
subscribed while the permission evaluator still held its Rx gates (#899).
The decision is now taken with TakeDecisionOutsideGate. The composition is
an internal static AuthorizedThen, pinned by a test whose decision emits
inside a held lock; with Take(1) restored it fails (the work saw the lock).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 24, 2026 20:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new cryptographic wire format and cross-replica key lifecycle warrant final human security review.

2 open issues
1 resolved since last review

Review effort: Balanced

Comment on lines +140 to +151
/// What a rotation makes of the key as it is at the OWNING hub: when <paramref name="rotate"/> still holds for
/// it, <paramref name="candidate"/> becomes current and the outgoing key is retired; otherwise it is left exactly
/// as it is, so a request that read the same overdue key as another ADOPTS the rotation that landed first
/// instead of replacing it. Pure.
/// </summary>
internal static SecretSealingKey RotateAtOwner(SecretSealingKey there, SecretSealingKey candidate,
Func<SecretSealingKey, bool> rotate, DateTimeOffset now)
=> rotate(there) ? there.RotatedTo(candidate, now, SecretSealingKeys.RotationGrace) : there;

// 🚨 The due-check and the rotation happen together, inside the update at the owning hub, which serialises
// them: two requests that both read an overdue K0 can never publish KA and KB, both naming K0 — the second
// finds KA current (not due) and leaves it. The answer is the key as stored afterwards, never the proposal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 27ae84c. The RotateAtOwner summary and the comment on Rotated now say the update lambda runs on the writer's mirror, not on the owning hub. What reaches the owner is the merge patch plus the writer's base values, and the owner merges them three-way. A patch on a stale base is refused on the current key's fields, and the writer reads back the stored key. A writer whose mirror already shows the rotation adopts it. That is the account Architecture/SecretSealing.md gives. Only the comments changed; the code did not.

rbuergi and others added 2 commits October 24, 2026 23:16
…eral 30 s test timeout

- SecretSealingKeyService: the RotateAtOwner summary and the Rotated comment said the owning hub
  runs the due-check and rotation. The update lambda runs on the WRITER's mirror, and the owner
  merges the patch three-way, refusing a stale base (as Architecture/SecretSealing.md says).
- SecretSealingAuthorizationTest: [Fact(Timeout = 30_000)] took TestTimeoutLiteralRatchetGuard
  to 381 (baseline 380) and turned shard 0 red. It is now 120_000, the house constant-attribute
  value; the guard is back to green locally (3/3).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 24, 2026 21:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new cryptographic protocol and cross-replica key lifecycle warrant human security review, particularly with an unresolved retirement-deadline race.

3 open issues

Review effort: Balanced

/// </summary>
public string? ProtectedPrivateKeyFor(string keyId, DateTimeOffset now)
=> keyId == CurrentId ? ProtectedCurrentPrivateKey
: (Retired ?? []).Where(r => r.Id == keyId).MinBy(r => r.ValidUntil) is { } retired && retired.StillValid(now)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changed in this PR. The shortening is real, but it is bounded by the delay between the two writes reaching the owner. That is the staleness of A's mirror, so seconds against a 7-day RotationGrace. It also errs closed: a value sealed to K0 stops opening slightly early, and no key ever opens past a deadline someone computed. "The earliest entry decides" is deliberate. The two ways of deriving the deadline from the accepted transition are "latest wins" and "the entry whose successor is the stored CurrentId". Under the owner's element-wise array merge, both let a delayed or replayed rotation extend or reopen an expired key, which is the exposure the tombstone rule exists to stop. Pinning the deadline to the accepted transition needs a conditional merge at the owner, which is a change to the merge-patch semantics and beyond this PR.

@rbuergi
rbuergi merged commit be3c548 into main Oct 24, 2026
65 checks passed
@rbuergi
rbuergi deleted the feat/secret-sealing branch October 31, 2026 09:12
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.

2 participants