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.

Sign up for GitHub

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

Add Host header (#650) #651

Merged
merged 1 commit into from
Dec 6, 2022
Merged

Add Host header (#650) #651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand
method: .CONNECT,
uri: "\(self.targetHost):\(self.targetPort)"
)
head.headers.replaceOrAdd(name: "host", value: "\(self.targetHost)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not the right host in all cases. We allow users to overwrite the host and also allow non-default ports.
We already have a method which can take care of this for us:

async-http-client/Sources/AsyncHTTPClient/RequestValidation.swift

Lines 114 to 129 in 49abfc3

extension HTTPHeaders {
mutating func addHostIfNeeded(for url: DeconstructedURL) {
// if no host header was set, let's use the url host
guard !self.contains(name: "host"),
var host = url.connectionTarget.host
else {
return
}
// if the request uses a non-default port, we need to add it after the host
if let port = url.connectionTarget.port,
port != url.scheme.defaultPort {
host += ":\(port)"
}
self.add(name: "host", value: host)
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems I can't use this nice method cause I don't have DeconstructedURL in this context.

Tried to build DeconstructedURL but it requires many lines of code.
I just implemented check on place using what we have.

Sending one more pull request #652 since this one is closed already.
Please let me know if I should build DeconstructedURL and use addHostIfNeeded instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dnadoba could you please take a look at #652

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure this is the right host in all cases: the Host header for CONNECT should be the Host of the target URI, the same thing we put in the request line. That's what this should be.

Copy link
Collaborator

@dnadoba dnadoba Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Given the following request:

let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
var request = HTTPClientRequest(url: "http://127.0.0.1/")
request.headers.add(name: "host", value: "swift.org")
let response = try await httpClient.execute(request, timeout: .seconds(10))

This will currently send a request header with

Host: swift.org

and not

Host: 127.0.0.1

This can be useful to e.g. to test a local nginx configuration without modifying /etc/hosts. AFAIK browsers don't support this for security reasons.

If we now add a CONNECT proxy into the mix with the following request:

var configuration = HTTPClient.Configuration()
configuration.proxy = .server(host: "example.com", port: 80)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
var request = HTTPClientRequest(url: "http://127.0.0.1/")
request.headers.add(name: "host", value: "swift.org")

we will make a connection to example.com and send it this request:

CONNECT 127.0.0.1:80 HTTP/1.1
Host: 127.0.0.1

and afterwards will send a HTTP request with the following header:

Host: swift.org

The HTTP/1.1 RFC doesn't really mention what the Host headers value should be for in the initial CONNECT request. I thought that the Host should be the same as the Host in the subsequent request. This would allow the proxy to connect you to a server that can handle the request. However I might be wrong, that is just a guess on my side.

curl doesn't use the Host in the header and also not the host of the proxy we connect to but will use the host of the request URL:

% curl -v -curl -H "Host: swift.org" --proxy "http://[::1]:8888" https://127.0.0.1/ 
*   Trying ::1:8888...
* Connected to ::1 (::1) port 8888 (#0)
* allocate connect buffer
* Establish HTTP proxy tunnel to 127.0.0.1:443
> CONNECT 127.0.0.1:443 HTTP/1.1
> Host: 127.0.0.1:443
> User-Agent: curl/7.85.0
> Proxy-Connection: Keep-Alive

I have tried setting up a proxy with URLSession to also see its behaviour but have failed to do so.

I think we should at least follow curl here and also use the host specified in the request URL for the initial CONNECT and not the host & port of the proxy.

What you you think @Lukasa?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are following curl here, no? Looking at the test, we appear to be doing exactly the same thing as curl.

if let authorization = self.proxyAuthorization {
head.headers.replaceOrAdd(name: "proxy-authorization", value: authorization.headerValue)
}
Expand Down
5 changes: 5 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTP1ProxyConnectHandlerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class HTTP1ProxyConnectHandlerTests: XCTestCase {

XCTAssertEqual(head.method, .CONNECT)
XCTAssertEqual(head.uri, "swift.org:443")
XCTAssertEqual(head.headers["host"].first, "swift.org")
XCTAssertNil(head.headers["proxy-authorization"].first)
XCTAssertEqual(try embedded.readOutbound(as: HTTPClientRequestPart.self), .end(nil))

Expand Down Expand Up @@ -76,6 +77,7 @@ class HTTP1ProxyConnectHandlerTests: XCTestCase {

XCTAssertEqual(head.method, .CONNECT)
XCTAssertEqual(head.uri, "swift.org:443")
XCTAssertEqual(head.headers["host"].first, "swift.org")
XCTAssertEqual(head.headers["proxy-authorization"].first, "Basic abc123")
XCTAssertEqual(try embedded.readOutbound(as: HTTPClientRequestPart.self), .end(nil))

Expand Down Expand Up @@ -109,6 +111,7 @@ class HTTP1ProxyConnectHandlerTests: XCTestCase {

XCTAssertEqual(head.method, .CONNECT)
XCTAssertEqual(head.uri, "swift.org:443")
XCTAssertEqual(head.headers["host"].first, "swift.org")
XCTAssertNil(head.headers["proxy-authorization"].first)
XCTAssertEqual(try embedded.readOutbound(as: HTTPClientRequestPart.self), .end(nil))

Expand Down Expand Up @@ -148,6 +151,7 @@ class HTTP1ProxyConnectHandlerTests: XCTestCase {

XCTAssertEqual(head.method, .CONNECT)
XCTAssertEqual(head.uri, "swift.org:443")
XCTAssertEqual(head.headers["host"].first, "swift.org")
XCTAssertNil(head.headers["proxy-authorization"].first)
XCTAssertEqual(try embedded.readOutbound(as: HTTPClientRequestPart.self), .end(nil))

Expand Down Expand Up @@ -187,6 +191,7 @@ class HTTP1ProxyConnectHandlerTests: XCTestCase {

XCTAssertEqual(head.method, .CONNECT)
XCTAssertEqual(head.uri, "swift.org:443")
XCTAssertEqual(head.headers["host"].first, "swift.org")
XCTAssertEqual(try embedded.readOutbound(as: HTTPClientRequestPart.self), .end(nil))

let responseHead = HTTPResponseHead(version: .http1_1, status: .ok)
Expand Down