logging: add a filter for query parameters

[dunglas]

Even if it's considered a bad security practice, it's common that query parameters contain sensitive information. Some examples:

• OAuth access token
• "magic links" and other temporary URLs
• PHP session IDs
• Mercure JWT (the reason why I started working on this patch in the first time)

This PR adds the possibility to redact such sensitive information from the logs.

[francislavoie]

LGTM! Just a couple comments

[wiese]

Cha­peau bas for the idea and initiative. I just saw you mention the PR on twitter, am not really involved with caddy – feel free to ignore my comment.

Maybe that's taking the second step before the first, but my first reaction when seeing the "REDACTED" value was thinking: this is going to be confusing sooner or later. Maybe the better way would be to anonymize/pseudonymize the data (i.e. not having the same log value for all incoming values, as seen in graylog, elastic). Does that resonate with you? Would you see this as an extra feature or an alternative, indeed?

[dunglas]

Maybe the better way would be to anonymize/pseudonymize the data

@wiese What would be the use case? Other similar software seems to redact and not to anonymize:

• Traefik
• Axway
• Kong

Also, it's how the current filters (replace and delete) work.
Maybe could we add more advanced anonymization/hashing features later if there is a use case?

[wiese]

What would be the use case? Other similar software seems to redact and not to anonymize

The use case, like for the current implementation, would be making sure the original values are not contained in the logs but (for pseudonymization) with the added benefit that

• different queries don't look alike, so as to avoid that wrong conclusions are drawn from that (e.g. two queries looking the same in the logs even when they were not, or spotting an illusive hot spot when grouping them)
• and vice versa, in queries which originally were the same, one can be found based on another and they can be grouped (think: by user JWT)

Of course, this functionality could totally exist in addition to redacting as is and added later if there is a demand. No worries, I just wanted to run the thought in case you say "oh, that's actually closer to what I want".

[francislavoie]

We could do a thing where if the replacement value is the string HASHED then it would SHA-256 hash the existing value (maybe truncated to 8 chars? I dunno) to deal with anonymizing without making different requests look alike.

Obviously it should be noted that this isn't a good idea for passwords for example, i.e. anything where brute-forcing the value would be problematic. In which case, simply redacting is better.

[dunglas]

Or maybe could we just add an extra action called hash in addition to replace and delete? It would be very easy to implement.

[dunglas]

Maybe could we add this feature in a follow-up PR? (I will do it, but it will be easier to add hash support everywhere when the pending PRs will be merged).

[mholt]

Hey Kevin, thanks for the contribution! I like where this is going.

Just wondering... could both replace and delete actions be reduced to a single replace action? Since deleting could be replacing with an empty string, and you wouldn't need different action types and stuff. It'd be simpler. You could then just list the replacements you want to perform on the query string.

Edit: I submitted this as "request changes" but I guess I should have just posted this as a regular comment. Sorry.

[dunglas]

Hi Matt,

This is possible but it could get messy if we add more actions such as hash. On the other hand, we could use @francislavoie's suggestion of using special strings.

Currently:

format filter {
    fields {
	uri query {
		replace foo1 REDACTED
                replace foo2 FOO2
		delete bar
		hash baz
	    }
    }
}

With your suggestion:

format filter {
     fields {
         uri query foo1 REDACTED foo2 FOO bar `` baz HASH
     }
}

I find the first one more readable but I'm ok to update the PR to use the second option if you prefer.

Also, in we implement your solution, maybe should we also deprecate the existing delete filter for consistency (using an empty string as a parameter of the replace directive also works).

WDYT?

[mholt]

Thanks for the extra information. That's very interesting! As discussed in Slack, I think the current approach is fine, since it allows other actions to be configured like hashing. It's also easier to read. Thanks!

[mholt]

Just one thing I noticed. Looking good though.

[francislavoie]

Thanks!

[mholt]

Thank you! LGTM except a little nit on naming / exporting one identifier.

[francislavoie]

Thanks!

Resolved