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

Support HTTP proxy (username, password) authentication #100

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

half0wl
Copy link

@half0wl half0wl commented Jan 4, 2022

I noticed that the Proxy-Authorization header was being inserted as a value in the Authorization: .. header when using RequestAuth.Proxy(..).

  1. Using requests.RequestAuth.Proxy:
@ val reqProxy = requests.RequestAuth.Proxy("username", "password")
reqProxy: requests.RequestAuth.Proxy = Proxy("username", "password")

@ val res = requests.get("http://localhost:8881", auth=reqProxy)
requests.TimeoutException: Request to http://localhost:8881 timed out. (readTimeout: 10000, connectTimout: 10000)
...<snip, irrelevant>
  1. Locally-received values using nc:
 learn-scala-hands-on ツ nc -l 8881
GET / HTTP/1.1
User-Agent: requests-scala
Accept-Encoding: gzip, deflate
Accept: */*
Authorization: Proxy-Authorization dXNlcm5hbWU6cGFzc3dvcmQ=  # <-- note this
Cache-Control: no-cache
Pragma: no-cache
Host: localhost:8881
Connection: keep-alive

Proxy-Authorization is a standalone header [0], and should not be passed in Authorization.

This PR fixes it by replacing RequestAuth.header with a RequestAuth.credentials that returns the built credential to be passed as a header value into the connection. In the Requester, it performs a match on RequestAuth.{TYPE} to determine the header key/value to set.

Unfortunately, I'm unable to get unit tests for proxy auth working - most services similar to httpbin (including it) doesn't echo the Proxy-Authorization value back, according to specs [1] [2]. We can spin up a local http server within the test environment to echo stuff back, though (it'd also reduce CI runtime if it no longer relies on external network requests). I'll leave that to you folks; for now I've commented out the test case and left some notes in there - if you switch to a locally-managed test env, uncommenting and pointing the request to the internal service should make the test pass :)

I manually tested the proxy auth portion against localhost:

(fix-proxy-auth ±) requests-scala ✖ nc -l 8881
GET / HTTP/1.1
User-Agent: requests-scala
Accept-Encoding: gzip, deflate
Accept: */*
Proxy-Authorization: Basic Zm9vOmJhcg==
Cache-Control: no-cache
Pragma: no-cache
Host: localhost:8881
Connection: keep-alive

[0] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Proxy-Authorization
[1] https://tools.ietf.org/html/rfc7235#section-4.4
[2] postmanlabs/httpbin#465

(p/s: I'm very new to Scala, would really love any and all feedback - thank you!)

case basic: RequestAuth.Basic =>
connection.setRequestProperty("Authorization", basic.credentials.get)
case bearer: RequestAuth.Bearer =>
connection.setRequestProperty("Authorization", bearer.credentials.get)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 236 and 238 are doing the same thing, maybe we can use case Foo | Bar to handle them both together in the same case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to tweak my implementation. Would appreciate any feedback you have on the latest round of changes :)

@half0wl
Copy link
Author

half0wl commented Jan 9, 2022

I've updated this to introduce a proxyAuth param instead of stuffing it into RequestAuth. I think that's a clearer implementation, but is a BC break since it removes RequestAuth.Proxy (which doesn't work with the current implementation, and it's also a security concern: dumping Proxy-Authorization .. as a value into Authorization allows the value to be captured by unintended servers).

(tl;dr this removes RequestAuth.Proxy in favor of an explicit proxyAuth parameter)

It's also worth noting that python-requests implements proxy auth through a connection URL (http[s]://[user]:[pass]@[host]:[port]) directly - it extracts the [user]:[pass] which gets b64-encoded and passed along in Proxy-Authorization: Basic <b64e>. That's a friendlier mechanism, IMO.

Tested locally:

// No proxy:
requests.get("http://httpbin.org/ip").text()
// => { "origin": "my_ip" }

// Auth required:
requests.get("http://httpbin.org/ip", proxy=("proxy.provider.com", 8011))
// => requests.RequestFailedException: Request to http://httpbin.org/ip failed with status code 407

// Incorrect auth:
requests.get("http://httpbin.org/ip", proxy=("proxy.provider.com", 8011), proxyAuth=("blah", ""))
// => requests.RequestFailedException: Request to http://httpbin.org/ip failed with status code 407

// Ok, proxied:
requests.get("http://httpbin.org/ip", proxy=("proxy.provider.com", 8011), proxyAuth=("user", "pass")).text()
// => { "origin": "proxy_ip" }

I also noticed that the JVM is removing the Proxy-Authorization header for HTTPS requests, unless jdk.http.auth.tunneling.disabledSchemes is set to "". I should probably flag that behaviour in the readme.

@lihaoyi let me know if you'd like credentials to a proxy service to reproduce/test this further

edit: fixes #79

@half0wl half0wl changed the title Fix incorrect Proxy-Authentication handling Support HTTP proxy (username, password) authentication Jan 9, 2022
@half0wl half0wl mentioned this pull request Jan 9, 2022
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

Successfully merging this pull request may close these issues.

2 participants