Add support for proxying in `WebsocketClient` #130

rnro · 2023-02-27T13:43:09Z

Added support for TLS and plain text proxying of Websocket traffic.

In the TLS case a CONNECT header is first sent establishing the proxied traffic.
In the plain text case the modified URI in the initial upgrade request header indicates to the proxy server that the traffic is to be proxied.
Use NIOWebSocketFrameAggregator to handle aggregating frame fragments. This brings with it more protections e.g. against memory exhaustion.
Accompanying config has been added to support this change.

This change also includes some performance gains by reducing the allocation and copies necessary to send ByteBuffer and ByteBufferView through WebSocket.send.

Sending ByteBuffer or ByteBufferView doesn’t require any allocation or copy of the data. Sending a String now correctly pre allocates the ByteBuffer if multibyte characters are present in the String.
Remove custom random websocket mask generation which would only generate bytes between UInt8.min..<UInt8.max, therefore excluding UInt8.max i.e. 255.

0xTim

Thanks for this! Could you add DocC comments to all the new public APIs? Thanks!

0xTim · 2023-03-01T10:17:55Z

Sources/WebSocketKit/WebSocket+Connect.swift

@@ -67,4 +67,68 @@ extension WebSocket {
            onUpgrade: onUpgrade
        )
    }
+
+    public static func connect(


Can you add DocC comments for the new public APIs? Thanks!

0xTim · 2023-03-01T10:18:30Z

Sources/WebSocketKit/WebSocketClient.swift

+        public var minNonFinalFragmentSize: Int
+        public var maxAccumulatedFrameCount: Int
+        public var maxAccumulatedFrameSize: Int
+


Can you add DocC comments describing what these are for?

0xTim · 2023-03-01T10:21:10Z

Tests/WebSocketKitTests/WebSocketKitTests.swift

+
+        let localWebsocketBin: WebsocketBin
+        let verifyProxyHead = { (ctx: ChannelHandlerContext, requestHead: HTTPRequestHead) in
+            XCTAssertEqual(requestHead.uri, "ws://apple.com/:\(ctx.localAddress!.port!)")


Just to confirm this is a simulated proxy and not something we're going out on the network for?

Yeah, we do a somewhat convoluted dance to insert and remove channel handlers to mimic a proxy e.g. intercepting the CONNECT request.

rnro · 2023-03-01T16:55:44Z

Thanks for the review @0xTim. I've pushed the changes you requested and also some performance improvements in a separate commit.

0xTim

Thanks for adding these! Few more places for DocC comments and then we're good to go!

0xTim · 2023-03-04T23:38:12Z

Sources/WebSocketKit/WebSocket+Connect.swift

+        )
+    }
+
+    public static func connect(


Missing comments

0xTim · 2023-03-04T23:38:30Z

Sources/WebSocketKit/WebSocket.swift

+        }
+    }
+
+    public func send(


Missing DocC comments

0xTim · 2023-03-04T23:38:56Z

Sources/WebSocketKit/WebSocketClient.swift

+        self.connect(scheme: scheme, host: host, port: port, path: path, query: query, headers: headers, proxy: nil, onUpgrade: onUpgrade)
+    }
+
+    public func connect(


Missing DocC comments

0xTim · 2023-03-04T23:43:06Z

Sources/WebSocketKit/WebSocketHandler.swift

@@ -2,27 +2,72 @@ import NIO
 import NIOWebSocket

 extension WebSocket {
+
+    public struct Configuration {


Missing DocC comments (and on the public functions in the type)

Added support for TLS and plain text proxying of Websocket traffic. In the TLS case a CONNECT header is first sent establishing the proxied traffic. In the plain text case the modified URI in the initial upgrade request header indicates to the proxy server that the traffic is to be proxied. Use `NIOWebSocketFrameAggregator` to handle aggregating frame fragments. This brings with it more protections e.g. against memory exhaustion. Accompanying config has been added to support this change.

Reduce allocation and copies necessary to send `ByteBuffer` and `ByteBufferView` through `WebSocket.send`. In fact sending `ByteBuffer` or `ByteBufferView` doesn’t require any allocation or copy of the data. Sending a `String` now correctly pre allocates the `ByteBuffer` if multibyte characters are present in the `String`. Remove custom random websocket mask generation which would only generate bytes between `UInt8.min..<UInt8.max`, therefore excluding `UInt8.max` aka `255`.

codecov-commenter · 2023-03-31T05:46:35Z

Codecov Report

Merging #130 (777ef32) into main (e0faccd) will decrease coverage by 3.62%.
The diff coverage is 78.80%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
- Coverage   71.15%   67.54%   -3.62%     
==========================================
  Files           5        6       +1     
  Lines         475      758     +283     
==========================================
+ Hits          338      512     +174     
- Misses        137      246     +109

Impacted Files	Coverage Δ
...bSocketKit/Concurrency/WebSocket+Concurrency.swift	`29.78% <ø> (ø)`
Sources/WebSocketKit/WebSocket+Connect.swift	`50.00% <36.84%> (-47.50%)`	⬇️
Sources/WebSocketKit/WebSocket.swift	`72.87% <77.41%> (+1.09%)`	⬆️
...urces/WebSocketKit/HTTPUpgradeRequestHandler.swift	`72.05% <77.77%> (ø)`
Sources/WebSocketKit/WebSocketHandler.swift	`61.97% <85.18%> (+6.65%)`	⬆️
Sources/WebSocketKit/WebSocketClient.swift	`86.00% <90.55%> (+18.03%)`	⬆️

rnro · 2023-03-31T05:49:05Z

Sorry about the delay. The missing DocC comments should now be there.

0xTim

Thanks!

VaporBot · 2023-04-11T14:34:56Z

These changes are now available in 2.8.0

0xTim · 2023-04-11T15:34:40Z

FYI nightly error is being tracked at swiftlang/swift#65064

0xTim requested changes Mar 1, 2023

View reviewed changes

rnro force-pushed the proxy_support branch from 0ca2f89 to 00f048d Compare March 1, 2023 16:57

rnro requested a review from 0xTim March 1, 2023 17:02

rnro force-pushed the proxy_support branch from 00f048d to 7c9f537 Compare March 1, 2023 18:03

0xTim requested changes Mar 4, 2023

View reviewed changes

rnro added 4 commits March 7, 2023 14:21

add DocC comments

e5cfcea

DocC comments for new APIs

777ef32

rnro force-pushed the proxy_support branch from 7c9f537 to 777ef32 Compare March 31, 2023 05:46

rnro requested a review from 0xTim March 31, 2023 05:46

0xTim approved these changes Apr 11, 2023

View reviewed changes

0xTim added the semver-minor Contains new APIs label Apr 11, 2023

0xTim changed the title ~~WebsocketClient supports proxying~~ Add support for proxying in WebsocketClient Apr 11, 2023

0xTim merged commit 2166cbe into vapor:main Apr 11, 2023

rnro deleted the proxy_support branch April 11, 2023 14:52

0xTim mentioned this pull request May 25, 2023

Example of using websocket-kit with system proxy? #110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for proxying in `WebsocketClient` #130

Add support for proxying in `WebsocketClient` #130

rnro commented Feb 27, 2023 •

edited

Loading

0xTim left a comment

0xTim Mar 1, 2023

0xTim Mar 1, 2023

0xTim Mar 1, 2023

rnro Mar 1, 2023

rnro commented Mar 1, 2023

0xTim left a comment

0xTim Mar 4, 2023

0xTim Mar 4, 2023

0xTim Mar 4, 2023

0xTim Mar 4, 2023

codecov-commenter commented Mar 31, 2023 •

edited

Loading

rnro commented Mar 31, 2023

0xTim left a comment

VaporBot commented Apr 11, 2023

0xTim commented Apr 11, 2023

Add support for proxying in WebsocketClient #130

Add support for proxying in WebsocketClient #130

Conversation

rnro commented Feb 27, 2023 • edited Loading

0xTim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnro commented Mar 1, 2023

0xTim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 31, 2023 • edited Loading

Codecov Report

rnro commented Mar 31, 2023

0xTim left a comment

Choose a reason for hiding this comment

VaporBot commented Apr 11, 2023

0xTim commented Apr 11, 2023

Add support for proxying in `WebsocketClient` #130

Add support for proxying in `WebsocketClient` #130

rnro commented Feb 27, 2023 •

edited

Loading

codecov-commenter commented Mar 31, 2023 •

edited

Loading