CP-25960: allowlist and require HTTPS for discovery-derived hosts

internal/cyberark/client_test.go

@@ -40,7 +40,10 @@ func TestCyberArkClient_PutSnapshot_MockAPI(t *testing.T) {
 	discoveryContextAPI, _ := dataupload.MockDataUploadServer(t)
 
 	// Unused by the Conjur path, but service discovery requires it to be set.
-	const identitySrv = "https://identity.example.invalid"
+	// 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.
+	const identitySrv = "https://127.0.0.1:1"
 
 	httpClient := servicediscovery.MockDiscoveryServer(t, servicediscovery.Services{
 		Identity: servicediscovery.ServiceEndpoint{

internal/cyberark/conjur/conjur.go

@@ -85,16 +85,21 @@ func (c *Client) exchange(ctx context.Context) (string, error) {
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode != http.StatusOK {
-		// Conjur returns a JSON error body with the actual reason; include a
-		// bounded prefix so the operator doesn't have to go read Conjur's own
-		// audit log to find out why. 401 here most often means the SA token
-		// audience != authenticator audience=conjur.
+		// Conjur returns a JSON error body with the actual reason. It can
+		// contain policy structure, service IDs and host identities, and this
+		// error is surfaced all the way up to a Kubernetes Pod Event
+		// (pkg/agent/run.go's PushingErr notification), which anyone with
+		// `get events` in the namespace can read — so the body is logged at
+		// V(2) for an operator to go find, not embedded in the returned
+		// error. 401 here most often means the SA token audience != the
+		// authenticator's audience=conjur.
 		errBody, _ := io.ReadAll(io.LimitReader(resp.Body, 4*1024))
 		// Drain the rest so the connection can be reused, bounded so a
 		// misbehaving server can't make this read unboundedly.
 		_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 1024*1024))
-		return "", fmt.Errorf("authn-jwt exchange rejected (%d): %s; verify service_id, the authenticator is enabled, and the SA token audience is 'conjur'",
-			resp.StatusCode, strings.TrimSpace(string(errBody)))
+		klog.FromContext(ctx).V(2).Info("authn-jwt exchange rejected", "statusCode", resp.StatusCode, "body", strings.TrimSpace(string(errBody)))
+		return "", fmt.Errorf("authn-jwt exchange rejected (%d); verify service_id, the authenticator is enabled, and the SA token audience is 'conjur' (run with -v=2 to see Conjur's response body)",
+			resp.StatusCode)
 	}
 	body, err := io.ReadAll(io.LimitReader(resp.Body, 64*1024))
 	if err != nil {

internal/cyberark/conjur/conjur_test.go

@@ -213,13 +213,21 @@ func TestInvalidate_ForcesReexchange(t *testing.T) {
 	require.Equal(t, 2, exchanges, "Invalidate must force the next AuthenticateRequest call to re-exchange")
 }
 
-func TestAuthenticateRequest_ExchangeErrorIncludesConjurResponseBody(t *testing.T) {
+// This error propagates all the way up to a Kubernetes Pod Event
+// (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.
+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)
 	_, err := c.AuthenticateRequest(req)
 	require.Error(t, err)
-	require.Contains(t, err.Error(), "CONJ00001E Invalid JWT token")
+	require.NotContains(t, err.Error(), "CONJ00001E Invalid JWT token")
+	require.Contains(t, err.Error(), "authn-jwt exchange rejected (401)")
 }

internal/cyberark/servicediscovery/discovery.go

@@ -9,9 +9,12 @@ import (
 	"net/url"
 	"os"
 	"path"
+	"strings"
 	"sync"
 	"time"
 
+	"k8s.io/klog/v2"
+
 	arkapi "github.com/jetstack/preflight/internal/cyberark/api"
 	"github.com/jetstack/preflight/pkg/version"
 )
@@ -40,6 +43,101 @@ 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.
+//
+// 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.
+var allowedRootDomains = []string{
+	"cyberark.cloud",
+	"cyberark-everest-dev.com",
+	"cyberark-everest-test.com",
+	"cyberark-everest-stage.com",
+	"sandbox-cyberark.cloud",
+	"integration-cyberark.cloud",
+	"pt-cyberark.cloud",
+	"cyberark-everest-integdev.cloud",
+	"cyberark-everest-preinteg.cloud",
+	"cyberark-everest-perf.cloud",
+	"cyberark-everest-pre-prod.cloud",
+	// Gov-cloud environments.
+	"dev-cyberarkgov.com",
+	"test-cyberarkgov.com",
+	"stage-cyberarkgov.com",
+	"integdev-cyberarkgov.cloud",
+	"integration-cyberarkgov.cloud",
+	"preprod-cyberarkgov.cloud",
+	"dum-preprod-cyberarkgov.cloud",
+	"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.
+//
+// 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
+	}
+	host = strings.ToLower(host)
+	for _, root := range allowedRootDomains {
+		if host == root || strings.HasSuffix(host, "."+root) {
+			return true
+		}
+	}
+	return false
+}
+
+// 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 {
+	if rawAPI == "" {
+		return ""
+	}
+	u, err := url.Parse(rawAPI)
+	if err != nil || u.Hostname() == "" {
+		klog.FromContext(ctx).Info("dropping unparseable service discovery API URL", "service", serviceName, "api", rawAPI)
+		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) {
+		klog.FromContext(ctx).Info("dropping service discovery API URL outside the allowed CyberArk domains", "service", serviceName, "host", u.Hostname())
+		return ""
+	}
+	return rawAPI
+}
+
 // Client is a Golang client for interacting with the CyberArk Discovery Service. It allows
 // users to fetch URLs for various APIs available in CyberArk. This client is specialised to
 // fetch only API endpoints, since only API endpoints are required by the Venafi Kubernetes Agent currently.
@@ -195,13 +293,30 @@ func (c *Client) DiscoverServices(ctx context.Context) (*Services, string, error
 		}
 	}
 
