Skip to content

Detach ImdsV2ManagedIdentitySource from AbstractManagedIdentity (refused-bequest cleanup)#6089

Merged
Robbie-Microsoft merged 1 commit into
mainfrom
rginsburg/imdsv2-source-hierarchy-cleanup
Jun 25, 2026
Merged

Detach ImdsV2ManagedIdentitySource from AbstractManagedIdentity (refused-bequest cleanup)#6089
Robbie-Microsoft merged 1 commit into
mainfrom
rginsburg/imdsv2-source-hierarchy-cleanup

Conversation

@Robbie-Microsoft

Copy link
Copy Markdown
Contributor

What

Removes the "refused bequest" code smell introduced by #6070: ImdsV2ManagedIdentitySource inherited AbstractManagedIdentity but used none of its token template, was forced to stub the abstract CreateRequestAsync with a throw, and was reached via a downcast in ManagedIdentityClient.

Fixes #6088.

Changes

  • New internal interface IImdsV2MtlsBindingSource exposing AcquireMtlsBindingForDelegationAsync.
  • ImdsV2ManagedIdentitySource now implements IImdsV2MtlsBindingSource instead of inheriting AbstractManagedIdentity; it owns its _requestContext/_isMtlsPopRequested fields, and the throwing CreateRequestAsync override is deleted.
  • ManagedIdentityClient source selection is split into a decision helper (SelectManagedIdentitySourceType(source, isImdsV2)) and instantiation. The PoP path constructs IMDSv2 directly through the interface — the downcast (is not ImdsV2ManagedIdentitySource) is gone. The bearer path resolves IMDS to IMDSv1 only.
  • Removed the obsolete ValidateServerCertificate_NotSetForImdsV2 test (asserted a base-class validation-callback behavior IMDSv2 no longer has).

Behavior preserved

All source-selection guards are unchanged: NoneFound throw, the per-request IMDSv1 fallback (isImdsV2 && !isMtlsPopRequested), the IMDSv1+PoP MtlsPopTokenNotSupportedinImdsV1 throw, the no-environment defaults, and the discovery cache (s_cachedSourceResult). The PoP→delegation vs. bearer routing in ManagedIdentityAuthRequest keeps the two paths mutually exclusive, and the mTLS PoP min-strength enforcement added in #6059 is retained at the top of AcquireImdsV2MtlsBindingAsync.

Validation

  • Build: 0 warnings / 0 errors across all TFMs (TreatWarningsAsErrors=true).
  • ImdsV2Tests: 92/92 pass.
  • ManagedIdentityTests: 433 pass / 1 skip / 0 fail.
  • No public API changes (all affected types are internal; PublicAPI.Unshipped.txt untouched).

Context

Follow-up to review feedback on #6070 (thread).

ImdsV2ManagedIdentitySource inherited AbstractManagedIdentity but used none of its token template and stubbed the abstract CreateRequestAsync with a throw (a refused bequest). ManagedIdentityClient selected via the shared AbstractManagedIdentity-typed path and downcast back to ImdsV2ManagedIdentitySource.

Introduce IImdsV2MtlsBindingSource implemented by ImdsV2ManagedIdentitySource instead of inheriting AbstractManagedIdentity. Split source selection in ManagedIdentityClient into a decision helper (SelectManagedIdentitySourceType) and instantiation, removing the downcast. All guards, discovery cache, and routing behavior preserved.

Remove the obsolete ValidateServerCertificate_NotSetForImdsV2 test (asserted a base-class validation-callback behavior IMDSv2 no longer has).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Robbie-Microsoft Robbie-Microsoft requested a review from a team as a code owner June 24, 2026 19:58
Copilot AI review requested due to automatic review settings June 24, 2026 19:58

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.

Pull request overview

This PR removes the “refused bequest” relationship between ImdsV2ManagedIdentitySource and AbstractManagedIdentity by introducing a focused internal interface for IMDSv2 mTLS binding minting, and refactors managed identity source selection to avoid downcasts while preserving existing routing/guard behavior.

Changes:

  • Introduced IImdsV2MtlsBindingSource and moved IMDSv2 “mint binding for delegation” behind the interface.
  • Refactored ManagedIdentityClient to separate source decision (SelectManagedIdentitySourceType) from instantiation, and to construct IMDSv2 via the interface for the PoP path.
  • Removed an obsolete IMDSv2 server-certificate-validation callback unit test that depended on the old inheritance structure.

Reviewed changes

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

File Description
tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ManagedIdentityTests.cs Removes an obsolete test tied to the previous IMDSv2 base-class behavior.
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs Detaches IMDSv2 from AbstractManagedIdentity and exposes binding minting via the new interface.
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/IImdsV2MtlsBindingSource.cs Adds an internal abstraction for the IMDSv2 mTLS binding minting operation used by delegation.
src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs Refactors selection/instantiation logic and routes PoP binding mint via the interface without downcasts.
Comments suppressed due to low confidence (1)

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs:416

  • AcquireMtlsBindingForDelegationAsync accepts a CancellationToken but only checks it once (ThrowIfCancellationRequested) and then performs all subsequent key/network operations using _requestContext.UserCancellationToken (e.g., GetCsrMetadataAsync / ExecuteCertificateRequestAsync / GetOrCreateKeyAsync). If these tokens ever differ, cancellation will not reliably abort the mint flow, which is inconsistent with other managed identity paths that pass the provided token through to HttpManager.
        public async Task<MtlsBindingInfo> AcquireMtlsBindingForDelegationAsync(
            ApiConfig.Parameters.AcquireTokenForManagedIdentityParameters parameters,
            bool forceRemint,
            CancellationToken cancellationToken)
        {
            _attestationTokenProvider = parameters.AttestationTokenProvider;
            _isMtlsPopRequested = true;

            if (forceRemint && _mtlsCache is MtlsBindingCache bindingCache)
            {
                bindingCache.RemoveBadCert(GetMtlsCertCacheKey(), _requestContext.Logger);
            }

            cancellationToken.ThrowIfCancellationRequested();

            return await AcquireMtlsBindingAsync().ConfigureAwait(false);
        }

@Robbie-Microsoft Robbie-Microsoft merged commit a06df82 into main Jun 25, 2026
17 checks passed
@Robbie-Microsoft Robbie-Microsoft deleted the rginsburg/imdsv2-source-hierarchy-cleanup branch June 25, 2026 19:21
This was referenced Jun 27, 2026
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.

Detach ImdsV2ManagedIdentitySource from AbstractManagedIdentity (refused-bequest cleanup)

4 participants