From 3a35b166697953cf6260a8eed048a10b2fe84719 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 9 Mar 2025 14:07:50 +0100 Subject: [PATCH] secret create: refactor, use limit reader, and touch up errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Swarm has size constraints on the size of secrets, but the client-side would read content into memory, regardless its size. This could lead to either the client reading too much into memory, or it sending data that's larger than the size limit of gRPC, which resulted in the error not being handled by SwarmKit and a generic gRPC error returned. Reading a secret from a file was added in [moby@c6f0b7f], which used a system.OpenSequential for reading ([FILE_FLAG_SEQUENTIAL_SCAN]). While there could be a very marginal benefit to prevent polluting the system's cache (Windows won’t aggressively keep it in the cache, freeing up system memory for other tasks). These details were not documented in code, and possibly may be too marginal, but adding a comment to outline won't hurt so this patch also adds a comment. This patch: - Rewrites readSecretData to not return a nil-error if no file was set, in stead only calling it when not using a driver. - Implements reading the data with a limit-reader to prevent reading large files into memory. - The limit is based on SwarmKits limits ([MaxSecretSize]), but made twice that size, just in case larger sizes are supported in future; the main goal is to have some constraints, and to prevent hitting the gRPC limit. - Updates some error messages to include STDIN (when used), or the filename (when used). Before this patch: ls -lh largefile -rw------- 1 thajeztah staff 8.1M Mar 9 00:19 largefile docker secret create nosuchfile ./nosuchfile Error reading content from "./nosuchfile": open ./nosuchfile: no such file or directory docker secret create toolarge ./largefile Error response from daemon: rpc error: code = ResourceExhausted desc = grpc: received message larger than max (8462870 vs. 4194304) docker secret create empty ./emptyfile Error response from daemon: rpc error: code = InvalidArgument desc = secret data must be larger than 0 and less than 512000 bytes cat ./largefile | docker secret create toolarge - Error response from daemon: rpc error: code = ResourceExhausted desc = grpc: received message larger than max (8462870 vs. 4194304) cat ./emptyfile | docker secret create empty - Error response from daemon: rpc error: code = InvalidArgument desc = secret data must be larger than 0 and less than 512000 bytes With this patch: docker secret create nosuchfile ./nosuchfile error reading from ./nosuchfile: open ./nosuchfile: no such file or directory docker secret create empty ./emptyfile error reading from ./emptyfile: data is empty docker secret create toolarge ./largefile Error response from daemon: rpc error: code = InvalidArgument desc = secret data must be larger than 0 and less than 512000 bytes cat ./largefile | docker secret create toolarge - Error response from daemon: rpc error: code = InvalidArgument desc = secret data must be larger than 0 and less than 512000 bytes cat ./emptyfile | docker secret create empty - error reading from STDIN: data is empty [moby@c6f0b7f]: https://github.com/moby/moby/commit/c6f0b7f448fac4d037d00f944a7908c60c04dff2 [FILE_FLAG_SEQUENTIAL_SCAN]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN [MaxSecretSize]: https://pkg.go.dev/github.com/moby/swarmkit/v2@v2.0.0-20250103191802-8c1959736554/api/validation#MaxSecretSize Signed-off-by: Sebastiaan van Stijn --- cli/command/secret/create.go | 77 ++++++++++++++++++++++--------- cli/command/secret/create_test.go | 10 ++-- 2 files changed, 60 insertions(+), 27 deletions(-) diff --git a/cli/command/secret/create.go b/cli/command/secret/create.go index 706e92c3d1..dd8a834135 100644 --- a/cli/command/secret/create.go +++ b/cli/command/secret/create.go @@ -52,14 +52,19 @@ func newSecretCreateCommand(dockerCli command.Cli) *cobra.Command { func runSecretCreate(ctx context.Context, dockerCli command.Cli, options createOptions) error { client := dockerCli.Client() - if options.driver != "" && options.file != "" { - return errors.Errorf("When using secret driver secret data must be empty") + var secretData []byte + if options.driver != "" { + if options.file != "" { + return errors.Errorf("When using secret driver secret data must be empty") + } + } else { + var err error + secretData, err = readSecretData(dockerCli.In(), options.file) + if err != nil { + return err + } } - secretData, err := readSecretData(dockerCli.In(), options.file) - if err != nil { - return errors.Errorf("Error reading content from %q: %v", options.file, err) - } spec := swarm.SecretSpec{ Annotations: swarm.Annotations{ Name: options.name, @@ -82,26 +87,54 @@ func runSecretCreate(ctx context.Context, dockerCli command.Cli, options createO return err } - fmt.Fprintln(dockerCli.Out(), r.ID) + _, _ = fmt.Fprintln(dockerCli.Out(), r.ID) return nil } -func readSecretData(in io.ReadCloser, file string) ([]byte, error) { - // Read secret value from external driver - if file == "" { - return nil, nil - } - if file != "-" { - var err error - in, err = sequential.Open(file) +// maxSecretSize is the maximum byte length of the [swarm.SecretSpec.Data] field, +// as defined by [MaxSecretSize] in SwarmKit. +// +// [MaxSecretSize]: https://pkg.go.dev/github.com/moby/swarmkit/v2@v2.0.0-20250103191802-8c1959736554/api/validation#MaxSecretSize +const maxSecretSize = 500 * 1024 // 500KB + +// readSecretData reads the secret from either stdin or the given fileName. +// +// It reads up to twice the maximum size of the secret ([maxSecretSize]), +// just in case swarm's limit changes; this is only a safeguard to prevent +// reading arbitrary files into memory. +func readSecretData(in io.Reader, fileName string) ([]byte, error) { + switch fileName { + case "-": + data, err := io.ReadAll(io.LimitReader(in, 2*maxSecretSize)) if err != nil { - return nil, err + return nil, fmt.Errorf("error reading from STDIN: %w", err) } - defer in.Close() + if len(data) == 0 { + return nil, errors.New("error reading from STDIN: data is empty") + } + return data, nil + case "": + return nil, errors.New("secret file is required") + default: + // Open file with [FILE_FLAG_SEQUENTIAL_SCAN] on Windows, which + // prevents Windows from aggressively caching it. We expect this + // file to be only read once. Given that this is expected to be + // a small file, this may not be a significant optimization, so + // we could choose to omit this, and use a regular [os.Open]. + // + // [FILE_FLAG_SEQUENTIAL_SCAN]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN + f, err := sequential.Open(fileName) + if err != nil { + return nil, fmt.Errorf("error reading from %s: %w", fileName, err) + } + defer f.Close() + data, err := io.ReadAll(io.LimitReader(f, 2*maxSecretSize)) + if err != nil { + return nil, fmt.Errorf("error reading from %s: %w", fileName, err) + } + if len(data) == 0 { + return nil, fmt.Errorf("error reading from %s: data is empty", fileName) + } + return data, nil } - data, err := io.ReadAll(in) - if err != nil { - return nil, err - } - return data, nil } diff --git a/cli/command/secret/create_test.go b/cli/command/secret/create_test.go index 884b7d771d..17520728c4 100644 --- a/cli/command/secret/create_test.go +++ b/cli/command/secret/create_test.go @@ -56,7 +56,7 @@ func TestSecretCreateErrors(t *testing.T) { } func TestSecretCreateWithName(t *testing.T) { - name := "foo" + const name = "secret-with-name" data, err := os.ReadFile(filepath.Join("testdata", secretDataFile)) assert.NilError(t, err) @@ -89,7 +89,7 @@ func TestSecretCreateWithDriver(t *testing.T) { expectedDriver := &swarm.Driver{ Name: "secret-driver", } - name := "foo" + const name = "secret-with-driver" cli := test.NewFakeCli(&fakeClient{ secretCreateFunc: func(_ context.Context, spec swarm.SecretSpec) (types.SecretCreateResponse, error) { @@ -118,7 +118,7 @@ func TestSecretCreateWithTemplatingDriver(t *testing.T) { expectedDriver := &swarm.Driver{ Name: "template-driver", } - const name = "foo" + const name = "secret-with-template-driver" cli := test.NewFakeCli(&fakeClient{ secretCreateFunc: func(_ context.Context, spec swarm.SecretSpec) (types.SecretCreateResponse, error) { @@ -137,7 +137,7 @@ func TestSecretCreateWithTemplatingDriver(t *testing.T) { }) cmd := newSecretCreateCommand(cli) - cmd.SetArgs([]string{name}) + cmd.SetArgs([]string{name, filepath.Join("testdata", secretDataFile)}) assert.Check(t, cmd.Flags().Set("template-driver", expectedDriver.Name)) assert.NilError(t, cmd.Execute()) assert.Check(t, is.Equal("ID-"+name, strings.TrimSpace(cli.OutBuffer().String()))) @@ -148,7 +148,7 @@ func TestSecretCreateWithLabels(t *testing.T) { "lbl1": "Label-foo", "lbl2": "Label-bar", } - const name = "foo" + const name = "secret-with-labels" cli := test.NewFakeCli(&fakeClient{ secretCreateFunc: func(_ context.Context, spec swarm.SecretSpec) (types.SecretCreateResponse, error) {