-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Secrets module (Lombiq Technologies: OCORE-124) #7891
Conversation
# Conflicts: # OrchardCore.sln
# Conflicts: # OrchardCore.sln
…Core into deanmarcussen/secrets
# Conflicts: # src/OrchardCore.Modules/OrchardCore.Email/Services/SmtpSettingsConfiguration.cs
var secret = JsonConvert.DeserializeObject(value, type) as SecretBase; | ||
|
||
return secret; | ||
return JsonConvert.DeserializeObject(value, type) as SecretBase; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and any other Newtonsoft.Json reference will need to be adapted once #14572 is done.
@@ -127,23 +127,23 @@ | |||
</div> | |||
|
|||
<div class="mb-3" asp-validation-class-for="EncryptionRsaSecret"> | |||
<label asp-for="EncryptionRsaSecret">@T["Encryption RSA secret"]</label> | |||
<label asp-for="EncryptionRsaSecret" class="form-label">@T["Encryption RSA secret"]</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-select the automatically generated secrets for all of such dropdowns. So, these features would work without further secret/encryption configuration by default, as today.
<ItemGroup> | ||
<EmbeddedResource Update="Views\Items\X509Secret.Fields.Thumbnail.cshtml"> | ||
<CopyToPublishDirectory>Never</CopyToPublishDirectory> | ||
</EmbeddedResource> | ||
</ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No exact reason, but I'd feel better with also an interface here, that's then referenced everywhere.
This pull request has merge conflicts. Please resolve those before requesting a review. |
{ | ||
return null; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add logging (informational)
|
||
public Task<SecretBase> GetSecretAsync(string name, Type type) | ||
{ | ||
if (!typeof(SecretBase).IsAssignableFrom(type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to check whether it is possible to (accidently) save a key, like your email password, to the database store even though it was originally provided via a configuration store.
My memory says we don't allow this to occur, I thi9nk we can display the secret (but not it's value), if hte store is readonly.
out of respect for @jtkech I will undertake completion of this pr. |
<div class="mb-3" asp-validation-class-for="EncryptionCertificateStoreLocation"> | ||
<div class="mb-3" asp-validation-class-for="EncryptionRsaSecret"> | ||
<label asp-for="EncryptionRsaSecret" class="form-label">@T["Encryption RSA secret"]</label> | ||
@await Component.InvokeAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nb: this component is not available unless the feature is on.
Currently it's written, like the background tasks feature, where the secrets services always exist, and are always on, and the secrets module, provides ui only, and is not required on.
However, for the component to be found, we need the module on, or have open id take a dependency on it (probably latter)
Something to tweak
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
Closing this pull request because it has been stale for very long. If you think this is still relevant, feel free to reopen it. |
Fixes #7137
Fixes #13205
Fixes #15087
Fixes #5558
Fixes #3259
Replaces #6421
Once this is merged, #14021 can continue.
Ready for a bit of a look, but not entirely ready.
Introduces a Secrets Module which can be used to abstract secrets in a way where they can be used across machines, and as part of deployment plans.
Uses a hybrid of asymmetric and symmetric encryption.
I've used RSA keys to encrypt a one time AES key, and the AES key is then used to encrypt each 'secret'
The idea being that the RSA keys must be shared outside of Orchard Core, and any imports / or setup recipes, will need to manage that RSA key, in order to decrypt the AES key. (mostly done due to not being able to limit the size of the 'secret' to be encrypted, and being unable to force it under the RSA size limits)
@kevinchalet if you have some time, would you mind taking a look at what I've done there? Still wip, and I see from some of the OpenId modules, that the key generation has some problems on Windows, which I'll need to allow for.
Also have stored them as the RSA key, rather than an X509 certificate, as you did there. Could be moved to a certificate if you thought that better?
Also need to have a good look at how setup will work with this, and the extra dependencies it has created