-
Notifications
You must be signed in to change notification settings - Fork 385
MSI POP Attestation #5612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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
src/client/Microsoft.Identity.Client.KeyAttestation/AttestationClient.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client.KeyAttestation/AttestationStatus.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client.KeyAttestation/ManagedIdentityAttestationExtensions.cs
Outdated
Show resolved
Hide resolved
| <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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the feature GA'd under this name - https://learn.microsoft.com/en-us/windows/security/identity-protection/credential-guard/
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
...ent/Microsoft.Identity.Client.KeyAttestation/Microsoft.Identity.Client.KeyAttestation.csproj
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client.KeyAttestation/PopKeyAttestor.cs
Outdated
Show resolved
Hide resolved
| <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.
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
src/client/Microsoft.Identity.Client.KeyAttestation/AttestationResult.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client.KeyAttestation/PublicAPI/net8.0/PublicAPI.Unshipped.txt
Show resolved
Hide resolved
| /// Static initializer that registers the KeyGuard attestation provider | ||
| /// when the KeyAttestation assembly is loaded. | ||
| /// </summary> | ||
| internal static class AttestationProviderInitializer |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)] |
There was a problem hiding this comment.
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.
Implements #5591