feat: Allow some paths to be redacted and some to be removed #84

ConradLang · 2023-08-29T00:50:08Z

Purpose

Pino has a limitation where you can either redact or remove specified redact.paths by setting redact.remove to true or false.

In some cases, it's reasonable to want to redact some properties e.g. headers.authorization so you know that you have an authorization header but in other cases, the header, e.g. x-envoy-peer-metadata only contributes to increasing the log message size and therefore cost.

To reduce costs, we should be able to remove the properties that are not relevant for diagnostic purposes.

Approach

A removePaths logger option has been added to allow you to redact some paths and remove other paths.

Example:

const logger = createLogger({
  name: 'my-app',
  redact: {
    paths: ['req.headers["x-redact-this"]'],
    removePaths: ['req.headers["x-remove-this"]'],
  },
});

Default paths can be added for removal if needed (currently an empty list):

logger/src/redact/index.ts

Line 6 in 9c16ede

export const defaultRemovePaths: string[] = [];

If you'd prefer not to remove the default paths, you can opt out by setting ignoreDefaultRemovePaths to true.

Example:

const logger = createLogger({
  name: 'my-app',
  redact: {
    paths: ['req.headers["x-redact-this"]'],
    removePaths: ['req.headers["x-remove-this"]'],
    ignoreDefaultRemovePaths: true,
  },
});

Notes

Reverted Vendor redactOptions #42 to allow redactOptions to be extended.
Running yarn build succeeds without errors.
Added an expectation in the test for properties not to be present.

Issues

Resolves #83

72636c · 2023-08-29T02:05:21Z

src/index.ts

  opts.redact = redact.addDefaultRedactPathStrings(opts.redact);
+  opts.redact = redact.addRemovePathsCensor(opts.redact);


Would a better name be configureCensor?

I'm a bit sceptical of this chaining of two standalone redaction functions. Is it obvious how they both modify opts.redact, and what guarantees that they evolve in a compatible, non-conflicting manner?

At minimum we will want tests that cover combinations of both, especially if we ponder adding default redaction config.

I've exported a configureRedact function that ensures the default redact paths are added before the censor is configured.

src/redact/index.ts

72636c · 2023-08-29T02:14:23Z

README.md

