Conversation
DefaultScriptWebHookProvider looked up an extension's system key in the in-memory host-secrets cache. When Flex Consumption specialization seeds that cache from a startup context whose SystemKeys is incomplete, the lookup misses, the provider generates a replacement, and the blind upsert overwrites the key already held by the subscribing service - producing 401s on every subsequent delivery. A present-but-partial cache was treated as authoritative, so a stale "no" was indistinguishable from a real one. Move get-or-create into SecretManager, the only component that can tell those apart: - GetOrCreateSystemKeyAsync returns a cached hit unchanged. On a miss it invalidates only the snapshot it missed on, via Interlocked.CompareExchange so a concurrent reload is not discarded, then performs one authoritative repository read before concluding the key does not exist. - Creation is gated on Created/Updated. Any other result means nothing was persisted, and returning the value would mint a webhook URL carrying a key that authorizes nothing. - DefaultScriptWebHookProvider now only derives the key name. The interface documents that the create path remains a last-writer-wins upsert, so a key persisted concurrently by another instance can still be replaced; closing that needs an ETag/CAS write the repositories do not expose today. Tests assert zero writes and exactly one read on the miss path, that the invalidated cache heals, that a cached hit never touches the repository, that the provider passes the exact derived key name, and that a created key is persisted encrypted and decrypts to the returned value.
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The new public GetOrCreateSystemKeyAsync API should validate keyName upfront (and a new test helper should use ordinal string comparison) to avoid unclear exceptions and culture-sensitive behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR prevents extension webhook system keys from being re-minted and overwritten when the in-memory host-secrets cache is present but incomplete (notably during Flex Consumption specialization), which could otherwise break external webhook callers (e.g., Event Grid) after host restarts.
Changes:
- Moved “get-or-create system key” logic into
SecretManager, including cache miss invalidation and a single authoritative repository read before creating a new key. - Updated
DefaultScriptWebHookProviderto only derive the system key name and delegate resolution/creation toSecretManager. - Added/updated unit tests and documented the behavior in release notes.
File summaries
| File | Description |
|---|---|
| test/WebJobs.Script.Tests/Security/SecretManagerTests.cs | Adds coverage for cache-miss invalidation, repository-read behavior, and persistence gating for system key creation. |
| test/WebJobs.Script.Tests/DefaultScriptWebHookProviderTests.cs | Updates tests to validate derived key name while delegating key resolution to ISecretManager.GetOrCreateSystemKeyAsync. |
| test/WebJobs.Script.Tests.Shared/TestSecretManager.cs | Extends the test ISecretManager implementation with GetOrCreateSystemKeyAsync. |
| src/WebJobs.Script.WebHost/WebHooks/DefaultScriptWebHookProvider.cs | Stops generating/upserting keys in the provider; delegates to SecretManager. |
| src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs | Introduces GetOrCreateSystemKeyAsync with targeted cache invalidation + authoritative read before create. |
| src/WebJobs.Script.WebHost/Security/KeyManagement/ISecretManager.cs | Adds the new API contract and documents remaining last-writer-wins limitation. |
| release_notes.md | Adds a release note describing the customer-visible fix for 401s after restart due to overwritten extension system keys. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+1945
to
+1946
| reader.Setup(r => r.ReadValue(It.IsAny<Key>())) | ||
| .Returns<Key>(k => new Key(k.Name, k.Value.StartsWith("enc:") ? k.Value.Substring(4) : k.Value) { IsStale = false }); |
Comment on lines
+294
to
+300
| public async Task<string> GetOrCreateSystemKeyAsync(string keyName) | ||
| { | ||
| // A cached hit is as trustworthy as it is today. Only a cached MISS is challenged, | ||
| // because the startup context may contain a partial host secret snapshot. | ||
| var cached = _hostSecrets; | ||
| if (cached != null && cached.SystemKeys.TryGetValue(keyName, out string value)) | ||
| { |
Author
|
@microsoft-github-policy-service agree company="Microsoft" |
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
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.
Issue describing the changes in this PR
No public issue - found while investigating an internal incident where Event Grid deliveries to a Functions app started returning 401 after a host restart. Happy to open a tracking issue if the team prefers one.
Related but distinct: #11834 reports EventGrid 401s during Flex Consumption cold start. That failure is transient - the reporter notes retries succeed once the key finishes loading. This one is permanent, because the key is overwritten rather than merely not-yet-loaded, so retries never recover. This PR does not fix #11834.
What's wrong
DefaultScriptWebHookProviderlooked up an extension's system key in the in-memory host-secrets cache. On Flex Consumption, specialization seeds that cache from a startup context whoseSystemKeyscan be incomplete. When it is: the lookup misses, the provider generates a replacement, andAddOrUpdateFunctionSecretAsyncblindly upserts it - overwriting the key the subscribing service already holds.The webhook URL then carries a key the subscription doesn't have, so every delivery 401s. It is not self-healing: the host has no way to notify Event Grid, and the subscription stores the full URL, so recovery requires re-creating the subscription by hand.
The root cause is that a present-but-incomplete cache was treated as authoritative. A stale "no" was indistinguishable from a real one, and only one of those should authorize a write.
The fix
Move get-or-create into
SecretManager, the only component that can tell them apart:GetOrCreateSystemKeyAsyncreturns a cached hit unchanged. On a miss it invalidates only the snapshot it missed on (Interlocked.CompareExchange, so a concurrent reload isn't discarded), then performs one authoritative repository read before concluding the key doesn't exist.Created/Updated. Any other result means nothing was persisted, and returning the value would mint a webhook URL carrying a key that authorizes nothing.DefaultScriptWebHookProvidernow only derives the key name.AddOrUpdateFunctionSecretAsyncis unchanged -KeysControllerstill depends on its result codes for HTTP status mapping.Known limitation
The create path is still a last-writer-wins upsert, so a key persisted concurrently by another instance can be replaced. That window only opens during true first creation - not on restarts, scale-out, or specialization. Closing it needs an ETag/CAS write no
ISecretsRepositoryexposes today. The interface documents this rather than over-promising.Tests
On the miss path: zero
WriteAsync/WriteSnapshotAsync, exactly oneReadAsync, and the invalidated cache heals. A second test proves a cached hit never touches the repository. A third seeds a host document missing only the target key and asserts exactly one write whose persisted ciphertext decrypts to the returned value.DefaultScriptWebHookProviderTestsverifies the exact derived key name.Mutation-checked: disabling the invalidation fails with
Expected invocation on the mock should never have been performed, but was 1 times: WriteAsync- the incident itself, caught by assertion rather than inferred from a returned value.Pull request checklist
in-procbranch is not requiredrelease_notes.mdentry is included in this PR.Host secret ... Updatedevent simply stops firing spuriously.