Skip to content

Implement mTLS Proof-of-Possession (mTLS PoP) token acquisition#610

Open
Robbie-Microsoft wants to merge 28 commits into
mainfrom
rginsburg/mtls_pop
Open

Implement mTLS Proof-of-Possession (mTLS PoP) token acquisition#610
Robbie-Microsoft wants to merge 28 commits into
mainfrom
rginsburg/mtls_pop

Conversation

@Robbie-Microsoft
Copy link
Copy Markdown

@Robbie-Microsoft Robbie-Microsoft commented Apr 8, 2026

mTLS Proof of Possession (mTLS PoP)

Implements mTLS Proof of Possession token acquisition for two scenarios, matching MSAL.NET's mTLS PoP implementation:

  • Path 1 (Confidential Client / SNI) — acquire an mTLS PoP token using a caller-supplied certificate presented during the TLS handshake to a regional mTLS endpoint
  • Path 2 (Managed Identity / IMDSv2) — acquire an mTLS PoP token on an Azure VM using an IMDS-issued binding certificate backed by a VBS KeyGuard-protected CNG key

Path 1 — Confidential Client

New option: confidential.WithMtlsProofOfPossession() on AcquireTokenByCredential.

The caller provides an x509.Certificate and matching private key as the client credential. MSAL presents the certificate during the TLS handshake to the regional mTLS endpoint — no client_assertion JWT is required. The token is
cached and discriminated by the certificate's x5t#S256 thumbprint so different certificates never share cache entries.

cred, err := confidential.NewCredentialFromCert([]*x509.Certificate{cert}, privateKey)
client, err := confidential.New(authority, clientID, cred)

result, err := client.AcquireTokenByCredential(ctx, scopes,
    confidential.WithMtlsProofOfPossession(),
)
// result.BindingCertificate — the cert bound to the token
// result.Metadata.TokenSource — Cache on repeat calls

Path 2 — Managed Identity (IMDSv2)

New option: managedidentity.WithMtlsProofOfPossession() on AcquireToken.

Fully automated — no certificates or keys to manage. MSAL handles the complete flow:

  1. Platform metadataGET /metadata/identity/getplatformmetadataclientID, tenantID, cuID, attestationEndpoint
  2. CNG key — created via NCryptCreatePersistedKey with a 3-level priority fallback (MSAL.NET parity):
    • KeyGuard (VBS Virtual Isolation) — requires Credential Guard active on the VM
    • Hardware (Software KSP, no VBS flags)
    • InMemory (ephemeral RSA)
  3. CSR — PKCS#10 built to match MSAL.NET Csr.Generate() exactly: CN={clientId} DC={tenantId} subject, RSASSA-PSS SHA-256 signature, CuID attribute (OID 1.3.6.1.4.1.311.90.2.10) in attributes [0]
  4. AttestationAttestationClientLib.dll produces a MAA JWT proving KeyGuard key protection (KeyGuard keys only; requires Trusted Launch VM)
  5. Credential issuancePOST /metadata/identity/issuecredential → binding cert issued by managedidentitysnissuer.login.microsoft.com
  6. TokenPOST {mtlsEndpoint}/{tenantID}/token with binding cert in TLS handshake
client, err := managedidentity.New(managedidentity.SystemAssigned())

result, err := client.AcquireToken(ctx, "https://graph.microsoft.com",
    managedidentity.WithMtlsProofOfPossession(),
)
// result.BindingCertificate — IMDS-issued cert; thumbprint matches cnf.x5t#S256 in JWT
// result.Metadata.TokenSource — Cache on repeat calls (cert + token both cached)

Caching

Path 1 Path 2
Token cache key token_type=mtls_pop + cert x5t#S256 token_type=mtls_pop + cert x5t#S256
Cert cache N/A — caller manages In-memory, 5-min pre-expiry buffer
CNG key N/A Persisted as MSALMtlsKey_{cuID} (Software KSP, USER scope)

Requirements

Path 1 Path 2
Platform Any Windows only (CNG + VBS)
Azure App registration with cert Azure VM with Managed Identity
VM Trusted Launch + Credential Guard active

Note: https://management.azure.com may return AADSTS392196 (resource not enrolled for mTLS PoP). Use https://graph.microsoft.com or https://storage.azure.com for testing.


Verified on

