-
Notifications
You must be signed in to change notification settings - Fork 194
[backend] feat(catalog): encryption of pwd values in db (#4605) #4689
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
base: release/current
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release/current #4689 +/- ##
=====================================================
+ Coverage 52.82% 52.91% +0.08%
- Complexity 4138 4166 +28
=====================================================
Files 966 968 +2
Lines 29007 29087 +80
Branches 2162 2169 +7
=====================================================
+ Hits 15323 15391 +68
- Misses 12779 12792 +13
+ Partials 905 904 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This pull request implements encryption for sensitive password values stored in the database. The feature adds native encryption support using Spring Security's Encryptors.delux() method, complementing the existing XTM Composer encryption strategy.
Key changes:
- Introduces
NativeEncryptionServicefor encrypting/decrypting sensitive configuration values using AES encryption - Adds
encryption_keyandencryption_saltconfiguration properties toOpenAEVAdminConfig - Updates all executor integrations (Tanium, SentinelOne, CrowdStrike, Caldera, OpenAEV) to support encryption/decryption during configuration loading
- Implements a
refresh()method in Integration classes to reload decrypted configurations on-demand
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
OpenAEVAdminConfig.java |
Adds encryption_key and encryption_salt configuration properties |
application.properties |
Documents new mandatory encryption configuration properties |
NativeEncryptionService.java |
New service implementing EncryptionService interface using Spring Security Encryptors |
EncryptionFactory.java |
Updated to return NativeEncryptionService for non-manager connectors instead of null |
UncypherableElementException.java |
New exception for encryption failures |
BaseIntegrationConfiguration.java |
Modified to encrypt values on save and decrypt on load based on field encryption flag |
Integration.java |
Adds encryptionService field and abstract refresh() method for reloading configs |
IntegrationFactory.java |
Updated to inject EncryptionFactory and HttpClientFactory |
ConfigurationMigration.java |
Passes encryption service when converting configurations |
| Executor factories (Tanium, SentinelOne, CrowdStrike, Caldera) | Updated to instantiate encryption service and pass to integrations |
| Executor integrations | Implement refresh() to reload and decrypt configurations, initialize client with decrypted config |
| Migration tests | Add validation that encrypted fields are actually encrypted in database |
| Integration tests | Updated to inject EncryptionFactory and test null encryption service handling |
...rc/test/java/io/openaev/integration/migration/CalderaExecutorConfigurationMigrationTest.java
Show resolved
Hide resolved
openaev-api/src/test/java/io/openaev/integration/impl/SentinelOneExecutorIntegrationTest.java
Outdated
Show resolved
Hide resolved
openaev-api/src/test/java/io/openaev/integration/impl/CalderaExecutorIntegrationTest.java
Outdated
Show resolved
Hide resolved
...est/java/io/openaev/integration/migration/CrowdStrikeExecutorConfigurationMigrationTest.java
Show resolved
Hide resolved
...aev-api/src/main/java/io/openaev/integration/configuration/BaseIntegrationConfiguration.java
Show resolved
Hide resolved
openaev-api/src/test/java/io/openaev/integration/impl/TaniumExecutorIntegrationTest.java
Outdated
Show resolved
Hide resolved
openaev-api/src/test/java/io/openaev/integration/impl/CrowdStrikeExecutorIntegrationTest.java
Outdated
Show resolved
Hide resolved
...aev-api/src/main/java/io/openaev/integration/configuration/BaseIntegrationConfiguration.java
Outdated
Show resolved
Hide resolved
...src/test/java/io/openaev/integration/migration/TaniumExecutorConfigurationMigrationTest.java
Show resolved
Hide resolved
...est/java/io/openaev/integration/migration/SentinelOneExecutorConfigurationMigrationTest.java
Show resolved
Hide resolved
| #openaev.admin.encryption_key=ChangeMe #mandatory | ||
| #openaev.admin.encryption_salt=ChangeMe #mandatory |
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.
question: This is a breaking changes for version upgrade right ?
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.
Yes, I have planned to add this to the doc and this will need to be correctly announced in the release note
| @NotNull Set<ConnectorInstanceConfiguration> configurations, Class<T> targetClass) | ||
| @NotNull ConnectorInstance instance, | ||
| Class<T> targetClass, | ||
| EncryptionService encryptionService) |
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.
It feels odd to pass a service as a method argument.
This breaks the dependency injection model.
We should move this logic into a dedicated service and embed the method there instead.
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.
Yeah, I do agree that was my initial intention but I had cyclic dependencies. I did not think of moving part of the code to a dedicated utils, I'll try to change that :)
| private final ObjectMapper mapper = new ObjectMapper(); | ||
| @Getter @Setter private boolean enable = false; | ||
|
|
||
| public static <T extends BaseIntegrationConfiguration> T fromConnectorInstanceConfigurationSet( |
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 method feels overly complex to me. Could we simplify it by passing the config and only decrypting the relevant keys?
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 method do not only do decrypting. It's used to create a Configuration java model from the database model. I guess I can simplify it a bit with another private method but it can't be used only for the relevant keys.
Proposed changes
Testing Instructions
Related issues
Checklist
Further comments
In your application.properties (or application-dev.properties), fill the openaev.admin.encryption_key and openaev.admin.encryption_salt properties. The documentation for the encryption library provided by spring is available here : https://docs.spring.io/spring-security/reference/features/integrations/cryptography.html#spring-security-crypto-encryption-text