-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Feature/encrypt secret properties #4347
Feature/encrypt secret properties #4347
Conversation
…e from sensitive data
Hi @tanelkuhi , these changes are significant and it's going to take me a while to go through them. I see that the concept of encryption is now deeply integrated into UI components, so I will need to think about it and the potential ramifications of the changes from PR as a whole. |
@sfmskywalker the correct approach to this is to create a secrets store that utilizes a vault like Azure Key Vault or AWS Secrets Manager, instead of a SQL database. |
@johnwc I think you're right, but I don't think Azure KeyVault can be made a hard dependency of Elsa. Instead, it needs to be pluggable, right? |
@sfmskywalker correct, it would need to be a feature library that adds it as the default store for secret store. The current SQL store should be made feature library as well if it isn't already. So that the developer building out the server can choose what store to use for secrets. |
@johnwc Yes, perfect. |
Hello @tanelkuhi, Thank you for your contribution and effort in creating this pull request. I have reviewed the changes and I have some concerns regarding the modifications to the core contracts, such as I propose an alternative approach to achieve the same goal without altering the core contracts. We could introduce new domain events that are invoked when evaluating an activity's input. This notification could have a boolean property called Here's a brief outline of the proposed solution:
This approach keeps the core contracts intact and provides a decoupled and flexible way to handle the persistence of values. It also ensures that the Secrets module's requirements do not impose on the core interfaces, maintaining the system's modularity. I believe this approach aligns more with the project's architecture and design principles, and I look forward to hearing your thoughts on this. |
@sfmskywalker To merge this PR, do you expect also, that changes about the stores, that @johnwc proposed, or just the ones in the latest comment. I think the store changes might be a bit too much of a scope change for us. |
@tanelkuhi No, those are out of scope and can be done as a separate PR at a later time by anyone else. |
e5d3ff8
to
923d01d
Compare
We have a requirement to encrypt certain properties that you can set in the Secret properties.
These changes aim to make sure, that if the encryption is enabled through the appsettings, then the properties specified there will get encrypted upon saving of the secret.
When next opening the secret, you will not see the encrypted property values in frontend, but will instead see asterisks.
Also I had to make sure, that when an activity is run so, that it uses a property from secret, that is encrypted, that the value will not show up in any logs - they must not be seen anywhere in the frontend or in any storages. These property values should be evaluated by the expression evaluatior every time they are used.
Since my knowledge of Elsa is not complete, I may have missed a place, so if you know better, please let me know.