[core.logging] Add RewriteAppender for filtering LogMeta. #91492

lukeelmers · 2021-02-16T14:53:20Z

Summary

Implements a RewriteAppender for the purposes of filtering / manipulating a LogRecord, as described in the original issue. The approach here is based on log4j2's RewriteAppender.

Each rewrite appender uses a designated policy which describes what part of the LogRecord to manipulate. This PR starts with a single meta policy for modifying LogMeta. The policy has a mode which can either be remove or update, and takes in a list of paths in the LogMeta object which are to be modified.

For the time being we are intentionally keeping this undocumented in the public docs in case we choose to make future changes, as the legacy filtering behavior was also undocumented.

Usage

Here's a full example configuration which logs both to the console and a log file, censoring the cookie request header from the http response logs:

logging:
  appenders:
    console:
      type: console
      layout:
        type: pattern
        highlight: true
        pattern: "[%date][%level][%logger] %message %meta"
    file:
      type: file
      fileName: ./kibana.log
      layout:
        type: json
    censor:
      type: rewrite
      appenders: [console, file]
      policy:
        type: meta
        mode: update
        properties:
          - path: "http.request.headers.cookie"
            value: "[REDACTED]"
  loggers:
    - name: http.server.response
      appenders: [censor]
      level: debug

The remove mode does not take a value as it simply deletes the provided path:

logging:
  appenders:
    censor:
      type: rewrite
      appenders: [console]
      policy:
        type: meta
        mode: remove
        properties:
          - path: "some.path.to.delete"

elasticmachine · 2021-02-17T05:14:41Z

Pinging @elastic/kibana-core (Team:Core)

packages/kbn-logging/src/appenders.ts

lukeelmers · 2021-02-17T03:53:42Z

src/core/server/logging/logging_config.ts

Currently all sensitive headers are filtered in the http response logging, and the filters cannot be disabled.

In this PR, I removed the filtering code from http, and here have "wrapped" our default console appender in a rewrite appender that performs the same logic.

However, there is one main caveat: While this ensures nothing sensitive is logged with a default configuration, the protection is gone as soon as you customize your config to use something other than the console appender. I tried to think of another way to enforce this more widely while also making it configurable, and I didn't come up with any great solutions that didn't involve creating another standalone config option. Open to suggestions though!

While this ensures nothing sensitive is logged with a default configuration, the protection is gone as soon as you customize your config to use something other than the console appender. I tried to think of another way to enforce this more widely while also making it configurable, and I didn't come up with any great solutions that didn't involve creating another standalone config option.

That might be a reason not to use RewriteAppender internally to filter out sensitive data (but provide it as public API). We can either implement a custom filtering logic always applicable to http.request / http.response contexts. Or filter out sensitive properties when creating meta object https://github.com/elastic/kibana/blob/master/src/core/server/http/logging/get_response_log.ts

How it works now in the Legacy platform? Does it always censor the sensitive content?

Or filter out sensitive properties when creating meta object

@restrry Yeah, that's the approach we take now, the only issue is that it can't be disabled, and IIRC this was one of the reasons we wanted a rewrite appender to begin with, for the case where we have a reason to allow sensitive headers through for support/debugging purposes.

That's why the only alternative that comes to mind is a new config option to disable filters, which would then be used inside the http response logger getResponseLog. But in the spirit of having One Way Of Doing Things (TM), it'd be nice to use the existing config if we can.

Actually, as I think about it, this is an issue that would possibly be solved by our discussion around having logger-specific default appender configurations.

How it works now in the Legacy platform? Does it always censor the sensitive content?

It filters them out by default (just authorization and cookie), but you can disable that using the (undocumented) logging.filter settings - e.g. logging.filter.cookie=none to remove filter from that header in hapi/good.

kibana/packages/kbn-legacy-logging/src/get_logging_config.ts

Lines 53 to 60 in 72670eb

// I'm adding the default here because if you add another filter

// using the commandline it will remove authorization. I want users

// to have to explicitly set --logging.filter.authorization=none or

// --logging.filter.cookie=none to have it show up in the logs.

filter: _.defaults(config.filter, {

authorization: 'remove',

cookie: 'remove',

}),

