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 fc19370ccb..17cb4184f3 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 8f925cc0b0..3c67946b29 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 a8dda0a258..bcb3729426 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 1739fefa9c..1f570bcac3 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 ec4ea403d6..e2620ada35 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 b25ed155cb..125da44332 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 1721a46ef9..96c0c7b84e 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 6485e309e5..7e10a0c4f5 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 723a212ea7..664c1010d2 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) + 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''`, + }, } - 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) - } - // 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 c98a26adce..8fe4ded9c8 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 bda9dcb6e7..f875707a5e 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 96c0c7b84e..2346cc1670 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 7e10a0c4f5..765548f6b9 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 664c1010d2..4d5ef1749e 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",