-
Notifications
You must be signed in to change notification settings - Fork 219
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
MsAuth10AtPop part2 #2109
MsAuth10AtPop part2 #2109
Conversation
return Task.CompletedTask; | ||
}); | ||
builder.WithProofOfPosessionKeyId(tokenAcquisitionOptions.PopPublicKey, "pop"); | ||
builder.OnBeforeTokenRequest((data) => |
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 guess this is "new POP" as defined by ESTS (as opposed to MsAuth10ATPOP)? I would add some comments here to make it clear.
Not critical: For "new POP" I would recommend that you use MSAL's higher level API WithProofOfPossession(config)
. Since you won't be letting MSAL generate the SHR, you can do this:
class CryptoProvider : IPoPCryptoProvider
{
string CannonicalPublicKeyJwk { get; } => JwkClaim;
string CryptographicAlgorithm { get; } => "RS256"; // RSA only
public byte[] Sign(byte[] data)
{
throw new NotImplementedException(); // Wilson will generate the SHR
}
}
PoPAuthenticationConfiguration config = new PoPAuthenticationConfiguration();
config.CryptoProvider = new CryptoProvider();
builder.WithProofOfPossesion(config);
Note that this implies that MSAL computes the key id based on the JWK, i.e. kid=hash(JWK)
. If you want to support JWK and kid provided by user, I think the code that you wrote is the way to go.
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.
Also note that this is RSA only. If you need to support other algorithms such as ECD, we need to run a small test to see if another param needs to be exposed or not. In MSAL we decided to expose CryptographicAlgorithm
but I don't know if this is needed when Wilson creates the SHR.
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.
Also happy to hear how we can re-design the PoPAuthenticationConfiguration
interface. The original design did not take into account Wilson creating the SHR. It works, but it's clunky...
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.
@jmprieur said you synced off-line and are okay with this proposal, is that right?
jwkClaim, | ||
clientId); | ||
|
||
data.BodyParameters.Remove("client_assertion"); |
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.
@bgavrilMS
Would it be possible, when we use builder.WithProofOfPosessionKeyId(popPublicKey) that MSAL does not compute the signed assertion? (as it's not necessary)
I can create a GitHub issue in the MSAL.NET repo if you agree.
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.
LGTM
Thanks @jennyf19
successfully tested
No description provided.