Yeah, that's the approach we take now, the only issue is that it can't be disabled, and IIRC this was one of the reasons we wanted a rewrite appender to begin with, for the case where we have a reason to allow sensitive headers through for support/debugging purposes.

I'd say it's nice to have. It is better to not add such a feature than to add it but with the possibility of sensitive data leakage due to incorrect configuration:

While this ensures nothing sensitive is logged with a default configuration, the protection is gone as soon as you customize your config to use something other than the console appender.

Actually, as I think about it, this is an issue that would possibly be solved by our discussion around having logger-specific default appender configurations.

Are you implying that we can hide the Meta object from the logs? The pattern does not apply to json layout.

I'd say it's nice to have. It is better to not add such a feature than to add it but with the possibility of sensitive data leakage due to incorrect configuration

Fair point, so do you think we should:

Keep the hardcoded filtering of sensitive header fields, meaning we can't turn those off for debugging purposes

Go with option 1 but also introduce a new config flag to disable this for debugging purposes (e.g. logging.filterSensitiveHttpHeaders or whatever). The flag would default to true but could be disabled if someone wanted to use a custom rewrite appender.

Some other option I'm not thinking of?

Ideally I'd like to not introduce another config flag, but I'm also unsure of how we could otherwise make it possible to disable the filtering.

Are you implying that we can hide the Meta object from the logs?

No, sorry that was a bit vague. I meant if we went the route of having predefined default appender configurations on a per-logger basis (similar to the ES config file), then this would be another use case for those and would solve this problem. But if we go the simpler route of removing the meta from the pattern by default, that won't help us in this case.

Keep the hardcoded filtering of sensitive header fields, meaning we can't turn those off for debugging purposes

As for me, it's an acceptable trade-off. Any other opinions? @joshdover @pgayvallet @TinaHeiligers

Yeah the more I think about it, the hardcoded approach feels like the safest option for the time being. And then if we were to consider an enhancement in 8.x to support logger-specific appender defaults as discussed in #90457, it would allow us to move these settings there anyway.

src/core/server/logging/logging_config.ts

docs/development/core/server/kibana-plugin-core-server.savedobjectsfindresult.sort.md

src/core/server/logging/appenders/rewrite/policies/meta/meta_policy.ts

TinaHeiligers · 2021-02-17T15:50:08Z

@lukeelmers

For the time being we are intentionally keeping this undocumented in case we choose to make future changes, as the legacy filtering behavior was also undocumented.

While I understand that the initial implementation is pretty much a MVP, IMHO, I must disagree with not documenting the RewriteAppender in at least the dev docs. Being able to censor/filter sensitive data is a powerful feature, even at an MVP stage and I envisage it leading to some head scratching down the line when folks are using logs for debugging.

The fact the filtering in the LP isn't documented shouldn't hold us back from doing the right thing, at least at the dev level.

lukeelmers · 2021-02-17T17:02:14Z

@TinaHeiligers I'm perfectly comfortable documenting this for devs -- I think the only reason we previously decided to make this internal was because the legacy filtering is also undocumented and we wanted to reserve the right to make changes in the future (or decide whether we want to support this functionality long-term at all).

Since we don't yet have proper docs on the new logging system, I will just add a mention to the README for now and clarify that it's experimental/internal.

mshustov · 2021-02-17T15:10:51Z

src/core/server/logging/appenders/rewrite/policies/index.ts

