Merge pull request #170 from datum-cloud/bug/kernel-state-validation

fix: validate kernel state against prevResult in cmdCheck

Author: privateip

internal/cni/cni_test.go

@@ -662,6 +662,66 @@ func TestCmdCheckMissingVPC(t *testing.T) {
 	}
 }
 
+func TestCmdCheckWithPrevResultMissingResources(t *testing.T) {
+	// Build a prevResult matching what buildResult produces for veth mode.
+	prevResult := `{` +
+		`"cniVersion":"1.0.0",` +
+		`"interfaces":[` +
+		`{"name":"galactic-abc-def","mac":"aa:bb:cc:dd:ee:01","mtu":1500,"sandbox":""},` +
+		`{"name":"galactic-def-abc","mac":"aa:bb:cc:dd:ee:02","mtu":1500,"sandbox":"/proc/1/ns/net"}` +
+		`],` +
+		`"ips":[` +
+		`{"version":"6","address":"fd00:10:ff01::1234/80","gateway":"fd00:10:ff01::1","interface":1}` +
+		`]}`
+	conf := fmt.Sprintf(
+		`{"cniVersion":"1.0.0","name":"test",`+
+			`"type":"galactic-cni","vpc":"%s",`+
+			`"vpcattachment":"%s",`+
+			`"prevResult":%s}`,
+		testVPC, testAttachment, prevResult,
+	)
+	args := &skel.CmdArgs{
+		ContainerID: testContainerID,
+		Netns:       "/proc/1/ns/net",
+		StdinData:   []byte(conf),
+	}
+
+	err := cmdCheck(args)
+	if err == nil {
+		t.Fatalf("expected CHECK failure for missing resources, got nil")
+	}
+	if !strings.Contains(err.Error(), "CHECK failed") {
+		t.Fatalf("error %q does not contain 'CHECK failed'", err.Error())
+	}
+	// prevResult parsing should succeed; errors come from missing kernel state.
+	if !strings.Contains(err.Error(), "prevResult validation") {
+		t.Fatalf("error %q does not contain 'prevResult validation'", err.Error())
+	}
+}
+
+func TestCmdCheckWithInvalidPrevResult(t *testing.T) {
+	// prevResult that is structurally valid JSON but not a valid CNI result.
+	conf := fmt.Sprintf(
+		`{"cniVersion":"1.0.0","name":"test",`+
+			`"type":"galactic-cni","vpc":"%s",`+
+			`"vpcattachment":"%s",`+
+			`"prevResult":{"not":"a valid cni result"}}`,
+		testVPC, testAttachment,
+	)
+	args := &skel.CmdArgs{
+		ContainerID: testContainerID,
+		StdinData:   []byte(conf),
+	}
+
+	err := cmdCheck(args)
+	if err == nil {
+		t.Fatalf("expected CHECK failure for invalid prevResult, got nil")
+	}
+	if !strings.Contains(err.Error(), "prevResult validation") {
+		t.Fatalf("error %q does not contain 'prevResult validation'", err.Error())
+	}
+}
+
 // ---- resourceTracker ------------------------------------------------------
 
 func TestResourceTrackerCleanupZeroValue(t *testing.T) {

internal/cni/ops_check.go

@@ -6,13 +6,15 @@ package cni
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"net"
 	"net/http"
 	"time"
 
 	"github.com/containernetworking/cni/pkg/skel"
+	type100 "github.com/containernetworking/cni/pkg/types/100"
 	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/vishvananda/netlink"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -57,6 +59,13 @@ func cmdCheck(args *skel.CmdArgs) error {
 		}
 	}
 
+	// Validate kernel state against prevResult (CNI spec §4.3).
+	if pluginConf.RawPrevResult != nil {
+		if err := checkPrevResult(pluginConf.RawPrevResult, hostName, args.Netns); err != nil {
+			errs = append(errs, fmt.Errorf("prevResult validation: %w", err))
+		}
+	}
+
 	if len(errs) > 0 {
 		return fmt.Errorf("CHECK failed: %w", errors.Join(errs...))
 	}
@@ -196,3 +205,145 @@ func checkTerminationRoutes(vpc, vpcAttachment string, terminations []Terminatio
 	}
 	return nil
 }
