From faf94b033d1a86a040c71c9ad29fe46da3cafd1a Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 14 Oct 2015 10:38:51 +0100 Subject: [PATCH 1/7] Extend test to check we don't overwrite HostConfig --- test/666_abuse_of_start_test.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/666_abuse_of_start_test.sh b/test/666_abuse_of_start_test.sh index fee0e2e9..d575e311 100755 --- a/test/666_abuse_of_start_test.sh +++ b/test/666_abuse_of_start_test.sh @@ -20,8 +20,9 @@ proxy docker_api_on $HOST1 POST /containers/c3/start '{"Binds":[],"Dns":null,"Dn 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]" -# 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" end_suite From 20c8b1eaf6e701f50b23ac5198e7b9fd6a033266 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 14 Oct 2015 10:38:53 +0100 Subject: [PATCH 2/7] Reverse the sense of a test, to simplify the next change and remove unused struct --- proxy/start_container_interceptor.go | 35 +++++++++++----------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/proxy/start_container_interceptor.go b/proxy/start_container_interceptor.go index bac9b887..0d8963f4 100644 --- a/proxy/start_container_interceptor.go +++ b/proxy/start_container_interceptor.go @@ -2,41 +2,32 @@ 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 containerShouldAttach(container) && r.Header.Get("Content-Type") == "application/json" { + hostConfig := map[string]interface{}{} + if err := unmarshalRequestBody(r, &hostConfig); err != nil { return err } - } - if err := marshalRequestBody(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 { + return err + } + } + + if err := marshalRequestBody(r, hostConfig); err != nil { + return err + } } i.proxy.createWait(r, container.ID) return nil From 50198a2159c46c1089f730539b08509387774e94 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 14 Oct 2015 10:43:31 +0100 Subject: [PATCH 3/7] Don't create a HostConfig accidentally if none was passed in --- proxy/start_container_interceptor.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/proxy/start_container_interceptor.go b/proxy/start_container_interceptor.go index 0d8963f4..89d0ebc4 100644 --- a/proxy/start_container_interceptor.go +++ b/proxy/start_container_interceptor.go @@ -12,22 +12,25 @@ func (i *startContainerInterceptor) InterceptRequest(r *http.Request) error { return err } + // 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" { hostConfig := map[string]interface{}{} if err := unmarshalRequestBody(r, &hostConfig); err != nil { return err } + if len(hostConfig) > 0 { + i.proxy.addWeaveWaitVolume(hostConfig) + if dnsDomain := i.proxy.getDNSDomain(); dnsDomain != "" { + if err := i.proxy.setWeaveDNS(hostConfig, container.Config.Hostname, dnsDomain); 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 err := marshalRequestBody(r, hostConfig); err != nil { return err } } - - if err := marshalRequestBody(r, hostConfig); err != nil { - return err - } } i.proxy.createWait(r, container.ID) return nil From 81c9b3f32732b69263014f6b2cdb3b18b5c4fcbd Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 14 Oct 2015 10:55:23 +0100 Subject: [PATCH 4/7] Check that HostConfig parameters supplied on /start are passed to Docker, also what happens when you supply a HostConfig named HostConfig --- test/666_abuse_of_start_test.sh | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/666_abuse_of_start_test.sh b/test/666_abuse_of_start_test.sh index d575e311..81ed84ab 100755 --- a/test/666_abuse_of_start_test.sh +++ b/test/666_abuse_of_start_test.sh @@ -12,7 +12,12 @@ proxy_start_container $HOST1 --name=c1 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]" +assert "docker_on $HOST1 inspect -f '{{.HostConfig.NetworkMode}} {{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' c2" "container:c1 false 0 [$docker_bridge_ip]" + +# 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"}}' +assert "docker_on $HOST1 inspect -f '{{.HostConfig.NetworkMode}} {{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' c5" "container:c1 false 0 [$docker_bridge_ip]" # 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 @@ -25,4 +30,9 @@ 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"}}' +assert "docker_on $HOST1 inspect -f '{{.HostConfig.NetworkMode}} {{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' c6" "container:c1 false 0 [$docker_bridge_ip]" + end_suite From 17b030cca14edbcb8566753f4397cd2d8ee171f2 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 14 Oct 2015 11:13:41 +0100 Subject: [PATCH 5/7] Cope with client sending a HostConfig named HostConfig --- proxy/start_container_interceptor.go | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/proxy/start_container_interceptor.go b/proxy/start_container_interceptor.go index 89d0ebc4..1d0d17df 100644 --- a/proxy/start_container_interceptor.go +++ b/proxy/start_container_interceptor.go @@ -15,11 +15,22 @@ func (i *startContainerInterceptor) InterceptRequest(r *http.Request) error { // 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" { - hostConfig := map[string]interface{}{} - if err := unmarshalRequestBody(r, &hostConfig); err != nil { + params := map[string]interface{}{} + if err := unmarshalRequestBody(r, ¶ms); err != nil { return err } - if len(hostConfig) > 0 { + // 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 { @@ -27,7 +38,8 @@ func (i *startContainerInterceptor) InterceptRequest(r *http.Request) error { } } - if err := marshalRequestBody(r, hostConfig); err != nil { + // Note we marshal the original top-level dictionary to avoid disturbing anything else + if err := marshalRequestBody(r, params); err != nil { return err } } From 119d87b795ca31af8ae6dbf3fbc5a4773c925c76 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Wed, 14 Oct 2015 12:04:57 +0100 Subject: [PATCH 6/7] Extract repeated test into function --- test/666_abuse_of_start_test.sh | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/666_abuse_of_start_test.sh b/test/666_abuse_of_start_test.sh index 81ed84ab..39b41297 100755 --- a/test/666_abuse_of_start_test.sh +++ b/test/666_abuse_of_start_test.sh @@ -8,22 +8,25 @@ 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 '{{.HostConfig.NetworkMode}} {{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' c2" "container:c1 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"}}' -assert "docker_on $HOST1 inspect -f '{{.HostConfig.NetworkMode}} {{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' c5" "container:c1 false 0 [$docker_bridge_ip]" +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 and check this doesn't remove previous parameters proxy docker_on $HOST1 create --name=c4 --memory-swap -1 $SMALL_IMAGE echo foo @@ -33,6 +36,6 @@ 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"}}' -assert "docker_on $HOST1 inspect -f '{{.HostConfig.NetworkMode}} {{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' c6" "container:c1 false 0 [$docker_bridge_ip]" +check_hostconfig c6 container:c1 end_suite From 526c9c3d7313901cb079923d9c35a7f43108c559 Mon Sep 17 00:00:00 2001 From: Paul Bellamy Date: Mon, 19 Oct 2015 13:38:10 +0100 Subject: [PATCH 7/7] Add test for {"HostConfig":{}} overriding previous settings, but still connecting to weave --- test/666_abuse_of_start_test.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/666_abuse_of_start_test.sh b/test/666_abuse_of_start_test.sh index 39b41297..41761583 100755 --- a/test/666_abuse_of_start_test.sh +++ b/test/666_abuse_of_start_test.sh @@ -38,4 +38,10 @@ 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