From ff503882f7f3218dc0bfea5c3a5f551e8023dbd1 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Wed, 9 Aug 2023 22:18:12 +0200 Subject: [PATCH 1/9] daemon: Improve NetworkingConfig & EndpointSettings validation So far, only a subset of NetworkingConfig was validated when calling ContainerCreate. Other parameters would be validated when the container was started. And the same goes for EndpointSettings on NetworkConnect. This commit adds two validation steps: 1. Check if the IP addresses set in endpoint's IPAMConfig are valid, when ContainerCreate and ConnectToNetwork is called ; 2. Check if the network allows static IP addresses, only on ConnectToNetwork as we need the libnetwork's Network for that and it might not exist until NetworkAttachment requests are sent to the Swarm leader (which happens only when starting the container) ; Signed-off-by: Albin Kerouanton --- api/swagger.yaml | 4 +++ daemon/container_operations.go | 47 ++++++++++++++++++++--------- daemon/container_operations_unix.go | 2 ++ daemon/create.go | 25 ++++++--------- daemon/create_test.go | 21 ------------- docs/api/version-history.md | 3 ++ 6 files changed, 50 insertions(+), 52 deletions(-) delete mode 100644 daemon/create_test.go diff --git a/api/swagger.yaml b/api/swagger.yaml index 952c2a46f4599877f00339943b3d5323f66a93e7..0ceef48b089d977c265563fae4d9fae7ae54c168 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -10038,6 +10038,10 @@ paths: responses: 200: description: "No error" + 400: + description: "bad parameter" + schema: + $ref: "#/definitions/ErrorResponse" 403: description: "Operation not supported for swarm scoped networks" schema: diff --git a/daemon/container_operations.go b/daemon/container_operations.go index a87cccb3555c32e73475973958c302cd6b9c4f1c..7e480a411bfccda09fed9f5b01793b0f251813aa 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -562,39 +562,56 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C return nil } -// hasUserDefinedIPAddress returns whether the passed IPAM configuration contains IP address configuration -func hasUserDefinedIPAddress(ipamConfig *networktypes.EndpointIPAMConfig) bool { - return ipamConfig != nil && (len(ipamConfig.IPv4Address) > 0 || len(ipamConfig.IPv6Address) > 0) -} - -// User specified ip address is acceptable only for networks with user specified subnets. -func validateNetworkingConfig(n *libnetwork.Network, epConfig *networktypes.EndpointSettings) error { - if n == nil || epConfig == nil { +// validateEndpointSettings checks whether the given epConfig is valid. The nw parameter might be nil as a container +// can be created with a reference to a network that don't exist yet. In that case, only partial validation will be +// done. +func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *networktypes.EndpointSettings) error { + if epConfig == nil { return nil } - if !containertypes.NetworkMode(n.Name()).IsUserDefined() { - if hasUserDefinedIPAddress(epConfig.IPAMConfig) && !enableIPOnPredefinedNetwork() { + + ipamConfig := &networktypes.EndpointIPAMConfig{} + if epConfig.IPAMConfig != nil { + ipamConfig = epConfig.IPAMConfig + } + + if !containertypes.NetworkMode(nwName).IsUserDefined() { + hasStaticAddresses := ipamConfig.IPv4Address != "" || ipamConfig.IPv6Address != "" + // On Linux, user specified IP address is accepted only by networks with user specified subnets. + if hasStaticAddresses && !enableIPOnPredefinedNetwork() { return runconfig.ErrUnsupportedNetworkAndIP } if len(epConfig.Aliases) > 0 && !serviceDiscoveryOnDefaultNetwork() { return runconfig.ErrUnsupportedNetworkAndAlias } } - if !hasUserDefinedIPAddress(epConfig.IPAMConfig) { + + if ipamConfig.IPv4Address != "" { + if addr := net.ParseIP(ipamConfig.IPv4Address); addr == nil || addr.To4() == nil || addr.IsUnspecified() { + return fmt.Errorf("invalid IPv4 address: %s", ipamConfig.IPv4Address) + } + } + if ipamConfig.IPv6Address != "" { + if addr := net.ParseIP(ipamConfig.IPv6Address); addr == nil || addr.To4() != nil || addr.IsUnspecified() { + return fmt.Errorf("invalid IPv6 address: %s", ipamConfig.IPv6Address) + } + } + + if nw == nil { return nil } - _, _, nwIPv4Configs, nwIPv6Configs := n.IpamConfig() + _, _, nwIPv4Configs, nwIPv6Configs := nw.IpamConfig() for _, s := range []struct { ipConfigured bool subnetConfigs []*libnetwork.IpamConf }{ { - ipConfigured: len(epConfig.IPAMConfig.IPv4Address) > 0, + ipConfigured: len(ipamConfig.IPv4Address) > 0, subnetConfigs: nwIPv4Configs, }, { - ipConfigured: len(epConfig.IPAMConfig.IPv6Address) > 0, + ipConfigured: len(ipamConfig.IPv6Address) > 0, subnetConfigs: nwIPv6Configs, }, } { @@ -657,7 +674,7 @@ func (daemon *Daemon) updateNetworkConfig(container *container.Container, n *lib } } - if err := validateNetworkingConfig(n, endpointConfig); err != nil { + if err := validateEndpointSettings(n, n.Name(), endpointConfig); err != nil { return err } diff --git a/daemon/container_operations_unix.go b/daemon/container_operations_unix.go index c19a1115610ecf6a0dd3ec427d6d87f8ec133962..25549ea8d054088380bd5bd33c2fa842b3a9059a 100644 --- a/daemon/container_operations_unix.go +++ b/daemon/container_operations_unix.go @@ -367,11 +367,13 @@ func isLinkable(child *container.Container) bool { return ok } +// TODO(aker): remove when we make the default bridge network behave like any other network func enableIPOnPredefinedNetwork() bool { return false } // serviceDiscoveryOnDefaultNetwork indicates if service discovery is supported on the default network +// TODO(aker): remove when we make the default bridge network behave like any other network func serviceDiscoveryOnDefaultNetwork() bool { return false } diff --git a/daemon/create.go b/daemon/create.go index 81c9a481ca275623a30a2bff333be3f292ea1c8c..f593f59ddcd1fbc5350b362bccba43803f0c7ff0 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "net" "runtime" "strings" "time" @@ -98,7 +97,7 @@ func (daemon *Daemon) containerCreate(ctx context.Context, daemonCfg *configStor } } - err = verifyNetworkingConfig(opts.params.NetworkingConfig) + err = daemon.validateNetworkingConfig(opts.params.NetworkingConfig) if err != nil { return containertypes.CreateResponse{Warnings: warnings}, errdefs.InvalidParameter(err) } @@ -321,8 +320,8 @@ func (daemon *Daemon) mergeAndVerifyConfig(config *containertypes.Config, img *i return nil } -// verifyNetworkingConfig validates if the nwConfig is valid. -func verifyNetworkingConfig(nwConfig *networktypes.NetworkingConfig) error { +// validateNetworkingConfig checks whether a container's NetworkingConfig is valid. +func (daemon *Daemon) validateNetworkingConfig(nwConfig *networktypes.NetworkingConfig) error { if nwConfig == nil { return nil } @@ -331,20 +330,14 @@ func verifyNetworkingConfig(nwConfig *networktypes.NetworkingConfig) error { if v == nil { return fmt.Errorf("no EndpointSettings for %s", k) } - if v.IPAMConfig != nil { - if v.IPAMConfig.IPv4Address != "" && net.ParseIP(v.IPAMConfig.IPv4Address).To4() == nil { - return fmt.Errorf("invalid IPv4 address: %s", v.IPAMConfig.IPv4Address) - } - if v.IPAMConfig.IPv6Address != "" { - n := net.ParseIP(v.IPAMConfig.IPv6Address) - // if the address is an invalid network address (ParseIP == nil) or if it is - // an IPv4 address (To4() != nil), then it is an invalid IPv6 address - if n == nil || n.To4() != nil { - return fmt.Errorf("invalid IPv6 address: %s", v.IPAMConfig.IPv6Address) - } - } + + // The referenced network k might not exist when the container is created, so just ignore the error in that case. + nw, _ := daemon.FindNetwork(k) + if err := validateEndpointSettings(nw, k, v); err != nil { + return err } } + return nil } diff --git a/daemon/create_test.go b/daemon/create_test.go deleted file mode 100644 index c5d116098b3e224c4756bccfa3c943f85ec99973..0000000000000000000000000000000000000000 --- a/daemon/create_test.go +++ /dev/null @@ -1,21 +0,0 @@ -package daemon // import "github.com/docker/docker/daemon" - -import ( - "testing" - - "github.com/docker/docker/api/types/network" - "gotest.tools/v3/assert" - is "gotest.tools/v3/assert/cmp" -) - -// Test case for 35752 -func TestVerifyNetworkingConfig(t *testing.T) { - name := "mynet" - endpoints := make(map[string]*network.EndpointSettings, 1) - endpoints[name] = nil - nwConfig := &network.NetworkingConfig{ - EndpointsConfig: endpoints, - } - err := verifyNetworkingConfig(nwConfig) - assert.Check(t, is.Error(err, "no EndpointSettings for mynet"), "should produce an error because no EndpointSettings were passed") -} diff --git a/docs/api/version-history.md b/docs/api/version-history.md index d2365eb53dccb6deceff9fd413e4b9ff32be59c5..44f0384187ad0269187385a398c3037da8762da1 100644 --- a/docs/api/version-history.md +++ b/docs/api/version-history.md @@ -47,6 +47,9 @@ keywords: "API, Docker, rcli, REST, documentation" 1.44. * `POST /containers/create` now accepts multiple `EndpointSettings` in `NetworkingConfig.EndpointSettings`. +* `POST /containers/create` and `POST /networks/{id}/connect` will now catch + validation errors that were previously only returned during `POST /containers/{id}/start`. + Note that this change is _unversioned_ and applies to all API versions. ## v1.43 API changes From 4bd055327480aa8cb7a4a5ab7f4c1ff1b97ffa2e Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Thu, 10 Aug 2023 01:42:35 +0200 Subject: [PATCH 2/9] daemon: Return all validation errors for NetworkingConfig and EndpointSettings Thus far, validation code would stop as soon as a bad value was found. Now, we try to validate as much as we can, to return all errors to the API client. Signed-off-by: Albin Kerouanton --- daemon/container_operations.go | 17 ++++++++++------- daemon/create.go | 11 +++++++++-- docs/api/version-history.md | 2 ++ 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 7e480a411bfccda09fed9f5b01793b0f251813aa..42900d1155b9fd9e2b3e568f87613a82be76d13e 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -18,6 +18,7 @@ import ( "github.com/docker/docker/daemon/config" "github.com/docker/docker/daemon/network" "github.com/docker/docker/errdefs" + "github.com/docker/docker/internal/multierror" "github.com/docker/docker/libnetwork" "github.com/docker/docker/libnetwork/netlabel" "github.com/docker/docker/libnetwork/options" @@ -575,30 +576,32 @@ func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *n ipamConfig = epConfig.IPAMConfig } + var errs []error + if !containertypes.NetworkMode(nwName).IsUserDefined() { hasStaticAddresses := ipamConfig.IPv4Address != "" || ipamConfig.IPv6Address != "" // On Linux, user specified IP address is accepted only by networks with user specified subnets. if hasStaticAddresses && !enableIPOnPredefinedNetwork() { - return runconfig.ErrUnsupportedNetworkAndIP + errs = append(errs, runconfig.ErrUnsupportedNetworkAndIP) } if len(epConfig.Aliases) > 0 && !serviceDiscoveryOnDefaultNetwork() { - return runconfig.ErrUnsupportedNetworkAndAlias + errs = append(errs, runconfig.ErrUnsupportedNetworkAndAlias) } } if ipamConfig.IPv4Address != "" { if addr := net.ParseIP(ipamConfig.IPv4Address); addr == nil || addr.To4() == nil || addr.IsUnspecified() { - return fmt.Errorf("invalid IPv4 address: %s", ipamConfig.IPv4Address) + errs = append(errs, fmt.Errorf("invalid IPv4 address: %s", ipamConfig.IPv4Address)) } } if ipamConfig.IPv6Address != "" { if addr := net.ParseIP(ipamConfig.IPv6Address); addr == nil || addr.To4() != nil || addr.IsUnspecified() { - return fmt.Errorf("invalid IPv6 address: %s", ipamConfig.IPv6Address) + errs = append(errs, fmt.Errorf("invalid IPv6 address: %s", ipamConfig.IPv6Address)) } } if nw == nil { - return nil + return multierror.Join(errs...) } _, _, nwIPv4Configs, nwIPv6Configs := nw.IpamConfig() @@ -624,12 +627,12 @@ func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *n } } if !foundSubnet { - return runconfig.ErrUnsupportedNetworkNoSubnetAndIP + errs = append(errs, runconfig.ErrUnsupportedNetworkNoSubnetAndIP) } } } - return nil + return multierror.Join(errs...) } // cleanOperationalData resets the operational data from the passed endpoint settings diff --git a/daemon/create.go b/daemon/create.go index f593f59ddcd1fbc5350b362bccba43803f0c7ff0..682b2b2a6506260e13e528cc6c65d7c3dd99d118 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -20,6 +20,7 @@ import ( "github.com/docker/docker/daemon/images" "github.com/docker/docker/errdefs" "github.com/docker/docker/image" + "github.com/docker/docker/internal/multierror" "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/runconfig" ocispec "github.com/opencontainers/image-spec/specs-go/v1" @@ -326,18 +327,24 @@ func (daemon *Daemon) validateNetworkingConfig(nwConfig *networktypes.Networking return nil } + var errs []error for k, v := range nwConfig.EndpointsConfig { if v == nil { - return fmt.Errorf("no EndpointSettings for %s", k) + errs = append(errs, fmt.Errorf("invalid config for network %s: EndpointsConfig is nil", k)) + continue } // The referenced network k might not exist when the container is created, so just ignore the error in that case. nw, _ := daemon.FindNetwork(k) if err := validateEndpointSettings(nw, k, v); err != nil { - return err + errs = append(errs, fmt.Errorf("invalid config for network %s: %w", k, err)) } } + if len(errs) > 0 { + return errdefs.InvalidParameter(multierror.Join(errs...)) + } + return nil } diff --git a/docs/api/version-history.md b/docs/api/version-history.md index 44f0384187ad0269187385a398c3037da8762da1..904c2122561451cc67a180e9c8e0e5e2fe8e0fc0 100644 --- a/docs/api/version-history.md +++ b/docs/api/version-history.md @@ -49,6 +49,8 @@ keywords: "API, Docker, rcli, REST, documentation" `NetworkingConfig.EndpointSettings`. * `POST /containers/create` and `POST /networks/{id}/connect` will now catch validation errors that were previously only returned during `POST /containers/{id}/start`. + These endpoints will also return the full set of validation errors they find, + instead of returning only the first one. Note that this change is _unversioned_ and applies to all API versions. ## v1.43 API changes From bfd8c6deb7f6e92abccc14a6ccf8965406f1f695 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Thu, 10 Aug 2023 11:18:15 +0200 Subject: [PATCH 3/9] daemon: Validate EndpointSettings.IPAMConfig.LinkLocalIPs Signed-off-by: Albin Kerouanton --- daemon/container_operations.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 42900d1155b9fd9e2b3e568f87613a82be76d13e..3a86f23e27a05de6886851480fbf19e13cffc18a 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -599,6 +599,11 @@ func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *n errs = append(errs, fmt.Errorf("invalid IPv6 address: %s", ipamConfig.IPv6Address)) } } + for _, addr := range ipamConfig.LinkLocalIPs { + if parsed := net.ParseIP(addr); parsed == nil || parsed.IsUnspecified() { + errs = append(errs, fmt.Errorf("invalid link-local IP address %s", addr)) + } + } if nw == nil { return multierror.Join(errs...) From 19c07198b6ff40845ee2121b51330368d82e140e Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Thu, 10 Aug 2023 12:09:14 +0200 Subject: [PATCH 4/9] daemon: Check if endpoint address is in allowed range This issue wasn't caught on ContainerCreate or NetworkConnect (when container wasn't started yet). Signed-off-by: Albin Kerouanton --- daemon/container_operations.go | 54 +++++++++++-------- .../docker_cli_network_unix_test.go | 10 ++-- 2 files changed, 36 insertions(+), 28 deletions(-) diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 3a86f23e27a05de6886851480fbf19e13cffc18a..28c654a8c0e67325112e996c2a6c6552ab87d19b 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -610,34 +610,42 @@ func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *n } _, _, nwIPv4Configs, nwIPv6Configs := nw.IpamConfig() - for _, s := range []struct { - ipConfigured bool - subnetConfigs []*libnetwork.IpamConf - }{ - { - ipConfigured: len(ipamConfig.IPv4Address) > 0, - subnetConfigs: nwIPv4Configs, - }, - { - ipConfigured: len(ipamConfig.IPv6Address) > 0, - subnetConfigs: nwIPv6Configs, - }, - } { - if s.ipConfigured { - foundSubnet := false - for _, cfg := range s.subnetConfigs { - if len(cfg.PreferredPool) > 0 { - foundSubnet = true - break - } + if err := validateEndpointIPAddress(nwIPv4Configs, ipamConfig.IPv4Address); err != nil { + errs = append(errs, err) + } + if err := validateEndpointIPAddress(nwIPv6Configs, ipamConfig.IPv6Address); err != nil { + errs = append(errs, err) + } + + return multierror.Join(errs...) +} + +func validateEndpointIPAddress(nwIPAMConfig []*libnetwork.IpamConf, epAddr string) error { + if epAddr == "" { + return nil + } + + var customSubnet bool + parsedAddr := net.ParseIP(epAddr) + for _, conf := range nwIPAMConfig { + if conf.PreferredPool != "" { + customSubnet = true + + _, allowedRange, _ := net.ParseCIDR(conf.PreferredPool) + if conf.SubPool != "" { + _, allowedRange, _ = net.ParseCIDR(conf.SubPool) } - if !foundSubnet { - errs = append(errs, runconfig.ErrUnsupportedNetworkNoSubnetAndIP) + + if allowedRange.Contains(parsedAddr) { + return nil } } } - return multierror.Join(errs...) + if customSubnet { + return fmt.Errorf("no predefined subnet or ip-range contain the IP address: %s", epAddr) + } + return runconfig.ErrUnsupportedNetworkNoSubnetAndIP } // cleanOperationalData resets the operational data from the passed endpoint settings diff --git a/integration-cli/docker_cli_network_unix_test.go b/integration-cli/docker_cli_network_unix_test.go index da78bc26927faa9c77f470bab0049f8e04ad312f..c618987a96532ec4d061021d572004e21ac0f325 100644 --- a/integration-cli/docker_cli_network_unix_test.go +++ b/integration-cli/docker_cli_network_unix_test.go @@ -1289,9 +1289,9 @@ func (s *DockerNetworkSuite) TestDockerNetworkConnectPreferredIP(c *testing.T) { verifyIPAddresses(c, "c0", "n0", "172.28.99.88", "2001:db8:1234::9988") // connect the container to the second network specifying an ip addresses - dockerCmd(c, "network", "connect", "--ip", "172.30.55.44", "--ip6", "2001:db8:abcd::5544", "n1", "c0") - verifyIPAddressConfig(c, "c0", "n1", "172.30.55.44", "2001:db8:abcd::5544") - verifyIPAddresses(c, "c0", "n1", "172.30.55.44", "2001:db8:abcd::5544") + dockerCmd(c, "network", "connect", "--ip", "172.30.5.44", "--ip6", "2001:db8:abcd::5544", "n1", "c0") + verifyIPAddressConfig(c, "c0", "n1", "172.30.5.44", "2001:db8:abcd::5544") + verifyIPAddresses(c, "c0", "n1", "172.30.5.44", "2001:db8:abcd::5544") // Stop and restart the container dockerCmd(c, "stop", "c0") @@ -1300,8 +1300,8 @@ func (s *DockerNetworkSuite) TestDockerNetworkConnectPreferredIP(c *testing.T) { // verify requested addresses are applied and configs are still there verifyIPAddressConfig(c, "c0", "n0", "172.28.99.88", "2001:db8:1234::9988") verifyIPAddresses(c, "c0", "n0", "172.28.99.88", "2001:db8:1234::9988") - verifyIPAddressConfig(c, "c0", "n1", "172.30.55.44", "2001:db8:abcd::5544") - verifyIPAddresses(c, "c0", "n1", "172.30.55.44", "2001:db8:abcd::5544") + verifyIPAddressConfig(c, "c0", "n1", "172.30.5.44", "2001:db8:abcd::5544") + verifyIPAddresses(c, "c0", "n1", "172.30.5.44", "2001:db8:abcd::5544") // Still it should fail to connect to the default network with a specified IP (whatever ip) out, _, err := dockerCmdWithError("network", "connect", "--ip", "172.21.55.44", "bridge", "c0") From 04a47e88d28a90300913b1ef174b30add1eb2e47 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Fri, 15 Sep 2023 10:50:45 +0200 Subject: [PATCH 5/9] api/t/net: move endpoint structs into endpoint.go Signed-off-by: Albin Kerouanton --- api/types/network/endpoint.go | 54 +++++++++++++++++++++++++++++++++++ api/types/network/network.go | 53 ---------------------------------- 2 files changed, 54 insertions(+), 53 deletions(-) create mode 100644 api/types/network/endpoint.go diff --git a/api/types/network/endpoint.go b/api/types/network/endpoint.go new file mode 100644 index 0000000000000000000000000000000000000000..423214fdb96ca99d83a98e743f18fb02226a6157 --- /dev/null +++ b/api/types/network/endpoint.go @@ -0,0 +1,54 @@ +package network + +// EndpointSettings stores the network endpoint details +type EndpointSettings struct { + // Configurations + IPAMConfig *EndpointIPAMConfig + Links []string + Aliases []string + // Operational data + NetworkID string + EndpointID string + Gateway string + IPAddress string + IPPrefixLen int + IPv6Gateway string + GlobalIPv6Address string + GlobalIPv6PrefixLen int + MacAddress string + DriverOpts map[string]string +} + +// Copy makes a deep copy of `EndpointSettings` +func (es *EndpointSettings) Copy() *EndpointSettings { + epCopy := *es + if es.IPAMConfig != nil { + epCopy.IPAMConfig = es.IPAMConfig.Copy() + } + + if es.Links != nil { + links := make([]string, 0, len(es.Links)) + epCopy.Links = append(links, es.Links...) + } + + if es.Aliases != nil { + aliases := make([]string, 0, len(es.Aliases)) + epCopy.Aliases = append(aliases, es.Aliases...) + } + return &epCopy +} + +// EndpointIPAMConfig represents IPAM configurations for the endpoint +type EndpointIPAMConfig struct { + IPv4Address string `json:",omitempty"` + IPv6Address string `json:",omitempty"` + LinkLocalIPs []string `json:",omitempty"` +} + +// Copy makes a copy of the endpoint ipam config +func (cfg *EndpointIPAMConfig) Copy() *EndpointIPAMConfig { + cfgCopy := *cfg + cfgCopy.LinkLocalIPs = make([]string, 0, len(cfg.LinkLocalIPs)) + cfgCopy.LinkLocalIPs = append(cfgCopy.LinkLocalIPs, cfg.LinkLocalIPs...) + return &cfgCopy +} diff --git a/api/types/network/network.go b/api/types/network/network.go index 6c5ef9ca4019ee8fb47cdbef8e43aeb099aabc57..e42630f2566e25685b412827989d65de5102644f 100644 --- a/api/types/network/network.go +++ b/api/types/network/network.go @@ -9,46 +9,12 @@ type Address struct { PrefixLen int } -// EndpointIPAMConfig represents IPAM configurations for the endpoint -type EndpointIPAMConfig struct { - IPv4Address string `json:",omitempty"` - IPv6Address string `json:",omitempty"` - LinkLocalIPs []string `json:",omitempty"` -} - -// Copy makes a copy of the endpoint ipam config -func (cfg *EndpointIPAMConfig) Copy() *EndpointIPAMConfig { - cfgCopy := *cfg - cfgCopy.LinkLocalIPs = make([]string, 0, len(cfg.LinkLocalIPs)) - cfgCopy.LinkLocalIPs = append(cfgCopy.LinkLocalIPs, cfg.LinkLocalIPs...) - return &cfgCopy -} - // PeerInfo represents one peer of an overlay network type PeerInfo struct { Name string IP string } -// EndpointSettings stores the network endpoint details -type EndpointSettings struct { - // Configurations - IPAMConfig *EndpointIPAMConfig - Links []string - Aliases []string - // Operational data - NetworkID string - EndpointID string - Gateway string - IPAddress string - IPPrefixLen int - IPv6Gateway string - GlobalIPv6Address string - GlobalIPv6PrefixLen int - MacAddress string - DriverOpts map[string]string -} - // Task carries the information about one backend task type Task struct { Name string @@ -65,25 +31,6 @@ type ServiceInfo struct { Tasks []Task } -// Copy makes a deep copy of `EndpointSettings` -func (es *EndpointSettings) Copy() *EndpointSettings { - epCopy := *es - if es.IPAMConfig != nil { - epCopy.IPAMConfig = es.IPAMConfig.Copy() - } - - if es.Links != nil { - links := make([]string, 0, len(es.Links)) - epCopy.Links = append(links, es.Links...) - } - - if es.Aliases != nil { - aliases := make([]string, 0, len(es.Aliases)) - epCopy.Aliases = append(aliases, es.Aliases...) - } - return &epCopy -} - // NetworkingConfig represents the container's networking configuration for each of its interfaces // Carries the networking configs specified in the `docker run` and `docker network connect` commands type NetworkingConfig struct { From 81ab8db1c38ee3dd1cd28a1ce16d990e7bfcdfa3 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Fri, 15 Sep 2023 11:15:23 +0200 Subject: [PATCH 6/9] api/t/net: add missing comment to ValidateIPAM Signed-off-by: Albin Kerouanton --- api/types/network/ipam.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/types/network/ipam.go b/api/types/network/ipam.go index a8e7280e5c874d70dc6d20e8b7b48fd12f5a414a..17f370ef7efeb7a4ba00afaea4d350a12d84b034 100644 --- a/api/types/network/ipam.go +++ b/api/types/network/ipam.go @@ -51,6 +51,8 @@ func HasIPv6Subnets(ipam *IPAM) bool { return false } +// ValidateIPAM checks whether the network's IPAM passed as argument is valid. It returns a joinError of the list of +// errors found. func ValidateIPAM(ipam *IPAM) error { if ipam == nil { return nil From 3092b261e2d2379954194f7889170e808b434034 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Fri, 15 Sep 2023 12:44:30 +0200 Subject: [PATCH 7/9] daemon: move most of validateEndpointSettings into api/t/net Signed-off-by: Albin Kerouanton --- api/types/network/endpoint.go | 81 ++++++++++++++++++++++++++++++++++ daemon/container_operations.go | 75 ++++++++++--------------------- libnetwork/network.go | 24 ++++++++++ 3 files changed, 128 insertions(+), 52 deletions(-) diff --git a/api/types/network/endpoint.go b/api/types/network/endpoint.go index 423214fdb96ca99d83a98e743f18fb02226a6157..a119dca9097547d9046c791208f683e2d90d6e2d 100644 --- a/api/types/network/endpoint.go +++ b/api/types/network/endpoint.go @@ -1,5 +1,13 @@ package network +import ( + "errors" + "fmt" + "net" + + "github.com/docker/docker/internal/multierror" +) + // EndpointSettings stores the network endpoint details type EndpointSettings struct { // Configurations @@ -52,3 +60,76 @@ func (cfg *EndpointIPAMConfig) Copy() *EndpointIPAMConfig { cfgCopy.LinkLocalIPs = append(cfgCopy.LinkLocalIPs, cfg.LinkLocalIPs...) return &cfgCopy } + +// NetworkSubnet describes a user-defined subnet for a specific network. It's only used to validate if an +// EndpointIPAMConfig is valid for a specific network. +type NetworkSubnet interface { + // Contains checks whether the NetworkSubnet contains [addr]. + Contains(addr net.IP) bool + // IsStatic checks whether the subnet was statically allocated (ie. user-defined). + IsStatic() bool +} + +// IsInRange checks whether static IP addresses are valid in a specific network. +func (cfg *EndpointIPAMConfig) IsInRange(v4Subnets []NetworkSubnet, v6Subnets []NetworkSubnet) error { + var errs []error + + if err := validateEndpointIPAddress(cfg.IPv4Address, v4Subnets); err != nil { + errs = append(errs, err) + } + if err := validateEndpointIPAddress(cfg.IPv6Address, v6Subnets); err != nil { + errs = append(errs, err) + } + + return multierror.Join(errs...) +} + +func validateEndpointIPAddress(epAddr string, ipamSubnets []NetworkSubnet) error { + if epAddr == "" { + return nil + } + + var staticSubnet bool + parsedAddr := net.ParseIP(epAddr) + for _, subnet := range ipamSubnets { + if subnet.IsStatic() { + staticSubnet = true + if subnet.Contains(parsedAddr) { + return nil + } + } + } + + if staticSubnet { + return fmt.Errorf("no configured subnet or ip-range contain the IP address %s", epAddr) + } + + return errors.New("user specified IP address is supported only when connecting to networks with user configured subnets") +} + +// Validate checks whether cfg is valid. +func (cfg *EndpointIPAMConfig) Validate() error { + if cfg == nil { + return nil + } + + var errs []error + + if cfg.IPv4Address != "" { + if addr := net.ParseIP(cfg.IPv4Address); addr == nil || addr.To4() == nil || addr.IsUnspecified() { + errs = append(errs, fmt.Errorf("invalid IPv4 address: %s", cfg.IPv4Address)) + } + } + if cfg.IPv6Address != "" { + if addr := net.ParseIP(cfg.IPv6Address); addr == nil || addr.To4() != nil || addr.IsUnspecified() { + errs = append(errs, fmt.Errorf("invalid IPv6 address: %s", cfg.IPv6Address)) + } + } + for _, addr := range cfg.LinkLocalIPs { + if parsed := net.ParseIP(addr); parsed == nil || parsed.IsUnspecified() { + errs = append(errs, fmt.Errorf("invalid link-local IP address: %s", addr)) + } + } + + return multierror.Join(errs...) +} diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 28c654a8c0e67325112e996c2a6c6552ab87d19b..0e41b8b41afe98ad429acf9af3a289964d005b16 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -563,9 +563,8 @@ func (daemon *Daemon) allocateNetwork(cfg *config.Config, container *container.C return nil } -// validateEndpointSettings checks whether the given epConfig is valid. The nw parameter might be nil as a container -// can be created with a reference to a network that don't exist yet. In that case, only partial validation will be -// done. +// validateEndpointSettings checks whether the given epConfig is valid. The nw parameter can be nil, in which case it +// won't try to check if the endpoint IP addresses are within network's subnets. func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *networktypes.EndpointSettings) error { if epConfig == nil { return nil @@ -578,6 +577,8 @@ func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *n var errs []error + // TODO(aker): move this into api/types/network/endpoint.go once enableIPOnPredefinedNetwork and + // serviceDiscoveryOnDefaultNetwork are removed. if !containertypes.NetworkMode(nwName).IsUserDefined() { hasStaticAddresses := ipamConfig.IPv4Address != "" || ipamConfig.IPv6Address != "" // On Linux, user specified IP address is accepted only by networks with user specified subnets. @@ -589,63 +590,33 @@ func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *n } } - if ipamConfig.IPv4Address != "" { - if addr := net.ParseIP(ipamConfig.IPv4Address); addr == nil || addr.To4() == nil || addr.IsUnspecified() { - errs = append(errs, fmt.Errorf("invalid IPv4 address: %s", ipamConfig.IPv4Address)) - } - } - if ipamConfig.IPv6Address != "" { - if addr := net.ParseIP(ipamConfig.IPv6Address); addr == nil || addr.To4() != nil || addr.IsUnspecified() { - errs = append(errs, fmt.Errorf("invalid IPv6 address: %s", ipamConfig.IPv6Address)) - } - } - for _, addr := range ipamConfig.LinkLocalIPs { - if parsed := net.ParseIP(addr); parsed == nil || parsed.IsUnspecified() { - errs = append(errs, fmt.Errorf("invalid link-local IP address %s", addr)) - } + // TODO(aker): add a proper multierror.Append + if err := ipamConfig.Validate(); err != nil { + errs = append(errs, err.(interface{ Unwrap() []error }).Unwrap()...) } - if nw == nil { - return multierror.Join(errs...) - } - - _, _, nwIPv4Configs, nwIPv6Configs := nw.IpamConfig() - if err := validateEndpointIPAddress(nwIPv4Configs, ipamConfig.IPv4Address); err != nil { - errs = append(errs, err) - } - if err := validateEndpointIPAddress(nwIPv6Configs, ipamConfig.IPv6Address); err != nil { - errs = append(errs, err) - } - - return multierror.Join(errs...) -} + if nw != nil { + _, _, v4Configs, v6Configs := nw.IpamConfig() -func validateEndpointIPAddress(nwIPAMConfig []*libnetwork.IpamConf, epAddr string) error { - if epAddr == "" { - return nil - } - - var customSubnet bool - parsedAddr := net.ParseIP(epAddr) - for _, conf := range nwIPAMConfig { - if conf.PreferredPool != "" { - customSubnet = true - - _, allowedRange, _ := net.ParseCIDR(conf.PreferredPool) - if conf.SubPool != "" { - _, allowedRange, _ = net.ParseCIDR(conf.SubPool) - } + var nwIPv4Subnets, nwIPv6Subnets []networktypes.NetworkSubnet + for _, nwIPAMConfig := range v4Configs { + nwIPv4Subnets = append(nwIPv4Subnets, nwIPAMConfig) + } + for _, nwIPAMConfig := range v6Configs { + nwIPv6Subnets = append(nwIPv6Subnets, nwIPAMConfig) + } - if allowedRange.Contains(parsedAddr) { - return nil - } + // TODO(aker): add a proper multierror.Append + if err := ipamConfig.IsInRange(nwIPv4Subnets, nwIPv6Subnets); err != nil { + errs = append(errs, err.(interface{ Unwrap() []error }).Unwrap()...) } } - if customSubnet { - return fmt.Errorf("no predefined subnet or ip-range contain the IP address: %s", epAddr) + if err := multierror.Join(errs...); err != nil { + return fmt.Errorf("invalid endpoint settings:\n%w", err) } - return runconfig.ErrUnsupportedNetworkNoSubnetAndIP + + return nil } // cleanOperationalData resets the operational data from the passed endpoint settings diff --git a/libnetwork/network.go b/libnetwork/network.go index a0fd365a1fe09a6e95cef2314555f228208a5862..a652ddb0dcf500b25582fc4acccca79d7f6959b3 100644 --- a/libnetwork/network.go +++ b/libnetwork/network.go @@ -71,6 +71,8 @@ type networkDBTable struct { } // IpamConf contains all the ipam related configurations for a network +// +// TODO(aker): use proper net/* structs instead of string literals. type IpamConf struct { // PreferredPool is the master address pool for containers and network interfaces. PreferredPool string @@ -92,6 +94,28 @@ func (c *IpamConf) Validate() error { return nil } +// Contains checks whether the ipamSubnet contains [addr]. +func (c *IpamConf) Contains(addr net.IP) bool { + if c == nil { + return false + } + if c.PreferredPool == "" { + return false + } + + _, allowedRange, _ := net.ParseCIDR(c.PreferredPool) + if c.SubPool != "" { + _, allowedRange, _ = net.ParseCIDR(c.SubPool) + } + + return allowedRange.Contains(addr) +} + +// IsStatic checks whether the subnet was statically allocated (ie. user-defined). +func (c *IpamConf) IsStatic() bool { + return c != nil && c.PreferredPool != "" +} + // IpamInfo contains all the ipam related operational info for a network type IpamInfo struct { PoolID string From acf825def27f846ddb0ec4d41ca7f9f5b51df9e5 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Fri, 15 Sep 2023 13:40:30 +0200 Subject: [PATCH 8/9] api/t/net: test EndpointIPAMConfig.Validate() Signed-off-by: Albin Kerouanton --- api/types/network/endpoint_test.go | 188 +++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 api/types/network/endpoint_test.go diff --git a/api/types/network/endpoint_test.go b/api/types/network/endpoint_test.go new file mode 100644 index 0000000000000000000000000000000000000000..c28dfe3245f1ae6cb032a137089165a03b7b54d2 --- /dev/null +++ b/api/types/network/endpoint_test.go @@ -0,0 +1,188 @@ +package network + +import ( + "net" + "testing" + + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" +) + +type subnetStub struct { + static bool + contains map[string]bool +} + +func (stub subnetStub) IsStatic() bool { + return stub.static +} + +func (stub subnetStub) Contains(addr net.IP) bool { + v, ok := stub.contains[addr.String()] + return ok && v +} + +func TestEndpointIPAMConfigWithOutOfRangeAddrs(t *testing.T) { + testcases := []struct { + name string + ipamConfig *EndpointIPAMConfig + v4Subnets []NetworkSubnet + v6Subnets []NetworkSubnet + expectedErrors []string + }{ + { + name: "valid config", + ipamConfig: &EndpointIPAMConfig{ + IPv4Address: "192.168.100.10", + IPv6Address: "2a01:d2:af:420b:25c1:1816:bb33:855c", + LinkLocalIPs: []string{"169.254.169.254", "fe80::42:a8ff:fe33:6230"}, + }, + v4Subnets: []NetworkSubnet{ + subnetStub{static: true, contains: map[string]bool{"192.168.100.10": true}}, + }, + v6Subnets: []NetworkSubnet{ + subnetStub{static: true, contains: map[string]bool{"2a01:d2:af:420b:25c1:1816:bb33:855c": true}}, + }, + }, + { + name: "static addresses out of range", + ipamConfig: &EndpointIPAMConfig{ + IPv4Address: "192.168.100.10", + IPv6Address: "2a01:d2:af:420b:25c1:1816:bb33:855c", + }, + v4Subnets: []NetworkSubnet{ + subnetStub{static: true, contains: map[string]bool{"192.168.100.10": false}}, + }, + v6Subnets: []NetworkSubnet{ + subnetStub{static: true, contains: map[string]bool{"2a01:d2:af:420b:25c1:1816:bb33:855c": false}}, + }, + expectedErrors: []string{ + "no configured subnet or ip-range contain the IP address 192.168.100.10", + "no configured subnet or ip-range contain the IP address 2a01:d2:af:420b:25c1:1816:bb33:855c", + }, + }, + { + name: "static addresses with dynamic network subnets", + ipamConfig: &EndpointIPAMConfig{ + IPv4Address: "192.168.100.10", + IPv6Address: "2a01:d2:af:420b:25c1:1816:bb33:855c", + }, + v4Subnets: []NetworkSubnet{ + subnetStub{static: false}, + }, + v6Subnets: []NetworkSubnet{ + subnetStub{static: false}, + }, + expectedErrors: []string{ + "user specified IP address is supported only when connecting to networks with user configured subnets", + "user specified IP address is supported only when connecting to networks with user configured subnets", + }, + }, + } + + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + err := tc.ipamConfig.IsInRange(tc.v4Subnets, tc.v6Subnets) + if tc.expectedErrors == nil { + assert.NilError(t, err) + return + } + + if _, ok := err.(interface{ Unwrap() []error }); !ok { + t.Fatal("returned error isn't a multierror") + } + errs := err.(interface{ Unwrap() []error }).Unwrap() + assert.Check(t, len(errs) == len(tc.expectedErrors), "errs: %+v", errs) + + for _, expected := range tc.expectedErrors { + assert.Check(t, is.ErrorContains(err, expected)) + } + }) + } + +} + +func TestEndpointIPAMConfigWithInvalidConfig(t *testing.T) { + testcases := []struct { + name string + ipamConfig *EndpointIPAMConfig + expectedErrors []string + }{ + { + name: "valid config", + ipamConfig: &EndpointIPAMConfig{ + IPv4Address: "192.168.100.10", + IPv6Address: "2a01:d2:af:420b:25c1:1816:bb33:855c", + LinkLocalIPs: []string{"169.254.169.254", "fe80::42:a8ff:fe33:6230"}, + }, + }, + { + name: "invalid IP addresses", + ipamConfig: &EndpointIPAMConfig{ + IPv4Address: "foo", + IPv6Address: "bar", + LinkLocalIPs: []string{"baz", "foobar"}, + }, + expectedErrors: []string{ + "invalid IPv4 address: foo", + "invalid IPv6 address: bar", + "invalid link-local IP address: baz", + "invalid link-local IP address: foobar", + }, + }, + { + name: "ipv6 address with a zone", + ipamConfig: &EndpointIPAMConfig{IPv6Address: "fe80::1cc0:3e8c:119f:c2e1%ens18"}, + expectedErrors: []string{ + "invalid IPv6 address: fe80::1cc0:3e8c:119f:c2e1%ens18", + }, + }, + { + name: "unspecified address is invalid", + ipamConfig: &EndpointIPAMConfig{ + IPv4Address: "0.0.0.0", + IPv6Address: "::", + LinkLocalIPs: []string{"0.0.0.0", "::"}, + }, + expectedErrors: []string{ + "invalid IPv4 address: 0.0.0.0", + "invalid IPv6 address: ::", + "invalid link-local IP address: 0.0.0.0", + "invalid link-local IP address: ::", + }, + }, + { + name: "empty link-local", + ipamConfig: &EndpointIPAMConfig{ + LinkLocalIPs: []string{""}, + }, + expectedErrors: []string{"invalid link-local IP address:"}, + }, + } + + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + err := tc.ipamConfig.Validate() + if tc.expectedErrors == nil { + assert.NilError(t, err) + return + } + + if _, ok := err.(interface{ Unwrap() []error }); !ok { + t.Fatal("returned error isn't a multierror") + } + errs := err.(interface{ Unwrap() []error }).Unwrap() + assert.Check(t, len(errs) == len(tc.expectedErrors), "errs: %+v", errs) + + for _, expected := range tc.expectedErrors { + assert.Check(t, is.ErrorContains(err, expected)) + } + }) + } +} From e19e541e2c4bab69a7c7503f1ac4e0003d569c65 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Fri, 15 Sep 2023 16:32:01 +0200 Subject: [PATCH 9/9] libnet: add comment to ipamType Signed-off-by: Albin Kerouanton --- libnetwork/network.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libnetwork/network.go b/libnetwork/network.go index a652ddb0dcf500b25582fc4acccca79d7f6959b3..3787e9dc32925c3ad51571a083da50e67f0a7b96 100644 --- a/libnetwork/network.go +++ b/libnetwork/network.go @@ -174,7 +174,7 @@ type Network struct { created time.Time scope string // network data scope labels map[string]string - ipamType string + ipamType string // ipamType is the name of the IPAM driver ipamOptions map[string]string addrSpace string ipamV4Config []*IpamConf