Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,6 @@ public T WithCorrelationId(Guid correlationId)
/// <param name="httpRequestMessage">An HTTP request to the protected resource which requires a PoP token. The PoP token will be cryptographically bound to the request.</param>
/// <remarks>
/// <list type="bullet">
/// <item>This is an experimental API. The method signature may change in the future without involving a major version upgrade.</item>
/// <item> An Authentication header is automatically added to the request</item>
/// <item> The PoP token is bound to the HTTP request, more specifically to the HTTP method (GET, POST, etc.) and to the Uri (path and query, but not query parameters). </item>
/// <item>MSAL creates, reads and stores a key in memory that will be cycled every 8 hours.</item>
Expand All @@ -383,16 +382,23 @@ public T WithProofOfPosession(HttpRequestMessage httpRequestMessage)
return WithProofOfPosession(httpRequestMessage, defaultCryptoProvider);
}

// Allows testing the PoP flow with any crypto. Consider making this public.
/// <summary>
/// Modifies the token acquisition request so that the acquired token is a Proof of Possession token (PoP), rather than a Bearer token.
/// PoP tokens are similar to Bearer tokens, but are bound to the HTTP request and to a cryptographic key, which MSAL can manage on Windows.
/// See https://aka.ms/msal-net-pop
/// </summary>
/// <param name="httpRequestMessage">An HTTP request to the protected resource which requires a PoP token. The PoP token will be cryptographically bound to the request.</param>
/// <param name="popCryptoProvider">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.</param>
/// <remarks>
/// <list type="bullet">
/// <item> An Authentication header is automatically added to the request</item>
/// <item> The PoP token is bound to the HTTP request, more specifically to the HTTP method (GET, POST, etc.) and to the Uri (path and query, but not query parameters). </item>
/// <item>MSAL creates, reads and stores a key in memory that will be cycled every 8 hours.</item>
/// </list>
/// </remarks>
internal T WithProofOfPosession(HttpRequestMessage httpRequestMessage, IPoPCryptoProvider popCryptoProvider)
{
if (!ServiceBundle.Config.ExperimentalFeaturesEnabled)
{
throw new MsalClientException(
MsalError.ExperimentalFeature,
MsalErrorMessage.ExperimentalFeature(nameof(WithProofOfPosession)));
}

if (httpRequestMessage is null)
{
throw new ArgumentNullException(nameof(httpRequestMessage));
Expand All @@ -406,7 +412,67 @@ internal T WithProofOfPosession(HttpRequestMessage httpRequestMessage, IPoPCrypt
CommonParameters.AddApiTelemetryFeature(ApiTelemetryFeature.WithPoPScheme);
CommonParameters.AuthenticationScheme = new PoPAuthenticationScheme(httpRequestMessage, popCryptoProvider);

return this as T;
return (T)this;
}

/// <summary>
/// Modifies the token acquisition request so that the acquired token is a Proof of Possession token (PoP), rather than a Bearer token.
/// PoP tokens are similar to Bearer tokens, but are bound to the HTTP request and to a cryptographic key, which MSAL can manage on Windows.
/// See https://aka.ms/msal-net-pop
/// </summary>
/// <remarks>
/// <list type="bullet">
/// <item> An Authentication header is automatically added to the request</item>
/// <item> The PoP token is bound to the HTTP request, more specifically to the HTTP method (GET, POST, etc.) and to the Uri (path and query, but not query parameters). </item>
/// <item>MSAL creates, reads and stores a key in memory that will be cycled every 8 hours.</item>
/// </list>
/// </remarks>
public T WithProofOfPosession()
{
CommonParameters.UsingProofOfPossesion = true;
return (T)this;
}

/// <summary>
/// Specifies the HTTP method of the HTTP request to the protected resource which requires a PoP token.
/// The PoP token will be cryptographically bound to the request.
/// </summary>
/// <param name="httpMethod">Http Method for proof of possesion request.</param>
/// <returns></returns>
public T WithProofOfPosessionMethod(HttpMethod httpMethod)
{
CommonParameters.PopMethod = httpMethod;
return (T)this;
}

/// <summary>
/// Specifies the URL of the HTTP request to the protected resource which requires a PoP token.
/// The PoP token will be cryptographically bound to the request.
/// </summary>
/// <param name="uri">Protected resource URL.</param>
/// <returns></returns>
public T WithProofOfPosessionUri(Uri uri)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't like this strategy of piling on options on the AcquireToken* method because it "polutes" . There will be at least 4 params on SHR -> 4 methods on each AcqurieToken* entry point.

Instead, you can add a simple config object, similar to

public AcquireTokenInteractiveParameterBuilder WithSystemWebViewOptions(SystemWebViewOptions options)

So we could have a single method:

WithPoP(PoPOptions options = null)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The main question here is - what should the default be here? I would argue for a secure default, where POP takes into account the method (GET / POST) and the url ...

{
CommonParameters.PopUri = uri;
return (T)this;
}

/// <summary>
/// Specifies 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.
/// All symetric operations are SHA256
/// </summary>
/// <param name="popCryptoProvider"> Proof of posession cryptography provider</param>
/// <returns></returns>
public T WithPopCryptoProvider(IPoPCryptoProvider popCryptoProvider)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you are trying to tackle 2 stories at the same time, which makes it difficult to implement and review:

  1. Expose the crypto details to the user
  2. Make the method and uri optional

I don't think you should tackle #2 right now, nobody asked for it and it will complicate things.

@bgavrilMS bgavrilMS Oct 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think IPoPCryptoProvider is too low level and a higher level abstraction should be exposed to the user.

To help figure out the best solution, I would recommend you try to create 2 implementations that will help our users:

  1. An implementation of this interface that uses an X509Certificate2
  2. An implementation of this interfaces that uses ECD encyrption instead of RSA

Also, I believe that this public API should be discussed with Brian and George as well.

{
if (popCryptoProvider is null)
{
throw new ArgumentNullException(nameof(popCryptoProvider));
}

CommonParameters.PopCryptoProvider = popCryptoProvider;
return (T)this;
}
#endif

Expand All @@ -423,6 +489,25 @@ internal void ValidateAndCalculateApiId()
CommonParameters.ApiId = CalculateApiEventId();
CommonParameters.ApiTelemId = ApiTelemetryId;
CommonParameters.CorrelationId = CommonParameters.UseCorrelationIdFromUser ? CommonParameters.UserProvidedCorrelationId : Guid.NewGuid();

#if DESKTOP || NET_CORE
if (CommonParameters.UsingProofOfPossesion)
{
if (CommonParameters.PopUri == null )
{
throw new MsalClientException(MsalError.PopUriCannotBeNull, "Proof of possesion endpoint is null.");

@bgavrilMS bgavrilMS Oct 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Uri is not mandatory as far as I understand. Please check with Brian / George.
So the user has the ability to say "don't bind the uri to the pop token"

}

HttpRequestMessage message = new HttpRequestMessage(CommonParameters.PopMethod != null ? CommonParameters.PopMethod : HttpMethod.Get,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, this isn't how it's supposed to work. The user has the ability to say "don't bind the method to the pop token".

CommonParameters.PopUri);

IPoPCryptoProvider defaultCryptoProvider = CommonParameters.PopCryptoProvider != null ?
CommonParameters.PopCryptoProvider : ServiceBundle.PlatformProxy.GetDefaultPoPCryptoProvider();

CommonParameters.AddApiTelemetryFeature(ApiTelemetryFeature.WithPoPScheme);
CommonParameters.AuthenticationScheme = new PoPAuthenticationScheme(message, defaultCryptoProvider);
}
#endif
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@

using System;
using System.Collections.Generic;
using System.Net.Http;
using Microsoft.Identity.Client.AuthScheme;
using Microsoft.Identity.Client.AuthScheme.Bearer;
using Microsoft.Identity.Client.AuthScheme.PoP;
using Microsoft.Identity.Client.TelemetryCore;
using Microsoft.Identity.Client.TelemetryCore.Internal;
using Microsoft.Identity.Client.TelemetryCore.Internal.Events;
Expand All @@ -23,6 +25,10 @@ internal class AcquireTokenCommonParameters
public string Claims { get; set; }
public AuthorityInfo AuthorityOverride { get; set; }
public ApiTelemetryId ApiTelemId { get; set; } = ApiTelemetryId.Unknown;
public bool UsingProofOfPossesion { get; set; }
public HttpMethod PopMethod { get; set; }
public Uri PopUri { get; set; }
public IPoPCryptoProvider PopCryptoProvider { get; set; }

public IAuthenticationScheme AuthenticationScheme { get; set; } = new BearerAuthenticationScheme();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ namespace Microsoft.Identity.Client.AuthScheme.PoP
/// Ideally there should be a single public / private key pair associated with a machine, so implementers of this interface
/// should consider exposing a singleton.
/// </remarks>
internal interface IPoPCryptoProvider
public interface IPoPCryptoProvider
{
/// <summary>
/// The cannonical representation of the JWK. See https://tools.ietf.org/html/rfc7638#section-3
Expand Down
5 changes: 5 additions & 0 deletions src/client/Microsoft.Identity.Client/MsalError.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
/// </summary>
public const string PopUriCannotBeNull = "Pop_Uri_Cannot_Be_Null";

#if iOS
/// <summary>
/// Xamarin.iOS specific. This error indicates that keychain access has not be enabled for the application.
Expand Down