diff --git a/internal/cyberark/client_test.go b/internal/cyberark/client_test.go index 9b64d543..b52e3507 100644 --- a/internal/cyberark/client_test.go +++ b/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{ 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..b6e27809 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,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 diff --git a/internal/cyberark/servicediscovery/discovery_test.go b/internal/cyberark/servicediscovery/discovery_test.go index 23c2f1b6..61fe7787 100644 --- a/internal/cyberark/servicediscovery/discovery_test.go +++ b/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) diff --git a/internal/envelope/keyfetch/client.go b/internal/envelope/keyfetch/client.go index 370c61f7..cbbadeb1 100644 --- a/internal/envelope/keyfetch/client.go +++ b/internal/envelope/keyfetch/client.go @@ -4,6 +4,7 @@ import ( "context" "crypto/rsa" "crypto/x509" + "errors" "fmt" "io" "net/http" @@ -118,6 +119,16 @@ func (c *Client) FetchKey(ctx context.Context) (PublicKey, error) { return PublicKey{}, fmt.Errorf("failed to get services from discovery client: %w", err) } + if services.DiscoveryContext.API == "" { + // Mirrors cyberark.NewDatauploadClient's check on the same field — + // without this, url.JoinPath("", ...) silently returns a relative + // path and the request below fails with the opaque + // "unsupported protocol scheme \"\"" instead of naming the real cause + // (service discovery didn't return a discoverycontext endpoint, e.g. + // because its host isn't on the allowed CyberArk domain list). + return PublicKey{}, errors.New("service discovery returned an empty discovery API") + } + endpoint, err := url.JoinPath(services.DiscoveryContext.API, "discovery-context/jwks") if err != nil { return PublicKey{}, fmt.Errorf("failed to construct endpoint URL: %w", err) @@ -143,8 +154,15 @@ 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, _ := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) - return PublicKey{}, fmt.Errorf("unexpected status code %d from %s: %s", resp.StatusCode, endpoint, string(body)) + 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) } body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) diff --git a/internal/envelope/keyfetch/client_test.go b/internal/envelope/keyfetch/client_test.go index f8406ff0..d1b88849 100644 --- a/internal/envelope/keyfetch/client_test.go +++ b/internal/envelope/keyfetch/client_test.go @@ -32,7 +32,10 @@ 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", + // 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. + API: "https://127.0.0.1:1", }, DiscoveryContext: servicediscovery.ServiceEndpoint{ IsActive: true, @@ -288,7 +291,9 @@ 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", + // Never dialed, so a loopback address is fine — see the + // comment on the identical field above. + API: "https://127.0.0.1:1", }, DiscoveryContext: servicediscovery.ServiceEndpoint{ IsActive: true, @@ -334,7 +339,9 @@ 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", + // Never dialed, so a loopback address is fine — see the + // comment on the identical field above. + API: "https://127.0.0.1:1", }, SecretsManager: servicediscovery.ServiceEndpoint{ IsActive: true, @@ -449,6 +456,21 @@ func TestClient_FetchKey(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "no valid RSA keys found") }) + + t.Run("empty discoverycontext API from discovery fails with a clear error, not an opaque one", func(t *testing.T) { + // An empty jwksServerURL makes DiscoveryContext.API "" in the + // discovery response (mainActiveAPI requires a non-empty api field, + // same as a genuinely absent/inactive service) — e.g. because its + // host wasn't on the allowed CyberArk domain list. Without the + // explicit check this fails with url.JoinPath("", ...) producing a + // relative path and client.Do erroring with the opaque + // `unsupported protocol scheme ""`. + client, _ := testClientSetup(t, "") + _, err := client.FetchKey(t.Context()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "service discovery returned an empty discovery API") + }) } // TestClient_FetchKey_UsernamePasswordAuth covers the legacy auth path, which diff --git a/pkg/testutil/envtest.go b/pkg/testutil/envtest.go index 9a49588a..226da2d2 100644 --- a/pkg/testutil/envtest.go +++ b/pkg/testutil/envtest.go @@ -290,8 +290,12 @@ 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 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. + API: "https://127.0.0.1:1", }, DiscoveryContext: servicediscovery.ServiceEndpoint{ API: discoveryContextAPI,