fix(server): force USE_SSL=false on the S3 secret when cache proxy is set (#526)

The previous "force plaintext HTTP" mechanism — three SET GLOBAL
statements at session level — was based on a misread of duckdb-httpfs
behaviour. The code comment claimed DuckDB ignored secret settings and
tunnelled HTTPS for AWS endpoints; what's actually happening is the
inverse:

    // duckdb-httpfs/src/include/s3fs.hpp:30-38
    template <class TYPE>
    SettingLookupResult TryGetSecretKeyOrSetting(...) {
        Value temp_result;
        auto setting_scope = reader.TryGetSecretKeyOrSetting(...);
        if (!temp_result.IsNull() &&
            !(setting_scope.GetScope() == SettingScope::GLOBAL
              && !use_env_variables_for_secret_settings)) {
            result = temp_result.GetValue<TYPE>();
        }
        return setting_scope;
    }

DuckDB explicitly *drops* GLOBAL-scope settings when reading the s3
secret's use_ssl / url_style, unless the env-var-for-secret-settings
flag is enabled. So `SET GLOBAL s3_use_ssl = false` was a no-op on the
secret-backed S3 path. Reads happened to render as HTTP because
DuckLake's parquet read path consults the *raw* http_proxy setting and
session-level URL form — different code, doesn't read the secret. Writes
go through the full s3fs path which honours the secret, gets
use_ssl=true, and tunnels via HTTPS CONNECT — invisible to the cache
proxy except as opaque byte counts.

Operationally that meant every parquet write produced zero
forward-proxy log lines and an upstream-rejected write surfaced only as
a generic DuckDB-side "HTTP code 501" with no proxy-side breadcrumb to
correlate against.

Fix: embed USE_SSL=false / URL_STYLE='path' on the secret itself when
HTTPProxy is set, in all three secret builders (config, credential
chain, AWS SDK). resolveS3SecretTransport centralises the decision so
the three sites stay in lock-step. Drop the two SET GLOBAL no-ops and
rewrite the comment to reflect the actual mechanism. http_proxy stays
because that *is* a session-level setting DuckDB honours.

Side effect: with the secret forcing path-style, the credential-chain
branch now also emits URL_STYLE/USE_SSL when no explicit endpoint is
configured (previously it skipped the clause when both endpoint was
empty AND no proxy was set). The proxy-set case is the only one that
materially changes shape; the no-proxy / no-endpoint case is still
left untouched (resolveS3SecretTransport falls through to the org's
preferred values).

Tests:

- TestResolveS3SecretTransport: 5 cases covering proxy-overrides-vhost,
  proxy-with-defaults, no-proxy defaults, vhost+ssl honoured,
  explicit-path preserved.
- TestBuildConfigSecretEmitsHTTPWhenProxySet: asserts the SQL contains
  URL_STYLE 'path' / USE_SSL false with proxy set, and the org's
  configured vhost/true otherwise.
- TestBuildCredentialChainSecretEmitsHTTPWhenProxySet: pinned the new
  behaviour where the credential-chain secret emits URL_STYLE/USE_SSL
  when proxy is set even with no endpoint, and stays minimal when
  neither proxy nor endpoint is set.

Followup to #524 / #525: with this in place, the forwardUncached
logging from those PRs now actually fires on writes, restoring full
request/response visibility for parquet uploads.

Author: benben

server/s3_secret_test.go

@@ -0,0 +1,126 @@
+package server
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestResolveS3SecretTransport pins down the discriminator that decides
+// what URL_STYLE / USE_SSL we embed on the duckdb_s3 secret. The choice
+// is operationally load-bearing: with HTTPProxy set, every S3 byte must
+// flow as plain HTTP through the cache proxy's forwardUncached path so
+// each request gets a logged Started/Finished pair. Any value other than
+// path + false bumps writes back to HTTPS CONNECT, where the proxy can
+// only see target+byte counts (TLS terminates between the worker and S3).
+func TestResolveS3SecretTransport(t *testing.T) {
+	tests := []struct {
+		name       string
+		cfg        DuckLakeConfig
+		wantStyle  string
+		wantUseSSL string
+	}{
+		{
+			name:       "HTTPProxy set forces path + false regardless of cfg",
+			cfg:        DuckLakeConfig{HTTPProxy: "http://10.0.0.1:8080", S3URLStyle: "vhost", S3UseSSL: true},
+			wantStyle:  "path",
+			wantUseSSL: "false",
+		},
+		{
+			name:       "HTTPProxy set with no other overrides",
+			cfg:        DuckLakeConfig{HTTPProxy: "http://10.0.0.1:8080"},
+			wantStyle:  "path",
+			wantUseSSL: "false",
+		},
+		{
+			name:       "no proxy: defaults match MinIO compatibility",
+			cfg:        DuckLakeConfig{},
+			wantStyle:  "path",
+			wantUseSSL: "false",
+		},
+		{
+			name:       "no proxy: vhost+ssl honored",
+			cfg:        DuckLakeConfig{S3URLStyle: "vhost", S3UseSSL: true},
+			wantStyle:  "vhost",
+			wantUseSSL: "true",
+		},
+		{
+			name:       "no proxy: explicit path style preserved",
+			cfg:        DuckLakeConfig{S3URLStyle: "path", S3UseSSL: true},
+			wantStyle:  "path",
+			wantUseSSL: "true",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotStyle, gotUseSSL := resolveS3SecretTransport(tt.cfg)
+			if gotStyle != tt.wantStyle {
+				t.Errorf("urlStyle = %q, want %q", gotStyle, tt.wantStyle)
+			}
+			if gotUseSSL != tt.wantUseSSL {
+				t.Errorf("useSSL = %q, want %q", gotUseSSL, tt.wantUseSSL)
+			}
+		})
+	}
+}
+
+// TestBuildConfigSecretEmitsHTTPWhenProxySet asserts the SQL produced by
+// buildConfigSecret contains URL_STYLE 'path' / USE_SSL false when an
+// HTTP proxy is in front, and the org's preferred values otherwise.
+func TestBuildConfigSecretEmitsHTTPWhenProxySet(t *testing.T) {
+	withProxy := buildConfigSecret(DuckLakeConfig{
+		S3AccessKey: "AKIA",
+		S3SecretKey: "secret",
+		S3Region:    "us-east-1",
+		S3Endpoint:  "s3.us-east-1.amazonaws.com",
+		S3URLStyle:  "vhost",
+		S3UseSSL:    true,
+		HTTPProxy:   "http://10.0.0.1:8080",
+	})
+	if !strings.Contains(withProxy, "URL_STYLE 'path'") {
+		t.Errorf("expected URL_STYLE 'path' with proxy set, got:\n%s", withProxy)
+	}
+	if !strings.Contains(withProxy, "USE_SSL false") {
+		t.Errorf("expected USE_SSL false with proxy set, got:\n%s", withProxy)
+	}
+
+	withoutProxy := buildConfigSecret(DuckLakeConfig{
+		S3AccessKey: "AKIA",
+		S3SecretKey: "secret",
+		S3Region:    "us-east-1",
+		S3Endpoint:  "s3.us-east-1.amazonaws.com",
+		S3URLStyle:  "vhost",
+		S3UseSSL:    true,
+	})
+	if !strings.Contains(withoutProxy, "URL_STYLE 'vhost'") {
+		t.Errorf("expected URL_STYLE 'vhost' without proxy, got:\n%s", withoutProxy)
+	}
+	if !strings.Contains(withoutProxy, "USE_SSL true") {
+		t.Errorf("expected USE_SSL true without proxy, got:\n%s", withoutProxy)
+	}
+}
+
+// TestBuildCredentialChainSecretEmitsHTTPWhenProxySet covers the
+// credential-chain branch, which previously only emitted USE_SSL /
+// URL_STYLE when an explicit endpoint was configured. With HTTPProxy set,
+// we always need them on the secret regardless of whether an endpoint
+// was given, otherwise the secret falls back to the AWS default
+// (use_ssl=true) and writes go via HTTPS CONNECT.
+func TestBuildCredentialChainSecretEmitsHTTPWhenProxySet(t *testing.T) {
+	// No endpoint, but proxy set — must still emit USE_SSL false / path.
+	noEndpointWithProxy := buildCredentialChainSecret(DuckLakeConfig{
+		HTTPProxy: "http://10.0.0.1:8080",
+	})
+	if !strings.Contains(noEndpointWithProxy, "USE_SSL false") {
+		t.Errorf("expected USE_SSL false with proxy + no endpoint, got:\n%s", noEndpointWithProxy)
+	}
+	if !strings.Contains(noEndpointWithProxy, "URL_STYLE 'path'") {
+		t.Errorf("expected URL_STYLE 'path' with proxy + no endpoint, got:\n%s", noEndpointWithProxy)
+	}
+
+	// No endpoint, no proxy — secret stays minimal (DuckDB defaults apply).
+	bare := buildCredentialChainSecret(DuckLakeConfig{})
+	if strings.Contains(bare, "USE_SSL") || strings.Contains(bare, "URL_STYLE") {
+		t.Errorf("expected no SSL/URL_STYLE clauses when neither endpoint nor proxy set, got:\n%s", bare)
+	}
+}

