From f2424bd3753af96ab2be741ae55af11841a8f0bd Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 13 Feb 2019 16:47:30 +0100 Subject: [PATCH 1/3] Fix labels copying value from environment variables This patch fixes a bug where labels use the same behavior as `--env`, resulting in a value to be copied from environment variables with the same name as the label if no value is set (i.e. a simple key, no `=` sign, no value). An earlier pull request addressed similar cases for `docker run`; 2b17f4c8a8caad552025edb05a73db683fb8a5c6, but this did not address the same situation for (e.g.) `docker service create`. Digging in history for this bug, I found that use of the `ValidateEnv` function for labels was added in the original implementation of the labels feature in https://github.com/docker/docker/commit/abb5e9a0777469e64fe2c7ecfa66ea01083d2071#diff-ae476143d40e21ac0918630f7365ed3cR34 However, the design never intended it to expand environment variables, and use of this function was either due to either a "copy/paste" of the equivalent `--env` flags, or a misunderstanding (the name `ValidateEnv` does not communicate that it also expands environment variables), and the existing `ValidateLabel` was designed for _engine_ labels (which required a value to be set). Following the initial implementation, other parts of the code followed the same (incorrect) approach, therefore leading the bug to be introduced in services as well. This patch: - updates the `ValidateLabel` to match the expected validation rules (this function is no longer used since 31dc5c0a9a8bdc11c7ad335aebb753ed527caa5a), and the daemon has its own implementation) - corrects various locations in the code where `ValidateEnv` was used instead of `ValidateLabel`. Before this patch: ```bash export SOME_ENV_VAR=I_AM_SOME_ENV_VAR docker service create --label SOME_ENV_VAR --tty --name test busybox docker service inspect --format '{{json .Spec.Labels}}' test {"SOME_ENV_VAR":"I_AM_SOME_ENV_VAR"} ``` After this patch: ```bash export SOME_ENV_VAR=I_AM_SOME_ENV_VAR docker service create --label SOME_ENV_VAR --tty --name test busybox docker container inspect --format '{{json .Config.Labels}}' test {"SOME_ENV_VAR":""} ``` Signed-off-by: Sebastiaan van Stijn --- cli/command/config/create.go | 2 +- cli/command/image/build.go | 2 +- cli/command/network/create.go | 2 +- cli/command/secret/create.go | 2 +- cli/command/service/opts.go | 4 +- cli/command/volume/create.go | 2 +- opts/file.go | 2 +- opts/opts.go | 20 +++++- opts/opts_test.go | 111 ++++++++++++++++++++++++++++++---- 9 files changed, 124 insertions(+), 23 deletions(-) diff --git a/cli/command/config/create.go b/cli/command/config/create.go index fc19370ccb73..17cb4184f338 100644 --- a/cli/command/config/create.go +++ b/cli/command/config/create.go @@ -25,7 +25,7 @@ type CreateOptions struct { func newConfigCreateCommand(dockerCli command.Cli) *cobra.Command { createOpts := CreateOptions{ - Labels: opts.NewListOpts(opts.ValidateEnv), + Labels: opts.NewListOpts(opts.ValidateLabel), } cmd := &cobra.Command{ diff --git a/cli/command/image/build.go b/cli/command/image/build.go index 8f925cc0b0b7..3c67946b2956 100644 --- a/cli/command/image/build.go +++ b/cli/command/image/build.go @@ -93,7 +93,7 @@ func newBuildOptions() buildOptions { tags: opts.NewListOpts(validateTag), buildArgs: opts.NewListOpts(opts.ValidateEnv), ulimits: opts.NewUlimitOpt(&ulimits), - labels: opts.NewListOpts(opts.ValidateEnv), + labels: opts.NewListOpts(opts.ValidateLabel), extraHosts: opts.NewListOpts(opts.ValidateExtraHost), } } diff --git a/cli/command/network/create.go b/cli/command/network/create.go index a8dda0a25819..bcb3729426fd 100644 --- a/cli/command/network/create.go +++ b/cli/command/network/create.go @@ -39,7 +39,7 @@ type createOptions struct { func newCreateCommand(dockerCli command.Cli) *cobra.Command { options := createOptions{ driverOpts: *opts.NewMapOpts(nil, nil), - labels: opts.NewListOpts(opts.ValidateEnv), + labels: opts.NewListOpts(opts.ValidateLabel), ipamAux: *opts.NewMapOpts(nil, nil), ipamOpt: *opts.NewMapOpts(nil, nil), } diff --git a/cli/command/secret/create.go b/cli/command/secret/create.go index 1739fefa9ca2..1f570bcac332 100644 --- a/cli/command/secret/create.go +++ b/cli/command/secret/create.go @@ -25,7 +25,7 @@ type createOptions struct { func newSecretCreateCommand(dockerCli command.Cli) *cobra.Command { options := createOptions{ - labels: opts.NewListOpts(opts.ValidateEnv), + labels: opts.NewListOpts(opts.ValidateLabel), } cmd := &cobra.Command{ diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index ec4ea403d6cf..e2620ada353c 100644 --- a/cli/command/service/opts.go +++ b/cli/command/service/opts.go @@ -520,9 +520,9 @@ type serviceOptions struct { func newServiceOptions() *serviceOptions { return &serviceOptions{ - labels: opts.NewListOpts(opts.ValidateEnv), + labels: opts.NewListOpts(opts.ValidateLabel), constraints: opts.NewListOpts(nil), - containerLabels: opts.NewListOpts(opts.ValidateEnv), + containerLabels: opts.NewListOpts(opts.ValidateLabel), env: opts.NewListOpts(opts.ValidateEnv), envFile: opts.NewListOpts(nil), groups: opts.NewListOpts(nil), diff --git a/cli/command/volume/create.go b/cli/command/volume/create.go index b25ed155cb69..125da4433204 100644 --- a/cli/command/volume/create.go +++ b/cli/command/volume/create.go @@ -22,7 +22,7 @@ type createOptions struct { func newCreateCommand(dockerCli command.Cli) *cobra.Command { options := createOptions{ driverOpts: *opts.NewMapOpts(nil, nil), - labels: opts.NewListOpts(opts.ValidateEnv), + labels: opts.NewListOpts(opts.ValidateLabel), } cmd := &cobra.Command{ diff --git a/opts/file.go b/opts/file.go index 1721a46ef9ab..96c0c7b84ebe 100644 --- a/opts/file.go +++ b/opts/file.go @@ -10,7 +10,7 @@ import ( "unicode/utf8" ) -var whiteSpaces = " \t" +const whiteSpaces = " \t" // ErrBadKey typed error for bad environment variable type ErrBadKey struct { diff --git a/opts/opts.go b/opts/opts.go index 6485e309e5f5..7e10a0c4f568 100644 --- a/opts/opts.go +++ b/opts/opts.go @@ -267,10 +267,24 @@ func validateDomain(val string) (string, error) { } // ValidateLabel validates that the specified string is a valid label, and returns it. -// Labels are in the form on key=value. +// +// Labels are in the form of key=value; key must be a non-empty string, and not +// contain whitespaces. A value is optional (defaults to an empty string if omitted). +// +// Leading whitespace is removed during validation but values are kept as-is +// otherwise, so any string value is accepted for both, which includes whitespace +// (for values) and quotes (surrounding, or embedded in key or value). +// +// TODO discuss if quotes (and other special characters) should be valid or invalid for keys +// TODO discuss if leading/trailing whitespace in keys should be preserved (and valid) func ValidateLabel(val string) (string, error) { - if strings.Count(val, "=") < 1 { - return "", fmt.Errorf("bad attribute format: %s", val) + arr := strings.SplitN(val, "=", 2) + key := strings.TrimLeft(arr[0], whiteSpaces) + if key == "" { + return "", fmt.Errorf("empty label name: '%s'", val) + } + if strings.ContainsAny(key, whiteSpaces) { + return "", fmt.Errorf("label '%s' has white spaces", key) } return val, nil } diff --git a/opts/opts_test.go b/opts/opts_test.go index 723a212ea74a..664c1010d253 100644 --- a/opts/opts_test.go +++ b/opts/opts_test.go @@ -4,6 +4,8 @@ import ( "fmt" "strings" "testing" + + "gotest.tools/assert" ) func TestValidateIPAddress(t *testing.T) { @@ -175,19 +177,104 @@ func TestValidateDNSSearch(t *testing.T) { } func TestValidateLabel(t *testing.T) { - if _, err := ValidateLabel("label"); err == nil || err.Error() != "bad attribute format: label" { - t.Fatalf("Expected an error [bad attribute format: label], go %v", err) - } - if actual, err := ValidateLabel("key1=value1"); err != nil || actual != "key1=value1" { - t.Fatalf("Expected [key1=value1], got [%v,%v]", actual, err) - } - // Validate it's working with more than one = - if actual, err := ValidateLabel("key1=value1=value2"); err != nil { - t.Fatalf("Expected [key1=value1=value2], got [%v,%v]", actual, err) + tests := []struct { + name string + value string + expectedErr string + }{ + { + name: "empty", + expectedErr: `empty label name: ''`, + }, + { + name: "whitespace only ", + value: " ", + expectedErr: `empty label name: ' '`, + }, + { + name: "whitespace around equal-sign", + value: " = ", + expectedErr: `empty label name: ' = '`, + }, + { + name: "leading whitespace", + value: " label=value", + }, + { + name: "whitespaces in key without value", + value: "this is a label without value", + expectedErr: `label 'this is a label without value' has white spaces`, + }, + { + name: "whitespaces in key", + value: "this is a label=value", + expectedErr: `label 'this is a label' has white spaces`, + }, + { + name: "whitespaces in value", + value: "label=a value that has whitespace", + }, + { + name: "trailing whitespace in value", + value: "label=value ", + }, + { + name: "leading whitespace in value", + value: "label= value", + }, + { + name: "no value", + value: "label", + }, + { + name: "no key", + value: "=label", + expectedErr: `empty label name: '=label'`, + }, + { + name: "empty value", + value: "label=", + }, + { + name: "key value", + value: "key1=value1", + }, + { + name: "double equal-signs", + value: "key1=value1=value2", + }, + { + name: "multiple equal-signs", + value: "key1=value1=value2=value", + }, + { + name: "double quotes in key and value", + value: `key"with"quotes={"hello"}`, + }, + { + name: "double quotes around key and value", + value: `"quoted-label"="quoted value"`, + }, + { + name: "single quotes in key and value", + value: `key'with'quotes=hello'with'quotes`, + }, + { + name: "single quotes around key and value", + value: `'quoted-label'='quoted value''`, + }, } - // Validate it's working with one more - if actual, err := ValidateLabel("key1=value1=value2=value3"); err != nil { - t.Fatalf("Expected [key1=value1=value2=value2], got [%v,%v]", actual, err) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + val, err := ValidateLabel(tc.value) + if tc.expectedErr != "" { + assert.Error(t, err, tc.expectedErr) + return + } + assert.NilError(t, err) + assert.Equal(t, val, tc.value) + }) } } From b5d0d179e713194349f0954568907721a38c206f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 13 Feb 2019 16:48:59 +0100 Subject: [PATCH 2/3] Add back validation for invalid label values on containers This adds validation to `docker container run` / `docker container create`; Validation of labels provided through flags was removed in 31dc5c0a9a8bdc11c7ad335aebb753ed527caa5a, after the validation was changed to fix labels without values, and to prevent labels from being expanded with environment variables in 2b17f4c8a8caad552025edb05a73db683fb8a5c6 However, now empty label names from _files_ (`--label-file`) followed different validation rules than labels passed through `--label`. This patch adds back minimal validation for labels passed through the command-line Before this patch: ```bash docker container create \ --name label \ --label==with-leading-equal-sign \ --label=without-value \ --label=somelabel=somevalue \ --label " = " \ --label=with-quotes-in-value='{"foo"}' \ --label='with"quotes"in-key=test' \ busybox docker container inspect --format '{{json .Config.Labels}}' label ``` ```json { "": "with-leading-equal-sign", " ": " ", "somelabel": "somevalue", "with\"quotes\"in-key": "test", "with-quotes-in-value": "{\"foo\"}", "without-value": "" } ``` After this patch: ```bash docker container create \ --name label \ --label==with-leading-equal-sign \ --label=without-value \ --label=somelabel=somevalue \ --label " = " \ --label=with-quotes-in-value='{"foo"}' \ --label='with"quotes"in-key=test' \ busybox invalid argument "=with-leading-equal-sign" for "-l, --label" flag: invalid label format: "=with-leading-equal-sign" ``` Signed-off-by: Sebastiaan van Stijn --- cli/command/container/opts.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index c98a26adced1..8fe4ded9c89f 100644 --- a/cli/command/container/opts.go +++ b/cli/command/container/opts.go @@ -147,7 +147,7 @@ func addFlags(flags *pflag.FlagSet) *containerOptions { expose: opts.NewListOpts(nil), extraHosts: opts.NewListOpts(opts.ValidateExtraHost), groupAdd: opts.NewListOpts(nil), - labels: opts.NewListOpts(nil), + labels: opts.NewListOpts(opts.ValidateLabel), labelsFile: opts.NewListOpts(nil), linkLocalIPs: opts.NewListOpts(nil), links: opts.NewListOpts(opts.ValidateLink), From e5702e000c89d1afe1e39587d9891092552a8375 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 19 Mar 2019 03:17:02 +0100 Subject: [PATCH 3/3] Tweak validation messages Signed-off-by: Sebastiaan van Stijn --- opts/envfile_test.go | 4 ++-- opts/file.go | 2 +- opts/opts.go | 4 ++-- opts/opts_test.go | 12 ++++++------ 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/opts/envfile_test.go b/opts/envfile_test.go index bda9dcb6e7dc..f875707a5eb0 100644 --- a/opts/envfile_test.go +++ b/opts/envfile_test.go @@ -100,7 +100,7 @@ func TestParseEnvFileBadlyFormattedFile(t *testing.T) { if _, ok := err.(ErrBadKey); !ok { t.Fatalf("Expected an ErrBadKey, got [%v]", err) } - expectedMessage := "poorly formatted environment: variable 'f ' has white spaces" + expectedMessage := "poorly formatted environment: variable 'f ' contains whitespaces" if err.Error() != expectedMessage { t.Fatalf("Expected [%v], got [%v]", expectedMessage, err.Error()) } @@ -134,7 +134,7 @@ another invalid line` if _, ok := err.(ErrBadKey); !ok { t.Fatalf("Expected an ErrBadKey, got [%v]", err) } - expectedMessage := "poorly formatted environment: variable 'first line' has white spaces" + expectedMessage := "poorly formatted environment: variable 'first line' contains whitespaces" if err.Error() != expectedMessage { t.Fatalf("Expected [%v], got [%v]", expectedMessage, err.Error()) } diff --git a/opts/file.go b/opts/file.go index 96c0c7b84ebe..2346cc1670b3 100644 --- a/opts/file.go +++ b/opts/file.go @@ -51,7 +51,7 @@ func parseKeyValueFile(filename string, emptyFn func(string) (string, bool)) ([] // trim the front of a variable, but nothing else variable := strings.TrimLeft(data[0], whiteSpaces) if strings.ContainsAny(variable, whiteSpaces) { - return []string{}, ErrBadKey{fmt.Sprintf("variable '%s' has white spaces", variable)} + return []string{}, ErrBadKey{fmt.Sprintf("variable '%s' contains whitespaces", variable)} } if len(variable) == 0 { return []string{}, ErrBadKey{fmt.Sprintf("no variable name on line '%s'", line)} diff --git a/opts/opts.go b/opts/opts.go index 7e10a0c4f568..765548f6b906 100644 --- a/opts/opts.go +++ b/opts/opts.go @@ -281,10 +281,10 @@ func ValidateLabel(val string) (string, error) { arr := strings.SplitN(val, "=", 2) key := strings.TrimLeft(arr[0], whiteSpaces) if key == "" { - return "", fmt.Errorf("empty label name: '%s'", val) + return "", fmt.Errorf("invalid label '%s': empty name", val) } if strings.ContainsAny(key, whiteSpaces) { - return "", fmt.Errorf("label '%s' has white spaces", key) + return "", fmt.Errorf("label '%s' contains whitespaces", key) } return val, nil } diff --git a/opts/opts_test.go b/opts/opts_test.go index 664c1010d253..4d5ef1749eb1 100644 --- a/opts/opts_test.go +++ b/opts/opts_test.go @@ -184,17 +184,17 @@ func TestValidateLabel(t *testing.T) { }{ { name: "empty", - expectedErr: `empty label name: ''`, + expectedErr: `invalid label '': empty name`, }, { name: "whitespace only ", value: " ", - expectedErr: `empty label name: ' '`, + expectedErr: `invalid label ' ': empty name`, }, { name: "whitespace around equal-sign", value: " = ", - expectedErr: `empty label name: ' = '`, + expectedErr: `invalid label ' = ': empty name`, }, { name: "leading whitespace", @@ -203,12 +203,12 @@ func TestValidateLabel(t *testing.T) { { name: "whitespaces in key without value", value: "this is a label without value", - expectedErr: `label 'this is a label without value' has white spaces`, + expectedErr: `label 'this is a label without value' contains whitespaces`, }, { name: "whitespaces in key", value: "this is a label=value", - expectedErr: `label 'this is a label' has white spaces`, + expectedErr: `label 'this is a label' contains whitespaces`, }, { name: "whitespaces in value", @@ -229,7 +229,7 @@ func TestValidateLabel(t *testing.T) { { name: "no key", value: "=label", - expectedErr: `empty label name: '=label'`, + expectedErr: `invalid label '=label': empty name`, }, { name: "empty value",