CP-26002: validate ARK_DISCOVERY_API itself against the domain allowlist

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 {

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

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")
 }

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(`<empty body>`)
-		}
-		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(`<empty body>`)
-		}
-		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 {

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")
+}

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)

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 {