+	// Drop any of the three API URLs whose host isn't one of the CyberArk
+	// domains we actually trust, before anything downstream authenticates
+	// 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 is required unconditionally, unlike discoveryContextAPI and
 	// secretsManagerAPI below: it's present and active for every healthy
 	// tenant, so callers may rely on it being non-empty without checking it
 	// themselves again.
 	if identityAPI == "" {
-		return nil, "", fmt.Errorf("didn't find %s in service discovery response, "+
-			"which may indicate a suspended tenant; unable to detect CyberArk Identity API URL", IdentityServiceName)
+		if rawIdentityAPI == "" {
+			return nil, "", fmt.Errorf("didn't find %s in service discovery response, "+
+				"which may indicate a suspended tenant; unable to detect CyberArk Identity API URL", IdentityServiceName)
+		}
+		// 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)
 	}
 	// discoveryContextAPI and secretsManagerAPI are deliberately not required
 	// here, unlike identityAPI above: not every caller needs both, and

internal/cyberark/servicediscovery/discovery_test.go

@@ -50,6 +50,149 @@ func Test_DiscoverIdentityAPIURL(t *testing.T) {
 		},
 	}
 
+	t.Run("identity API host outside the allowed CyberArk domains is rejected", func(t *testing.T) {
+		logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
+		ctx := klog.NewContext(t.Context(), logger)
+
+		httpClient := MockDiscoveryServer(t, Services{
+			Identity: ServiceEndpoint{
+				API: "https://ajp5871.id.attacker.example",
+			},
+			DiscoveryContext: ServiceEndpoint{
+				API: mockDiscoveryContextAPIURL,
+			},
+			SecretsManager: ServiceEndpoint{
+				API: mockSecretsManagerAPIURL,
+			},
+		})
+
+		client := New(httpClient, MockDiscoverySubdomain)
+		services, _, err := client.DiscoverServices(ctx)
+		require.Error(t, err)
+		assert.Nil(t, services)
+		// The error must say the host was rejected, not the unrelated
+		// "suspended tenant" message reserved for a genuinely absent
+		// identity_administration entry (see the no-identity-in-response
+		// case in the tests map above, which still gets that message).
+		assert.Contains(t, err.Error(), "not on an allowed CyberArk domain")
+	})
+
+	t.Run("secrets_manager and discovery_context hosts outside the allowed CyberArk domains are dropped, not fatal", func(t *testing.T) {
+		logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
+		ctx := klog.NewContext(t.Context(), logger)
+
+		httpClient := MockDiscoveryServer(t, Services{
+			Identity: ServiceEndpoint{
+				API: mockIdentityAPIURL,
+			},
+			DiscoveryContext: ServiceEndpoint{
+				API: "https://venafi-test.inventory.attacker.example",
+			},
+			SecretsManager: ServiceEndpoint{
+				API: "https://venafi-test.secretsmgr.attacker.example",
+			},
+		})
+
+		client := New(httpClient, MockDiscoverySubdomain)
+		services, _, err := client.DiscoverServices(ctx)
+		require.NoError(t, err)
+		assert.Equal(t, mockIdentityAPIURL, services.Identity.API)
+		assert.Equal(t, "", services.DiscoveryContext.API)
+		assert.Equal(t, "", services.SecretsManager.API)
+	})
+
+	t.Run("plain-HTTP identity host is rejected even though the hostname is allowlisted", func(t *testing.T) {
+		logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
+		ctx := klog.NewContext(t.Context(), logger)
+
+		httpClient := MockDiscoveryServer(t, Services{
+			Identity: ServiceEndpoint{
+				API: "http://ajp5871.id.integration-cyberark.cloud",
+			},
+			DiscoveryContext: ServiceEndpoint{
+				API: mockDiscoveryContextAPIURL,
+			},
+			SecretsManager: ServiceEndpoint{
+				API: mockSecretsManagerAPIURL,
+			},
+		})
+
+		client := New(httpClient, MockDiscoverySubdomain)
+		services, _, err := client.DiscoverServices(ctx)
+		require.Error(t, err)
+		assert.Nil(t, services)
+	})
+
+	t.Run("plain-HTTP secrets_manager and discovery_context hosts are dropped, not fatal", func(t *testing.T) {
+		logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
+		ctx := klog.NewContext(t.Context(), logger)
+
+		httpClient := MockDiscoveryServer(t, Services{
+			Identity: ServiceEndpoint{
+				API: mockIdentityAPIURL,
+			},
+			DiscoveryContext: ServiceEndpoint{
+				API: "http://venafi-test.inventory.integration-cyberark.cloud",
+			},
+			SecretsManager: ServiceEndpoint{
+				API: "http://venafi-test.secretsmgr.integration-cyberark.cloud",
+			},
+		})
+
+		client := New(httpClient, MockDiscoverySubdomain)
+		services, _, err := client.DiscoverServices(ctx)
+		require.NoError(t, err)
+		assert.Equal(t, mockIdentityAPIURL, services.Identity.API)
+		assert.Equal(t, "", services.DiscoveryContext.API)
+		assert.Equal(t, "", services.SecretsManager.API)
+	})
+
+	t.Run("gov-cloud root domains are accepted", func(t *testing.T) {
+		logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
+		ctx := klog.NewContext(t.Context(), logger)
+
+		httpClient := MockDiscoveryServer(t, Services{
+			Identity: ServiceEndpoint{
+				API: "https://ajp5871.id.cyberarkgov.cloud",
+			},
+			DiscoveryContext: ServiceEndpoint{
+				API: "https://venafi-test.inventory.integration-cyberarkgov.cloud",
+			},
+			SecretsManager: ServiceEndpoint{
+				API: "https://venafi-test.secretsmgr.dev-cyberarkgov.com",
+			},
+		})
+
+		client := New(httpClient, MockDiscoverySubdomain)
+		services, _, err := client.DiscoverServices(ctx)
+		require.NoError(t, err)
+		assert.Equal(t, "https://ajp5871.id.cyberarkgov.cloud", services.Identity.API)
+		assert.Equal(t, "https://venafi-test.inventory.integration-cyberarkgov.cloud", services.DiscoveryContext.API)
+		assert.Equal(t, "https://venafi-test.secretsmgr.dev-cyberarkgov.com", services.SecretsManager.API)
+	})
+
+	t.Run("uppercase host is still accepted (net/url doesn't lowercase the host)", func(t *testing.T) {
+		logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
+		ctx := klog.NewContext(t.Context(), logger)
+
+		httpClient := MockDiscoveryServer(t, Services{
+			Identity: ServiceEndpoint{
+				API: "https://AJP5871.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://AJP5871.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)