[Breaking change]: DefaultKeyResolution.ShouldGenerateNewKey has a slightly altered meaning

[amcasey]

Description

DefaultKeyResolution.ShouldGenerateNewKey no longer reflects whether the default key is close to its expiration time.

Version

.NET 9 Preview 3

Previous behavior

It was an undocumented, but consistent, feature of the API that ShouldGenerateNewKey would be true if the default key was within two days (an over-simplification) of its expiration time. The amount of lead time was based on the polling interval of ICacheableKeyRingProvider, which was not something IDefaultKeyResolver.ResolveDefaultKeyPolicy should have depended upon (since, for example, alternative implementations would probably not be aware of these details).

New behavior

If ShouldGenerateNewKey is true, it now indicates that either there is no default key or that for some other policy reason (i.e. in a specialized implementation of IDefaultKeyResolver, a new key should be generated. The ICacheableKeyRingProvider will make its own decision about whether the expiration time is close enough to warrant generating a new key.

Type of breaking change

• [ ] Binary incompatible: Existing binaries may encounter a breaking change in behavior, such as failure to load or execute, and if so, require recompilation.
• [ ] Source incompatible: When recompiled using the new SDK or component or to target the new runtime, existing source code may require source changes to compile successfully.
• [X] Behavioral change: Existing binaries may behave differently at run time.

Reason for change

First, we wanted to change the logic around key generation near expiration time and, second, this makes it more straightforward to implement a custom IDefaultKeyResolver.

Note that the documentation for this type already states that "This API supports infrastructure and is not intended to be used directly from your code. This API may change or be removed in future releases."

Recommended action

If you have an IDefaultKeyResolver implementation that tries to replicate the expiry logic, that logic can be removed (however, leaving it is fine as well).

If you were consuming IDefaultKeyResolver directly, for the express purpose of determining whether expiration was pending and that continues to be important, you can the default key's ExpirationDate property directly.

Affected APIs

Microsoft.AspNetCore.DataProtection.KeyManagement.Internal.DefaultKeyResolution.ShouldGenerateNewKey

[amcasey]

This is the change in question: https://github.com/dotnet/aspnetcore/pull/54264

[gewarren]

@amcasey We currently don't expose this API in the API reference docs. Is that expected?

https://github.com/dotnet/AspNetApiDocs/blob/0315ddc00e4a9fddaad89ef8d79e91f54a315488/aspnet-core/xml/_filter.xml#L295-L297

[amcasey]

@gewarren I believe that dates from a period when we believed that internal types should be part of the public API. I suspect that, in order to discourage people from using them, we opted not to include them in the docs (even though the types themselves have doc comments). So that matches my expectations, yes.

I'm not sure what that means for our attempt to document this breaking change. @halter73 or @captainsafia may remember another time when we made a breaking change to a pubternal type with no public documentation?

[captainsafia]

@halter73 or @captainsafia may remember another time when we made a breaking change to a pubternal type with no public documentation?

I don't recall any explicit scenario but it strikes me that we can document this is as a breaking behavioral change without referencing the API similar to what we recently did for anti forgery with minimal APIs (ref).