From 1f221e600533483a72bf82182a73e4c08ab36cc4 Mon Sep 17 00:00:00 2001 From: Joyce Ling <115662568+sfc-gh-ext-simba-jl@users.noreply.github.com> Date: Mon, 21 Aug 2023 22:05:31 -0700 Subject: [PATCH 1/9] Fix the OCSP retry URL used in privatelink --- connection_util.go | 2 ++ ocsp.go | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/connection_util.go b/connection_util.go index 2a45e363c..849a8a398 100644 --- a/connection_util.go +++ b/connection_util.go @@ -281,10 +281,12 @@ func populateChunkDownloader( func (sc *snowflakeConn) setupOCSPPrivatelink(app string, host string) error { ocspCacheServer := fmt.Sprintf("http://ocsp.%v/ocsp_response_cache.json", host) + logger.Debugf("OCSP Cache Server for Privatelink: %v\n", ocspCacheServer) if err := os.Setenv(cacheServerURLEnv, ocspCacheServer); err != nil { return err } ocspRetryHost := fmt.Sprintf("http://ocsp.%v/retry/", host) + "%v/%v" + logger.Debugf("OCSP Retry URL for Privatelink: %v\n", ocspRetryHost) if err := os.Setenv(ocspRetryURLEnv, ocspRetryHost); err != nil { return err } diff --git a/ocsp.go b/ocsp.go index f0a30b995..ead25d222 100644 --- a/ocsp.go +++ b/ocsp.go @@ -485,6 +485,10 @@ func getRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate) var hostname string if retryURL := os.Getenv(ocspRetryURLEnv); retryURL != "" { hostname = fmt.Sprintf(retryURL, u.Hostname(), base64.StdEncoding.EncodeToString(ocspReq)) + u0, err := url.Parse(hostname) + if err == nil { + u = u0 + } } else { hostname = u.Hostname() } @@ -495,6 +499,10 @@ func getRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate) u = u0 } } + + logger.Debugf("Fetching OCSP response from server: %v\n", u) + logger.Debugf("Host in headers: %v\n", hostname) + headers := make(map[string]string) headers[httpHeaderContentType] = "application/ocsp-request" headers[httpHeaderAccept] = "application/ocsp-response" From da7d1fe1e59666ee26a354a0ed42dd67c5d6b3eb Mon Sep 17 00:00:00 2001 From: Joyce Ling <115662568+sfc-gh-ext-simba-jl@users.noreply.github.com> Date: Mon, 21 Aug 2023 22:10:15 -0700 Subject: [PATCH 2/9] fix hostname for retry URL --- ocsp.go | 1 + 1 file changed, 1 insertion(+) diff --git a/ocsp.go b/ocsp.go index ead25d222..68d989519 100644 --- a/ocsp.go +++ b/ocsp.go @@ -487,6 +487,7 @@ func getRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate) hostname = fmt.Sprintf(retryURL, u.Hostname(), base64.StdEncoding.EncodeToString(ocspReq)) u0, err := url.Parse(hostname) if err == nil { + hostname = u0.Hostname() u = u0 } } else { From 0b7f5ed253b5d2c14ee3ab3c8d7587c9884410a6 Mon Sep 17 00:00:00 2001 From: Dawid Heyman Date: Wed, 13 Sep 2023 16:07:45 +0200 Subject: [PATCH 3/9] Add GET fallback --- ocsp.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/ocsp.go b/ocsp.go index 68d989519..7b1bc8adb 100644 --- a/ocsp.go +++ b/ocsp.go @@ -415,6 +415,11 @@ func retryOCSP( ctx, client, req, ocspHost, headers, totalTimeout*time.Duration(multiplier)).doPost().setBody(reqBody).execute() if err != nil { + logger.Infof("error when executing POST request to retry OCSP: %v", err) + logger.Infof("falling back to GET OCSP request") + res, err = newRetryHTTP( + ctx, client, req, ocspHost, headers, + totalTimeout*time.Duration(multiplier)).execute() return ocspRes, ocspResBytes, &ocspStatus{ code: ocspFailedSubmit, err: err, @@ -450,6 +455,17 @@ func retryOCSP( } } +func fullOCSPURL(url *url.URL) string { + fullUrl := url.Hostname() + if url.Path != "" { + if !strings.HasPrefix(url.Path, "/") { + fullUrl += "/" + } + fullUrl += url.Path + } + return fullUrl +} + // getRevocationStatus checks the certificate revocation status for subject using issuer certificate. func getRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate) *ocspStatus { logger.Infof("Subject: %v, Issuer: %v\n", subject.Subject, issuer.Subject) @@ -484,14 +500,14 @@ func getRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate) hostnameStr := os.Getenv(ocspTestResponderURLEnv) var hostname string if retryURL := os.Getenv(ocspRetryURLEnv); retryURL != "" { - hostname = fmt.Sprintf(retryURL, u.Hostname(), base64.StdEncoding.EncodeToString(ocspReq)) + hostname = fmt.Sprintf(retryURL, fullOCSPURL(u), base64.StdEncoding.EncodeToString(ocspReq)) u0, err := url.Parse(hostname) if err == nil { hostname = u0.Hostname() u = u0 } } else { - hostname = u.Hostname() + hostname = fullOCSPURL(u) } if hostnameStr != "" { u0, err := url.Parse(hostnameStr) From b450a5596e9c8c6e04a0f606310d382f662a2f16 Mon Sep 17 00:00:00 2001 From: Dawid Heyman Date: Thu, 14 Sep 2023 16:11:16 +0200 Subject: [PATCH 4/9] Fixed fallback OCSP GET request --- ocsp.go | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 53 insertions(+), 7 deletions(-) diff --git a/ocsp.go b/ocsp.go index 7b1bc8adb..82a39db15 100644 --- a/ocsp.go +++ b/ocsp.go @@ -415,11 +415,6 @@ func retryOCSP( ctx, client, req, ocspHost, headers, totalTimeout*time.Duration(multiplier)).doPost().setBody(reqBody).execute() if err != nil { - logger.Infof("error when executing POST request to retry OCSP: %v", err) - logger.Infof("falling back to GET OCSP request") - res, err = newRetryHTTP( - ctx, client, req, ocspHost, headers, - totalTimeout*time.Duration(multiplier)).execute() return ocspRes, ocspResBytes, &ocspStatus{ code: ocspFailedSubmit, err: err, @@ -433,7 +428,6 @@ func retryOCSP( err: fmt.Errorf("HTTP code is not OK. %v: %v", res.StatusCode, res.Status), } } - logger.Debug("reading contents") ocspResBytes, err = io.ReadAll(res.Body) if err != nil { return ocspRes, ocspResBytes, &ocspStatus{ @@ -441,7 +435,58 @@ func retryOCSP( err: err, } } - logger.Debug("parsing OCSP response") + ocspRes, err = ocsp.ParseResponse(ocspResBytes, issuer) + if err != nil { + logger.Warnf("error when parsing ocsp response: %v\n", err) + logger.Warnf("performing GET fallback request to OCSP\n") + return fallbackRetryOCSPToGETRequest(ctx, client, req, ocspHost, headers, issuer, totalTimeout) + } + + logger.Debugf("OCSP ResponseStatus from server: %v\n", ocspRes.Status) + return ocspRes, ocspResBytes, &ocspStatus{ + code: ocspSuccess, + } +} + +func fallbackRetryOCSPToGETRequest( + ctx context.Context, + client clientInterface, + req requestFunc, + ocspHost *url.URL, + headers map[string]string, + issuer *x509.Certificate, + totalTimeout time.Duration) ( + ocspRes *ocsp.Response, + ocspResBytes []byte, + ocspS *ocspStatus) { + multiplier := 1 + if atomic.LoadUint32((*uint32)(&ocspFailOpen)) == (uint32)(OCSPFailOpenFalse) { + multiplier = 3 // up to 3 times for Fail Close mode + } + res, err := newRetryHTTP( + ctx, client, req, ocspHost, headers, + totalTimeout*time.Duration(multiplier)).execute() + if err != nil { + return ocspRes, ocspResBytes, &ocspStatus{ + code: ocspFailedSubmit, + err: err, + } + } + defer res.Body.Close() + logger.Debugf("GET fallback StatusCode from OCSP Server: %v\n", res.StatusCode) + if res.StatusCode != http.StatusOK { + return ocspRes, ocspResBytes, &ocspStatus{ + code: ocspFailedResponse, + err: fmt.Errorf("HTTP code is not OK. %v: %v", res.StatusCode, res.Status), + } + } + ocspResBytes, err = io.ReadAll(res.Body) + if err != nil { + return ocspRes, ocspResBytes, &ocspStatus{ + code: ocspFailedExtractResponse, + err: err, + } + } ocspRes, err = ocsp.ParseResponse(ocspResBytes, issuer) if err != nil { return ocspRes, ocspResBytes, &ocspStatus{ @@ -450,6 +495,7 @@ func retryOCSP( } } + logger.Debugf("GET fallback OCSP ResponseStatus from server: %v\n", ocspRes.Status) return ocspRes, ocspResBytes, &ocspStatus{ code: ocspSuccess, } From e392583de943fb08108329bd71138fdaa24f8a45 Mon Sep 17 00:00:00 2001 From: Dawid Heyman Date: Fri, 15 Sep 2023 11:45:31 +0200 Subject: [PATCH 5/9] Add logs --- ocsp.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/ocsp.go b/ocsp.go index 82a39db15..a1eaed141 100644 --- a/ocsp.go +++ b/ocsp.go @@ -442,7 +442,7 @@ func retryOCSP( return fallbackRetryOCSPToGETRequest(ctx, client, req, ocspHost, headers, issuer, totalTimeout) } - logger.Debugf("OCSP ResponseStatus from server: %v\n", ocspRes.Status) + logger.Debugf("OCSP Status from server: %v\n", ocspRes.Status) return ocspRes, ocspResBytes, &ocspStatus{ code: ocspSuccess, } @@ -495,12 +495,25 @@ func fallbackRetryOCSPToGETRequest( } } - logger.Debugf("GET fallback OCSP ResponseStatus from server: %v\n", ocspRes.Status) + logger.Debugf("GET fallback OCSP Status from server: %v\n", printStatus(ocspRes)) return ocspRes, ocspResBytes, &ocspStatus{ code: ocspSuccess, } } +func printStatus(response *ocsp.Response) string { + switch response.Status { + case ocsp.Good: + return "Good" + case ocsp.Revoked: + return "Revoked" + case ocsp.Unknown: + return "Unknown" + default: + return fmt.Sprintf("%d", response.Status) + } +} + func fullOCSPURL(url *url.URL) string { fullUrl := url.Hostname() if url.Path != "" { From 4dd49e7a9ddeb1abaf933461a6d374a65f2adae5 Mon Sep 17 00:00:00 2001 From: Dawid Heyman Date: Fri, 15 Sep 2023 11:47:15 +0200 Subject: [PATCH 6/9] Fix lint --- arrow_test.go | 2 +- ocsp.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/arrow_test.go b/arrow_test.go index cca07603d..e4d103c78 100644 --- a/arrow_test.go +++ b/arrow_test.go @@ -13,7 +13,7 @@ import ( "time" ) -//A test just to show Snowflake version +// A test just to show Snowflake version func TestCheckVersion(t *testing.T) { conn := openConn(t) defer conn.Close() diff --git a/ocsp.go b/ocsp.go index a1eaed141..0cb3862c6 100644 --- a/ocsp.go +++ b/ocsp.go @@ -515,14 +515,14 @@ func printStatus(response *ocsp.Response) string { } func fullOCSPURL(url *url.URL) string { - fullUrl := url.Hostname() + fullURL := url.Hostname() if url.Path != "" { if !strings.HasPrefix(url.Path, "/") { - fullUrl += "/" + fullURL += "/" } - fullUrl += url.Path + fullURL += url.Path } - return fullUrl + return fullURL } // getRevocationStatus checks the certificate revocation status for subject using issuer certificate. From 5b31205da8bd17d069989daee079d2141ac9a01d Mon Sep 17 00:00:00 2001 From: Dawid Heyman Date: Fri, 15 Sep 2023 12:21:51 +0200 Subject: [PATCH 7/9] Add review suggestions --- connection_util.go | 6 +++--- ocsp.go | 31 +++++++++++++++---------------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/connection_util.go b/connection_util.go index a1fc5a7f2..4d37dea28 100644 --- a/connection_util.go +++ b/connection_util.go @@ -285,9 +285,9 @@ func (sc *snowflakeConn) setupOCSPPrivatelink(app string, host string) error { if err := os.Setenv(cacheServerURLEnv, ocspCacheServer); err != nil { return err } - ocspRetryHost := fmt.Sprintf("http://ocsp.%v/retry/", host) + "%v/%v" - logger.Debugf("OCSP Retry URL for Privatelink: %v\n", ocspRetryHost) - if err := os.Setenv(ocspRetryURLEnv, ocspRetryHost); err != nil { + ocspRetryHostTemplate := fmt.Sprintf("http://ocsp.%v/retry/", host) + "%v/%v" + logger.Debugf("OCSP Retry URL for Privatelink: %v\n", ocspRetryHostTemplate) + if err := os.Setenv(ocspRetryURLEnv, ocspRetryHostTemplate); err != nil { return err } return nil diff --git a/ocsp.go b/ocsp.go index 023e6a931..5371b154c 100644 --- a/ocsp.go +++ b/ocsp.go @@ -360,14 +360,14 @@ func checkOCSPCacheServer( headers := make(map[string]string) res, err := newRetryHTTP(ctx, client, req, ocspServerHost, headers, totalTimeout, defaultTimeProvider).execute() if err != nil { - logger.Errorf("failed to get OCSP cache from OCSP Cache Server. %v\n", err) + logger.Errorf("failed to get OCSP cache from OCSP Cache Server. %v", err) return nil, &ocspStatus{ code: ocspFailedSubmit, err: err, } } defer res.Body.Close() - logger.Debugf("StatusCode from OCSP Cache Server: %v\n", res.StatusCode) + logger.Debugf("StatusCode from OCSP Cache Server: %v", res.StatusCode) if res.StatusCode != http.StatusOK { return nil, &ocspStatus{ code: ocspFailedResponse, @@ -381,7 +381,7 @@ func checkOCSPCacheServer( if err := dec.Decode(&respd); err == io.EOF { break } else if err != nil { - logger.Errorf("failed to decode OCSP cache. %v\n", err) + logger.Errorf("failed to decode OCSP cache. %v", err) return nil, &ocspStatus{ code: ocspFailedExtractResponse, err: err, @@ -437,12 +437,12 @@ func retryOCSP( } ocspRes, err = ocsp.ParseResponse(ocspResBytes, issuer) if err != nil { - logger.Warnf("error when parsing ocsp response: %v\n", err) - logger.Warnf("performing GET fallback request to OCSP\n") + logger.Warnf("error when parsing ocsp response: %v", err) + logger.Warnf("performing GET fallback request to OCSP") return fallbackRetryOCSPToGETRequest(ctx, client, req, ocspHost, headers, issuer, totalTimeout) } - logger.Debugf("OCSP Status from server: %v\n", ocspRes.Status) + logger.Debugf("OCSP Status from server: %v", ocspRes.Status) return ocspRes, ocspResBytes, &ocspStatus{ code: ocspSuccess, } @@ -463,9 +463,8 @@ func fallbackRetryOCSPToGETRequest( if atomic.LoadUint32((*uint32)(&ocspFailOpen)) == (uint32)(OCSPFailOpenFalse) { multiplier = 3 // up to 3 times for Fail Close mode } - res, err := newRetryHTTP( - ctx, client, req, ocspHost, headers, - totalTimeout*time.Duration(multiplier)).execute() + res, err := newRetryHTTP(ctx, client, req, ocspHost, headers, + totalTimeout*time.Duration(multiplier), defaultTimeProvider).execute() if err != nil { return ocspRes, ocspResBytes, &ocspStatus{ code: ocspFailedSubmit, @@ -473,7 +472,7 @@ func fallbackRetryOCSPToGETRequest( } } defer res.Body.Close() - logger.Debugf("GET fallback StatusCode from OCSP Server: %v\n", res.StatusCode) + logger.Debugf("GET fallback StatusCode from OCSP Server: %v", res.StatusCode) if res.StatusCode != http.StatusOK { return ocspRes, ocspResBytes, &ocspStatus{ code: ocspFailedResponse, @@ -495,7 +494,7 @@ func fallbackRetryOCSPToGETRequest( } } - logger.Debugf("GET fallback OCSP Status from server: %v\n", printStatus(ocspRes)) + logger.Debugf("GET fallback OCSP Status from server: %v", printStatus(ocspRes)) return ocspRes, ocspResBytes, &ocspStatus{ code: ocspSuccess, } @@ -527,7 +526,7 @@ func fullOCSPURL(url *url.URL) string { // getRevocationStatus checks the certificate revocation status for subject using issuer certificate. func getRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate) *ocspStatus { - logger.Infof("Subject: %v, Issuer: %v\n", subject.Subject, issuer.Subject) + logger.Infof("Subject: %v, Issuer: %v", subject.Subject, issuer.Subject) status, ocspReq, encodedCertID := validateWithCache(subject, issuer) if isValidOCSPStatus(status.code) { @@ -536,8 +535,8 @@ func getRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate) if ocspReq == nil || encodedCertID == nil { return status } - logger.Infof("cache missed\n") - logger.Infof("OCSP Server: %v\n", subject.OCSPServer) + logger.Infof("cache missed") + logger.Infof("OCSP Server: %v", subject.OCSPServer) if len(subject.OCSPServer) == 0 || isTestNoOCSPURL() { return &ocspStatus{ code: ocspNoServer, @@ -576,8 +575,8 @@ func getRevocationStatus(ctx context.Context, subject, issuer *x509.Certificate) } } - logger.Debugf("Fetching OCSP response from server: %v\n", u) - logger.Debugf("Host in headers: %v\n", hostname) + logger.Debugf("Fetching OCSP response from server: %v", u) + logger.Debugf("Host in headers: %v", hostname) headers := make(map[string]string) headers[httpHeaderContentType] = "application/ocsp-request" From 34019cd43a57062d749e7a39727b8336a91f0dc0 Mon Sep 17 00:00:00 2001 From: Dawid Heyman Date: Tue, 19 Sep 2023 11:46:12 +0200 Subject: [PATCH 8/9] test --- ocsp_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/ocsp_test.go b/ocsp_test.go index 257e87978..e2deb515f 100644 --- a/ocsp_test.go +++ b/ocsp_test.go @@ -340,6 +340,44 @@ func TestOCSPRetry(t *testing.T) { } } +func TestFullOCSPURL(t *testing.T) { + testcases := []tcFullOCSPURL{ + { + url: &url.URL{Host: "some-ocsp-url.com"}, + expectedURLString: "some-ocsp-url.com", + }, + { + url: &url.URL{ + Host: "some-ocsp-url.com", + Path: "/some-path", + }, + expectedURLString: "some-ocsp-url.com/some-path", + }, + { + url: &url.URL{ + Host: "some-ocsp-url.com", + Path: "some-path", + }, + expectedURLString: "some-ocsp-url.com/some-path", + }, + } + + for _, testcase := range testcases { + t.Run("", func(t *testing.T) { + returnedStringURL := fullOCSPURL(testcase.url) + if returnedStringURL != testcase.expectedURLString { + t.Fatalf("failed to match returned OCSP url string; expected: %v, got: %v", + testcase.expectedURLString, returnedStringURL) + } + }) + } +} + +type tcFullOCSPURL struct { + url *url.URL + expectedURLString string +} + func TestOCSPCacheServerRetry(t *testing.T) { dummyOCSPHost := &url.URL{ Scheme: "https", From 44521c966f6d254e155765380be3396462d9557c Mon Sep 17 00:00:00 2001 From: Dawid Heyman Date: Tue, 19 Sep 2023 12:16:22 +0200 Subject: [PATCH 9/9] Add ocsp retry privatelink cleanup --- data1.txt.gz | 0 driver_ocsp_test.go | 1 + ocsp.go | 4 +++- 3 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 data1.txt.gz diff --git a/data1.txt.gz b/data1.txt.gz new file mode 100644 index 000000000..e69de29bb diff --git a/driver_ocsp_test.go b/driver_ocsp_test.go index 9070c4c88..8ce109a3b 100644 --- a/driver_ocsp_test.go +++ b/driver_ocsp_test.go @@ -49,6 +49,7 @@ func cleanup() { unsetenv(ocspTestResponderTimeoutEnv) unsetenv(ocspTestResponderURLEnv) unsetenv(ocspTestNoOCSPURLEnv) + unsetenv(ocspRetryURLEnv) unsetenv(cacheDirEnv) } diff --git a/ocsp.go b/ocsp.go index 5371b154c..c809a24c6 100644 --- a/ocsp.go +++ b/ocsp.go @@ -442,12 +442,14 @@ func retryOCSP( return fallbackRetryOCSPToGETRequest(ctx, client, req, ocspHost, headers, issuer, totalTimeout) } - logger.Debugf("OCSP Status from server: %v", ocspRes.Status) + logger.Debugf("OCSP Status from server: %v", printStatus(ocspRes)) return ocspRes, ocspResBytes, &ocspStatus{ code: ocspSuccess, } } +// fallbackRetryOCSPToGETRequest is the third level of retry method. Some OCSP responders do not support POST requests +// and will return with a "malformed" request error. In that case we also try to perform a GET request func fallbackRetryOCSPToGETRequest( ctx context.Context, client clientInterface,