Transmit EPK and use as public key during decrypt

[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.

[GregDomzalski]

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

[brentschmaltz]

@GregDomzalski we might as well drop #2119 and just take this PR.

[GregDomzalski]

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

[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]

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

[brentschmaltz]

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

[GregDomzalski]

@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 :smile:

[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!

[GregDomzalski]

Hey @brentschmaltz - just following up here.

[jennyf19]

@brentschmaltz do we want to take this?