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

net/url: add URL.Redacted to return password-free string #34855

Open
nrxr opened this issue Oct 11, 2019 · 13 comments · May be fixed by #35578

Comments

@nrxr
Copy link

@nrxr nrxr commented Oct 11, 2019

Hiding the password from an URL is very useful for logging purposes. I have seen in the past code for doing this (or just plain passwords in logs). I built a small helper for this and I use it in my projects but I think it makes sense to have it in the standard library.

I wrote a small PR for this #34686 with the code (and test suite) for this feature.

It's a simple derivation from URL.String() that masks the password if exists from the string being passed. It makes no modification at all to the URL itself but to a copy of it. URL.Redacted() is just filtering passwords out of URL.String().

For example https://user@host.tld would be https://user@host.tld but https://user:password@host.tld would be https://user:xxxxx@host.tld. Making obvious that a password has been masked is good for visual-debugging purposes, so it's known a password is being passed in the URL.

@gopherbot gopherbot added this to the Proposal milestone Oct 11, 2019
@gopherbot gopherbot added the Proposal label Oct 11, 2019
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 16, 2019

Do people still use passwords in URLs?

There are many different kinds of data in URLs. Some URL parameters may be sensitive, so why single out passwords?

The helper package is small, and seems to work well. Is this really widely used enough to add to the standard library? https://golang.org/doc/faq#x_in_std

@nrxr

This comment has been minimized.

Copy link
Author

@nrxr nrxr commented Oct 20, 2019

@ianlancetaylor Hi!

Yes, people still does. Specially when connecting to other servers, not over HTTP. Here's a list from the top of my head of services to which I connect my go apps and have passwords in its URLs: postgresql, mongo, rabbitmq and mysql.

I singled out passwords because the use case I can see for this is logging purposes, mainly. I've been a DevOps at a pair of fairly large companies and saw lots of passwords in our logs.

I thought we were alone in this until I listened to Go Time's podcast on security and actually one of the things mentioned were hiding passwords from logs. That's when I saw it was not just me and hence why I proposed it here.

Widely used: no. Should be widely used? Probably.

Huge point against the inclusion of this: there's no other programming language that includes such method in their standard library.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 21, 2019

I don't believe Mask is the right name. I thought it was about IP masking when I first saw the issue.
cmd/go uses a function like this called Redacted.
We could expose that code under that name if needed.
One nice thing about the API we designed for cmd/go is that Redacted returns a string, not a *url.URL. That makes it clearer that the result is for printing and not for future direct use in an HTTP request.

/cc @bcmills

@nrxr

This comment has been minimized.

Copy link
Author

@nrxr nrxr commented Oct 21, 2019

@rsc this is the case for Mask as well, it returns a string. I like Redacted better, indeed.

@rsc rsc changed the title proposal: net/url: add URL.Mask() method that hides the password proposal: net/url: add URL.Redacted to return password-free string Oct 30, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 30, 2019

I retitled the issue to use the Redacted name.
Does anyone object to adding this functionality?

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 6, 2019

Everyone seems in favor of this, so it seems like a likely accept.
Leaving open for a week for final comments.

@bvisness

This comment has been minimized.

Copy link

@bvisness bvisness commented Nov 7, 2019

As a small detail, you might also want to add an analogous Redacted method to the UserInfo type from net/url.

I think if you are handling passwords you might as well provide a way to easily hide them, but to @ianlancetaylor's point, the user password field is not the only thing you may want to redact from a URL, so perhaps a Redact method on the base URL type should also be able to redact query parameters, or even parts of the path. But those options might just add too much complexity to be worth it.

I certainly wouldn't want anyone to think they were safe just because they called Redacted, but didn't actually use plaintext passwords in this way.

@bvisness

This comment has been minimized.

Copy link

@bvisness bvisness commented Nov 7, 2019

Given that the func Redacted used internally by cmd/go also only redacts the password, I have put my thumbs-up on this, but I think it's worth thinking about whether a method in the standard library would need more flexibility.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 13, 2019

@bvisness, if you need a super-fancy redactor that can strip things from URL parameters, probably you should reach for a third-party package. Or maybe think about not putting authentication information into URL parameters, where it leaks via Referer and other headers.

No change in consensus, so accepting.

@rsc rsc modified the milestones: Proposal, Go1.15 Nov 13, 2019
@rsc rsc changed the title proposal: net/url: add URL.Redacted to return password-free string net/url: add URL.Redacted to return password-free string Nov 13, 2019
@nrxr

This comment has been minimized.

Copy link
Author

@nrxr nrxr commented Nov 13, 2019

@ianlancetaylor @rsc Hi! What’s the fix needed and I’ll write it in a PR tonight.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 13, 2019

No rush, we are in the release freeze for 1.14 right now.

nrxr pushed a commit to nrxr/go that referenced this issue Nov 14, 2019
Returning an URL.String() without the password is very useful for
situations where the URL is supposed to be logged and the password is
not useful to be shown.

This method re-uses URL.String() but with the password scrubbed and
substituted for a "xxxxx" in order to make it obvious that there was a
password. If the URL had no password then no "xxxxx" will be shown.

Fixes golang#34855
@nrxr

This comment has been minimized.

Copy link
Author

@nrxr nrxr commented Nov 14, 2019

Already understood what NeedsFix means (if somehow you got here because of a search on that sentence, here's the meaning of NeedsFix in this context: https://golang.org/doc/contribute.html#check_tracker).

Already created a PR with the implementation I think fulfills this need.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 14, 2019

Change https://golang.org/cl/207082 mentions this issue: net/url: add URL.Redacted() to return password-free string

nrxr pushed a commit to nrxr/go that referenced this issue Nov 15, 2019
Returning an URL.String() without the password is very useful for
situations where the URL is supposed to be logged and the password is
not useful to be shown.

This method re-uses URL.String() but with the password scrubbed and
substituted for a "xxxxx" in order to make it obvious that there was a
password. If the URL had no password then no "xxxxx" will be shown.

Fixes golang#34855
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.