Fixed token validation bug when using a custom IDataProtectionProvider.

[sbolofsson]

• When using a custom IDataProtectionProvider with OpenIddict, the access token validation fails.
  • Tested only with password grant flow but it seems like all other flows are also impacted by this bug.
• Looking at the source code of OpenIddict it seems to fail because other data protection providers are not guaranteed to generate the same magic header prefix (CfDJ8) in access tokens as the built-in/native Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingBasedDataProtectionProvider
• I have expanded the magic header guard with a check that validates if the current IDataProtectionProvider is the built-in.
  • NB: Proper type checking is not possible because the KeyRingBasedDataProtectionProvider is marked as internal.

[kevinchalet]

Thanks for your PR.

Tested only with password grant flow but it seems like all other flows are also impacted by this bug.

It's not really a bug but a deliberate design choice: while it's defined by KeyRingBasedDataProtector, the 0x09F0C9F0/CfDJ8 magic header is actually mentioned in multiple places in the Data Protection docs as the special value identifying DP payloads (i.e not just payloads produced by the default KeyRingBasedDataProtector implementation):

• https://learn.microsoft.com/en-us/aspnet/core/security/data-protection/implementation/authenticated-encryption-details?view=aspnetcore-7.0#protected-payload-format
• https://learn.microsoft.com/en-us/aspnet/core/security/data-protection/implementation/subkeyderivation?view=aspnetcore-7.0#additional-authenticated-data-and-subkey-derivation
• https://learn.microsoft.com/en-us/aspnet/core/security/data-protection/compatibility/replacing-machinekey?view=aspnetcore-7.0

As such, I'm not sure non-conforming IDataProtector is something I'd want to support. What's your scenario for creating a non-conforming implementation?

[sbolofsson]

Use case:

• We're upgrading an ASP.NET Web API (.NET Framework 4.8) that uses Owin to generate bearer tokens to ASP.NET Core.
• The tokens are generated using the (at the time of implementation) preferred method in .NET Framework, i.e. machine keys.
• Tokens currently generated by Owin does not start with CfDJ8, since they are based on the machine key in web.config, and not using the new DP system.
• Our architecture has a desktop client application deployed to lots of corporate machines that we don't have control over.
• The client application is communicating with the Web API. It caches the auth token on disk and re-uses it for API communication in the entire validity period of the token.
• We have to ensure that the Owin auth tokens already out there are still being validated server side when we upgrade to ASP.NET Core. Thus, we have to support the same auth token validation mechanism as Owin did.
• We're using this library which can integrate the old machine key way of doing things with the ASP.NET Core DataProtection API: https://github.com/daixinkai/AspNetCore.Owin/blob/master/src/DataProtection/README.md
  • It simply wraps the old .NET Framework code into the DP interfaces.

So it's not a non-conformant Data Protection system that I'm hinting at. It's simply a way of migrating away from legacy Owin to your library. However, I have worked around the issue in a less elegant way by hooking into the event handlers pipeline before OpenIddict runs these (for our set up) "problematic" handlers. Also ensuring that newly generated tokens are based on the new DP system.

But just a question back. I know the prefix is in the design spec. But the current string prefix validation check could be entirely removed and an invalid auth token would still be discarded in the same method. So what's the purpose of the check?

[sbolofsson]

Just to make it perfectly clear. We have a workaround. But I just spend almost an entire day to find out that it's a simple string check that "prevents" the UseDataProtection methods in OpenIddict to work the way I thought 😃 I don't see why it should limit any custom IDataProtectionProvider implementations that people would want to register. That doesn't seems to be the promise/premise of the methods - so I just wanted to give back to your awesome library through this PR and to save others migrating away from Owin/machine keys any similar headache 😊

And the extra guard that I suggested may not be perfect. But I don't think that the current string prefix check is perfect either. You can only assume that if the prefix is there, then yes it was generated by ASP.NET Core. But if it's not there, then you cannot make any assumptions as to what DP system generated the token. It may still be perfectly valid.

[kevinchalet]

So it's not a non-conformant Data Protection system that I'm hinting at. It's simply a way of migrating away from legacy Owin to your library.

The Data Protection implementation is not the only thing that differs between OWIN/Katana and ASP.NET Core: the authentication ticket serializer/data format was also updated in ASP.NET Core 1.0 to use ClaimsPrincipal (with potentially multiple identities attached) and support (de)serializing Claim.Properties (which, amusingly, was added for ASOS/OpenIddict: aspnet/Security#465).

As such, replacing the IDataProtector implementation is not enough, you also need to use the same serialization format. Not to mention that OpenIddict >= 3.0 uses private claims instead of AuthenticationProperties entries to represent things like creation/expiration dates or audiences, which requires some mapping (done in OpenIddict*DataProtectionFormatter for the default implementation).

For this scenario, my recommendation so far has always been to use AspNetTicketBridge and create a custom SecureDataFormat<AuthenticationTicket> with the correct DP purposes (e.g Microsoft.Owin.Security.OAuth + Access_Token + v1 for access tokens) and an instance of OwinTicketSerializer. Once in place, all you need is a custom IOpenIddict*Handler<ValidateTokenContext> handler that calls it, sets context.Principal and does the necessary mapping if the ticket is valid.

But the current string prefix validation check could be entirely removed and an invalid auth token would still be discarded in the same method. So what's the purpose of the check?

It's a perf' optimization, to avoid calling DP when we know the payload is not a DP-protected token (i.e misses the magic header). It's worth mentioning that while DP is pretty optimized, there are cases where we need to call it multiple times (e.g for introspection or revocation requests, you need to call DP once per token type with different "purposes"), which has a certain cost.

We don't have the same issue with IdentityModel, as it provides a CanReadToken(...) API we leverage to determine whether the token has a format it's likely to accept later when actually validating the token.

I don't see why it should limit any custom IDataProtectionProvider implementations that people would want to register.

Well, you could ask the same question for JWT tokens: sure you could create a JsonWebTokenHandler that produces tokens that don't start with "ey...", but they wouldn't be standard-compliant... is it really a scenario you'd want to support? 😅

And the extra guard that I suggested may not be perfect. But I don't think that the current string prefix check is perfect either. You can only assume that if the prefix is there, then yes it was generated by ASP.NET Core. But if it's not there, then you cannot make any assumptions as to what DP system generated the token. It may still be perfectly valid.

Your approach is not unreasonable, but as I said, it's not enough to magically accept tokens produced by the Katana OAuth 2.0 authorization server middleware: you'd also need a custom IOpenIddict*DataProtectionFormatter implementation that uses the same format as Katana, which is not exactly trivial to write yourself. So it would certainly simplify things a tiny bit, but not enough to make using old tokens "easy".

I'll think more about it before giving you a definitive answer 😃

That doesn't seems to be the promise/premise of the methods - so I just wanted to give back to your awesome library through this PR and to save others migrating away from Owin/machine keys any similar headache 😊

Glad you like the library! If it was useful to your company, please consider encouraging your employer to sponsor the project on GitHub: https://github.com/sponsors/kevinchalet ❤️

[sbolofsson]

I stand corrected. You're right. I'm wrong. I think this is an x-y problem then. It's more about easing the upgrade path from Owin to OpenIddict. I.e. being able to validate old tokens in the old machine key format, while issuing (and validating) new tokens using the new format. You can close this PR then.

[kevinchalet]

It's more about easing the upgrade path from Owin to OpenIddict.

I'm sold, PR merged. Congrats for your first contribution to OpenIddict 👍🏻