Header processing is too simplified for IPv6

[uablrek]

The code for extracting ports, and a pointer to the payload is too simplified:

kube-network-policies/pkg/networkpolicy/packet.go

Lines 77 to 84 in c7fc563

	// TCP, UDP and SCTP srcPort and dstPort are the first 4 bytes after the IP header
	t.srcPort = int(binary.BigEndian.Uint16(b[hdrlen : hdrlen+2]))
	t.dstPort = int(binary.BigEndian.Uint16(b[hdrlen+2 : hdrlen+4]))
	// Obtain the offset of the payload
	// TODO allow to filter by the payload
	if len(b) >= hdrlen+dataOffset {
	t.payload = b[hdrlen+dataOffset:]
	}

It assumes that there are no extension headers. This is mentioned in:

kube-network-policies/pkg/networkpolicy/packet.go

Line 57 in c7fc563

// NextHeader (not extension headers supported)

But should be documented, as it is a serious limitation. It must eventually be addressed, and then the copied packet length in:

kube-network-policies/pkg/networkpolicy/controller.go

Line 231 in c7fc563

MaxPacketLen: 128, // only interested in the headers

must be raised to 1280 (all extension headers must fit into a minimum sized IPv6 packet).

[uablrek]

I am unsure how fragments are handled. I don't think it's a problem since only the first packet of every connection is handled.

[aojea]

do you want to take it Lars?

[uablrek]

Yes. I would like to learn howto make bit manipulations in go (I have done it in C). The difficult thing is to check for too-short packets in every step.

/assign

[aojea]

must be raised to 1280 (all extension headers must fit into a minimum sized IPv6 packet).

yeah, I can see in multiple repos they initialize this value to 0xffff so now I'm thinking I may did some premature optimization here thinking that the less I copy the better, feel free to use a larger value you think is reasonable but please modify the stringer to not dump the entire packet ,

kube-network-policies/pkg/networkpolicy/packet.go

Line 23 in c7fc563

return fmt.Sprintf("%s:%d %s:%d %s\n%s", p.srcIP.String(), p.srcPort, p.dstIP.String(), p.dstPort, p.proto, hex.Dump(p.payload))

just the first X bits because is very useful for debugging , special for DNS or plain protocols

I0425 16:32:52.606641       8 controller.go:364] Evaluating packet [44] 10.64.2.44:34220 10.0.186.101:80 TCP
00000000  00 00 a0 02 7f 94 d0 ff  00 00 02 04 05 8c 04 02  |................|
00000010  08 0a 40 e1 ed 7b 00 00  00 00 01 03 03 07        |..@..{........|
I0425 16:32:52.606688       8 controller.go:389] [Packet 44] Egress NetworkPolicies: 0 Allowed: true
I0425 16:32:52.606845       8 controller.go:316] Finished syncing packet 44 took: 309.494µs accepted: true
I0425 16:32:52.666386       8 controller.go:301] Processing sync for packet 45
I0425 16:32:52.666465       8 controller.go:364] Evaluating packet [45] 10.64.2.52:45292 10.64.1.7:53 UDP
00000000  74 2d 73 65 72 76 69 63  65 2d 33 08 64 6e 73 2d  |t-service-3.dns-|
00000010  39 34 36 39 03 73 76 63  07 63 6c 75 73 74 65 72  |9469.svc.cluster|
00000020  05 6c 6f 63 61 6c 00 00  05 00 01 00 00 29 10 00  |.local.......)..|
00000030  00 00 00 00 00 0c 00 0a  00 08 32 bd 30 ed d5 05  |..........2.0...|
00000040  93 37                                             |.7|

[uablrek]

No, your optimization is correct. It should be 1280 if you only want headers. There is no need to copy 9000 byte jumbo frames in full to user-space. Beside performance, there is a much larger risk of filling the netlink socket buffer.

[uablrek]

@aojea I have looked at https://pkg.go.dev/github.com/google/gopacket, and thinking of investigate if it can be used in packet.go. Have you considered it and discarded it?

I see an opportunity to unit-test packet-parsing with actual real packet in pcap format (I did that in nfqloadbalancer). It is a very tedious work to manufacture packets by hand.

