Transmit EPK and use as public key during decrypt

[Greg Domzalski (GregDomzalski)]

RFC 7518 Section 4.6.1.1 states:

The "epk" (ephemeral public key) value created by the originator for
the use in key agreement algorithms. This key is represented as a
JSON Web Key [JWK] public key value. It MUST contain only public key
parameters and SHOULD contain only the minimum JWK parameters
necessary to represent the key; other JWK parameters included can be
checked for consistency and honored, or they can be ignored. This
Header Parameter MUST be present and MUST be understood and processed
by implementations when these algorithms are used.

EPK was referred to by comments in the code, but it seems that it was never used. Since the spec clearly states that the "Header Parameter MUST be present and MUST be understood and processed", I make the argument that this PR fixes a bug (#1951) and is not a feature enhancement.

This change does the following:

• Takes the EncryptingKey's public parameter, encodes it as a JWK public key, and adds it into the JWT header.
• Upon decryption, the discovered keys are used as the private decryption key. (This is typically TokenValidationParameters.TokenDecryptionKey)
• JWK is extracted from the header and converted into an EcdsaSecurityKey. It is then used as the public key for the key agreement.

This PR also contains an extra commit that I've covered in PR #2119 . I can remove that from this change list if we decide not to proceed with that other PR. This other PR / commit has been addressed in the 7.x release line and this PR has been rebased onto that.

[Greg Domzalski (GregDomzalski)]

@microsoft-github-policy-service agree company="Yubico"

[BrentSchmaltz (brentschmaltz)]

Greg Domzalski (@GregDomzalski) we might as well drop #2119 and just take this PR.

[Greg Domzalski (GregDomzalski)]

Yup. Fair enough. I'll close that PR.

[Greg Domzalski (GregDomzalski)]

I've updated this PR to be based on the new 7.x code.

I still need to do some regression testing on my side. But it would be really nice if we could get some traction on this PR. We're about to launch a product that takes a dependency on ECDH based key exchange / wrap algorithms working properly. I would really like to get these changes pushed upstream.

Please let me know what I can do to help make that a reality 😄

[BrentSchmaltz (brentschmaltz)]

Greg Domzalski (@GregDomzalski) this looks good, we will have to account for users who have used the work-around.
I am thinking AppContext switch.

[BrentSchmaltz (brentschmaltz)]

https://learn.microsoft.com/en-us/dotnet/api/system.appcontext?view=net-8.0

[Greg Domzalski (GregDomzalski)]

BrentSchmaltz (@brentschmaltz) Thanks for the feedback. That seems like a reasonable approach.

What would you like the default behavior to be?

Based on the documentation and assuming an opt-in to the new behavior, a possible switch identifier could be:
Switch.Microsoft.IdentityModel.UseRfcDefinitionOfEpkAndKid or perhaps
Switch.Microsoft.IdentityModel.SetEpkAndKidHeadersAccordingToRfc

I'm not quite sure how to describe the change in behavior, so I'm happy to take other name suggestions 😄

[Greg Domzalski (GregDomzalski)]

Alrighty. Let me know if that aligns with what you were thinking.

The diff definitely looks a lot cleaner - basically just adds at this point.

I introduced a centralized (public) class in Microsoft.IdentityModel.Tokens for defining constants for the switch values. There's only one const now - but I figured it would be a useful thing to have for consumers. Not to mention it provides a place to hang some documentation off from.

Here's the signature and accompanying documentation:

namespace Microsoft.IdentityModel.Tokens;

/// <summary>
/// Identifiers used for switching between different app compat behaviors within the Microsoft.IdentityModel libraries.
/// </summary>
/// <remarks>
/// The Microsoft.IdentityModel libraries use <see cref="System.AppContext" /> to turn on or off certain API behavioral
/// changes that might have an effect on application compatibility. This class defines the set of switches that are
/// available to modify library behavior. Application compatibility is favored as the default - so if your application
/// needs to rely on the new behavior, you will need to enable the switch manually. Setting a switch's value can be
/// done programmatically through the <see cref="System.AppContext.SetSwitch" /> method, or through other means such as
/// setting it through MSBuild, app configuration, or registry settings. These alternate methods are described in the
/// <see cref="System.AppContext.SetSwitch" /> documentation.
/// </remarks>
public static class AppCompatSwitches
{
    /// <summary>
    /// Uses <see cref="EncryptingCredentials.KeyExchangePublicKey"/> for the token's `kid` header parameter. When using
    /// ECDH-based key wrap algorithms the public key portion of <see cref="EncryptingCredentials.Key" /> is also written
    /// to the token's `epk` header parameter.
    /// </summary>
    /// <remarks>
    /// Enabling this switch improves the library's conformance to RFC 7518 with regards to how the header values for
    /// `kid` and `epk` are set in ECDH key wrap scenarios. The previous behavior erroneously used key ID of
    /// <see cref="EncryptingCredentials.Key"/> as the `kid` parameter, and did not automatically set `epk` as the spec
    /// defines. This switch enables the intended behavior where <see cref="EncryptingCredentials.KeyExchangePublicKey"/>
    /// is used for `kid` and the public portion of <see cref="EncryptingCredentials.Key"/> is used for `epk`.
    /// </remarks>
    public const string UseRfcDefinitionOfEpkAndKid = "Switch.Microsoft.IdentityModel.UseRfcDefinitionOfEpkAndKid";
}

Let me know if there's any additional API or doc review that needs to happen as a result of this addition.

I really appreciate your time looking into this and providing feedback. Thanks!

[Greg Domzalski (GregDomzalski)]

Hey BrentSchmaltz (@brentschmaltz) - just following up here.

[jennyf19]

BrentSchmaltz (@brentschmaltz) do we want to take this?

[BrentSchmaltz (brentschmaltz)]

jennyf19 we should take this for 8.

[BrentSchmaltz (brentschmaltz)]

Greg Domzalski (@GregDomzalski) there are some conflicts, are you able to fix them?
We would like to get this into our 8 release that will be picked up by asp.net 9.

[Greg Domzalski (GregDomzalski)]

Jamie A Hankins (@jamiehankins) Aaron Fortner (@AaFortner) - Can you guys take this over?

[Greg Domzalski (GregDomzalski)]

Hi BrentSchmaltz (@brentschmaltz) - I won't be able to do this personally as I'm no longer with Yubico, but I did hand this off prior to my departure. I'll work with my former colleagues to make sure we get this branch updated and merged in a timely manner. Thanks for the sign-off.

[Jamie A Hankins (jamiehankins)]

I don't and Aaron Fortner (@AaFortner) doesn't likely have write access to the fork. It takes less than five minutes to pull dev and rebase this branch on it. The only conflict is in a class header comment.

If Greg Domzalski (@GregDomzalski) still has write access to this, then maybe he can add us. Otherwise, we'd need to get our IT people involved.

[Greg Domzalski (GregDomzalski)]

Looks like I do still have access to this PR itself which means I can simply clone the fork and point the PR to that. I'll give that a try in the morning.

[Aaron Fortner (AaFortner)]

I sync'd up with Greg Domzalski (@GregDomzalski) offline, and it sounds like he hit some access issues after all. I'll follow up with our IT to see if we can get Jamie and/or myself added.

[BrentSchmaltz (brentschmaltz)]

Aaron Fortner (@AaFortner) Greg Domzalski (@GregDomzalski) the PR is small enough that i could cherry pick in the commits.
If it gets to be too much effort let me know.

[Jamie A Hankins (jamiehankins)]

BrentSchmaltz (@brentschmaltz) It wasn't big at all. Just had to get access handled.

It should be up to date now.

[BrentSchmaltz (brentschmaltz)]

Jamie A Hankins (@jamiehankins) Greg Domzalski (@GregDomzalski) thanks folks, merged.