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

Jump to bottom

proposal: net/http: omit contents of Authorization / Cookies header in string output #26551

Closed
adamdecaf opened this issue Jul 23, 2018 · 3 comments
Closed

proposal: net/http: omit contents of Authorization / Cookies header in string output #26551

adamdecaf opened this issue Jul 23, 2018 · 3 comments
Labels
FrozenDueToAge Proposal
Milestone
Proposal

Comments

@adamdecaf
Copy link
Contributor

Consider the following code:

import (
	"fmt"
	"net/http"
)

func main() {
	req, err := http.NewRequest("GET", "https://example.com", nil)
	if err != nil {
		panic(err)
	}
	req.Header.Set("Authorization", "secret")
	fmt.Println(req)
}

// Prints
// &{GET https://example.com HTTP/1.1 %!s(int=1) %!s(int=1) map[Authorization:[secret]] <nil> %!s(func() (io.ReadCloser, error)=<nil>) %!s(int64=0) [] %!s(bool=false) example.com map[] map[] %!s(*multipart.Form=<nil>) map[]   %!s(*tls.ConnectionState=<nil>) %!s(<-chan struct {}=<nil>) %!s(*http.Response=<nil>) <nil>}

This type of code is fairly common, but can lead to exposing the Authorization header (or Cookie). Instead we should opt to not expose these sensitive headers by default. There's similar precedent for masking userinfo passwords from urls in #24572.

We need to keep req.Header.Get("Authorization") unchanged as servers read the auth blob that way. Instead I propose we add:

func (h Header) String() string

As part of the implementation we might need to specify a format for headers. Looking through various Printf encodings gives us:

verb output
%s map[Authorization:[secret]]
%#v Header:http.Header{"Authorization":[]string{"secret"}}
%+v Header:map[Authorization:[secret]]

With a fixed encoding we are able to write the formatter and always mask these sensitive headers.
@gopherbot gopherbot added this to the Proposal milestone Jul 23, 2018
@FMNSSun
Copy link

FMNSSun commented Jul 24, 2018 •
edited

I guess that would fix one problem, but there's plenty of APIs out there that require costum headers such as X-API-TOKEN or API-TOKEN or use query string parameters and you can't catch them all - sure that's not really a reason to not hide the most common one (Authorization) but it's generally a bad idea as far as security is concerned to do logging like that. You shouldn't log critical data if you can't guarantee the security of the logs (if you log to stdout that's probably less of a problem unless the user pipes it to a file - but

&{GET https://example.com HTTP/1.1 %!s(int=1) %!s(int=1) map[Authorization:[secret]] <nil> %!s(func() (io.ReadCloser, error)=<nil>) %!s(int64=0) [] %!s(bool=false) example.com map[] map[] %!s(*multipart.Form=<nil>) map[]   %

isn't exactly user-friendly logging but more "debug logging". Things like fmt.Println(obj) in security critical contexts should probably be abandoned in favor of custom logging where you log what you actually need and don't log things that are critical (for example if you send sensitive information in FormData then you wouldn't necessarily be logging the FormData part).

Especially because I don't think that fmt.Println(obj) is part of any API stability guarantees (?) so maybe someday somebody adds a custom String() method or the internal fields of the struct change etc. so doing logging like this isn't exactly stable either. It's fine for debugging on the fly but probably not what you want to be doing when you're writing security critical production code.

Also, if we're talking about debugging then printing everything is better than hiding information as debug logging should contain enough information for devs to see what's going on and what fields have which values to see if maybe they were set wrongly. I guess this depends on what the intended use case of fmt.Println(req) was/is.

@adamdecaf
Copy link
Contributor Author

Security critical contexts often have all sorts of other restrictions, sure. I'm talking about providing a slightly more safe default. I've seen several projects which just log whole http.Response projects.

Maybe adding func (r *Response) String() string is better suited?

@rsc
Copy link
Contributor

rsc commented Aug 6, 2018

What @FMNSSun said sounds persuasive to me: there are other things one might hide, so it's a false sense of security, and it actively hinders debugging. Discussed with @bradfitz too. Let's leave things as they are.

@rsc rsc closed this as completed Aug 6, 2018
@golang golang locked and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
4 participants
@FMNSSun @rsc @adamdecaf @gopherbot