+  name: 'my-app',
+  redact: {
+    paths: ['req.headers["x-redact-this"]'],
+    removePaths: ['req.headers["x-remove-this"]'],


Are there sensible defaults that we should be thinking of bundling into this package, rather than expecting each team to manually review and add removePaths?

There are some x-envoy-* headers likely related to internal routing but I'm not sure if these should be blanket added for all services.

Also, being OSS, should we add default headers for removal or is it ok since it's a SEEK opinionated logger?

It's SEEK opinionated so I'd be fine with any configuration that is not sensitive, and I reckon headers for an OSS project like Envoy would be fine.

Do we have an example of a SEEK application that would be impaired by having x-envoy-* removed by default? We'd presumably allow them to override/disable the defaults if needed.

I've asked cloud platform about the x-envoy-* headers so we'll have to wait for an answer.

Ah apologies if I was vague here; I meant adding logic here to remove the headers from logs by default, not changing our infra to stop attaching the headers.

@72636c : I've just pushed a change that allows us to add default paths for removal.

Caveat: Since the defaultRemovePaths is empty, it isn't really tested, but you can pull this branch, add a path e.g. req.headers["x-envoy-peer-metadata"] and see the should remove default paths when ignoreDefaultRemovePaths is missing test result.
You might need to console.log({input, log}) in the testLog function to observe.

You can also ignoreDefaultRemovePaths if you don't want the default remove paths to be added.

I had to refactor the addDefaultRedactPathStrings (and rename to appendDefaultRedactAndRemove) to get this working.

Something I'm wondering is should we only override the censor when it is undefined with the caveat that the remove may not work proper?

Something I'm wondering is should we only override the censor when it is undefined with the caveat that the remove may not work proper?

I've updated this so that the input censor is used for redaction but removePaths properties are still removed.

src/redact/index.ts

72636c · 2023-08-29T03:02:56Z

src/redact/index.ts

+        censor: (_value, path) =>
+          redactMap[path.join('.')] ? '[Redacted]' : undefined,


This seems a bit dicey. How does the redaction package handle the difference between e.g.

{ a: { b: 'pii' } } { 'a.b': 'pii' }

I've resolved this issue by refactoring out the key builder and putting in a comprehensive test.

I've used a regular expression as I'm not sure of a better way to work out if the identifier name is out-of-spec.

ConradLang · 2023-08-30T02:11:37Z

src/redact/index.ts

@@ -21,3 +24,46 @@ export const addDefaultRedactPathStrings = (
  }
  return redact;


@72636c , @etaoins : Seems like the default redact paths aren't added when redact is an object.

I feel like this may be an issue separate to this PR.

I've fixed this in Feat: Enable configuring default remove paths

Pino has a limitation where you can either redact or remove specified `redact.paths` by setting `redact.remove` to `true` or `false`. A logger option has been added to allow you to redact some paths and remove other paths. Example: ```typescript const logger = createLogger({ name: 'my-app', redact: { paths: ['req.headers["x-redact-this"]'], removePaths: ['req.headers["x-remove-this"]'], }, }); ```

Property access should be more performant than string compare.

Renamed to configureRedactCensor.

Default redact paths should always be added before configuring the censor.

Also corrected `ExtendedRedact` definition as `removePaths` should only have been on the pino `redactOptions` type.

kosanna · 2023-09-26T21:23:23Z

Will convert to draft while discussions are in progress

ConradLang · 2023-10-09T04:53:27Z

Closing this PR as #92 was more appropriate and the concensus from the upstream repo (pinojs/pino#1815) was that a custom serializer was the best option.

To address #83, we could consider adding guidance to the readme around usage of redact and the option to use a custom serializer to remove specific properties with some examples.

ConradLang requested a review from 72636c as a code owner August 29, 2023 00:50

ConradLang self-assigned this Aug 29, 2023

ConradLang requested review from kosanna and runk as code owners August 29, 2023 00:50

ConradLang marked this pull request as draft August 29, 2023 00:50

ConradLang force-pushed the add-remove-paths branch from a9f4d13 to b96df3e Compare August 29, 2023 01:23

ConradLang marked this pull request as ready for review August 29, 2023 01:29

ConradLang changed the title ~~feat: Redact or remove paths~~ feat: Allow some paths to be redacted and some to be removed Aug 29, 2023

72636c reviewed Aug 29, 2023

View reviewed changes

src/redact/index.ts Show resolved Hide resolved

72636c reviewed Aug 29, 2023

View reviewed changes

ConradLang force-pushed the add-remove-paths branch from 606d95c to b3605ab Compare August 30, 2023 01:50

ConradLang commented Aug 30, 2023

View reviewed changes

ConradLang requested review from etaoins and 72636c August 30, 2023 02:11

ConradLang force-pushed the add-remove-paths branch from 92ce611 to 44e4af3 Compare September 20, 2023 06:13

ConradLang added 8 commits September 20, 2023 16:13

refactor: Use map instead of includes

1470729

Property access should be more performant than string compare.

refactor: Handle non-standard property names

19b77a5

refactor: Rename addRemovePathsCensor

150481e

Renamed to configureRedactCensor.

refactor: Use set instead of map

aa16fcb

refactor: Orchestrate redact configuration

91a9f13

Default redact paths should always be added before configuring the censor.

Feat: Enable configuring default remove paths

8bcb486

Also corrected `ExtendedRedact` definition as `removePaths` should only have been on the pino `redactOptions` type.

fix: Input censor is always preserved for redact

a4cc18b

ConradLang force-pushed the add-remove-paths branch from 44e4af3 to a4cc18b Compare September 20, 2023 06:14

ConradLang mentioned this pull request Sep 25, 2023

Feature Request: Capability to redact some paths as well as remove other paths pinojs/pino#1815

Open

kosanna marked this pull request as draft September 26, 2023 21:23

ConradLang mentioned this pull request Oct 4, 2023

feat: Omit specific headers by default #92

Merged

ConradLang closed this Oct 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow some paths to be redacted and some to be removed #84

feat: Allow some paths to be redacted and some to be removed #84

ConradLang commented Aug 29, 2023 •

edited

Loading

72636c Aug 29, 2023

ConradLang Aug 29, 2023

72636c Aug 29, 2023

ConradLang Aug 30, 2023

72636c Aug 29, 2023

ConradLang Aug 29, 2023 •

edited

Loading

72636c Aug 29, 2023

ConradLang Aug 30, 2023

72636c Aug 30, 2023

ConradLang Aug 30, 2023

ConradLang Aug 31, 2023

72636c Aug 29, 2023

ConradLang Aug 30, 2023

ConradLang Aug 30, 2023

ConradLang Aug 30, 2023

kosanna commented Sep 26, 2023

ConradLang commented Oct 9, 2023

		opts.redact = redact.addDefaultRedactPathStrings(opts.redact);
		opts.redact = redact.addRemovePathsCensor(opts.redact);

		censor: (_value, path) =>
		redactMap[path.join('.')] ? '[Redacted]' : undefined,

		@@ -21,3 +24,46 @@ export const addDefaultRedactPathStrings = (
		}
		return redact;

feat: Allow some paths to be redacted and some to be removed #84

feat: Allow some paths to be redacted and some to be removed #84

Conversation

ConradLang commented Aug 29, 2023 • edited Loading

Purpose

Approach

Notes

Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConradLang Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kosanna commented Sep 26, 2023

ConradLang commented Oct 9, 2023

ConradLang commented Aug 29, 2023 •

edited

Loading

ConradLang Aug 29, 2023 •

edited

Loading