Skip to content

[Epic] Azure Service Bus — no message-lock renewal; session settlement misreports success #307

Description

@brenpike

Summary

The Azure Service Bus adapter's documented invariants largely hold, and the 488-line .Auth package is clean on every classic credential-handling failure mode. The substantive findings are a missing lock-renewal mechanism, a settlement that lies about its outcome, and one hardening gap on a secret.

Note that the untrusted-header steering finding from this shard is tracked in the header trust boundary epic, since the root cause is the shared context-merge seam rather than this module.

The credential package is clean

Recorded explicitly because this was the audit's predicted highest-risk package:

Credentials enter only two ways. A connection-string SAS is bound from configuration and held on a DI singleton, consumed once at lazy client build. For AAD, the credential is constructed eagerly at registration and the client secret or certificate passes through into the Azure.Identity credential — the package itself retains only the client id. Token acquisition, caching, refresh and expiry are wholly delegated to Azure.Identity and the SDK's CBS layer.

The package mints no SAS token, maintains no token cache, performs no secret comparison, and uses no crypto primitive on a secret. Because there is no cache, there is no cross-entity cache-keying risk. The .Auth package contains zero logging; the main package logs only the fully-qualified namespace and never the connection string. The certificate-not-found exception includes only a thumbprint, which is not a secret.

The one residual gap is ServiceBusOptions.ConnectionString — the single secret-bearing member not marked [JsonIgnore], on a type whose two other sensitive members are marked, which shows whole-object serialization was anticipated. Any consumer diagnostic that dumps bound options emits the shared access key verbatim. One attribute.

Two documented-by-design sharp edges are worth surfacing in docs rather than filing as defects: a blank secret or thumbprint silently downgrades to DefaultAzureCredential (so an empty environment variable authenticates as the ambient host identity instead of failing), and a supplied TokenCredential is silently discarded when the connection string carries a SAS.

No message-lock renewal

Repo-wide grep for RenewMessageLock returns zero hits. Non-session PeekLock receivers use a bare SDK receiver with no renewal loop and without the processor's auto-renewal machinery.

The session path does renew its session lock, which makes the asymmetry read as an oversight rather than a decision.

Any handler running longer than the entity's lock duration — often left at the 30-second default — loses the lock. The broker redelivers while the first handler is still running, and the first handler's Complete throws MessageLockLost. Because each redelivery increments DeliveryCount, a consistently slow but otherwise successful handler will walk a healthy message all the way to dead-letter.

Settlement that reports success without settling

Under session mode, if the held session has been released before settlement runs, the adapter's complete, abandon and deadletter methods return a completed task without touching the broker — and the receiver then reports SettlementResult.Settled().

This contradicts the module's own documented settlement doctrine (a settlement that cannot find its delivery reports Failed) and the recently introduced three-valued outcome. The core records a successful acknowledgement while the broker still holds the delivery, so the message is redelivered after lock expiry. For deadletter it is worse: the poison message is not dead-lettered but the pipeline believes it was, so it re-enters circulation.

An existing test pins the adapter-level no-op but nothing asserts the outcome the pipeline reports — which is exactly why this was invisible.

Also in scope

The scoped receiver and sender escape a disposed DI scope (the same pattern as the core; latent today because disposal is currently soft). The ReceiveOnly transaction suppression scope is disposed before the async sends it governs complete, so a send can enlist in the ambient receive transaction it was meant to be kept out of. Config-bound retry policy never binds — that one is tracked in the config binding epic since it shares the internal set root cause.

File scope

  • src/Chatter.MessageBrokers.AzureServiceBus/**
  • src/Chatter.MessageBrokers.AzureServiceBus.Auth/**

Versioning

Non-breaking, except that adding a lock-renewal duration knob is an additive public surface change warranting a minor bump.


Children

Tracked elsewhere

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    audit-2026-08From the 2026-08 security/bug/performance auditdifficulty:hightriage difficulty ratingeffort:mtriage effort ratingmoscow:musttriage moscow ratingpriority:hightriage priority ratingreadiness:readytriage readiness ratingrisk:mediumtriage risk ratingtype:epictriage type ratingwave:1Leaf module wave - fully parallel

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions