Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Pop updated #2105

Merged
merged 20 commits into from
Nov 4, 2020
Merged

Pop updated #2105

merged 20 commits into from
Nov 4, 2020

Conversation

trwalke
Copy link
Member

@trwalke trwalke commented Oct 15, 2020

Updates for #2013

@trwalke
Refactoring POP api.
82bd149 
Exposing ICrypto provider to allow users to pass in their own
@trwalke
Refactoring api for pop.
5ba3ff6 
adding pop config object
refactoring iCryptoProvider
@trwalke
Adding elliptic curve popCryptoProvider implementation.
fe15d42 
Adding config object to pop api
refactoring
bgavrilMS
bgavrilMS reviewed Oct 15, 2020
View reviewed changes
src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs Outdated
/// <item>MSAL creates, reads and stores a key in memory that will be cycled every 8 hours.</item>
/// </list>
/// </remarks>
public T WithProofOfPosession(PopAuthenticationConfiguration popAuthenticationConfiguration)
Copy link
Member

Choose a reason for hiding this comment

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

IMO this should be the only POP method. Let's delete the old one.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a breaking change?
would we do that because it's an experimental feature?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, experimental features can introduce breaking changes at any time.

bgavrilMS
bgavrilMS reviewed Oct 16, 2020
View reviewed changes
src/client/Microsoft.Identity.Client/AppConfig/PopAuthenticationConfiguration.cs Outdated
/// <summary>
/// This will be populated with the POP authentication header.
/// </summary>
public AuthenticationHeaderValue PopAuthenticationRequestHeader {get; private set;}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this is. If you're not sure either, delete it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

We used to populate the user provided HTTP Request headers with the authentication headers calculated form the key. Im assuming this is no longer needed?

This value was already being used for tests.

Copy link
Member

@bgavrilMS bgavrilMS Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't really make that much sense since this is a config option, but developers cannot set this property.

The way consumers are supposed to retrieve the pop token is via AuthenticationResult. We can provide more covenient mechasism upon demand.

Copy link
Member

Choose a reason for hiding this comment

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

 var r2 = await confidentialApp.AcquireTokenForClient(s_keyvaultScope)
              .WithProofOfPosession(popConfig)
              .ExecuteAsync(CancellationToken.None)
              .ConfigureAwait(false);

image

bgavrilMS
bgavrilMS reviewed Oct 16, 2020
View reviewed changes
src/client/Microsoft.Identity.Client/AppConfig/PopAuthenticationConfiguration.cs
/// A provider that can handle the asymmetric key operations needed by POP, that encapsulates a pair of public and private keys
/// and some typical crypto operations
/// </summary>
public IPoPCryptoProvider PopCryptoProvider { get; set; }
Copy link
Member

@bgavrilMS bgavrilMS Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if we should actually put this crypto provider at the Application level instead of the Request level ? This way we can cache things like Thumbprint.

I suggest that for now we go with this and we discuss this in the API Review.

With the current implementation, we have to compute the thumbprint of the JWK at every request. If we moved the PoPAuthenticationScheme at app level, we'd compute it less often...

Copy link
Contributor

Choose a reason for hiding this comment

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

@bgavrilMS: Could there be that there is a need for a PopAuthenticationScheme per resource? or per endpoint?

bgavrilMS
bgavrilMS requested changes Oct 16, 2020
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

Generally looks good, see inline comments.

jennyf19
jennyf19 reviewed Oct 17, 2020
View reviewed changes
src/client/Microsoft.Identity.Client/AuthScheme/PoP/PoPAuthenticationScheme.cs
@@ -19,9 +20,7 @@ namespace Microsoft.Identity.Client.AuthScheme.PoP
internal class PoPAuthenticationScheme : IAuthenticationScheme
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: some places it' has PoP and some have Pop. maybe go with Pop?

jmprieur
jmprieur reviewed Oct 17, 2020
View reviewed changes
src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs Outdated Show resolved Hide resolved
src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs Outdated
/// <item>MSAL creates, reads and stores a key in memory that will be cycled every 8 hours.</item>
/// </list>
/// </remarks>
public T WithProofOfPosession(PopAuthenticationConfiguration popAuthenticationConfiguration)
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a breaking change?
would we do that because it's an experimental feature?

src/client/Microsoft.Identity.Client/MsalError.cs Outdated
@@ -822,6 +822,11 @@ public static class MsalError
/// </summary>
public const string CryptoNet45 = "crypto_net45";

/// <summary>
/// Proof of posession authentication attempt is missing a an endpoint to bing to.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Proof of posession authentication attempt is missing a an endpoint to bing to.
/// Proof of posession authentication attempt is missing an endpoint to bind to.

