diff --git a/internal/cyberark/client_test.go b/internal/cyberark/client_test.go index 9b64d543..06b187a6 100644 --- a/internal/cyberark/client_test.go +++ b/internal/cyberark/client_test.go @@ -40,7 +40,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. - const identitySrv = "https://identity.example.invalid" + // Never dialed, so it just needs a host servicediscovery's allowlist accepts. + const identitySrv = "https://identity.example.integration-cyberark.cloud" httpClient := servicediscovery.MockDiscoveryServer(t, servicediscovery.Services{ Identity: servicediscovery.ServiceEndpoint{ diff --git a/internal/cyberark/conjur/conjur.go b/internal/cyberark/conjur/conjur.go index 08fc464b..64333a42 100644 --- a/internal/cyberark/conjur/conjur.go +++ b/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 { diff --git a/internal/cyberark/conjur/conjur_test.go b/internal/cyberark/conjur/conjur_test.go index 8cab98e4..8069a4c4 100644 --- a/internal/cyberark/conjur/conjur_test.go +++ b/internal/cyberark/conjur/conjur_test.go @@ -213,7 +213,14 @@ 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() @@ -221,5 +228,6 @@ func TestAuthenticateRequest_ExchangeErrorIncludesConjurResponseBody(t *testing. 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)") } diff --git a/internal/cyberark/servicediscovery/discovery.go b/internal/cyberark/servicediscovery/discovery.go index bfbd66dd..623f99a0 100644 --- a/internal/cyberark/servicediscovery/discovery.go +++ b/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,81 @@ 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 we just made a +// successful, TLS-authenticated discovery call to. The latter matters for +// ARK_DISCOVERY_API-overridden (dev/CI/test) discovery endpoints: whatever +// host that override already points at is exactly as trusted as the +// discovery call itself, so a service response pointing back at that same +// host can't be a new SSRF target. +func isAllowedServiceHost(host, discoveryHost string) bool { + if host == discoveryHost { + return true + } + for _, root := range allowedRootDomains { + if host == root || strings.HasSuffix(host, "."+root) { + return true + } + } + return false +} + +// sanitizeServiceAPI returns rawAPI unchanged if 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 !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,6 +273,14 @@ 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. + 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 diff --git a/internal/cyberark/servicediscovery/discovery_test.go b/internal/cyberark/servicediscovery/discovery_test.go index 23c2f1b6..89520118 100644 --- a/internal/cyberark/servicediscovery/discovery_test.go +++ b/internal/cyberark/servicediscovery/discovery_test.go @@ -50,6 +50,76 @@ 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) + }) + + 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("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) + }) + for name, testSpec := range tests { t.Run(name, func(t *testing.T) { logger := ktesting.NewLogger(t, ktesting.DefaultConfig) diff --git a/internal/envelope/keyfetch/client_test.go b/internal/envelope/keyfetch/client_test.go index f8406ff0..b7f9f282 100644 --- a/internal/envelope/keyfetch/client_test.go +++ b/internal/envelope/keyfetch/client_test.go @@ -32,7 +32,7 @@ func testClientSetup(t *testing.T, jwksServerURL string) (*Client, cyberark.Clie IsActive: true, Type: "main", // Unused by the Conjur path, but service discovery requires it. - API: "https://identity.example.invalid", + API: "https://identity.example.integration-cyberark.cloud", }, DiscoveryContext: servicediscovery.ServiceEndpoint{ IsActive: true, @@ -288,7 +288,7 @@ func TestClient_FetchKey(t *testing.T) { IsActive: true, Type: "main", // Unused by the Conjur path, but service discovery requires it. - API: "https://identity.example.invalid", + API: "https://identity.example.integration-cyberark.cloud", }, DiscoveryContext: servicediscovery.ServiceEndpoint{ IsActive: true, @@ -334,7 +334,7 @@ func TestClient_FetchKey(t *testing.T) { IsActive: true, Type: "main", // Unused by the Conjur path, but service discovery requires it. - API: "https://identity.example.invalid", + API: "https://identity.example.integration-cyberark.cloud", }, SecretsManager: servicediscovery.ServiceEndpoint{ IsActive: true, diff --git a/pkg/testutil/envtest.go b/pkg/testutil/envtest.go index 9a49588a..3b65a200 100644 --- a/pkg/testutil/envtest.go +++ b/pkg/testutil/envtest.go @@ -290,8 +290,9 @@ func FakeCyberArk(t testing.TB) (httpClient *http.Client, jwtFilePath string) { Identity: servicediscovery.ServiceEndpoint{ // Required unconditionally by DiscoverServices, present for every // healthy tenant — see servicediscovery/discovery.go. Unused by - // the Conjur path itself. - API: "https://identity.example.invalid", + // the Conjur path itself. Never dialed, so it just needs a host + // servicediscovery's allowlist accepts. + API: "https://identity.example.integration-cyberark.cloud", }, DiscoveryContext: servicediscovery.ServiceEndpoint{ API: discoveryContextAPI,