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

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

Closed
nrxr opened this issue Oct 11, 2019 · 16 comments
Closed

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

nrxr opened this issue Oct 11, 2019 · 16 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone
Go1.15

Comments

@nrxr
Copy link

nrxr commented Oct 11, 2019 •
edited

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
@ianlancetaylor
Copy link
Contributor

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

@andybons andybons mentioned this issue Oct 16, 2019
Open
@nrxr
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

rsc commented Oct 30, 2019

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

@rsc
Copy link
Contributor

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
Copy link

bvisness commented Nov 7, 2019 •
edited

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
Copy link

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
Copy link
Contributor

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
@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal-FinalCommentPeriod labels Nov 13, 2019
@nrxr
Copy link
Author

nrxr commented Nov 13, 2019

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

@ianlancetaylor
Copy link
Contributor

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
net/url: add URL.Redacted() to return password-free string
4a3b9c2 
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 nrxr mentioned this issue Nov 14, 2019
Closed
@nrxr
Copy link
Author

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
Copy link

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
net/url: add URL.Redacted() to return password-free string
43e2477 
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
@odeke-em
Copy link
Member

Awesome, thank you @nrxr and great to have you contributing to and using Go!
I’ve reviewed your CL and added some suggestions, please take a look. Thanks.

@odeke-em
Copy link
Member

@oiooj @bcmills @jayconrod, turns out there is a parallel issue here that #37873 would benefit from when CL https://golang.org/cl/207082 is merged, and we won’t worry about percent encoring of [].

We could then perhaps make an edit to remove usages of cmd/go/internal/web.Redacted(), in favor of (*url.URL).Redacted()

nrxr pushed a commit to nrxr/go that referenced this issue Apr 7, 2020
net/url: add URL.Redacted() to return password-free string
e56499a 
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 pushed a commit to nrxr/go that referenced this issue Apr 7, 2020
net/url: add URL.Redacted() to return a password scrubbed string
4c0d033 
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 pushed a commit to nrxr/go that referenced this issue Apr 7, 2020
net/url: add URL.Redacted() to return a password scrubbed string
ad2e110 
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
Copy link
Author

nrxr commented Apr 7, 2020

@odeke-em Hi! Sorry for the delay; couple of busy weeks. I got around and wrote the suggestions you made and sent them to gerrit.

nrxr pushed a commit to nrxr/go that referenced this issue Apr 8, 2020
net/url: add URL.Redacted() to return a password scrubbed string
46d06db 
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
@ianlancetaylor ianlancetaylor mentioned this issue Jun 12, 2020
Closed
@roidelapluie roidelapluie mentioned this issue Jul 21, 2020
Open
@burhan burhan mentioned this issue Mar 18, 2021
Closed
@golang golang locked and limited conversation to collaborators Apr 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Milestone
Go1.15
Development

Successfully merging a pull request may close this issue.

net/url: add URL.Redacted to return a password scrubbed string nrxr/go
6 participants
@rsc @ianlancetaylor @bvisness @odeke-em @gopherbot @nrxr