Add missing settings to plugin allowed list #1814

peternied · 2022-04-29T22:35:16Z

Description

As settings have migrated from opendistro_security -> plugins.security, there were missed settings for the audit filter entries. This caused unknown setting errors when starting OpenSearch, and these settings values were not being applied correctly affecting the runtime behavior of the audit log filters.

Created FilterEntries enum, as the filter keys/namespaced-keys are used
in more than once spot if we add a new filter key, we need a way to know
if the entries were updated in the otherside of the codebase where the
defaults are used. In the settings configuration, if a new enum value
is added and not included a runtime exception will fire on startup,
ensuring new settings are always included.

This could be better where the enum defines the default values and how
the settings are extracted from json, but that would be a considerable
larger scope with the inclusion of generics.

Signed-off-by: Peter Nied [email protected]

Issues Resolved

Closes [BUG] Documented audit log settings do not work #1581

Check List

New functionality includes testing
~~New functionality has been documented~~
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Created FilterEntries enum, as the filter keys/namespaced-keys are used in more than once spot if we add a new filter key, we need a way to know if the entries were updated in the otherside of the codebase where the defaults are used. In the settings configuration, if a new enum value is added and not included a runtime exception will fire on startup, ensuring new settings are always included. This could be better where the enum defines the default values and how the settings are extracted from json, but that would be a considerable larger scope with the inclusion of generics. Signed-off-by: Peter Nied <[email protected]>

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

Signed-off-by: Peter Nied <[email protected]>

peternied · 2022-05-27T18:12:13Z

Moving back to draft until all test cases have been fix

Signed-off-by: Peter Nied <[email protected]>

peternied · 2022-05-27T22:57:08Z

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java