+
+export const createRewritePolicy = (config: RewritePolicyConfig): RewritePolicy => {
+  switch (config.type) {
+    case 'meta':


I can't find MetaRewritePolicy in log4j http://logging.apache.org/log4j/2.x/manual/appenders.html#RewritePolicy
Do we call so because we apply a policy to Meta object only?

I think MetaRewritePolicy is a combined modification of log4j's MapRewritePolicy and PropertiesRewritePolicy where we're using the type field and the properties field rather than the keyValuePair in MetaRewritePolicy and we're allowing fields to be added too ( what PropertiesRewritePolicy does). And, yes, the policy is applied to the meta property. It's hard to tell though because our implementation isn't a 1-to-1 with log4j.

Do we call so because we apply a policy to Meta object only?

Yeah exactly, they have a map and properties rewrite policy, so I called this meta as it is only scoped to the LogMeta. With this pattern, in the future we could have something like a message or level policy that will rewrite those items specifically.

src/core/server/logging/appenders/rewrite/policies/index.ts

src/core/server/logging/appenders/rewrite/policies/meta/meta_policy.test.ts

src/core/server/logging/appenders/rewrite/policies/meta/meta_policy.ts

src/core/server/logging/logging_config.ts

mshustov · 2021-02-17T16:42:53Z

src/core/server/logging/logging_config.ts

While this ensures nothing sensitive is logged with a default configuration, the protection is gone as soon as you customize your config to use something other than the console appender. I tried to think of another way to enforce this more widely while also making it configurable, and I didn't come up with any great solutions that didn't involve creating another standalone config option.

That might be a reason not to use RewriteAppender internally to filter out sensitive data (but provide it as public API). We can either implement a custom filtering logic always applicable to http.request / http.response contexts. Or filter out sensitive properties when creating meta object https://github.com/elastic/kibana/blob/master/src/core/server/http/logging/get_response_log.ts

How it works now in the Legacy platform? Does it always censor the sensitive content?

src/core/server/logging/appenders/rewrite/rewrite_appender.ts

src/core/server/logging/logging_system.ts

packages/kbn-logging/src/appenders.ts

TinaHeiligers

I went through the initial implementation and it seems to make sense. I tried to pull the code and run it locally but keep hitting a config error on start up (probably local).

I'd really like to see something in the README on the final implementation though.

src/core/server/logging/logging_config.ts

TinaHeiligers · 2021-02-17T20:09:06Z

src/core/server/logging/appenders/rewrite/policies/index.ts

+
+export const createRewritePolicy = (config: RewritePolicyConfig): RewritePolicy => {
+  switch (config.type) {
+    case 'meta':


I think MetaRewritePolicy is a combined modification of log4j's MapRewritePolicy and PropertiesRewritePolicy where we're using the type field and the properties field rather than the keyValuePair in MetaRewritePolicy and we're allowing fields to be added too ( what PropertiesRewritePolicy does). And, yes, the policy is applied to the meta property. It's hard to tell though because our implementation isn't a 1-to-1 with log4j.

src/core/server/logging/appenders/rewrite/policies/meta/meta_policy.ts

lukeelmers · 2021-02-18T03:41:03Z

I tried to pull the code and run it locally but keep hitting a config error on start up (probably local).

I'll update the README, but the example config I gave in the PR description should be working without an error if you want to play around with it.

TinaHeiligers · 2021-02-18T14:54:51Z

... but the example config I gave in the PR description should be working without an error if you want to play around with it.

A direct copy-paste throws the following:

/Users/christianeheiligers/Projects/kibana/packages/kbn-config-schema/target/out/types/literal_type.js:17
    handleError(type, { value, valids: [expectedValue] }) {
                        ^
TypeError: Cannot destructure property 'value' of 'undefined' as it is undefined.

Changing kind to type gets rid of that but also doesn't work directly:

/Users/christianeheiligers/Projects/kibana/packages/kbn-config-schema/target/out/types/type.js:51
            throw new errors_1.ValidationError(error, namespace);
                  ^
Error: [config validation of [logging].appenders.file]: types that failed validation:
- [config validation of [logging].0.type]: expected value to equal [console]
- [config validation of [logging].1.fileName]: expected value of type [string] but got [undefined]
- [config validation of [logging].2.type]: expected value to equal [legacy-appender]
- [config validation of [logging].3.type]: expected value to equal [rewrite]
- [config validation of [logging].4.type]: expected value to equal [rolling-file]

Removing the file appender solves that. What am I missing in the config?

lukeelmers · 2021-02-18T15:11:03Z

Ah sorry @TinaHeiligers, I added that example before #90764 was merged and some of the properties were renamed. I've updated now, let me know if you hit any issues.

…og4j.

pgayvallet

Few NITS, overall looking great

src/core/server/http/logging/get_response_log.ts

src/core/server/logging/appenders/rewrite/policies/meta/meta_policy.ts

src/core/server/logging/README.md

src/core/server/logging/appenders/rewrite/rewrite_appender.ts

src/core/server/logging/logging_system.ts

mshustov · 2021-02-23T09:58:51Z

src/core/server/http/logging/get_response_log.ts

-      result[key] = FORBIDDEN_HEADERS.includes(key) ? REDACTED_HEADER_TEXT : headers[key];
+      result[key] = redactSensitiveHeaders(
+        key,
+        Array.isArray(headers[key]) ? [...headers[key]] : headers[key]


optional: It's good we enforce immutability, but we pay the extra cost when cloning the array. IMO we can assume that no-one mutates the array.

src/core/server/logging/logging_config.ts

lukeelmers · 2021-02-23T22:58:45Z

@elasticmachine merge upstream

lukeelmers · 2021-02-24T20:14:53Z

@elasticmachine merge upstream

TinaHeiligers

This is a great addition to our new logging system and even though we're not using the implementation in the default config right now, it's going to be super useful when we get around to #92082.
LGTM!

kibanamachine · 2021-02-24T22:25:31Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #108933 failed 102d490
💚 Build #108932 succeeded 3d2a945
💚 Build #108584 succeeded dcee4b9
💚 Build #108243 succeeded e0f4c58
💚 Build #108086 succeeded a6632ee

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @lukeelmers

)

kibanamachine · 2021-02-24T22:28:14Z

💚 Backport successful

✅ 7.x / #92724

Successful backport PRs will be merged automatically after passing CI.

…92724) Co-authored-by: Luke Elmers <luke.elmers@elastic.co>

* master: (38 commits) Fixes Cypress flake by adding pipe, click, and should (elastic#92762) [Discover] Fix filtering selected sidebar fields (elastic#91828) [ML] Fixes positions of calendar arrow buttons in start datafeed modal (elastic#92625) [dev/build_ts_refs] check that commit in outDirs matches mergeBase (elastic#92513) add dep on `@kbn/config` so it is built first [Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn (elastic#90481) [Lens] Fix Workspace hidden when using Safari (elastic#92616) [Lens] Fixes vertical alignment validation messages (elastic#91878) forbid x-elastic-product-origin header in elasticsearch configuration (elastic#92359) [Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules (elastic#92081) [ILM][Accessibility] Added A11y test for ILM new policy form. (elastic#92570) [Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values (elastic#90634) Automatically generated Api documentation (elastic#86232) Increase index pattern select limit to 1000 (elastic#92093) [core.logging] Add RewriteAppender for filtering LogMeta. (elastic#91492) [Security Solution][Detection Rules] Update prebuilt rule threats to match schema (elastic#92281) [Security Solutions][Detection Engine] Fixes bug with not being able to duplicate indicator matches (elastic#92565) [Dashboard] Export appropriate references from byValue panels (elastic#91567) [Upgrade Assistant] Align code between branches (elastic#91862) [Security Solution][Case] Fix alerts push (elastic#91638) ...

lukeelmers self-assigned this Feb 16, 2021

lukeelmers force-pushed the feat/rewrite-appender branch from 68e0019 to 4372efa Compare February 17, 2021 03:04

lukeelmers added v8.0.0 Feature:Logging release_note:skip Skip the PR/issue when compiling release notes labels Feb 17, 2021

lukeelmers added 2 commits February 16, 2021 20:56

Add RewriteAppender with a 'meta' rewrite policy.

752bdf1

Add unit tests.

71e46ce

lukeelmers force-pushed the feat/rewrite-appender branch from 4372efa to 0d7b5c9 Compare February 17, 2021 03:56

lukeelmers added 2 commits February 16, 2021 21:24

Filter sensitive headers using rewrite appender.

e1c6402

Update generated docs.

ae5facc

lukeelmers force-pushed the feat/rewrite-appender branch from 0d7b5c9 to ae5facc Compare February 17, 2021 04:24

lukeelmers marked this pull request as ready for review February 17, 2021 05:14

lukeelmers requested a review from a team as a code owner February 17, 2021 05:14

lukeelmers added the Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// label Feb 17, 2021

lukeelmers requested a review from mshustov February 17, 2021 05:14

lukeelmers commented Feb 17, 2021

View reviewed changes

lukeelmers requested a review from TinaHeiligers February 17, 2021 05:16

mshustov reviewed Feb 17, 2021

View reviewed changes

lukeelmers added v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Feb 17, 2021

TinaHeiligers reviewed Feb 17, 2021

View reviewed changes

lukeelmers added 3 commits February 18, 2021 15:22

Rename policy.transform to policy.rewrite for consistency with log4j.

03a0397

Address initial feedback.

61d4450

Switch from appenders.update to appenders.addAppender to align with l…

0dc0afc

…og4j.

lukeelmers added 3 commits February 21, 2021 14:21

Re-add hardcoded headers filtering to http logs.

0516db3

Use fixed timestamp in meta policy tests.

c777305

Clean up documentation.

e0f4c58

pgayvallet reviewed Feb 22, 2021

View reviewed changes

lukeelmers added 5 commits February 22, 2021 13:38

Prevent mutating array headers.

11b5512

Extract inline types from MetaRewritePolicy.

b46e404

Clean up unknown appender error message.

f6ae2ea

Resolve merge conflicts.

353d8e5

Merge branch 'master' into feat/rewrite-appender

dcee4b9

mshustov reviewed Feb 23, 2021

View reviewed changes

mshustov approved these changes Feb 23, 2021

View reviewed changes

mshustov reviewed Feb 23, 2021

View reviewed changes

src/core/server/logging/logging_config.ts Outdated Show resolved Hide resolved

lukeelmers added 2 commits February 23, 2021 15:37

Remove MetaRewritePolicy 'add' mode.

f9f28dd

Remove default rewrite-appender.

3d2a945

lukeelmers mentioned this pull request Feb 23, 2021

Implement filtering mechanism in the Logging service #57547

Closed

Merge branch 'master' into feat/rewrite-appender

102d490

kibanamachine and others added 2 commits February 24, 2021 15:15

Merge branch 'master' into feat/rewrite-appender

aa5d21e

Fix logging system jest tests.

fb23cb1

TinaHeiligers approved these changes Feb 24, 2021

View reviewed changes

lukeelmers enabled auto-merge (squash) February 24, 2021 21:20

lukeelmers merged commit 0280d5a into elastic:master Feb 24, 2021

kibanamachine mentioned this pull request Feb 24, 2021

[7.x] [core.logging] Add RewriteAppender for filtering LogMeta. (#91492) #92724

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 24, 2021

[core.logging] Add RewriteAppender for filtering LogMeta. (elastic#91492

39ec5bf

)

lukeelmers deleted the feat/rewrite-appender branch February 24, 2021 23:25

kibanamachine added a commit that referenced this pull request Feb 25, 2021

[core.logging] Add RewriteAppender for filtering LogMeta. (#91492) (#…

ecdef59

…92724) Co-authored-by: Luke Elmers <luke.elmers@elastic.co>

	// I'm adding the default here because if you add another filter
	// using the commandline it will remove authorization. I want users
	// to have to explicitly set --logging.filter.authorization=none or
	// --logging.filter.cookie=none to have it show up in the logs.
	filter: _.defaults(config.filter, {
	authorization: 'remove',
	cookie: 'remove',
	}),

[core.logging] Add RewriteAppender for filtering LogMeta. #91492

[core.logging] Add RewriteAppender for filtering LogMeta. #91492

Uh oh!

Conversation

lukeelmers commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Usage

Uh oh!

elasticmachine commented Feb 17, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TinaHeiligers commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukeelmers commented Feb 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukeelmers commented Feb 18, 2021

Uh oh!

TinaHeiligers commented Feb 18, 2021

Uh oh!

lukeelmers commented Feb 18, 2021

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mshustov Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukeelmers commented Feb 23, 2021

Uh oh!

lukeelmers commented Feb 16, 2021 •

edited

Loading

mshustov Feb 19, 2021 •

edited

Loading

TinaHeiligers commented Feb 17, 2021 •

edited

Loading

mshustov Feb 23, 2021 •

edited

Loading