-
Notifications
You must be signed in to change notification settings - Fork 10.3k
data protection API creates a new default key if decrypting an existing one fails #21096
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
Comments
DataProtection is meant to be reasonably fool proof and a black box, and minimises extensibility to support that goal. Throwing exceptions rather than recovering is not something we would consider when recovery is possible and an application can keep going. @GrabYourPitchforks any thoughts? |
However in this case the 'recovery' is actively harmful -- simply failing would be much better. This affects the (presumably common) use case of a web farm by breaking the ability of existing servers to decrypt stored data/cookies/xsrf tokens -- crashing the server and having it restart from scratch via an external recovery mecahanism would be preferable. (In general I view this as a good example of why applications shouldn't continue after encountering unexpected errors). |
@blowdart Isn't there the ability to put the system into a read-only mode? That would allow exceptions to propagate back up the stack. |
Yes, but then you have to find a way to create new keys before expiry. Which is, well, painful, if you have a keystore that times out. |
In a scenario like this that might be desirable, though, if only one entity is ever supposed to be populating the key store. A cron job or similar could do it. |
We encountered this same problem with data protection as well. We have configured the keys to be stored in Azure blob storage and to be protected by a Key in Azure KeyVault. dataProtectionBuilder.PersistKeysToAzureBlobStorage One instance of the app failed to connect to KeyVault due to what I believe was a transient error. It then output the following warning and created a new key and saved it into blob storage. [WRN] Key "62300cc4-bce9" is ineligible to be the default key because its CreateEncryptor method failed. This then broke all other instances of the app as they did not have the new key. We restarted all instances to get requests to work. Presumably they would have eventually refreshed and read the new key. In this situation instead of creating a new key it would have been better if it returned error or retried internally. |
Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue. This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue! |
Describe the bug
The error below is an example of starting an app, then downloading a key from azure storage which fails with a timeout (it appears to have downloaded the data and then timed out in this case).
This was a transient failure, but the built in policy in src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs seems to be to log the issue, and continue, whereas in this case propagating the exception (or a wrapped version thereof) back to the top level caller would be more appropriate.
This is because we share the data protection keys across a number of processes (for web apis with cookie authentication, and for protecting data at reset). In that case generating a new key and ignoring any existing one is actively harmful as the activation date of the newly generated key is immediate. Each running process will refresh its key list around 24 hours after starting, and thus the key list of each prcoess can be out of sync for some time causing data encrypted with newer keys to fail to be decrypted by processes that haven't yet refreshed their key lists. It would be better here to propagate the failure (which in our case would lead to a process fail and restart).
If there are known specific circumstances under which it is better to handle key encryptor creation failure perhaps CanCreateAuthenticatedEncryptor could handle those specific circumstances only, rather than the general exception swallowing behaviour, or perhaps there could be a policy setting to not handle those failures at all?
To Reproduce
Blocking access to the key vault url containing the encryption key, then start an app that uses that encryption key should reproduce the problem.
Further technical details
dotnet --info
VS2019 16.5.4
The text was updated successfully, but these errors were encountered: