Skip to content

Add unified disabled_categories audit log setting#6266

Closed
itsmevichu wants to merge 6 commits into
opensearch-project:mainfrom
itsmevichu:feature/gh-6222
Closed

Add unified disabled_categories audit log setting#6266
itsmevichu wants to merge 6 commits into
opensearch-project:mainfrom
itsmevichu:feature/gh-6222

Conversation

@itsmevichu

@itsmevichu itsmevichu commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Introduces a new unified disabled_categories setting for audit logging that applies to both the REST and transport layers, simplifying configuration. The existing disabled_rest_categories and disabled_transport_categories settings are deprecated but remain supported for backward compatibility.

  • Category: New feature

  • Why these changes are required?

Previously, users had to configure separate settings for REST and transport audit categories, requiring knowledge of which layer each category belonged to. This often led to misconfigurations, especially with transport-only categories such as CLUSTER_SETTINGS_CHANGED and INDEX_SETTINGS_CHANGED.

  • What is the old behavior before changes and new behavior after changes?

Before: Users configured disabled_rest_categories and disabled_transport_categories independently.

After: Users can configure a single setting:
config: audit: disabled_categories: - AUTHENTICATED - GRANTED_PRIVILEGES

Precedence:

disabled_categories takes precedence when configured.
If absent, the existing split settings continue to work unchanged.
Using the deprecated split settings emits a deprecation warning.

Issues Resolved

#6222

Is this a backport? No.

Testing

Unit tests and manual testing

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • 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.

@itsmevichu
Add unified disabled_categories audit log setting
6fad53f 
Signed-off-by: Vishnutheep B <vishnutheep@gmail.com>
@itsmevichu itsmevichu marked this pull request as draft June 29, 2026 16:12
@github-actions

github-actions Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit f987ff1.

PathLineSeverityDescription
config/audit.yml13mediumTop-level disabled_categories block disables AUTHENTICATED and GRANTED_PRIVILEGES audit events globally. These categories record successful logins and privilege grants — suppressing them reduces visibility into authentication and authorization activity across both REST and Transport APIs. This goes beyond the prior REST-only default and could mask unauthorized access patterns.
src/main/java/org/opensearch/security/support/ConfigConstants.java221mediumNew OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT includes CLUSTER_SETTINGS_CHANGED and INDEX_SETTINGS_CHANGED in addition to AUTHENTICATED and GRANTED_PRIVILEGES. The old REST default only suppressed AUTHENTICATED and GRANTED_PRIVILEGES; the new unified default expands suppression of cluster/index setting changes to REST auditing as well, reducing audit coverage for configuration-change events on the REST path.
src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java363lowDeprecation log message contains 'Settingssssss' (repeated trailing letters). While likely a typo, it is anomalous in production code paths and could indicate a hastily inserted or copy-pasted code block that deserves review.
src/test/java/org/opensearch/security/auditlog/config/AuditConfigSerializeTest.java449lowcompareJson was changed to silently catch all exceptions and return false rather than propagating them. This causes JSON parse errors or unexpected exceptions during test assertion to look like a simple equality failure, potentially masking broken serialization behavior in security-sensitive audit config tests.

The table above displays the top 10 most important findings.

Total: 4 | Critical: 0 | High: 0 | Medium: 2 | Low: 2

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions

github-actions Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit f1256a5)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Inconsistent Default Behavior

When loading from Settings, disabledCategoriesConfigured is derived from settings.hasValue(...), but the disabledCategories value uses OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT as a fallback (which includes AUTHENTICATED, GRANTED_PRIVILEGES, CLUSTER_SETTINGS_CHANGED, INDEX_SETTINGS_CHANGED). This is fine at runtime because isDisabledCategoriesConfigured() gates use of the field, but the default set is exposed via getDisabledCategories() and serialized (see testDefaultSerialize/testNullSerialize). Since REST/transport defaults remain unchanged and the unified default differs from the split defaults, a user reading the serialized config or an admin API response may be confused about what is actually excluded when disabled_categories was never set. Consider returning an empty set (or null) when not configured, or documenting this precedence clearly.

 
final Set<AuditCategory> disabledCategories = AuditCategory.parse(
    fromSettingStringSet(
        settings,
        FilterEntries.DISABLE_CATEGORIES,
        ConfigConstants.OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT
    )
);
Overly Permissive Category Whitelist