trwalke
trwalke commented Oct 19, 2020
View reviewed changes
src/client/Microsoft.Identity.Client/Internal/JsonWebTokenConstants.cs Outdated
@@ -11,6 +11,7 @@ internal class JsonWebTokenConstants
internal class Algorithms
{
public const string RsaSha256 = "RS256";
public const string EdcSha256 = "ES256";
Copy link
Member Author

@trwalke trwalke Oct 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is what I was referring to. Elliptic curve can be ES256/ES384/ES512 etc. This value will need to be updated based on the conical JWK created by the elliptic curve keys.

@bgavrilMS

Copy link
Contributor

Choose a reason for hiding this comment

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

How does this relate to the list of algorithms in https://docs.microsoft.com/en-us/azure/key-vault/keys/about-keys#signverify and https://docs.microsoft.com/en-us/azure/key-vault/keys/about-keys#signverify1 ?

trwalke
trwalke commented Oct 19, 2020
View reviewed changes
src/client/Microsoft.Identity.Client/AuthScheme/PoP/PoPAuthenticationScheme.cs Outdated
//Need to update header to signal EC key is being used
if (JWK[JsonWebKeyParameterNames.Kty].ToString() == JsonWebKeyParameterNames.EC)
{
return JsonWebTokenConstants.Algorithms.EdcSha256;
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to add support for other elliptic curve key sizes here. currently only 256 is supported.
@bgavrilMS

Copy link
Member Author

Choose a reason for hiding this comment

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

supported values
https://docs.microsoft.com/en-us/azure/key-vault/keys/about-keys#signverify

jmprieur
jmprieur reviewed Oct 20, 2020
View reviewed changes
Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

I've left a few comments and suggestions

src/client/Microsoft.Identity.Client/AppConfig/PopAuthenticationConfiguration.cs Outdated Show resolved Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/PopAuthenticationConfiguration.cs
/// A provider that can handle the asymmetric key operations needed by POP, that encapsulates a pair of public and private keys
/// and some typical crypto operations
/// </summary>
public IPoPCryptoProvider PopCryptoProvider { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

@bgavrilMS: Could there be that there is a need for a PopAuthenticationScheme per resource? or per endpoint?

src/client/Microsoft.Identity.Client/AppConfig/PopAuthenticationConfiguration.cs Outdated Show resolved Hide resolved
src/client/Microsoft.Identity.Client/AuthScheme/PoP/IPoPCryptoProvider.cs Show resolved Hide resolved
src/client/Microsoft.Identity.Client/AuthScheme/PoP/IPoPCryptoProvider.cs
/// See https://docs.microsoft.com/en-us/azure/key-vault/keys/about-keys#signverify for ECD
/// See https://docs.microsoft.com/en-us/azure/key-vault/keys/about-keys#signverify-1 for RSA
/// </summary>
string CryptographicAlgorithm { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Are symmetric key algorithm supported? if not, do we want to mention it?

src/client/Microsoft.Identity.Client/Internal/JsonWebTokenConstants.cs Outdated
@@ -11,6 +11,7 @@ internal class JsonWebTokenConstants
internal class Algorithms
{
public const string RsaSha256 = "RS256";
public const string EdcSha256 = "ES256";
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this relate to the list of algorithms in https://docs.microsoft.com/en-us/azure/key-vault/keys/about-keys#signverify and https://docs.microsoft.com/en-us/azure/key-vault/keys/about-keys#signverify1 ?

tests/Microsoft.Identity.Test.Integration.Win8/App.config Show resolved Hide resolved
bgavrilMS
bgavrilMS requested changes Oct 20, 2020
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

Please remove popConfig.PopAuthenticationRequestHeader and rely on AuthenticationResult instead. Otherwise LGTM!

trwalke and others added 12 commits October 22, 2020 08:17
@trwalke
Updates
c5a6250
@trwalke
Merge remote-tracking branch 'origin/master' into trwalke/PopPart2
73ea027 
# Conflicts:
#	tests/Microsoft.Identity.Test.Common/Core/Helpers/ECDCertificatePopCryptoProvider.cs
#	tests/Microsoft.Identity.Test.Common/Core/Helpers/RSACertificatePopCryptoProvider.cs
#	tests/Microsoft.Identity.Test.Integration/Microsoft.Identity.Test.Integration.csproj
#	tests/Microsoft.Identity.Test.Unit/ExceptionTests/ExperimentalFeatureTests.cs
Fixing tests
7e94f03 
Refactoring
Fix error
0d81635
removing PackageTargetFallback
21123fd
Test fix
b5c2cb8
Refactoring to remove from public client
2bca1e3
test fix
5183173
@trwalke
refactoring
5f14727
@trwalke
refactoring
08e648e
@trwalke @jmprieur
Update src/client/Microsoft.Identity.Client/AppConfig/PopAuthenticati…
881480e 
…onConfiguration.cs

Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>
@trwalke
Merge remote-tracking branch 'origin/master' into trwalke/PopPart2
5894d80
@trwalke trwalke merged commit c085471 into master Nov 4, 2020
@trwalke trwalke deleted the trwalke/PopPart2 branch June 22, 2021 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennyf19 jennyf19 jennyf19 left review comments

@jmprieur jmprieur jmprieur left review comments

@bgavrilMS bgavrilMS bgavrilMS requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@trwalke @bgavrilMS @jmprieur @jennyf19