Repository: snowflakedb/gosnowflake
Pull request #1288 (2025-01-12 14:00:13)

SNOW-1859664 use correct transport for calling cloud providers
### Description
See linked issue. The problem is, that since 1.7.2 even though the user specifies `insecureMode` (pre-1.12.1) and/or `disableOCSPchecks` (post-1.12.1); the client we create for executing cloud storage operations; always uses `SnowflakeTransport` regardless of the OCSP verification settings. 
This Transport always performs OCSP validation, regardless of the user's wishes, whether they want OCSP validation to be performed or not.

This is unexpected. What is even worse if there's any configuration issues on the user's infrastructure (e.g. the privatelink OCSP endpoint isn't configured in the DNS), we will still try the OCSP validation and even with the new OCSP strategy; with privatelink (since the 'OCSP Responder' is in this case, behind the Snowflake account), we'll retry a lot of times adding extra minutes of delay before failing with mysterious error messages like
```
operation error S3: PutObject, exceeded maximum number of attempts, 3, https response error StatusCode: 0, RequestID: , HostID: , request send failed, Put <remoteLocation>?x-id=PutObject\": EOF"
```

and since 1.7.2 due to improved error handling, we actually stopped ignoring these errors and are now surfacing to the user so they can fix it. But performing OCSP in an environment which is explicitly configured not to perform OCSP was not intended.

The change here aims to make the `Config` part of the cloud storage client operation; use a Transport depending on the `Config`, thus the OCSP validation not performed if the user explicitly configured the driver with `disableOCSPChecks` / `insecureMode`.

### Checklist
- [x] Created tests which fail without the change (if possible)
- [ ] Extended the README / documentation, if necessary


sfc-gh-dszmolka: in addition to the automated tests; manual tests were performed issuing
* `PUT` / `GET` (with 1mb CSV)
* `LIST`
* `RM` 

commands on stages, on accounts placed in

* AWS US East 1
* Azure West Europe
* GCP US Central 1

with and without `disableOCSPChecks` / `insecureMode` , 

Patch:
diff --git a/azure_storage_client.go b/azure_storage_client.go
index de611fc..b948d35 100644
--- a/azure_storage_client.go
+++ b/azure_storage_client.go
@@ -52,7 +52,7 @@ func (util *snowflakeAzureClient) createClient(info *execResponseStageInfo, _ bo
 				RetryDelay: 2 * time.Second,
 			},
 			Transport: &http.Client{
-				Transport: SnowflakeTransport,
+				Transport: getTransport(util.cfg),
 			},
 		},
 	})
@@ -74,7 +74,7 @@ func (util *snowflakeAzureClient) getFileHeader(meta *fileMetadata, filename str
 		return nil, err
 	}
 	path := azureLoc.path + strings.TrimLeft(filename, "/")