[aojea]

gopacket brings a lot of dependencies and increase the size of the binaries cilium/cilium#25980 ,

[uablrek]

Ok. It is probably possible to use pcap-captured packets for unit-test anyway.

[aojea]

I don't know if is possible to play with build tags, @BenTheElder is the expert on that

[BenTheElder]

If you only use it in _test.go that's like having a build-tag.

It will still be part of the dependency graph for the module.

Another option is to have a module used only at test time that isolates these tests, but then it can only test public APIs.

Ok. It is probably possible to use pcap-captured packets for unit-test anyway.

To me it sounds like we could grab some some real packets and stick that data in a test file statically?

You could also use gopacket to generate fake packets statically in an isolated tools module?

[uablrek]

To me it sounds like we could grab some some real packets and stick that data in a test file statically?
You could also use gopacket to generate fake packets statically in an isolated tools module?

Sounds like good options. All that's needed in _test are the byte[] data

[uablrek]

There is a use-case that makes this a kind/bug:

If an IPv6-UDP packet is fragmented it will have a fragment extension header before the UDP header. When unknown protocols are allowed (#49), then a fragmented IPV6-UDP packets will never be subject to network policies.

There should be an e2e test with fragmented UDP packets (which would fail for now)

/kind bug

[uablrek]

I wrote a simple program to convert pcap files to [][]byte slices. It's trivial (~60loc), but I don't know where to put it.

Example use:

./pcap2go --variable packetsUdpFragIPv6 ipv6-udp.pcap >> packet_test.go

pcap2go.go

package main

import (
	"flag"
	"fmt"
	"os"

	"github.com/google/gopacket"
	"github.com/google/gopacket/pcap"
)

func main() {
	variable := flag.String("variable", "pcap_packets", "Name of the variable")
	flag.Parse()
	if flag.NArg() < 1 {
		fmt.Println(`
pcap2go [options] <file>

Read a pcap-file and emit packets declared as go []byte
slices. Intended for including captured packets in unit tests.
`)
		flag.PrintDefaults()
		return
	}
	if err := readFile(flag.Arg(0), *variable); err != nil {
		fmt.Println(err)
		os.Exit(1)
	}
}

func readFile(file, variable string) error {
	handle, err := pcap.OpenOffline(file)
	if err != nil {
		return err
	}
	packetSource := gopacket.NewPacketSource(handle, handle.LinkType())
	fmt.Printf("var %s = [][]byte{\n", variable)
	for packet := range packetSource.Packets() {
		fmt.Printf("\t[]byte{\n")
		printBytes(packet.Data())
		fmt.Printf("\t},\n")
	}
	fmt.Printf("}\n")
	return nil
}
func printBytes(b []byte) {
	for i := 0; i < len(b); i++ {
		if (i % 16) == 0 {
			fmt.Printf("\t\t")
		}
		if (i % 16) == 15 {
			fmt.Printf("0x%02x,\n", b[i])
		} else {
			fmt.Printf("0x%02x, ", b[i])
		}
	}
	if (len(b) % 16) != 0 {
		fmt.Printf("\n")
	}
}

[aojea]

we use the hack/tools trick for these things https://pkg.go.dev/k8s.io/kubernetes/hack/tools

[uablrek]

I created https://github.com/uablrek/pcap2go, but I don't intend to add it to https://pkg.go.dev/k8s.io/kubernetes/hack/tools since it's so trivial (any programmer can recreate it from scratch in less than 1h), and it's not used in kubernetes, but in kubernetes-sigs.

Instead I plan to describe it in the testing/README.md file.

[BenTheElder]

I created https://github.com/uablrek/pcap2go, but I don't intend to add it to https://pkg.go.dev/k8s.io/kubernetes/hack/tools since it's so trivial (any programmer can recreate it from scratch in less than 1h), and it's not used in kubernetes, but in kubernetes-sigs.

Ah, miscommunication, k8s.io/kubernetes/hack/tools is just an example of a tools module, the suggestion is we could import it in a hack/tools/go.mod in this repo following the same pattern.

It sounds reasonable not to for now though