Skip to content

Propagate KeyGuard attestation failures and bridge native MAA logs to the MSAL logger#6081

Merged
gladjohn merged 4 commits into
mainfrom
gladjohn/keyguard-attestation-error-and-logs
Jun 26, 2026
Merged

Propagate KeyGuard attestation failures and bridge native MAA logs to the MSAL logger#6081
gladjohn merged 4 commits into
mainfrom
gladjohn/keyguard-attestation-error-and-logs

Conversation

@gladjohn

Copy link
Copy Markdown
Contributor

Summary

Fixes two defects in the IMDSv2 mTLS PoP + Credential Guard / KeyGuard attestation flow.

1. Propagate KeyGuard attestation failures (instead of sending an empty token to IMDS)

Previously, a failed attestation was mapped to null, which caused an empty / non-attested certificate request to be sent to IMDS — silently dropping the real failure (e.g. an MAA 400 PolicyEvaluationError / x-ms-azurevm-dbxvalidated=false deny).

  • WithAttestationSupport now throws MsalServiceException("attestation_failed") carrying Status / NativeErrorCode / reason instead of returning null. The failure originates from the MAA service, so it surfaces as a service exception.
  • ImdsV2ManagedIdentitySource.GetAttestationJwtAsync treats a null/empty token from a configured provider as a hard failure for KeyGuard keys (no silent non-attested fallback) and avoids double-wrapping MsalServiceException.
  • AttestationClient maps the native return code to a readable reason via AttestationErrors.Describe so the cause propagates.
  • Client-side preconditions (mtls_pop_requires_keyguard, credential_guard_requires_cng) remain MsalClientException.

2. Bridge native MAA logs into the MSAL logger

Native AttestationClientLib (MAA) logs were written only to System.Diagnostics.Trace and never reached the MSAL ILoggerAdapter, so failures were invisible in MSAL verbose logs.

  • AttestationLogger.CreateLoggerBridge forwards native log callbacks into the MSAL logger (Error/Warn/Info/DebugError/Warning/Info/Verbose), gated on IsLoggingEnabled, exception-safe, with a Trace fallback when no logger is supplied.
  • AttestationClient takes an optional ILoggerAdapter (keeping a strong reference to the bridge delegate for the native callback lifetime); PopKeyAttestor forwards the logger.

Validation

  • Verified end-to-end on a real Confidential VM: native MAA logs now flow through the MSAL logger, and the attested happy path works.
  • Added/updated unit tests in ImdsV2Tests covering attestation_failed propagation (NativeError / Exception / TokenEmpty / NotInitialized / empty-JWT, and null / empty / throwing providers) and the log bridge (level mapping, routing, gating, exception safety, null logger) plus AttestationErrors.Describe.
  • Microsoft.Identity.Test.Unit builds clean (net8.0 + netstandard2.0); ImdsV2Tests 99/99 pass.

Fixes #6079
Fixes #6080

… the MSAL logger

KeyGuard attestation failures were silently swallowed: a failed attestation
returned null, which caused an empty / non-attested certificate request to be
sent to IMDS instead of surfacing the failure. Separately, native
AttestationClientLib (MAA) logs were written only to System.Diagnostics.Trace
and never reached the MSAL ILoggerAdapter, so the real failure reason was
invisible in MSAL verbose logs.

Error propagation:
- WithAttestationSupport now throws MsalServiceException("attestation_failed")
  carrying Status / NativeErrorCode / reason instead of returning null. The
  failure originates from the MAA service, so it is surfaced as a service error.
- ImdsV2ManagedIdentitySource.GetAttestationJwtAsync treats a null/empty token
  from a configured provider as a hard failure for KeyGuard keys (no silent
  non-attested fallback) and avoids double-wrapping MsalServiceException.
- AttestationClient maps the native return code to a readable reason via
  AttestationErrors.Describe so the cause propagates to the caller.
- Client-side preconditions (mtls_pop_requires_keyguard,
  credential_guard_requires_cng) remain MsalClientException.

Log bridging:
- AttestationLogger.CreateLoggerBridge forwards native AttestationClientLib log
  callbacks into the MSAL ILoggerAdapter (Error/Warn/Info/Debug ->
  Error/Warning/Info/Verbose), gated on IsLoggingEnabled and exception-safe,
  with a Trace fallback when no logger is supplied.
- AttestationClient takes an optional ILoggerAdapter and keeps a strong
  reference to the bridge delegate for the native callback lifetime;
  PopKeyAttestor forwards the logger.