VM: MSIV2 (CentralUSEUAP, System-Assigned MI, Trusted Launch, Credential Guard active)
Key type: VBS KeyGuard-protected RSA-2048
Resource: https://graph.microsoft.com

Call 1 — TokenSource: Network
  cnf.x5t#S256: JL-waq-4TBfhpikp6X0du5SyIuOWAVCf5NBDDBEjmKs
  xms_tbflags:  2
  appidacr:     "2"
  aud:          https://graph.microsoft.com

Call 2 — TokenSource: Cache ✅

Documentation

Adds mTLS PoP token support for two paths:

Path 1 — Confidential Client (SNI certificate):
- MtlsPopAuthenticationScheme implementing AuthenticationScheme
- BuildMtlsEndpoint() for public/gov/china/dsts cloud routing
- NewMtlsHTTPClient() in comm package for per-request mTLS transport
- WithMtlsProofOfPossession() AcquireByCredentialOption on CCA
- WithSendCertificateOverMtls() ClientOption for bearer-over-mTLS
- WithAzureRegion() and AutoDetectRegion support
- Validation: tenanted authority, certificate credential, region required
- Cache isolation via x5t#S256 thumbprint in AppKey()
- AuthResult.BindingCertificate populated on success

Path 2 — Managed Identity (IMDSv2, Windows):
- WithMtlsProofOfPossession() AcquireTokenOption on MI client
- getPlatformMetadata() + issueCredential() IMDS v2 client
- GetOrCreateKeyGuardKey() using Windows CNG NCrypt (VBS KeyGuard)
- Non-Windows stub returns clear error
- In-memory cert cache with 5-min pre-expiry buffer
- Token acquisition via mTLS against IMDS-provided endpoint

Supporting changes:
- AuthParams: UseMtlsTransport, MtlsBindingCert fields
- TokenResponse: BindingCertificate field
- AuthResult: BindingCertificate field
- fake.AccessTokens: FromMtlsCertificate stub
- apps/errors/mtlspop.go: error code constants

Tests: 12 authority tests, 9 confidential tests (all passing)
Docs: docs/mtls-pop.md, docs/mtls-pop-manual-testing.md

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Robbie-Microsoft Robbie-Microsoft changed the title Implement mTLS Proof of Possession Implement mTLS Proof-of-Possession (mTLS PoP) token acquisition Apr 8, 2026
Robbie-Microsoft and others added 10 commits April 8, 2026 21:14
Path 2 (Managed Identity / IMDSv2) bug fixes:
- Fix JSON tags in csrMetadata struct (snake_case -> camelCase to match IMDS API)
- Add nested csrMetadataCuID struct with vmId field as required by IMDS JSON schema
- Add x-ms-client-request-id UUID header to all IMDS requests (required, 400 without)
- Add ?cred-api-version=2.0 query param to issuecredential URL
- Remove incorrect getIMDSAttestedDocument approach (PKCS7 != MAA JWT)

CNG Windows fixes:
- Fix NCryptSignHash: use bcryptPKCS1PaddingInfo struct with NCRYPT_PAD_PKCS1_FLAG
- Skip Export Policy property on Platform Crypto Provider (NTE_NOT_SUPPORTED)
- Add NCryptDeleteKey to clean up un-finalized keys from prior failed runs
- Detect broken keys by testing exportPublicKey; delete+recreate if needed

AttestationClientLib.dll integration:
- Add procInitAttestationLib, procAttestKeyGuardImportKey, procFreeAttestationToken
- Add attestationLogInfo struct and dummyLogCallback (non-null required by DLL)
- Implement GetKeyGuardAttestationJWT() for obtaining MAA JWT via AttestationClientLib
- Add cStringToGoString() helper to read ANSI null-terminated strings from DLL
- Improve error message: attestation failure now explains Trusted Launch requirement
- Add GetKeyGuardAttestationJWT stub to cng_stub.go (non-Windows builds)

Test fixes:
- Replace t.Context() with context.Background() in mtlspop_test.go (t.Context needs
  Go 1.24, module targets Go 1.18; fixes build failure on Go 1.22)

Manual test programs:
- Add apps/tests/devapps/mtls-pop/path1_confidential/main.go (4 error cases pass)
- Add apps/tests/devapps/mtls-pop/path2_managedidentity/main.go (MI flow)
- Add test cert/key/pfx for Path 1 testing