+
+// checkPrevResult validates that kernel state matches the interfaces and IPs
+// recorded in the prevResult returned by the most recent ADD. Per the CNI spec
+// §4.3, CHECK must verify that managed resources have not drifted.
+func checkPrevResult(rawPrevResult map[string]interface{}, _ string, netns string) error {
+	// RawPrevResult is map[string]interface{} — marshal back to JSON, then
+	// parse as a versioned CNI result.
+	jsonBytes, err := json.Marshal(rawPrevResult)
+	if err != nil {
+		return fmt.Errorf("marshal prevResult: %w", err)
+	}
+	res, err := type100.NewResult(jsonBytes)
+	if err != nil {
+		return fmt.Errorf("parse prevResult: %w", err)
+	}
+	result, err := type100.GetResult(res)
+	if err != nil {
+		return fmt.Errorf("get prevResult: %w", err)
+	}
+
+	// Validate each interface declared in prevResult against the kernel.
+	for _, iface := range result.Interfaces {
+		if iface.Name == "" {
+			continue
+		}
+
+		// Host-side interface: validate MAC and MTU from the host namespace.
+		if iface.Sandbox == "" {
+			if err := validateHostInterface(iface.Name, iface.Mac, iface.Mtu); err != nil {
+				return fmt.Errorf("interface %q (host): %w", iface.Name, err)
+			}
+			continue
+		}
+
+		// Guest-side interface: validate MAC and MTU from inside the container netns.
+		if err := validateGuestInterface(iface.Name, iface.Mac, iface.Mtu, netns); err != nil {
+			return fmt.Errorf("interface %q (guest): %w", iface.Name, err)
+		}
+	}
+
+	// Validate each IP assignment against the kernel.
+	for _, ipConfig := range result.IPs {
+		if ipConfig.Interface == nil {
+			continue
+		}
+		idx := *ipConfig.Interface
+		if idx < 0 || idx >= len(result.Interfaces) {
+			return fmt.Errorf("ipConfig interface index %d out of range [0, %d)", idx, len(result.Interfaces))
+		}
+		targetIface := result.Interfaces[idx]
+		if targetIface.Sandbox == "" {
+			// Host-side IP — not expected in our plugin, but skip gracefully.
+			continue
+		}
+		if err := validateIPOnInterface(ipConfig.Address.IP, targetIface.Name, netns); err != nil {
+			return fmt.Errorf("ip %s on %q (guest): %w", ipConfig.Address.String(), targetIface.Name, err)
+		}
+	}
+
+	return nil
+}
+
+// validateHostInterface checks that a host-side interface's MAC and MTU match
+// the values recorded in prevResult.
+func validateHostInterface(name, wantMac string, wantMtu int) error {
+	link, err := netlink.LinkByName(name)
+	if err != nil {
+		return fmt.Errorf("find link: %w", err)
+	}
+	if wantMac != "" && link.Attrs().HardwareAddr.String() != wantMac {
+		return fmt.Errorf("MAC mismatch: expected %q, got %q", wantMac, link.Attrs().HardwareAddr.String())
+	}
+	if wantMtu > 0 && link.Attrs().MTU != wantMtu {
+		return fmt.Errorf("MTU mismatch: expected %d, got %d", wantMtu, link.Attrs().MTU)
+	}
+	return nil
+}
+
+// validateGuestInterface checks that a guest-side interface's MAC and MTU match
+// the values recorded in prevResult, reading from inside the container netns.
+func validateGuestInterface(name, wantMac string, wantMtu int, netns string) error {
+	containerNS, err := ns.GetNS(netns)
+	if err != nil {
+		return fmt.Errorf("get container netns %q: %w", netns, err)
+	}
+	defer containerNS.Close() //nolint:errcheck // netns close on teardown
+
+	return containerNS.Do(func(_ ns.NetNS) error {
+		handle, err := netlink.NewHandle()
+		if err != nil {
+			return fmt.Errorf("create netlink handle: %w", err)
+		}
+		defer handle.Close() //nolint:errcheck // netlink cleanup on teardown
+
+		link, err := handle.LinkByName(name)
+		if err != nil {
+			return fmt.Errorf("find link: %w", err)
+		}
+		if wantMac != "" && link.Attrs().HardwareAddr.String() != wantMac {
+			return fmt.Errorf("MAC mismatch: expected %q, got %q", wantMac, link.Attrs().HardwareAddr.String())
+		}
+		if wantMtu > 0 && link.Attrs().MTU != wantMtu {
+			return fmt.Errorf("MTU mismatch: expected %d, got %d", wantMtu, link.Attrs().MTU)
+		}
+		return nil
+	})
+}
+
+// validateIPOnInterface verifies that the given IP address is assigned to the
+// named interface inside the container netns.
+func validateIPOnInterface(ip net.IP, name, netns string) error {
+	containerNS, err := ns.GetNS(netns)
+	if err != nil {
+		return fmt.Errorf("get container netns %q: %w", netns, err)
+	}
+	defer containerNS.Close() //nolint:errcheck // netns close on teardown
+
+	return containerNS.Do(func(_ ns.NetNS) error {
+		handle, err := netlink.NewHandle()
+		if err != nil {
+			return fmt.Errorf("create netlink handle: %w", err)
+		}
+		defer handle.Close() //nolint:errcheck // netlink cleanup on teardown
+
+		link, err := handle.LinkByName(name)
+		if err != nil {
+			return fmt.Errorf("find link: %w", err)
+		}
+
+		addrs, err := handle.AddrList(link, netlink.FAMILY_V6)
+		if err != nil {
+			return fmt.Errorf("list addresses: %w", err)
+		}
+
+		for _, addr := range addrs {
+			if addr.IP.Equal(ip) {
+				return nil
+			}
+		}
+		return fmt.Errorf("ip %s not assigned to %q", ip, name)
+	})
+}