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.

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

Log Authorization header as "[REDACTED]"? #5669

Closed
AndreKR opened this issue Aug 2, 2023 · 11 comments
Closed

Log Authorization header as "[REDACTED]"? #5669

AndreKR opened this issue Aug 2, 2023 · 11 comments
Labels
discussion 💬 The right solution needs to be found

Comments

@AndreKR
Copy link

AndreKR commented Aug 2, 2023

I just spend an hour debugging why my application seemingly isn't sending Authorization headers, until I found out that Caddy logs them as empty arrays.

Would it be possible/better to log them as the string "[REDACTED]" which is commonly used to replace sensitive values in log output?

@AndreKR AndreKR changed the title Log authorization header as "[REDACTED]"? Log Authorization header as "[REDACTED]"? Aug 2, 2023
@francislavoie
Copy link
Member

Hmm, yeah, maybe. I don't like REDACTED though, it's too "loud". I think something like --- might be less noisy to read but hopefully still imply redaction. Or *** but I don't like the look of that as much 😅

@mholt mholt added the discussion 💬 The right solution needs to be found label Aug 2, 2023
@mholt
Copy link
Member

mholt commented Aug 2, 2023

What about...

"Authorization": ["▒▒▒▒▒▒"]

😅

(I jest.)

Do we want to add an artificial value though? I feel like that could throw off some things.

@francislavoie
Copy link
Member

I'm kinda leaning towards doing nothing here. We should probably just make it as clear as possible in the docs. Though I think it's already pretty decent, it's in the paragraph underneath the TOC on https://caddyserver.com/docs/caddyfile/directives/log:

image

@AndreKR
Copy link
Author

AndreKR commented Oct 6, 2023

How about [redacted], it's less loud than the all-caps thing.

A missing auth header is an incredibly common error pattern.

So I think you really want to distinguish between empty and non-empty in the logs. Let me rephrase that... it should not look like there's an auth token missing (because you would immediately assume that's the problem) when in reality we don't know because the value was redacted.

@mholt
Copy link
Member

mholt commented Dec 29, 2023

I'm actually leaning toward the Unicode I jokingly posted above, or doing nothing at all, to avoid confusion. What if the header's real value is "REDACTED"?

A missing header wouldn't be logged at all, it wouldn't be empty...

@AndreKR
Copy link
Author

AndreKR commented Dec 29, 2023

It's your call, but I disagree. (With doing nothing. The unicode would be slightly weird but alright for me.)

"[redacted]" is well-known in our industry as a placeholder for information that was removed for privacy reasons.

Similarly well-known is the fact that when you find an empty string where you expected a value then more often than not something went wrong.

A missing header wouldn't be logged at all, it wouldn't be empty...

Let's say you forgot to set the auth token in the configuration of some client. The client would send Authorization: \r\n in the headers. Would that be logged or not? (Whatever the answer is, it's something you would have to look up in Caddy's documentation.)

@mholt
Copy link
Member

mholt commented Dec 29, 2023

I feel like the client would not set an empty header though? Are there popular clients that do this?

I just feel like using an arbitrary string is confusing. I'm not sure what standard makes "[redacted]" more well-known than "redacted" or "REDACTED" or "xxx" or "**" or "???" or anything else for that matter. For example, password inputs definitely don't use [redacted] - they use unicode...

@AndreKR
Copy link
Author

AndreKR commented Dec 29, 2023

I feel like the client would not set an empty header though? Are there popular clients that do this?

So I just tested this in the two scripting environments I had at hand. PHP with curl_setopt($ch, CURLOPT_HEADERS, ['Authorization:']) and Node.js with https.get('https://httpbin.org/headers', {headers: {"Authorization": ""}}, (resp) => { [...] });.

The result was that cURL does not send an empty header this way. If you really want to send an empty header with curl, apparently you have to use a semicolon: curl_setopt($ch, CURLOPT_HEADERS, ['Authorization;']).

Node.js does happily send the empty header.

I'm not sure what standard makes "[redacted]" more well-known than "redacted" or "REDACTED" or "xxx" or "**" or "???" or anything else for that matter.

I didn't mean the format [redacted] specifically, just the word "redacted". For example Sentry uses <redacted>. So your first three examples would all be equivalent, although personally I'd prefer some kind of framing to make it visually stand out.

For example, password inputs definitely don't use [redacted] - they use unicode...

We're talking about logs, not about password fields.

I would collect examples from the Caddy forum, but Caddy users are explicitly discouraged to redact any information, so I'll turn to Serverfault instead.

If you search Serverfault for xxx you will see it mostly be used in prose as a placeholder for stuff that doesn't matter in the context.

If instead you search for "redacted", you will see lots and lots of examples where people are providing logs with sensitive information redacted.

@mholt
Copy link
Member

mholt commented Dec 29, 2023

Thanks for trying it out. I guess it kind of depends on the client (sample size 2).

I didn't mean the format [redacted] specifically, just the word "redacted". For example Sentry uses . So your first three examples would all be equivalent, although personally I'd prefer some kind of framing to make it visually stand out.

It's mostly the arbitrary-ness of it that I find difficult to settle on... and adding extra bytes to the log unnecessarily. 🤷‍♂️

We're talking about logs, not about password fields.

But they both redact, and that's what I was trying to illustrate. People are much more used to password field redaction I'd bet.

I would collect examples from the Caddy forum, but Caddy users are explicitly discouraged to redact any information

Just want to clarify: they're discouraged from redacting non-secret information like domain names and such. Secrets absolutely should be redacted! 💯

Anyway, I dunno -- I'll think about it. I don't have a strong technical reason not to do this other than what I've mentioned, I just try to find the most elegant, standards-conforming solutions when possible (especially when it involves external systems, which logs often do).

@pldubouilh
Copy link

pldubouilh commented Mar 28, 2024

I also bumped on the issue, I was hunting for the value of my auth. header and thought it was empty because of this empty log message.

For what it's worth redact, a fairly popular rust library, is redacting by printing the REDACTED keyword, as well as the type e.g. [REDACTED &str].

@mholt
Copy link
Member

mholt commented Mar 29, 2024

Maybe let's just go with REDACTED then. I'll push a commit momentarily.

EDIT: I also kinda like ▒▒▒▒▒▒ despite joking about it. Would that be bad?

@mholt mholt closed this as completed in 7b48ce0 Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 The right solution needs to be found
Projects
None yet
Development

No branches or pull requests

4 participants