Header processing is too simplified for IPv6

[uablrek]

The code for extracting ports, and a pointer to the payload is too simplified: https://github.com/kubernetes-sigs/kube-network-policies/blob/c7fc563db4e9f011d200da021b59098ac02e9bdb/pkg/networkpolicy/packet.go#L77-L84

It assumes that there are no extension headers. This is mentioned in: https://github.com/kubernetes-sigs/kube-network-policies/blob/c7fc563db4e9f011d200da021b59098ac02e9bdb/pkg/networkpolicy/packet.go#L57

But should be documented, as it is a serious limitation. It must eventually be addressed, and then the copied packet length in: https://github.com/kubernetes-sigs/kube-network-policies/blob/c7fc563db4e9f011d200da021b59098ac02e9bdb/pkg/networkpolicy/controller.go#L231

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 ,

https://github.com/kubernetes-sigs/kube-network-policies/blob/c7fc563db4e9f011d200da021b59098ac02e9bdb/pkg/networkpolicy/packet.go#L23

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 https://github.com/cilium/cilium/issues/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

```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] 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