Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Hiding sensitive request data when using convertReqRes #57

Closed
lancegliser opened this issue Feb 24, 2021 · 4 comments · Fixed by #161
Closed

Hiding sensitive request data when using convertReqRes #57

lancegliser opened this issue Feb 24, 2021 · 4 comments · Fixed by #161
Assignees
@trentm
Labels
8.1-candidate agent-nodejs Make available for APM Agents project planning. enhancement New feature or request lib:winston

Comments

@lancegliser
Copy link

I'm currently using { convertReqRes: true } to fill out the ECS fields. Lovely addition, thanks for it!
But, I do have one problem. I've got some headers that shouldn't slip through to my Kibana users, such as authorization: bearer {token}.

I've got my own formatter for Winston I'm using after this one, and I can see the data you've produced at info Symbol(message), but not message. I can't actually get into Symbol(message), as it's private by design. How should I get that data obscured or removed?
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Feb 24, 2021
@trentm trentm added enhancement New feature or request lib:winston labels Mar 17, 2021
trentm added a commit that referenced this issue Mar 18, 2021
@trentm
feat: separate winston ecsFormat to composable ecsFields and ecsStrin…
6fda9d3 
…gify

The current winston usage is a single ecsFormat formatter that handles
both (a) putting together the ECS fields and then (b) serializing that
to JSON (along with ecs-logging's preference for some serialized field
order).

    const log = winston.createLogger({
        format: ecsFormat(<options>),

Splitting these two steps into separate winston formatters allows for
some composability. For example, redacting sensitive info, as requested
in #57.

    const { ecsFields, ecsStringify } = require('@elastic/ecs-winston-format')
    const log = winston.createLogger({
        format: winston.format.combine(
            ecsFields(<options>),
            // Add a custom formatter to redact fields here.
            ecsStringify()
        ),

Fixes: #57
@trentm trentm mentioned this issue Mar 18, 2021
Closed
5 tasks
@trentm
Copy link
Member

trentm commented Mar 18, 2021

@lancegliser Hi, thanks for the ticket. Sorry for the delay.

I started a draft PR with a possible solution for this. Basically I split the ecsFormat functionality into two: (a) gather the ECS fields in one formatter, then (b) do the JSON stringification in another one. That allows one to use winston.format.combine() to place another Winston formatter in the middle that handles redaction. See #65 for details.

Here is an example usage: https://github.com/elastic/ecs-logging-nodejs/blob/trentm/winston-redact/loggers/winston/examples/redact-fields.js#L83-L91
One could have a simple formatter that simply hardcodes censoring the "http.request.headers.authorization" field. However, I played with a Winston formatter that uses the fast-redact module for generic redaction (as the pino logger does for its redact feature).

Would you be willing to give this a try? I can roll this into a release soon.

@trentm trentm self-assigned this Mar 18, 2021
@lancegliser
Copy link
Author

I left some comments about this on the PR #65 on Apr 8. My mistake. Should I keep comments here, or on the PR?

@trentm trentm added this to the 7.16 milestone Aug 16, 2021
@trentm trentm modified the milestones: 7.16, 8.0-candidate Aug 31, 2021
@AlexanderWert AlexanderWert removed this from the 8.0-candidate milestone Oct 11, 2021
@lancegliser lancegliser mentioned this issue Oct 12, 2021
Closed
@trentm trentm mentioned this issue Oct 19, 2023
Merged
2 tasks
@lancegliser lancegliser mentioned this issue Oct 25, 2023
Closed
@lancegliser
Copy link
Author

Good morning @trentm.

We closed out the serialization issue last week. Tried installing the additions today, but it appears the branch that separates is still outstanding. Needing anything from me here?

@trentm
Copy link
Member

trentm commented Oct 25, 2023

Hello @lancegliser. I am good. I am working my way through a number of issues and plan to get to this one soon.

trentm added a commit that referenced this issue Oct 26, 2023
@trentm
feat: separate winston ecsFormat to composable ecsFields and ecsStrin…
078f730 
…gify

Fixes: #57
Obsoletes: #65
@trentm trentm mentioned this issue Oct 26, 2023
Merged
trentm added a commit that referenced this issue Oct 26, 2023
@trentm
feat(winston): separate winston ecsFormat to composable ecsFields and…
cfb344f 
… ecsStringify (take 2) (#161)

This adds `ecsFields` and `ecsStringify` formats. `ecsFormat` is now just a
composition of the two. Named import is now preferred, but the old default
export remains for now for backwards compat.

The single downside here is that the order of serialized fields no longer
follows the spec (https://github.com/elastic/ecs-logging/blob/main/spec/spec.json)
suggestion of having `@timestamp` then `log.level` then `message` then
the rest ordering. This was a trade-off with avoiding creating an additional
object for each log record.

Fixes: #57
Obsoletes: #65
@trentm trentm mentioned this issue Oct 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@trentm trentm

Labels
8.1-candidate agent-nodejs Make available for APM Agents project planning. enhancement New feature or request lib:winston
Projects
APM-Agents (OLD)
Status: Done
Milestone
No milestone
3 participants
@trentm @AlexanderWert @lancegliser