diff --git a/cmd/kosli/snapshotS3.go b/cmd/kosli/snapshotS3.go index 7ed2670c6..eea892832 100644 --- a/cmd/kosli/snapshotS3.go +++ b/cmd/kosli/snapshotS3.go @@ -15,6 +15,7 @@ const snapshotS3ShortDesc = `Report a snapshot of the content of an AWS S3 bucke const snapshotS3LongDesc = snapshotS3ShortDesc + awsAuthDesc + ` You can report the entire bucket content, or filter some of the content using ^--include^ / ^--exclude^ (literal prefix match) or ^--include-regex^ / ^--exclude-regex^ (Go regular expressions matched against the full object key). In all cases, the content is reported as one artifact. If you wish to report separate files/dirs within the same bucket as separate artifacts, you need to run the command twice. +Object keys that cannot be stored as a local file, such as keys containing a ^..^ path segment, are rejected and fail the snapshot, naming the key. Two keys that resolve to the same local file are also an error. A legitimate key of that shape can be left out with ^--exclude-regex^ (anchor and escape it, since the pattern is a regular expression matched against the whole key); when ^--include^ or ^--include-regex^ is set, exclude filters are ignored, so narrow the include filter instead. ` + kosliIgnoreDesc diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 7a410e8fe..9b750e4f6 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -4,12 +4,15 @@ import ( "context" "encoding/base64" "encoding/hex" + "errors" "fmt" + "io/fs" "os" "path/filepath" "regexp" "strings" "sync" + "syscall" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -25,7 +28,6 @@ import ( "github.com/kosli-dev/cli/internal/digest" "github.com/kosli-dev/cli/internal/filters" "github.com/kosli-dev/cli/internal/logger" - "github.com/kosli-dev/cli/internal/utils" ) // EcsEnvRequest represents the PUT request body to be sent to kosli from ECS @@ -553,11 +555,63 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex return s3Data, nil } +// localPathForS3Key turns an S3 object key into a path under the download +// directory, or rejects it. A key holding a ".." segment resolves onto a path +// it does not name, taking another key's place or leaving the directory. +func localPathForS3Key(key string) (string, error) { + // Windows separates on '\\' and drops trailing dots and spaces from a + // name, so ".. " and "..." resolve as ".." there. + segments := strings.FieldsFunc(key, func(r rune) bool { return r == '/' || r == '\\' }) + for _, segment := range segments { + if strings.HasPrefix(segment, "..") && strings.TrimRight(segment, ". ") == "" { + return "", unusableS3KeyError(key, `contains a segment that resolves to ".."`) + } + } + + // A leading '\\' is left for filepath.IsLocal: rooted on Windows, an + // ordinary filename elsewhere. + rel := strings.TrimLeft(key, "/") + if filepath.Clean(rel) == "." { + return "", unusableS3KeyError(key, "names no file") + } + if !filepath.IsLocal(rel) { + return "", unusableS3KeyError(key, "is not a local path") + } + + return rel, nil +} + +// unusableS3KeyError is only for failures the key itself causes. Advising +// exclusion on a machine fault such as a full disk would drop a legitimate +// object from the snapshot. +func unusableS3KeyError(key, reason string) error { + return fmt.Errorf("object key [%s] cannot be stored as a local file: %s; exclude it with --exclude-regex, or narrow the include filter if one is set", key, reason) +} + func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket string, logger *logger.Logger) error { - file, err := utils.CreateFile(filepath.Join(dirName, key)) + rel, err := localPathForS3Key(key) if err != nil { return err } + dest := filepath.Join(dirName, rel) + err = os.MkdirAll(filepath.Dir(dest), 0770) + if errors.Is(err, syscall.ENOTDIR) { + // Legal in S3, impossible on disk: an object "a" and a key under "a/". + return unusableS3KeyError(key, "one of its parent prefixes has already been downloaded as an object") + } + if err != nil { + return fmt.Errorf("object key [%s]: %w", key, err) + } + // O_EXCL fails the snapshot when two keys map to one file rather than + // letting the second overwrite the first. Directories are not covered: + // "A/x" and "a/y" share one on a case-insensitive filesystem. + file, err := os.OpenFile(dest, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666) + if errors.Is(err, fs.ErrExist) { + return unusableS3KeyError(key, "another object already downloaded to the same local path") + } + if err != nil { + return fmt.Errorf("object key [%s]: %w", key, err) + } defer func() { if err := file.Close(); err != nil { logger.Warn("failed to close file %s: %v", file.Name(), err) @@ -570,7 +624,7 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin WriterAt: file, }) if err != nil { - return err + return fmt.Errorf("failed to download object key [%s]: %w", key, err) } if result.ContentLength != nil { logger.Debug("downloaded", file.Name(), *result.ContentLength, "bytes") diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go index 3b0716ea9..1b15e5932 100644 --- a/internal/aws/aws_test.go +++ b/internal/aws/aws_test.go @@ -3,7 +3,11 @@ package aws import ( "context" "fmt" + "io/fs" + "os" + "path/filepath" "regexp" + "runtime" "testing" "time" @@ -1227,6 +1231,221 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientFilterEquivalence() { } } +func (suite *AWSTestSuite) TestGetS3DataFromClientRejectsKeysWithDotDotSegments() { + trustedBody := []byte("trusted release content\n") + attackerBody := []byte("attacker controlled content\n") + + poisoned := &FakeS3Client{ + Bucket: fakeS3TestBucketName, + Objects: map[string][]byte{ + "protected/release.bin": trustedBody, + "uploads/user-a/../../protected/release.bin": attackerBody, + }, + } + + _, err := getS3DataFromClient(poisoned, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.Error(suite.T(), err, "a key containing a \"..\" segment must fail the snapshot instead of silently overwriting another object's download") + require.Contains(suite.T(), err.Error(), "uploads/user-a/../../protected/release.bin") +} + +// Accept rows compare joined paths because the helper returns the key +// uncleaned; filepath.Join is what collapses "a//b" and "./a.txt". +func (suite *AWSTestSuite) TestLocalPathForS3Key() { + for _, t := range []struct { + name string + key string + wantPath string // accept: the path filepath.Join(dir, key) produced before this change + wantErr bool + wantErrMsg string + }{ + {name: "an ordinary nested key", key: "protected/release.bin", wantPath: "protected/release.bin"}, + {name: "a plain filename", key: "a.txt", wantPath: "a.txt"}, + {name: "a short nested key", key: "a/z", wantPath: "a/z"}, + {name: "a dotfile", key: ".kosli_ignore", wantPath: ".kosli_ignore"}, + {name: "a key with spaces", key: "file with spaces.txt", wantPath: "file with spaces.txt"}, + {name: "a key with punctuation", key: "weird!*'().txt", wantPath: "weird!*'().txt"}, + {name: "a unicode key", key: "ünïcödé/файл.txt", wantPath: "ünïcödé/файл.txt"}, + {name: "a dot followed by a space is a literal name", key: ". ", wantPath: ". "}, + {name: "a name that merely starts with two dots", key: "..hidden", wantPath: "..hidden"}, + {name: "a backslash key is a literal filename on this OS", key: `dir\file.txt`, wantPath: `dir\file.txt`}, + {name: "a leading slash is trimmed", key: "/etc/passwd", wantPath: "etc/passwd"}, + {name: "doubled leading slashes are trimmed", key: "//x", wantPath: "x"}, + {name: "a leading dot segment is dropped by Join", key: "./a.txt", wantPath: "a.txt"}, + {name: "a doubled interior slash is collapsed by Join", key: "a//b", wantPath: "a/b"}, + {name: "a dot segment is dropped by Join", key: "a/./b", wantPath: "a/b"}, + // filepath.IsLocal rejects reserved device names and colons on Windows only. + {name: "a reserved Windows name", key: "CON", wantPath: "CON", wantErr: runtime.GOOS == "windows", wantErrMsg: "is not a local path"}, + {name: "a drive-looking segment", key: "C:evil", wantPath: "C:evil", wantErr: runtime.GOOS == "windows", wantErrMsg: "is not a local path"}, + {name: "a colon segment", key: "a:b", wantPath: "a:b", wantErr: runtime.GOOS == "windows", wantErrMsg: "is not a local path"}, + { + name: "a traversing key is rejected", + key: "uploads/user-a/../../protected/release.bin", + wantErr: true, + wantErrMsg: `resolves to ".."`, + }, + { + name: "a backslash-separated traversal is rejected", + key: `uploads/user-a/..\..\..\..\Users\Public\kosli-poc.txt`, + wantErr: true, + wantErrMsg: `resolves to ".."`, + }, + { + name: "a short backslash-separated traversal is rejected", + key: `uploads/user-a/..\x`, + wantErr: true, + wantErrMsg: `resolves to ".."`, + }, + {name: "a bare \"..\" is rejected", key: "..", wantErr: true, wantErrMsg: `resolves to ".."`}, + // Windows drops trailing spaces and dots from a name, so these resolve as "..". + {name: "a \"..\" with a trailing space is rejected", key: "a/.. /x", wantErr: true, wantErrMsg: `resolves to ".."`}, + {name: "three dots are rejected", key: "a/.../b", wantErr: true, wantErrMsg: `resolves to ".."`}, + {name: "a bare \"./.\" is rejected", key: "./.", wantErr: true, wantErrMsg: "names no file"}, + {name: "a trailing \"..\" segment is rejected", key: "a/..", wantErr: true, wantErrMsg: `resolves to ".."`}, + {name: "an empty key is rejected", key: "", wantErr: true, wantErrMsg: "names no file"}, + {name: "a bare slash is rejected", key: "/", wantErr: true, wantErrMsg: "names no file"}, + {name: "doubled slashes with nothing else are rejected", key: "//", wantErr: true, wantErrMsg: "names no file"}, + {name: "a bare dot is rejected", key: ".", wantErr: true, wantErrMsg: "names no file"}, + } { + suite.Run(t.name, func() { + got, err := localPathForS3Key(t.key) + if t.wantErr { + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), t.key) + require.Contains(suite.T(), err.Error(), t.wantErrMsg) + return + } + require.NoError(suite.T(), err) + require.Equal(suite.T(), filepath.Join("base", t.wantPath), filepath.Join("base", got)) + }) + } +} + +func (suite *AWSTestSuite) TestDownloadFileFromBucketRefusesToOverwrite() { + tempDir := suite.T().TempDir() + preexisting := filepath.Join(tempDir, "README.md") + require.NoError(suite.T(), os.WriteFile(preexisting, []byte("pre-existing content\n"), 0666)) + + client := &FakeS3Client{ + Bucket: fakeS3TestBucketName, + Objects: map[string][]byte{ + "README.md": []byte(fakeReadmeBody), + }, + } + + err := downloadFileFromBucket(client, tempDir, "README.md", fakeS3TestBucketName, logger.NewStandardLogger()) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "object key [README.md]") + require.Contains(suite.T(), err.Error(), "--exclude-regex") + + content, readErr := os.ReadFile(preexisting) + require.NoError(suite.T(), readErr) + require.Equal(suite.T(), "pre-existing content\n", string(content), + "the pre-existing file must be left untouched, not truncated") +} + +// Neither key holds a ".." segment, so O_EXCL is what catches this pair. +func (suite *AWSTestSuite) TestGetS3DataFromClientCollidingKeysAreAnError() { + client := &FakeS3Client{ + Bucket: fakeS3TestBucketName, + Objects: map[string][]byte{ + "a//b": []byte(fakeReadmeBody), + "a/b": []byte(fakeTemplateBody), + }, + } + + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.Error(suite.T(), err) + // '/' sorts before 'b', so "a//b" downloads first and "a/b" collides. + require.Contains(suite.T(), err.Error(), "object key [a/b]", "the error must name the key that collided") + require.Contains(suite.T(), err.Error(), "--exclude-regex") +} + +// An object "a" alongside the prefix "a/" is legal in S3 and impossible on a +// filesystem, so MkdirAll fails with ENOTDIR once "a" lands first. +func (suite *AWSTestSuite) TestGetS3DataFromClientObjectAndPrefixCollideAreAnError() { + client := &FakeS3Client{ + Bucket: fakeS3TestBucketName, + Objects: map[string][]byte{ + "a": []byte(fakeReadmeBody), + "a/b": []byte(fakeTemplateBody), + }, + } + + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "object key [a/b]") + require.Contains(suite.T(), err.Error(), "--exclude-regex") +} + +func (suite *AWSTestSuite) TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnFilesystemErrors() { + if runtime.GOOS == "windows" { + suite.T().Skip("chmod on a directory does not block file creation on Windows") + } + if os.Getuid() == 0 { + suite.T().Skip("root ignores directory permissions") + } + tempDir := suite.T().TempDir() + require.NoError(suite.T(), os.Chmod(tempDir, 0500)) + suite.T().Cleanup(func() { _ = os.Chmod(tempDir, 0700) }) + + client := &FakeS3Client{ + Bucket: fakeS3TestBucketName, + Objects: map[string][]byte{ + "README.md": []byte(fakeReadmeBody), + "sub/README.md": []byte(fakeReadmeBody), + }, + } + + // A bare key fails in OpenFile; a nested one has a directory left to + // create, so it fails in MkdirAll. + for _, key := range []string{"README.md", "sub/README.md"} { + suite.Run(key, func() { + err := downloadFileFromBucket(client, tempDir, key, fakeS3TestBucketName, logger.NewStandardLogger()) + require.Error(suite.T(), err) + require.ErrorIs(suite.T(), err, fs.ErrPermission) + require.Contains(suite.T(), err.Error(), fmt.Sprintf("object key [%s]", key)) + require.NotContains(suite.T(), err.Error(), "--exclude-regex") + }) + } +} + +// Equal fingerprints mean the odd-shaped keys landed on the same paths as the +// plain ones. +func (suite *AWSTestSuite) TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys() { + unusualBody := []byte("unusual key content\n") + otherBody := []byte("other content\n") + thirdBody := []byte("third content\n") + + unusual := &FakeS3Client{ + Bucket: fakeS3TestBucketName, + Objects: map[string][]byte{ + "/lead.txt": unusualBody, + "a//b": otherBody, + "./c.txt": thirdBody, + `d\e.txt`: []byte(fakeNotesBody), + }, + } + today := &FakeS3Client{ + Bucket: fakeS3TestBucketName, + Objects: map[string][]byte{ + "lead.txt": unusualBody, + "a/b": otherBody, + "c.txt": thirdBody, + `d\e.txt`: []byte(fakeNotesBody), + }, + } + + unusualData, err := getS3DataFromClient(unusual, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + todayData, err := getS3DataFromClient(today, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Len(suite.T(), unusualData, 1) + require.Len(suite.T(), todayData, 1) + + require.Equal(suite.T(), todayData[0].Digests, unusualData[0].Digests, + "odd-shaped keys accepted by the containment rule must still fingerprint identically to the plain keys they land on") +} + func skipIfCredsUnset(T *testing.T, requireEnvVars bool, creds *AWSStaticCreds) { if requireEnvVars { // skips the test case if it requires env vars and they are not set diff --git a/internal/aws/s3_contract_test.go b/internal/aws/s3_contract_test.go index ec9ea96d3..d01e03e2d 100644 --- a/internal/aws/s3_contract_test.go +++ b/internal/aws/s3_contract_test.go @@ -5,6 +5,7 @@ import ( "errors" "os" "path/filepath" + "slices" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -43,6 +44,24 @@ func runS3ContractTests(t *testing.T, client S3API, bucket, existingKey string) } }) + // Which of two colliding keys fails a snapshot follows from this order. + // Paginated because a single page can hold one key, sorted either way. + t.Run("ListObjectsV2 returns keys in lexicographic order", func(t *testing.T) { + var keys []string + paginator := s3.NewListObjectsV2Paginator(client, &s3.ListObjectsV2Input{ + Bucket: aws.String(bucket), + }) + for paginator.HasMorePages() { + page, err := paginator.NextPage(context.TODO()) + require.NoError(t, err) + for _, object := range page.Contents { + keys = append(keys, *object.Key) + } + } + require.GreaterOrEqual(t, len(keys), 2, "the bucket must hold at least two objects for order to be checked") + require.True(t, slices.IsSorted(keys), "S3 returns keys in UTF-8 binary order: %v", keys) + }) + t.Run("ListObjectsV2 with MaxKeys paginates via ContinuationToken", func(t *testing.T) { // Request one object per page to force pagination. The paginator only // follows a page when IsTruncated is true AND NextContinuationToken is