Better support for UNIX Domain sockets #228

dimitribouniol · 2020-05-27T07:05:55Z

These changes upgrade the unix domain socket handling to address the following issues:

Using socket paths currently requires obtuse URL objects with a specially constructed baseURL. These are not easily serializable as strings.
It is currently not possible to connect to a server over a socket path that uses TLS.

To maintain backward compatibility, this PR introduces two new URL schemes: http+unix://, and https+unix://. These url schemes, while not universal, are used in a standard way by other services (some - examples), and work by %-encoding the socket path as the url hostname. These are easy to construct using standard swift types:

let socketURL = URL(string: "https+unix://\(path.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/echo-uri")

This produces a url that looks something like this: https+unix://%2Ftmp%2FfilePath.socket/echo-uri, which is fully supported by URL, and can be serialized and deserialized to and from a String easily.

When the https+unix:// URL scheme is used, TLS is turned on. It is still up to the user to perform certificate validation under this scheme, but at the very least an encrypted connection over a socket is now possible if the server is configured to only provide such a configuration.

I'd also like to think I did a good job keeping things neat and tidy, and inline with the style of the repo, but I'm open to any and all feedback! (This is my first time contributing to any large open source project, so please be kind 😅)

Where to go from here

Some future improvements that can be made:

More tests…
Some documentation on Request outlining the different possibilities.
It would be nice to have a factory on URL to build a unix socket URL from a socket path and server path.
We need some constants for "http", "https", "unix", "http+unix", and "https+unix" - it's a bit unwieldy at the moment.
Perhaps deprecating the unix: scheme for the next major version? It's use is a bit unwieldy from a user's point of view (especially as many frameworks like Vapor have client constructors that take in strings as the URLs), and I doubt its currently widely used.

Motivation: Using a base URL as the socket path only works when the URL object is maintained as long as possible through the stack. Additionally, it doesn't currently provide a way to use TLS over UNIX sockets. Modifications: Added two tests to test out the to-be supported URL schemes, http+unix, and https+unix, which encode the socket path as a %-escaped hostname, as some existing services already do. Result: Some failing tests.

swift-server-bot · 2020-05-27T07:05:57Z

Can one of the admins verify this patch?

swift-server-bot · 2020-05-27T07:05:57Z

Can one of the admins verify this patch?

swift-server-bot · 2020-05-27T07:05:57Z

Can one of the admins verify this patch?

swift-server-bot · 2020-05-27T07:05:57Z

Can one of the admins verify this patch?

Lukasa

Cool, this generally looks really good! I'm in favour of the change, but I have a few notes in the diff.

Lukasa · 2020-05-27T08:49:08Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+        /// Socket path, resolved from `URL`.
+        public let socketPath: String
+        /// URI, resolved from `URL`.
+        public let uri: String


Is it necessary for either of these to be public? In particular, uri is extremely not well distinguished from url, and if we're keeping both in the API we really need to explain how they differ.

Not at all — continuing on that thought, the host will be empty for socket-based Requests, even though that's where its encoded… Should we merge the implementations to make the socket path available as the host?

Lukasa · 2020-05-27T08:50:29Z

