Refuse to store a provider key in plaintext (fail closed on write, tolerant on read) by rbuergi · Pull Request #2126 · Systemorph/MeshWeaver

rbuergi · 2026-08-24T09:47:04Z

🚨 Rotation is required regardless, and it is not this PR

This stops NEW plaintext credentials. It does not un-leak the existing ones. Provider/_Policy
has publicRead: true, and every value written in the clear is also in the node's version
history — so a green merge here is not "the leak is handled". Any credential that was stored
unencrypted must be rotated at its source, and that is the operator's action, not a code change.

The contradiction this PR was stuck on

Since 2026-08-24 the branch neither refused plaintext nor passed:

ProviderKeyProtectorTest.NoMasterKey_RefusesToWrite_ButStillReads asserted Protect("plain") throws.
The branch's most recent commit was "revert the protector throw for real — it was breaking the seeder".

The mechanism was removed and the assertion asserting it was left standing.

Why the seeder broke — and why it is not exempted

ProviderCredentialSeed (the boot-time step that copies {Section}:ApiKey from a deployment's
configuration onto its Provider/{name} node) never wanted the passthrough. It wanted a
capability question — "can this deployment encrypt?" — and answered it by calling Protect and
inspecting what came back, which only worked because Protect degraded:

var protectedKey = protector is null ? c.ApiKey : protector.Protect(c.ApiKey);
var outcome = Decide(current, hasConfiguredKey: true, protectionAvailable: IsProtected(protectedKey));

So it already refuses correctly — ProviderSeedOutcome.RefusedUnprotected, logged at Error, node
left keyless. It just discovered the state from a fallback. Making Protect throw turns that into
an exception inside a sequential per-provider pipeline, which aborts the seed for every remaining
provider instead of reporting one refusal.

The choice, plainly

Option 1 (give the seeder a master key) — chosen where it applies, but it cannot be the whole
answer. The seeder's own fixture, ProviderCredentialSeedWithoutMasterKeyTest, is an
unconfigured deployment, and unconfigured deployments exist in production — that is the reported
leak. The refusal path must keep working without a master key, so no amount of configuration
removes the need for the seeder to decide.

Where option 1 does apply is the test environment, and that turned out to be the real finding:
MeshWeaver.Fixture now configures a test master key for every mesh test. The default test
environment was the misconfigured one — which is precisely why this survived. As the original PR
body put it, ProviderKeyEncryptionTest stayed green because it configures a master key, so the
degradation only happened where nobody was testing. It lives in ServiceSetup rather than
test/appsettings.json because eleven test projects ship their own file that shadows the shared
one. Fixtures that must exercise the unprotected deployment register their own IConfiguration and
still do.

Option 2 (stop seeding secrets) would delete a convergence the platform needs: measured on
memex.systemorph.com, Provider/Anthropic was created keyless on 2026-08-14, Anthropic__ApiKey
was configured after it, and the node stayed keyless until a human pasted the key in on 08-21. The
seeder exists to close that gap; removing it re-opens a real operational defect to fix a different one.

Option 3 (a named exemption) was not needed — so it was not taken. The seeder asks
IMasterKeyProvider before protecting:

private static string? ProtectOrNothing(
    IProviderKeyProtector? protector, IMasterKeyProvider? masterKeys, string? configuredKey)
{
    if (protector is null || masterKeys?.GetMasterKey() is null)
        return null;                       // a DECISION NOT TO WRITE, never a fallback
    return protector.Protect(configuredKey);
}

null feeds IsProtected(...) → RefusedUnprotected. There is no branch from here to a persist,
the plaintext is never returned, and the produced bytes are still checked for the enc: tag before
the write. The refusal in Protect is therefore absolute, with no exempted caller.

The passthrough was in five more places, all silent

This is the part the PR title understates. IProviderKeyProtector is not provider-key-only — it also
carries a GitHub PAT, the plugin registry's sync-token signing key, an installation's
registry credential, and the Entra EA refresh token. Every one of them wrote
protector?.Protect(x) ?? x on top of a degrading Protect — the same leak, in four more
subsystems, equally unreported. Leaving them would have shipped a PR titled "refuse to store a key in
plaintext" that refused in one place out of six.

