Parse IPv6 extension headers #51

uablrek · 2024-07-05T13:54:36Z

IPv6 extension headers must be parsed, at least fragment headers.

Fixes: #15

uablrek · 2024-07-05T13:56:02Z

A unit test is added, that currently fails, but high-lite the fragmented IPv6-UPD problem.

uablrek · 2024-07-05T15:10:57Z

The extension packets are assumed to be in the recommended order, i.e. a fragment header is the last one, if it exist.

The IPPROTO_HOPOPTS, IPPROTO_ROUTING and IPPROTO_DSTOPTS extension headers are not tested. It is non-trivial to generate these header for testing.

uablrek · 2024-07-05T15:12:48Z

/cc @aojea

uablrek · 2024-07-05T15:14:56Z

The capture-chunk is still 128 byte. I think that the fragment case is the most important, and that doesn't require more

uablrek · 2024-07-06T05:59:35Z

There may be problems also with IPv4 fragments. I think they will be handled as un-fragmented packets, but the L4-header is missing, so garbage data will be used. I will test it and rename this PR to something like "Parse IPv6 extension headers and handle fragments", if necessary.

I make this PR a "draft" until things are sorted out

uablrek · 2024-07-06T06:07:08Z

FYI https://github.com/uablrek/pcap2go is used to create packet test-data

pkg/networkpolicy/packet.go

aojea · 2024-07-06T11:10:35Z

pkg/networkpolicy/packet_test.go

