Merge pull request #1534 from weaveworks/1531-check-hostconfig

Take more care over HostConfig on start request in proxy

proxy/start_container_interceptor.go

@@ -2,41 +2,47 @@ package proxy
 
 import (
 	"net/http"
-
-	"github.com/fsouza/go-dockerclient"
 )
 
 type startContainerInterceptor struct{ proxy *Proxy }
 
-type startContainerRequestBody struct {
-	HostConfig *docker.HostConfig `json:"HostConfig,omitempty" yaml:"HostConfig,omitempty"`
-}
-
 func (i *startContainerInterceptor) InterceptRequest(r *http.Request) error {
 	container, err := inspectContainerInPath(i.proxy.client, r.URL.Path)
 	if err != nil {
 		return err
 	}
 
-	if !containerShouldAttach(container) || r.Header.Get("Content-Type") != "application/json" {
-		i.proxy.createWait(r, container.ID)
-		return nil
-	}
-
-	hostConfig := map[string]interface{}{}
-	if err := unmarshalRequestBody(r, &hostConfig); err != nil {
-		return err
-	}
-
-	i.proxy.addWeaveWaitVolume(hostConfig)
-	if dnsDomain := i.proxy.getDNSDomain(); dnsDomain != "" {
-		if err := i.proxy.setWeaveDNS(hostConfig, container.Config.Hostname, dnsDomain); err != nil {
+	// If the client has sent some JSON which might be a HostConfig, add our
+	// parameters back into it, otherwise Docker will consider them overwritten
+	if containerShouldAttach(container) && r.Header.Get("Content-Type") == "application/json" {
+		params := map[string]interface{}{}
+		if err := unmarshalRequestBody(r, &params); err != nil {
 			return err
 		}
-	}
+		// HostConfig can be sent either as a struct named HostConfig, or unnamed at top level
+		var hostConfig map[string]interface{}
+		if subParam, found := params["HostConfig"]; found {
+			if typecast, ok := subParam.(map[string]interface{}); ok {
+				hostConfig = typecast
+			}
+			// We can't reliably detect what Docker will see as a top-level HostConfig,
+			// so just assume any parameter at top level indicates there is one.
+		} else if len(params) > 0 {
+			hostConfig = params
+		}
+		if hostConfig != nil {
+			i.proxy.addWeaveWaitVolume(hostConfig)
+			if dnsDomain := i.proxy.getDNSDomain(); dnsDomain != "" {
+				if err := i.proxy.setWeaveDNS(hostConfig, container.Config.Hostname, dnsDomain); err != nil {
+					return err
+				}
+			}
 
-	if err := marshalRequestBody(r, hostConfig); err != nil {
-		return err
+			// Note we marshal the original top-level dictionary to avoid disturbing anything else
+			if err := marshalRequestBody(r, params); err != nil {
+				return err
+			}
+		}
 	}
 	i.proxy.createWait(r, container.ID)
 	return nil

test/666_abuse_of_start_test.sh

@@ -8,20 +8,40 @@ weave_on $HOST1 launch
 docker_bridge_ip=$(weave_on $HOST1 docker-bridge-ip)
 proxy_start_container $HOST1 --name=c1
 
+check_hostconfig() {
+    docker_on $HOST1 attach c2 >/dev/null 2>&1 || true # Wait for container to exit
+    assert "docker_on $HOST1 inspect -f '{{.HostConfig.NetworkMode}} {{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' $1" "$2 false 0 [$docker_bridge_ip]"
+}
+
 # Start c2 with a sneaky HostConfig
 proxy docker_on $HOST1 create --name=c2 $SMALL_IMAGE $CHECK_ETHWE_UP
 proxy docker_api_on $HOST1 POST /containers/c2/start '{"NetworkMode": "container:c1"}'
-docker_on $HOST1 attach c2 >/dev/null 2>&1 || true # Wait for container to exit
-assert "docker_on $HOST1 inspect -f '{{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' c2" "false 0 [$docker_bridge_ip]"
+check_hostconfig c2 container:c1
+
+# Start c5 with a differently sneaky HostConfig
+proxy docker_on $HOST1 create --name=c5 $SMALL_IMAGE $CHECK_ETHWE_UP
+proxy docker_api_on $HOST1 POST /containers/c5/start '{"HostConfig": {"NetworkMode": "container:c1"}}'
+check_hostconfig c5 container:c1
 
 # Start c3 with HostConfig having empty binds and null dns/networking settings
 proxy docker_on $HOST1 create --name=c3 -v /tmp:/hosttmp $SMALL_IMAGE $CHECK_ETHWE_UP
 proxy docker_api_on $HOST1 POST /containers/c3/start '{"Binds":[],"Dns":null,"DnsSearch":null,"ExtraHosts":null,"VolumesFrom":null,"Devices":null,"NetworkMode":""}'
-docker_on $HOST1 attach c3 >/dev/null 2>&1 || true # Wait for container to exit
-assert "docker_on $HOST1 inspect -f '{{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' c3" "false 0 [$docker_bridge_ip]"
+check_hostconfig c3 default
 
-# Start c4 with an 'null' HostConfig
-proxy docker_on $HOST1 create --name=c4 $SMALL_IMAGE echo foo
+# Start c4 with an 'null' HostConfig and check this doesn't remove previous parameters
+proxy docker_on $HOST1 create --name=c4 --memory-swap -1 $SMALL_IMAGE echo foo
 assert_raises "proxy docker_api_on $HOST1 POST /containers/c4/start 'null'"
+assert "docker_on $HOST1 inspect -f '{{.HostConfig.MemorySwap}}' c4" "-1"
+
+# Start c6 with both named and unnamed HostConfig
+proxy docker_on $HOST1 create --name=c6 $SMALL_IMAGE $CHECK_ETHWE_UP
+proxy docker_api_on $HOST1 POST /containers/c6/start '{"NetworkMode": "container:c2", "HostConfig": {"NetworkMode": "container:c1"}}'
+check_hostconfig c6 container:c1
+
+# Start c7 with an empty HostConfig and check this removes previous parameters, but still attaches to weave
+proxy docker_on $HOST1 create --name=c7 --memory-swap -1 $SMALL_IMAGE $CHECK_ETHWE_UP
+proxy docker_api_on $HOST1 POST /containers/c7/start '{"HostConfig":{}}'
+assert "docker_on $HOST1 inspect -f '{{.HostConfig.MemorySwap}}' c7" "0"
+check_hostconfig c7 default
 
 end_suite