Call site	Before	Now
`ProviderCredentialSeed`	probe via passthrough	asks `IMasterKeyProvider`, reports `RefusedUnprotected`
`ModelProviderService.Protect`	duplicated master-key probe + dead null branch	delegates; one rule, one place
`ModelProviderLayoutAreas.SaveKey`	`protector is null ? newKey : …`	protects inside the chain → refusal lands on `onError`, not out of an `OnNext`
`GitHubCredentialService.Protect`	`protector is null ? plaintext : …`	required; refusal reaches the caller
`SyncTokenSigningKeyService.Protect`	`protector?.Protect(e) ?? e`	required — this key signs every sync token
`InstanceAutoRegistrationService`	`protector?.Protect(k) ?? k`	required

ConfigMasterKeyProvider's "no key configured" line moves from Information to Error. It used
to read "keys stored as plaintext" at Information — the log line that described the leak and was
never acted on.

🚨 The asymmetry is unchanged and deliberate

Unprotect() stays a passthrough. Instances already hold keys written in the clear by the old
behaviour; refusing on read would take them down on upgrade for data the platform itself produced.
Fail on the way IN, tolerate on the way OUT.

Operational consequence, stated plainly

A deployment with no Ai:KeyProtection:MasterKey can no longer store a credential: entering a
provider key fails with an actionable message, a GitHub credential cannot be saved, a registry can
no longer mint a sync-token signing key, and EA token rotation fails. That is the intended reading of
"a missing master key is a configuration fault, not a degraded mode" — every one of those paths was
previously writing a live secret in cleartext. deploy/helm/values.yaml and values.aks.yaml already
carry the key; it is supplied out-of-band per environment.

Tests