server/server.go

@@ -1375,18 +1375,20 @@ func AttachDuckLake(db *sql.DB, dlCfg DuckLakeConfig, sem chan struct{}, dataDir
 	// read (some settings don't propagate to DuckLake's subcatalogs post-attach,
 	// same gotcha as pg_pool_max_connections).
 	if dlCfg.HTTPProxy != "" {
-		// Force plaintext HTTP + path-style at the session level in addition to
-		// the S3 secret's USE_SSL/URL_STYLE — DuckDB observed to ignore secret
-		// settings and tunnel via HTTPS CONNECT when the endpoint looks like AWS
-		// S3, which the proxy can't cache (encrypted tunnel).
-		for _, stmt := range []string{
-			fmt.Sprintf("SET GLOBAL http_proxy = '%s'", dlCfg.HTTPProxy),
-			"SET GLOBAL s3_use_ssl = false",
-			"SET GLOBAL s3_url_style = 'path'",
-		} {
-			if _, err := db.Exec(stmt); err != nil {
-				slog.Warn("Failed to set httpfs proxy config.", "stmt", stmt, "error", err)
-			}
+		// Only http_proxy is set globally. Session-level SET GLOBAL
+		// s3_use_ssl = false / s3_url_style = 'path' used to live here too,
+		// as a "belt and suspenders" against DuckDB allegedly tunnelling
+		// HTTPS for AWS endpoints — but that read of the bug was wrong:
+		// duckdb-httpfs/src/include/s3fs.hpp's TryGetSecretKeyOrSetting
+		// explicitly *drops* GLOBAL-scope settings when reading the s3
+		// secret's use_ssl/url_style, so those SET GLOBALs were no-ops on
+		// the secret-backed S3 path. The actual fix is to embed
+		// USE_SSL=false / URL_STYLE='path' on the secret itself, which
+		// resolveS3SecretTransport now does whenever HTTPProxy is set —
+		// see buildConfigSecret / buildCredentialChainSecret /
+		// buildAWSSdkSecret.
+		if _, err := db.Exec(fmt.Sprintf("SET GLOBAL http_proxy = '%s'", dlCfg.HTTPProxy)); err != nil {
+			slog.Warn("Failed to set httpfs proxy config.", "stmt", "SET GLOBAL http_proxy", "error", err)
 		}
 		slog.Info("Routed httpfs traffic through forward HTTP proxy.", "proxy", dlCfg.HTTPProxy)
 	}
@@ -1777,22 +1779,45 @@ func RefreshS3Secret(db *sql.DB, dlCfg DuckLakeConfig, duckLakeSem chan struct{}
 	return nil
 }
 
+// resolveS3SecretTransport picks the URL_STYLE and USE_SSL values to embed in
+// the DuckDB S3 secret. When the cache proxy is in front of the worker
+// (HTTPProxy set), we force url_style=path + use_ssl=false so all S3
+// traffic flows as plain HTTP through forwardUncached/HandleProxy on the
+// proxy side — that's the only path that gives us per-request log lines
+// with method, status, and body preview. Without this override, an S3
+// secret with use_ssl=true makes httpfs tunnel writes via HTTPS CONNECT,
+// where the proxy can only log target+byte counts (handleConnect can't
+// see inside the TLS stream).
+//
+// Per duckdb-httpfs/src/include/s3fs.hpp:30-38, S3KeyValueReader's
+// TryGetSecretKeyOrSetting drops GLOBAL-scope settings unless the
+// use_env_variables_for_secret_settings flag is on. So a session-level
+// SET GLOBAL s3_use_ssl = false is silently ignored on the s3fs path —
+// the only effective place to set USE_SSL is on the secret itself, which
+// is what this helper does.
+func resolveS3SecretTransport(dlCfg DuckLakeConfig) (urlStyle string, useSSL string) {
+	if dlCfg.HTTPProxy != "" {
+		return "path", "false"
+	}
+	urlStyle = dlCfg.S3URLStyle
+	if urlStyle == "" {
+		urlStyle = "path" // default for MinIO compatibility
+	}
+	useSSL = "false"
+	if dlCfg.S3UseSSL {
+		useSSL = "true"
+	}
+	return urlStyle, useSSL
+}
+
 // buildConfigSecret builds a CREATE SECRET statement with explicit credentials
 func buildConfigSecret(dlCfg DuckLakeConfig) string {
 	region := dlCfg.S3Region
 	if region == "" {
 		region = "us-east-1"
 	}
 
-	urlStyle := dlCfg.S3URLStyle
-	if urlStyle == "" {
-		urlStyle = "path" // Default to path style for MinIO compatibility
-	}
-
-	useSSL := "false"
-	if dlCfg.S3UseSSL {
-		useSSL = "true"
-	}
+	urlStyle, useSSL := resolveS3SecretTransport(dlCfg)
 
 	// Build base secret with explicit credentials
 	secret := fmt.Sprintf(`
@@ -1847,21 +1872,19 @@ func buildCredentialChainSecret(dlCfg DuckLakeConfig) string {
 		secret += fmt.Sprintf(",\n\t\t\tREGION '%s'", dlCfg.S3Region)
 	}
 
-	// Add endpoint if specified (for custom S3-compatible storage)
-	if dlCfg.S3Endpoint != "" {
-		secret += fmt.Sprintf(",\n\t\t\tENDPOINT '%s'", dlCfg.S3Endpoint)
-
-		// Also set URL style and SSL for custom endpoints
-		urlStyle := dlCfg.S3URLStyle
-		if urlStyle == "" {
-			urlStyle = "path"
+	// Set URL style and SSL on the secret itself. duckdb-httpfs only honors
+	// these from the secret (or env vars if the env-var-for-secret-settings
+	// flag is on) — `SET GLOBAL s3_use_ssl = ...` at the session level is
+	// dropped by S3KeyValueReader::TryGetSecretKeyOrSetting because it
+	// filters GLOBAL-scope settings unless the env-var path is enabled.
+	// Setting it on the secret is the only knob that actually controls the
+	// http_proto = use_ssl ? "https://" : "http://" decision in s3fs.cpp.
+	if dlCfg.S3Endpoint != "" || dlCfg.HTTPProxy != "" {
+		if dlCfg.S3Endpoint != "" {
+			secret += fmt.Sprintf(",\n\t\t\tENDPOINT '%s'", dlCfg.S3Endpoint)
 		}
+		urlStyle, useSSL := resolveS3SecretTransport(dlCfg)
 		secret += fmt.Sprintf(",\n\t\t\tURL_STYLE '%s'", urlStyle)
-
-		useSSL := "false"
-		if dlCfg.S3UseSSL {
-			useSSL = "true"
-		}
 		secret += fmt.Sprintf(",\n\t\t\tUSE_SSL %s", useSSL)
 	}
 
@@ -1919,17 +1942,12 @@ func buildAWSSdkSecret(ctx context.Context, dlCfg DuckLakeConfig) (string, error
 		secret += fmt.Sprintf(",\n\t\t\tSESSION_TOKEN '%s'", creds.SessionToken)
 	}
 
-	if dlCfg.S3Endpoint != "" {
-		secret += fmt.Sprintf(",\n\t\t\tENDPOINT '%s'", dlCfg.S3Endpoint)
-		urlStyle := dlCfg.S3URLStyle
-		if urlStyle == "" {
-			urlStyle = "path"
+	if dlCfg.S3Endpoint != "" || dlCfg.HTTPProxy != "" {
+		if dlCfg.S3Endpoint != "" {
+			secret += fmt.Sprintf(",\n\t\t\tENDPOINT '%s'", dlCfg.S3Endpoint)
 		}
+		urlStyle, useSSL := resolveS3SecretTransport(dlCfg)
 		secret += fmt.Sprintf(",\n\t\t\tURL_STYLE '%s'", urlStyle)
-		useSSL := "false"
-		if dlCfg.S3UseSSL {
-			useSSL = "true"
-		}
 		secret += fmt.Sprintf(",\n\t\t\tUSE_SSL %s", useSSL)
 	}