Add `RawSocketBootstrap` #2320

dnadoba · 2022-11-22T14:21:16Z

Motivation:

We want to support custom IP protocols through POSIX Raw Socket API. To allow this, users will need to bootstrap a DatagramChannel with a socket in raw mode.

Modifications:

add RawSocketBoostrap which configures a DatagramChannel with a raw socket.
add end-to-end test

Result:

NIO can send and receive IP packets.

Alternatives

RawSocketBootstrap is inspired by the socket mode name (SOCK_RAW). Instead we could also call it IPBootstrap to reflect that the resulting channel will operate on IP packets.

Lukasa

Thanks, this looks great! I think we need a few extra tests though, as we're currently not coming close to testing all the new code that was added here.

Tests/NIOPosixTests/RawSocketBootstrapTests.swift

Lukasa · 2022-11-22T14:30:15Z

Tests/NIOPosixTests/RawSocketBootstrapTests.swift

+
+        let elg = MultiThreadedEventLoopGroup(numberOfThreads: 1)
+        defer { XCTAssertNoThrow(try elg.syncShutdownGracefully()) }
+        let channel = try RawSocketBootstrap(group: elg)


We probably need a test of the ip_hdrincl path.

Tested now in RawSocketBootstrapTests.testIpHdrincl

Lukasa · 2022-11-22T14:30:34Z

Tests/NIOPosixTests/RawSocketBootstrapTests.swift

