Feat/audit mdc routing

[Taiwo435]

Description

Enrich Log4j MDC (Mapped Diagnostic Context) with audit event attributes before each log call in Log4JSink, enabling operators to use Log4j's native RoutingAppender to split audit logs by category, user, action, or request type into separate files.

Four MDC keys added:

• audit_category: event type (e.g., GRANTED_PRIVILEGES, FAILED_LOGIN, COMPLIANCE_DOC_WRITE)
• audit_action: the privilege/action being audited (e.g., indices_data_write_index)
• audit_user: the effective user for the request
• audit_request_type: the transport request class (e.g., IndexRequest, BulkShardRequest)

Why

Operators running high-volume clusters need to route different audit event types to different destinations — security-critical events (FAILED_LOGIN, MISSING_PRIVILEGES) to a SIEM, compliance events to long-term storage, and routine GRANTED_PRIVILEGES to a rotated local file. Today, all events go to the same log file, requiring post-processing to separate them.

With MDC enrichment, a standard Log4j RoutingAppender config handles this natively:

<Routing name="AuditRouter">
  <Routes pattern="$${ctx:audit_category}">
    <Route ref="SecurityAlerts" key="FAILED_LOGIN"/>
    <Route ref="ComplianceLogs" key="COMPLIANCE_DOC_WRITE"/>
    <Route ref="DefaultAudit"/>
  </Routes>
</Routing>

How it works

• Log4JSink.doStore() sets 4 MDC keys before logging, removes them after (in a finally block)
• Values are sanitized for filename safety via a pre-compiled regex (colons, slashes, brackets, etc. → _)
• MDC keys are owned by the audit sink — only our 4 keys are removed after logging, preserving any pre-existing MDC entries from other plugins
• Disabled sinks skip MDC entirely (no unnecessary ThreadContext manipulation)

WARNING: audit_user should NOT be used as a RoutingAppender routing key for file paths — it is unbounded/attacker-influenceable and could cause inode/disk exhaustion. Use audit_category (bounded enum) instead.

Testing

Unit tests (Log4JSinkMdcTest) — 12 tests covering:

• MDC contains correct category/action/user/request_type during log call
• Action values sanitized for filename safety (colons, slashes, brackets → underscores)
• Null fields default to "unknown"
• MDC keys removed after doStore() completes (not clearMap())
• Different categories produce different MDC values (routing works)
• Concurrent threads do not leak MDC between events
• Long values preserved without truncation
• Special characters: only filesystem-dangerous chars sanitized
• Disabled sink skips MDC entirely
• Multiple sequential calls each get fresh MDC
• Multi-chunk split messages all share same MDC values
• MDC cleaned up even when serialization produces errors

Check List

