From 030a141150bde6d46d36f0f346a769c0033fd095 Mon Sep 17 00:00:00 2001 From: rzisholz Date: Sun, 6 Sep 2026 14:36:32 +0300 Subject: [PATCH 1/6] CP-26002: validate ARK_DISCOVERY_API itself against the domain allowlist The domain/HTTPS allowlist added for identity/discoverycontext/secrets_manager (CP-25960) had a carve-out: any host equal to the discovery endpoint's own host was trusted automatically, since ARK_DISCOVERY_API itself was never checked. That carve-out is exactly the attack CP-23593's PoC demonstrates -- an attacker with ARK_DISCOVERY_API write access (a tampered pod spec or Helm values) bootstraps the whole trust chain from their own infrastructure. HTTPS-only closed the loopback shape of it (no valid cert for 127.0.0.1), but a rogue host with a real globally-trusted cert wasn't caught. DiscoverServices now requires its own base URL to be HTTPS on an allowed CyberArk domain too, collapsing the "same host as discoveryHost" carve-out into "host is on the allowlist" -- so isAllowedServiceHost is gone; only hostOnAllowedRootDomain remains. This breaks every test that feeds a real httptest mock address (127.0.0.1) into a Services value or ARK_DISCOVERY_API, since those addresses aren't on the allowlist either. Fixed by adding a small shared test-only registry (internal/cyberark/testing/mockdial.go): servicediscovery.MockDiscoveryServer launders any loopback address in the Services it's given (and its own ARK_DISCOVERY_API override) into a fake CyberArk-domain-looking hostname, registering a dial redirect to the real address. Every other package's Mock*Server (conjur, dataupload, identity) wraps its own returned client the same way, since tests freely reuse one mock's client to call a different mock's server -- any of them might end up being the one that has to resolve a fake host registered elsewhere. --- internal/cyberark/conjur/mock.go | 17 +++- internal/cyberark/dataupload/mock.go | 6 ++ internal/cyberark/identity/mock.go | 6 ++ .../cyberark/servicediscovery/discovery.go | 79 +++++++++--------- internal/cyberark/servicediscovery/mock.go | 82 ++++++++++++++++--- internal/cyberark/testing/mockdial.go | 56 +++++++++++++ 6 files changed, 193 insertions(+), 53 deletions(-) create mode 100644 internal/cyberark/testing/mockdial.go diff --git a/internal/cyberark/conjur/mock.go b/internal/cyberark/conjur/mock.go index c3846bd0..7f591207 100644 --- a/internal/cyberark/conjur/mock.go +++ b/internal/cyberark/conjur/mock.go @@ -4,8 +4,21 @@ import ( "net/http" "net/http/httptest" "testing" + + cyberarktesting "github.com/jetstack/preflight/internal/cyberark/testing" ) +// mockClientFor wraps srv's client so it can also reach any fake +// CyberArk-domain-looking host registered by another mock's +// cyberarktesting.RegisterMockHost (e.g. servicediscovery.MockDiscoveryServer), +// since a test may reuse this client to make a discovery call rather than +// discovery's own — see servicediscovery.MockDiscoveryServer's doc comment. +func mockClientFor(srv *httptest.Server) *http.Client { + client := srv.Client() + cyberarktesting.WrapMockTransport(client.Transport.(*http.Transport)) + return client +} + // MockConjurExchangeServer returns a TLS server whose authn-jwt endpoint returns the given token. func MockConjurExchangeServer(t testing.TB, token string) (*httptest.Server, *http.Client) { t.Helper() @@ -16,7 +29,7 @@ func MockConjurExchangeServer(t testing.TB, token string) (*httptest.Server, *ht } _, _ = w.Write([]byte(token)) })) - return srv, srv.Client() + return srv, mockClientFor(srv) } func MockConjurExchangeServerStatus(t testing.TB, status int) (*httptest.Server, *http.Client) { @@ -33,5 +46,5 @@ func MockConjurExchangeServerStatusBody(t testing.TB, status int, body []byte) ( w.WriteHeader(status) _, _ = w.Write(body) })) - return srv, srv.Client() + return srv, mockClientFor(srv) } diff --git a/internal/cyberark/dataupload/mock.go b/internal/cyberark/dataupload/mock.go index 28403775..0c67183b 100644 --- a/internal/cyberark/dataupload/mock.go +++ b/internal/cyberark/dataupload/mock.go @@ -20,6 +20,7 @@ import ( "k8s.io/client-go/transport" arkapi "github.com/jetstack/preflight/internal/cyberark/api" + cyberarktesting "github.com/jetstack/preflight/internal/cyberark/testing" "github.com/jetstack/preflight/pkg/version" ) @@ -78,6 +79,11 @@ func MockDataUploadServer(t testing.TB) (string, *http.Client) { mds.serverURL = server.URL httpClient := server.Client() + // So this client can also reach a fake CyberArk-domain-looking host + // registered by another mock (servicediscovery.MockDiscoveryServer), in + // case a test reuses it to make a discovery call rather than discovery's + // own client. + cyberarktesting.WrapMockTransport(httpClient.Transport.(*http.Transport)) httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext) return server.URL, httpClient } diff --git a/internal/cyberark/identity/mock.go b/internal/cyberark/identity/mock.go index 928ff868..0c3e69de 100644 --- a/internal/cyberark/identity/mock.go +++ b/internal/cyberark/identity/mock.go @@ -12,6 +12,7 @@ import ( "k8s.io/client-go/transport" arkapi "github.com/jetstack/preflight/internal/cyberark/api" + cyberarktesting "github.com/jetstack/preflight/internal/cyberark/testing" "github.com/jetstack/preflight/pkg/version" _ "embed" @@ -81,6 +82,11 @@ func MockIdentityServer(t testing.TB) (string, *http.Client) { server := httptest.NewTLSServer(mis) t.Cleanup(server.Close) httpClient := server.Client() + // So this client can also reach a fake CyberArk-domain-looking host + // registered by another mock (servicediscovery.MockDiscoveryServer), in + // case a test reuses it to make a discovery call rather than discovery's + // own client. + cyberarktesting.WrapMockTransport(httpClient.Transport.(*http.Transport)) httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext) return server.URL, httpClient } diff --git a/internal/cyberark/servicediscovery/discovery.go b/internal/cyberark/servicediscovery/discovery.go index b6e27809..a2deb0ae 100644 --- a/internal/cyberark/servicediscovery/discovery.go +++ b/internal/cyberark/servicediscovery/discovery.go @@ -43,15 +43,19 @@ const ( maxDiscoverBodySize = 2 * 1024 * 1024 ) -// allowedRootDomains are the only root domains a discovery response is -// allowed to point us at for identity/discoverycontext/secrets_manager. -// Without this, mainActiveAPI's ep.API is trusted verbatim from the response -// body and handed straight to the Conjur/Identity clients, which then POST -// the agent's SA token (or username/password) to it — an SSRF-shaped hole if -// the response is ever tampered with. Mirrors the per-env ROOT_DOMAIN -// allowlist already enforced on the discoverycontext-regional-resources side -// (token.py, for the JWT `iss` host) — copied by value here since these -// domains rarely change and the agent has no access to that env-keyed map. +// allowedRootDomains are the only root domains trusted for both (a) the +// discovery bootstrap call itself — c.baseURL, which is ARK_DISCOVERY_API if +// set — and (b) the identity/discoverycontext/secrets_manager hosts that +// call's response points us at. Without (b), mainActiveAPI's ep.API is +// trusted verbatim from the response body and handed straight to the +// Conjur/Identity clients, which then POST the agent's SA token (or +// username/password) to it. Without (a), ARK_DISCOVERY_API write access +// (e.g. a tampered pod spec or Helm values) bootstraps the entire trust +// chain from arbitrary infrastructure regardless of (b) — see CP-26002 / +// CP-23593. Mirrors the per-env ROOT_DOMAIN allowlist already enforced on +// the discoverycontext-regional-resources side (token.py, for the JWT `iss` +// host) — copied by value here since these domains rarely change and the +// agent has no access to that env-keyed map. // // Source of truth is the `everest_env_utils` package's ROOT_DOMAIN map // (published to Artifactory as everest_env_utils_cyberark, v2.0.117 as of @@ -81,26 +85,14 @@ var allowedRootDomains = []string{ "cyberarkgov.cloud", } -// isAllowedServiceHost reports whether host is, or is a subdomain of, one of -// allowedRootDomains — or is exactly discoveryHost, the host the discovery -// request was addressed to. The latter matters for ARK_DISCOVERY_API- -// overridden (dev/CI/test) discovery endpoints: whatever host that override -// already points at is at least as trusted as the discovery call itself. +// hostOnAllowedRootDomain reports whether host is, or is a subdomain of, one +// of allowedRootDomains. // -// This comparison is hostname-only: discoveryHost carries no port, and -// ARK_DISCOVERY_API is not guaranteed to be HTTPS or to be the host the -// request actually landed on after redirects — the follow-up in CP-26002 -// (validating ARK_DISCOVERY_API itself against allowedRootDomains, removing -// this whole escape hatch) is the actual fix for both gaps; a host:port -// comparison here alone would break every test that currently relies on -// same-host-different-port mocks without that same rework. -func isAllowedServiceHost(host, discoveryHost string) bool { - // DNS is case-insensitive and net/url doesn't normalise host case (it - // lowercases the scheme but not the host), so a discovery response with - // any uppercase in a legitimate hostname must still match here. - if strings.EqualFold(host, discoveryHost) { - return true - } +// DNS is case-insensitive and net/url doesn't normalise host case (it +// lowercases the scheme but not the host — verified), so a discovery +// response with any uppercase in an otherwise-legitimate hostname must +// still match here. +func hostOnAllowedRootDomain(host string) bool { host = strings.ToLower(host) for _, root := range allowedRootDomains { if host == root || strings.HasSuffix(host, "."+root) { @@ -111,9 +103,9 @@ func isAllowedServiceHost(host, discoveryHost string) bool { } // sanitizeServiceAPI returns rawAPI unchanged if its scheme is https and its -// host is allowed, or "" (treated the same as "service not present in the -// response") if not. -func sanitizeServiceAPI(ctx context.Context, serviceName, rawAPI, discoveryHost string) string { +// host is on an allowed root domain, or "" (treated the same as "service not +// present in the response") if not. +func sanitizeServiceAPI(ctx context.Context, serviceName, rawAPI string) string { if rawAPI == "" { return "" } @@ -123,15 +115,10 @@ func sanitizeServiceAPI(ctx context.Context, serviceName, rawAPI, discoveryHost return "" } if u.Scheme != "https" { - // Rejecting plain HTTP also closes the loopback-attacker shape of - // the isAllowedServiceHost "same host as discoveryHost" case: a - // same-host rogue endpoint (e.g. an attacker-controlled - // ARK_DISCOVERY_API pointing at 127.0.0.1) can't present a - // certificate this client's TLS verification will accept. klog.FromContext(ctx).Info("dropping non-HTTPS service discovery API URL", "service", serviceName, "scheme", u.Scheme) return "" } - if !isAllowedServiceHost(u.Hostname(), discoveryHost) { + if !hostOnAllowedRootDomain(u.Hostname()) { klog.FromContext(ctx).Info("dropping service discovery API URL outside the allowed CyberArk domains", "service", serviceName, "host", u.Hostname()) return "" } @@ -242,6 +229,18 @@ func (c *Client) DiscoverServices(ctx context.Context) (*Services, string, error return nil, "", fmt.Errorf("invalid base URL for service discovery: %w", err) } + // CP-26002: the discovery bootstrap call itself must be to an allowed + // CyberArk domain over HTTPS too — otherwise ARK_DISCOVERY_API write + // access (e.g. a tampered pod spec or Helm values) would let an attacker + // point trust-chain bootstrap at arbitrary infrastructure, which the + // sanitizeServiceAPI checks below wouldn't catch on their own: they used + // to also accept anything matching this same host, on the assumption + // that whatever ARK_DISCOVERY_API pointed at was already trusted as much + // as the discovery call itself. It no longer is assumed; it's checked. + if u.Scheme != "https" || !hostOnAllowedRootDomain(u.Hostname()) { + return nil, "", fmt.Errorf("service discovery base URL %q is not HTTPS on an allowed CyberArk domain; refusing to bootstrap trust from it", c.baseURL) + } + u.Path = path.Join(u.Path, "api/public/tenant-discovery") u.RawQuery = url.Values{"bySubdomain": []string{c.subdomain}}.Encode() @@ -298,9 +297,9 @@ func (c *Client) DiscoverServices(ctx context.Context) (*Services, string, error // against it. A dropped URL is treated exactly like one absent from the // response — see the required/optional distinction below. rawIdentityAPI := identityAPI - identityAPI = sanitizeServiceAPI(ctx, IdentityServiceName, identityAPI, u.Hostname()) - discoveryContextAPI = sanitizeServiceAPI(ctx, DiscoveryContextServiceName, discoveryContextAPI, u.Hostname()) - secretsManagerAPI = sanitizeServiceAPI(ctx, SecretsManagerServiceName, secretsManagerAPI, u.Hostname()) + identityAPI = sanitizeServiceAPI(ctx, IdentityServiceName, identityAPI) + discoveryContextAPI = sanitizeServiceAPI(ctx, DiscoveryContextServiceName, discoveryContextAPI) + secretsManagerAPI = sanitizeServiceAPI(ctx, SecretsManagerServiceName, secretsManagerAPI) // identityAPI is required unconditionally, unlike discoveryContextAPI and // secretsManagerAPI below: it's present and active for every healthy diff --git a/internal/cyberark/servicediscovery/mock.go b/internal/cyberark/servicediscovery/mock.go index b784d8a4..dd3fe69d 100644 --- a/internal/cyberark/servicediscovery/mock.go +++ b/internal/cyberark/servicediscovery/mock.go @@ -5,15 +5,20 @@ import ( "crypto/rand" "encoding/hex" "encoding/json" + "fmt" + "net" "net/http" "net/http/httptest" + "net/url" "strings" + "sync/atomic" "testing" "text/template" "k8s.io/client-go/transport" arkapi "github.com/jetstack/preflight/internal/cyberark/api" + cyberarktesting "github.com/jetstack/preflight/internal/cyberark/testing" "github.com/jetstack/preflight/pkg/version" _ "embed" @@ -37,6 +42,33 @@ type mockDiscoveryServer struct { successResponse string } +var fakeHostCounter atomic.Uint64 + +// launderIfLoopback rewrites rawURL to an allowlisted-domain-looking +// hostname and registers a dial redirect (via cyberarktesting.RegisterMockHost) +// to its real address, if rawURL's host is a loopback IP (a real httptest +// mock server address). Any other value — including deliberately-invalid +// test hosts like "attacker.example" — is returned unchanged, since those +// must still be rejected by the code under test, not laundered into passing. +func launderIfLoopback(rawURL string) string { + if rawURL == "" { + return rawURL + } + u, err := url.Parse(rawURL) + if err != nil { + return rawURL + } + ip := net.ParseIP(u.Hostname()) + if ip == nil || !ip.IsLoopback() { + return rawURL + } + fakeHost := fmt.Sprintf("mock-%d.integration-cyberark.cloud", fakeHostCounter.Add(1)) + cyberarktesting.RegisterMockHost(fakeHost, u.Host) + u.Host = fakeHost + u.Scheme = "https" + return u.String() +} + // MockDiscoveryServer starts a mocked CyberArk service discovery server and // returns an HTTP client with the CA certs needed to connect to it. // @@ -49,27 +81,55 @@ type mockDiscoveryServer struct { // supplied in `services`. // Other subdomains, can be used to trigger various failure responses. // +// Any of services' API fields that point at a real loopback mock server +// (rather than a fake CyberArk-domain-looking test hostname) is laundered — +// see launderIfLoopback — into a fake CyberArk-domain hostname, with a dial +// redirect registered via cyberarktesting.RegisterMockHost so any +// WrapMockTransport-wrapped client (not just this one) can still reach it. +// This keeps DiscoverServices' domain/HTTPS allowlist (which now also +// covers ARK_DISCOVERY_API itself, see CP-26002) from stripping out real +// dataupload/conjur/identity mock addresses that other packages' tests +// embed here. +// // The returned HTTP client has a transport which logs requests and responses // depending on log level of the logger supplied in the context. func MockDiscoveryServer(t testing.TB, services Services) *http.Client { + mds := &mockDiscoveryServer{t: t} + server := httptest.NewTLSServer(mds) + t.Cleanup(server.Close) + + httpClient := server.Client() + baseTransport := httpClient.Transport.(*http.Transport).Clone() + cyberarktesting.WrapMockTransport(baseTransport) + + discoveryFakeHost := fmt.Sprintf("mock-%d.integration-cyberark.cloud", fakeHostCounter.Add(1)) + cyberarktesting.RegisterMockHost(discoveryFakeHost, mustHostPort(t, server.URL)) + t.Setenv("ARK_DISCOVERY_API", "https://"+discoveryFakeHost) + + services.Identity.API = launderIfLoopback(services.Identity.API) + services.DiscoveryContext.API = launderIfLoopback(services.DiscoveryContext.API) + services.SecretsManager.API = launderIfLoopback(services.SecretsManager.API) + tmpl := template.Must(template.New("mockDiscoverySuccess").Parse(discoverySuccessTemplate)) buf := &bytes.Buffer{} - err := tmpl.Execute(buf, services) - if err != nil { + if err := tmpl.Execute(buf, services); err != nil { panic(err) } - mds := &mockDiscoveryServer{ - t: t, - successResponse: buf.String(), - } - server := httptest.NewTLSServer(mds) - t.Cleanup(server.Close) - t.Setenv("ARK_DISCOVERY_API", server.URL) - httpClient := server.Client() - httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext) + mds.successResponse = buf.String() + + httpClient.Transport = transport.NewDebuggingRoundTripper(baseTransport, transport.DebugByContext) return httpClient } +func mustHostPort(t testing.TB, rawURL string) string { + t.Helper() + u, err := url.Parse(rawURL) + if err != nil { + t.Fatalf("MockDiscoveryServer: invalid server URL %q: %v", rawURL, err) + } + return u.Host +} + func (mds *mockDiscoveryServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { mds.t.Log(r.Method, r.RequestURI) if r.Method != http.MethodGet { diff --git a/internal/cyberark/testing/mockdial.go b/internal/cyberark/testing/mockdial.go new file mode 100644 index 00000000..b202116b --- /dev/null +++ b/internal/cyberark/testing/mockdial.go @@ -0,0 +1,56 @@ +package testing + +import ( + "context" + "net" + "net/http" + "sync" +) + +// mockHostMu and mockHostAddr back a process-wide registry of fake hostname +// -> real host:port, shared across every package's Mock*Server helper. It +// has to be process-wide rather than scoped to one http.Client: tests freely +// reuse whichever mock's client is convenient to make a call against a +// *different* mock's server (e.g. keyfetch's test setup discards +// servicediscovery.MockDiscoveryServer's own client and instead reuses +// conjur.MockConjurExchangeServer's), so any mock client might end up being +// the one that has to dial any other mock's registered fake host. +var ( + mockHostMu sync.Mutex + mockHostAddr = map[string]string{} +) + +// RegisterMockHost tells every WrapMockTransport-wrapped client to redirect +// dials for fakeHost to realHostPort instead. Used by servicediscovery's +// mock to embed another package's mock server address in a discovery +// response under a CyberArk-domain-looking hostname, since +// servicediscovery.DiscoverServices now allowlists both ARK_DISCOVERY_API +// itself and the hosts it returns (CP-25960, CP-26002). +func RegisterMockHost(fakeHost, realHostPort string) { + mockHostMu.Lock() + defer mockHostMu.Unlock() + mockHostAddr[fakeHost] = realHostPort +} + +// WrapMockTransport installs a DialContext on transport that redirects any +// RegisterMockHost-registered fake host to its real address, and disables +// TLS hostname verification, since a fake host never matches the httptest +// server's actual certificate SAN. Every package's Mock*Server should call +// this on its returned client's transport, so that client can reach a fake +// host registered by any other mock, regardless of which mock's client a +// test ends up reusing for a given call. +func WrapMockTransport(transport *http.Transport) { + transport.TLSClientConfig = transport.TLSClientConfig.Clone() + transport.TLSClientConfig.InsecureSkipVerify = true + transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { + if host, _, err := net.SplitHostPort(addr); err == nil { + mockHostMu.Lock() + realAddr, ok := mockHostAddr[host] + mockHostMu.Unlock() + if ok { + addr = realAddr + } + } + return (&net.Dialer{}).DialContext(ctx, network, addr) + } +} From a5ff45c2ff11157b0836bf6db1fbd089a24c8ea0 Mon Sep 17 00:00:00 2001 From: rzisholz Date: Tue, 8 Sep 2026 14:02:55 +0300 Subject: [PATCH 2/6] CP-26094: warn-only tenant-subdomain check on discovery-derived hosts Raised by wallrj-cyberark on PR #829 (CP-25960): allowedRootDomains admits every tenant's host, not just the caller's own -- a tampered response can still redirect the SA-token POST to a different tenant's secrets_manager/ identity_administration host. sanitizeServiceAPI now logs (Info, not enforced) when a discovery-derived host's leading label doesn't match the caller's own subdomain, reusing the two-shape matching discoverycontext-regional-resources' token.py:197-212 (_is_host_subdomain_matching) uses for its own inbound host-binding check. Not enforcement yet: we only have live evidence of the dot shape for jetstack-secure's three actual services (identity/discoverycontext/ secrets_manager render as id/inventory/secretsmgr in every mock and the one live-verified tenant); the hyphen shape is only confirmed for a different service (discoverycontext's own GraphQL host, per token.py's docstring example). Failing closed on an unverified shape risks breaking real agents in a service-label combination we haven't observed. CP-26094 tracks coming back to decide on enforcement once this telemetry confirms the shape holds. --- .../cyberark/servicediscovery/discovery.go | 44 +++++++++++++++-- .../servicediscovery/discovery_test.go | 48 +++++++++++++++++++ 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/internal/cyberark/servicediscovery/discovery.go b/internal/cyberark/servicediscovery/discovery.go index a2deb0ae..dda7d621 100644 --- a/internal/cyberark/servicediscovery/discovery.go +++ b/internal/cyberark/servicediscovery/discovery.go @@ -102,10 +102,38 @@ func hostOnAllowedRootDomain(host string) bool { return false } +// hostLeadingLabelMatchesSubdomain reports whether host's leading label +// names subdomain, tolerating the two label shapes actually observed on +// CyberArk API Gateway hosts (mirrors discoverycontext-regional-resources' +// _is_host_subdomain_matching, token.py:197-212, used there to validate the +// inbound Host header against a JWT's subdomain claim): +// - {subdomain}.{service}.{domain} e.g. eh1c6a8z1wf8hi.inventory.integration-cyberark.cloud +// - {subdomain}-{service}.{domain} e.g. disco4asaf-discoverycontext.integration-cyberark.cloud +// +// Unlike token.py, this doesn't check against one fixed service-name suffix +// (identity/discoverycontext/secrets_manager each render under a different, +// undocumented service label — "id"/"inventory"/"secretsmgr" observed live, +// not the JSON service_name values) — it accepts any hyphen suffix, which is +// looser than token.py's exact match but appropriate for a warn-only check. +func hostLeadingLabelMatchesSubdomain(host, subdomain string) bool { + if subdomain == "" { + return true + } + label, _, _ := strings.Cut(host, ".") + return label == subdomain || strings.HasPrefix(label, subdomain+"-") +} + // sanitizeServiceAPI returns rawAPI unchanged if its scheme is https and its // host is on an allowed root domain, or "" (treated the same as "service not // present in the response") if not. -func sanitizeServiceAPI(ctx context.Context, serviceName, rawAPI string) string { +// +// subdomain is used only for a warn-only check, not enforcement: see +// hostLeadingLabelMatchesSubdomain's doc comment for why — we don't yet have +// live evidence covering all three services' host shapes across every +// environment, and fail-closed on an unverified assumption risks a real +// outage. CP-26010 tracks turning this into enforcement once telemetry +// confirms it holds. +func sanitizeServiceAPI(ctx context.Context, serviceName, rawAPI, subdomain string) string { if rawAPI == "" { return "" } @@ -122,6 +150,14 @@ func sanitizeServiceAPI(ctx context.Context, serviceName, rawAPI string) string klog.FromContext(ctx).Info("dropping service discovery API URL outside the allowed CyberArk domains", "service", serviceName, "host", u.Hostname()) return "" } + if !hostLeadingLabelMatchesSubdomain(u.Hostname(), subdomain) { + // Not dropped -- see the function doc comment. A tampered response + // could still redirect within the same allowed root domain to a + // different tenant's host; this is the visibility half of closing + // that gap, not the enforcement half. + klog.FromContext(ctx).Info("service discovery API URL's host doesn't look like it belongs to this tenant's subdomain", + "service", serviceName, "host", u.Hostname(), "subdomain", subdomain) + } return rawAPI } @@ -297,9 +333,9 @@ func (c *Client) DiscoverServices(ctx context.Context) (*Services, string, error // against it. A dropped URL is treated exactly like one absent from the // response — see the required/optional distinction below. rawIdentityAPI := identityAPI - identityAPI = sanitizeServiceAPI(ctx, IdentityServiceName, identityAPI) - discoveryContextAPI = sanitizeServiceAPI(ctx, DiscoveryContextServiceName, discoveryContextAPI) - secretsManagerAPI = sanitizeServiceAPI(ctx, SecretsManagerServiceName, secretsManagerAPI) + identityAPI = sanitizeServiceAPI(ctx, IdentityServiceName, identityAPI, c.subdomain) + discoveryContextAPI = sanitizeServiceAPI(ctx, DiscoveryContextServiceName, discoveryContextAPI, c.subdomain) + secretsManagerAPI = sanitizeServiceAPI(ctx, SecretsManagerServiceName, secretsManagerAPI, c.subdomain) // identityAPI is required unconditionally, unlike discoveryContextAPI and // secretsManagerAPI below: it's present and active for every healthy diff --git a/internal/cyberark/servicediscovery/discovery_test.go b/internal/cyberark/servicediscovery/discovery_test.go index 61fe7787..3cc94911 100644 --- a/internal/cyberark/servicediscovery/discovery_test.go +++ b/internal/cyberark/servicediscovery/discovery_test.go @@ -12,6 +12,26 @@ import ( _ "k8s.io/klog/v2/ktesting/init" ) +func Test_hostLeadingLabelMatchesSubdomain(t *testing.T) { + tests := map[string]struct { + host, subdomain string + want bool + }{ + "dot shape, matches": {"eh1c6a8z1wf8hi.inventory.integration-cyberark.cloud", "eh1c6a8z1wf8hi", true}, + "dot shape, different tenant": {"eh1c6a8z1wf8hi.inventory.integration-cyberark.cloud", "someone-else", false}, + "hyphen shape, matches": {"disco4asaf-discoverycontext.integration-cyberark.cloud", "disco4asaf", true}, + "hyphen shape, different tenant": { + "disco4asaf-discoverycontext.integration-cyberark.cloud", "someone-else", false, + }, + "empty subdomain never flags anything": {"anything.at.all", "", true}, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tt.want, hostLeadingLabelMatchesSubdomain(tt.host, tt.subdomain)) + }) + } +} + func Test_DiscoverIdentityAPIURL(t *testing.T) { tests := map[string]struct { subdomain string @@ -193,6 +213,34 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) { assert.Equal(t, "https://AJP5871.ID.Integration-CyberArk.Cloud", services.Identity.API) }) + t.Run("a host on the allowed domain but a different tenant's subdomain is warned about, not dropped", func(t *testing.T) { + // Deliberately not enforcement -- see sanitizeServiceAPI's doc + // comment for why. This also matches every other test in this file: + // none of the mock*APIURL constants' leading labels are + // MockDiscoverySubdomain ("tlskp-test"), and none of those tests + // fail, which already exercises this path -- this test just makes + // the "not dropped" property explicit and named. + logger := ktesting.NewLogger(t, ktesting.DefaultConfig) + ctx := klog.NewContext(t.Context(), logger) + + httpClient := MockDiscoveryServer(t, Services{ + Identity: ServiceEndpoint{ + API: "https://some-other-tenant.id.integration-cyberark.cloud", + }, + DiscoveryContext: ServiceEndpoint{ + API: mockDiscoveryContextAPIURL, + }, + SecretsManager: ServiceEndpoint{ + API: mockSecretsManagerAPIURL, + }, + }) + + client := New(httpClient, MockDiscoverySubdomain) + services, _, err := client.DiscoverServices(ctx) + require.NoError(t, err) + assert.Equal(t, "https://some-other-tenant.id.integration-cyberark.cloud", services.Identity.API) + }) + for name, testSpec := range tests { t.Run(name, func(t *testing.T) { logger := ktesting.NewLogger(t, ktesting.DefaultConfig) From ed9c325655a902a67a597533f8894b1d45d65ba0 Mon Sep 17 00:00:00 2001 From: rzisholz Date: Tue, 8 Sep 2026 14:56:53 +0300 Subject: [PATCH 3/6] CP-26094: correct threat-model overclaim, assert V(2) log content Two more points from wallrj-cyberark's review round on #829/#830. allowedRootDomains' doc comment claimed to close "an SSRF-shaped hole if the response is ever tampered with", implying defence against a network attacker. Verified against the actual threat model: the discovery call is already HTTPS against system roots, so a plain network attacker can't alter the response, and one who could defeat that TLS session could equally intercept whichever host the allowlist permits instead -- the allowlist buys nothing there. What it actually constrains is our own discovery service returning a bad host (compromised, buggy, or otherwise misbehaving) inside an intact TLS session. Reworded to say that precisely, and noted the tenant-scoping gap (cyberark.cloud admits every tenant's host) explicitly rather than leaving it implicit. TestAuthenticateRequest_ExchangeErrorOmitsConjurResponseBody's comment claimed "ktesting has no easy log-buffer assertion in this codebase" -- wrong, pkg/agent/config_test.go:1040's recordLogs helper already does this and six tests use it. Added the same assertion here: the test now proves the Conjur response body is still present in the V(2) log line, not just absent from the returned error, so a future change deleting the klog line entirely would fail this test too. --- internal/cyberark/conjur/conjur_test.go | 20 +++++++++++--- .../cyberark/servicediscovery/discovery.go | 27 ++++++++++++++----- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/internal/cyberark/conjur/conjur_test.go b/internal/cyberark/conjur/conjur_test.go index 8069a4c4..21725d6f 100644 --- a/internal/cyberark/conjur/conjur_test.go +++ b/internal/cyberark/conjur/conjur_test.go @@ -10,6 +10,10 @@ import ( "time" "github.com/stretchr/testify/require" + "k8s.io/klog/v2" + "k8s.io/klog/v2/ktesting" + + _ "k8s.io/klog/v2/ktesting/init" ) type staticSource struct{ tok string } @@ -217,17 +221,25 @@ func TestInvalidate_ForcesReexchange(t *testing.T) { // (pkg/agent/run.go's PushingErr notification), readable by anyone with `get // events` in the namespace — so Conjur's response body (which can contain // policy structure, service IDs and host identities) must not appear in it. -// The body is still logged at V(2) for an operator to go find, but that's -// exercised via the "authn-jwt exchange rejected" log line, not asserted -// here — ktesting has no easy log-buffer assertion in this codebase. +// The body is still logged at V(2) for an operator to go find — asserted +// below via the log buffer, not just by omission from the returned error, so +// a future change that deletes the klog line entirely (removing the +// operator's only way to see Conjur's response) would fail this test too. func TestAuthenticateRequest_ExchangeErrorOmitsConjurResponseBody(t *testing.T) { srv, httpClient := MockConjurExchangeServerStatusBody(t, http.StatusUnauthorized, []byte(`{"error":{"message":"CONJ00001E Invalid JWT token"}}`)) defer srv.Close() c := New(httpClient, srv.URL, "dev-cluster", "conjur", staticSource{tok: "the-jwt"}) - req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://example.com/x", nil) + + logger := ktesting.NewLogger(t, ktesting.NewConfig(ktesting.BufferLogs(true), ktesting.Verbosity(2))) + buf := logger.GetSink().(ktesting.Underlier).GetBuffer() + ctx := klog.NewContext(t.Context(), logger) + req, _ := http.NewRequestWithContext(ctx, http.MethodGet, "https://example.com/x", nil) + _, err := c.AuthenticateRequest(req) require.Error(t, err) require.NotContains(t, err.Error(), "CONJ00001E Invalid JWT token") require.Contains(t, err.Error(), "authn-jwt exchange rejected (401)") + require.Contains(t, buf.String(), "authn-jwt exchange rejected") + require.Contains(t, buf.String(), "CONJ00001E Invalid JWT token") } diff --git a/internal/cyberark/servicediscovery/discovery.go b/internal/cyberark/servicediscovery/discovery.go index dda7d621..7b1cedbb 100644 --- a/internal/cyberark/servicediscovery/discovery.go +++ b/internal/cyberark/servicediscovery/discovery.go @@ -46,14 +46,29 @@ const ( // allowedRootDomains are the only root domains trusted for both (a) the // discovery bootstrap call itself — c.baseURL, which is ARK_DISCOVERY_API if // set — and (b) the identity/discoverycontext/secrets_manager hosts that -// call's response points us at. Without (b), mainActiveAPI's ep.API is +// call's response points us at. +// +// This is defence-in-depth against our own service, not a fix for a +// compromised transport. The discovery call is already HTTPS against system +// roots, so a plain network attacker can't alter what comes back, and an +// attacker who *can* defeat that TLS session could equally intercept +// whichever host this allowlist would have permitted instead — the +// allowlist buys nothing against either. What it does constrain is (b): if +// the discovery service itself is compromised, buggy, or returns a host +// influenced by something else, mainActiveAPI's ep.API would otherwise be // trusted verbatim from the response body and handed straight to the // Conjur/Identity clients, which then POST the agent's SA token (or -// username/password) to it. Without (a), ARK_DISCOVERY_API write access -// (e.g. a tampered pod spec or Helm values) bootstraps the entire trust -// chain from arbitrary infrastructure regardless of (b) — see CP-26002 / -// CP-23593. Mirrors the per-env ROOT_DOMAIN allowlist already enforced on -// the discoverycontext-regional-resources side (token.py, for the JWT `iss` +// username/password) to it — here, and only here, does the allowlist bind. +// (a) is the same control applied to the bootstrap call's own base URL, so a +// tampered ARK_DISCOVERY_API (pod spec/Helm values) can't redirect (b)'s +// trust anchor off-domain either — see CP-26002 / CP-23593. Note +// `cyberark.cloud` still admits every tenant's host, so this doesn't stop a +// misbehaving discovery service naming a different tenant's host; that gap +// is tracked separately (hostLeadingLabelMatchesSubdomain, CP-26094, +// warn-only pending more evidence). +// +// Mirrors the per-env ROOT_DOMAIN allowlist already enforced on the +// discoverycontext-regional-resources side (token.py, for the JWT `iss` // host) — copied by value here since these domains rarely change and the // agent has no access to that env-keyed map. // From e8b6cd1261d376ddafc4246088f5730ed5fda410 Mon Sep 17 00:00:00 2001 From: rzisholz Date: Tue, 8 Sep 2026 15:11:23 +0300 Subject: [PATCH 4/6] Fix review findings on the mock-transport rework, trim internal jargon from comments Test infrastructure fixes: - WrapMockTransport panicked on a nil TLSClientConfig; guard it. - MockDiscoveryServer had an unsynchronised write: successResponse was assigned after the server started accepting connections. Build it first. - launderIfLoopback forced the laundered URL's scheme to https even when the original was plain HTTP, hiding a real scheme mismatch behind a confusing TLS handshake error instead of the intended rejection. Keep the original scheme. Behaviour fixes: - hostOnAllowedRootDomain didn't strip a trailing dot, so a legal absolute FQDN (e.g. "host.cyberark.cloud.") was wrongly rejected. - No test exercised the base-URL check directly -- every existing test reached it through a mock that already sets an allowed value. Added one that sets a disallowed value directly. Also removed internal ticket references and jargon from comments across the files touched by this branch -- this is a public repo. --- .../cyberark/servicediscovery/discovery.go | 84 +++++-------------- .../servicediscovery/discovery_test.go | 41 +++++++++ internal/cyberark/servicediscovery/mock.go | 24 +++--- internal/cyberark/testing/mockdial.go | 7 +- internal/envelope/keyfetch/client.go | 9 +- 5 files changed, 81 insertions(+), 84 deletions(-) diff --git a/internal/cyberark/servicediscovery/discovery.go b/internal/cyberark/servicediscovery/discovery.go index 7b1cedbb..4bac0b5a 100644 --- a/internal/cyberark/servicediscovery/discovery.go +++ b/internal/cyberark/servicediscovery/discovery.go @@ -46,37 +46,14 @@ const ( // allowedRootDomains are the only root domains trusted for both (a) the // discovery bootstrap call itself — c.baseURL, which is ARK_DISCOVERY_API if // set — and (b) the identity/discoverycontext/secrets_manager hosts that -// call's response points us at. -// -// This is defence-in-depth against our own service, not a fix for a -// compromised transport. The discovery call is already HTTPS against system -// roots, so a plain network attacker can't alter what comes back, and an -// attacker who *can* defeat that TLS session could equally intercept -// whichever host this allowlist would have permitted instead — the -// allowlist buys nothing against either. What it does constrain is (b): if -// the discovery service itself is compromised, buggy, or returns a host -// influenced by something else, mainActiveAPI's ep.API would otherwise be -// trusted verbatim from the response body and handed straight to the -// Conjur/Identity clients, which then POST the agent's SA token (or -// username/password) to it — here, and only here, does the allowlist bind. -// (a) is the same control applied to the bootstrap call's own base URL, so a -// tampered ARK_DISCOVERY_API (pod spec/Helm values) can't redirect (b)'s -// trust anchor off-domain either — see CP-26002 / CP-23593. Note -// `cyberark.cloud` still admits every tenant's host, so this doesn't stop a -// misbehaving discovery service naming a different tenant's host; that gap -// is tracked separately (hostLeadingLabelMatchesSubdomain, CP-26094, -// warn-only pending more evidence). -// -// Mirrors the per-env ROOT_DOMAIN allowlist already enforced on the -// discoverycontext-regional-resources side (token.py, for the JWT `iss` -// host) — copied by value here since these domains rarely change and the -// agent has no access to that env-keyed map. -// -// Source of truth is the `everest_env_utils` package's ROOT_DOMAIN map -// (published to Artifactory as everest_env_utils_cyberark, v2.0.117 as of -// 2026-09-03), not the Lambda's local commercial-only clone — that clone -// omits the GOV_* environments entirely, which would have made this -// allowlist silently break every gov-cloud tenant's agent. +// call's response points us at. Without this, a compromised or misbehaving +// discovery service could point (b) at an arbitrary host and this client +// would POST the agent's SA token (or username/password) straight to it. +// This doesn't defend against a network-level attacker capable of +// tampering with an HTTPS response in transit — that's a separate problem — +// it constrains what a bad discovery response itself can point us at. Note +// it doesn't distinguish between tenants either: any host on these domains +// is accepted regardless of which tenant it belongs to. var allowedRootDomains = []string{ "cyberark.cloud", "cyberark-everest-dev.com", @@ -101,14 +78,10 @@ var allowedRootDomains = []string{ } // hostOnAllowedRootDomain reports whether host is, or is a subdomain of, one -// of allowedRootDomains. -// -// DNS is case-insensitive and net/url doesn't normalise host case (it -// lowercases the scheme but not the host — verified), so a discovery -// response with any uppercase in an otherwise-legitimate hostname must -// still match here. +// of allowedRootDomains. Hostnames are case-insensitive and may carry a +// trailing dot (a legal absolute FQDN), so normalise before comparing. func hostOnAllowedRootDomain(host string) bool { - host = strings.ToLower(host) + host = strings.ToLower(strings.TrimSuffix(host, ".")) for _, root := range allowedRootDomains { if host == root || strings.HasSuffix(host, "."+root) { return true @@ -118,18 +91,9 @@ func hostOnAllowedRootDomain(host string) bool { } // hostLeadingLabelMatchesSubdomain reports whether host's leading label -// names subdomain, tolerating the two label shapes actually observed on -// CyberArk API Gateway hosts (mirrors discoverycontext-regional-resources' -// _is_host_subdomain_matching, token.py:197-212, used there to validate the -// inbound Host header against a JWT's subdomain claim): -// - {subdomain}.{service}.{domain} e.g. eh1c6a8z1wf8hi.inventory.integration-cyberark.cloud -// - {subdomain}-{service}.{domain} e.g. disco4asaf-discoverycontext.integration-cyberark.cloud -// -// Unlike token.py, this doesn't check against one fixed service-name suffix -// (identity/discoverycontext/secrets_manager each render under a different, -// undocumented service label — "id"/"inventory"/"secretsmgr" observed live, -// not the JSON service_name values) — it accepts any hyphen suffix, which is -// looser than token.py's exact match but appropriate for a warn-only check. +// names subdomain, tolerating the two label shapes observed in practice: +// - {subdomain}.{service}.{domain} +// - {subdomain}-{service}.{domain} func hostLeadingLabelMatchesSubdomain(host, subdomain string) bool { if subdomain == "" { return true @@ -142,12 +106,9 @@ func hostLeadingLabelMatchesSubdomain(host, subdomain string) bool { // host is on an allowed root domain, or "" (treated the same as "service not // present in the response") if not. // -// subdomain is used only for a warn-only check, not enforcement: see -// hostLeadingLabelMatchesSubdomain's doc comment for why — we don't yet have -// live evidence covering all three services' host shapes across every -// environment, and fail-closed on an unverified assumption risks a real -// outage. CP-26010 tracks turning this into enforcement once telemetry -// confirms it holds. +// subdomain is used only for a warn-only check, not enforcement — we don't +// yet have enough evidence to fail closed on it without risking breaking +// real agents. func sanitizeServiceAPI(ctx context.Context, serviceName, rawAPI, subdomain string) string { if rawAPI == "" { return "" @@ -280,14 +241,9 @@ func (c *Client) DiscoverServices(ctx context.Context) (*Services, string, error return nil, "", fmt.Errorf("invalid base URL for service discovery: %w", err) } - // CP-26002: the discovery bootstrap call itself must be to an allowed - // CyberArk domain over HTTPS too — otherwise ARK_DISCOVERY_API write - // access (e.g. a tampered pod spec or Helm values) would let an attacker - // point trust-chain bootstrap at arbitrary infrastructure, which the - // sanitizeServiceAPI checks below wouldn't catch on their own: they used - // to also accept anything matching this same host, on the assumption - // that whatever ARK_DISCOVERY_API pointed at was already trusted as much - // as the discovery call itself. It no longer is assumed; it's checked. + // The bootstrap call itself must be to an allowed domain over HTTPS too, + // not just the hosts it later points us at — otherwise ARK_DISCOVERY_API + // alone could bootstrap trust from arbitrary infrastructure. if u.Scheme != "https" || !hostOnAllowedRootDomain(u.Hostname()) { return nil, "", fmt.Errorf("service discovery base URL %q is not HTTPS on an allowed CyberArk domain; refusing to bootstrap trust from it", c.baseURL) } diff --git a/internal/cyberark/servicediscovery/discovery_test.go b/internal/cyberark/servicediscovery/discovery_test.go index 3cc94911..882f53e3 100644 --- a/internal/cyberark/servicediscovery/discovery_test.go +++ b/internal/cyberark/servicediscovery/discovery_test.go @@ -2,6 +2,7 @@ package servicediscovery import ( "fmt" + "net/http" "testing" "github.com/stretchr/testify/assert" @@ -32,6 +33,46 @@ func Test_hostLeadingLabelMatchesSubdomain(t *testing.T) { } } +func Test_hostOnAllowedRootDomain(t *testing.T) { + tests := map[string]struct { + host string + want bool + }{ + "exact match": {"cyberark.cloud", true}, + "subdomain": {"id.cyberark.cloud", true}, + "uppercase": {"ID.CyberArk.Cloud", true}, + "trailing dot": {"id.cyberark.cloud.", true}, + "unrelated domain": {"attacker.example", false}, + "looks like a suffix only": {"notcyberark.cloud", false}, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tt.want, hostOnAllowedRootDomain(tt.host)) + }) + } +} + +func Test_DiscoverServices_RejectsDisallowedBaseURL(t *testing.T) { + tests := map[string]string{ + "plain HTTP": "http://platform-discovery.cyberark.cloud/", + "disallowed domain": "https://attacker.example/", + "host with no scheme": "platform-discovery.cyberark.cloud", + } + for name, baseURL := range tests { + t.Run(name, func(t *testing.T) { + t.Setenv("ARK_DISCOVERY_API", baseURL) + + logger := ktesting.NewLogger(t, ktesting.DefaultConfig) + ctx := klog.NewContext(t.Context(), logger) + + client := New(&http.Client{}, MockDiscoverySubdomain) + services, _, err := client.DiscoverServices(ctx) + require.Error(t, err) + assert.Nil(t, services) + }) + } +} + func Test_DiscoverIdentityAPIURL(t *testing.T) { tests := map[string]struct { subdomain string diff --git a/internal/cyberark/servicediscovery/mock.go b/internal/cyberark/servicediscovery/mock.go index dd3fe69d..9bc06bc5 100644 --- a/internal/cyberark/servicediscovery/mock.go +++ b/internal/cyberark/servicediscovery/mock.go @@ -65,7 +65,6 @@ func launderIfLoopback(rawURL string) string { fakeHost := fmt.Sprintf("mock-%d.integration-cyberark.cloud", fakeHostCounter.Add(1)) cyberarktesting.RegisterMockHost(fakeHost, u.Host) u.Host = fakeHost - u.Scheme = "https" return u.String() } @@ -94,7 +93,17 @@ func launderIfLoopback(rawURL string) string { // The returned HTTP client has a transport which logs requests and responses // depending on log level of the logger supplied in the context. func MockDiscoveryServer(t testing.TB, services Services) *http.Client { - mds := &mockDiscoveryServer{t: t} + services.Identity.API = launderIfLoopback(services.Identity.API) + services.DiscoveryContext.API = launderIfLoopback(services.DiscoveryContext.API) + services.SecretsManager.API = launderIfLoopback(services.SecretsManager.API) + + tmpl := template.Must(template.New("mockDiscoverySuccess").Parse(discoverySuccessTemplate)) + buf := &bytes.Buffer{} + if err := tmpl.Execute(buf, services); err != nil { + panic(err) + } + + mds := &mockDiscoveryServer{t: t, successResponse: buf.String()} server := httptest.NewTLSServer(mds) t.Cleanup(server.Close) @@ -106,17 +115,6 @@ func MockDiscoveryServer(t testing.TB, services Services) *http.Client { cyberarktesting.RegisterMockHost(discoveryFakeHost, mustHostPort(t, server.URL)) t.Setenv("ARK_DISCOVERY_API", "https://"+discoveryFakeHost) - services.Identity.API = launderIfLoopback(services.Identity.API) - services.DiscoveryContext.API = launderIfLoopback(services.DiscoveryContext.API) - services.SecretsManager.API = launderIfLoopback(services.SecretsManager.API) - - tmpl := template.Must(template.New("mockDiscoverySuccess").Parse(discoverySuccessTemplate)) - buf := &bytes.Buffer{} - if err := tmpl.Execute(buf, services); err != nil { - panic(err) - } - mds.successResponse = buf.String() - httpClient.Transport = transport.NewDebuggingRoundTripper(baseTransport, transport.DebugByContext) return httpClient } diff --git a/internal/cyberark/testing/mockdial.go b/internal/cyberark/testing/mockdial.go index b202116b..6ea31b2f 100644 --- a/internal/cyberark/testing/mockdial.go +++ b/internal/cyberark/testing/mockdial.go @@ -2,6 +2,7 @@ package testing import ( "context" + "crypto/tls" "net" "net/http" "sync" @@ -40,7 +41,11 @@ func RegisterMockHost(fakeHost, realHostPort string) { // host registered by any other mock, regardless of which mock's client a // test ends up reusing for a given call. func WrapMockTransport(transport *http.Transport) { - transport.TLSClientConfig = transport.TLSClientConfig.Clone() + if transport.TLSClientConfig == nil { + transport.TLSClientConfig = &tls.Config{} + } else { + transport.TLSClientConfig = transport.TLSClientConfig.Clone() + } transport.TLSClientConfig.InsecureSkipVerify = true transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { if host, _, err := net.SplitHostPort(addr); err == nil { diff --git a/internal/envelope/keyfetch/client.go b/internal/envelope/keyfetch/client.go index cbbadeb1..22aa77f0 100644 --- a/internal/envelope/keyfetch/client.go +++ b/internal/envelope/keyfetch/client.go @@ -154,12 +154,9 @@ func (c *Client) FetchKey(ctx context.Context) (PublicKey, error) { defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - // The response body isn't included in the returned error — same leak - // class as CP-25964 (conjur.go's authn-jwt exchange error), just - // against the discoverycontext host instead. This error doesn't - // currently reach a Pod Event (only postData failures do, via - // eventf), but keep the body out of it so that stays true if the - // call sites ever change. + // Body logged, not returned — it can contain server-side details we + // don't want surfacing in a Kubernetes Event if this error ever + // reaches one. body, _ := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) logger.V(2).Info("unexpected status code fetching JWKS", "statusCode", resp.StatusCode, "endpoint", endpoint, "body", string(body)) return PublicKey{}, fmt.Errorf("unexpected status code %d from %s", resp.StatusCode, endpoint) From 7a20418e34281268da5be9d75dc8fb9e8822302d Mon Sep 17 00:00:00 2001 From: rzisholz Date: Wed, 9 Sep 2026 12:12:06 +0300 Subject: [PATCH 5/6] Stop echoing untrusted response content into errors that can reach an Event Two more places with the same leak class as the Conjur error-body fix earlier in this stack: an error built from unbounded or untrusted input, returned all the way up to where it can land on a Kubernetes Event. - discovery.go: the identity-endpoint-rejected error embedded the raw discovery response value verbatim. That value is untrusted, unbounded input from the discovery service. It's still visible to an operator via the existing Info log line (which logs the parsed hostname, not the raw string); the returned error no longer repeats it. - dataupload.go: both non-2xx branches returned the response body (bounded to 500 bytes, but still unvetted) directly in the error. Now logged at V(2) instead, matching the pattern already used for the Conjur and JWKS-fetch error paths. Added a test proving the body is still visible in the log, not just absent from the error. --- internal/cyberark/dataupload/dataupload.go | 13 ++++------ .../cyberark/dataupload/dataupload_test.go | 26 +++++++++++++++++-- .../cyberark/servicediscovery/discovery.go | 10 ++++--- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/internal/cyberark/dataupload/dataupload.go b/internal/cyberark/dataupload/dataupload.go index b3043c3b..71bcefee 100644 --- a/internal/cyberark/dataupload/dataupload.go +++ b/internal/cyberark/dataupload/dataupload.go @@ -13,6 +13,7 @@ import ( "net/url" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/klog/v2" arkapi "github.com/jetstack/preflight/internal/cyberark/api" "github.com/jetstack/preflight/internal/cyberark/identity" @@ -171,10 +172,8 @@ func (c *CyberArkClient) PutSnapshot(ctx context.Context, snapshot Snapshot) err if code := res.StatusCode; code < 200 || code >= 300 { body, _ := io.ReadAll(io.LimitReader(res.Body, 500)) - if len(body) == 0 { - body = []byte(``) - } - return fmt.Errorf("received response with status code %d: %s", code, bytes.TrimSpace(body)) + klog.FromContext(ctx).V(2).Info("unexpected status code uploading snapshot", "statusCode", code, "body", string(bytes.TrimSpace(body))) + return fmt.Errorf("received response with status code %d", code) } return nil @@ -245,10 +244,8 @@ func (c *CyberArkClient) retrievePresignedUploadURL(ctx context.Context, checksu if code := res.StatusCode; code < 200 || code >= 300 { body, _ := io.ReadAll(io.LimitReader(res.Body, 500)) - if len(body) == 0 { - body = []byte(``) - } - return "", "", fmt.Errorf("received response with status code %d: %s", code, bytes.TrimSpace(body)) + klog.FromContext(ctx).V(2).Info("unexpected status code retrieving upload URL", "statusCode", code, "body", string(bytes.TrimSpace(body))) + return "", "", fmt.Errorf("received response with status code %d", code) } response := struct { diff --git a/internal/cyberark/dataupload/dataupload_test.go b/internal/cyberark/dataupload/dataupload_test.go index d78c4bf3..5f2b0697 100644 --- a/internal/cyberark/dataupload/dataupload_test.go +++ b/internal/cyberark/dataupload/dataupload_test.go @@ -63,7 +63,8 @@ func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) { }, authenticate: setToken("fail-token"), requireFn: func(t *testing.T, err error) { - require.ErrorContains(t, err, "while retrieving snapshot upload URL: received response with status code 500: should authenticate using the correct bearer token") + require.ErrorContains(t, err, "while retrieving snapshot upload URL: received response with status code 500") + require.NotContains(t, err.Error(), "should authenticate using the correct bearer token") }, }, { @@ -85,7 +86,8 @@ func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) { }, authenticate: setToken("success-token"), requireFn: func(t *testing.T, err error) { - require.ErrorContains(t, err, "while retrieving snapshot upload URL: received response with status code 500: mock error") + require.ErrorContains(t, err, "while retrieving snapshot upload URL: received response with status code 500") + require.NotContains(t, err.Error(), "mock error") }, }, } @@ -104,3 +106,23 @@ func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) { }) } } + +// TestCyberArkClient_PutSnapshot_LogsResponseBodyOnError proves the response +// body dropped from the returned error (see the test above) is still visible +// to an operator via the log, not just absent from the error. +func TestCyberArkClient_PutSnapshot_LogsResponseBodyOnError(t *testing.T) { + logger := ktesting.NewLogger(t, ktesting.NewConfig(ktesting.BufferLogs(true), ktesting.Verbosity(2))) + buf := logger.GetSink().(ktesting.Underlier).GetBuffer() + ctx := klog.NewContext(t.Context(), logger) + + datauploadAPIBaseURL, httpClient := dataupload.MockDataUploadServer(t) + authenticate := func(req *http.Request) (string, error) { + req.Header.Set("Authorization", "Bearer fail-token") + return "foo@example.com", nil + } + cyberArkClient := dataupload.New(httpClient, datauploadAPIBaseURL, "test-tenant-uuid", authenticate) + + err := cyberArkClient.PutSnapshot(ctx, dataupload.Snapshot{ClusterID: "test", AgentVersion: "test-version"}) + require.Error(t, err) + require.Contains(t, buf.String(), "should authenticate using the correct bearer token") +} diff --git a/internal/cyberark/servicediscovery/discovery.go b/internal/cyberark/servicediscovery/discovery.go index 4bac0b5a..db5479d0 100644 --- a/internal/cyberark/servicediscovery/discovery.go +++ b/internal/cyberark/servicediscovery/discovery.go @@ -319,10 +319,12 @@ func (c *Client) DiscoverServices(ctx context.Context) (*Services, string, error } // The response did name an identity_administration endpoint, but its // host isn't on our allowlist — a distinct, more actionable failure - // than "suspended tenant" (see sanitizeServiceAPI's Info log for - // which host was rejected and why). - return nil, "", fmt.Errorf("%s endpoint %q is not on an allowed CyberArk domain over HTTPS; refusing to use it", - IdentityServiceName, rawIdentityAPI) + // than "suspended tenant". The rejected value itself isn't embedded + // here (see sanitizeServiceAPI's Info log for that) since this error + // can reach a Kubernetes Event, and the value is untrusted, unbounded + // input from the discovery response. + return nil, "", fmt.Errorf("%s endpoint is not on an allowed CyberArk domain over HTTPS; refusing to use it "+ + "(see the agent's logs for the rejected value)", IdentityServiceName) } // discoveryContextAPI and secretsManagerAPI are deliberately not required // here, unlike identityAPI above: not every caller needs both, and From 6ad5a78a7cdb0ee98eaf59b15a737248372e3a00 Mon Sep 17 00:00:00 2001 From: rzisholz Date: Wed, 9 Sep 2026 12:35:11 +0300 Subject: [PATCH 6/6] Address review: leak in base-URL error, unmatchable warning, untested guard Three fixes for issues that blocked the previous round, plus the follow-ups raised alongside them. Blockers: - The base-URL rejection error printed the URL verbatim, and that error reaches a Pod Event. ARK_DISCOVERY_API can carry credentials, so this reopened exactly the leak class the rest of this work closes. The error now names only scheme and host. - The tenant-subdomain warning could never match for identity_administration: that host is keyed on the Identity tenant's own identifier, not the platform subdomain, so it logged for every healthy tenant on every uncached lookup. Excluded, with the reason recorded; enforcing it later would otherwise have failed every tenant closed. Also folds case, which the rest of the file already did. - The base-URL test passed with the guard deleted: it only asserted that some error occurred, and each case errored for an unrelated reason -- one by making a real request to the internet. It now asserts the error and supplies a transport that fails the test if any request is attempted. Verified by deleting the guard and watching it fail. Also: - Replaced the fake-host registry with a loopback exemption on the allowlist itself, which is the reviewer's suggestion and their design. Deletes mockdial.go and removes InsecureSkipVerify and the dial rewriting from the build entirely; the three unrelated mocks go back to a plain srv.Client(). - servicediscovery.New now validates the base URL and returns an error, so a bad ARK_DISCOVERY_API is a startup configuration failure rather than a push failure that retries for ten minutes and crash-loops. The runtime check stays as defence in depth. - Refuse redirects on the Conjur exchange. It POSTs the agent's token as a form field, and Go strips Authorization across a host change but never strips bodies, so a 3xx could have moved that token to a host the allowlist never saw. - Restored the substance of the allowlist provenance note, without the internal detail, and corrected three comments that still described the escape hatch removed earlier in this stack. --- internal/cyberark/client_test.go | 12 +-- internal/cyberark/conjur/conjur.go | 11 ++- internal/cyberark/conjur/mock.go | 17 +--- internal/cyberark/dataupload/mock.go | 6 -- .../identity/cmd/testidentity/main.go | 5 +- internal/cyberark/identity/identity_test.go | 4 +- internal/cyberark/identity/mock.go | 6 -- .../cyberark/servicediscovery/discovery.go | 80 +++++++++++++++---- .../servicediscovery/discovery_test.go | 72 ++++++++++++----- internal/cyberark/servicediscovery/mock.go | 68 ++-------------- internal/cyberark/testing/mockdial.go | 61 -------------- internal/envelope/keyfetch/client_test.go | 20 ++--- pkg/client/client_cyberark.go | 7 +- pkg/testutil/envtest.go | 10 +-- 14 files changed, 173 insertions(+), 206 deletions(-) delete mode 100644 internal/cyberark/testing/mockdial.go diff --git a/internal/cyberark/client_test.go b/internal/cyberark/client_test.go index b52e3507..d111f99e 100644 --- a/internal/cyberark/client_test.go +++ b/internal/cyberark/client_test.go @@ -39,10 +39,8 @@ func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) { discoveryContextAPI, _ := dataupload.MockDataUploadServer(t) - // Unused by the Conjur path, but service discovery requires it to be set. - // Never dialed, so a loopback address is fine — see pkg/testutil/envtest.go's - // identical const for why this is preferred over a real, resolvable - // CyberArk zone name. + // Required by service discovery, unused by the Conjur path, never + // dialed. MockDiscoveryServer relaxes the allowlist to loopback. const identitySrv = "https://127.0.0.1:1" httpClient := servicediscovery.MockDiscoveryServer(t, servicediscovery.Services{ @@ -64,7 +62,8 @@ func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) { JWTFilePath: jwtFile.Name(), } - discoveryClient := servicediscovery.New(httpClient, cfg.Subdomain) + discoveryClient, err := servicediscovery.New(httpClient, cfg.Subdomain) + require.NoError(t, err) serviceMap, tenantUUID, err := discoveryClient.DiscoverServices(t.Context()) if err != nil { @@ -173,7 +172,8 @@ func TestCyberArkClient_PutSnapshot_RealAPI(t *testing.T) { cfg, err := cyberark.LoadClientConfigFromEnvironment() require.NoError(t, err) - discoveryClient := servicediscovery.New(httpClient, cfg.Subdomain) + discoveryClient, err := servicediscovery.New(httpClient, cfg.Subdomain) + require.NoError(t, err) serviceMap, tenantUUID, err := discoveryClient.DiscoverServices(t.Context()) if err != nil { diff --git a/internal/cyberark/conjur/conjur.go b/internal/cyberark/conjur/conjur.go index 64333a42..59942d90 100644 --- a/internal/cyberark/conjur/conjur.go +++ b/internal/cyberark/conjur/conjur.go @@ -46,7 +46,16 @@ type Client struct { } func New(httpClient *http.Client, baseURL, serviceID, account string, src jwtsource.Source) *Client { - return &Client{httpClient: httpClient, baseURL: baseURL, serviceID: serviceID, account: account, src: src, tokenTTL: defaultTokenTTL} + // The exchange POSTs the agent's service-account token as a form field. + // Go strips Authorization across a host change but never strips bodies, so + // a 3xx here would re-send that token to an unvalidated host. Nothing on + // this path legitimately redirects. Shallow copy, so the Transport and its + // connection pool are still shared. + noRedirect := *httpClient + noRedirect.CheckRedirect = func(req *http.Request, _ []*http.Request) error { + return fmt.Errorf("refusing to follow a redirect to %q: the authn-jwt exchange carries the agent's token in its body", req.URL.Hostname()) + } + return &Client{httpClient: &noRedirect, baseURL: baseURL, serviceID: serviceID, account: account, src: src, tokenTTL: defaultTokenTTL} } // Invalidate clears the cached token, forcing the next AuthenticateRequest diff --git a/internal/cyberark/conjur/mock.go b/internal/cyberark/conjur/mock.go index 7f591207..c3846bd0 100644 --- a/internal/cyberark/conjur/mock.go +++ b/internal/cyberark/conjur/mock.go @@ -4,21 +4,8 @@ import ( "net/http" "net/http/httptest" "testing" - - cyberarktesting "github.com/jetstack/preflight/internal/cyberark/testing" ) -// mockClientFor wraps srv's client so it can also reach any fake -// CyberArk-domain-looking host registered by another mock's -// cyberarktesting.RegisterMockHost (e.g. servicediscovery.MockDiscoveryServer), -// since a test may reuse this client to make a discovery call rather than -// discovery's own — see servicediscovery.MockDiscoveryServer's doc comment. -func mockClientFor(srv *httptest.Server) *http.Client { - client := srv.Client() - cyberarktesting.WrapMockTransport(client.Transport.(*http.Transport)) - return client -} - // MockConjurExchangeServer returns a TLS server whose authn-jwt endpoint returns the given token. func MockConjurExchangeServer(t testing.TB, token string) (*httptest.Server, *http.Client) { t.Helper() @@ -29,7 +16,7 @@ func MockConjurExchangeServer(t testing.TB, token string) (*httptest.Server, *ht } _, _ = w.Write([]byte(token)) })) - return srv, mockClientFor(srv) + return srv, srv.Client() } func MockConjurExchangeServerStatus(t testing.TB, status int) (*httptest.Server, *http.Client) { @@ -46,5 +33,5 @@ func MockConjurExchangeServerStatusBody(t testing.TB, status int, body []byte) ( w.WriteHeader(status) _, _ = w.Write(body) })) - return srv, mockClientFor(srv) + return srv, srv.Client() } diff --git a/internal/cyberark/dataupload/mock.go b/internal/cyberark/dataupload/mock.go index 0c67183b..28403775 100644 --- a/internal/cyberark/dataupload/mock.go +++ b/internal/cyberark/dataupload/mock.go @@ -20,7 +20,6 @@ import ( "k8s.io/client-go/transport" arkapi "github.com/jetstack/preflight/internal/cyberark/api" - cyberarktesting "github.com/jetstack/preflight/internal/cyberark/testing" "github.com/jetstack/preflight/pkg/version" ) @@ -79,11 +78,6 @@ func MockDataUploadServer(t testing.TB) (string, *http.Client) { mds.serverURL = server.URL httpClient := server.Client() - // So this client can also reach a fake CyberArk-domain-looking host - // registered by another mock (servicediscovery.MockDiscoveryServer), in - // case a test reuses it to make a discovery call rather than discovery's - // own client. - cyberarktesting.WrapMockTransport(httpClient.Transport.(*http.Transport)) httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext) return server.URL, httpClient } diff --git a/internal/cyberark/identity/cmd/testidentity/main.go b/internal/cyberark/identity/cmd/testidentity/main.go index 0a8df80b..9c972a4a 100644 --- a/internal/cyberark/identity/cmd/testidentity/main.go +++ b/internal/cyberark/identity/cmd/testidentity/main.go @@ -50,7 +50,10 @@ func run(ctx context.Context) error { var rootCAs *x509.CertPool httpClient := http_client.NewDefaultClient(version.UserAgent(), rootCAs) - sdClient := servicediscovery.New(httpClient, subdomain) + sdClient, err := servicediscovery.New(httpClient, subdomain) + if err != nil { + return err + } services, _, err := sdClient.DiscoverServices(ctx) if err != nil { return fmt.Errorf("while performing service discovery: %s", err) diff --git a/internal/cyberark/identity/identity_test.go b/internal/cyberark/identity/identity_test.go index 0915f46c..cb141e40 100644 --- a/internal/cyberark/identity/identity_test.go +++ b/internal/cyberark/identity/identity_test.go @@ -53,7 +53,9 @@ func TestLoginUsernamePassword_RealAPI(t *testing.T) { arktesting.SkipIfNoEnv(t) subdomain := os.Getenv("ARK_SUBDOMAIN") httpClient := http.DefaultClient - services, _, err := servicediscovery.New(httpClient, subdomain).DiscoverServices(t.Context()) + sdClient, err := servicediscovery.New(httpClient, subdomain) + require.NoError(t, err) + services, _, err := sdClient.DiscoverServices(t.Context()) require.NoError(t, err) loginUsernamePasswordTests(t, func(t testing.TB) inputs { diff --git a/internal/cyberark/identity/mock.go b/internal/cyberark/identity/mock.go index 0c3e69de..928ff868 100644 --- a/internal/cyberark/identity/mock.go +++ b/internal/cyberark/identity/mock.go @@ -12,7 +12,6 @@ import ( "k8s.io/client-go/transport" arkapi "github.com/jetstack/preflight/internal/cyberark/api" - cyberarktesting "github.com/jetstack/preflight/internal/cyberark/testing" "github.com/jetstack/preflight/pkg/version" _ "embed" @@ -82,11 +81,6 @@ func MockIdentityServer(t testing.TB) (string, *http.Client) { server := httptest.NewTLSServer(mis) t.Cleanup(server.Close) httpClient := server.Client() - // So this client can also reach a fake CyberArk-domain-looking host - // registered by another mock (servicediscovery.MockDiscoveryServer), in - // case a test reuses it to make a discovery call rather than discovery's - // own client. - cyberarktesting.WrapMockTransport(httpClient.Transport.(*http.Transport)) httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext) return server.URL, httpClient } diff --git a/internal/cyberark/servicediscovery/discovery.go b/internal/cyberark/servicediscovery/discovery.go index db5479d0..7c8a31eb 100644 --- a/internal/cyberark/servicediscovery/discovery.go +++ b/internal/cyberark/servicediscovery/discovery.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "io" + "net" "net/http" "net/url" "os" @@ -54,6 +55,12 @@ const ( // it constrains what a bad discovery response itself can point us at. Note // it doesn't distinguish between tenants either: any host on these domains // is accepted regardless of which tenant it belongs to. +// +// This mirrors an authoritative allowlist maintained outside this repository +// and must be kept in step with it: because it now also gates the bootstrap +// URL, a missing root domain stops those agents starting at all. Keep the +// gov-cloud entries — an earlier draft omitted them, which would have broken +// every gov-cloud agent. var allowedRootDomains = []string{ "cyberark.cloud", "cyberark-everest-dev.com", @@ -77,11 +84,21 @@ var allowedRootDomains = []string{ "cyberarkgov.cloud", } +// allowLoopbackHosts additionally accepts loopback addresses, so tests can +// use a local httptest server. Unreachable in production: unexported, and +// only MockDiscoveryServer (which requires a testing.TB) sets it. +var allowLoopbackHosts bool + // hostOnAllowedRootDomain reports whether host is, or is a subdomain of, one // of allowedRootDomains. Hostnames are case-insensitive and may carry a // trailing dot (a legal absolute FQDN), so normalise before comparing. func hostOnAllowedRootDomain(host string) bool { host = strings.ToLower(strings.TrimSuffix(host, ".")) + if allowLoopbackHosts { + if ip := net.ParseIP(host); ip != nil && ip.IsLoopback() { + return true + } + } for _, root := range allowedRootDomains { if host == root || strings.HasSuffix(host, "."+root) { return true @@ -91,17 +108,25 @@ func hostOnAllowedRootDomain(host string) bool { } // hostLeadingLabelMatchesSubdomain reports whether host's leading label -// names subdomain, tolerating the two label shapes observed in practice: -// - {subdomain}.{service}.{domain} -// - {subdomain}-{service}.{domain} +// names subdomain, tolerating the two label shapes seen in practice: +// {subdomain}.{service}.{domain} and {subdomain}-{service}.{domain}. func hostLeadingLabelMatchesSubdomain(host, subdomain string) bool { if subdomain == "" { return true } - label, _, _ := strings.Cut(host, ".") + label, _, _ := strings.Cut(strings.ToLower(host), ".") + subdomain = strings.ToLower(subdomain) return label == subdomain || strings.HasPrefix(label, subdomain+"-") } +// subdomainCheckApplies excludes identity_administration, whose host is keyed +// on the Identity tenant's own identifier rather than the platform subdomain +// (subdomain "venafi-test" is served identity at "ajp5871.id."), so +// the check would warn for every healthy tenant. +func subdomainCheckApplies(serviceName string) bool { + return serviceName != IdentityServiceName +} + // sanitizeServiceAPI returns rawAPI unchanged if its scheme is https and its // host is on an allowed root domain, or "" (treated the same as "service not // present in the response") if not. @@ -126,7 +151,7 @@ func sanitizeServiceAPI(ctx context.Context, serviceName, rawAPI, subdomain stri klog.FromContext(ctx).Info("dropping service discovery API URL outside the allowed CyberArk domains", "service", serviceName, "host", u.Hostname()) return "" } - if !hostLeadingLabelMatchesSubdomain(u.Hostname(), subdomain) { + if subdomainCheckApplies(serviceName) && !hostLeadingLabelMatchesSubdomain(u.Hostname(), subdomain) { // Not dropped -- see the function doc comment. A tampered response // could still redirect within the same allowed root domain to a // different tenant's host; this is the visibility half of closing @@ -162,15 +187,39 @@ func mainActiveAPI(eps []ServiceEndpoint) string { return "" } +// validateBaseURL reports whether rawURL is usable as the discovery bootstrap +// endpoint: parseable, HTTPS, and on an allowed root domain. +// +// The error names only the scheme and host, never rawURL: ARK_DISCOVERY_API +// can carry credentials, and this error reaches a Kubernetes Event. +func validateBaseURL(rawURL string) error { + u, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("not a valid URL") + } + if u.Scheme != "https" || !hostOnAllowedRootDomain(u.Hostname()) { + return fmt.Errorf("%s://%s is not HTTPS on an allowed CyberArk domain", u.Scheme, u.Hostname()) + } + return nil +} + // New creates a new CyberArk Service Discovery client. If the ARK_DISCOVERY_API // environment variable is set, it is used as the base URL for the service // discovery API. Otherwise, the production URL is used. -func New(httpClient *http.Client, subdomain string) *Client { +// +// The base URL is validated here so that a bad ARK_DISCOVERY_API is reported +// as the configuration error it is, at startup, rather than surfacing later as +// a repeating push failure. +func New(httpClient *http.Client, subdomain string) (*Client, error) { baseURL := os.Getenv("ARK_DISCOVERY_API") if baseURL == "" { baseURL = ProdDiscoveryAPIBaseURL } + if err := validateBaseURL(baseURL); err != nil { + return nil, fmt.Errorf("invalid service discovery base URL (from ARK_DISCOVERY_API): %w; refusing to bootstrap trust from it", err) + } + client := &Client{ client: httpClient, baseURL: baseURL, @@ -182,7 +231,7 @@ func New(httpClient *http.Client, subdomain string) *Client { cachedResponseMutex: sync.Mutex{}, } - return client + return client, nil } // DiscoveryResponse represents the full JSON response returned by the CyberArk api/tenant-discovery/public API @@ -236,18 +285,21 @@ func (c *Client) DiscoverServices(ctx context.Context) (*Services, string, error return c.cachedResponse, c.cachedTenantID, nil } + // Repeats New()'s check, so the guarantee holds for a Client built any + // other way and no request is issued if it doesn't. + // + // Note this validates the host we address, not the host that answers: + // only the Conjur exchange sets CheckRedirect, so elsewhere a 3xx can + // still move a request to a host that was never checked. + if err := validateBaseURL(c.baseURL); err != nil { + return nil, "", fmt.Errorf("invalid service discovery base URL: %w; refusing to bootstrap trust from it", err) + } + u, err := url.Parse(c.baseURL) if err != nil { return nil, "", fmt.Errorf("invalid base URL for service discovery: %w", err) } - // The bootstrap call itself must be to an allowed domain over HTTPS too, - // not just the hosts it later points us at — otherwise ARK_DISCOVERY_API - // alone could bootstrap trust from arbitrary infrastructure. - if u.Scheme != "https" || !hostOnAllowedRootDomain(u.Hostname()) { - return nil, "", fmt.Errorf("service discovery base URL %q is not HTTPS on an allowed CyberArk domain; refusing to bootstrap trust from it", c.baseURL) - } - u.Path = path.Join(u.Path, "api/public/tenant-discovery") u.RawQuery = url.Values{"bySubdomain": []string{c.subdomain}}.Encode() diff --git a/internal/cyberark/servicediscovery/discovery_test.go b/internal/cyberark/servicediscovery/discovery_test.go index 882f53e3..4cc62178 100644 --- a/internal/cyberark/servicediscovery/discovery_test.go +++ b/internal/cyberark/servicediscovery/discovery_test.go @@ -52,27 +52,55 @@ func Test_hostOnAllowedRootDomain(t *testing.T) { } } -func Test_DiscoverServices_RejectsDisallowedBaseURL(t *testing.T) { +// failOnDial is an http.RoundTripper that fails the test if it is ever used. +// It pins the property the base-URL guard exists to provide: a disallowed +// ARK_DISCOVERY_API must be rejected without a request being issued. +type failOnDial struct{ t *testing.T } + +func (f failOnDial) RoundTrip(req *http.Request) (*http.Response, error) { + f.t.Errorf("no request should be made for a disallowed base URL, got one to %q", req.URL.Redacted()) + return nil, fmt.Errorf("unexpected request") +} + +func Test_RejectsDisallowedBaseURL(t *testing.T) { tests := map[string]string{ - "plain HTTP": "http://platform-discovery.cyberark.cloud/", - "disallowed domain": "https://attacker.example/", - "host with no scheme": "platform-discovery.cyberark.cloud", + "plain HTTP": "http://platform-discovery.cyberark.cloud/", + "disallowed domain": "https://attacker.example/", + "host with no scheme": "platform-discovery.cyberark.cloud", + "loopback when not set": "https://127.0.0.1:1234/", } for name, baseURL := range tests { t.Run(name, func(t *testing.T) { t.Setenv("ARK_DISCOVERY_API", baseURL) - logger := ktesting.NewLogger(t, ktesting.DefaultConfig) - ctx := klog.NewContext(t.Context(), logger) - - client := New(&http.Client{}, MockDiscoverySubdomain) - services, _, err := client.DiscoverServices(ctx) + // Rejected at construction, so no client is built and no + // request is ever attempted. + client, err := New(&http.Client{Transport: failOnDial{t}}, MockDiscoverySubdomain) require.Error(t, err) - assert.Nil(t, services) + require.ErrorContains(t, err, "refusing to bootstrap trust") + assert.Nil(t, client) }) } } +// Test_DiscoverServices_RevalidatesBaseURL covers the defence-in-depth repeat +// of the check inside DiscoverServices, for a Client not built via New(). +func Test_DiscoverServices_RevalidatesBaseURL(t *testing.T) { + logger := ktesting.NewLogger(t, ktesting.DefaultConfig) + ctx := klog.NewContext(t.Context(), logger) + + client := &Client{ + client: &http.Client{Transport: failOnDial{t}}, + baseURL: "http://platform-discovery.cyberark.cloud/", + subdomain: MockDiscoverySubdomain, + } + + services, _, err := client.DiscoverServices(ctx) + require.Error(t, err) + require.ErrorContains(t, err, "refusing to bootstrap trust") + assert.Nil(t, services) +} + func Test_DiscoverIdentityAPIURL(t *testing.T) { tests := map[string]struct { subdomain string @@ -127,7 +155,8 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) { }, }) - client := New(httpClient, MockDiscoverySubdomain) + client, err := New(httpClient, MockDiscoverySubdomain) + require.NoError(t, err) services, _, err := client.DiscoverServices(ctx) require.Error(t, err) assert.Nil(t, services) @@ -154,7 +183,8 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) { }, }) - client := New(httpClient, MockDiscoverySubdomain) + client, err := New(httpClient, MockDiscoverySubdomain) + require.NoError(t, err) services, _, err := client.DiscoverServices(ctx) require.NoError(t, err) assert.Equal(t, mockIdentityAPIURL, services.Identity.API) @@ -178,7 +208,8 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) { }, }) - client := New(httpClient, MockDiscoverySubdomain) + client, err := New(httpClient, MockDiscoverySubdomain) + require.NoError(t, err) services, _, err := client.DiscoverServices(ctx) require.Error(t, err) assert.Nil(t, services) @@ -200,7 +231,8 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) { }, }) - client := New(httpClient, MockDiscoverySubdomain) + client, err := New(httpClient, MockDiscoverySubdomain) + require.NoError(t, err) services, _, err := client.DiscoverServices(ctx) require.NoError(t, err) assert.Equal(t, mockIdentityAPIURL, services.Identity.API) @@ -224,7 +256,8 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) { }, }) - client := New(httpClient, MockDiscoverySubdomain) + client, err := New(httpClient, MockDiscoverySubdomain) + require.NoError(t, err) services, _, err := client.DiscoverServices(ctx) require.NoError(t, err) assert.Equal(t, "https://ajp5871.id.cyberarkgov.cloud", services.Identity.API) @@ -248,7 +281,8 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) { }, }) - client := New(httpClient, MockDiscoverySubdomain) + client, err := New(httpClient, MockDiscoverySubdomain) + require.NoError(t, err) services, _, err := client.DiscoverServices(ctx) require.NoError(t, err) assert.Equal(t, "https://AJP5871.ID.Integration-CyberArk.Cloud", services.Identity.API) @@ -276,7 +310,8 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) { }, }) - client := New(httpClient, MockDiscoverySubdomain) + client, err := New(httpClient, MockDiscoverySubdomain) + require.NoError(t, err) services, _, err := client.DiscoverServices(ctx) require.NoError(t, err) assert.Equal(t, "https://some-other-tenant.id.integration-cyberark.cloud", services.Identity.API) @@ -299,7 +334,8 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) { }, }) - client := New(httpClient, testSpec.subdomain) + client, err := New(httpClient, testSpec.subdomain) + require.NoError(t, err) services, _, err := client.DiscoverServices(ctx) if testSpec.expectedError != nil { diff --git a/internal/cyberark/servicediscovery/mock.go b/internal/cyberark/servicediscovery/mock.go index 9bc06bc5..adcb56e3 100644 --- a/internal/cyberark/servicediscovery/mock.go +++ b/internal/cyberark/servicediscovery/mock.go @@ -5,20 +5,15 @@ import ( "crypto/rand" "encoding/hex" "encoding/json" - "fmt" - "net" "net/http" "net/http/httptest" - "net/url" "strings" - "sync/atomic" "testing" "text/template" "k8s.io/client-go/transport" arkapi "github.com/jetstack/preflight/internal/cyberark/api" - cyberarktesting "github.com/jetstack/preflight/internal/cyberark/testing" "github.com/jetstack/preflight/pkg/version" _ "embed" @@ -42,32 +37,6 @@ type mockDiscoveryServer struct { successResponse string } -var fakeHostCounter atomic.Uint64 - -// launderIfLoopback rewrites rawURL to an allowlisted-domain-looking -// hostname and registers a dial redirect (via cyberarktesting.RegisterMockHost) -// to its real address, if rawURL's host is a loopback IP (a real httptest -// mock server address). Any other value — including deliberately-invalid -// test hosts like "attacker.example" — is returned unchanged, since those -// must still be rejected by the code under test, not laundered into passing. -func launderIfLoopback(rawURL string) string { - if rawURL == "" { - return rawURL - } - u, err := url.Parse(rawURL) - if err != nil { - return rawURL - } - ip := net.ParseIP(u.Hostname()) - if ip == nil || !ip.IsLoopback() { - return rawURL - } - fakeHost := fmt.Sprintf("mock-%d.integration-cyberark.cloud", fakeHostCounter.Add(1)) - cyberarktesting.RegisterMockHost(fakeHost, u.Host) - u.Host = fakeHost - return u.String() -} - // MockDiscoveryServer starts a mocked CyberArk service discovery server and // returns an HTTP client with the CA certs needed to connect to it. // @@ -80,23 +49,13 @@ func launderIfLoopback(rawURL string) string { // supplied in `services`. // Other subdomains, can be used to trigger various failure responses. // -// Any of services' API fields that point at a real loopback mock server -// (rather than a fake CyberArk-domain-looking test hostname) is laundered — -// see launderIfLoopback — into a fake CyberArk-domain hostname, with a dial -// redirect registered via cyberarktesting.RegisterMockHost so any -// WrapMockTransport-wrapped client (not just this one) can still reach it. -// This keeps DiscoverServices' domain/HTTPS allowlist (which now also -// covers ARK_DISCOVERY_API itself, see CP-26002) from stripping out real -// dataupload/conjur/identity mock addresses that other packages' tests -// embed here. +// Sets allowLoopbackHosts for the duration of the test, so DiscoverServices' +// allowlist accepts these loopback mocks. Deliberately invalid test hosts +// (attacker.example, plain http://) are unaffected and still rejected. // // The returned HTTP client has a transport which logs requests and responses // depending on log level of the logger supplied in the context. func MockDiscoveryServer(t testing.TB, services Services) *http.Client { - services.Identity.API = launderIfLoopback(services.Identity.API) - services.DiscoveryContext.API = launderIfLoopback(services.DiscoveryContext.API) - services.SecretsManager.API = launderIfLoopback(services.SecretsManager.API) - tmpl := template.Must(template.New("mockDiscoverySuccess").Parse(discoverySuccessTemplate)) buf := &bytes.Buffer{} if err := tmpl.Execute(buf, services); err != nil { @@ -107,27 +66,16 @@ func MockDiscoveryServer(t testing.TB, services Services) *http.Client { server := httptest.NewTLSServer(mds) t.Cleanup(server.Close) - httpClient := server.Client() - baseTransport := httpClient.Transport.(*http.Transport).Clone() - cyberarktesting.WrapMockTransport(baseTransport) + allowLoopbackHosts = true + t.Cleanup(func() { allowLoopbackHosts = false }) - discoveryFakeHost := fmt.Sprintf("mock-%d.integration-cyberark.cloud", fakeHostCounter.Add(1)) - cyberarktesting.RegisterMockHost(discoveryFakeHost, mustHostPort(t, server.URL)) - t.Setenv("ARK_DISCOVERY_API", "https://"+discoveryFakeHost) + t.Setenv("ARK_DISCOVERY_API", server.URL) - httpClient.Transport = transport.NewDebuggingRoundTripper(baseTransport, transport.DebugByContext) + httpClient := server.Client() + httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext) return httpClient } -func mustHostPort(t testing.TB, rawURL string) string { - t.Helper() - u, err := url.Parse(rawURL) - if err != nil { - t.Fatalf("MockDiscoveryServer: invalid server URL %q: %v", rawURL, err) - } - return u.Host -} - func (mds *mockDiscoveryServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { mds.t.Log(r.Method, r.RequestURI) if r.Method != http.MethodGet { diff --git a/internal/cyberark/testing/mockdial.go b/internal/cyberark/testing/mockdial.go deleted file mode 100644 index 6ea31b2f..00000000 --- a/internal/cyberark/testing/mockdial.go +++ /dev/null @@ -1,61 +0,0 @@ -package testing - -import ( - "context" - "crypto/tls" - "net" - "net/http" - "sync" -) - -// mockHostMu and mockHostAddr back a process-wide registry of fake hostname -// -> real host:port, shared across every package's Mock*Server helper. It -// has to be process-wide rather than scoped to one http.Client: tests freely -// reuse whichever mock's client is convenient to make a call against a -// *different* mock's server (e.g. keyfetch's test setup discards -// servicediscovery.MockDiscoveryServer's own client and instead reuses -// conjur.MockConjurExchangeServer's), so any mock client might end up being -// the one that has to dial any other mock's registered fake host. -var ( - mockHostMu sync.Mutex - mockHostAddr = map[string]string{} -) - -// RegisterMockHost tells every WrapMockTransport-wrapped client to redirect -// dials for fakeHost to realHostPort instead. Used by servicediscovery's -// mock to embed another package's mock server address in a discovery -// response under a CyberArk-domain-looking hostname, since -// servicediscovery.DiscoverServices now allowlists both ARK_DISCOVERY_API -// itself and the hosts it returns (CP-25960, CP-26002). -func RegisterMockHost(fakeHost, realHostPort string) { - mockHostMu.Lock() - defer mockHostMu.Unlock() - mockHostAddr[fakeHost] = realHostPort -} - -// WrapMockTransport installs a DialContext on transport that redirects any -// RegisterMockHost-registered fake host to its real address, and disables -// TLS hostname verification, since a fake host never matches the httptest -// server's actual certificate SAN. Every package's Mock*Server should call -// this on its returned client's transport, so that client can reach a fake -// host registered by any other mock, regardless of which mock's client a -// test ends up reusing for a given call. -func WrapMockTransport(transport *http.Transport) { - if transport.TLSClientConfig == nil { - transport.TLSClientConfig = &tls.Config{} - } else { - transport.TLSClientConfig = transport.TLSClientConfig.Clone() - } - transport.TLSClientConfig.InsecureSkipVerify = true - transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { - if host, _, err := net.SplitHostPort(addr); err == nil { - mockHostMu.Lock() - realAddr, ok := mockHostAddr[host] - mockHostMu.Unlock() - if ok { - addr = realAddr - } - } - return (&net.Dialer{}).DialContext(ctx, network, addr) - } -} diff --git a/internal/envelope/keyfetch/client_test.go b/internal/envelope/keyfetch/client_test.go index d1b88849..8ef555ec 100644 --- a/internal/envelope/keyfetch/client_test.go +++ b/internal/envelope/keyfetch/client_test.go @@ -31,10 +31,8 @@ func testClientSetup(t *testing.T, jwksServerURL string) (*Client, cyberark.Clie Identity: servicediscovery.ServiceEndpoint{ IsActive: true, Type: "main", - // Unused by the Conjur path, but service discovery requires it. - // Never dialed, so a loopback address is fine — passes the - // allowlist via the same-host-as-discovery escape hatch rather - // than depending on a real, resolvable CyberArk zone name. + // Required by service discovery, unused here, never dialed. + // MockDiscoveryServer relaxes the allowlist to loopback. API: "https://127.0.0.1:1", }, DiscoveryContext: servicediscovery.ServiceEndpoint{ @@ -53,7 +51,8 @@ func testClientSetup(t *testing.T, jwksServerURL string) (*Client, cyberark.Clie _ = servicediscovery.MockDiscoveryServer(t, services) // Create discovery client - discoveryClient := servicediscovery.New(httpClient, servicediscovery.MockDiscoverySubdomain) + discoveryClient, err := servicediscovery.New(httpClient, servicediscovery.MockDiscoverySubdomain) + require.NoError(t, err) // Create test config — JWTFilePath is empty; jwtsource.NewFileSource will use DefaultTokenPath, // but the conjur mock accepts any jwt value so no real file read occurs. @@ -94,7 +93,8 @@ func testKeyfetchClientWithIdentityAuth(t *testing.T, jwksServerURL string) (*Cl }, } _ = servicediscovery.MockDiscoveryServer(t, services) - discoveryClient := servicediscovery.New(httpClient, servicediscovery.MockDiscoverySubdomain) + discoveryClient, err := servicediscovery.New(httpClient, servicediscovery.MockDiscoverySubdomain) + require.NoError(t, err) client := &Client{ discoveryClient: discoveryClient, @@ -311,7 +311,8 @@ func TestClient_FetchKey(t *testing.T) { _ = servicediscovery.MockDiscoveryServer(t, services) // Create discovery client - discoveryClient := servicediscovery.New(httpClient, servicediscovery.MockDiscoverySubdomain) + discoveryClient, err := servicediscovery.New(httpClient, servicediscovery.MockDiscoverySubdomain) + require.NoError(t, err) cfg := cyberark.ClientConfig{ Subdomain: servicediscovery.MockDiscoverySubdomain, @@ -354,7 +355,8 @@ func TestClient_FetchKey(t *testing.T) { _ = servicediscovery.MockDiscoveryServer(t, services) // Create discovery client with a subdomain that triggers failure - discoveryClient := servicediscovery.New(httpClient, "bad-request") + discoveryClient, err := servicediscovery.New(httpClient, "bad-request") + require.NoError(t, err) cfg := cyberark.ClientConfig{ Subdomain: "bad-request", @@ -362,7 +364,7 @@ func TestClient_FetchKey(t *testing.T) { JWTFilePath: "testdata/fake-jwt", } - _, err := NewClient(t.Context(), discoveryClient, cfg, httpClient) + _, err = NewClient(t.Context(), discoveryClient, cfg, httpClient) require.Error(t, err) assert.Contains(t, err.Error(), "failed to get services from discovery client") diff --git a/pkg/client/client_cyberark.go b/pkg/client/client_cyberark.go index 7ffcebf4..e072b450 100644 --- a/pkg/client/client_cyberark.go +++ b/pkg/client/client_cyberark.go @@ -54,10 +54,15 @@ func NewCyberArk(httpClient *http.Client, serviceID, account, jwtSource, jwtFile configLoader := func() (cyberark.ClientConfig, error) { return cfg, nil } + discoveryClient, err := servicediscovery.New(httpClient, cfg.Subdomain) + if err != nil { + return nil, err + } + return &CyberArkClient{ configLoader: configLoader, httpClient: httpClient, - discoveryClient: servicediscovery.New(httpClient, cfg.Subdomain), + discoveryClient: discoveryClient, }, nil } diff --git a/pkg/testutil/envtest.go b/pkg/testutil/envtest.go index 226da2d2..e022918b 100644 --- a/pkg/testutil/envtest.go +++ b/pkg/testutil/envtest.go @@ -288,13 +288,9 @@ func FakeCyberArk(t testing.TB) (httpClient *http.Client, jwtFilePath string) { discoveryContextAPI, _ := dataupload.MockDataUploadServer(t) httpClient = servicediscovery.MockDiscoveryServer(t, servicediscovery.Services{ Identity: servicediscovery.ServiceEndpoint{ - // Required unconditionally by DiscoverServices, present for every - // healthy tenant — see servicediscovery/discovery.go. Unused by - // the Conjur path itself. Never dialed, so a loopback address is - // fine — it passes the allowlist via the same-host-as-discovery - // escape hatch (MockDiscoveryServer's ARK_DISCOVERY_API is also - // 127.0.0.1) without depending on a real, resolvable CyberArk - // zone name for something this test never intends to reach. + // Required by DiscoverServices, but unused by the Conjur path + // and never dialed. Loopback is accepted because + // MockDiscoveryServer relaxes the allowlist to loopback. API: "https://127.0.0.1:1", }, DiscoveryContext: servicediscovery.ServiceEndpoint{