Skip to content

Commit 628a463

Browse files
committed
fix: validate kernel state against prevResult in cmdCheck
cmdCheck performed existence checks but never validated that the kernel state matched the result returned by the most recent ADD. Per the CNI spec §4.3, CHECK must verify managed resources have not drifted from what the plugin previously reported. - Parse prevResult from PluginConf.RawPrevResult using the CNI types 100 parser - Validate host-side interface MAC and MTU against kernel state - Validate guest-side interface MAC and MTU from inside the container network namespace - Validate IP assignments are present on the correct guest interface - Add unit tests for prevResult parsing and invalid prevResult handling Fixes #166
1 parent 9052abc commit 628a463

2 files changed

Lines changed: 211 additions & 0 deletions

File tree

internal/cni/cni_test.go

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -662,6 +662,66 @@ func TestCmdCheckMissingVPC(t *testing.T) {
662662
}
663663
}
664664

665+
func TestCmdCheckWithPrevResultMissingResources(t *testing.T) {
666+
// Build a prevResult matching what buildResult produces for veth mode.
667+
prevResult := `{` +
668+
`"cniVersion":"1.0.0",` +
669+
`"interfaces":[` +
670+
`{"name":"galactic-abc-def","mac":"aa:bb:cc:dd:ee:01","mtu":1500,"sandbox":""},` +
671+
`{"name":"galactic-def-abc","mac":"aa:bb:cc:dd:ee:02","mtu":1500,"sandbox":"/proc/1/ns/net"}` +
672+
`],` +
673+
`"ips":[` +
674+
`{"version":"6","address":"fd00:10:ff01::1234/80","gateway":"fd00:10:ff01::1","interface":1}` +
675+
`]}`
676+
conf := fmt.Sprintf(
677+
`{"cniVersion":"1.0.0","name":"test",`+
678+
`"type":"galactic-cni","vpc":"%s",`+
679+
`"vpcattachment":"%s",`+
680+
`"prevResult":%s}`,
681+
testVPC, testAttachment, prevResult,
682+
)
683+
args := &skel.CmdArgs{
684+
ContainerID: testContainerID,
685+
Netns: "/proc/1/ns/net",
686+
StdinData: []byte(conf),
687+
}
688+
689+
err := cmdCheck(args)
690+
if err == nil {
691+
t.Fatalf("expected CHECK failure for missing resources, got nil")
692+
}
693+
if !strings.Contains(err.Error(), "CHECK failed") {
694+
t.Fatalf("error %q does not contain 'CHECK failed'", err.Error())
695+
}
696+
// prevResult parsing should succeed; errors come from missing kernel state.
697+
if !strings.Contains(err.Error(), "prevResult validation") {
698+
t.Fatalf("error %q does not contain 'prevResult validation'", err.Error())
699+
}
700+
}
701+
702+
func TestCmdCheckWithInvalidPrevResult(t *testing.T) {
703+
// prevResult that is structurally valid JSON but not a valid CNI result.
704+
conf := fmt.Sprintf(
705+
`{"cniVersion":"1.0.0","name":"test",`+
706+
`"type":"galactic-cni","vpc":"%s",`+
707+
`"vpcattachment":"%s",`+
708+
`"prevResult":{"not":"a valid cni result"}}`,
709+
testVPC, testAttachment,
710+
)
711+
args := &skel.CmdArgs{
712+
ContainerID: testContainerID,
713+
StdinData: []byte(conf),
714+
}
715+
716+
err := cmdCheck(args)
717+
if err == nil {
718+
t.Fatalf("expected CHECK failure for invalid prevResult, got nil")
719+
}
720+
if !strings.Contains(err.Error(), "prevResult validation") {
721+
t.Fatalf("error %q does not contain 'prevResult validation'", err.Error())
722+
}
723+
}
724+
665725
// ---- resourceTracker ------------------------------------------------------
666726