+
+        let receivedMessages = Set(try channel.waitForDatagrams(count: 10).map(\.data).map { buffer in
+            String(
+                decoding: buffer.readableBytesView.dropFirst(4 * 5), // skip the IPv4 header


Ideally we'd actually validate that this is an IPv4 header.

Tested in now in all tests of RawSocketBootstrapTests

…ctually create a socket and skipping if we fail because of `EPERM`

Sources/NIOPosix/RawSocketBootstrap.swift

weissi · 2022-11-25T17:33:33Z

Tests/NIOPosixTests/IPv4Header.swift

+            let headerChecksum: UInt16 = buffer.readInteger(),
+            let sourceIpAddress: UInt32 = buffer.readInteger(),
+            let destinationIpAddress: UInt32 = buffer.readInteger()
+        else { return nil }


this is incorrect because it forgets to reset the buffer if you half-parsed something.

Also this doesn't follow the usual pattern of read*/write* methods. And lastly, there's an implementation of this already:

check PCAP.swift on this PR: more PCAP swift-nio-extras#158 : https://github.com/apple/swift-nio-extras/pull/158/files#diff-8b0f17fa27d694721788c7adab20151f44ff3672becc16dd728bd530c0de44ba

also here (this code is changed by the above PR): https://github.com/apple/swift-nio-extras/blob/main/Tests/NIOExtrasTests/WritePCAPHandlerTest.swift#L746

Done in 20ed380

Tests/NIOPosixTests/IPv4Header.swift

weissi · 2022-11-25T17:37:38Z

Tests/NIOPosixTests/RawSocketBootstrapTests.swift

+            // like we do.
+            XCTAssertEqual(Int(header.totalLength), data.readableBytes)
+            // On BSD the checksum is always zero
+            XCTAssertEqual(header.headerChecksum, 0)


I don't think we should make this platform specific. I think we should have a bsdHeaderChecksum and bsdTotalLength etc properties. And maybe maybe a headerChecksumForPlatformRawSockets or so properties that then (platform specifically) return the correct one.

I thought about this too but couldn't come up with good names. These properties should return the correct value (as specified in RFC-791). The problem though is not only naming but also that the workaround (i.e. adding 20 to totalLength) requires knowing if we have just parsed the IPv4 packet from a ByteBuffer we got from a raw socket or not. If we create a IPv4Header ourselves e.g. for sending through the Raw Socket API, the totalLength will be correct and adding 20 will result in a wrong value.

The options I see are:

store this information in the type and dynamically return the correct value

This works awful if we reuse the same value to e.g. echo a packet

use different types for receiving and sending.

stores this information statically and we will need to explicitly convert the type when we want to echo a packet where we can get the totalLength fixed up

use some rather long name which conveys that this is only for the receiving side e.g. platformIndependetTotalLengthForRecivedPacket

add some documentation and leave it to the call site to handle it

I kind of want to use the "rather long name" variant but don't yet have name I like much.

I also don't think we need to worry too much about this, as this is testing code. It's ok if the parsing is a bit janky.

I want to eventually make IPv4Header public API so we can still discuss this here. However, I will make this in a separate PR so it is not blocking RawSocketBootstrap to land.

I'd go for some long names to explain this. And +1 for the public API, this should be reconciled with the already-existing IPv4/6 parsing code in swift-nio-extras.

Added them in fabaf9b

Lukasa

I'm happy enough with this change, but it'd be good to confirm that @weissi is happy too.

Tests/NIOPosixTests/IPv4Header.swift

weissi · 2022-11-30T11:44:34Z

Tests/NIOPosixTests/IPv4Header.swift

+
+extension ByteBuffer {
+    @discardableResult
+    mutating func readIPv4Header() -> IPv4Header? {


As discussed offline this should IMHO really not be platform specific.
I think we should have readIPv4Header which reads it correctly. And then we can have another one which is readIPv4HeaderWithDarwinRawSocketQuirks() or something which does the weird thing.
Finally we can then have a readIPv4HeaderFromOSRawSocket() or something which chooses (in a platform specific way) the right one for the platform

But any readFoo that can't read something written by a writeFoo even if on a different platform is a major red flag

Is there a reason someone would want readIPv4HeaderWithDarwinRawSocketQuirks on non Darwin/BSD platforms?

Yes, unless impossible we should offer everything on every platform. There's no reason not to have it.

There's no reason we shouldn't unit test the darwin quirks on Linux too.

Added in 1ac14d2

finagolfin · 2022-12-02T14:18:34Z

Tests/NIOPosixTests/IPv4Header.swift

+        // like e.g. we do now too.
+        return totalLength + 20
+        #elseif os(Linux)
+        return totalLength


This pull broke building the tests for Android. I just confirmed that adding Android here fixes it again, finagolfin/swift-android-sdk#83.

@dnadoba, mind adding these Android checks in a subsequent commit?

Oh yeah sure, sorry about that. I think we can also just remove os(Linux) and just use an #else

Done in #2328

dnadoba added the semver/minor Adds new public API. label Nov 22, 2022

dnadoba requested a review from Lukasa November 22, 2022 14:21

Lukasa requested changes Nov 22, 2022

View reviewed changes

dnadoba added 3 commits November 23, 2022 22:46

Add RawSocketBootsrap

5f0176c

Add more tests

2decd8d

Add IPv4Header and tests for .ip_hdrincl

5b2dfd6

dnadoba force-pushed the dn-raw-socket branch from 0498fed to 5b2dfd6 Compare November 23, 2022 21:46

dnadoba added 2 commits November 23, 2022 22:49

generate_linux_tests.rb

bb5c1b4

Check if we have enough rights for the raw sockets API by trying to a…

26e5c14

…ctually create a socket and skipping if we fail because of `EPERM`

dnadoba requested a review from Lukasa November 24, 2022 10:32

dnadoba added 2 commits November 24, 2022 11:46

generate_linux_tests.rb

330226c

Make capitalization consistent

be58e9e

weissi reviewed Nov 25, 2022

View reviewed changes

Sources/NIOPosix/RawSocketBootstrap.swift Outdated Show resolved Hide resolved

weissi reviewed Nov 25, 2022

View reviewed changes

Tests/NIOPosixTests/IPv4Header.swift Outdated Show resolved Hide resolved

weissi reviewed Nov 25, 2022

View reviewed changes

dnadoba added 2 commits November 28, 2022 10:48

Use write/read methods on ByteBuffer

20ed380

Add NIO prefix to RawSocketBootstrap

981d0b8

Lukasa approved these changes Nov 28, 2022

View reviewed changes

weissi reviewed Nov 28, 2022

View reviewed changes

Tests/NIOPosixTests/IPv4Header.swift Outdated Show resolved Hide resolved

dnadoba added 2 commits November 28, 2022 18:20

Fix review comments

fabaf9b

Merge branch 'main' into dn-raw-socket

6779f72

dnadoba requested a review from weissi November 29, 2022 07:48

weissi reviewed Nov 30, 2022

View reviewed changes

dnadoba added 2 commits November 30, 2022 14:38

Add readIPv4HeaderFromOSRawSocket and writeIPv4HeaderToOSRawSocket

1ac14d2

Add read/writeIPv4HeaderFromBSDRawSocket

1ad4dc4

weissi approved these changes Dec 1, 2022

View reviewed changes

dnadoba merged commit 810544e into apple:main Dec 1, 2022

dnadoba deleted the dn-raw-socket branch December 1, 2022 14:35

dnadoba mentioned this pull request Dec 1, 2022

Add a raw socket API #2286

Closed

finagolfin reviewed Dec 2, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `RawSocketBootstrap` #2320

Add `RawSocketBootstrap` #2320

dnadoba commented Nov 22, 2022

Lukasa left a comment

Lukasa Nov 22, 2022

dnadoba Nov 23, 2022

Lukasa Nov 22, 2022

dnadoba Nov 23, 2022

weissi Nov 25, 2022

dnadoba Nov 28, 2022

weissi Nov 25, 2022

dnadoba Nov 28, 2022

Lukasa Nov 28, 2022

dnadoba Nov 28, 2022

weissi Nov 28, 2022

dnadoba Nov 28, 2022

Lukasa left a comment

weissi Nov 30, 2022

dnadoba Nov 30, 2022 •

edited

Loading

weissi Nov 30, 2022

weissi Nov 30, 2022

dnadoba Nov 30, 2022

finagolfin Dec 2, 2022

dnadoba Dec 2, 2022

dnadoba Dec 2, 2022

Add RawSocketBootstrap #2320

Add RawSocketBootstrap #2320

Conversation

dnadoba commented Nov 22, 2022

Motivation:

Modifications:

Result:

Alternatives

Lukasa 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnadoba Nov 30, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `RawSocketBootstrap` #2320

Add `RawSocketBootstrap` #2320

dnadoba Nov 30, 2022 •

edited

Loading