ProviderKeyProtectorTest.NoMasterKey_RefusesToWrite_ButStillReads — the assertion that has
been red for two days, kept as-is and now green. It is the test that fails without the fix.
ProviderKeyNoPlaintextTest — refusal names the fault, never echoes the key; and the keyless
boundary is now genuinely asserted (null / "" / already-tagged still pass without a master key)
rather than asserting a service is non-null.
ProviderKeyProtectorRegistrationTest — asserted Unprotect(Protect(x)) == x "whether or not
this host configures a master key", which a passthrough satisfies. Now asserts the stored form
is enc:v1: ciphertext, which it cannot.
NoPlaintextCredentialFallbackGuard (new ratchet) — fails on any Protect call site carrying
??, is null ? or ?.Protect(. It caught the first version of the seeder's own fix, which
is the point: the guard, not care, is what keeps the refusal from widening back.

Verified locally, Release -warnaserror clean on every touched project:

Suite	Result
`MeshWeaver.AI.Test` (provider-key / seed / service)	36 / 36
`MeshWeaver.Auth.Test` (sync-token + instance registration)	22 / 22
`MeshWeaver.GitSync.Test`	187 / 187
`MeshWeaver.PluginCatalog.Test` (`ModuleAutoDiscovery`)	14 / 14
`MeshWeaver.Graph.Test` (protector registration)	1 / 1
`MeshWeaver.Documentation.Test` (all guards)	70 / 70

What's New entry: Category: Fix — and it says the rotation part out loud, for the same reason this
body does.

🤖 Generated with Claude Code

One follow-up, in the sibling repo — deliberately not in this PR

MeshWeaver.Plugins → src/MeshWeaver.Blazor.Portal/Chat/ThreadChatView.razor.cs:1717
(StoreHarnessCredential) carries the same shape this PR removes here:

var stored = protector is null ? credential : protector.Protect(credential);

It compiles fine against this change (behaviour-only, no signature change) and it stops leaking as
soon as this merges — but the refusal would surface as an unhandled Blazor exception instead of
its own "Couldn't store the credential: {ex.Message}" status, because Protect runs before the
CreateOrUpdateNode(...).Subscribe(_, ex => …) that has the handler. The fix is the same one applied
to ModelProviderLayoutAreas.SaveKey: protect inside the chain so the refusal lands on the existing
onError. Platform first, then the dependent repo.

… write ProviderKeyProtector.Protect() returned the plaintext unchanged whenever no master key was configured: var key = masterKeyProvider.GetMasterKey(); if (key is null) return plaintext; // encryption disabled → passthrough so an unconfigured deployment PERSISTED RAW PROVIDER KEYS into node content. Nothing failed and nothing logged at the call site, and ProviderKeyEncryptionTest stayed green because it configures a master key — the degradation only occurred where nobody was testing. Found 2026-08-24 with a live OpenRouter key sitting in cleartext in Provider/OpenRouter, readable by anyone holding read on that namespace. A provider key is the one value in that record that must never be written in the clear, so a missing master key is a CONFIGURATION FAULT, not a degraded mode. Protect() now throws, naming the missing config key AND the two supported ways to avoid storing a literal at all (ModelDefinition.ApiKeySecretRef, or the provider's {section}:ApiKey). A refusal that does not say what to do next is how the passthrough survived unnoticed. 🚨 THE ASYMMETRY IS DELIBERATE. Unprotect() stays a passthrough: instances already hold keys written in the clear by the old behaviour, and refusing on READ would take them down on upgrade for data the platform itself produced. Fail on the way IN, tolerate on the way OUT — new plaintext is impossible, existing installations keep working while they migrate. Also note where the fix does NOT belong: ModelProviderService.Protect() guards `protector is null`, but IProviderKeyProtector is registered unconditionally (TryAddSingleton in LanguageModelNodeType), so that branch is dead. The first attempt patched it there and the new test caught it by not throwing — the fallback is one layer down, in the protector itself. ProviderKeyNoPlaintextTest pins the unconfigured deployment: storing a key throws, the message names the fault, and it never echoes the key it refused. A keyless provider (Copilot, local Claude Code CLI) stays valid — null/empty is returned before the master-key check. ProviderKeyEncryptionTest still passes, which is what proves the configured path is untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

This PR hardens provider credential storage by ensuring provider keys are never persisted in plaintext when a deployment is missing the configured master key, while keeping reads tolerant to avoid breaking upgrades that already contain legacy plaintext values.

Changes:

Update ProviderKeyProtector.Protect() to fail closed (throw) when asked to protect a non-empty, untagged key but no master key is configured.
Add a new regression test covering the “no master key configured” deployment case and asserting the refusal does not echo the secret.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
test/MeshWeaver.AI.Test/ProviderKeyNoPlaintextTest.cs	Adds a regression test for refusing to store non-empty provider keys without a master key configured.
src/MeshWeaver.AI/ProviderKeyProtector.cs	Changes provider-key protection to throw (fail closed) when no master key is configured, while keeping `Unprotect()` tolerant for legacy plaintext.

Suppressed comments (2)

test/MeshWeaver.AI.Test/ProviderKeyNoPlaintextTest.cs:64

The test name implies the protector is missing, but this fixture still registers IProviderKeyProtector; the scenario is “no master key configured”. Renaming the test makes the intent match the actual setup.

    [Fact]
    public async Task WithoutAProtector_StoringAKey_IsRefused_NotSilentlyStoredInPlaintext()
    {

test/MeshWeaver.AI.Test/ProviderKeyNoPlaintextTest.cs:89

This test claims keyless providers are still allowed without a protector, but it only asserts ModelProviderService can be resolved. To actually pin the “null/empty key is a valid write” requirement, create a provider with apiKey:null and assert the stored ApiKey remains null (and no exception is thrown).

    [Fact]
    public void AKeylessProvider_IsStillAllowed_WithoutAProtector()
    {
        // Keyless providers (GitHub Copilot, local Claude Code CLI) legitimately carry no key.
        // Refusing those would break them, so null/empty must stay a valid write.
        var service = Mesh.ServiceProvider.GetRequiredService<ModelProviderService>();
        Assert.NotNull(service);   // constructing the service must not require a protector
    }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

github-actions · 2026-08-24T09:59:22Z