667727
func TestResourceTrackerCleanupZeroValue(t *testing.T) {

internal/cni/ops_check.go

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,15 @@ package cni
66

77
import (
88
"context"
9+
"encoding/json"
910
"errors"
1011
"fmt"
1112
"net"
1213
"net/http"
1314
"time"
1415

1516
"github.com/containernetworking/cni/pkg/skel"
17+
type100 "github.com/containernetworking/cni/pkg/types/100"
1618
"github.com/containernetworking/plugins/pkg/ns"
1719
"github.com/vishvananda/netlink"
1820
ctrl "sigs.k8s.io/controller-runtime"
@@ -57,6 +59,13 @@ func cmdCheck(args *skel.CmdArgs) error {
5759
}
5860
}
5961

62+
// Validate kernel state against prevResult (CNI spec §4.3).
63+
if pluginConf.RawPrevResult != nil {
64+
if err := checkPrevResult(pluginConf.RawPrevResult, hostName, args.Netns); err != nil {
65+
errs = append(errs, fmt.Errorf("prevResult validation: %w", err))
66+
}
67+
}
68+
6069
if len(errs) > 0 {
6170
return fmt.Errorf("CHECK failed: %w", errors.Join(errs...))
6271
}
@@ -196,3 +205,145 @@ func checkTerminationRoutes(vpc, vpcAttachment string, terminations []Terminatio
196205
}
197206
return nil
198207
}
208+
209+
// checkPrevResult validates that kernel state matches the interfaces and IPs
210+
// recorded in the prevResult returned by the most recent ADD. Per the CNI spec
211+
// §4.3, CHECK must verify that managed resources have not drifted.
212+
func checkPrevResult(rawPrevResult map[string]interface{}, _ string, netns string) error {
213+
// RawPrevResult is map[string]interface{} — marshal back to JSON, then
214+
// parse as a versioned CNI result.
215+
jsonBytes, err := json.Marshal(rawPrevResult)
216+
if err != nil {
217+
return fmt.Errorf("marshal prevResult: %w", err)
218+
}
219+
res, err := type100.NewResult(jsonBytes)
220+
if err != nil {
221+
return fmt.Errorf("parse prevResult: %w", err)
222+
}
223+
result, err := type100.GetResult(res)
224+
if err != nil {
225+
return fmt.Errorf("get prevResult: %w", err)
226+
}
227+
228+
// Validate each interface declared in prevResult against the kernel.
229+
for _, iface := range result.Interfaces {
230+
if iface.Name == "" {
231+
continue
232+
}
233+
234+
// Host-side interface: validate MAC and MTU from the host namespace.
235+
if iface.Sandbox == "" {
236+
if err := validateHostInterface(iface.Name, iface.Mac, iface.Mtu); err != nil {
237+
return fmt.Errorf("interface %q (host): %w", iface.Name, err)
238+
}
239+
continue
240+
}
241+
242+
// Guest-side interface: validate MAC and MTU from inside the container netns.
243+
if err := validateGuestInterface(iface.Name, iface.Mac, iface.Mtu, netns); err != nil {
244+
return fmt.Errorf("interface %q (guest): %w", iface.Name, err)
245+
}
246+
}
247+
248+
// Validate each IP assignment against the kernel.
249+
for _, ipConfig := range result.IPs {
250+
if ipConfig.Interface == nil {
251+
continue
252+
}
253+
idx := *ipConfig.Interface
254+
if idx < 0 || idx >= len(result.Interfaces) {
255+
return fmt.Errorf("ipConfig interface index %d out of range [0, %d)", idx, len(result.Interfaces))
256+
}
257+
targetIface := result.Interfaces[idx]
258+
if targetIface.Sandbox == "" {
259+
// Host-side IP — not expected in our plugin, but skip gracefully.
260+
continue
261+
}
262+
if err := validateIPOnInterface(ipConfig.Address.IP, targetIface.Name, netns); err != nil {
263+
return fmt.Errorf("ip %s on %q (guest): %w", ipConfig.Address.String(), targetIface.Name, err)
264+
}
265+
}
266+
267+
return nil
268+
}
269+
270+
// validateHostInterface checks that a host-side interface's MAC and MTU match
271+
// the values recorded in prevResult.
272+
func validateHostInterface(name, wantMac string, wantMtu int) error {
273+
link, err := netlink.LinkByName(name)
274+
if err != nil {
275+
return fmt.Errorf("find link: %w", err)
276+
}
277+
if wantMac != "" && link.Attrs().HardwareAddr.String() != wantMac {
278+
return fmt.Errorf("MAC mismatch: expected %q, got %q", wantMac, link.Attrs().HardwareAddr.String())
279+
}
280+
if wantMtu > 0 && link.Attrs().MTU != wantMtu {
281+
return fmt.Errorf("MTU mismatch: expected %d, got %d", wantMtu, link.Attrs().MTU)
282+
}
283+
return nil
284+
}
285+
286+
// validateGuestInterface checks that a guest-side interface's MAC and MTU match
287+
// the values recorded in prevResult, reading from inside the container netns.
288+
func validateGuestInterface(name, wantMac string, wantMtu int, netns string) error {
289+
containerNS, err := ns.GetNS(netns)
290+
if err != nil {
291+
return fmt.Errorf("get container netns %q: %w", netns, err)
292+
}
293+
defer containerNS.Close() //nolint:errcheck // netns close on teardown
294+
295+
return containerNS.Do(func(_ ns.NetNS) error {
296+
handle, err := netlink.NewHandle()
297+
if err != nil {
298+
return fmt.Errorf("create netlink handle: %w", err)
299+
}
300+
defer handle.Close() //nolint:errcheck // netlink cleanup on teardown
301+
302+
link, err := handle.LinkByName(name)
303+
if err != nil {
304+
return fmt.Errorf("find link: %w", err)
305+
}
306+
if wantMac != "" && link.Attrs().HardwareAddr.String() != wantMac {
307+
return fmt.Errorf("MAC mismatch: expected %q, got %q", wantMac, link.Attrs().HardwareAddr.String())
308+
}
309+
if wantMtu > 0 && link.Attrs().MTU != wantMtu {
310+
return fmt.Errorf("MTU mismatch: expected %d, got %d", wantMtu, link.Attrs().MTU)
311+
}
312+
return nil
313+
})
314+
}
315+
316+
// validateIPOnInterface verifies that the given IP address is assigned to the
317+
// named interface inside the container netns.
318+
func validateIPOnInterface(ip net.IP, name, netns string) error {
319+
containerNS, err := ns.GetNS(netns)
320+
if err != nil {
321+
return fmt.Errorf("get container netns %q: %w", netns, err)
322+
}
323+
defer containerNS.Close() //nolint:errcheck // netns close on teardown
324+
325+
return containerNS.Do(func(_ ns.NetNS) error {
326+
handle, err := netlink.NewHandle()
327+
if err != nil {
328+
return fmt.Errorf("create netlink handle: %w", err)
329+
}
330+
defer handle.Close() //nolint:errcheck // netlink cleanup on teardown
331+
332+
link, err := handle.LinkByName(name)
333+
if err != nil {
334+
return fmt.Errorf("find link: %w", err)
335+
}
336+
337+
addrs, err := handle.AddrList(link, netlink.FAMILY_V6)
338+
if err != nil {
339+
return fmt.Errorf("list addresses: %w", err)
340+
}
341+
342+
for _, addr := range addrs {
343+
if addr.IP.Equal(ip) {
344+
return nil
345+
}
346+
}
347+
return fmt.Errorf("ip %s not assigned to %q", ip, name)
348+
})
349+
}

0 commit comments

Comments
 (0)