+	}
+}
+
+var packetsUdpFragIPv6 = [][]byte{


add a comment explaining how this is generated so future us can create more of these payloads

Comment added in packet_test.go, and pcap files added in test-data/

better add it under the same folder pkg/networkpolicy/test-data/ and add a small README.md doc in the folder explaining https://github.com/uablrek/pcap2go

uablrek · 2024-07-06T14:26:36Z

There may be problems also with IPv4 fragments.

Probably not, because when conntrack is used (which it always is in K8s), IPv4 packets are reassembled. (BTW the answer is very informative)

uablrek · 2024-07-06T14:45:04Z

Considering the above 👆, shall we assume that no IPv4 fragments are ever delivered to user-space, or should we be prepared for the very unlikely event?

aojea · 2024-07-06T15:21:53Z

Considering the above 👆, shall we assume that no IPv4 fragments are ever delivered to user-space, or should we be prepared for the very unlikely event?

seems reasonable, but leave a comment with the link to that great explanation to document the reason

uablrek · 2024-07-07T07:47:47Z

I added checks for packets with incomplete headers and corrupted packets (header lengths too small). But the kernel will verify this and discard those packets. If the capture length is changed 128->1280, all those checks should be redundant.

@aojea Do you think I should set capture length 1280, and remove all too-small and corrupted tests?

(never mind the lint-errors, I'll fix them later)

uablrek · 2024-07-07T14:56:35Z

IPPROTO_DSTOPTS may appear again after IPPROTO_FRAGMENT in the recommended order. I will fix that when I find a way to test it.

danwinship · 2024-07-08T15:41:03Z

pkg/networkpolicy/packet_test.go

+// in the nfqueue
+const captureChunk = 128
+
+func TestUdpFragmentIPv6(t *testing.T) {


"UDP" not "Udp". Golang style doesn't force acronyms to "titlecase" (though they can be all-lowercase if needed).

(Likewise everywhere.)

danwinship · 2024-07-08T15:42:33Z

pkg/networkpolicy/packet_test.go

+	}{
+		{
+			name:  "UDP first fragment",
+			input: packetsUdpFragIPv6[0][14:(14 + captureChunk)],


14 should not be a magic number

danwinship · 2024-07-08T15:43:26Z

pkg/networkpolicy/packet_test.go

+	}
+}
+
+var packetsUdpFragIPv6 = [][]byte{


seems like this would make more sense as three separate variables with useful names.
also, maybe split into L2 header, L3 header, payload, with some minimal comments?

I disagree. If packet test data is generated from pcap, it should not be post-processed. That would make it an exquisite pain to create new test data (exactly what I wanted to avoid by using a capture).

On the other hand, if test-data is manufactured by hand, there should of course be comments

I saved the original pcap files in test-data/ so people can analyze it with wireshark for instance

danwinship · 2024-07-08T15:51:18Z

pkg/networkpolicy/packet_test.go

+			},
+		},
+		{
+			name:  "PSH, ACK",


we should only be receiving packets for ctstate new connections...

Yeah, but IMHO parsePacket() should be more-or-less generic. It should not panic if some TCP packet arrives that is not a SYN for instance. I think "test too much" here should be ok.

uablrek · 2024-07-09T08:42:12Z

TL;DR; SRv6 is a horrible edge use-case!

In my quest for IPv6 packets with extension headers I revisited SRv6. I tested it after viewing an excellent presentation by Daniel Bernier, Bell Canada at KubeCon in Valencia 2022.

With SRv6 there is a routing header, followed by another IPv6 header, and then the TCP header:

This will probably never happen, but network-policies is in the forwarding plane, so it might be possible so see something like this.

As I see it we have two choices:

Parse this and return addresses from the second IPv6 header, and detect proto TCP
Reject as "not supported"

The second can be "reject any packet with a routing header".

aojea · 2024-07-09T10:13:00Z

@aojea Do you think I should set capture length 1280, and remove all too-small and corrupted tests?

I prefer to make the code more resilient than depending on the kernel to make the right thing

There are some linter errors and some job failing that I do not know if is a flake, but this should be ready for review

uablrek · 2024-07-09T12:47:46Z

I made the PR "ready for review". IPv6 fragments are handled, but there are some "exotic" cases that should also be covered, like SRv6, and repeated destopts. Perhaps extension headers that are not in the recommended order, but I rather reject those. Also unit-tests for extension headers (except fragments) are missing.

But since IPv6 fragment handling is most urgent, and may be considered a bug, the remaining stuff can be added in a later kind/feature PR. I'll be happy to continue working with that.

aojea · 2024-07-16T21:51:40Z

pkg/networkpolicy/packet.go

 	}

-	var dataOffset int
+	havePorts := true


when is this false? preparing for icmp?

aojea · 2024-07-16T21:52:12Z

pkg/networkpolicy/packet.go

+			} else {
+				// Not first fragment, we have no L4 header and the
+				// payload begins after this header
+				l4offset = -1


is protocol = nxtHeader correct in this case? it will be syscall.IPPROTO_FRAGMENT

l4offset = -1. does not seem to be handled later, should this return here?

is protocol = nxtHeader correct in this case? it will be syscall.IPPROTO_FRAGMENT

It's intentional, but strictly not correct since there is an L4 protocol in the nxtHeader field. But there is no L4 header! The fragment data begins immediately after the fragment header. So to identify the packet as a fragment, syscall.IPPROTO_FRAGMENT is kept in the protocol variable. I'll make a comment.

l4offset = -1. does not seem to be handled later, should this return here?

It's probably a just-in-case left-over. I'll check it.

uablrek · 2024-07-17T09:26:01Z

Updated after comments.

I figured that we can return an empty packet.proto for fragments, and for any other packets we want to accept. The default is however still to return an error for non tcp/udp/sctp. I was uncertain about this, since it requires that only tcp/udp/sctp packets are routed to the nfqueue.

The alternative is to return an empty packet.proto for all packets we want to accept, not only fragments. If we do that, no "allow unknown protocols" pr is needed. The control is in parsePacket().

I have prepared a commit that rejects IPPROTO_ROUTING headers, and handle the remaining case for headers in the recommended order: extra IPPROTO_DSTOPTS headers after the first fragment header. But it needs rebase after these updates. I also add a ipproto field in the packet struct. Not really needed, but "good to have" for logging and future extensions.

uablrek · 2024-07-17T09:43:39Z

I am still working on tests, both unit-test and real traffic tests, with IPv6 extension headers. I have not figured out an easy way, but am planning to use a tap device that adds some IPv6 extension headers. I.e. packets are routed to a tap device, a user-space program add some headers, and send the packets back. I have done it before, and got quite far. IMO tap devices, same as nfqueue, are often overlooked for feature implementations and testing. A hyped alternative is eBPF, but that is much more complicated, and should be avoided unless performance is really needed.

A good example of the power of tap devices is tayga NAT64 (rock-solid since 2011!)

uablrek · 2024-07-17T10:41:20Z

I discovered that lksctp (Linux kernel SCTP) doesn't fragment on IP-level (this was new to me). The MTU of the outgoing device is examined, and fragmentation is made on SCTP level. This means that IPv6 fragments will never be used for lksctp. I don't know about other sctp implementations, but I will only test lksctp.

aojea · 2024-07-17T13:20:02Z

pkg/networkpolicy/controller.go

+			c.nfq.SetVerdict(*a.PacketID, nfqueue.NfAccept) //nolint:errcheck
+			return 0


Dan and I were discussing offline, and we concluded that:

network policies only care about L4 protocol if you set the Ports fields

if not Port field is specified , is legit to assume that the user/admin can consider the network policy as "any" protocol (some from sftim about users wanting to control ICMP is spot on)

As conclusion, I think we set the packet.proto to empty , then srcPort and dstPort will be 0, and IIUIC the network policy logic will be able do the right thing.

This leaves the parsePacket() errors to short or malformed packets (module bugs) than we can say we deny by default , as we don't expect at this point to receive a packet from the kernel that is malformed

can you just remove this switch case?

And just check for an empty v1.Protocol? What is the best if-condition:

if string(packet.proto) == ""

?

(I couldn't figure that out, hence the switch)

oh, not check at all , just pass the packet as is to the next stage

c.evaluatePacket(packet) will evaluate the packet only with the IPs information

aojea · 2024-07-17T13:21:53Z

pkg/networkpolicy/packet.go

+		// This "should not happen" since only { tcp, udp, sctp } are
+		// routed to the nfqueue
 		return t, fmt.Errorf("unknown protocol %d", protocol)


we are going to route any protocol here, so we can emit verdict on icmp per example, more on this in https://github.com/kubernetes-sigs/kube-network-policies/pull/51/files#r1681040192

aojea · 2024-07-18T18:26:26Z

pkg/networkpolicy/packet.go

+	if payloadOffset > len(b) {
+		// We must have a complete L4 header, however payload may be zero-lengh
+		return t, ErrorTooShort
+	}


is this invalid? why this should be an error ?

ah, ok, if payloadOffset is equal to len(b) is a valid packet

aojea · 2024-07-18T18:28:39Z

pkg/networkpolicy/packet.go

 	// Obtain the offset of the payload
 	// TODO allow to filter by the payload
-	if len(b) >= hdrlen+dataOffset {
-		t.payload = b[hdrlen+dataOffset:]
+	if payloadOffset > 0 {


can this happen? I don't know if I prefer the old check

dataOffset is a tcp-header thing. It should be used to compute the hdrlen for tcp-headers only. I think there was a bug in the old code for sctp and udp payloads. I looked that the hdrlen was added twice, since it was included in dataOffset (but I didn't really check carefully, since I re-wrote it)

Oh, payloadOffset must be >0 now since we return in the default: branch. I can remove the check.

And add parsePacket unit test

aojea · 2024-07-18T19:40:31Z

/lgtm
/approve

Thanks

k8s-ci-robot · 2024-07-18T19:40:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, uablrek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aojea]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2024

k8s-ci-robot requested a review from danwinship July 5, 2024 13:54

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 5, 2024

k8s-ci-robot requested a review from thockin July 5, 2024 13:54

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 5, 2024

k8s-ci-robot requested a review from aojea July 5, 2024 15:12

uablrek marked this pull request as ready for review July 5, 2024 15:15

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2024

uablrek marked this pull request as draft July 6, 2024 06:00

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2024

aojea reviewed Jul 6, 2024

View reviewed changes

pkg/networkpolicy/packet.go Show resolved Hide resolved

aojea reviewed Jul 6, 2024

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 7, 2024

danwinship reviewed Jul 8, 2024

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 9, 2024

uablrek mentioned this pull request Jul 9, 2024

allow unknown protocols #49

Closed

uablrek force-pushed the packet-parse branch from 4f8ecfa to 169cc67 Compare July 9, 2024 12:25

uablrek marked this pull request as ready for review July 9, 2024 12:47

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2024

k8s-ci-robot requested a review from aojea July 9, 2024 12:47

uablrek force-pushed the packet-parse branch from 169cc67 to 004d787 Compare July 10, 2024 09:24

aojea reviewed Jul 16, 2024

View reviewed changes

uablrek force-pushed the packet-parse branch from 004d787 to b9ba4e0 Compare July 17, 2024 09:12

aojea reviewed Jul 17, 2024

View reviewed changes

uablrek force-pushed the packet-parse branch 2 times, most recently from 3c6b0ad to 59745d1 Compare July 18, 2024 18:21

aojea reviewed Jul 18, 2024

View reviewed changes

uablrek force-pushed the packet-parse branch from 59745d1 to b30eee1 Compare July 18, 2024 18:51

Handle IPv6 extensions headers

10849b1

And add parsePacket unit test

uablrek force-pushed the packet-parse branch from b30eee1 to 10849b1 Compare July 18, 2024 19:14

k8s-ci-robot assigned aojea Jul 18, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2024

k8s-ci-robot merged commit 17360aa into kubernetes-sigs:main Jul 18, 2024
9 checks passed

uablrek deleted the packet-parse branch July 19, 2024 02:15

uablrek mentioned this pull request Jul 19, 2024

Reject unsupported IPv6 extension headers #55

Draft

		c.nfq.SetVerdict(*a.PacketID, nfqueue.NfAccept) //nolint:errcheck
		return 0

Parse IPv6 extension headers #51

Parse IPv6 extension headers #51

Conversation

uablrek commented Jul 5, 2024

uablrek commented Jul 5, 2024

uablrek commented Jul 5, 2024 • edited Loading

uablrek commented Jul 5, 2024

uablrek commented Jul 5, 2024

uablrek commented Jul 6, 2024 • edited Loading

uablrek commented Jul 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uablrek commented Jul 6, 2024 • edited Loading

uablrek commented Jul 6, 2024 • edited Loading

aojea commented Jul 6, 2024

uablrek commented Jul 7, 2024 • edited Loading

uablrek commented Jul 7, 2024

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

uablrek commented Jul 9, 2024

aojea commented Jul 9, 2024

uablrek commented Jul 9, 2024

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

uablrek commented Jul 17, 2024

uablrek commented Jul 17, 2024

uablrek commented Jul 17, 2024

aojea Jul 17, 2024 • 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

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

aojea commented Jul 18, 2024

k8s-ci-robot commented Jul 18, 2024

uablrek commented Jul 5, 2024 •

edited

Loading

uablrek commented Jul 6, 2024 •

edited

Loading

uablrek commented Jul 6, 2024 •

edited

Loading

uablrek commented Jul 6, 2024 •

edited

Loading

uablrek commented Jul 7, 2024 •

edited

Loading

aojea Jul 17, 2024 •

edited

Loading