Sources/AsyncHTTPClient/HTTPHandler.swift

            func supports(scheme: String) -> Bool {
                switch self {
                case .host:
                    return Kind.hostSchemes.contains(scheme)
-                case .unixSocket:
+                case .unixSocket(scheme: _):
                    return Kind.unixSchemes.contains(scheme)


This supports spelling is getting unwieldy here, not least because we're no longer using half of the switch statement. Should this just be a constructor of Kind from String?

I'm not too sure — we are already constructing Kind from String in https://github.com/swift-server/async-http-client/pull/228/files#diff-67e2f60fc709da32af89dfbfc0c774aaR116-R129, so that method can certainly be fixed up to no longer rely on supports().

My bigger question is that supports() seems like it is being used by some redirect logic here:

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

Line 962 in 070c1e5

guard self.request.kind.supports(scheme: self.request.scheme) else {

It seems like the older version was just making sure that the new scheme matched http/https - while the one currently in the repo will only accept http redirects from http requests, and socket redirects from socket requests. @krzyzanowskim Do you remember more about why you set it up this way?

I propose that if the request is an http request, we keep it as such, but if it is a socket request, we allow it to redirect to any supported scheme, but that is based on my limited understanding of what's going on here 😅

I propose that if the request is an http request, we keep it as such, but if it is a socket request, we allow it to redirect to any supported scheme, but that is based on my limited understanding of what's going on here 😅

Yeah, that seems a reasonable design choice.

Would you support refactoring this method to something like this, since it is now only used for redirects?

Call site (which was checking against the old scheme for some reason that escapes me):

This would also simplify the initializer tremendously:

@krzyzanowskim Do you remember more about why you set it up this way?

Sorry, I don't remember details at the moment. I'm sorry not being helpful here.

@krzyzanowskim Not a problem! I think the original code may have also had the same issue, so ultimately we just need more tests that capture the underlying problem 😅

Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift

weissi · 2020-05-27T12:02:31Z

Very cool!

dimitribouniol · 2020-05-27T14:56:20Z

Thank you @Lukasa and @weissi for your feedback! Should I amend my previous commits and replace the ones here, or would you prefer I add additional commits that address the remaining changes?

Lukasa · 2020-05-27T14:59:06Z

It's entirely up to you: we'll squash when we merge, so we'll end up with only one commit anyway.

dimitribouniol · 2020-05-27T18:25:35Z

I addressed most of the changes - all that's left are the following related but out of scope issues:

Should we combine `host` and `socketPath`?

This should probably be a different discussion/PR due to where Request.host is being used, but from what I can determine, this would probably be the right thing to do in the long run.

From what I could tell:

The socket path would end up in the Host header for the request (currently it is "")
TLS would use the socket path on certs as the common name/hostname (currently it is checking for "" as opposed to nil, which it's doing for IPs)
- ~~Should IPs also be part of the cert validation? Doesn't make sense for them to be omitted honestly...~~
- I take this back: The spec says:

Literal IPv4 and IPv6 addresses are not permitted in "HostName".

Responses would propagate the socket path as the host as well (currently, it's also "")
We would need better test coverage for these scenarios.

I'd be happy to submit the above as a separate PR. ➙ #229

It's likely redirects for sockets and regular hosts never behaved as expected

The redirect logic checks the current request rather than the new URL to see if it can redirect to it - this likely never worked as designed due to the fact that it would always pass, since we have a request that is already valid, and we are checking It for validity...

Again, I'd be happy to refactor this and submit the fix as a separate PR. ➙ #230

Lukasa

This patch LGTM.

Lukasa · 2020-05-28T09:49:41Z

@swift-server-bot test this please

Lukasa · 2020-05-28T09:49:56Z

As for the follow-on issues you identified, I'm more than happy for you to address them in follow-up PRs.

Lukasa · 2020-05-28T09:50:39Z

It seems there are some minor formatting issues that need to be cleaned up.

weissi · 2020-05-28T11:04:04Z

@dimitribouniol apologies, there seem to be formatting issues. If you just run scripts/sanity.sh on your machine, it should automatically invoke SwiftFormat and fix those.

…for the new schemes Motivation: Before I start adding support for new URL schemes, I consolidated the existing use of unix sockets and https TLS handling. Modifications: Consolidated socket path and uri handling to one place, added an associated type to the unixSocket case, and added a computed requiresTLS property to the Key Scheme. Result: Much cleaner code that is now ready for some new schemes!

Motivation: We can now not only make connections over sockets using the existing unix: baseURL approach, but also by %-escaping the socket path as the URL's hostname, and using the http+unix: or https+unix: schemes for plain HTTP or HTTP over TLS support. Modifications: Added references to the new url schemes where necessary, aand added a missingSocketPath error for when the socket path is not encoded as the hostname. Result: The tests now pass, and connections can be made over non-special URLs, supporting TLS if necessary.

… to socket paths Motivation: Binding HTTPBin to a socket path generates many warnings in the console that are largely inconsequential, but cloud the results. This removes one of them 😅 (I'm not sure how to address the others) Modifications: TCP_NODELAY is now a default option in NIO, so we can leave out the setting completely since it isn't supported when binding to socket paths. Result: One fewer warning in the console 🙃

dimitribouniol · 2020-05-28T15:26:42Z

I've just addressed the formatting issues — I'm creating separate tracking issues for the two points I brought up above for further discussion.

weissi · 2020-06-01T12:04:56Z

@swift-server-bot test this please

dimitribouniol · 2020-06-01T16:23:45Z

Seems like on linux, the client will throw a NIOSSLExtraError.invalidSNIHostname if the SNI is equal to "" (length must be between 1 and 255), so I replace an empty host with nil before proceeding (just like is done for IP addresses).

@weissi Could you re-trigger the tests please? (unless I'm able to do it using the same command you used?)

dimitribouniol · 2020-06-01T18:18:30Z

@weissi Please hold off on re-testing - I got Docker running locally, and it's still failing 😅

…r would result in an SNI error Motivation: Testing on Linux seemed to reveal an `NIOSSLExtraError.invalidSNIHostname` error that was thrown when using `https+unix` URLs, which don't have their hostname filled in. The same NIO failure does not occur on macOS, as it seems that Network.framework does not mind an empty hostname. Modifications: Added an extra check to determine if the host is empty before passing it to a `NIOSSLClientHandler`. Result: testHTTPSPlusUNIX() no longer fails on Linux.

dimitribouniol · 2020-06-01T19:12:33Z

@weissi All tests now pass on my local machine! Please re-test 🙂

weissi · 2020-06-01T19:12:52Z

@swift-server-bot test this please

weissi

Thank you! LGTM

Lukasa requested changes May 27, 2020

View reviewed changes

dimitribouniol force-pushed the http-plus-unix branch 3 times, most recently from 014536c to c6c32eb Compare May 27, 2020 17:58

dimitribouniol force-pushed the http-plus-unix branch from c6c32eb to 4263f56 Compare May 27, 2020 18:39

Lukasa approved these changes May 28, 2020

View reviewed changes

weissi requested a review from artemredkin May 28, 2020 11:04

dimitribouniol added 3 commits May 28, 2020 08:22

dimitribouniol force-pushed the http-plus-unix branch from 4263f56 to bada1bb Compare May 28, 2020 15:25

This was referenced May 28, 2020

Should we combine .host and .socketPath on HTTPClient.Request? #229

Open

It's likely redirects for sockets and regular hosts never behaved as expected #230

Closed

dimitribouniol requested a review from weissi May 30, 2020 05:54

dimitribouniol force-pushed the http-plus-unix branch from 84d4064 to 7b7051a Compare June 1, 2020 19:04

weissi approved these changes Jun 1, 2020

View reviewed changes

weissi added the 🆕 semver/minor Adds new public API. label Jun 1, 2020

weissi added this to the 1.2.0 milestone Jun 1, 2020

weissi merged commit 364d106 into swift-server:master Jun 2, 2020

dimitribouniol deleted the http-plus-unix branch June 2, 2020 17:36

tanner0101 mentioned this pull request Jun 24, 2020

Support for Unix Domain Sockets vapor/vapor#2371

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better support for UNIX Domain sockets #228

Better support for UNIX Domain sockets #228

dimitribouniol commented May 27, 2020 •

edited

Loading

swift-server-bot commented May 27, 2020

swift-server-bot commented May 27, 2020

swift-server-bot commented May 27, 2020

swift-server-bot commented May 27, 2020

Lukasa left a comment

Lukasa May 27, 2020

dimitribouniol May 27, 2020

Lukasa May 27, 2020

dimitribouniol May 27, 2020 •

edited

Loading

Lukasa May 27, 2020

dimitribouniol May 27, 2020

krzyzanowskim May 28, 2020

dimitribouniol May 28, 2020

weissi commented May 27, 2020

dimitribouniol commented May 27, 2020

Lukasa commented May 27, 2020

dimitribouniol commented May 27, 2020 •

edited

Loading

Lukasa left a comment

Lukasa commented May 28, 2020

Lukasa commented May 28, 2020

Lukasa commented May 28, 2020

weissi commented May 28, 2020

dimitribouniol commented May 28, 2020

weissi commented Jun 1, 2020

dimitribouniol commented Jun 1, 2020

dimitribouniol commented Jun 1, 2020

dimitribouniol commented Jun 1, 2020

weissi commented Jun 1, 2020

weissi left a comment

Better support for UNIX Domain sockets #228

Better support for UNIX Domain sockets #228

Conversation

dimitribouniol commented May 27, 2020 • edited Loading

Where to go from here

swift-server-bot commented May 27, 2020

swift-server-bot commented May 27, 2020

swift-server-bot commented May 27, 2020

swift-server-bot commented May 27, 2020

Lukasa left a comment

Choose a reason for hiding this comment

Lukasa May 27, 2020

Choose a reason for hiding this comment

dimitribouniol May 27, 2020

Choose a reason for hiding this comment

Lukasa May 27, 2020

Choose a reason for hiding this comment

dimitribouniol May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Lukasa May 27, 2020

Choose a reason for hiding this comment

dimitribouniol May 27, 2020

Choose a reason for hiding this comment

krzyzanowskim May 28, 2020

Choose a reason for hiding this comment

dimitribouniol May 28, 2020

Choose a reason for hiding this comment

weissi commented May 27, 2020

dimitribouniol commented May 27, 2020

Lukasa commented May 27, 2020

dimitribouniol commented May 27, 2020 • edited Loading

Should we combine host and socketPath?

It's likely redirects for sockets and regular hosts never behaved as expected

Lukasa left a comment

Choose a reason for hiding this comment

Lukasa commented May 28, 2020

Lukasa commented May 28, 2020

Lukasa commented May 28, 2020

weissi commented May 28, 2020

dimitribouniol commented May 28, 2020

weissi commented Jun 1, 2020

dimitribouniol commented Jun 1, 2020

dimitribouniol commented Jun 1, 2020

dimitribouniol commented Jun 1, 2020

weissi commented Jun 1, 2020

weissi left a comment

Choose a reason for hiding this comment

dimitribouniol commented May 27, 2020 •

edited

Loading

dimitribouniol May 27, 2020 •

edited

Loading

dimitribouniol commented May 27, 2020 •

edited

Loading

Should we combine `host` and `socketPath`?