pino icon indicating copy to clipboard operation
pino copied to clipboard

Feature Request: Make paths parameter optional for redaction

Open seprich opened this issue 4 years ago • 10 comments

Currently the paths parameter under the redact object is mandatory. This creates a problem in many applications where the developers cannot guarantee where in the object tree a sensitive information may appear. For example if application code catches and logs an error there can be many places from where errors are thrown and sensitive information may appear under unpredictable object paths which we cannot know before deploying to production.

On the other hand if we use a function for the censor parameter, it could analyze the value e.g. by matching with regexp and could return either the uncensored original value or some '[redacted]' string if only this function could be applied to all leaf nodes of the object tree without having to use that paths parameter guestimate.

Current redact system allows sensitive data to slip into logs too easily and for applications where security considerations are paramount this may be a "deal breaker" for using this lib.

seprich avatar May 31 '21 07:05 seprich

Thanks for the long explanation @seprich! I would love to implement such feature, however it would cause a massive slowdown. As soon as https://github.com/pinojs/pino/pull/1003 lands and v7 is released, you'll be able to create full transport without slowing down the main thread. I think we should be able to provide the plumbing for you (or anybody else) that wishes to implement such logic.

mcollina avatar May 31 '21 08:05 mcollina

Isn't transport a consumer though. Because redacting is more of a "middleware" layer. Combining redacting with e.g. pino-cloudwatch or with any actual consumer should be possible, right ? Or the new v7 transport is stackable ?

seprich avatar May 31 '21 08:05 seprich

Or the new v7 transport is stackable ?

Not as they are in the PR but I think we might add some API to do that as it seems a valid usecase. Essentially we need to add a few more steps on the pipe. We should add a better dev experience for this.

Essentially we should model:

node server.js | deep-redact-filter | destination

mcollina avatar May 31 '21 10:05 mcollina

Got me thinking.. actually the deep-redact-filter is just a special case of a transformer. If we could have an API to add a list of transformers:

node server.js | transformer_1 | ... | transformer_N | destination

where each transformer is just a function which takes the top-level content object and returns a modified object back. That way application developers could make any kind of filters and manglers they need in the context of their application - redacting sensitive contents being just one special case.

seprich avatar May 31 '21 14:05 seprich

I think @davidmarkclements would be highly pleased by that view. That's what I have in mind.

mcollina avatar May 31 '21 15:05 mcollina

Indeed I am - this is where I want to take the new transport system.

An additional API on top of this plumbing that I think would be really cool is adding a an option to the redact option that makes the redaction occur in a separate worker thread (e.g. it would unshift the ready made redact filter with specified options to the front of the pipeline)

davidmarkclements avatar May 31 '21 16:05 davidmarkclements

const logger = require('pino')({
  redact: {
    paths: ['key', 'path.to.key', 'stuff.thats[*].secret', 'path["with-hyphen"]'],
    censor (...blah) { return etc },
    transport: true
  }
})

davidmarkclements avatar May 31 '21 16:05 davidmarkclements

I understand that v7 is still fairly fresh, but wanted to check if this feature is in the works by anyone (I'm unable to contribute at this time, but am facing this same problem of not knowing the exact path to sensitive attributes). Alternatively if the '*' wildcard could be made to specify an attribute at any depth in the object, that would also work, but I'm guessing that affects the performance considerably.

CompleteScone avatar Oct 28 '21 17:10 CompleteScone

This would need to be championed by someone with a vested interest in it. Either by contributing the code themselves, or hiring someone to do it. Otherwise, it's going to sit until someone has the time and interest.

jsumners avatar Oct 28 '21 19:10 jsumners

That makes sense.

As a note for others, I ended up passing a custom formatter for the log that loops through all object keys recursively, compares to a list of attribute names to redact, and then redacts them. This would potentially have performance issues with large objects, and assumes any instance of the attribute name needs to be redacted, but it works for my use case.

CompleteScone avatar Oct 28 '21 19:10 CompleteScone