From 638cf86cbe9f3975e7d7a4ad67e656cb54f4ed73 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 2 Jul 2019 13:21:04 +0200 Subject: [PATCH 1/4] TestMaskSecretKeys: add more test-cases Add tests for - case-insensitive matching of fields - recursive masking Signed-off-by: Sebastiaan van Stijn (cherry picked from commit db5f811216e70bcb4a10e477c1558d6c68f618c5) Signed-off-by: Tibor Vass (cherry picked from commit 18dac2cf32faeaada3bd4e8e2bffa576ad4329fe) Signed-off-by: Sebastiaan van Stijn --- api/server/middleware/debug_test.go | 30 ++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/api/server/middleware/debug_test.go b/api/server/middleware/debug_test.go index a64b73e0d7..3d78d7e084 100644 --- a/api/server/middleware/debug_test.go +++ b/api/server/middleware/debug_test.go @@ -23,7 +23,6 @@ func TestMaskSecretKeys(t *testing.T) { input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, }, - { path: "/secrets/create?key=val", input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, @@ -32,8 +31,13 @@ func TestMaskSecretKeys(t *testing.T) { { path: "/v1.30/some/other/path", input: map[string]interface{}{ - "password": "pass", + "password": "pass", + "secret": "secret", + "jointoken": "jointoken", + "unlockkey": "unlockkey", + "signingcakey": "signingcakey", "other": map[string]interface{}{ + "password": "pass", "secret": "secret", "jointoken": "jointoken", "unlockkey": "unlockkey", @@ -41,8 +45,13 @@ func TestMaskSecretKeys(t *testing.T) { }, }, expected: map[string]interface{}{ - "password": "*****", + "password": "*****", + "secret": "*****", + "jointoken": "*****", + "unlockkey": "*****", + "signingcakey": "*****", "other": map[string]interface{}{ + "password": "*****", "secret": "*****", "jointoken": "*****", "unlockkey": "*****", @@ -50,6 +59,21 @@ func TestMaskSecretKeys(t *testing.T) { }, }, }, + { + path: "/v1.30/some/other/path", + input: map[string]interface{}{ + "PASSWORD": "pass", + "other": map[string]interface{}{ + "PASSWORD": "pass", + }, + }, + expected: map[string]interface{}{ + "PASSWORD": "*****", + "other": map[string]interface{}{ + "PASSWORD": "*****", + }, + }, + }, } for _, testcase := range tests { From 685f13f3fd29689edba4ed6e51a3cb0a90b8ee1f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 2 Jul 2019 13:29:24 +0200 Subject: [PATCH 2/4] TestMaskSecretKeys: use subtests Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 32d70c7e21631224674cd60021d3ec908c2d888c) Signed-off-by: Tibor Vass (cherry picked from commit ebb542b3f88d7f5551f6b6e1d8d2774a2c166409) Signed-off-by: Sebastiaan van Stijn --- api/server/middleware/debug_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/api/server/middleware/debug_test.go b/api/server/middleware/debug_test.go index 3d78d7e084..e19a0ced2f 100644 --- a/api/server/middleware/debug_test.go +++ b/api/server/middleware/debug_test.go @@ -9,26 +9,31 @@ import ( func TestMaskSecretKeys(t *testing.T) { tests := []struct { + doc string path string input map[string]interface{} expected map[string]interface{} }{ { + doc: "secret create with API version", path: "/v1.30/secrets/create", input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, }, { + doc: "secret create with API version and trailing slashes", path: "/v1.30/secrets/create//", input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, }, { + doc: "secret create with query param", path: "/secrets/create?key=val", input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, }, { + doc: "other paths with API version", path: "/v1.30/some/other/path", input: map[string]interface{}{ "password": "pass", @@ -60,6 +65,7 @@ func TestMaskSecretKeys(t *testing.T) { }, }, { + doc: "other paths with API version case insensitive", path: "/v1.30/some/other/path", input: map[string]interface{}{ "PASSWORD": "pass", @@ -77,7 +83,9 @@ func TestMaskSecretKeys(t *testing.T) { } for _, testcase := range tests { - maskSecretKeys(testcase.input, testcase.path) - assert.Check(t, is.DeepEqual(testcase.expected, testcase.input)) + t.Run(testcase.doc, func(t *testing.T) { + maskSecretKeys(testcase.input, testcase.path) + assert.Check(t, is.DeepEqual(testcase.expected, testcase.input)) + }) } } From 1eadbf1bd02b039b6ad4625e385be8640a4b32a4 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 2 Jul 2019 14:21:03 +0200 Subject: [PATCH 3/4] DebugRequestMiddleware: unconditionally scrub data field Commit 77b8465d7e68ca102d7aae839c7b3fe0ecd28398 added a secret update endpoint to allow updating labels on existing secrets. However, when implementing the endpoint, the DebugRequestMiddleware was not updated to scrub the Data field (as is being done when creating a secret). When updating a secret (to set labels), the Data field should be either `nil` (not set), or contain the same value as the existing secret. In situations where the Data field is set, and the `dockerd` daemon is running with debugging enabled / log-level debug, the base64-encoded value of the secret is printed to the daemon logs. The docker cli does not have a `docker secret update` command, but when using `docker stack deploy`, the docker cli sends the secret data both when _creating_ a stack, and when _updating_ a stack, thus leaking the secret data if the daemon runs with debug enabled: 1. Start the daemon in debug-mode dockerd --debug 2. Initialize swarm docker swarm init 3. Create a file containing a secret echo secret > my_secret.txt 4. Create a docker-compose file using that secret cat > docker-compose.yml <<'EOF' version: "3.3" services: web: image: nginx:alpine secrets: - my_secret secrets: my_secret: file: ./my_secret.txt EOF 5. Deploy the stack docker stack deploy -c docker-compose.yml test 6. Verify that the secret is scrubbed in the daemon logs DEBU[2019-07-01T22:36:08.170617400Z] Calling POST /v1.30/secrets/create DEBU[2019-07-01T22:36:08.171364900Z] form data: {"Data":"*****","Labels":{"com.docker.stack.namespace":"test"},"Name":"test_my_secret"} 7. Re-deploy the stack to trigger an "update" docker stack deploy -c docker-compose.yml test 8. Notice that this time, the Data field is not scrubbed, and the base64-encoded secret is logged DEBU[2019-07-01T22:37:35.828819400Z] Calling POST /v1.30/secrets/w3hgvwpzl8yooq5ctnyp71v52/update?version=34 DEBU[2019-07-01T22:37:35.829993700Z] form data: {"Data":"c2VjcmV0Cg==","Labels":{"com.docker.stack.namespace":"test"},"Name":"test_my_secret"} This patch modifies `maskSecretKeys` to unconditionally scrub `Data` fields. Currently, only the `secrets` and `configs` endpoints use a field with this name, and no other POST API endpoints use a data field, so scrubbing this field unconditionally will only scrub requests for those endpoints. If a new endpoint is added in future where this field should not be scrubbed, we can re-introduce more fine-grained (path-specific) handling. This patch introduces some change in behavior: - In addition to secrets, requests to create or update _configs_ will now have their `Data` field scrubbed. Generally, the actual data should not be interesting for debugging, so likely will not be problematic. In addition, scrubbing this data for configs may actually be desirable, because (even though they are not explicitely designed for this purpose) configs may contain sensitive data (credentials inside a configuration file, e.g.). - Requests that send key/value pairs as a "map" and that contain a key named "data", will see the value of that field scrubbed. This means that (e.g.) setting a `label` named `data` on a config, will scrub/mask the value of that label. - Note that this is already the case for any label named `jointoken`, `password`, `secret`, `signingcakey`, or `unlockkey`. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit c7ce4be93ae8edd2da62a588e01c67313a4aba0c) Signed-off-by: Tibor Vass (cherry picked from commit 73db8c77bfb2d0cbdf71ce491f3d3e66c9dd5be6) Signed-off-by: Sebastiaan van Stijn --- api/server/middleware/debug.go | 24 ++++++++++++++---------- api/server/middleware/debug_test.go | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/api/server/middleware/debug.go b/api/server/middleware/debug.go index 2cef1d46c3..31165bf918 100644 --- a/api/server/middleware/debug.go +++ b/api/server/middleware/debug.go @@ -71,9 +71,22 @@ func maskSecretKeys(inp interface{}, path string) { } if form, ok := inp.(map[string]interface{}); ok { + scrub := []string{ + // Note: The Data field contains the base64-encoded secret in 'secret' + // and 'config' create and update requests. Currently, no other POST + // API endpoints use a data field, so we scrub this field unconditionally. + // Change this handling to be conditional if a new endpoint is added + // in future where this field should not be scrubbed. + "data", + "jointoken", + "password", + "secret", + "signingcakey", + "unlockkey", + } loop0: for k, v := range form { - for _, m := range []string{"password", "secret", "jointoken", "unlockkey", "signingcakey"} { + for _, m := range scrub { if strings.EqualFold(m, k) { form[k] = "*****" continue loop0 @@ -81,14 +94,5 @@ func maskSecretKeys(inp interface{}, path string) { } maskSecretKeys(v, path) } - - // Route-specific redactions - if strings.HasSuffix(path, "/secrets/create") { - for k := range form { - if k == "Data" { - form[k] = "*****" - } - } - } } } diff --git a/api/server/middleware/debug_test.go b/api/server/middleware/debug_test.go index e19a0ced2f..361273feda 100644 --- a/api/server/middleware/debug_test.go +++ b/api/server/middleware/debug_test.go @@ -32,6 +32,24 @@ func TestMaskSecretKeys(t *testing.T) { input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, }, + { + doc: "secret update with API version", + path: "/v1.30/secrets/mysecret/update", + input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, + expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, + }, + { + doc: "secret update with API version and trailing slashes", + path: "/v1.30/secrets/mysecret/update//", + input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, + expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, + }, + { + doc: "secret update with query parameter", + path: "/secrets/mysecret/update?version=34", + input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, + expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, + }, { doc: "other paths with API version", path: "/v1.30/some/other/path", From c6873818701a40549207214d4c15f1588ed2b1d5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 3 Jul 2019 16:16:22 +0200 Subject: [PATCH 4/4] DebugRequestMiddleware: Remove path handling Path-specific rules were removed, so this is no longer used. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 530e63c1a61b105a6f7fc143c5acb9b5cd87f958) Signed-off-by: Tibor Vass (cherry picked from commit f8a0f26843bc5aff33cf9201b75bd4bdbb48a3ad) Signed-off-by: Sebastiaan van Stijn --- api/server/middleware/debug.go | 16 +++-------- api/server/middleware/debug_test.go | 42 +++-------------------------- 2 files changed, 8 insertions(+), 50 deletions(-) diff --git a/api/server/middleware/debug.go b/api/server/middleware/debug.go index 31165bf918..a02c1bc7de 100644 --- a/api/server/middleware/debug.go +++ b/api/server/middleware/debug.go @@ -41,7 +41,7 @@ func DebugRequestMiddleware(handler func(ctx context.Context, w http.ResponseWri var postForm map[string]interface{} if err := json.Unmarshal(b, &postForm); err == nil { - maskSecretKeys(postForm, r.RequestURI) + maskSecretKeys(postForm) formStr, errMarshal := json.Marshal(postForm) if errMarshal == nil { logrus.Debugf("form data: %s", string(formStr)) @@ -54,18 +54,10 @@ func DebugRequestMiddleware(handler func(ctx context.Context, w http.ResponseWri } } -func maskSecretKeys(inp interface{}, path string) { - // Remove any query string from the path - idx := strings.Index(path, "?") - if idx != -1 { - path = path[:idx] - } - // Remove trailing / characters - path = strings.TrimRight(path, "/") - +func maskSecretKeys(inp interface{}) { if arr, ok := inp.([]interface{}); ok { for _, f := range arr { - maskSecretKeys(f, path) + maskSecretKeys(f) } return } @@ -92,7 +84,7 @@ func maskSecretKeys(inp interface{}, path string) { continue loop0 } } - maskSecretKeys(v, path) + maskSecretKeys(v) } } } diff --git a/api/server/middleware/debug_test.go b/api/server/middleware/debug_test.go index 361273feda..fb1740d54a 100644 --- a/api/server/middleware/debug_test.go +++ b/api/server/middleware/debug_test.go @@ -10,49 +10,16 @@ import ( func TestMaskSecretKeys(t *testing.T) { tests := []struct { doc string - path string input map[string]interface{} expected map[string]interface{} }{ { - doc: "secret create with API version", - path: "/v1.30/secrets/create", + doc: "secret/config create and update requests", input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, }, { - doc: "secret create with API version and trailing slashes", - path: "/v1.30/secrets/create//", - input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, - expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, - }, - { - doc: "secret create with query param", - path: "/secrets/create?key=val", - input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, - expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, - }, - { - doc: "secret update with API version", - path: "/v1.30/secrets/mysecret/update", - input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, - expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, - }, - { - doc: "secret update with API version and trailing slashes", - path: "/v1.30/secrets/mysecret/update//", - input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, - expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, - }, - { - doc: "secret update with query parameter", - path: "/secrets/mysecret/update?version=34", - input: map[string]interface{}{"Data": "foo", "Name": "name", "Labels": map[string]interface{}{}}, - expected: map[string]interface{}{"Data": "*****", "Name": "name", "Labels": map[string]interface{}{}}, - }, - { - doc: "other paths with API version", - path: "/v1.30/some/other/path", + doc: "masking other fields (recursively)", input: map[string]interface{}{ "password": "pass", "secret": "secret", @@ -83,8 +50,7 @@ func TestMaskSecretKeys(t *testing.T) { }, }, { - doc: "other paths with API version case insensitive", - path: "/v1.30/some/other/path", + doc: "case insensitive field matching", input: map[string]interface{}{ "PASSWORD": "pass", "other": map[string]interface{}{ @@ -102,7 +68,7 @@ func TestMaskSecretKeys(t *testing.T) { for _, testcase := range tests { t.Run(testcase.doc, func(t *testing.T) { - maskSecretKeys(testcase.input, testcase.path) + maskSecretKeys(testcase.input) assert.Check(t, is.DeepEqual(testcase.expected, testcase.input)) }) }