[MSI V2] - Adds Authentication Operation to MSI v2 mTLS flows #5533
Conversation
| if (AuthenticationRequestParameters.IsMtlsPopRequested && | ||
| !(AuthenticationRequestParameters.AuthenticationScheme is MtlsPopAuthenticationOperation)) | ||
| { | ||
| var priorCert = _managedIdentityClient.RuntimeMtlsBindingCertificate; |
There was a problem hiding this comment.
What if _managedIdentityClient.RuntimeMtlsBindingCertificate is null? Should this be inside the if statement?
There was a problem hiding this comment.
Guarded with priorCert != null
|
|
||
| // Prime the scheme before any cache lookup if we already have a binding cert from a prior mint | ||
| if (AuthenticationRequestParameters.IsMtlsPopRequested && | ||
| !(AuthenticationRequestParameters.AuthenticationScheme is MtlsPopAuthenticationOperation)) |
There was a problem hiding this comment.
Can you please explain to me why this 2nd check is needed? If AuthenticationRequestParameters.IsMtlsPopRequested is true, then why do we care if the authentication scheme is not MtlsPopAuthenticationOperation?
There was a problem hiding this comment.
Simplified, you are correct it is not needed
|
|
||
| ManagedIdentityRequest request = await CreateRequestAsync(resource).ConfigureAwait(false); | ||
|
|
||
| if (parameters.IsMtlsPopRequested && request?.MtlsCertificate != null) |
There was a problem hiding this comment.
Can you leave a comment here explaining when the MtlsCertificate would already be in the request? Would it be there when there was already a mTLS PoP request?
There was a problem hiding this comment.
Added a brief comment in AbstractManagedIdentity.AuthenticateAsync explaining that IMDSv2 mints the binding cert during CSR/attestation and exposes it on the request
| RuntimeMtlsBindingCertificate = cert; | ||
| //dispose prior stored cert on replacement | ||
| old?.Dispose(); |
There was a problem hiding this comment.
nit: I would swap these lines
There was a problem hiding this comment.
How does Dispose() work? Is that part of .NET? Does it communicate to the cert store on the OS?
There was a problem hiding this comment.
Added an XML remark clarifying that Dispose() releases the object’s resources and does not remove a cert from the OS store (that’s X509Store.Remove)
Fixes -
Changes proposed in this request
• Purpose: Persist and reuse mTLS binding certificate to enable cache lookups under mtls_pop scheme.
• Non-goals: Certificate rotation strategy, multi-cert pool, persistence across processes.
• Follow-up items: (a) Coalescing concurrent first-mint, (b) Attaching binding cert to result (if not in this PR), (c) Tests enabling previously skipped assertions.
• Risk assessment: Low; internal-only; no behavior change for bearer path.
Testing
unit tests
Performance impact
none
Documentation