-	containerClient, err := createContainerClient(client.URL())
+	containerClient, err := createContainerClient(client.URL(), util.cfg)
 	if err != nil {
 		return nil, &SnowflakeError{
 			Message: "failed to create container client",
@@ -188,7 +188,7 @@ func (util *snowflakeAzureClient) uploadFile(
 			Message: "failed to cast to azure client",
 		}
 	}
-	containerClient, err := createContainerClient(client.URL())
+	containerClient, err := createContainerClient(client.URL(), util.cfg)
 
 	if err != nil {
 		return &SnowflakeError{
@@ -273,7 +273,7 @@ func (util *snowflakeAzureClient) nativeDownloadFile(
 			Message: "failed to cast to azure client",
 		}
 	}
-	containerClient, err := createContainerClient(client.URL())
+	containerClient, err := createContainerClient(client.URL(), util.cfg)
 	if err != nil {
 		return &SnowflakeError{
 			Message: "failed to create container client",
@@ -348,10 +348,10 @@ func (util *snowflakeAzureClient) detectAzureTokenExpireError(resp *http.Respons
 		strings.Contains(errStr, "Server failed to authenticate the request")
 }
 
-func createContainerClient(clientURL string) (*container.Client, error) {
+func createContainerClient(clientURL string, cfg *Config) (*container.Client, error) {
 	return container.NewClientWithNoCredential(clientURL, &container.ClientOptions{ClientOptions: azcore.ClientOptions{
 		Transport: &http.Client{
-			Transport: SnowflakeTransport,
+			Transport: getTransport(cfg),
 		},
 	}})
 }
diff --git a/connection.go b/connection.go
index bd7dc08..81d8df3 100644
--- a/connection.go
+++ b/connection.go
@@ -790,7 +790,7 @@ func buildSnowflakeConn(ctx context.Context, config Config) (*snowflakeConn, err
 	if sc.cfg.Transporter == nil {
 		if sc.cfg.DisableOCSPChecks || sc.cfg.InsecureMode {
 			// no revocation check with OCSP. Think twice when you want to enable this option.
-			st = snowflakeInsecureTransport
+			st = snowflakeNoOcspTransport
 		} else {
 			// set OCSP fail open mode
 			ocspResponseCacheLock.Lock()
@@ -856,3 +856,21 @@ func buildSnowflakeConn(ctx context.Context, config Config) (*snowflakeConn, err
 
 	return sc, nil
 }
+
+func getTransport(cfg *Config) http.RoundTripper {
+	if cfg == nil {
+		logger.Debug("getTransport: got nil Config, will perform OCSP validation for cloud storage")
+		return SnowflakeTransport
+	}
+	// if user configured a custom Transporter, prioritize that
+	if cfg.Transporter != nil {
+		logger.Debug("getTransport: using Transporter configured by the user")
+		return cfg.Transporter
+	}
+	if cfg.DisableOCSPChecks || cfg.InsecureMode {
+		logger.Debug("getTransport: skipping OCSP validation for cloud storage")
+		return snowflakeNoOcspTransport
+	}
+	logger.Debug("getTransport: will perform OCSP validation for cloud storage")
+	return SnowflakeTransport
+}
diff --git a/file_transfer_agent.go b/file_transfer_agent.go
index c30f986..74d6ec8 100644
--- a/file_transfer_agent.go
+++ b/file_transfer_agent.go
@@ -597,7 +597,7 @@ type s3BucketAccelerateConfigGetter interface {
 
 type s3ClientCreator interface {
 	extractBucketNameAndPath(location string) (*s3Location, error)
-	createClient(info *execResponseStageInfo, useAccelerateEndpoint bool) (cloudClient, error)
+	createClientWithConfig(info *execResponseStageInfo, useAccelerateEndpoint bool, cfg *Config) (cloudClient, error)
 }
 
 func (sfa *snowflakeFileTransferAgent) transferAccelerateConfigWithUtil(s3Util s3ClientCreator) error {
@@ -605,7 +605,7 @@ func (sfa *snowflakeFileTransferAgent) transferAccelerateConfigWithUtil(s3Util s
 	if err != nil {
 		return err
 	}
-	s3Cli, err := s3Util.createClient(sfa.stageInfo, false)
+	s3Cli, err := s3Util.createClientWithConfig(sfa.stageInfo, false, sfa.sc.cfg)
 	if err != nil {
 		return err
 	}
diff --git a/gcs_storage_client.go b/gcs_storage_client.go
index cee14e3..08ad6f6 100644
--- a/gcs_storage_client.go
+++ b/gcs_storage_client.go
@@ -73,7 +73,7 @@ func (util *snowflakeGcsClient) getFileHeader(meta *fileMetadata, filename strin
 			for k, v := range gcsHeaders {
 				req.Header.Add(k, v)
 			}
-			client := newGcsClient()
+			client := newGcsClient(util.cfg)
 			// for testing only
 			if meta.mockGcsClient != nil {
 				client = meta.mockGcsClient
@@ -226,7 +226,7 @@ func (util *snowflakeGcsClient) uploadFile(
 		for k, v := range gcsHeaders {
 			req.Header.Add(k, v)
 		}
-		client := newGcsClient()
+		client := newGcsClient(util.cfg)
 		// for testing only
 		if meta.mockGcsClient != nil {
 			client = meta.mockGcsClient
@@ -307,7 +307,7 @@ func (util *snowflakeGcsClient) nativeDownloadFile(
 		for k, v := range gcsHeaders {
 			req.Header.Add(k, v)
 		}
-		client := newGcsClient()
+		client := newGcsClient(util.cfg)
 		// for testing only
 		if meta.mockGcsClient != nil {
 			client = meta.mockGcsClient
@@ -409,9 +409,9 @@ func (util *snowflakeGcsClient) isTokenExpired(resp *http.Response) bool {
 	return resp.StatusCode == 401
 }
 
-func newGcsClient() gcsAPI {
+func newGcsClient(cfg *Config) gcsAPI {
 	return &http.Client{
-		Transport: SnowflakeTransport,
+		Transport: getTransport(cfg),
 	}
 }
 
diff --git a/ocsp.go b/ocsp.go
index 6a5b0c3..679c29e 100644
--- a/ocsp.go
+++ b/ocsp.go
@@ -637,7 +637,7 @@ func getRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate)
 	}
 	ocspClient := &http.Client{
 		Timeout:   timeout,
-		Transport: snowflakeInsecureTransport,
+		Transport: snowflakeNoOcspTransport,
 	}
 	ocspRes, ocspResBytes, ocspS := retryOCSP(
 		ctx, ocspClient, http.NewRequest, u, headers, ocspReq, issuer, timeout)
@@ -786,7 +786,7 @@ func downloadOCSPCacheServer() {
 	}
 	ocspClient := &http.Client{
 		Timeout:   timeout,
-		Transport: snowflakeInsecureTransport,
+		Transport: snowflakeNoOcspTransport,
 	}
 	ret, ocspStatus := checkOCSPCacheServer(context.Background(), ocspClient, http.NewRequest, u, timeout)
 	if ocspStatus.code != ocspSuccess {
@@ -1075,8 +1075,8 @@ func init() {
 	initOCSPCache()
 }
 
-// snowflakeInsecureTransport is the transport object that doesn't do certificate revocation check.
-var snowflakeInsecureTransport = &http.Transport{
+// snowflakeNoOcspTransport is the transport object that doesn't do certificate revocation check with OCSP.
+var snowflakeNoOcspTransport = &http.Transport{
 	MaxIdleConns:    10,
 	IdleConnTimeout: 30 * time.Minute,
 	Proxy:           http.ProxyFromEnvironment,
diff --git a/s3_storage_client.go b/s3_storage_client.go
index 35d962b..61a568b 100644
--- a/s3_storage_client.go
+++ b/s3_storage_client.go
@@ -59,13 +59,20 @@ func (util *snowflakeS3Client) createClient(info *execResponseStageInfo, useAcce
 		BaseEndpoint:  endPoint,
 		UseAccelerate: useAccelerateEndpoint,
 		HTTPClient: &http.Client{
-			Transport: SnowflakeTransport,
+			Transport: getTransport(util.cfg),
 		},
 		ClientLogMode: S3LoggingMode,
 		Logger:        s3Logger,
 	}), nil
 }
 
+// to be used with S3 transferAccelerateConfigWithUtil
+func (util *snowflakeS3Client) createClientWithConfig(info *execResponseStageInfo, useAccelerateEndpoint bool, cfg *Config) (cloudClient, error) {
+	// copy snowflakeFileTransferAgent's config onto the cloud client so we could decide which Transport to use
+	util.cfg = cfg
+	return util.createClient(info, useAccelerateEndpoint)
+}
+
 func getS3CustomEndpoint(info *execResponseStageInfo) *string {
 	var endPoint *string
 	isRegionalURLEnabled := info.UseRegionalURL || info.UseS3RegionalURL


Test patch:
diff --git a/client_test.go b/client_test.go
index 2e8beaa..134f89b 100644
--- a/client_test.go
+++ b/client_test.go
@@ -23,7 +23,7 @@ func (t *DummyTransport) RoundTrip(r *http.Request) (*http.Response, error) {
 		}
 		return &http.Response{StatusCode: 200}, nil
 	}
-	return snowflakeInsecureTransport.RoundTrip(r)
+	return snowflakeNoOcspTransport.RoundTrip(r)
 }
 
 func TestInternalClient(t *testing.T) {
diff --git a/connection_test.go b/connection_test.go
index e28cb3e..7b8a157 100644
--- a/connection_test.go
+++ b/connection_test.go
@@ -826,3 +826,56 @@ func TestBeginCreatesTransaction(t *testing.T) {
 		}
 	})
 }
+
+type EmptyTransporter struct{}
+
+func (t EmptyTransporter) RoundTrip(*http.Request) (*http.Response, error) {
+	return nil, nil
+}
+
+func TestGetTransport(t *testing.T) {
+	testcases := []struct {
+		name      string
+		cfg       *Config
+		transport http.RoundTripper
+	}{
+		{
+			name:      "DisableOCSPChecks and InsecureMode false",
+			cfg:       &Config{Account: "one", DisableOCSPChecks: false, InsecureMode: false},
+			transport: SnowflakeTransport,
+		},
+		{
+			name:      "DisableOCSPChecks true and InsecureMode false",
+			cfg:       &Config{Account: "two", DisableOCSPChecks: true, InsecureMode: false},
+			transport: snowflakeNoOcspTransport,
+		},
+		{
+			name:      "DisableOCSPChecks false and InsecureMode true",
+			cfg:       &Config{Account: "three", DisableOCSPChecks: false, InsecureMode: true},
+			transport: snowflakeNoOcspTransport,
+		},
+		{
+			name:      "DisableOCSPChecks and InsecureMode missing from Config",
+			cfg:       &Config{Account: "four"},
+			transport: SnowflakeTransport,
+		},
+		{
+			name:      "whole Config is missing",
+			cfg:       nil,
+			transport: SnowflakeTransport,
+		},
+		{
+			name:      "Using custom Transporter",
+			cfg:       &Config{Account: "five", DisableOCSPChecks: true, InsecureMode: false, Transporter: EmptyTransporter{}},
+			transport: EmptyTransporter{},
+		},
+	}
+	for _, test := range testcases {
+		t.Run(test.name, func(t *testing.T) {
+			result := getTransport(test.cfg)
+			if test.transport != result {
+				t.Errorf("Failed to return the correct transport, input :%#v, expected: %v, got: %v", test.cfg, test.transport, result)
+			}
+		})
+	}
+}
diff --git a/driver_test.go b/driver_test.go
index e866767..ba465dd 100644
--- a/driver_test.go
+++ b/driver_test.go
@@ -1993,7 +1993,7 @@ type CountingTransport struct {
 
 func (t *CountingTransport) RoundTrip(r *http.Request) (*http.Response, error) {
 	t.requests++
-	return snowflakeInsecureTransport.RoundTrip(r)
+	return snowflakeNoOcspTransport.RoundTrip(r)
 }
 
 func TestOpenWithTransport(t *testing.T) {
diff --git a/file_transfer_agent_test.go b/file_transfer_agent_test.go
index bf6c6a5..0b484fb 100644
--- a/file_transfer_agent_test.go
+++ b/file_transfer_agent_test.go
@@ -53,15 +53,15 @@ func TestGetBucketAccelerateConfiguration(t *testing.T) {
 
 type s3ClientCreatorMock struct {
 	extract func(string) (*s3Location, error)
-	create  func(info *execResponseStageInfo, useAccelerateEndpoint bool) (cloudClient, error)
+	create  func(info *execResponseStageInfo, useAccelerateEndpoint bool, cfg *Config) (cloudClient, error)
 }
 
 func (mock *s3ClientCreatorMock) extractBucketNameAndPath(location string) (*s3Location, error) {
 	return mock.extract(location)
 }
 
-func (mock *s3ClientCreatorMock) createClient(info *execResponseStageInfo, useAccelerateEndpoint bool) (cloudClient, error) {
-	return mock.create(info, useAccelerateEndpoint)
+func (mock *s3ClientCreatorMock) createClientWithConfig(info *execResponseStageInfo, useAccelerateEndpoint bool, cfg *Config) (cloudClient, error) {
+	return mock.create(info, useAccelerateEndpoint, cfg)
 }
 
 type s3BucketAccelerateConfigGetterMock struct {
@@ -96,7 +96,7 @@ func TestGetBucketAccelerateConfigurationTooManyRetries(t *testing.T) {
 			extract: func(s string) (*s3Location, error) {
 				return &s3Location{bucketName: "test", s3Path: "test"}, nil
 			},
-			create: func(info *execResponseStageInfo, useAccelerateEndpoint bool) (cloudClient, error) {
+			create: func(info *execResponseStageInfo, useAccelerateEndpoint bool, cfg *Config) (cloudClient, error) {
 				return &s3BucketAccelerateConfigGetterMock{err: errors.New("testing")}, nil
 			},
 		})
@@ -146,7 +146,7 @@ func TestGetBucketAccelerateConfigurationFailedCreateClient(t *testing.T) {
 			extract: func(s string) (*s3Location, error) {
 				return &s3Location{bucketName: "test", s3Path: "test"}, nil
 			},
-			create: func(info *execResponseStageInfo, useAccelerateEndpoint bool) (cloudClient, error) {
+			create: func(info *execResponseStageInfo, useAccelerateEndpoint bool, cfg *Config) (cloudClient, error) {
 				return nil, errors.New("failed creation")
 			},
 		})
@@ -172,7 +172,7 @@ func TestGetBucketAccelerateConfigurationInvalidClient(t *testing.T) {
 			extract: func(s string) (*s3Location, error) {
 				return &s3Location{bucketName: "test", s3Path: "test"}, nil
 			},
-			create: func(info *execResponseStageInfo, useAccelerateEndpoint bool) (cloudClient, error) {
+			create: func(info *execResponseStageInfo, useAccelerateEndpoint bool, cfg *Config) (cloudClient, error) {
 				return 1, nil
 			},
 		})
diff --git a/ocsp_test.go b/ocsp_test.go
index 1204b06..9a1eb89 100644
--- a/ocsp_test.go
+++ b/ocsp_test.go
@@ -34,7 +34,7 @@ func TestOCSP(t *testing.T) {
 	}
 
 	transports := []*http.Transport{
-		snowflakeInsecureTransport,
+		snowflakeNoOcspTransport,
 		SnowflakeTransport,
 	}