@@ -130,9 +131,9 @@ public static AuditConfig from(final Settings settings) {
     */
    @JsonInclude(JsonInclude.Include.NON_NULL)
    public static class Filter {
+        private static Set<String> FIELDS = DefaultObjectMapper.getFields(Filter.class);


After adding changes to the .from method, this made the order of these static fields was incorrect causing static constructors to fail. A great reminder to use lazy loading avoiding static order of operations management.

…it config Signed-off-by: Peter Nied <[email protected]>

codecov-commenter · 2022-05-27T23:50:22Z

Codecov Report

Merging #1814 (a905e8e) into main (94cc878) will increase coverage by 0.13%.
The diff coverage is 96.72%.

@@             Coverage Diff              @@
##               main    #1814      +/-   ##
============================================
+ Coverage     60.87%   61.00%   +0.13%     
- Complexity     3215     3226      +11     
============================================
  Files           256      256              
  Lines         18012    18050      +38     
  Branches       3211     3212       +1     
============================================
+ Hits          10965    11012      +47     
+ Misses         5467     5459       -8     
+ Partials       1580     1579       -1

Impacted Files	Coverage Δ
...ensearch/security/auditlog/impl/AuditCategory.java	`100.00% <ø> (ø)`
.../opensearch/security/OpenSearchSecurityPlugin.java	`79.67% <80.00%> (+<0.01%)`	⬆️
...ensearch/security/auditlog/config/AuditConfig.java	`99.12% <100.00%> (+0.27%)`	⬆️
...pensearch/security/auditlog/impl/AuditMessage.java	`73.73% <100.00%> (+0.26%)`	⬆️
...iance/ComplianceIndexingOperationListenerImpl.java	`62.31% <0.00%> (+1.44%)`	⬆️
...ecurity/configuration/ConfigurationRepository.java	`74.31% <0.00%> (+2.18%)`	⬆️
...earch/security/ssl/util/SSLConnectionTestUtil.java	`95.45% <0.00%> (+2.27%)`	⬆️
...urity/ssl/transport/SecuritySSLNettyTransport.java	`66.66% <0.00%> (+4.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94cc878...a905e8e. Read the comment docs.

peternied · 2022-05-31T19:55:39Z

@opensearch-project/security Could I get a review on this pull request?

cliu123 · 2022-06-08T20:21:02Z

@peternied Could you please explain what the root cause of the issue #1581 is? That'll help me understand how this PR fixes the issue.

cliu123 · 2022-06-08T20:21:02Z

@peternied Could you please explain what the root cause of the issue #1581 is? That'll help me understand how this PR fixes the issue.

peternied · 2022-06-09T14:27:08Z

@cliu123 I've updated the description with the root cause/the nature of the fix.

As settings have migrated from opendistro_security -> plugins.security, there were missed settings for the audit filter entries. This caused unknown setting errors when starting OpenSearch, and these settings values were not being applied correctly affecting the runtime behavior of the audit log filters.

cliu123 · 2022-06-09T18:12:16Z

@peternied Please merge when you are ready.

peternied · 2022-06-09T18:15:38Z

There is an issue with the main branch build process, I am going to hold off on merging until it has been resolved

* Add missing settings to plugin allowed list As settings have migrated from opendistro_security -> plugins.security, there were missed settings for the audit filter entries. This caused unknown setting errors when starting OpenSearch, and these settings values were not being applied correctly affecting the runtime behavior of the audit log filters. Signed-off-by: Stephen Crawford <[email protected]>

* Add missing settings to plugin allowed list As settings have migrated from opendistro_security -> plugins.security, there were missed settings for the audit filter entries. This caused unknown setting errors when starting OpenSearch, and these settings values were not being applied correctly affecting the runtime behavior of the audit log filters.

peternied self-assigned this Apr 29, 2022

peternied added the bug Something isn't working label Apr 29, 2022

DarshitChanpura reviewed May 2, 2022

View reviewed changes

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java Outdated Show resolved Hide resolved

DarshitChanpura previously approved these changes May 3, 2022

View reviewed changes

peternied added 4 commits May 11, 2022 21:19

Attempt to fix test cases

acbbaf3

Signed-off-by: Peter Nied <[email protected]>

Fix test cases

23ed50c

Signed-off-by: Peter Nied <[email protected]>

Revert build.gradle debugging

33a98c6

Signed-off-by: Peter Nied <[email protected]>

Clarify how the setup works for tests

ab39340

Signed-off-by: Peter Nied <[email protected]>

peternied dismissed DarshitChanpura’s stale review via ab39340 May 26, 2022 22:51

Merge branch 'main' into fix-1581

55e6af5

Signed-off-by: Peter Nied <[email protected]>

peternied force-pushed the fix-1581 branch from a64dc26 to acbbaf3 Compare May 26, 2022 22:58

Remove debug statement

750aa22

Signed-off-by: Peter Nied <[email protected]>

peternied marked this pull request as ready for review May 26, 2022 23:01

peternied requested a review from a team May 26, 2022 23:01

peternied added 8 commits May 26, 2022 23:01

Fix spotless issues

3d2e4d8

Signed-off-by: Peter Nied <[email protected]>

Merge remote-tracking branch 'origin/main' into fix-1581

e20a03b

Signed-off-by: Peter Nied <[email protected]>

Fix broken import

be0e707

Signed-off-by: Peter Nied <[email protected]>

Small fix

1545e7d

Signed-off-by: Peter Nied <[email protected]>

Fix unused/missing imports

780ad32

Signed-off-by: Peter Nied <[email protected]>

Clarify default detection logic

f4f0048

Signed-off-by: Peter Nied <[email protected]>

Ignore case of 'none' value

f6cc619

Signed-off-by: Peter Nied <[email protected]>

Remove invalid java

63c932c

Signed-off-by: Peter Nied <[email protected]>

peternied marked this pull request as draft May 27, 2022 18:11

peternied added 3 commits May 27, 2022 18:49

Starting unit tests

d952577

Signed-off-by: Peter Nied <[email protected]>

renaming?

b134d4d

Signed-off-by: Peter Nied <[email protected]>

Add unit tests

2fc5860

Signed-off-by: Peter Nied <[email protected]>

peternied commented May 27, 2022

View reviewed changes

Also handle deprecated settings when moving values to the updated aud…

a905e8e

…it config Signed-off-by: Peter Nied <[email protected]>

peternied marked this pull request as ready for review May 31, 2022 16:04

DarshitChanpura approved these changes Jun 2, 2022

View reviewed changes

cliu123 approved these changes Jun 9, 2022

View reviewed changes

peternied merged commit 001d73f into opensearch-project:main Jun 9, 2022

peternied deleted the fix-1581 branch June 9, 2022 20:13

peternied mentioned this pull request Feb 13, 2023

[BUG] Unknown setting for plugins security audit #1392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing settings to plugin allowed list #1814

Add missing settings to plugin allowed list #1814

peternied commented Apr 29, 2022 •

edited

Loading

peternied commented May 27, 2022

peternied May 27, 2022

codecov-commenter commented May 27, 2022 •

edited

Loading

peternied commented May 31, 2022

cliu123 commented Jun 8, 2022

cliu123 commented Jun 8, 2022

peternied commented Jun 9, 2022

cliu123 commented Jun 9, 2022

peternied commented Jun 9, 2022

Add missing settings to plugin allowed list #1814

Add missing settings to plugin allowed list #1814

Conversation

peternied commented Apr 29, 2022 • edited Loading

Description

Issues Resolved

Check List

peternied commented May 27, 2022

peternied May 27, 2022

Choose a reason for hiding this comment

codecov-commenter commented May 27, 2022 • edited Loading

Codecov Report

peternied commented May 31, 2022

cliu123 commented Jun 8, 2022

cliu123 commented Jun 8, 2022

peternied commented Jun 9, 2022

cliu123 commented Jun 9, 2022

peternied commented Jun 9, 2022

peternied commented Apr 29, 2022 •

edited

Loading

codecov-commenter commented May 27, 2022 •

edited

Loading