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

[roeezis]

Summary

Two hardening fixes, plus a fix for the underlying trust-chain gap in "Broken Trust Chain in Tenant Discovery Bootstrap":

• Discovery host allowlist (Medium): service discovery's identity/discoverycontext/secrets_manager API hosts were 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 that host — no allowlist. Now constrains each discovered host to a known CyberArk root domain (commercial + gov-cloud), or to the same host the discovery call itself succeeded against (covers ARK_DISCOVERY_API dev/CI overrides and same-origin test mocks with no per-env map needed). A dropped host is treated the same as one absent from the response. This is defence-in-depth against our own discovery service misbehaving (compromised, buggy, or returning a host influenced by something else) — the discovery call is already HTTPS against system roots, so it doesn't defend against a network-level attacker who could equally intercept whichever host the allowlist permits instead.
• HTTPS-only: discovery-derived hosts must now use https. Also closes the loopback-misconfiguration shape of the "same host as discoveryHost" allowance above — a rogue ARK_DISCOVERY_API pointed at a plain-HTTP 127.0.0.1 server can't present a certificate this client's TLS verification will accept once HTTPS is required. (Note: someone who can set ARK_DISCOVERY_API on the agent already controls the pod spec and therefore the SA token file itself — /var/run/secrets/tokens/jwt — so this guards against a plain-HTTP dev configuration reaching production, not against that attacker gaining anything they didn't already have.) Key/fingerprint pinning is a separate, larger design decision and not part of this PR.
• Conjur error body (Low): the authn-jwt exchange error included up to 4KiB of Conjur's response body, which propagates to a Kubernetes Pod Event — readable by anyone with get events in the namespace. Now logged at V(2) instead; the error/Event carries only the status code and the existing troubleshooting hint.

Test plan

• go vet ./...
• make test-unit — 434 tests, 4 skipped, 2 pre-existing unrelated failures (KUBEBUILDER_ASSETS-gated test not run via make's own env in this invocation path is unaffected; the json.RawMessage/jsontext.Value message-drift failure in pkg/client is pre-existing on master, unrelated to this change)
• golangci-lint clean
• New tests: servicediscovery host-allowlist accept/reject cases (incl. gov-cloud domains and plain-HTTP rejection); conjur error-body-omitted-from-returned-error case, now also asserting the body is still present in the V(2) log line

[roeezis]

Follow-up commit: the allowlist added above was copied from the identity authorizer Lambda's local commercial-only clone of `everest_env_utils`'s `ROOT_DOMAIN` map. Checked the real package (`everest_env_utils_cyberark` v2.0.117, installed in `discoverycontext-file-ingestor-service`'s venv) and found 8 more entries for the GOV_* environments that the Lambda's local clone omits. Added them — without this, any gov-cloud tenant's agent would have every discovery-derived host rejected (fatal for identity, since it's required unconditionally).

[roeezis]

Added HTTPS-only enforcement on discovery-derived hosts (commit above). Not bundling key/fingerprint pinning — that needs an onboarding-time design decision (out-of-band pin distribution + a key-rotation story), not a mechanical fix; will file separately.

[wallrj-cyberark]

Review of the discovery host allowlist and the Conjur error-body change. The direction is right and the tests are good. Seven points below, two of which are availability risks rather than security ones.

Two findings could not be anchored inline because they are outside the diff:

• internal/envelope/keyfetch/client.go:147 still does fmt.Errorf("unexpected status code %d from %s: %s", resp.StatusCode, endpoint, string(body)). That is the same class of leak as CP-25964, against the discovery-context host, and it is not addressed here. It does not reach a Pod Event (only postData failures go through eventf), so it is lower risk, but it is worth either fixing alongside or noting explicitly as out of scope so the next reader does not assume the class is closed.
• Residual on the allowlist itself: cyberark.cloud admits every tenant's host, so a tampered response can still redirect the SA-token POST to another tenant's secretsmgr/id host. That is a much smaller blast radius than arbitrary SSRF, but the response already carries tenant_id/subdomain and the client already knows c.subdomain, so tenant-level pinning is at least worth recording as a deliberate non-goal in the comment.

[wallrj-cyberark]

A rejected identity_administration host is fatal, and the error the operator sees says the wrong thing.

When sanitizeServiceAPI drops the identity URL, identityAPI becomes "" and the check below returns didn't find identity_administration in service discovery response, which may indicate a suspended tenant. The tenant is fine; the agent simply does not recognise the root domain.

This matters because the allowlist is a hardcoded copy of a CyberArk-internal ROOT_DOMAIN map baked into a customer-deployed binary. The moment a new environment root domain is added, or a tenant's identity API moves, every agent in that environment fails at startup and the only accurate signal is a separate Info line. The response already demonstrates that CyberArk emits service hosts outside these roots — testdata/discovery_success.json.template has idaptive_risk_analytics on *.analytics.idaptive.qa and fmcdp on tagtig.io.

At minimum, distinguish the two cases so the error names the real cause. Consider also whether dropping identity should be fatal at all, or whether an escape hatch (an extra-allowed-domain env var) is warranted given the agent cannot be updated on CyberArk's release cadence.

[roeezis]

Fixed in 6fea542. Split the fatal-identity error into two cases: absent from the response (kept the existing "suspended tenant" message) vs. present but rejected by the allowlist (new message naming the rejected host explicitly). Confirmed the testdata fixture does carry out-of-root-domain services today (idaptive.qa, tagtig.io) — didn't spot that when I wrote the allowlist. Not adding an escape-hatch env var for extra allowed domains without discussing it first; that's a real widening of the trust boundary, not a mechanical fix.

[wallrj-cyberark]

The host comparison is case-sensitive, but net/url does not normalise host case.

url.Parse lowercases the scheme but leaves the host exactly as written — confirmed:

url.Parse("HTTPS://Tenant.CyberArk.Cloud/api") -> scheme="https" hostname="Tenant.CyberArk.Cloud"

So a discovery response carrying any uppercase in the hostname fails both host == root and strings.HasSuffix(host, "."+root) and is dropped. DNS is case-insensitive, so such a response is entirely legitimate. For identity_administration the result is a hard startup failure with the misleading "suspended tenant" error.

It fails closed rather than open, so this is availability rather than a bypass, but it is a one-line fix.

Suggested change

	if host == root || strings.HasSuffix(host, "."+root) {
	for _, root := range allowedRootDomains {
	if strings.EqualFold(host, root) || strings.HasSuffix(strings.ToLower(host), "."+root) {
	return true
	}
	}

(the same normalisation should be applied to the host == discoveryHost comparison above)

[roeezis]

Confirmed: url.Parse lowercases the scheme but not the host. Fixed in 6fea542 — strings.EqualFold for the host==discoveryHost case, lowercased comparison against allowedRootDomains for the rest. Added a test with a mixed-case identity host.

[wallrj-cyberark]

The same-host escape hatch compares hostname only, so the port is unconstrained — and the doc comment's justification does not hold for an HTTP override.

Two separate points:

1. discoveryHost is u.Hostname(), which drops the port. With ARK_DISCOVERY_API=https://127.0.0.1:8443, a tampered response naming https://127.0.0.1:9443 for secrets_manager is accepted, and the agent POSTs its SA token to a different listener on the same host. Comparing u.Host (host:port) instead would cost nothing and close it — the mock server would still pass its own URL through, it just would not pass sibling ports.

2. The comment says discoveryHost is "the host we just made a successful, TLS-authenticated discovery call to". ARK_DISCOVERY_API may be plain http://, in which case the discovery call was never TLS-authenticated at all, and the HTTPS check on the service URL does not retroactively authenticate the discovery host. It also is not necessarily the host the call succeeded against, since http.Client follows redirects by default and this is the requested host, not the final one. That direction is fail-safe, but the comment as written will mislead the next reader.

[roeezis]

Both points are correct, and I looked at fixing the port comparison directly in this PR before deciding not to. A host:port check here would break every test currently relying on the same-host-different-port shape of this escape hatch (dataupload/conjur/identity mocks each on their own port, matched only by hostname today) -- fixing that cleanly needs the same mock-transport rework already in #830 (stacked on this branch), which removes the escape hatch entirely rather than tightening it. Doing a partial fix here and then deleting it in #830 seemed worse than just pointing at #830. Corrected the doc comment in 6fea542 to stop overclaiming TLS-authentication and to say this plainly. If you'd rather see #829 stand on its own without depending on #830 landing, say so and I'll patch it here too.

[wallrj-cyberark]

Dropping discoverycontext makes an already-poor downstream failure newly reachable.

The comment below correctly notes that keyfetch does not check DiscoveryContext.API. Previously an empty value only happened for an absent or inactive service; now a live-but-unrecognised host produces the same empty string. The resulting failure in keyfetch.FetchKey is opaque — verified:

url.JoinPath("", "discovery-context/jwks") -> "discovery-context/jwks", err=nil
http.NewRequest(...)                        -> err=nil
client.Do(...)                              -> Get "discovery-context/jwks": unsupported protocol scheme ""

An operator sees unsupported protocol scheme "" with nothing tying it back to a dropped discovery host. Adding the missing empty check in keyfetch.FetchKey (matching NewDatauploadClient's) would make the new failure mode self-explanatory.

[roeezis]

Confirmed the failure mode (unsupported protocol scheme ""). Fixed in 6fea542 -- FetchKey now checks services.DiscoveryContext.API == "" explicitly, mirroring NewDatauploadClient's existing check on the same field, with a new test covering it.

[wallrj-cyberark]

Swapping .invalid for a name in a live CyberArk zone gives up a hermeticity guarantee.

example.invalid is reserved by RFC 6761 and is guaranteed never to resolve, so if any code path ever dialled it the test failed instantly and locally. identity.example.integration-cyberark.cloud sits under a zone that resolves today:

$ getent hosts integration-cyberark.cloud
1.1.1.1  integration-cyberark.cloud

The specific label does not resolve right now, but nothing stops a wildcard or a new record appearing, at which point a regression that used to fail fast instead makes real network egress from CI to CyberArk infrastructure.

Since MockDiscoveryServer sets ARK_DISCOVERY_API to its own https://127.0.0.1:PORT, the same-host clause already accepts any 127.0.0.1 URL. Pointing these placeholders at an unused loopback port keeps them unroutable off-box and needs no change to the allowlist. The same applies to internal/cyberark/client_test.go:44 and internal/envelope/keyfetch/client_test.go:35, :291, :337.

[roeezis]

Fixed in 6fea542, and the same swap at the other three locations you named. Used a loopback literal (https://127.0.0.1:1) instead -- it passes the allowlist via the same-host-as-discovery escape hatch (MockDiscoveryServer's own ARK_DISCOVERY_API is also 127.0.0.1) without depending on integration-cyberark.cloud staying free of a record at this exact label.

[roeezis]

On the tenant-scoping residual (cyberark.cloud admits every tenant's host): added a warn-only check in 83c0913 (stacked in #830). sanitizeServiceAPI now logs when a discovery-derived host's leading label doesn't match the caller's own subdomain, using the same two-shape host-matching pattern used elsewhere for inbound host-binding checks.

Not enforcing yet: I only have live evidence of the dot shape ({subdomain}.{service}.{domain}) for the three services this agent actually calls -- id/inventory/secretsmgr, observed in every mock and one live-verified tenant. The hyphen shape is confirmed for a different service, not necessarily these three. Failing closed on an unverified shape risks breaking real agents in a combination I haven't observed, so this ships as telemetry first -- coming back to decide on enforcement once it's confirmed.

On keyfetch/client.go:147 -- fixed in 6fea542 rather than deferred, same commit as the other 5 points.

[wallrj-cyberark]

One nit on the follow-up commit. Everything else from the previous round looks properly addressed — thanks for chasing the gov-cloud domains and for the 127.0.0.1:1 placeholder, which is a better answer than the one I suggested.

[wallrj-cyberark]

I dispute this last clause — the codebase already has the helper.

pkg/agent/config_test.go:1040:

func recordLogs(t *testing.T) (logr.Logger, ktesting.Buffer) {
	log := ktesting.NewLogger(t, ktesting.NewConfig(ktesting.BufferLogs(true)))
	testingLogger, ok := log.GetSink().(ktesting.Underlier)
	require.True(t, ok)
	return log, testingLogger.GetBuffer()
}

Six tests in that file already assert against the buffer with gotLogs.String(). It is three lines, not a gap in the tooling.

I wrote the assertion for this test to check I was not talking nonsense, and it passes:

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, buf.String(), "authn-jwt exchange rejected")
require.Contains(t, buf.String(), "CONJ00001E Invalid JWT token")

--- PASS: TestScratch_BodyIsLoggedAtV2 (0.01s)
    captured buffer:
    INFO authn-jwt exchange rejected statusCode=401 body="{\"error\":{\"message\":\"CONJ00001E Invalid JWT token\"}}"

Two things that are genuinely non-obvious, and are probably what you hit:

• conjur.go:100 logs via klog.FromContext(ctx), so the logger has to reach it through the request context — klog.NewContext(t.Context(), logger). Passing it any other way silently captures nothing and the buffer just comes back empty.
• You need ktesting.Verbosity(2), otherwise the V(2) line is filtered out and, again, you get an empty buffer rather than a failure that tells you why.

Worth doing, because it closes the actual loop: right now the test proves the body is absent from the error, but nothing proves it is still present in the log. Someone deleting the klog line to "fix" a leak would keep this test green and quietly remove the operator's only way to see Conjur's response.

If you would rather not, that is fine — but then please reword rather than leave the claim, since the next person will believe it:

Suggested change

	// here — ktesting has no easy log-buffer assertion in this codebase.
	// here.

[roeezis]

Verified — wrong. pkg/agent/config_test.go:1040's recordLogs helper does exactly this, six tests already use it. Added the same pattern here: TestAuthenticateRequest_ExchangeErrorOmitsConjurResponseBody now asserts the body is still present in the V(2) log line via ktesting.BufferLogs, not just absent from the returned error. Reworded the comment to state that rather than the false claim.

[wallrj-cyberark]

Coming back on the allowlist with a threat-model question rather than a code one. I am not asking you to remove it — I am asking that the description stop claiming more for it than it delivers, because that shapes what gets built next.

[wallrj-cyberark]

Which attacker does this stop? I can only make it bind against one, and it is not the one the description implies.

The discovery call is already HTTPS against system roots, so work through who can actually change what comes back:

1. Network attacker with no trusted certificate. They cannot alter the discovery response at all — TLS stops them. The allowlist has nothing to defend.
2. Network attacker who can pass TLS for the discovery host (misissued certificate, an extra CA in the pod trust store, an intercepting proxy). They tamper with the response — and they can equally intercept the connection to whichever host the allowlist permits. They name a genuine *.cyberark.cloud host and intercept that too. The allowlist buys nothing.
3. The discovery service itself returns a host it should not — compromised, or buggy, or reflecting input someone else influenced. TLS is intact everywhere; the only lever is the response body. Here, and only here, the allowlist binds.

So this is defence-in-depth against our own service misbehaving. That is a legitimate thing to want, and worth keeping. But it is not a fix for a broken trust chain at the network layer, and the description reads as though it is.

Two consequences worth being explicit about:

Requiring HTTPS does not substitute for the allowlist. Anyone can get a publicly trusted certificate for a domain they own, so in case 3 https://evil.example/ is accepted by TLS and the agent POSTs its SA token there. The allowlist is the only thing standing in the way. Conversely, in case 2, HTTPS does not help either. The two controls are not layers on the same threat — they address different ones, and neither covers case 2.

In case 3, cyberark.cloud admits every tenant's host. A compromised discovery service does not need an outside domain; it names another tenant's id or secretsmgr host. That makes the tenant-scoping check the load-bearing control and the root-domain list the weaker one — which is the opposite of the emphasis here. I appreciate why you shipped it warn-only, and I am not arguing with that call.

Separately, on the loopback shape the comment below cites: an attacker who can set ARK_DISCOVERY_API on the agent already controls the pod's environment, and the token is /var/run/secrets/tokens/jwt in that same pod (jwtsource.DefaultTokenPath). They can read the file; they have no need to make the agent post it to them. Requiring HTTPS is still worth having — it stops a plain-HTTP dev configuration reaching production — but it should be described as protecting against misconfiguration, not against that attacker.

None of this needs a code change. What I would like is for the description and this comment to say "constrains a misbehaving discovery service" rather than implying it closes a network-level gap, so that nobody later reads the allowlist as a reason not to do the pinning work.

[roeezis]

Agreed with the threat model — walked through the same three cases and land in the same place: TLS already stops case 1, case 2 gains nothing from this check (an attacker who can defeat the discovery host's TLS can equally intercept whichever host the allowlist permits), and case 3 (our own discovery service misbehaving) is the only place it binds. Reworded the doc comment to say exactly that rather than "SSRF-shaped hole"/"tampered with", and made the tenant-scoping gap explicit there too (cyberark.cloud admits every tenant's host; hostLeadingLabelMatchesSubdomain is warn-only, not a fix for it). Also corrected the "loopback-attacker" framing in both PR descriptions to "loopback-misconfiguration" — same point applies to ARK_DISCOVERY_API: someone who can set it already has the pod's token file directly.

[wallrj-cyberark]

Approving. The allowlist and HTTPS work is sound, and the last round addressed everything I raised — the split identity error, the case-insensitive comparison, the keyfetch empty check and the 127.0.0.1:1 fixtures all look right.

Two things I would normally have asked for before merge. I am not holding this up for them, because #830 is stacked on this branch and still open, so they can land there instead. They should land, though.

1. The new identity-rejection error puts an untrusted URL into a Pod Event

internal/cyberark/servicediscovery/discovery.go:319 formats rawIdentityAPI into the returned error. That value comes straight from the discovery response body, and the error reaches the same Warning Pod Event this PR is scrubbing Conjur bodies out of: pkg/client/client_cyberark.go:79 returns it unwrapped, and pkg/agent/run.go:388 writes it with eventf("Warning", "PushingErr", ...) on every backoff retry.

So a tampered or buggy response can put text of its choosing in front of anyone with get events in the namespace, bounded only by maxDiscoverBodySize. This is the leak class the PR exists to close, reintroduced two files away. Quoting u.Hostname(), or nothing at all, avoids it — the rejected host is already in the Info log. The unparseable-URL log at :123 logs rawAPI verbatim too.

Still present in #830 at :324.

2. The upload path still embeds upstream response bodies

internal/cyberark/dataupload/dataupload.go:177 and :251 both still do io.ReadAll(io.LimitReader(res.Body, 500)) and put the result in the error, and both are on that same PushingErr path. Worth either giving them the same treatment or saying in the description that they are out of scope, so the class is not recorded as closed when the most reachable sites are untouched. Still present in #830.

Already handled in #830

Flagging so they are not raised again: the V(2) buffer assertion landed in 1000d8f, and the stale "keyfetch's client doesn't check discoveryContextAPI" sentence is gone. Thank you for both.

One deliberate call worth recording

identity_administration is required unconditionally and now allowlist-gated, but on the Conjur JWT path it is never dialed. So a tenant whose identity host sits on a root domain missing from the hardcoded list gets a hard outage on a path that never needed that host, with no override and no fix until a new agent release ships. You have decided knowingly to keep it fatal and I am not reopening it — but please make sure it is written down somewhere an on-call engineer will find it, because the symptom will not point at the allowlist.

Six smaller observations, none blocking, listed so they are not lost

• Startup key-fetch failure disables secret encryption for the pod lifetime. pkg/agent/run.go:175 runs loadEncryptor once and sets encryptSecrets = false on failure with no retry. A dropped secrets_manager or rejected identity host is a new trigger for that. Uploads keep succeeding without secret data until the pod restarts, and nothing alerts. Pre-existing shape, but this PR widens the trigger.

• A dropped host is cached as empty for an hour, with the reason logged once. The rejection produces a single Info line at drop time; for the next hour every consumer error says only that the API is empty. An operator may reasonably conclude the tenant is not onboarded. Either carry the reason in Services or re-log on each cached return.

• One error string covers three rejection reasons. "is not on an allowed CyberArk domain over HTTPS" also covers the unparseable-URL and empty-host branches, so a malformed URL such as https:///tenant is reported as an allowlist failure. Returning (string, error) from sanitizeServiceAPI would remove both that ambiguity and the need for rawIdentityAPI, which also fixes point 1 above.

• The JWKS V(2) log caps at 1 MiB where Conjur caps at 4 KiB. internal/envelope/keyfetch/client.go:163 logs up to maxResponseBytes; conjur.go:96 uses 4*1024. Worth matching the smaller cap and draining the rest.

• The host allowlist does not strip a trailing dot. ajp5871.id.cyberark.cloud. matches neither the equality nor the suffix test, and Go's TLS stack accepts absolute FQDNs. No such host appears in testdata, so the trigger is unevidenced. strings.TrimSuffix(host, ".") closes it.

• Settled earlier, not reopening: the same-host escape hatch and the 127.0.0.1:1 fixtures both go with #830, and the -v=2 hint is valid — it is the preserved shorthand for --log-level, which I withdrew on the other PR.