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

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

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

Comments

Projects
None yet
4 participants
@adamdecaf
Copy link
Contributor

commented Jul 23, 2018

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

@gopherbot gopherbot added the Proposal label Jul 23, 2018

@FMNSSun

This comment has been minimized.

Copy link

commented Jul 24, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

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

This comment has been minimized.

Copy link
Contributor

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 Aug 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.