• New functionality includes testing
• 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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 26cbb0f)

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 Concurrency Bug ThreadContext.put is called before msg.toJsonSplitIndices(...).forEach(...), but the forEach may execute the logging on a different thread if the stream is parallel, or the call to toJsonSplitIndices itself could throw before the try block if invoked outside it. More importantly, if toJsonSplitIndices throws (e.g., NullPointerException during serialization), the MDC values will have already been set but the try/finally covers this correctly. However, note that ThreadContext.put calls happen inside the try block — if the first put succeeds and a later put throws (unlikely but possible with unusual ThreadContext implementations), the finally cleanup covers it. The larger concern: MDC is thread-local, so if audit logging is ever dispatched asynchronously (via a thread pool executor for the sink), the MDC set on the caller thread will not be visible on the log-emitting thread. Confirm the sink runs synchronously on the calling thread in all execution paths. if (mdcRoutingEnabled) { try { // Push audit attributes into Log4j MDC so operators can use // RoutingAppender with $${ctx:audit_category} etc. to split logs. // WARNING: Do not use audit_user as a RoutingAppender routing key for file paths — // it is unbounded/attacker-influenceable and could cause inode/disk exhaustion. // Prefer audit_category (bounded enum) for file-based routing. ThreadContext.put(MDC_CATEGORY, sanitizeForMdc(msg.getCategory() != null ? msg.getCategory().name() : null)); ThreadContext.put(MDC_ACTION, sanitizeForMdc(msg.getPrivilege())); ThreadContext.put(MDC_USER, sanitizeForMdc(msg.getEffectiveUser())); ThreadContext.put(MDC_REQUEST_TYPE, sanitizeForMdc(msg.getRequestType())); msg.toJsonSplitIndices(maximumIndexCharactersPerMessage).forEach(message -> auditLogger.log(logLevel, message)); } finally { ThreadContext.remove(MDC_CATEGORY); ThreadContext.remove(MDC_ACTION); ThreadContext.remove(MDC_USER); ThreadContext.remove(MDC_REQUEST_TYPE); } } else { msg.toJsonSplitIndices(maximumIndexCharactersPerMessage).forEach(message -> auditLogger.log(logLevel, message)); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 26cbb0f

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Cap MDC value length to prevent filesystem issues The sanitizer does not cap the length of values, yet the code comment explicitly warns that user-controlled fields like audit_user can be attacker-influenced and unbounded. If any of these MDC values ends up in a RoutingAppender file path, an extremely long value (e.g., a 500+ char DN as tested) can exceed filesystem NAME_MAX (typically 255) and cause logging failures. Consider truncating to a reasonable maximum length. src/main/java/org/opensearch/security/auditlog/sink/Log4JSink.java [94-99] +static final int MAX_MDC_VALUE_LENGTH = 200; + static String sanitizeForMdc(String value) { if (value == null || value.isEmpty()) { return "unknown"; } - return UNSAFE_MDC_CHARS.matcher(value).replaceAll("_"); + String sanitized = UNSAFE_MDC_CHARS.matcher(value).replaceAll("_"); + if (sanitized.length() > MAX_MDC_VALUE_LENGTH) { + sanitized = sanitized.substring(0, MAX_MDC_VALUE_LENGTH); + } + return sanitized; } Suggestion importance[1-10]: 5 __ Why: Valid concern for RoutingAppender file paths, but the existing test testLongValuesNotTruncated explicitly asserts that long values are preserved by design, and the code comments warn operators to prefer bounded fields for routing. The suggestion contradicts the intended behavior.	Low
	Preserve pre-existing MDC values on restore The MDC put calls should save and restore any pre-existing values for these keys, not just remove them. If a caller further up the stack had already set audit_category (or another key), this code will clobber it and then leave the key blank after doStore returns. Save the prior values before put and restore them (or remove if null) in the finally block to avoid corrupting caller-owned ThreadContext state. src/main/java/org/opensearch/security/auditlog/sink/Log4JSink.java [61-82] if (mdcRoutingEnabled) { + final String prevCategory = ThreadContext.get(MDC_CATEGORY); + final String prevAction = ThreadContext.get(MDC_ACTION); + final String prevUser = ThreadContext.get(MDC_USER); + final String prevRequestType = ThreadContext.get(MDC_REQUEST_TYPE); try { - // Push audit attributes into Log4j MDC so operators can use - // RoutingAppender with $${ctx:audit_category} etc. to split logs. - // WARNING: Do not use audit_user as a RoutingAppender routing key for file paths — - // it is unbounded/attacker-influenceable and could cause inode/disk exhaustion. - // Prefer audit_category (bounded enum) for file-based routing. ThreadContext.put(MDC_CATEGORY, sanitizeForMdc(msg.getCategory() != null ? msg.getCategory().name() : null)); ThreadContext.put(MDC_ACTION, sanitizeForMdc(msg.getPrivilege())); ThreadContext.put(MDC_USER, sanitizeForMdc(msg.getEffectiveUser())); ThreadContext.put(MDC_REQUEST_TYPE, sanitizeForMdc(msg.getRequestType())); msg.toJsonSplitIndices(maximumIndexCharactersPerMessage).forEach(message -> auditLogger.log(logLevel, message)); } finally { - ThreadContext.remove(MDC_CATEGORY); - ThreadContext.remove(MDC_ACTION); - ThreadContext.remove(MDC_USER); - ThreadContext.remove(MDC_REQUEST_TYPE); + if (prevCategory != null) ThreadContext.put(MDC_CATEGORY, prevCategory); else ThreadContext.remove(MDC_CATEGORY); + if (prevAction != null) ThreadContext.put(MDC_ACTION, prevAction); else ThreadContext.remove(MDC_ACTION); + if (prevUser != null) ThreadContext.put(MDC_USER, prevUser); else ThreadContext.remove(MDC_USER); + if (prevRequestType != null) ThreadContext.put(MDC_REQUEST_TYPE, prevRequestType); else ThreadContext.remove(MDC_REQUEST_TYPE); } } else { Suggestion importance[1-10]: 4 __ Why: The comment in sanitizeForMdc states these MDC keys are owned by the audit sink and no other code should set them, so save-and-restore is likely unnecessary. However, the suggestion adds defensive robustness against ThreadContext corruption, which has minor value.	Low

---

Previous suggestions

Suggestions up to commit c9e6e8d

Category	Suggestion	Impact
Security	Cap MDC value length to protect routing Since the MDC values may be used by operators in RoutingAppender file paths, an unbounded length (e.g., long DN or attacker-influenced user field) can still cause filesystem errors or path-length exceptions. Enforce a maximum length inside sanitizeForMdc to defensively cap the value, protecting operators who route by user/action. src/main/java/org/opensearch/security/auditlog/sink/Log4JSink.java [94-99] +static final int MAX_MDC_VALUE_LENGTH = 256; + static String sanitizeForMdc(String value) { if (value == null || value.isEmpty()) { return "unknown"; } - return UNSAFE_MDC_CHARS.matcher(value).replaceAll("_"); + String sanitized = UNSAFE_MDC_CHARS.matcher(value).replaceAll("_"); + if (sanitized.length() > MAX_MDC_VALUE_LENGTH) { + sanitized = sanitized.substring(0, MAX_MDC_VALUE_LENGTH); + } + return sanitized; } Suggestion importance[1-10]: 5 __ Why: Capping length is a reasonable defense-in-depth measure, especially given the security warning about unbounded/attacker-influenced values. However, the existing test testLongValuesNotTruncated explicitly asserts values are not truncated, so this change would conflict with the intended design.	Low
General	Preserve pre-existing MDC values on restore The current implementation unconditionally removes the MDC keys in the finally block, which will erase values that any outer/caller code may have already placed under those keys on the same thread. Save the previous values before put and restore them afterwards, so the sink acts as a proper stack-friendly context modifier rather than a destructive one. src/main/java/org/opensearch/security/auditlog/sink/Log4JSink.java [61-79] if (mdcRoutingEnabled) { + final String prevCategory = ThreadContext.get(MDC_CATEGORY); + final String prevAction = ThreadContext.get(MDC_ACTION); + final String prevUser = ThreadContext.get(MDC_USER); + final String prevRequestType = ThreadContext.get(MDC_REQUEST_TYPE); try { - // Push audit attributes into Log4j MDC so operators can use - // RoutingAppender with $${ctx:audit_category} etc. to split logs. - // WARNING: Do not use audit_user as a RoutingAppender routing key for file paths — - // it is unbounded/attacker-influenceable and could cause inode/disk exhaustion. - // Prefer audit_category (bounded enum) for file-based routing. ThreadContext.put(MDC_CATEGORY, sanitizeForMdc(msg.getCategory() != null ? msg.getCategory().name() : null)); ThreadContext.put(MDC_ACTION, sanitizeForMdc(msg.getPrivilege())); ThreadContext.put(MDC_USER, sanitizeForMdc(msg.getEffectiveUser())); ThreadContext.put(MDC_REQUEST_TYPE, sanitizeForMdc(msg.getRequestType())); msg.toJsonSplitIndices(maximumIndexCharactersPerMessage).forEach(message -> auditLogger.log(logLevel, message)); } finally { - ThreadContext.remove(MDC_CATEGORY); - ThreadContext.remove(MDC_ACTION); - ThreadContext.remove(MDC_USER); - ThreadContext.remove(MDC_REQUEST_TYPE); + restoreOrRemove(MDC_CATEGORY, prevCategory); + restoreOrRemove(MDC_ACTION, prevAction); + restoreOrRemove(MDC_USER, prevUser); + restoreOrRemove(MDC_REQUEST_TYPE, prevRequestType); } } Suggestion importance[1-10]: 4 __ Why: The comment in the code explicitly states "These MDC keys are owned by the audit sink — no other code should set them", so restoring previous values may be unnecessary. However, defensive save/restore is a reasonable pattern for MDC manipulation.	Low

Suggestions up to commit be4a3b4

Category	Suggestion	Impact
General	Restore prior MDC values instead of removing Unconditionally overwriting and then removing these MDC keys can clobber values previously set by the caller and leave the ThreadContext in a different state than it was found (removed instead of restored). Save the prior values (if any) before put, and restore them in finally instead of blindly remove-ing, to be a good citizen and prevent bleed/loss across nested logging contexts. src/main/java/org/opensearch/security/auditlog/sink/Log4JSink.java [53-68] public boolean doStore(final AuditMessage msg) { if (enabled) { + final String prevCategory = ThreadContext.get("audit_category"); + final String prevAction = ThreadContext.get("audit_action"); + final String prevUser = ThreadContext.get("audit_user"); + final String prevRequestType = ThreadContext.get("audit_request_type"); try { - // Push audit attributes into Log4j MDC so operators can use - // RoutingAppender with $${ctx:audit_category} etc. to split logs. - // Values are sanitized for safe use in filenames (colons in action names etc.) ThreadContext.put("audit_category", sanitizeForMdc(msg.getCategory() != null ? msg.getCategory().name() : "UNKNOWN")); ThreadContext.put("audit_action", sanitizeForMdc(msg.getPrivilege())); ThreadContext.put("audit_user", sanitizeForMdc(msg.getEffectiveUser())); ThreadContext.put("audit_request_type", sanitizeForMdc(msg.getRequestType())); msg.toJsonSplitIndices(maximumIndexCharactersPerMessage).forEach(message -> auditLogger.log(logLevel, message)); } finally { - ThreadContext.remove("audit_category"); - ThreadContext.remove("audit_action"); - ThreadContext.remove("audit_user"); - ThreadContext.remove("audit_request_type"); + restore("audit_category", prevCategory); + restore("audit_action", prevAction); + restore("audit_user", prevUser); + restore("audit_request_type", prevRequestType); } } return true; } +private static void restore(String key, String prev) { + if (prev == null) { + ThreadContext.remove(key); + } else { + ThreadContext.put(key, prev); + } +} + Suggestion importance[1-10]: 6 __ Why: Preserving and restoring prior MDC values is a valid defensive improvement to avoid clobbering caller-set context, though in practice these keys are documented as owned by the audit sink, limiting impact.	Low
Security	Harden filename sanitization against control chars The sanitizer does not strip control characters, path traversal sequences (., ..), or leading/trailing whitespace/dots, which are also unsafe or ambiguous for filenames — especially on Windows. Consider also stripping ASCII control characters and rejecting ./.. values to avoid RoutingAppender writing to unintended paths. src/main/java/org/opensearch/security/auditlog/sink/Log4JSink.java [79-84] private static String sanitizeForMdc(String value) { if (value == null || value.isEmpty()) { return "unknown"; } - return UNSAFE_FILENAME_CHARS.matcher(value).replaceAll("_"); + String sanitized = UNSAFE_FILENAME_CHARS.matcher(value).replaceAll("_"); + // Strip ASCII control characters + sanitized = sanitized.replaceAll("[\\p{Cntrl}]", "_"); + if (sanitized.equals(".") || sanitized.equals("..") || sanitized.trim().isEmpty()) { + return "unknown"; + } + return sanitized; } Suggestion importance[1-10]: 5 __ Why: Handling control characters and ./.. edge cases adds defense-in-depth for filename safety, but the values come from internal audit fields, so risk is moderate.	Low

[DarshitChanpura]

Nice, focused feature — the finally-based cleanup, removing only the sink's own four keys (not clearMap()), and skipping work when the sink is disabled are all the right calls, and pre-compiling the Pattern is good.

Two test-quality issues below are worth fixing before merge — I pulled the branch and empirically confirmed both (details in the inline comments). There's also a security note on routing keys, plus some optional polish. Overall a useful addition.

[DarshitChanpura]

Swallowed in-thread assertions (verified empirically). These assertThat(...) calls run on the spawned worker thread, so an AssertionError here never propagates to the JUnit thread and can't fail the test.

I confirmed this on the branch: inserting a guaranteed-false assertion into this worker-thread body still passes the test. So the in-thread cleanup checks (here and in thread2) are effectively dead.

The post-join() value assertions below do run, so value-bleed between events is covered — it's only the per-worker-thread cleanup that goes unverified. Suggestion: capture failures into a shared AtomicReference<Throwable> (or synchronized list) inside each thread and assert it's empty after join().

[Taiwo435]

Addressed

[DarshitChanpura]

This test never actually throws (verified empirically). The name is testMdcCleanedUpEvenWhenLoggingThrows, but nothing on this path throws — it's a normal multi-index store. I confirmed on the branch by asserting that doStore throws: that assertion fails, so the finally exception path (the whole point of the test) is never exercised.

Suggestion: inject a failing appender/layout so auditLogger.log(...) actually throws and then assert the MDC keys are still removed, or rename to something like testMdcCleanedUpAfterMultiChunkStore to match what it verifies.

[Taiwo435]

Addressed

[DarshitChanpura]

Security note — routing key safety. audit_user is unbounded and attacker-influenceable, so using it as a RoutingAppender key ($${ctx:audit_user}) lets a caller mint arbitrarily many distinct filenames -> inode/disk exhaustion. The example in the PR description safely routes on audit_category (a bounded enum); worth calling out explicitly in the docs/PR that audit_user should not be used as an unbounded routing key. (audit_action/privilege and audit_request_type are effectively bounded — action strings and request class names — so those are lower risk.)

[Taiwo435]

Addressed

[DarshitChanpura]

Fallback casing is inconsistent. audit_category falls back to "UNKNOWN" (uppercase) here, while the other three fall back to "unknown" (lowercase, via sanitizeForMdc). An operator keying a route on the "missing" value has to remember both spellings. Passing a null category name straight through sanitizeForMdc (which already returns "unknown") would unify it.

Minor, related: the four key names are string literals duplicated across the put here and the remove calls in the finally. Extracting private static final String constants removes any risk of put/remove drift and documents the owned key set in one place.

[Taiwo435]

Addressed

[DarshitChanpura]

Optional — unconditional per-event cost. This runs 4x put + 4x regex + 4x remove on every audit event regardless of whether any operator has configured a RoutingAppender. On high-volume clusters that's overhead nobody using the default config benefits from. Consider gating enrichment behind a setting (e.g. ...log4j.enable_mdc_routing, default off) so those clusters pay nothing.

[Taiwo435]

Addressed

[DarshitChanpura]

Optional — control chars not sanitized. The regex strips filename-dangerous chars but not control characters (\n, \r). Since audit_user can be attacker-influenced, a value containing newlines flows through unchanged; if an operator surfaces %X{audit_user} in a pattern layout this becomes a log-forging vector. Low severity for a routing key, but consider stripping control characters too.

[Taiwo435]

Addressed

[github-actions]

PR Code Analyzer ❗

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

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/auditlog/sink/Log4JSink.java	64	medium	The sanitizeForMdc method does not strip the dot character '.', which means attacker-controlled MDC values (e.g., audit_user) could contain '..' sequences. If an operator ignores the documented warning and configures RoutingAppender to use ${ctx:audit_user} in a file path, this enables path traversal outside the intended log directory. The warning comment acknowledges the risk but does not enforce it at the code level.
src/main/java/org/opensearch/security/auditlog/sink/Log4JSink.java	66	low	The UNSAFE_MDC_CHARS regex preserves space characters in sanitized MDC values (confirmed by the testControlCharactersStrippedFromMdc test: 'fake-log-entry_ injected' retains a leading space). Spaces in MDC values used in Log4j pattern layouts or RoutingAppender configurations can cause unexpected tokenization or format mismatches, though this is unlikely to be exploitable as a standalone issue.

The table above displays the top 10 most important findings.

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

---

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]

Persistent review updated to latest commit c9e6e8d

[codecov]

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.39%. Comparing base (e71c4b1) to head (c9e6e8d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...g/opensearch/security/auditlog/sink/Log4JSink.java	88.23%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6349      +/-   ##
==========================================
- Coverage   75.45%   75.39%   -0.06%     
==========================================
  Files         456      456              
  Lines       29924    29941      +17     
  Branches     4530     4533       +3     
==========================================
- Hits        22578    22575       -3     
- Misses       5251     5269      +18     
- Partials     2095     2097       +2     

Files with missing lines	Coverage Δ
...g/opensearch/security/auditlog/sink/Log4JSink.java	89.28% <88.23%> (+5.95%)	⬆️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Persistent review updated to latest commit 26cbb0f