NOTE: Path 2 end-to-end requires a Trusted Launch Azure VM with attestation-capable
vTPM (Is Capable For Attestation = True, EK certificate present). Standard VMs
without Trusted Launch will fail at AttestKeyGuardImportKey with TPM_RC_HANDLE.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nch req

- Fix error reference table: use actual plain-English error messages returned
  by the library (not the code constants from apps/errors/mtlspop.go which are
  NOT what callers receive in err.Error())
- Add missing attestation error to error table
- Add note explaining the disconnect between error code constants and actual messages
- Update architecture diagram for Path 2: add GetKeyGuardAttestationJWT step
  (AttestationClientLib.dll → MAA JWT) and attestation_token in issuecredential call
- Update comparison table: MAA attestation is now Implemented (not Planned) via
  AttestationClientLib.dll integration
- Add Trusted Launch VM requirement to Path 2 (Secure Boot + vTPM required for
  Is Capable For Attestation = True)
- Fix CertFromPEM call signature in testing README sample code
  (was CertFromPEM(certPEM, keyPEM); correct is CertFromPEM(append(certPEM, keyPEM...), "))
- Fix error message strings in testing README error-case examples
- Add Trusted Launch requirement + attestation failure to Path 2 common failures
- Add downstream API call to Path 1 happy path test (makeDownstreamCall):
  builds mTLS HTTP client with binding cert, calls resource/v1.0/organization,
  distinguishes 200/401/403 to confirm TLS handshake and token acceptance
- Add -resource flag to path1_confidential/main.go for configurable downstream target

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Guard keys

MSAL.NET uses 'Microsoft Software Key Storage Provider' with
NCRYPT_USE_VIRTUAL_ISOLATION_FLAG (0x00020000) | NCRYPT_USE_PER_BOOT_KEY_FLAG
(0x00040000) passed to NCryptFinalizeKey to request VBS KeyGuard-protected keys.
This is NOT the same as 'Microsoft Platform Crypto Provider' (TPM-backed).

AttestKeyGuardImportKey in AttestationClientLib.dll expects a VBS KeyGuard key,
not a TPM key. Using Platform Crypto Provider was the root cause of attestation
failure — the DLL would fall through to TPM attestation which requires a fully
provisioned vTPM (Is Capable For Attestation = True).

Changes:
- Always use 'Microsoft Software Key Storage Provider' (remove Platform Crypto Provider)
- Add NCRYPT_USE_VIRTUAL_ISOLATION_FLAG and NCRYPT_USE_PER_BOOT_KEY_FLAG constants
- Attempt NCryptFinalizeKey with VBS flags; fall back to plain finalization if the
  flags are rejected (NTE_BAD_FLAGS) on VMs without KeyGuard Key Isolation support
- Always set Export Policy (was skipped for Platform Provider — no longer needed)
- Add procNCryptGetProperty and isKeyGuardProtected() for VBS protection verification
  (mirrors MSAL.NET KeyGuardAttestationTests.IsKeyGuardProtected using 'Virtual Iso' prop)
- Remove usingPlatformProvider flag (no longer needed)

On a VM with KeyGuard Key Isolation enabled:
  NCryptFinalizeKey with VBS flags succeeds -> VBS-backed key -> attestation works
On this VM (VBS flags rejected):
  Falls back to plain Software KSP key -> attestation DLL tries TPM -> fails

References:
- MSAL.NET KeyGuardAttestationTests.cs: CreateKeyGuardKey + IsKeyGuardProtected
- NCRYPT_USE_VIRTUAL_ISOLATION_FLAG defined as 0x00020000 in test constants

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
test-key.pem and test-mtls.pfx are secrets and must not be version-controlled.
test-cert.pem (public cert only) is safe to keep in the repo.

Changes:
- .gitignore: ignore test-key.pem and test-mtls.pfx under mtls-pop/
- git rm --cached both files (local copies preserved)
- docs/mtls-pop-manual-testing.md: Step 1 updated to explain that test-cert.pem
  is pre-committed, test-key.pem must be generated locally, with openssl and
  PowerShell instructions; note to re-upload cert if regenerated
- path1_confidential/main.go: comment updated to document key generation command

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…fallback

- Add managed_identity_key_type.go: enum (KeyGuard/Hardware/InMemory), mirrors
  MSAL.NET ManagedIdentityKeyType
- cng_windows.go: remove NCRYPT_MACHINE_KEY_FLAG (switch to USER scope);
  replace GetOrCreateKeyGuardKey with GetOrCreateManagedIdentityKey implementing
  3-level fallback: KeyGuard (VBS) > Hardware (Software KSP) > InMemory (rsa.GenerateKey).
  Add tryGetOrCreateCNGKey helper. isKeyGuardProtected re-validates after creation (once).
- cng_stub.go: update non-Windows stub to match new signature
- imdsv2.go: call GetOrCreateManagedIdentityKey; fail with clear error if keyType !=
  keyTypeKeyGuard (mirrors MSAL.NET mtls_pop_requires_keyguard check); make attestation
  conditional on keyType == keyTypeKeyGuard && attestationEndpoint != empty
- docs: add Credential Guard setup steps (registry + reboot), update error messages

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add VmssID to csrMetadataCuID struct (matches MSAL.NET CuidInfo)
- Rewrite generateCSR to build full PKCS#10 CSR manually via encoding/asn1:
  * Subject: CN={clientId}, DC={tenantId} (using DC OID 0.9.2342.19200300.100.1.25)
  * Custom PKCS#10 attribute OID 1.3.6.1.4.1.311.90.2.10 with JSON CuID value
    (SET { UTF8String(json) } in attributes [0] - NOT as x509 extension)
  * RSASSA-PSS / SHA-256 signature (OID 1.2.840.113549.1.1.10 with PSS params)
- Add helper tagExplicit for ASN.1 context-specific wrapping
- Update generateCSR call site to pass tenantID and cuID
- All unit tests pass

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
management.azure.com triggers AADSTS392196 (resource not enrolled for
mTLS PoP) on this tenant. graph.microsoft.com is enrolled and matches
the resource used in the successful msaljs e2e test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NCryptSignHash was always using PKCS1v15 (NCRYPT_PAD_PKCS1_FLAG=0x2)
regardless of opts. When generateCSR passes *rsa.PSSOptions, the CNG
key must use PSS padding (NCRYPT_PAD_PSS_FLAG=0x8) with
BCRYPT_PSS_PADDING_INFO{pszAlgId, cbSalt}.

Refactored Sign() into:
- signPSS()     -- uses NCRYPT_PAD_PSS_FLAG + BCRYPT_PSS_PADDING_INFO
- signPKCS1v15() -- original PKCS1v15 path (unchanged)

This was the root cause of 'unexpected claims' from IMDS: the CSR
AlgorithmIdentifier declared RSASSA-PSS but the actual signature was
PKCS1v15, causing signature verification failure on the IMDS side.

After this fix: IMDS accepts the CSR and issues the binding certificate.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ey discrimination

The token returned by the AAD mTLS endpoint has token_type='mtls_pop'.
Previously, the cache read used BearerAuthenticationScheme (type='Bearer'),
which never matched the stored token type. Fix by using
NewMtlsPopAuthenticationScheme(cert) for both Write and Read so the
AuthnScheme.AccessTokenType() and AuthnScheme.KeyID() match on both sides.

This makes the second AcquireToken call return from cache (TokenSource=1)
instead of making a new network request.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Architecture diagram: replace x509.CreateCertificateRequest() with
  accurate manual ASN.1 PKCS#10 description (RSA-PSS, CN+DC subject,
  CuID attribute OID 1.3.6.1.4.1.311.90.2.10 in attributes[0])
- Remove all result.TokenType references (AuthResult has no such field)
- Replace with result.BindingCertificate != nil checks (correct way to
  verify an mTLS PoP token was returned)
- Step 4 checklists: add JWT cnf.x5t#S256, xms_tbflags:2, appidacr:2
- Path 2: change resource to graph.microsoft.com (storage.azure.com not
  tested; management.azure.com returns AADSTS392196 in most tenants)
- Add AADSTS392196 to failure scenarios with resource guidance
- Fix certutil KSP name in step 4 (Software KSP, USER scope)
- Comparing Tokens: clarify token_type is NOT in JWT payload; appears
  only in HTTP response; confirmed by decoded live token
- Add CNG key persistence note: MSALMtlsKey_{cuID} in Software KSP

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread apps/tests/devapps/mtls-pop/path1_confidential/main.go Fixed
Comment thread apps/managedidentity/imdsv2.go Fixed
Comment thread apps/managedidentity/imdsv2.go Fixed
Robbie-Microsoft and others added 5 commits April 9, 2026 02:26
Use AZURE_TENANT_ID env var (or -tenant flag) for the error-case
authority in the Path 1 test driver. Errors trigger before any network
call so any tenanted-format value works. Falls back to all-zeros GUID
placeholder if neither is set.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
AcquireTokenByCredential had cognitive complexity 21 (SonarCloud limit 15).
Extract mTLS param setup into applyMtlsParams() and validateMtlsPopAuthority()
helper functions to bring complexity within limits while preserving identical
behavior.

Resolves SonarCloud annotation on PR #610.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace single-expression make() capacity with step-by-step overflow
checks using math.MaxInt before each addition, as suggested by CodeQL
'Size computation for allocation may overflow' finding on PR #610.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract decodeJWTClaims into shared internal/jwtutil package to eliminate
the identical copy in path1_confidential and path2_managedidentity.
Reduces code duplication flagged by SonarCloud (6.3% > 3% threshold).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cng_windows.go: extract ncryptSign() helper to share the two-step
NCryptSignHash pattern (size query + actual sign) between signPSS()
and signPKCS1v15(). Removes ~30 duplicate lines.

jwtutil: add PrintTokenInfo() sharing token preview, expiry, TokenSource,
and JWT claim printing across both test drivers. Removes ~15 duplicate
lines from path1 and path2 main.go.

All unit tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Robbie-Microsoft Robbie-Microsoft marked this pull request as ready for review April 9, 2026 03:10
Robbie-Microsoft and others added 3 commits April 9, 2026 15:36
…m calls

Add AuthResult.BindingTLSCertificate (*tls.Certificate) so managed identity
callers using WithMtlsProofOfPossession() can make downstream mTLS API calls.
Previously the CNG-backed private key was never exposed, making downstream
calls impossible for Path 2.

- base.AuthResult: add BindingTLSCertificate *tls.Certificate field
- imdsv2.go: populate BindingTLSCertificate on both cache-hit and network paths
- path2 test driver: add makeDownstreamCall() using BindingTLSCertificate
- docs: fix Step 6 to use result.BindingTLSCertificate (removed invalid result.PrivateKey reference)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- mtls-pop.md: replace vague downstream section with separate Path 1 / Path 2
  examples; Path 2 now uses result.BindingTLSCertificate; remove internal
  GetOrCreateKeyGuardKey reference
- mtls-pop.md: update Path 2 API comment to mention BindingTLSCertificate
- mtls-pop-manual-testing.md: add downstream 401 to Step 4 expected results,
  clarifying that 401 confirms TLS + token success when no Graph role is assigned

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Duplicate literals -> constants:
- confidential/mtlspop_test.go: const loginMicrosoftonline for 5 occurrences
- managedidentity/imdsv2.go: const errMsgCSRTooLarge for 3 occurrences
- path1_confidential/main.go: consts certFileName, keyFileName, graphScope, bodyFmt

Cognitive complexity reductions:
- oauth.go Credential(): extract credentialFromTokenProvider(), credentialFromMtls(),
  resolveMtlsEndpoint() — reduces from 23 to below 15
- oauth.go UsernamePassword(): extract usernamePasswordFederated() — reduces from 16 to below 15
- imdsv2.go acquireTokenForImdsV2(): extract buildMtlsBindingInfo(), maybeAttest(),
  fetchMtlsPopToken() — reduces from 40 to below 15
- managedidentity.go AcquireToken(): extract tryAcquireFromCache() — reduces from 17 to below 15
- path1_confidential/main.go testErrorCases(): extract tryClient() helper — reduces from 19 to below 15

SHA1 finding:
- accesstokens.go thumbprint(): add //NOSONAR comment explaining SHA1 is required by
  RFC 7517 §4.8 for the x5t header used with ADFS/DSTS

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread apps/internal/oauth/ops/accesstokens/accesstokens.go Dismissed
Robbie-Microsoft and others added 6 commits April 9, 2026 16:22
- confidential/mtlspop_test.go: const fakeTenantAuthority for 3 occurrences of
  'https://login.microsoftonline.com/fakeTenant'
- managedidentity/imdsv2.go: extract buildCuIDAttribute() from generateCSR() to reduce
  cognitive complexity from 16 to below 15

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
169.254.169.254 is the well-known Azure IMDS link-local address used by
all Azure SDKs (MSAL.NET, MSAL Python, MSAL JS). It is not a sensitive
hardcoded IP — it is a platform-defined constant. Added //NOSONAR and
explanatory comment to suppress the false-positive security hotspots.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
New docs/mtls-pop-architecture.md covering:
- Go-to-CNG interop via syscall.NewLazyDLL (no CGo) vs msal-dotnet P/Invoke
- cngSigner as crypto.Signer: PSS/PKCS1v15 dispatch, key never leaves CNG
- 3-level key fallback (KeyGuard/Hardware/InMemory) mirroring MSAL.NET
  WindowsManagedIdentityKeyProvider; VBS verification via 'Virtual Iso' property
- AttestationClientLib.dll: msal-dotnet bundles via NuGet; msal-go loads
  via syscall (known difference); how to obtain and deploy manually
- Manual PKCS#10 CSR via encoding/asn1: why x509.CreateCertificateRequest
  cannot be used; full structure including DN, SPKI, cuID attribute, PSS sig
- Binding cert cache: double-checked locking, 5-min expiry buffer
- SHA1 vs SHA256 thumbprint: ADFS/DSTS parity with msal-dotnet
  IsSha2CredentialSupported

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Removed standalone 'Regional mTLS Token Endpoints' section
- Moved endpoint table and region explanation into Path 1 section,
  framed as 'why a region is required' for the mTLS handshake
- Added note that Path 2 does not require a region from the caller
- Fixed AttestationClientLib.dll note: not a system DLL; link to arch doc

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two reasons: (1) proprietary binary — cannot redistribute in an
open-source Go module repo, (2) Go modules have no native asset
bundling mechanism equivalent to NuGet runtimes/ folder.
Also note it is low-friction in practice since Path 2 only runs
on controlled Trusted Launch Azure VM environments.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove incorrect licensing reason. The real reason is technical:
Go modules have no native asset bundling mechanism equivalent to
NuGet's runtimes/win-x64/native/ folder.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Robbie-Microsoft and others added 3 commits April 9, 2026 18:52
…ientLib.dll

- mtls-pop-architecture.md: expand section 4 to clearly explain WHY
  msal-go cannot bundle the DLL automatically — NuGet has a first-class
  native asset pipeline (runtimes/win-x64/native/) that MSBuild copies
  to output dirs; Go modules are source-only with no equivalent mechanism,
  and CGo doesn't help because the DLL is a runtime LoadLibrary dependency
  not a link-time dependency
- mtls-pop.md: inline the obtain-and-deploy steps in Path 2 requirements
  instead of just linking to the architecture doc
- mtls-pop-manual-testing.md: add dedicated Step 2 for obtaining and
  deploying AttestationClientLib.dll; renumber subsequent steps

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e comparison table

- mtls-pop.md: remove 'Comparison with Other MSAL Libraries' table
  and ASCII architecture diagram (both moved/converted)
- mtls-pop-architecture.md: add 'Flow Diagrams' section at top with
  Mermaid sequenceDiagrams for Path 1 (Confidential Client) and
  Path 2 (Managed Identity / IMDSv2)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename 'Why Go Uses the .NET Approach' -> 'Cross-SDK Implementation Comparison'
- Add msal-java row (JSSE/JNA + ncrypt.dll, in-process)
- Update trailing note to reflect all three in-process SDKs
- Consistent Approach column values (In-process vs .NET subprocess)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@sonarqubecloud
Copy link
Copy Markdown

}

