MSI POP Attestation #5612
MSI POP Attestation #5612
Conversation
src/client/Microsoft.Identity.Client.MtlsPop/ManagedIdentityPopExtensions.cs
Outdated
Show resolved
Hide resolved
...ent/Microsoft.Identity.Client.KeyAttestation/Microsoft.Identity.Client.KeyAttestation.csproj
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // CreateManagedIdentityAsync does a probe; Add one more CSR response for the actual acquire. | ||
| httpManager.AddMockHandler(MockHelpers.MockCsrResponse()); | ||
| // Add mocks for successful non-attested flow (CSR + issuecredential + token) |
There was a problem hiding this comment.
don't we need to update existing POP tests to use WithAttestationSupport()
There was a problem hiding this comment.
No. Tests already use .WithAttestationProviderForTests() which mocks what .WithAttestationSupport() does in production. No changes needed.
There was a problem hiding this comment.
I am concerned that if we ever remove the public API .WithAttestationSupport() our tests will never catch it
|
|
||
| Microsoft Visual Studio Solution File, Format Version 12.00 | ||
| # Visual Studio Version 18 | ||
| VisualStudioVersion = 18.0.11217.181 d18.0 |
There was a problem hiding this comment.
The solution file does not add the new project, only deletes the old one.
| { | ||
| /// <summary> | ||
| /// Managed façade for <c>AttestationClientLib.dll</c>. Holds initialization state, | ||
| /// does ref-count hygiene on <see cref="SafeNCryptKeyHandle"/>, and returns a JWT. | ||
| /// </summary> | ||
| internal sealed class AttestationClient : IDisposable |
There was a problem hiding this comment.
This should not be part of the public API
| /// High-level outcome categories returned by <see cref="AttestationClient.Attest"/>. | ||
| /// </summary> | ||
| internal enum AttestationStatus | ||
| public enum AttestationStatus |
There was a problem hiding this comment.
Should not be part of the public API
| } | ||
|
|
||
| // Register the attestation token provider | ||
| return builder.WithAttestationProviderForTests(async (req, ct) => |
There was a problem hiding this comment.
Why is builder method suffixed with "ForTests" ? It is production code.
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Identity.Client" /> | ||
| <PackageReference Include="Microsoft.Identity.Client.MtlsPop" /> |
There was a problem hiding this comment.
do we need to add the new project reference?
| <Version>$(MicrosoftIdentityClientVersion)-preview</Version> | ||
| <!-- Copyright needs to be in the form of © not (c) to be compliant --> | ||
| <Title>MSAL.NET extension for managed identity proof-of-possession flows</Title> | ||
| <Title>MSAL.NET extension for KeyGuard attestation support</Title> |
There was a problem hiding this comment.
the feature GA'd under this name - https://learn.microsoft.com/en-us/windows/security/identity-protection/credential-guard/
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <!-- Temporarily disable PublicAPI analyzer to generate correct signatures --> |
| /// Key discovery / rotation is the caller's responsibility. | ||
| /// </summary> | ||
| internal static class PopKeyAttestor | ||
| public static class PopKeyAttestor |
| <ItemGroup> | ||
| <ProjectReference Include="..\..\src\client\Microsoft.Identity.Client\Microsoft.Identity.Client.csproj" /> | ||
| <ProjectReference Include="..\..\src\client\Microsoft.Identity.Client.MtlsPop\Microsoft.Identity.Client.MtlsPop.csproj" /> | ||
| <ProjectReference Include="..\..\src\client\Microsoft.Identity.Client.KeyAttestation\Microsoft.Identity.Client.KeyAttestation.csproj" /> |
There was a problem hiding this comment.
we may also need to fix the OneBranch release pipeline and update the project name
|
|
||
| // CreateManagedIdentityAsync does a probe; Add one more CSR response for the actual acquire. | ||
| httpManager.AddMockHandler(MockHelpers.MockCsrResponse()); | ||
| // Add mocks for successful non-attested flow (CSR + issuecredential + token) |
There was a problem hiding this comment.
I am concerned that if we ever remove the public API .WithAttestationSupport() our tests will never catch it
| /// The netstandard2.0 target relies on the IsExternalInit shim (see IsExternalInit.cs) to enable 'init'. | ||
| /// </remarks> | ||
| internal sealed record AttestationResult( | ||
| public sealed record AttestationResult( |
There was a problem hiding this comment.
does this need to be public? think the safest way it to just rename the project and not recreate a new project. seems like there are many changes that are not necessary
| Microsoft.Identity.Client.KeyAttestation.AttestationStatus.NotInitialized = 3 -> Microsoft.Identity.Client.KeyAttestation.AttestationStatus | ||
| Microsoft.Identity.Client.KeyAttestation.AttestationStatus.Success = 0 -> Microsoft.Identity.Client.KeyAttestation.AttestationStatus | ||
| Microsoft.Identity.Client.KeyAttestation.AttestationStatus.TokenEmpty = 2 -> Microsoft.Identity.Client.KeyAttestation.AttestationStatus | ||
| Microsoft.Identity.Client.KeyAttestation.ManagedIdentityAttestationExtensions |
There was a problem hiding this comment.
The old publicAPI file should match the new public API file.
Implements #5591