DISABLED_CATEGORIES validator whitelist omits COMPLIANCE_DOC_READ/WRITE/EXTERNAL_CONFIG (correctly rejecting them per the test) but the runtime checkRestFilter/checkTransportFilter early-return true from the disabledCategoriesConfigured branch without deferring to compliance categories. If a category is not listed in the unified disabled_categories but was configurable via the legacy split settings (e.g., certain transport-only categories), the unified path silently allows them through the same branch. This is the intended precedence, but ensure the REST endpoint validation for disabled_categories and the actual runtime allowed set are aligned; today the validator forbids compliance categories in disabled_categories, which means users cannot disable them via the unified setting even though they could historically via split settings.

 
public static final Set<AuditCategory> DISABLED_CATEGORIES = Set.of(
    AuditCategory.BAD_HEADERS,
    AuditCategory.SSL_EXCEPTION,
    AuditCategory.AUTHENTICATED,
    AuditCategory.FAILED_LOGIN,
    AuditCategory.GRANTED_PRIVILEGES,
    AuditCategory.MISSING_PRIVILEGES,
    AuditCategory.INDEX_EVENT,
    AuditCategory.OPENDISTRO_SECURITY_INDEX_ATTEMPT,
    AuditCategory.CLUSTER_SETTINGS_CHANGED,
    AuditCategory.INDEX_SETTINGS_CHANGED
);
Deprecation Warning Trigger

The deprecation warning in from(Settings) is emitted whenever any of the split rest/transport keys has a value, but settings.hasValue() returns true even for values that match the plugin-registered defaults in some flows. Since OpenSearchSecurityPlugin.getSettings() registers these split settings with non-empty defaults (AUTHENTICATED, GRANTED_PRIVILEGES, ...), users who never configured them but have the defaults applied via elasticsearch.yml/opensearch.yml explicit lines may see spurious deprecation warnings. Confirm hasValue only returns true when the user explicitly sets the key in yaml/API, not when it falls back to the registered default.

 
if (settings.hasValue(FilterEntries.DISABLE_REST_CATEGORIES.getKeyWithNamespace())
    || settings.hasValue(FilterEntries.DISABLE_REST_CATEGORIES.getLegacyKeyWithNamespace())
    || settings.hasValue(FilterEntries.DISABLE_TRANSPORT_CATEGORIES.getKeyWithNamespace())
    || settings.hasValue(FilterEntries.DISABLE_TRANSPORT_CATEGORIES.getLegacyKeyWithNamespace())) {
    final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(AuditConfig.class);
    deprecationLogger.deprecate(
        "disabled_rest_transport_categories",
        "Settings 'disabled_rest_categories' and 'disabled_transport_categories' are deprecated. Use 'disabled_categories' instead."
    );
}

@github-actions

github-actions Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to f1256a5

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid applying defaults when not configured

When disabled_categories is not configured, the code still parses the default list
and populates disabledCategories. This causes the field to be serialized/exposed as
if configured, and later the JSON serializer emits a non-null value even when only
legacy fields were set. Consider parsing an empty list (or null) when
disabledCategoriesConfigured is false to avoid confusing consumers of
getDisabledCategories() and misaligned serialization.

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java [258-267]

 final boolean disabledCategoriesConfigured = properties.containsKey(FilterEntries.DISABLE_CATEGORIES.getKey());
 
 // Defaults for disabledCategories are not applied; fallback is to REST/transport split settings.
