From 9f4a623222a4507636c9f5079ac6b11c3b4c71e8 Mon Sep 17 00:00:00 2001 From: Bruce Ma Date: Wed, 18 Sep 2019 17:08:43 +0800 Subject: [PATCH 1/3] utils: add validation function for interface name Signed-off-by: Bruce Ma --- pkg/utils/utils.go | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 324c40de..096f2c7b 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -15,14 +15,21 @@ package utils import ( + "bytes" + "fmt" "regexp" "github.com/containernetworking/cni/pkg/types" ) -// cniValidNameChars is the regexp used to validate valid characters in -// containerID and networkName -const cniValidNameChars = `[a-zA-Z0-9][a-zA-Z0-9_.\-]` +const ( + // cniValidNameChars is the regexp used to validate valid characters in + // containerID and networkName + cniValidNameChars = `[a-zA-Z0-9][a-zA-Z0-9_.\-]` + + // maxInterfaceNameLength is the length max of a valid interface name + maxInterfaceNameLength = 15 +) var cniReg = regexp.MustCompile(`^` + cniValidNameChars + `*$`) @@ -49,3 +56,21 @@ func ValidateNetworkName(networkName string) *types.Error { } return nil } + +// ValidateInterfaceName will validate the interface name based on the three rules below +// 1. The name must not be empty +// 2. The name must be less than 16 characters +// 3. The name must not contain / or any whitespace characters +// ref to https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/lib/utils.c?id=1f420318bda3cc62156e89e1b56d60cc744b48ad#n827 +func ValidateInterfaceName(ifName string) *types.Error { + if len(ifName) == 0 { + return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name is empty", "") + } + if len(ifName) > maxInterfaceNameLength { + return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name is overflow", fmt.Sprintf("interface name length should be less than %d characters", maxInterfaceNameLength+1)) + } + if bytes.ContainsAny([]byte(ifName), `/ `) { + return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name contains / or whitespace characters", "") + } + return nil +} From 7be1ac932deaa9011fbe38aebf2f4ac6b8c8e1a5 Mon Sep 17 00:00:00 2001 From: Bruce Ma Date: Wed, 18 Sep 2019 21:29:30 +0800 Subject: [PATCH 2/3] add interface name validation to libcni and skel Signed-off-by: Bruce Ma --- libcni/api.go | 3 +++ libcni/api_test.go | 50 +++++++++++++++++++++++++++++++++++++++++-- pkg/skel/skel.go | 9 ++++---- pkg/skel/skel_test.go | 40 +++++++++++++++++++++++++++++++++- 4 files changed, 95 insertions(+), 7 deletions(-) diff --git a/libcni/api.go b/libcni/api.go index 22b11174..893c1979 100644 --- a/libcni/api.go +++ b/libcni/api.go @@ -409,6 +409,9 @@ func (c *CNIConfig) addNetwork(ctx context.Context, name, cniVersion string, net if err := utils.ValidateNetworkName(name); err != nil { return nil, err } + if err := utils.ValidateInterfaceName(rt.IfName); err != nil { + return nil, err + } newConf, err := buildOneConfig(name, cniVersion, net, prevResult, rt) if err != nil { diff --git a/libcni/api_test.go b/libcni/api_test.go index 9f277a62..b1a6dfaf 100644 --- a/libcni/api_test.go +++ b/libcni/api_test.go @@ -1012,7 +1012,7 @@ var _ = Describe("Invoking plugins", func() { It("returns the error", func() { _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) Expect(err).To(Equal(&types.Error{ - Code: 4, + Code: types.ErrInvalidEnvironmentVariables, Msg: "invalid characters in containerID", Details: "some-%%container-id", })) @@ -1027,13 +1027,59 @@ var _ = Describe("Invoking plugins", func() { It("returns the error", func() { _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) Expect(err).To(Equal(&types.Error{ - Code: 7, + Code: types.ErrInvalidNetworkConfig, Msg: "invalid characters found in network name", Details: "invalid-%%-name", })) }) }) + Context("return errors when interface name is invalid", func() { + It("interface name is empty", func() { + runtimeConfig.IfName = "" + + _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name is empty", + Details: "", + })) + }) + + It("interface name is overflow", func() { + runtimeConfig.IfName = "1234567890123456" + + _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name is overflow", + Details: "interface name length should be less than 16 characters", + })) + }) + + It("interface name contains invalid characters /", func() { + runtimeConfig.IfName = "test/test" + + _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name contains / or whitespace characters", + Details: "", + })) + }) + + It("interface name contains invalid characters whitespace", func() { + runtimeConfig.IfName = "test test" + + _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name contains / or whitespace characters", + Details: "", + })) + }) + }) + Context("when the second plugin errors", func() { BeforeEach(func() { plugins[1].debug.ReportError = "plugin error: banana" diff --git a/pkg/skel/skel.go b/pkg/skel/skel.go index af506b48..da42db55 100644 --- a/pkg/skel/skel.go +++ b/pkg/skel/skel.go @@ -202,12 +202,13 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error, } if cmd != "VERSION" { - err = validateConfig(cmdArgs.StdinData) - if err != nil { + if err = validateConfig(cmdArgs.StdinData); err != nil { return err } - err = utils.ValidateContainerID(cmdArgs.ContainerID) - if err != nil { + if err = utils.ValidateContainerID(cmdArgs.ContainerID); err != nil { + return err + } + if err = utils.ValidateInterfaceName(cmdArgs.IfName); err != nil { return err } } diff --git a/pkg/skel/skel_test.go b/pkg/skel/skel_test.go index 26691d16..631a6abc 100644 --- a/pkg/skel/skel_test.go +++ b/pkg/skel/skel_test.go @@ -119,12 +119,50 @@ var _ = Describe("dispatching to the correct callback", func() { err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "") Expect(err).To(HaveOccurred()) Expect(err).To(Equal(&types.Error{ - Code: 4, + Code: types.ErrInvalidEnvironmentVariables, Msg: "invalid characters in containerID", Details: "some-%%container-id", })) }) + Context("return errors when interface name is invalid", func() { + It("interface name is overflow", func() { + environment["CNI_IFNAME"] = "1234567890123456" + + err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name is overflow", + Details: "interface name length should be less than 16 characters", + })) + }) + + It("interface name contains invalid characters /", func() { + environment["CNI_IFNAME"] = "test/test" + + err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name contains / or whitespace characters", + Details: "", + })) + }) + + It("interface name contains invalid characters whitespace", func() { + environment["CNI_IFNAME"] = "test test" + + err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name contains / or whitespace characters", + Details: "", + })) + }) + }) + It("does not call cmdCheck or cmdDel", func() { err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "") From eefc06974c66eb2e14b06d0849aa82ebff43b837 Mon Sep 17 00:00:00 2001 From: Bruce Ma Date: Fri, 20 Sep 2019 11:33:38 +0800 Subject: [PATCH 3/3] SPEC: update validation rules for interface name in docs and address some comments Signed-off-by: Bruce Ma --- SPEC.md | 2 +- libcni/api_test.go | 43 +++++++++++++++++++++++++++++++++++----- pkg/skel/skel_test.go | 46 ++++++++++++++++++++++++++++++++++++++----- pkg/utils/utils.go | 18 ++++++++++++----- 4 files changed, 93 insertions(+), 16 deletions(-) diff --git a/SPEC.md b/SPEC.md index 16fbe24e..81456673 100644 --- a/SPEC.md +++ b/SPEC.md @@ -68,7 +68,7 @@ The operations that CNI plugins must support are: - **Network namespace path**. This represents the path to the network namespace to be added, i.e. /proc/[pid]/ns/net or a bind-mount/link to it. - **Network configuration**. This is a JSON document describing a network to which a container can be joined. The schema is described below. - **Extra arguments**. This provides an alternative mechanism to allow simple configuration of CNI plugins on a per-container basis. - - **Name of the interface inside the container**. This is the name that should be assigned to the interface created inside the container (network namespace); consequently it must comply with the standard Linux restrictions on interface names. + - **Name of the interface inside the container**. This is the name that should be assigned to the interface created inside the container (network namespace); consequently it must comply with the standard Linux restrictions on interface names, must not be empty, must not be "." or "..", must be less than 16 characters and must not contain / or : or any whitespace characters. - Result: - **Interfaces list**. Depending on the plugin, this can include the sandbox (eg, container or hypervisor) interface name and/or the host interface name, the hardware addresses of each interface, and details about the sandbox (if any) the interface is in. - **IP configuration assigned to each interface**. The IPv4 and/or IPv6 addresses, gateways, and routes assigned to sandbox and/or host interfaces. diff --git a/libcni/api_test.go b/libcni/api_test.go index b1a6dfaf..05369ef8 100644 --- a/libcni/api_test.go +++ b/libcni/api_test.go @@ -1046,14 +1046,36 @@ var _ = Describe("Invoking plugins", func() { })) }) - It("interface name is overflow", func() { + It("interface name is too long", func() { runtimeConfig.IfName = "1234567890123456" _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) Expect(err).To(Equal(&types.Error{ Code: types.ErrInvalidEnvironmentVariables, - Msg: "interface name is overflow", - Details: "interface name length should be less than 16 characters", + Msg: "interface name is too long", + Details: "interface name should be less than 16 characters", + })) + }) + + It("interface name is .", func() { + runtimeConfig.IfName = "." + + _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name is . or ..", + Details: "", + })) + }) + + It("interface name is ..", func() { + runtimeConfig.IfName = ".." + + _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name is . or ..", + Details: "", })) }) @@ -1063,7 +1085,18 @@ var _ = Describe("Invoking plugins", func() { _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) Expect(err).To(Equal(&types.Error{ Code: types.ErrInvalidEnvironmentVariables, - Msg: "interface name contains / or whitespace characters", + Msg: "interface name contains / or : or whitespace characters", + Details: "", + })) + }) + + It("interface name contains invalid characters :", func() { + runtimeConfig.IfName = "test:test" + + _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name contains / or : or whitespace characters", Details: "", })) }) @@ -1074,7 +1107,7 @@ var _ = Describe("Invoking plugins", func() { _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) Expect(err).To(Equal(&types.Error{ Code: types.ErrInvalidEnvironmentVariables, - Msg: "interface name contains / or whitespace characters", + Msg: "interface name contains / or : or whitespace characters", Details: "", })) }) diff --git a/pkg/skel/skel_test.go b/pkg/skel/skel_test.go index 631a6abc..2284dd06 100644 --- a/pkg/skel/skel_test.go +++ b/pkg/skel/skel_test.go @@ -126,15 +126,39 @@ var _ = Describe("dispatching to the correct callback", func() { }) Context("return errors when interface name is invalid", func() { - It("interface name is overflow", func() { + It("interface name is too long", func() { environment["CNI_IFNAME"] = "1234567890123456" err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "") Expect(err).To(HaveOccurred()) Expect(err).To(Equal(&types.Error{ Code: types.ErrInvalidEnvironmentVariables, - Msg: "interface name is overflow", - Details: "interface name length should be less than 16 characters", + Msg: "interface name is too long", + Details: "interface name should be less than 16 characters", + })) + }) + + It("interface name is .", func() { + environment["CNI_IFNAME"] = "." + + err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name is . or ..", + Details: "", + })) + }) + + It("interface name is ..", func() { + environment["CNI_IFNAME"] = ".." + + err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name is . or ..", + Details: "", })) }) @@ -145,7 +169,19 @@ var _ = Describe("dispatching to the correct callback", func() { Expect(err).To(HaveOccurred()) Expect(err).To(Equal(&types.Error{ Code: types.ErrInvalidEnvironmentVariables, - Msg: "interface name contains / or whitespace characters", + Msg: "interface name contains / or : or whitespace characters", + Details: "", + })) + }) + + It("interface name contains invalid characters :", func() { + environment["CNI_IFNAME"] = "test:test" + + err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "") + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(&types.Error{ + Code: types.ErrInvalidEnvironmentVariables, + Msg: "interface name contains / or : or whitespace characters", Details: "", })) }) @@ -157,7 +193,7 @@ var _ = Describe("dispatching to the correct callback", func() { Expect(err).To(HaveOccurred()) Expect(err).To(Equal(&types.Error{ Code: types.ErrInvalidEnvironmentVariables, - Msg: "interface name contains / or whitespace characters", + Msg: "interface name contains / or : or whitespace characters", Details: "", })) }) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 096f2c7b..b8ec3887 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -18,6 +18,7 @@ import ( "bytes" "fmt" "regexp" + "unicode" "github.com/containernetworking/cni/pkg/types" ) @@ -60,17 +61,24 @@ func ValidateNetworkName(networkName string) *types.Error { // ValidateInterfaceName will validate the interface name based on the three rules below // 1. The name must not be empty // 2. The name must be less than 16 characters -// 3. The name must not contain / or any whitespace characters -// ref to https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/lib/utils.c?id=1f420318bda3cc62156e89e1b56d60cc744b48ad#n827 +// 3. The name must not be "." or ".." +// 3. The name must not contain / or : or any whitespace characters +// ref to https://github.com/torvalds/linux/blob/master/net/core/dev.c#L1024 func ValidateInterfaceName(ifName string) *types.Error { if len(ifName) == 0 { return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name is empty", "") } if len(ifName) > maxInterfaceNameLength { - return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name is overflow", fmt.Sprintf("interface name length should be less than %d characters", maxInterfaceNameLength+1)) + return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name is too long", fmt.Sprintf("interface name should be less than %d characters", maxInterfaceNameLength+1)) } - if bytes.ContainsAny([]byte(ifName), `/ `) { - return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name contains / or whitespace characters", "") + if ifName == "." || ifName == ".." { + return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name is . or ..", "") } + for _, r := range bytes.Runes([]byte(ifName)) { + if r == '/' || r == ':' || unicode.IsSpace(r) { + return types.NewError(types.ErrInvalidEnvironmentVariables, "interface name contains / or : or whitespace characters", "") + } + } + return nil }