From 3aed1f0b6aa27321b1d4aa1d8b853abaad137680 Mon Sep 17 00:00:00 2001 From: Aditya Thebe Date: Thu, 16 Jul 2026 22:46:35 +0545 Subject: [PATCH] fix(security): harden connection probes and 1Password reads Connection tests passed operator-supplied URLs directly to the HTTP client, allowing arbitrary paths to be requested from the query server. Pin probes to the validated host while using a fixed root URL, preserving configured authentication and TLS behavior. Require 1Password secret references and terminate op CLI options before passing the reference so user input cannot be interpreted as command flags. --- cmd/query/connections/actions.go | 81 ++++++++++++++++++++++++++- cmd/query/connections/actions_test.go | 18 ++++++ context/onepassword.go | 5 +- context/onepassword_test.go | 10 +++- 4 files changed, 109 insertions(+), 5 deletions(-) diff --git a/cmd/query/connections/actions.go b/cmd/query/connections/actions.go index 78a89ac..64d2ce4 100644 --- a/cmd/query/connections/actions.go +++ b/cmd/query/connections/actions.go @@ -1,6 +1,9 @@ package connections import ( + "context" + "crypto/tls" + "crypto/x509" "encoding/json" "fmt" "net" @@ -12,6 +15,7 @@ import ( dbconnection "github.com/flanksource/commons-db/connection" dbcontext "github.com/flanksource/commons-db/context" "github.com/flanksource/commons-db/models" + "github.com/flanksource/commons/http/middlewares" ) // connectionActionsHandler powers the connection form's "Test" split-button: @@ -247,14 +251,22 @@ func httpProbe(ctx dbcontext.Context, c *models.Connection, displayURL string) t if err != nil { return testResult{OK: false, Message: fmt.Sprintf("HTTP authentication setup failed: %v", redactError(err, c.URL, displayURL)), URL: displayURL} } + transport, err := newHTTPProbeTransport(httpConnection, target) + if err != nil { + return testResult{OK: false, Message: fmt.Sprintf("HTTP authentication setup failed: %v", redactError(err, c.URL, displayURL)), URL: displayURL} + } client := &http.Client{ - Transport: httpConnection.Transport(), + Transport: transport, Timeout: 8 * time.Second, CheckRedirect: func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse }, } - req, err := http.NewRequest(http.MethodGet, target.url.String(), nil) + probeURL := "http://connection-probe.invalid/" + if target.url.Scheme == "https" { + probeURL = "https://connection-probe.invalid/" + } + req, err := http.NewRequest(http.MethodGet, probeURL, nil) if err != nil { return testResult{OK: false, Message: fmt.Sprintf("HTTP request failed: %v", err), URL: displayURL} } @@ -284,6 +296,71 @@ type httpProbeTarget struct { hostHeader string } +// newHTTPProbeTransport preserves configured authentication and TLS while +// pinning the request to the validated connection target. +func newHTTPProbeTransport(conn dbconnection.HTTPConnection, target *httpProbeTarget) (http.RoundTripper, error) { + tlsConfig := &tls.Config{ + ServerName: target.url.Hostname(), + InsecureSkipVerify: conn.TLS.InsecureSkipVerify, //nolint:gosec // explicit per-connection opt-in + } + if !conn.TLS.CA.IsEmpty() { + roots, err := x509.SystemCertPool() + if err != nil || roots == nil { + roots = x509.NewCertPool() + } + if !roots.AppendCertsFromPEM([]byte(conn.TLS.CA.ValueStatic)) { + return nil, fmt.Errorf("invalid TLS CA certificate") + } + tlsConfig.RootCAs = roots + } + if conn.TLS.Cert.IsEmpty() != conn.TLS.Key.IsEmpty() { + return nil, fmt.Errorf("mTLS requires both a client certificate and private key") + } + if !conn.TLS.Cert.IsEmpty() { + certificate, err := tls.X509KeyPair([]byte(conn.TLS.Cert.ValueStatic), []byte(conn.TLS.Key.ValueStatic)) + if err != nil { + return nil, fmt.Errorf("invalid mTLS client certificate or private key: %w", err) + } + tlsConfig.Certificates = []tls.Certificate{certificate} + } + + base := &http.Transport{ + DialContext: func(ctx context.Context, network, _ string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, network, target.hostPort) + }, + TLSClientConfig: tlsConfig, + TLSHandshakeTimeout: conn.TLS.HandshakeTimeout, + } + if !conn.HTTPBasicAuth.IsEmpty() { + return roundTripperFunc(func(req *http.Request) (*http.Response, error) { + req.SetBasicAuth(conn.GetUsername(), conn.GetPassword()) + return base.RoundTrip(req) + }), nil + } + if !conn.Bearer.IsEmpty() { + return roundTripperFunc(func(req *http.Request) (*http.Response, error) { + req.Header.Set("Authorization", "Bearer "+conn.Bearer.ValueStatic) + return base.RoundTrip(req) + }), nil + } + if !conn.OAuth.IsEmpty() { + return middlewares.NewOauthTransport(middlewares.OauthConfig{ + ClientID: conn.OAuth.ClientID.String(), + ClientSecret: conn.OAuth.ClientSecret.String(), + TokenURL: conn.OAuth.TokenURL, + Params: conn.OAuth.Params, + Scopes: conn.OAuth.Scopes, + }).RoundTripper(base), nil + } + return base, nil +} + +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + func validatedHTTPProbeTarget(rawURL string) (*httpProbeTarget, error) { u, err := url.Parse(rawURL) if err != nil { diff --git a/cmd/query/connections/actions_test.go b/cmd/query/connections/actions_test.go index bef307f..b67e263 100644 --- a/cmd/query/connections/actions_test.go +++ b/cmd/query/connections/actions_test.go @@ -157,6 +157,24 @@ func TestTestConnectionHTTPReachable(t *testing.T) { } } +func TestTestConnectionHTTPProbesRootOnly(t *testing.T) { + var requestURI string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestURI = r.RequestURI + w.WriteHeader(http.StatusNoContent) + })) + defer ts.Close() + + ctx := dbcontext.NewContext(context.Background()) + res := testConnection(ctx, &models.Connection{Type: "http", URL: ts.URL + "/admin?token=secret"}) + if !res.OK { + t.Fatalf("expected reachable, got %+v", res) + } + if requestURI != "/" { + t.Fatalf("probe request URI = %q, want root only", requestURI) + } +} + func TestTestConnectionHTTPRedactsURL(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) diff --git a/context/onepassword.go b/context/onepassword.go index aaa15ff..50ce8ab 100644 --- a/context/onepassword.go +++ b/context/onepassword.go @@ -76,10 +76,13 @@ func tokenFingerprint(token string) string { // opRead invokes `op read` for a single reference. When a token is supplied it // is injected via OP_SERVICE_ACCOUNT_TOKEN for non-interactive resolution. func opRead(ctx Context, ref, token string) (string, error) { + if !strings.HasPrefix(ref, "op://") { + return "", fmt.Errorf("invalid 1password reference") + } if _, err := exec.LookPath("op"); err != nil { return "", fmt.Errorf("1password CLI (op) not found in PATH: %w", err) } - cmd := exec.CommandContext(ctx, "op", "read", "--no-newline", ref) + cmd := exec.CommandContext(ctx, "op", "read", "--no-newline", "--", ref) cmd.Env = os.Environ() if token != "" { cmd.Env = append(cmd.Env, "OP_SERVICE_ACCOUNT_TOKEN="+token) diff --git a/context/onepassword_test.go b/context/onepassword_test.go index 69739b7..ed6891a 100644 --- a/context/onepassword_test.go +++ b/context/onepassword_test.go @@ -44,6 +44,12 @@ func TestGetOnePasswordValueFromCache(t *testing.T) { } } +func TestOpReadRejectsNonSecretReferences(t *testing.T) { + if _, err := opRead(newOnePasswordTestContext(), "--help", ""); err == nil { + t.Fatal("expected a non-op reference to be rejected") + } +} + func TestOnePasswordTokenPrefersProperty(t *testing.T) { t.Setenv("OP_SERVICE_ACCOUNT_TOKEN", "env-token") @@ -65,8 +71,8 @@ func TestOnePasswordTokenPrefersProperty(t *testing.T) { func TestTokenFingerprintIsolatesCache(t *testing.T) { const ( - tokenA = "a" - tokenB = "b" + tokenA = "a" + tokenB = "b" unkeyedFingerprintForTokenA = "ca978112ca1bbdca" )