-final Set<AuditCategory> disabledCategories = AuditCategory.parse(
-    getOrDefault(
-        properties,
-        FilterEntries.DISABLE_CATEGORIES.getKey(),
-        ConfigConstants.OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT
-    )
-);
+final Set<AuditCategory> disabledCategories = disabledCategoriesConfigured
+    ? AuditCategory.parse(getOrDefault(properties, FilterEntries.DISABLE_CATEGORIES.getKey(), Collections.emptyList()))
+    : Collections.emptySet();
Suggestion importance[1-10]: 6

__

Why: Valid concern: when disabled_categories is not configured, defaults are still applied and serialized, which may confuse consumers and cause inconsistent behavior with the legacy fallback logic gated on isDisabledCategoriesConfigured().

Low
Validate only user-supplied disabled categories

Because filter.getDisabledCategories() is always populated (with defaults) even when
the user did not supply disabled_categories, this validation may now run against
defaults and could reject configs that only set the legacy fields if defaults ever
change. Gate the check on filter.isDisabledCategoriesConfigured() so only
user-provided categories are validated.

src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java [207-209]

-if (!DISABLED_CATEGORIES.containsAll(filter.getDisabledCategories())) {
+if (filter.isDisabledCategoriesConfigured() && !DISABLED_CATEGORIES.containsAll(filter.getDisabledCategories())) {
     throw new IllegalArgumentException("Invalid REST/Transport categories passed in the request");
 }
Suggestion importance[1-10]: 5

__

Why: Reasonable defensive improvement; gating validation on isDisabledCategoriesConfigured() prevents false rejections if defaults change, though currently defaults are within the allowed set.

Low
Possible issue
Avoid short-circuiting subsequent filter checks

Returning true here bypasses any subsequent filter logic that follows this block
(e.g., additional user/request checks below the legacy category check). Confirm no
filter step is being skipped; if there are further checks after the legacy branch,
refactor to only short-circuit the category check while continuing the remaining
filter evaluation.

src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java [1132-1140]

 if (auditConfigFilter.isDisabledCategoriesConfigured()) {
     if (auditConfigFilter.getDisabledCategories().contains(category)) {
         if (isTraceEnabled) {
             log.trace("Skipped audit log message because category {} not enabled", category);
         }
         return false;
     }
-    return true;
+} else if (auditConfigFilter.getDisabledTransportCategories().contains(category)) {
+    if (isTraceEnabled) {
+        log.trace("Skipped audit log message because category {} not enabled", category);
+    }
+    return false;
 }
+return true;
Suggestion importance[1-10]: 3

__

Why: The suggestion's improved code is functionally equivalent to the original in the visible diff since both branches ultimately return true after the category check; the concern about skipped filter logic is not substantiated by the shown context.

Low

Previous suggestions

Suggestions up to commit f987ff1
CategorySuggestion                                                                                                                                    Impact
General
Fix typo in deprecation warning message

The deprecation message contains a typo ("Settingssssss") that will appear in
user-facing deprecation logs. Fix the spelling to "Settings" so operators receive a
professional and clear deprecation warning.

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java [368]

 deprecationLogger.deprecate(
     "disabled_rest_transport_categories",
-    "Settingssssss 'disabled_rest_categories' and 'disabled_transport_categories' are deprecated. Use 'disabled_categories' instead."
+    "Settings 'disabled_rest_categories' and 'disabled_transport_categories' are deprecated. Use 'disabled_categories' instead."
 );
Suggestion importance[1-10]: 8

__

Why: The typo "Settingssssss" is clearly visible in the diff and would appear in user-facing deprecation logs, making this a valid and important fix.

Medium
Align default for disabled_categories setting

The new disabled_categories node setting is being registered here with a legacy
hardcoded default of ["AUTHENTICATED", "GRANTED_PRIVILEGES"], while
FilterEntries.DISABLE_CATEGORIES uses
OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT (which additionally includes
CLUSTER_SETTINGS_CHANGED/INDEX_SETTINGS_CHANGED). This inconsistency will lead to
different effective defaults depending on which registration path is used. Use the
same default constant in both places.

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [1736-1743]

 settings.add(
     Setting.listSetting(
         ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_CATEGORIES,
-        disabledCategories,
+        ConfigConstants.OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT,
         Function.identity(),
         Property.NodeScope
     )
 );
Suggestion importance[1-10]: 7

__

Why: Correctly identifies an inconsistency between the registered node setting default (only AUTHENTICATED/GRANTED_PRIVILEGES from the reused disabledCategories variable) and the intended default constant, which could cause confusing behavior.

Medium
Avoid repeated deprecation log emissions

The deprecation warning is emitted every time Filter.from(Map) is invoked, which can
occur on every audit config reload/read and flood deprecation logs. Consider
emitting it only once, or attaching it to a specific code path (e.g., initial load)
to avoid log spam.

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java [293-300]

 if (properties.containsKey(FilterEntries.DISABLE_REST_CATEGORIES.getKey())
     || properties.containsKey(FilterEntries.DISABLE_TRANSPORT_CATEGORIES.getKey())) {
+    // Note: DeprecationLogger deduplicates by key, but callers should ensure this path is not hot.
     final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(AuditConfig.class);
     deprecationLogger.deprecate(
         "disabled_rest_transport_categories",
         "Properties 'disabled_rest_categories' and 'disabled_transport_categories' are deprecated. Use 'disabled_categories' instead."
     );
 }
Suggestion importance[1-10]: 3

__

Why: DeprecationLogger typically deduplicates by key, and the suggestion itself only adds a comment without a concrete code change to prevent log spam.

Low
Possible issue
Guard against null disabled categories

filter.getDisabledCategories() can be null when the field is not provided in the
payload; containsAll(null) will throw a NullPointerException instead of producing
the intended validation error. Guard against null before performing the containment
check.

src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java [207-209]

-if (!DISABLED_CATEGORIES.containsAll(filter.getDisabledCategories())) {
+if (filter.getDisabledCategories() != null && !DISABLED_CATEGORIES.containsAll(filter.getDisabledCategories())) {
     throw new IllegalArgumentException("Invalid REST/Transport categories passed in the request");
 }
Suggestion importance[1-10]: 6

__

Why: If getDisabledCategories() returns null, containsAll(null) would throw NPE. However, the field may typically be initialized to a non-null default, so the actual risk depends on deserialization behavior.

Low
Suggestions up to commit 8fc06d5
CategorySuggestion                                                                                                                                    Impact
General
Fix typo in deprecation warning message

The deprecation message contains a typo ("Settingssssss") which will be logged
verbatim and is confusing to operators. Correct it to "Settings" for a clean,
professional deprecation warning.

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java [369]

 deprecationLogger.deprecate(
     "disabled_rest_transport_categories",
-    "Settingssssss 'disabled_rest_categories' and 'disabled_transport_categories' are deprecated. Use 'disabled_categories' instead."
+    "Settings 'disabled_rest_categories' and 'disabled_transport_categories' are deprecated. Use 'disabled_categories' instead."
 );
Suggestion importance[1-10]: 8

__

Why: The typo "Settingssssss" is a clear bug in a user-facing deprecation message that would appear in production logs, and the fix is straightforward and important for professionalism.

Medium
Align default handling with intended fallback semantics

The comment states that no defaults are applied to disabledCategories, but the code
passes OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT as the default. When
disabledCategoriesConfigured is false the defaults are still parsed and stored,
which is misleading; either pass an empty list as default or update the comment to
reflect reality.

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java [258-268]

 final boolean disabledCategoriesConfigured = properties.containsKey(FilterEntries.DISABLE_CATEGORIES.getKey());
 
-
-// Defaults for disabledCategories are not applied; fallback is to REST/transport split settings.
+// When disabled_categories is not configured, use an empty set; the fallback to legacy REST/transport
+// split settings is handled at filter-check time via isDisabledCategoriesConfigured().
 final Set<AuditCategory> disabledCategories = AuditCategory.parse(
     getOrDefault(
         properties,
         FilterEntries.DISABLE_CATEGORIES.getKey(),
-            ConfigConstants.OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT
+        Collections.emptyList()
     )
 );
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a mismatch between the comment and code behavior. Since disabledCategoriesConfigured gates usage, the actual value when unconfigured doesn't matter for logic, but the inconsistency is confusing.

Low
Possible issue
Avoid emitting null disabled_categories field

Serializing disabled_categories unconditionally will always emit the field (as null
when unset), which will be interpreted by AuditConfig.Filter.from as
disabledCategoriesConfigured=true and cause the unified path to override the split
REST/transport categories in tests. Only write the field when it is non-null to
preserve legacy test behavior.

src/integrationTest/java/org/opensearch/test/framework/AuditFilters.java [143]

 xContentBuilder.field("ignore_url_params", ignoreUrlParams);
-xContentBuilder.field("disabled_categories", disabledCategories);
+if (disabledCategories != null) {
+    xContentBuilder.field("disabled_categories", disabledCategories);
+}
 xContentBuilder.field("disabled_rest_categories", disabledRestCategories);
 xContentBuilder.field("disabled_transport_categories", disabledTransportCategories);
Suggestion importance[1-10]: 7

__

Why: This is a valid concern: unconditional serialization of disabled_categories as null could cause containsKey to return true in the from method, unexpectedly enabling the unified path and altering existing test semantics.

Medium
Prevent NPE when validating disabled categories

filter.getDisabledCategories() may be null if the input JSON does not provide
disabled_categories and the constructor path assigns null. Guard for null before
calling containsAll to prevent an NPE on validation of legacy payloads.

src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java [207-209]

-if (!DISABLED_CATEGORIES.containsAll(filter.getDisabledCategories())) {
+if (filter.getDisabledCategories() != null && !DISABLED_CATEGORIES.containsAll(filter.getDisabledCategories())) {
     throw new IllegalArgumentException("Invalid REST/Transport categories passed in the request");
 }
Suggestion importance[1-10]: 5

__

Why: Guarding against null is reasonable defensive programming, though the from method typically initializes to a default set. The existing DISABLED_REST/TRANSPORT checks below have the same pattern without null checks, suggesting this may not be a real issue.

Low
Suggestions up to commit 39b23b5
CategorySuggestion                                                                                                                                    Impact
General
Fix typo in deprecation log message

The deprecation message contains a typo ("Settingssssss") which will appear in
production deprecation logs. Fix the typo to make the message clear and
professional.

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java [369]

 deprecationLogger.deprecate(
     "disabled_rest_transport_categories",
-    "Settingssssss 'disabled_rest_categories' and 'disabled_transport_categories' are deprecated. Use 'disabled_categories' instead."
+    "Settings 'disabled_rest_categories' and 'disabled_transport_categories' are deprecated. Use 'disabled_categories' instead."
 );
Suggestion importance[1-10]: 8

__

Why: The typo "Settingssssss" is a real user-visible issue that would appear in production deprecation logs, and fixing it is a clear, correct improvement.

Medium
Gate category validation on explicit configuration

When disabled_categories is not configured in the request,
filter.getDisabledCategories() will contain the default set (including
CLUSTER_SETTINGS_CHANGED and INDEX_SETTINGS_CHANGED), which is validated. But when
parsed from a REST payload not including this field, the default fallback still
results in a non-empty set being validated against DISABLED_CATEGORIES — this is
fine, but the validation should only run when the field was actually provided to
avoid confusing errors when defaults are somehow altered. Consider gating this check
on filter.isDisabledCategoriesConfigured().

src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java [207-209]

-if (!DISABLED_CATEGORIES.containsAll(filter.getDisabledCategories())) {
+if (filter.isDisabledCategoriesConfigured() && !DISABLED_CATEGORIES.containsAll(filter.getDisabledCategories())) {
     throw new IllegalArgumentException("Invalid REST/Transport categories passed in the request");
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion is speculative since default disabledCategories are within the allowed DISABLED_CATEGORIES set, so it wouldn't cause current failures, but gating on configured flag is a reasonable defensive practice.

Low
Reconcile misleading comment about default handling

The comment states "Defaults for disabledCategories are not applied", but the code
passes OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT as the default value,
which contradicts the comment and causes getDisabledCategories() to return a
non-empty default even when the user did not configure the unified setting. Combined
with the isDisabledCategoriesConfigured() gate this may be intentional, but the
misleading comment (and default-passing) should be reconciled — either remove the
default here or update the comment.

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java [258-268]

 final boolean disabledCategoriesConfigured = properties.containsKey(FilterEntries.DISABLE_CATEGORIES.getKey());
 
-
-// Defaults for disabledCategories are not applied; fallback is to REST/transport split settings.
+// When unified setting not configured, defaults are still populated for backward-compatible getters.
 final Set<AuditCategory> disabledCategories = AuditCategory.parse(
     getOrDefault(
         properties,
         FilterEntries.DISABLE_CATEGORIES.getKey(),
-            ConfigConstants.OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT
+        ConfigConstants.OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT
     )
 );
Suggestion importance[1-10]: 3

__

Why: The observation about the misleading comment is accurate but has minor impact — only affects code clarity/documentation, not functionality.

Low
Possible issue
Ensure configured flag propagates through JSON deserialization

The disabledCategoriesConfigured flag is not populated during JSON deserialization
because it is not a @JsonProperty and the @JsonCreator constructor path does not set
it. This means isDisabledCategoriesConfigured() will always return false when the
config is loaded from the index, defeating the purpose of the unified setting when
configured via API. Ensure the flag is set based on whether disabled_categories is
present in the deserialized JSON (e.g., via a custom setter or by detecting a
non-null field during deserialization).

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java [551-554]

+@JsonIgnore
+public boolean isDisabledCategoriesConfigured() {
+    return disabledCategoriesConfigured;
+}
 
-
Suggestion importance[1-10]: 7

__

Why: This raises a valid concern that the disabledCategoriesConfigured flag may not be properly set during Jackson deserialization from the index, which could cause the unified setting to be silently ignored when loaded via API-stored config.

Medium
Suggestions up to commit 6fad53f
CategorySuggestion                                                                                                                                    Impact
General
Fix typo in deprecation log message

The deprecation message contains a typo ("Settingssssss") which will appear in
user-facing logs. Fix the typo to ensure the deprecation message is clear and
professional.

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java [368]

 if (settings.hasValue(FilterEntries.DISABLE_REST_CATEGORIES.getKeyWithNamespace())
     || settings.hasValue(FilterEntries.DISABLE_REST_CATEGORIES.getLegacyKeyWithNamespace())
     || settings.hasValue(FilterEntries.DISABLE_TRANSPORT_CATEGORIES.getKeyWithNamespace())
     || settings.hasValue(FilterEntries.DISABLE_TRANSPORT_CATEGORIES.getLegacyKeyWithNamespace())) {
     final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(AuditConfig.class);
     deprecationLogger.deprecate(
         "disabled_rest_transport_categories",
-        "Settingssssss 'disabled_rest_categories' and 'disabled_transport_categories' are deprecated. Use 'disabled_categories' instead."
+        "Settings 'disabled_rest_categories' and 'disabled_transport_categories' are deprecated. Use 'disabled_categories' instead."
     );
 }
Suggestion importance[1-10]: 8

__

Why: The typo "Settingssssss" is clearly a mistake that would appear in user-facing deprecation logs. Fixing it improves professionalism and clarity.

Medium
Add Filtered property to new audit setting

The legacy disabled_rest_categories and disabled_transport_categories settings
registered below include Property.Filtered (implied via the surrounding pattern),
but this new disabled_categories setting only declares Property.NodeScope. Without
Property.Filtered, the setting value may be exposed via node info APIs. Add
Property.Filtered for consistency and safety.

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [1736-1743]

 settings.add(
     Setting.listSetting(
         ConfigConstants.OPENDISTRO_SECURITY_AUDIT_CONFIG_DISABLED_CATEGORIES,
         disabledCategories,
         Function.identity(),
-        Property.NodeScope
+        Property.NodeScope,
+        Property.Filtered
     )
 );
Suggestion importance[1-10]: 6

__

Why: Adding Property.Filtered for consistency with related audit settings is a reasonable improvement, though the actual security impact depends on the nature of the setting values.

Low
Align fallback with documented behavior

The comment states that defaults are not applied when disabled_categories is not
configured, but getOrDefault is called with
OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT, which means a non-empty
default set will be populated into disabledCategories regardless. Since
isDisabledCategoriesConfigured() gates behavior, this is fine - but if any code path
later reads getDisabledCategories() without checking the flag, it will get the
defaults silently. Use an empty list as the fallback to match the documented intent.

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java [257-267]

 final boolean disabledCategoriesConfigured = properties.containsKey(FilterEntries.DISABLE_CATEGORIES.getKey());
 
 
 // Defaults for disabledCategories are not applied; fallback is to REST/transport split settings.
 final Set<AuditCategory> disabledCategories = AuditCategory.parse(
     getOrDefault(
         properties,
         FilterEntries.DISABLE_CATEGORIES.getKey(),
-            ConfigConstants.OPENDISTRO_SECURITY_AUDIT_DISABLED_CATEGORIES_DEFAULT
+        Collections.emptyList()
     )
 );
Suggestion importance[1-10]: 5

__

Why: The comment contradicts the actual behavior (defaults are applied), and the suggestion improves consistency, though current gating by isDisabledCategoriesConfigured() mitigates the practical issue.

Low
Validate only when user provided categories

getDisabledCategories() may return a non-empty set populated from defaults even when
the user did not configure disabled_categories (depending on from(Map) behavior).
Guard the validation with isDisabledCategoriesConfigured() to avoid validating
categories the user never provided, or ensure the returned set reflects only user
input.

src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java [207-209]

-if (!DISABLED_CATEGORIES.containsAll(filter.getDisabledCategories())) {
+if (filter.isDisabledCategoriesConfigured()
+    && !DISABLED_CATEGORIES.containsAll(filter.getDisabledCategories())) {
     throw new IllegalArgumentException("Invalid REST/Transport categories passed in the request");
 }
Suggestion importance[1-10]: 5

__

Why: Guarding validation by isDisabledCategoriesConfigured() could avoid validating default-populated values, but since defaults are valid categories anyway, the practical impact may be minor.

Low

@itsmevichu
Add unit tests
39b23b5 
Signed-off-by: Vishnutheep B <vishnutheep@gmail.com>
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 39b23b5

@itsmevichu itsmevichu marked this pull request as ready for review July 5, 2026 12:27
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8fc06d5

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f987ff1

@itsmevichu
Update AuditConfig.java
f1256a5 
Signed-off-by: Vishnutheep B <vishnutheep@gmail.com>
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f1256a5

@itsmevichu

itsmevichu commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Closing on behalf of #6271

@itsmevichu itsmevichu closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cwperks cwperks Awaiting requested review from cwperks cwperks is a code owner
@DarshitChanpura DarshitChanpura Awaiting requested review from DarshitChanpura DarshitChanpura is a code owner
@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner
@finnegancarroll finnegancarroll Awaiting requested review from finnegancarroll finnegancarroll is a code owner
@nibix nibix Awaiting requested review from nibix nibix is a code owner
@Rishav9852Kumar Rishav9852Kumar Awaiting requested review from Rishav9852Kumar Rishav9852Kumar is a code owner
@RyanL1997 RyanL1997 Awaiting requested review from RyanL1997 RyanL1997 is a code owner
@reta reta Awaiting requested review from reta reta is a code owner
@shikharj05 shikharj05 Awaiting requested review from shikharj05 shikharj05 is a code owner
@willyborankin willyborankin Awaiting requested review from willyborankin willyborankin is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@itsmevichu