Tests:
- Added/updated unit tests in ImdsV2Tests covering attestation_failed
  propagation (NativeError/Exception/TokenEmpty/NotInitialized/empty-JWT,
  null/empty/throwing providers) and the log bridge (level mapping, routing,
  gating, exception safety, null logger) plus AttestationErrors.Describe.

Fixes #6079
Fixes #6080
Copilot AI review requested due to automatic review settings June 19, 2026 21:52
@gladjohn
gladjohn requested a review from a team as a code owner June 19, 2026 21:52

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 fixes two related defects in the IMDSv2 mTLS PoP + Credential Guard/KeyGuard attestation path by (1) ensuring attestation failures are surfaced to callers (instead of silently falling back to a non-attested IMDS request) and (2) forwarding native AttestationClientLib (MAA) logs into the MSAL ILoggerAdapter for diagnosability.

Changes:

  • Convert KeyGuard attestation failure cases into MsalServiceException("attestation_failed") and treat null/empty attestation tokens as hard failures for KeyGuard keys.
  • Add a native-to-MSAL logger bridge (AttestationLogger.CreateLoggerBridge) and plumb ILoggerAdapter through PopKeyAttestorAttestationClient.
  • Expand unit coverage for attestation failure propagation, native error reason mapping, and logger level mapping/routing.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ImdsV2Tests.cs Updates/expands unit tests to validate attestation_failed propagation and native log bridging behavior.
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs Enforces hard-fail behavior for empty attestation JWTs (KeyGuard) and avoids double-wrapping MsalServiceException.
src/client/Microsoft.Identity.Client.KeyAttestation/PopKeyAttestor.cs Passes MSAL logger through to the native attestation client to enable bridged logging.
src/client/Microsoft.Identity.Client.KeyAttestation/ManagedIdentityAttestationExtensions.cs Changes .WithAttestationSupport() to throw MsalServiceException("attestation_failed") on attestation failure instead of returning null.
src/client/Microsoft.Identity.Client.KeyAttestation/Attestation/AttestationLogger.cs Implements logger bridging and level mapping from native AttestationClientLib logs into MSAL logs.
src/client/Microsoft.Identity.Client.KeyAttestation/Attestation/AttestationClient.cs Adds optional logger support, keeps a strong reference to the native callback delegate, and maps native error codes to readable reasons.

Exclude OperationCanceledException (incl. TaskCanceledException) from the generic catch in GetAttestationJwtAsync so cancellation bubbles out unchanged instead of being masked as an attestation_failed service error. Adds a unit test. Addresses PR review feedback.
@Sheshagiri

Copy link
Copy Markdown

thanks a lot @gladjohn for accomodating the changes. Would this also propage the logs from the underlying cpp code (AttestationClientLib) when we set AZURE_LOG_LEVEL=verbose?

@gladjohn

Copy link
Copy Markdown
Contributor Author

thanks a lot @gladjohn for accomodating the changes. Would this also propage the logs from the underlying cpp code (AttestationClientLib) when we set AZURE_LOG_LEVEL=verbose?

yes, @Sheshagiri it will

…ngelog

- AttestationLogger.CreateLoggerBridge: route native Error/Warning/Info to the
  scrubbed (non-PII) slot so MAA diagnostics stay visible, and route the most
  verbose output (Debug -> Verbose, plus unknown levels) through the PII slot so
  any richer payload fragments are redacted unless PII logging is enabled.
- Update routing test and add AttestationLogger_CreateLoggerBridge_VerboseLine_LogsAsPiiNotScrubbed.
- CHANGELOG: call out the fail-closed behavior change for KeyGuard attestation
  plus attestation_failed propagation and native MAA log bridging.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 24, 2026 00:57

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

…pport

WithAttestationSupport used IsNullOrEmpty, so a whitespace-only JWT from a
"Success" AttestationResult was returned as a valid token. Downstream IMDSv2
code (IsNullOrWhiteSpace) then threw a generic "returned no token" error,
discarding the richer Status/NativeErrorCode/Reason. Use IsNullOrWhiteSpace so
the failure is surfaced here with full detail. Adds regression test
MtlsPop_WithAttestationSupport_SuccessButWhitespaceJwt_ThrowsAttestationFailedWithReason.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jul 11, 2026
neha-bhargava added a commit that referenced this pull request Jul 14, 2026
Add 4.86.0 changelog section (#6096 failure metadata, #6099 background-refresh callback, #6092 explicit-region IMDS skip, #6081 KeyGuard attestation fixes) and a backfilled 4.85.2 section (#6077 refresh-token cache partitioning, #6059 mTLS PoP min-strength), then move all unshipped public API entries to shipped across the six TFMs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants