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/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/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/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/servicediscovery/discovery.go b/internal/cyberark/servicediscovery/discovery.go index b6e27809..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" @@ -43,21 +44,23 @@ 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 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. // -// 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. +// 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", @@ -81,27 +84,21 @@ 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. -// -// 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 +// 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 + } } - host = strings.ToLower(host) for _, root := range allowedRootDomains { if host == root || strings.HasSuffix(host, "."+root) { return true @@ -110,10 +107,34 @@ func isAllowedServiceHost(host, discoveryHost string) bool { return false } +// hostLeadingLabelMatchesSubdomain reports whether host's leading label +// 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(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 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. +// +// 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 "" } @@ -123,18 +144,21 @@ 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 "" } + 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 + // 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 } @@ -163,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, @@ -183,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 @@ -237,6 +285,16 @@ 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) @@ -298,9 +356,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, 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 @@ -313,10 +371,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 diff --git a/internal/cyberark/servicediscovery/discovery_test.go b/internal/cyberark/servicediscovery/discovery_test.go index 61fe7787..4cc62178 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" @@ -12,6 +13,94 @@ 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_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)) + }) + } +} + +// 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", + "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) + + // 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) + 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 @@ -66,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) @@ -93,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) @@ -117,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) @@ -139,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) @@ -163,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) @@ -187,12 +281,42 @@ 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) }) + 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, 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) + }) + for name, testSpec := range tests { t.Run(name, func(t *testing.T) { logger := ktesting.NewLogger(t, ktesting.DefaultConfig) @@ -210,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 b784d8a4..adcb56e3 100644 --- a/internal/cyberark/servicediscovery/mock.go +++ b/internal/cyberark/servicediscovery/mock.go @@ -49,22 +49,28 @@ type mockDiscoveryServer struct { // supplied in `services`. // Other subdomains, can be used to trigger various failure responses. // +// 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 { 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(), - } + + mds := &mockDiscoveryServer{t: t, successResponse: buf.String()} server := httptest.NewTLSServer(mds) t.Cleanup(server.Close) + + allowLoopbackHosts = true + t.Cleanup(func() { allowLoopbackHosts = false }) + t.Setenv("ARK_DISCOVERY_API", server.URL) + httpClient := server.Client() httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext) return httpClient 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) 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{