From de50943d61adec1aa65ff30598c6e0893fa61bf9 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Mon, 20 Jun 2022 15:42:01 +0530 Subject: [PATCH] jake/gh 20356 (#20485) * [MM-44667] Set InsecureSkipVerify for S3 when EnableInsecureOutgoingConnections is set * Cleanup s3New, customTransport * Lint fixes * Cleanup s3store_test.go * Using default transport ```release-note NONE ``` Co-authored-by: Jake Gutierrez Co-authored-by: Mattermod --- app/file.go | 5 ++- app/server.go | 3 +- app/users/profile_picture.go | 3 +- model/config.go | 3 +- shared/filestore/filesstore.go | 1 + shared/filestore/s3_overrides.go | 1 - shared/filestore/s3store.go | 21 +++++++--- shared/filestore/s3store_test.go | 69 ++++++++++++++++++++++++++++++++ 8 files changed, 95 insertions(+), 11 deletions(-) diff --git a/app/file.go b/app/file.go index 817fd663ae..31734f64f5 100644 --- a/app/file.go +++ b/app/file.go @@ -52,7 +52,7 @@ func (a *App) FileBackend() filestore.FileBackend { } func (a *App) CheckMandatoryS3Fields(settings *model.FileSettings) *model.AppError { - fileBackendSettings := settings.ToFileBackendSettings(false) + fileBackendSettings := settings.ToFileBackendSettings(false, false) err := fileBackendSettings.CheckMandatoryS3Fields() if err != nil { return model.NewAppError("CheckMandatoryS3Fields", "api.admin.test_s3.missing_s3_bucket", nil, err.Error(), http.StatusBadRequest) @@ -81,7 +81,8 @@ func (a *App) TestFileStoreConnection() *model.AppError { func (a *App) TestFileStoreConnectionWithConfig(cfg *model.FileSettings) *model.AppError { license := a.Srv().License() - backend, err := filestore.NewFileBackend(cfg.ToFileBackendSettings(license != nil && *license.Features.Compliance)) + insecure := a.Config().ServiceSettings.EnableInsecureOutgoingConnections + backend, err := filestore.NewFileBackend(cfg.ToFileBackendSettings(license != nil && *license.Features.Compliance, insecure != nil && *insecure)) if err != nil { return model.NewAppError("FileBackend", "api.file.no_driver.app_error", nil, err.Error(), http.StatusInternalServerError) } diff --git a/app/server.go b/app/server.go index 546af6477c..087bba50a3 100644 --- a/app/server.go +++ b/app/server.go @@ -359,8 +359,9 @@ func NewServer(options ...Option) (*Server, error) { } license := s.License() + insecure := s.Config().ServiceSettings.EnableInsecureOutgoingConnections // Step 7: Initialize filestore - backend, err := filestore.NewFileBackend(s.Config().FileSettings.ToFileBackendSettings(license != nil && *license.Features.Compliance)) + backend, err := filestore.NewFileBackend(s.Config().FileSettings.ToFileBackendSettings(license != nil && *license.Features.Compliance, insecure != nil && *insecure)) if err != nil { return nil, errors.Wrap(err, "failed to initialize filebackend") } diff --git a/app/users/profile_picture.go b/app/users/profile_picture.go index fbd413f52b..c0260c5a5b 100644 --- a/app/users/profile_picture.go +++ b/app/users/profile_picture.go @@ -57,7 +57,8 @@ func (us *UserService) GetProfileImage(user *model.User) ([]byte, bool, error) { func (us *UserService) FileBackend() (filestore.FileBackend, error) { license := us.license() - backend, err := filestore.NewFileBackend(us.config().FileSettings.ToFileBackendSettings(license != nil && *license.Features.Compliance)) + insecure := us.config().ServiceSettings.EnableInsecureOutgoingConnections + backend, err := filestore.NewFileBackend(us.config().FileSettings.ToFileBackendSettings(license != nil && *license.Features.Compliance, insecure != nil && *insecure)) if err != nil { return nil, err } diff --git a/model/config.go b/model/config.go index 4c942c0cdd..1dc3f20693 100644 --- a/model/config.go +++ b/model/config.go @@ -1529,7 +1529,7 @@ func (s *FileSettings) SetDefaults(isUpdate bool) { } } -func (s *FileSettings) ToFileBackendSettings(enableComplianceFeature bool) filestore.FileBackendSettings { +func (s *FileSettings) ToFileBackendSettings(enableComplianceFeature bool, skipVerify bool) filestore.FileBackendSettings { if *s.DriverName == ImageDriverLocal { return filestore.FileBackendSettings{ DriverName: *s.DriverName, @@ -1548,6 +1548,7 @@ func (s *FileSettings) ToFileBackendSettings(enableComplianceFeature bool) files AmazonS3SignV2: s.AmazonS3SignV2 != nil && *s.AmazonS3SignV2, AmazonS3SSE: s.AmazonS3SSE != nil && *s.AmazonS3SSE && enableComplianceFeature, AmazonS3Trace: s.AmazonS3Trace != nil && *s.AmazonS3Trace, + SkipVerify: skipVerify, } } diff --git a/shared/filestore/filesstore.go b/shared/filestore/filesstore.go index c17ea1f62d..9de111f876 100644 --- a/shared/filestore/filesstore.go +++ b/shared/filestore/filesstore.go @@ -52,6 +52,7 @@ type FileBackendSettings struct { AmazonS3SignV2 bool AmazonS3SSE bool AmazonS3Trace bool + SkipVerify bool } func (settings *FileBackendSettings) CheckMandatoryS3Fields() error { diff --git a/shared/filestore/s3_overrides.go b/shared/filestore/s3_overrides.go index 697809ee50..06cabb7fa5 100644 --- a/shared/filestore/s3_overrides.go +++ b/shared/filestore/s3_overrides.go @@ -15,7 +15,6 @@ import ( // from multiple Mattermost applications, and the Mattermost service itself does not // have any S3 credentials. type customTransport struct { - base http.RoundTripper host string scheme string client http.Client diff --git a/shared/filestore/s3store.go b/shared/filestore/s3store.go index d162daa213..91a211c062 100644 --- a/shared/filestore/s3store.go +++ b/shared/filestore/s3store.go @@ -5,8 +5,10 @@ package filestore import ( "context" + "crypto/tls" "io" "io/ioutil" + "net/http" "os" "path/filepath" "strings" @@ -34,6 +36,7 @@ type S3FileBackend struct { encrypt bool trace bool client *s3.Client + skipVerify bool } type S3FileBackendAuthError struct { @@ -87,6 +90,7 @@ func NewS3FileBackend(settings FileBackendSettings) (*S3FileBackend, error) { pathPrefix: settings.AmazonS3PathPrefix, encrypt: settings.AmazonS3SSE, trace: settings.AmazonS3Trace, + skipVerify: settings.SkipVerify, } cli, err := backend.s3New() if err != nil { @@ -121,20 +125,27 @@ func (b *S3FileBackend) s3New() (*s3.Client, error) { Region: b.region, } + tr, err := s3.DefaultTransport(b.secure) + if err != nil { + return nil, err + } + if b.skipVerify { + tr.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + } + opts.Transport = tr + // If this is a cloud installation, we override the default transport. if isCloud { - tr, err := s3.DefaultTransport(b.secure) - if err != nil { - return nil, err - } scheme := "http" if b.secure { scheme = "https" } + newTransport := http.DefaultTransport.(*http.Transport).Clone() + newTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: b.skipVerify} opts.Transport = &customTransport{ - base: tr, host: b.endpoint, scheme: scheme, + client: http.Client{Transport: newTransport}, } } diff --git a/shared/filestore/s3store_test.go b/shared/filestore/s3store_test.go index 0bbe6be4e8..85245afe27 100644 --- a/shared/filestore/s3store_test.go +++ b/shared/filestore/s3store_test.go @@ -7,6 +7,9 @@ import ( "crypto/rand" "encoding/base64" "fmt" + "net/http/httptest" + "net/http/httputil" + "net/url" "os" "strings" "testing" @@ -69,6 +72,7 @@ func TestMakeBucket(t *testing.T) { AmazonS3Region: "", AmazonS3PathPrefix: "", AmazonS3SSL: false, + SkipVerify: false, } fileBackend, err := NewS3FileBackend(cfg) @@ -77,3 +81,68 @@ func TestMakeBucket(t *testing.T) { err = fileBackend.MakeBucket() require.NoError(t, err) } + +func TestInsecureMakeBucket(t *testing.T) { + s3Host := os.Getenv("CI_MINIO_HOST") + if s3Host == "" { + s3Host = "localhost" + } + + s3Port := os.Getenv("CI_MINIO_PORT") + if s3Port == "" { + s3Port = "9000" + } + + s3Endpoint := fmt.Sprintf("%s:%s", s3Host, s3Port) + + proxySelfSignedHTTPS := newTLSProxyServer(&url.URL{Scheme: "http", Host: s3Endpoint}) + defer proxySelfSignedHTTPS.Close() + + enableInsecure, secure := true, false + + testCases := []struct { + description string + skipVerify bool + expectedAllowed bool + }{ + {"allow self-signed HTTPS when insecure enabled", enableInsecure, true}, + {"reject self-signed HTTPS when secured", secure, false}, + } + + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + // Generate a random bucket name + b := make([]byte, 30) + rand.Read(b) + bucketName := base64.StdEncoding.EncodeToString(b) + bucketName = strings.ToLower(bucketName) + bucketName = strings.Replace(bucketName, "+", "", -1) + bucketName = strings.Replace(bucketName, "/", "", -1) + + cfg := FileBackendSettings{ + DriverName: ImageDriverS3, + AmazonS3AccessKeyId: MinioAccessKey, + AmazonS3SecretAccessKey: MinioSecretKey, + AmazonS3Bucket: bucketName, + AmazonS3Endpoint: proxySelfSignedHTTPS.URL[8:], + AmazonS3Region: "", + AmazonS3PathPrefix: "", + AmazonS3SSL: true, + SkipVerify: testCase.skipVerify, + } + + fileBackend, err := NewS3FileBackend(cfg) + require.NoError(t, err) + + err = fileBackend.MakeBucket() + if testCase.expectedAllowed { + require.NoError(t, err) + } else { + require.Error(t, err) + } + }) + } +} +func newTLSProxyServer(backend *url.URL) *httptest.Server { + return httptest.NewTLSServer(httputil.NewSingleHostReverseProxy(backend)) +}