// getPlatformMetadata calls IMDS /getplatformmetadata to get the CSR metadata.
func getPlatformMetadata(ctx context.Context, httpClient ops.HTTPClient) (csrMetadata, error) {
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.

Should we validate the IMDS server header here before trusting the body? MSAL .NET does this as part of the probe

}

// cStringToGoString reads a null-terminated C string from memory at the given address.
func cStringToGoString(ptr uintptr) string {
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.

Can we put a hard upper bound on this unmanaged string read?

)

// cngSigner implements crypto.Signer using a CNG key handle.
type cngSigner struct {
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.

What is the lifecycle for hKey after we return &cngSigner{hKey: hKey, ...}? I don’t see a Close() or finalizer path that frees the successful NCrypt handle, so this looks like a handle leak.

}

// ignore cached access tokens when given claims
if o.claims == "" {
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.

With WithMtlsProofOfPossession() we return early into acquireTokenForImdsV2() and drop o.claims. That means WithClaims() is silently ignored for the IMDSv2 path, and the cache-bypass behavior no longer matches the normal MI path

}
c.authParams.Scopes = []string{resource}

if o.isMtlsPopRequested {
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.

I don’t see the selected managed identity ID (UserAssignedClientID / UserAssignedObjectID / UserAssignedResourceID) flowing into the IMDSv2 path. Do we have test coverage for this?

AccessToken: tokenResponse.AccessToken,
ExpiresOn: tokenResponse.ExpiresOn,
GrantedScopes: tokenResponse.GrantedScopes.Slice,
Account: account,
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.

do we not expose token_type?


// --- RSASSA-PSS AlgorithmIdentifier (OID 1.2.840.113549.1.1.10 with SHA-256 params) ---
// SEQUENCE { OID sha-256 } for hash algo and MGF
sha256AlgID, _ := asn1.Marshal(asn1.RawValue{
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.

what will happen if any of these encodes fail?

return csrMetadata{}, fmt.Errorf("reading platform metadata response: %w", err)
}
if resp.StatusCode != http.StatusOK {
return csrMetadata{}, fmt.Errorf("platform metadata returned status %d: %s", resp.StatusCode, string(body))
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.

can this expose any sensitive information?

cacheAuthParams := c.authParams
cacheAuthParams.Scopes = []string{resource}
cacheAuthParams.AuthnScheme = mtlsScheme
if stResp, cacheErr := cacheManager.Read(ctx, cacheAuthParams); cacheErr == nil {
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.

do we check to see if the cert is still valid? before returning?

cacheAuthParams := c.authParams
cacheAuthParams.Scopes = []string{resource}
cacheAuthParams.AuthnScheme = mtlsScheme
if stResp, cacheErr := cacheManager.Read(ctx, cacheAuthParams); cacheErr == nil {
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.

Don't we have MSAL cache- which may already do this?

// a JWT client_assertion in the request body. The token type remains Bearer.
// Requires the client to be configured with a certificate credential (NewCredFromCert).
// Mirrors MSAL.NET's WithSendCertificateOverMtls().
func WithSendCertificateOverMtls() Option {
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.

can we have two PRs - one for SNI and one for MSI

Copy link
Copy Markdown
Collaborator

@4gust 4gust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not a cryptography expert, so I may not be the best person to comment on the security aspects in depth. That said, from my perspective, the rest of the implementation looks solid and well thought out.

I do have one concern regarding the DLL side of things.

Comment thread docs/mtls-pop.md
- IMDSv2 enabled (`cred-api-version=2.0`)
- Windows OS with VBS (Virtualization-Based Security) KeyGuard available
- **[Trusted Launch Azure VM](https://learn.microsoft.com/azure/virtual-machines/trusted-launch)** with Secure Boot + vTPM — `Is Capable For Attestation: True` (verify with `tpmtool.exe getdeviceinformation`)
- `AttestationClientLib.dll` present alongside the binary. This DLL is not bundled by msal-go (Go modules have no native asset distribution mechanism — see [architecture doc](mtls-pop-architecture.md#why-msal-go-cannot-bundle-the-dll-automatically) for the full explanation). To obtain it:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be quite tricky, as Go does not support packaging DLLs directly with the code as mentioned here.

Enforcing this constraint will also be challenging. I would appreciate a review from Charles on this approach.

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.

The dev experience is rough but I doubt we can do much better given the constraints.

Setting that aside, this feature needs threat modeling for DLL preloading attacks

Comment thread docs/mtls-pop.md
Comment on lines +93 to +94
1. Run: `dotnet add package Microsoft.Azure.Security.KeyGuardAttestation --version <latest>`
2. Copy from: `%USERPROFILE%\.nuget\packages\microsoft.azure.security.keyguardattestation\<version>\runtimes\win-x64\native\AttestationClientLib.dll`
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.

Could they curl and unzip instead? If not, this should link to dotnet install instructions

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.

5 participants