search

OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.37k stars 2.38k forks source link

External storage for OpenID certificates #13205

Open Piedone opened 1 year ago

Piedone commented 1 year ago

Is your feature request related to a problem? Please describe.

Having the web server stateless, i.e. it not storing anything that you want to keep and can't be redeployed (like content, media), is useful for having a clear deployment story and a requirement for horizontal scaling. The OpenID module's OpenIdServerService stores certificates on the local file system, however. This causes the below exception if you wipe the storage (like when you deploy a new version of the app:

An error occurred while trying to extract a X.509 certificate.  

The key {ebf444b5-e9ea-43b6-9a1a-bdb4858a800e} was not found in the key ring. For more information go to http://aka.ms/dataprotectionwarning    

System.Security.Cryptography.CryptographicException:
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingBasedDataProtector.UnprotectCore (Microsoft.AspNetCore.DataProtection, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60)
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingBasedDataProtector.Unprotect (Microsoft.AspNetCore.DataProtection, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60)
   at Microsoft.AspNetCore.DataProtection.DataProtectionCommonExtensions.Unprotect (Microsoft.AspNetCore.DataProtection.Abstractions, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60)
   at OrchardCore.OpenId.Services.OpenIdServerService+<<GetCertificatesAsync>g__GetPasswordAsync|20_0>d.MoveNext (OrchardCore.OpenId, Version=1.5.0.0, Culture=neutral, PublicKeyToken=null)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at OrchardCore.OpenId.Services.OpenIdServerService+<<GetCertificatesAsync>g__GetCertificateAsync|20_1>d.MoveNext (OrchardCore.OpenId, Version=1.5.0.0, Culture=neutral, PublicKeyToken=null)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess (System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)
   at OrchardCore.OpenId.Services.OpenIdServerService+<GetCertificatesAsync>d__20.MoveNext (OrchardCore.OpenId, Version=1.5.0.0, Culture=neutral, PublicKeyToken=null)

This is logged here.

Despite this BTW, OpenID still appears to be working.

Related: https://github.com/OrchardCMS/OrchardCore/issues/7137

Describe the solution you'd like

Similar to Azure Data Protection we could have an implementation to store the certificates in a Blob Storage account. Basically, we could have something like IOpenIdServerCertificateStorage, with a default implementation that accesses local files like OpenIdServerService does today, and another feature that provides a Blob Storage-based implementation. (And later others can be added too.)

Describe alternatives you've considered

You need to fully override OpenIdServerService to implement this currently. Also see the comment by @kevinchalet here.

sebastienros commented 1 year ago

Kevin:

Also, using raw RSA keys instead of RSA keys embedded in X.509 certificates (for which we tend to see fewer issues in hostile environments) in conjunction with https://github.com/OrchardCMS/OrchardCore/pull/7891 might be a better approach than making the storage of the current feature replaceable.

Should we trust him?

kevinchalet commented 1 year ago

Should we trust him?

A tes risques et périls :trollface:

Piedone commented 1 year ago

Hmm, perhaps IOpenIdServerService.PruneManagedCertificatesAsync() can be used (or something else) to reset the certificate store after deployment, to get rid of the exception, at least.