Skip to content

Conversation

@Robbie-Microsoft
Copy link
Contributor

Implements #5591

@Robbie-Microsoft Robbie-Microsoft marked this pull request as ready for review December 8, 2025 18:22
@Robbie-Microsoft Robbie-Microsoft requested a review from a team as a code owner December 8, 2025 18:22
<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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What action do you want me to take here? CC @bgavrilMS

Copy link
Member

Choose a reason for hiding this comment

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

All comments should refer to this as Credential Guard.

<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" />
Copy link
Contributor

Choose a reason for hiding this comment

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

we may also need to fix the OneBranch release pipeline and update the project name

/// Static initializer that registers the KeyGuard attestation provider
/// when the KeyAttestation assembly is loaded.
/// </summary>
internal static class AttestationProviderInitializer
Copy link
Member

Choose a reason for hiding this comment

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

1 file per class pls

/// Implementation of IAttestationProvider for KeyGuard attestation.
/// This provider is automatically registered when the KeyAttestation package is loaded.
/// </summary>
internal class KeyGuardAttestationProvider : IAttestationProvider
Copy link
Member

@bgavrilMS bgavrilMS Dec 17, 2025

Choose a reason for hiding this comment

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

I think this PR is doing too much. The existing logic just sets builder.CommonParameters.AttestationTokenProvider. Why does this need to change with introduction of registry, new interfaces and classes etc. I do not understand the new design.

[assembly: InternalsVisibleTo("Microsoft.Identity.Client.Desktop.WinUI3" + KeyTokens.MSAL)]
[assembly: InternalsVisibleTo("Microsoft.Identity.Client.Broker" + KeyTokens.MSAL)]
[assembly: InternalsVisibleTo("Microsoft.Identity.Client.MtlsPop" + KeyTokens.MSAL)]
[assembly: InternalsVisibleTo("Microsoft.Identity.Client.KeyAttestation" + KeyTokens.MSAL)]
Copy link
Member

Choose a reason for hiding this comment

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

I think the AI was actually trying to eliminate this InternalsVisibleTo and came up with this new design. But you kept it.

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.

3 participants