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 <jakegut0108@gmail.com>
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

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)
 	}

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")
 	}

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
 	}

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,
 	}
 }
 

shared/filestore/filesstore.go

@@ -52,6 +52,7 @@ type FileBackendSettings struct {
 	AmazonS3SignV2          bool
 	AmazonS3SSE             bool
 	AmazonS3Trace           bool
+	SkipVerify              bool
 }
 
 func (settings *FileBackendSettings) CheckMandatoryS3Fields() error {

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

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},
 		}
 	}
 

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))
+}