Skip to content

Prefer keys old enough to have propagated #54309

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

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Mar 1, 2024

The code for choosing a fallback key when no default was found sensibly distinguished between keys that were old enough to have propagated to all instances and those that were not. The code for choosing the preferred key should do the same.

This should help in the case where two instances both discover that a new key is needed and create an immediately-activated key. One of them will get both keys back from the repository and it should choose the older of the two to match the choice of of the other instance (which only knows about its own key).

Part of #52678

The code for choosing a fallback key when no default was found sensibly distinguished between keys that were old enough to have propagated to all instances and those that were not.  The code for choosing the preferred key should do the same.

This should help in the case where two instances both discover that a new key is needed and create an immediately-activated key.  One of them will get both keys back from the repository and it should choose the _older_ of the two to match the choice of of the other instance (which only knows about its own key).
@ghost ghost added the area-dataprotection Includes: DataProtection label Mar 1, 2024
@amcasey
Copy link
Member Author

amcasey commented Mar 1, 2024

This probably actually fixes #52678, but I want to do some more validation before I call it done.

Edit: nope - you can have an older key reach shared storage later, either due to clock skew or a plain old race.
Edit 2: for this to fix the race, we'd have to be able to sort the unpropagated keys by the order in which they were persisted to storage (not even the repository, but the backing store).

@amcasey amcasey requested a review from captainsafia March 1, 2024 23:49
@amcasey amcasey marked this pull request as draft March 2, 2024 01:00
@amcasey
Copy link
Member Author

amcasey commented Mar 2, 2024

While this seems more sensible than the current behavior, it doesn't obviously get us closer to fixing any races, so it may not be worth the observable change.

@amcasey amcasey marked this pull request as ready for review March 4, 2024 21:30
@amcasey amcasey merged commit e72eca7 into dotnet:main Mar 4, 2024
@amcasey amcasey deleted the PropagationCutoff branch March 4, 2024 21:31
@danroth27 danroth27 added this to the 9.0-preview3 milestone Apr 29, 2024
amcasey added a commit to amcasey/aspnetcore that referenced this pull request Aug 6, 2024
This reverts commit e72eca7.

It was a speculative improvement and now we have concrete evidence of (minor) problems.

Fixes dotnet#57137
amcasey added a commit that referenced this pull request Aug 8, 2024
* Revert "Prefer keys old enough to have propagated (#54309)"

This reverts commit e72eca7.

It was a speculative improvement and now we have concrete evidence of (minor) problems.

Fixes #57137

* Add a targeted regression test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dataprotection Includes: DataProtection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants