From 71c5a8512c5a8bafe63508089ee5770788dcb89c Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Tue, 27 Jun 2023 09:54:53 +0530 Subject: [PATCH] MM-51753: URL encode the file path in S3 uploads (#23849) If the request is routed via bifrost, then we need to encode the path to avoid signature validation errors. This happens because in bifrost, we are signing the URL outside the SDK and therefore the signature sent from the bifrost client will contain the encoded path, whereas the original path is sent un-encoded. More info at: https://github.com/aws/aws-sdk-go/blob/a57c4d92784a43b716645a57b6fa5fb94fb6e419/aws/signer/v4/v4.go#L8 https://mattermost.atlassian.net/browse/MM-51753 ```release-note NONE ``` --- .../shared/filestore/filesstore_test.go | 27 +++++++++ server/platform/shared/filestore/s3store.go | 59 ++++++++++++------- 2 files changed, 64 insertions(+), 22 deletions(-) diff --git a/server/platform/shared/filestore/filesstore_test.go b/server/platform/shared/filestore/filesstore_test.go index 8746420376..11c29ddbff 100644 --- a/server/platform/shared/filestore/filesstore_test.go +++ b/server/platform/shared/filestore/filesstore_test.go @@ -124,6 +124,33 @@ func (s *FileBackendTestSuite) TestReadWriteFile() { s.EqualValues(readString, "test") } +func (s *FileBackendTestSuite) TestEncode() { + s3Backend, ok := s.backend.(*S3FileBackend) + // This test is only for S3backend. + if !ok { + return + } + s3Backend.isCloud = true + defer func() { + s3Backend.isCloud = false + }() + + originalPath := "dir1/test+.png" + encodedPath := s3Backend.prefixedPath(originalPath) + b := []byte("test") + written, err := s3Backend.WriteFile(bytes.NewReader(b), encodedPath) + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") + defer s3Backend.RemoveFile(encodedPath) + + files, err := s3Backend.ListDirectory("dir1") + s.Nil(err) + s.Require().Len(files, 1) + // There's another layer of encoding since the backend is Minio + // and it doesn't store the path unescaped. + s.Equal("dir1/test%252B.png", files[0]) +} + func (s *FileBackendTestSuite) TestReadWriteFileContext() { type ContextWriter interface { WriteFileContext(context.Context, io.Reader, string) (int64, error) diff --git a/server/platform/shared/filestore/s3store.go b/server/platform/shared/filestore/s3store.go index 07acfb0c85..283f8fd53d 100644 --- a/server/platform/shared/filestore/s3store.go +++ b/server/platform/shared/filestore/s3store.go @@ -17,6 +17,7 @@ import ( s3 "github.com/minio/minio-go/v7" "github.com/minio/minio-go/v7/pkg/credentials" "github.com/minio/minio-go/v7/pkg/encrypt" + "github.com/minio/minio-go/v7/pkg/s3utils" "github.com/pkg/errors" "github.com/mattermost/mattermost/server/public/shared/mlog" @@ -38,6 +39,7 @@ type S3FileBackend struct { client *s3.Client skipVerify bool timeout time.Duration + isCloud bool // field to indicate whether this is running under Mattermost cloud or not. } type S3FileBackendAuthError struct { @@ -100,11 +102,13 @@ func NewS3FileBackend(settings FileBackendSettings) (*S3FileBackend, error) { skipVerify: settings.SkipVerify, timeout: timeout, } - cli, err := backend.s3New() + isCloud := os.Getenv("MM_CLOUD_FILESTORE_BIFROST") != "" + cli, err := backend.s3New(isCloud) if err != nil { return nil, err } backend.client = cli + backend.isCloud = isCloud return backend, nil } @@ -113,10 +117,9 @@ func NewS3FileBackend(settings FileBackendSettings) (*S3FileBackend, error) { // // Additionally this function also takes a user defined region, if set // disables automatic region lookup. -func (b *S3FileBackend) s3New() (*s3.Client, error) { +func (b *S3FileBackend) s3New(isCloud bool) (*s3.Client, error) { var creds *credentials.Credentials - isCloud := os.Getenv("MM_CLOUD_FILESTORE_BIFROST") != "" if isCloud { creds = credentials.New(customProvider{isSignV2: b.signV2}) } else if b.accessKey == "" && b.secretKey == "" { @@ -233,7 +236,7 @@ func (sc *s3WithCancel) CancelTimeout() bool { // Caller must close the first return value func (b *S3FileBackend) Reader(path string) (ReadCloseSeeker, error) { - path = filepath.Join(b.pathPrefix, path) + path = b.prefixedPath(path) ctx, cancel := context.WithCancel(context.Background()) minioObject, err := b.client.GetObject(ctx, b.bucket, path, s3.GetObjectOptions{}) if err != nil { @@ -251,7 +254,7 @@ func (b *S3FileBackend) Reader(path string) (ReadCloseSeeker, error) { } func (b *S3FileBackend) ReadFile(path string) ([]byte, error) { - path = filepath.Join(b.pathPrefix, path) + path = b.prefixedPath(path) ctx, cancel := context.WithTimeout(context.Background(), b.timeout) defer cancel() minioObject, err := b.client.GetObject(ctx, b.bucket, path, s3.GetObjectOptions{}) @@ -268,7 +271,7 @@ func (b *S3FileBackend) ReadFile(path string) ([]byte, error) { } func (b *S3FileBackend) FileExists(path string) (bool, error) { - path = filepath.Join(b.pathPrefix, path) + path = b.prefixedPath(path) ctx, cancel := context.WithTimeout(context.Background(), b.timeout) defer cancel() @@ -286,7 +289,7 @@ func (b *S3FileBackend) FileExists(path string) (bool, error) { } func (b *S3FileBackend) FileSize(path string) (int64, error) { - path = filepath.Join(b.pathPrefix, path) + path = b.prefixedPath(path) ctx, cancel := context.WithTimeout(context.Background(), b.timeout) defer cancel() @@ -299,7 +302,7 @@ func (b *S3FileBackend) FileSize(path string) (int64, error) { } func (b *S3FileBackend) FileModTime(path string) (time.Time, error) { - path = filepath.Join(b.pathPrefix, path) + path = b.prefixedPath(path) ctx, cancel := context.WithTimeout(context.Background(), b.timeout) defer cancel() @@ -312,8 +315,8 @@ func (b *S3FileBackend) FileModTime(path string) (time.Time, error) { } func (b *S3FileBackend) CopyFile(oldPath, newPath string) error { - oldPath = filepath.Join(b.pathPrefix, oldPath) - newPath = filepath.Join(b.pathPrefix, newPath) + oldPath = b.prefixedPath(oldPath) + newPath = b.prefixedPath(newPath) srcOpts := s3.CopySrcOptions{ Bucket: b.bucket, Object: oldPath, @@ -340,8 +343,8 @@ func (b *S3FileBackend) CopyFile(oldPath, newPath string) error { } func (b *S3FileBackend) MoveFile(oldPath, newPath string) error { - oldPath = filepath.Join(b.pathPrefix, oldPath) - newPath = filepath.Join(b.pathPrefix, newPath) + oldPath = b.prefixedPath(oldPath) + newPath = b.prefixedPath(newPath) srcOpts := s3.CopySrcOptions{ Bucket: b.bucket, Object: oldPath, @@ -382,7 +385,7 @@ func (b *S3FileBackend) WriteFile(fr io.Reader, path string) (int64, error) { func (b *S3FileBackend) WriteFileContext(ctx context.Context, fr io.Reader, path string) (int64, error) { var contentType string - path = filepath.Join(b.pathPrefix, path) + path = b.prefixedPath(path) if ext := filepath.Ext(path); isFileExtImage(ext) { contentType = getImageMimeType(ext) } else { @@ -392,8 +395,7 @@ func (b *S3FileBackend) WriteFileContext(ctx context.Context, fr io.Reader, path options := s3PutOptions(b.encrypt, contentType) objSize := int64(-1) - isCloud := os.Getenv("MM_CLOUD_FILESTORE_BIFROST") != "" - if isCloud { + if b.isCloud { options.DisableContentSha256 = true } else { // We pass an object size only in situations where bifrost is not @@ -417,7 +419,7 @@ func (b *S3FileBackend) WriteFileContext(ctx context.Context, fr io.Reader, path } func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, error) { - fp := filepath.Join(b.pathPrefix, path) + fp := b.prefixedPath(path) ctx, cancel := context.WithTimeout(context.Background(), b.timeout) defer cancel() if _, err := b.client.StatObject(ctx, b.bucket, fp, s3.StatObjectOptions{}); err != nil { @@ -437,13 +439,12 @@ func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, error) { ctx2, cancel2 := context.WithTimeout(context.Background(), b.timeout) defer cancel2() objSize := -1 - isCloud := os.Getenv("MM_CLOUD_FILESTORE_BIFROST") != "" - if isCloud { + if b.isCloud { options.DisableContentSha256 = true } // We pass an object size only in situations where bifrost is not // used. Bifrost needs to run in HTTPS, which is not yet deployed. - if buf, ok := fr.(*bytes.Buffer); ok && !isCloud { + if buf, ok := fr.(*bytes.Buffer); ok && !b.isCloud { objSize = buf.Len() } info, err := b.client.PutObject(ctx2, b.bucket, partName, fr, int64(objSize), options) @@ -480,7 +481,7 @@ func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, error) { } func (b *S3FileBackend) RemoveFile(path string) error { - path = filepath.Join(b.pathPrefix, path) + path = b.prefixedPath(path) ctx, cancel := context.WithTimeout(context.Background(), b.timeout) defer cancel() if err := b.client.RemoveObject(ctx, b.bucket, path, s3.RemoveObjectOptions{}); err != nil { @@ -511,7 +512,7 @@ func getPathsFromObjectInfos(in <-chan s3.ObjectInfo) <-chan s3.ObjectInfo { } func (b *S3FileBackend) listDirectory(path string, recursion bool) ([]string, error) { - path = filepath.Join(b.pathPrefix, path) + path = b.prefixedPath(path) if !strings.HasSuffix(path, "/") && path != "" { // s3Clnt returns only the path itself when "/" is not present // appending "/" to make it consistent across all filestores @@ -551,7 +552,7 @@ func (b *S3FileBackend) ListDirectoryRecursively(path string) ([]string, error) func (b *S3FileBackend) RemoveDirectory(path string) error { opts := s3.ListObjectsOptions{ - Prefix: filepath.Join(b.pathPrefix, path), + Prefix: b.prefixedPath(path), Recursive: true, } ctx, cancel := context.WithTimeout(context.Background(), b.timeout) @@ -570,6 +571,20 @@ func (b *S3FileBackend) RemoveDirectory(path string) error { return nil } +func (b *S3FileBackend) prefixedPath(s string) string { + if b.isCloud { + // If the request is routed via bifrost, then we need to encode the path + // to avoid signature validation errors. + // This happens because in bifrost, we are signing the URL outside the SDK + // and therefore the signature sent from the bifrost client + // will contain the encoded path, whereas the original path is sent + // un-encoded. + // More info at: https://github.com/aws/aws-sdk-go/blob/a57c4d92784a43b716645a57b6fa5fb94fb6e419/aws/signer/v4/v4.go#L8 + s = s3utils.EncodePath(s) + } + return filepath.Join(b.pathPrefix, s) +} + func s3PutOptions(encrypted bool, contentType string) s3.PutObjectOptions { options := s3.PutObjectOptions{} if encrypted {