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

Automatically handling URIs with basic authentication #476

Closed
janko opened this issue May 28, 2018 · 11 comments
Closed

Automatically handling URIs with basic authentication #476

janko opened this issue May 28, 2018 · 11 comments

Comments

@janko
Copy link
Member

janko commented May 28, 2018

Would it make sense for http.rb to automatically apply basic authentication when given URIs that include it? For example, given a URL

url = "https://user:password@example.com"

it would be nice if instead of

uri = HTTP::URI.parse(url)
http = HTTP 
http = http.basic_auth(user: uri.user, pass: uri.password) if uri.user || uri.password
http.get(uri.to_s)

one could simply call

HTTP.get(url)

and have http.rb apply the basic authentication automatically.

@tarcieri
Copy link
Member

Yes, we should definitely do that

@ixti
Copy link
Member

ixti commented May 28, 2018

Absolutely agree!

@janko
Copy link
Member Author

janko commented May 28, 2018

Great, I'll start working on that.

@tarcieri
Copy link
Member

tarcieri commented May 28, 2018

From #477, @janko-m noted the Mozilla guidelines on HTTP AuthN show this pattern as deprecated:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication

I'm not sure what the best option is in this case, but when in doubt I think if the browsers want to phase it out, it's probably best to follow suit.

@janko
Copy link
Member Author

janko commented May 28, 2018

Yeah, I agree, I'll close this ticket then.

@janko janko closed this as completed May 28, 2018
@ixti
Copy link
Member

ixti commented May 28, 2018

I would like to note one edge case we should care of:

# url might or might not contain basic auth info
HTTP.basic_auth(:user => user, :pass => pass).get(url)

I know this is a rare case, but still might cause confusion. So I propose to:

  1. use basic_auth info in this case over the one inherited from URI - as this seems to be more inline with how we process query params (:params keyword takes precedence over the query params of URI).
  2. probably show warning that explicit user/pass was used

@ixti ixti mentioned this issue May 28, 2018
@gnoireaux

This comment has been minimized.

@jrochkind
Copy link
Contributor

Personally I think the Mozilla deprecation is with regard to user-facing user-agent UI. It's still part of the HTTP URI spec, and it's still often used in back-end systems, and I still think it would be useful for http-rb to handle it, I often need to write custom code to handle it itself, taking the auth out of the URI and passing it to http-rb separately. But it's not that hard to write that custom code, so not a huge deal.

(The existing pattern would still also have to be supported, because there are definitely times you want to supply the basic auth credentials "out of band" from the URI).

@tarcieri
Copy link
Member

It's still part of the HTTP URI spec

Could you more precisely describe what you're referring to? It indeed is part of the "authority" section of the URI generic syntax as described in RFC3986, however note that URIs are themselves decoupled from HTTP.

The core argument for not supporting basic auth in URIs in a library context (as opposed to a user-facing command line utility like curl) is a similar worry to browsers, however let me spell it out since people seem to be arguing others:

URIs in a library context may potentially be attacker-supplied and parsing the authority section may allow an attacker to unintentionally trigger HTTP basic auth in contexts where the library user doesn't want it.

We could potentially support an (off-by-default) option to automatically parse them.

@jrochkind
Copy link
Contributor

jrochkind commented Dec 10, 2020

Yes, I just mean it's still part of relevant applicable specs, and i believe there are no subsequent RFC's deprecating it or anything, even if some vendors have suggested it.

I don't entirely understand the attack vector, why unintentionally triggering HTTP basic auth is a vulnerability. If the attacker can supply the URI, they can specify the host and path and scheme (http or https), I do not understand the additional vulnerability of letting them specify basic auth. HOWEVER, I believe you that there is one if you tell me, I know security vulnerabilities can be brain-twisters.

I just know that these days I am frequently having to (or finding it very convenient to) deal with basic auth in the URI. Often in cases where an HTTP URI is supplied in a config variable (a callback/webhook, or an integration URI of some kind), and it sometimes has basic auth in it. It is more convenient to allow it to do so than to try to divide into multiple config variables.

But it's up to you, it is not that hard to work around if the library doesn't support it out of the box so the stakes aren't huge for me. I have been doing things like this:

def get_http_client(uri)
   parsed = URI.parse(uri)
   if parsed.user || parsed.password
      HTTP.basic_auth(user: parsed.user, pass: parsed.password)
   else
      HTTP
   end
end

get_http_client(uri).post(uri) # or whatever

It just annoyed me slightly that I had to keep doing that somewhat hacky thing in project after project and remembering to do it, and where I keep tweaking it to be slightly different (and why is the basic_auth arg pass instead of password anyway, I keep forgetting it), and sort of judge myself for using URI.parse since I know that's actually a pretty expensive operation, and just thought, wait, why doesn't http-rb just supprot this totally standard thing anyway, and found myself here.

But it's still pretty easy to workaround (hooray for chainable API), so, up to you.

@jrochkind
Copy link
Contributor

jrochkind commented Dec 10, 2020

trying to research the nature of the vulnerability, I find this:

A url like http://www.google.com:search@evil.com/ can be misleading to users. To someone not aware of semantics of URLs, could easily think that is a geniune google.com page. There are also some exploits that try to use strnage chars int eh passowrd (like a null byte) to fool the browser into stopping showing any more of the url.

That makes total sense to me, but seems specific to the browser context. It's not that the attacker can supply basic auth when you didn't know you were using auth, it's that it was used as a way to trick users into thinking it was a different URL than it really was, on visual inspection of it in a browser location bar. I don't think that is applicable to http-rb as a general-purpose library, intending to be a stdlib sort of thing even if not technically in stdlib.

But there may be other reasons too. ¯\(ツ)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants