CP-26002: validate ARK_DISCOVERY_API itself against the domain allowlist

[roeezis]

Summary

Stacked on #829 — please review/merge that first. This PR's diff includes #829's changes until it merges; the incremental change is the base-URL validation commit + the mock-transport rework it needed.

#829 allowlisted identity/discoverycontext/secrets_manager hosts, but had a carve-out: any host equal to the discovery endpoint's own host (ARK_DISCOVERY_API, if overridden) was trusted automatically, since that override's own host was never checked. That carve-out could let ARK_DISCOVERY_API pointed at a rogue server be trusted as the root of the whole discovery response. Worth being precise about the actual attacker here: someone able to set ARK_DISCOVERY_API on the agent (tampered pod spec/Helm values) already controls the pod, and therefore already has direct access to the SA token file (/var/run/secrets/tokens/jwt) — they don't need the agent to POST it anywhere. What this control actually defends is a misbehaving/compromised discovery service returning a bad host in an otherwise-intact TLS session, not a network-level attacker (who could equally intercept whichever host the allowlist permits instead — TLS already stops a plain MITM, and someone who can defeat that TLS session gains nothing extra from this check). HTTPS-only (also in #829) closed the loopback-misconfiguration shape of this (no valid cert for 127.0.0.1); this PR closes the remaining shape where the rogue host has a real, globally-trusted certificate.

DiscoverServices now requires its own base URL to be HTTPS on an allowed CyberArk domain too — collapsing the "same host as discoveryHost" carve-out into "host is on the allowlist" (isAllowedServiceHost is gone; only hostOnAllowedRootDomain remains).

Test infrastructure: this broke every test that feeds a real httptest mock address into a Services value or ARK_DISCOVERY_API (127.0.0.1 isn't on the allowlist either). Fixed with a small shared test-only registry (internal/cyberark/testing/mockdial.go): servicediscovery.MockDiscoveryServer launders any loopback address it's given (including its own ARK_DISCOVERY_API override) into a fake CyberArk-domain-looking hostname, registering a dial redirect to the real address. conjur/dataupload/identity's Mock*Server helpers wrap their own returned clients the same way, since tests freely reuse one mock's client to call a different mock's server — any of them might end up being the one that has to resolve a fake host registered elsewhere.

Key/fingerprint pinning is a separate, larger design decision and not part of this PR.

Also includes a warn-only tenant-subdomain check (hostLeadingLabelMatchesSubdomain) added in response to review — the root-domain allowlist above still admits any tenant's host on an allowed domain, which a misbehaving discovery service could exploit to redirect within-domain to a different tenant. Not enforced yet: live evidence only covers one of the two host-shapes for the three services this agent actually calls.

Test plan

• go vet ./...
• make test-unit — 434 tests, 4 skipped, 1 pre-existing unrelated failure (json.RawMessage/jsontext.Value message-drift in pkg/client, pre-existing on master)
• golangci-lint clean

[wallrj-cyberark]

Review of the CP-26002 commit and the test-infrastructure rework it needed. The security logic itself reads correctly: the userinfo trick (https://good.cyberark.cloud@attacker.example/) is handled by u.Hostname(), scheme comparison is safe because url.Parse lowercases it, and the mock-dial registry is order-independent because the map is consulted at dial time. go vet, make test-unit equivalents and go test -race ./internal/... are all clean on my checkout.

Eight findings below. The one I would act on before merge is the case-sensitive host comparison; the rest are smaller, and three concern the new test helpers rather than shipped behaviour.

[wallrj-cyberark]

The allowlist match is case-sensitive, and rejects a trailing-dot FQDN.

url.Parse lowercases the scheme but leaves the host exactly as written, so https://AJP5871.id.cyberark.cloud gives Hostname() == "AJP5871.id.cyberark.cloud" and fails both the equality test and the HasSuffix test. Hostnames are case-insensitive, so this is a false rejection.

It fails closed, which is the right direction, but the blast radius on the identity path is large. A mixed-case identity host is dropped, identityAPI ends up empty, and DiscoverServices aborts for every auth method, including Conjur, which never touches that host. The same applies to an operator-typed ARK_DISCOVERY_API containing a capital letter, which the new base-URL check now refuses outright.

A trailing dot has the same problem: https://platform-discovery.cyberark.cloud./ is a legal absolute name, and some operators use it to skip search-domain resolution. "platform-discovery.cyberark.cloud." does not end in ".cyberark.cloud", so it is refused.

Normalising once at the top fixes both:

Suggested change

	// hostOnAllowedRootDomain reports whether host is, or is a subdomain of, one
	// of allowedRootDomains.
	func hostOnAllowedRootDomain(host string) bool {
	for _, root := range allowedRootDomains {
	if host == root || strings.HasSuffix(host, "."+root) {
	return true
	}
	}
	return false
	}
	// 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, "."))
	for _, root := range allowedRootDomains {
	if host == root || strings.HasSuffix(host, "."+root) {
	return true
	}
	}
	return false
	}

[roeezis]

Case-sensitivity was already fixed in a prior commit — this comment is against the original diff before that landed. Trailing-dot handling was genuinely missing though; accepted your suggestion for that part (strings.TrimSuffix(host, ".")), added a test case for it.

[wallrj-cyberark]

A rejected identity host reports itself as a suspended tenant.

When sanitizeServiceAPI drops the identity URL, identityAPI becomes "" and line 301 returns didn't find identity_administration in service discovery response, which may indicate a suspended tenant. The response did contain it; we refused it. An operator following that message will go and look at tenant status, not at this allowlist.

This is not hypothetical. Commit c50afe1 in this PR fixes exactly that scenario for gov-cloud: without those eight domains, every gov tenant's agent would have died with a "suspended tenant" message. The list is hardcoded and copied by value, so the next new environment root domain will reproduce it.

Two things worth doing: have sanitizeServiceAPI report whether it dropped something, and return a distinct error for "the identity host was refused by the allowlist" naming the host. It also means an unlisted identity domain takes out the Conjur JWT path, which does not use that host at all — worth a comment either way if that coupling is deliberate.

[roeezis]

Also stale against the original diff — a later commit already split the fatal error into "absent from the response" (kept the existing message) vs. "present but rejected by the allowlist" (new message naming the rejected host). On the coupling point: yes, deliberate — the identity host and the Conjur exchange host are unrelated services, an unlisted identity domain shouldn't take out a Conjur-only agent, but identity is currently required unconditionally regardless of auth method. Worth a closer look at whether that's still the right call, but a bigger change than this PR.

[wallrj-cyberark]

Nothing tests this check — the headline control of the PR is uncovered.

Every test reaches DiscoverServices through MockDiscoveryServer, which now always sets ARK_DISCOVERY_API to https://mock-N.integration-cyberark.cloud. No test sets it to a value that should be rejected: grep -n Setenv internal/cyberark/servicediscovery/*_test.go returns nothing. I deleted these three lines locally and the whole suite still passes.

The five new subtests all cover sanitizeServiceAPI (the response-derived hosts), not c.baseURL. A small table test that calls t.Setenv("ARK_DISCOVERY_API", …) directly with http://platform-discovery.cyberark.cloud/, https://attacker.example/ and a bare hostname with no scheme would pin the behaviour and stop a future refactor (for instance moving the check above the cache lookup) from silently removing it.

[roeezis]

Confirmed the gap — added a test that sets ARK_DISCOVERY_API directly to a disallowed value (plain HTTP, disallowed domain, no scheme) and checks DiscoverServices errors, independent of MockDiscoveryServer's own allowed default.

[wallrj-cyberark]

Nil dereference if TLSClientConfig is nil.

(*tls.Config).Clone() returns nil for a nil receiver (crypto/tls/common.go:996), so line 44 panics rather than doing anything useful.

Every caller today uses httptest.NewTLSServer, whose client transport carries a non-nil config, so it works. But the doc comment above says "Every package's Mock*Server should call this on its returned client's transport", and a client from a plain httptest.NewServer is &http.Client{Transport: &http.Transport{}} — nil TLSClientConfig. The first plain-HTTP mock someone adds gets a nil-pointer panic here instead of a usable failure.

Guard it (needs a crypto/tls import):

if transport.TLSClientConfig == nil {
    transport.TLSClientConfig = &tls.Config{}
} else {
    transport.TLSClientConfig = transport.TLSClientConfig.Clone()
}
transport.TLSClientConfig.InsecureSkipVerify = true

[roeezis]

Confirmed and fixed — guard the nil case before calling Clone().

[wallrj-cyberark]

This InsecureSkipVerify = true lives in a package the shipped agent imports.

mockdial.go is a normal build file, not _test.go, and internal/cyberark/servicediscovery/mock.go imports it. So pkg/client -> servicediscovery -> internal/cyberark/testing is a compile-time dependency of the agent binary. I checked a go build of the repo root: net/http/httptest.init.0, net/http/httptest.serveFlag and internal/cyberark/testing.mockHostAddr are all in the symbol table. WrapMockTransport itself is pruned, but that is the linker's dead-code elimination, not a build constraint — an indirect reference is all it takes to retain it.

For a PR whose whole purpose is answering a pentest finding about the TLS trust chain, it is worth keeping a "disable certificate verification" helper out of the shipped build entirely — a //go:build tag on the mock files, or moving them behind _test.go / export_test.go.

Worth knowing that nothing will catch a regression here: gosec is enabled in .golangci.yaml, but the exclusion block lists gosec with text: .*, so G402 never fires.

[roeezis]

Confirmed with a symbol check on the built binary — mockHostAddr and httptest.serveFlag are both there. Not fixing in this PR: solving it properly needs the test-only helpers behind a build tag threaded through the Makefile/CI test invocation, which touches four packages' worth of pre-existing mock helpers (not introduced by this branch) and is a bigger infrastructure change than fits in a security-hardening stack. Filing separately. Also confirmed your read of the lint config: the gosec exclusion rule has no path scoping, so it's silenced repo-wide, not just here — also worth its own fix, also out of scope for this PR.

[wallrj-cyberark]

The server starts accepting connections before successResponse is written.

httptest.NewTLSServer(mds) on line 98 spawns the accept loop, and mds.successResponse is only assigned on line 118. ServeHTTP reads that field from the server goroutine, so this is an unsynchronised write to state a live server already owns.

Nothing can reach it today, because the caller does not yet know the address, so -race stays quiet (I ran go test -race ./internal/... and it is clean). But the reordering buys nothing: neither launderIfLoopback nor the template execution needs the server. Building the response first removes the hazard for free.

Suggested change

	mds := &mockDiscoveryServer{t: t}
	server := httptest.NewTLSServer(mds)
	t.Cleanup(server.Close)
	httpClient := server.Client()
	baseTransport := httpClient.Transport.(*http.Transport).Clone()
	cyberarktesting.WrapMockTransport(baseTransport)
	discoveryFakeHost := fmt.Sprintf("mock-%d.integration-cyberark.cloud", fakeHostCounter.Add(1))
	cyberarktesting.RegisterMockHost(discoveryFakeHost, mustHostPort(t, server.URL))
	t.Setenv("ARK_DISCOVERY_API", "https://"+discoveryFakeHost)
	services.Identity.API = launderIfLoopback(services.Identity.API)
	services.DiscoveryContext.API = launderIfLoopback(services.DiscoveryContext.API)
	services.SecretsManager.API = launderIfLoopback(services.SecretsManager.API)
	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(),
	}
	server := httptest.NewTLSServer(mds)
	t.Cleanup(server.Close)
	t.Setenv("ARK_DISCOVERY_API", server.URL)
	httpClient := server.Client()
	httpClient.Transport = transport.NewDebuggingRoundTripper(httpClient.Transport, transport.DebugByContext)
	mds.successResponse = buf.String()
	httpClient.Transport = transport.NewDebuggingRoundTripper(baseTransport, transport.DebugByContext)
	services.Identity.API = launderIfLoopback(services.Identity.API)
	services.DiscoveryContext.API = launderIfLoopback(services.DiscoveryContext.API)
	services.SecretsManager.API = launderIfLoopback(services.SecretsManager.API)
	tmpl := template.Must(template.New("mockDiscoverySuccess").Parse(discoverySuccessTemplate))
	buf := &bytes.Buffer{}
	if err := tmpl.Execute(buf, services); err != nil {
	panic(err)
	}
	mds := &mockDiscoveryServer{t: t, successResponse: buf.String()}
	server := httptest.NewTLSServer(mds)
	t.Cleanup(server.Close)
	httpClient := server.Client()
	baseTransport := httpClient.Transport.(*http.Transport).Clone()
	cyberarktesting.WrapMockTransport(baseTransport)
	discoveryFakeHost := fmt.Sprintf("mock-%d.integration-cyberark.cloud", fakeHostCounter.Add(1))
	cyberarktesting.RegisterMockHost(discoveryFakeHost, mustHostPort(t, server.URL))
	t.Setenv("ARK_DISCOVERY_API", "https://"+discoveryFakeHost)
	httpClient.Transport = transport.NewDebuggingRoundTripper(baseTransport, transport.DebugByContext)

[roeezis]

Confirmed and fixed as suggested — build the response before starting the server.

[wallrj-cyberark]

Forcing the scheme to https will hide a plain-HTTP mock behind a confusing TLS error.

Every mock in the tree is an httptest.NewTLSServer, so u.Scheme is already https and line 68 is a no-op today. The moment someone passes a plain httptest.NewServer URL, though, the laundered value claims HTTPS and the client attempts a TLS handshake against a plain-HTTP listener. That surfaces as tls: first record does not look like a TLS handshake from a call site unrelated to the mock that caused it.

Keeping the original scheme is both safer and a truer test: a plain-HTTP endpoint should be dropped by sanitizeServiceAPI, which is precisely the CP-23593 rule this PR is enforcing.

Suggested change

	fakeHost := fmt.Sprintf("mock-%d.integration-cyberark.cloud", fakeHostCounter.Add(1))
	cyberarktesting.RegisterMockHost(fakeHost, u.Host)
	u.Host = fakeHost
	u.Scheme = "https"
	return u.String()
	fakeHost := fmt.Sprintf("mock-%d.integration-cyberark.cloud", fakeHostCounter.Add(1))
	cyberarktesting.RegisterMockHost(fakeHost, u.Host)
	u.Host = fakeHost
	return u.String()

[roeezis]

Fixed as suggested — keep the original scheme instead of forcing https.

[wallrj-cyberark]

This agent does not document a -v flag; it is --log-level.

pkg/logs/logs.go:104 deliberately renames --v to --log-level with -v kept only as a shorthand, and the comment there explains why. Both charts document the long form: deploy/charts/venafi-kubernetes-agent/values.yaml:200 and deploy/charts/disco-agent/values.yaml:240 show - --log-level=6 # To enable HTTP request logging.

-v=2 does still parse, so this is not broken. But this hint is read out of a Kubernetes Pod Event by an operator who will then go and edit extraArgs in the chart, where -v appears nowhere.

Suggested change

	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)
	return "", fmt.Errorf("authn-jwt exchange rejected (%d); verify service_id, the authenticator is enabled, and the SA token audience is 'conjur' (run with --log-level=2 to see Conjur's response body)",
	resp.StatusCode)

[wallrj-cyberark]

Withdrawing this one — I was wrong, -v=2 works and the message is fine as written.

pkg/logs/logs.go renames the flag but keeps the shorthand:

if f.Name == "v" {
    f.Name = "log-level"
    f.Shorthand = "v"

So -v survives the rename. Confirmed against the built binary:

$ go run . agent --help
  -v, --log-level Level   number for the log level verbosity. 0=Info, 1=Debug, 2=Trace. ...

and -v=2 parses at runtime (I ran it with a deliberately bogus second flag, and the only complaint was about the bogus flag). Sorry for the noise — no change needed here.

[wallrj-cyberark]

I have rebased this branch onto master and force-pushed it (aac6b1b -> e8b6cd1). Apologies for touching your branch without asking first — #829 was squash-merged, so this PR was showing its parent commits back as its own diff and GitHub could not work out whether it merged.

I rebased only the four commits that belong to this PR, with git rebase --onto origin/master 6fea5423, so the squashed #829 content came from master rather than being replayed. There were no conflicts, and I resolved nothing by hand.

The one content change is that the branch now picks up #828, which landed on master after you branched: an eight-line comment change in pkg/client/client_cyberark_test.go. I diffed it against the merged commit and it is identical, so nothing of mine is in the tree.

Before pushing I checked go build ./..., go vet ./internal/... ./pkg/testutil/..., and go test across ./internal/... and ./pkg/testutil/... — all pass. pkg/client still has only the pre-existing KUBEBUILDER_ASSETS failure, and both tests #828 touched pass.

The diff is now 9 files rather than 13, which should be the real scope of this PR. My earlier review comments still stand and the two follow-ups recorded on #829 — the untrusted URL in the identity-rejection error, and the response bodies in the upload path — are both still open here.

[wallrj-cyberark]

Three blockers, then seven things worth changing that are not blocking. Detail is inline; this is the summary.

Blockers

1. The new base-URL rejection error prints ARK_DISCOVERY_API verbatim, and that error reaches Pod Events. Any credentials in the URL become readable by anyone with get events. It is the same leak class this PR closes for the Conjur and JWKS bodies.
2. The tenant-subdomain warning can never match for identity_administration. It logs at V(0) on every agent, every hour. Operators will learn to ignore it, and promoting it to enforcement as written would fail every tenant closed.
3. Test_DiscoverServices_RejectsDisallowedBaseURL passes with the guard deleted. I deleted the guard and reran it to check.

All three are small fixes.

Not blocking

Inline: the fake-host registry (I think it can be much smaller, and I have a working patch), the InsecureSkipVerify and real-dial fallthrough, the duplicated laundering in MockDiscoveryServer, the redirect gap, and the dropped provenance note.

Two more have no line in this diff to hang off:

Stale comments describing the escape hatch you removed. pkg/testutil/envtest.go:294-297 and internal/envelope/keyfetch/client_test.go:36-37 still say https://127.0.0.1:1 passes the allowlist "via the same-host-as-discovery escape hatch", and that MockDiscoveryServer's ARK_DISCOVERY_API is also 127.0.0.1. Both are false now. It only passes because launderIfLoopback rewrites it. A reader either concludes production still trusts any host equal to the discovery host, or deletes launderIfLoopback and breaks every FakeCyberArk and keyfetch test. internal/cyberark/client_test.go:42-45 defers to the envtest.go comment, so it goes stale with it. One line naming the real mechanism would do.

A bad ARK_DISCOVERY_API is a retrying network error, not a startup config error. servicediscovery.New reads the environment variable and cannot return an error, and ValidateAndCombineConfig never calls DiscoverServices. So an operator typo of http:// starts the agent, passes config validation, then fails on the first push, emits PushingErr events for ten minutes, exits 1 and crash-loops. A genuine config error is reported immediately as "While evaluating configuration". New has one production caller, which already returns an error, so making it return (*Client, error) is about ten one-line edits. Keep the runtime check as defence in depth.

How I verified these, and what I could not verify

Checked against e8b6cd1.

Blocker 3 was confirmed empirically. I removed the if u.Scheme != "https" || !hostOnAllowedRootDomain(...) block and reran:

--- PASS: Test_DiscoverServices_RejectsDisallowedBaseURL (6.87s)
    --- PASS: .../host_with_no_scheme (0.00s)
    --- PASS: .../plain_HTTP (6.83s)
    --- PASS: .../disallowed_domain (0.04s)

Note the 6.83s. Without the guard that subtest makes a real outbound request to platform-discovery.cyberark.cloud from the unit-test suite.

The registry simplification was also confirmed empirically, not just read. I implemented it and ran the result: 6 files changed, +13 / -118, mockdial.go deleted, no InsecureSkipVerify left anywhere. All ten packages under ./internal/... and ./pkg/testutil/... pass, including under -race. pkg/client shows only the pre-existing KUBEBUILDER_ASSETS failure, which also fails on this PR's head unchanged.

Baseline on the current head for comparison: those same ten packages pass.

I have not run the e2e suites, so nothing here says anything about behaviour under load, retry paths, or version drift.

[wallrj-cyberark]

Blocker. This embeds the raw base URL with %q, and DiscoverServices errors travel unchanged into a Pod Event.

Set ARK_DISCOVERY_API (it arrives via secretKeyRef in deploy/charts/disco-agent/templates/deployment.yaml) to something like https://user:s3cret@proxy.internal/. pkg/client/client_cyberark.go:79-81 returns this error unchanged, and pkg/agent/run.go:388 emits it as Warning PushingErr retrying in %v after error: %s. The password is then in the Pod's events, readable by anyone with get events, repeating every 30s to 3m for ten minutes per crash loop.

That is the same class of leak this PR closes for the Conjur and JWKS response bodies, so it would be a shame to open a new one here.

u is already parsed on line 239. u.Redacted() or u.Hostname() gives the operator everything they need to debug the typo.

[roeezis]

Confirmed and fixed. Traced the path you named — pkg/client/client_cyberark.go returns it unchanged, run.go emits it as PushingErr — so a credential in ARK_DISCOVERY_API would have been in the Pod's events, repeating for the whole backoff window. The error now names only scheme and host, and the rejected value never appears in it. Same treatment applied to the identity-rejection error in the previous commit, which had the same shape.

[wallrj-cyberark]

Blocker. This warning fires for identity_administration on every real agent, because the identity host's leading label is the identity tenant ID, not the tenant subdomain.

In testdata/discovery_success.json.template the subdomain is venafi-test but the identity host is ajp5871.id.integration-cyberark.cloud. So hostLeadingLabelMatchesSubdomain returns false and this V(0) line logs on every uncached DiscoverServices call, which is hourly, on every agent. discoverycontext and secrets_manager do match (venafi-test.inventory..., venafi-test.secretsmgr...), so this is specific to identity.

Two consequences. Operators learn to ignore the line, which costs you the visibility the warning was added for. And promoting it to enforcement later, which the doc comment says is the plan, would reject the identity endpoint for every tenant.

The test comment at line 258 already notes that none of the mock URLs' leading labels match, so the check is passing vacuously in the suite too.

Suggest skipping IdentityServiceName, or comparing it against DiscoveryResponse.IdentityID instead.

Separately, hostLeadingLabelMatchesSubdomain compares label == subdomain at line 102 without folding case, while the rest of the file is careful to fold. The mixed-case host that the test at line 241 protects would be flagged for a matching tenant. strings.EqualFold there.

[roeezis]

Blocker confirmed. The fixture makes it plain: subdomain venafi-test, identity host ajp5871.id.…. That's a different identifier namespace, not a mismatch, so the check could never pass for identity — a V(0) line per uncached lookup per agent, and enforcing it later would have failed every tenant closed.

Excluded identity_administration via subdomainCheckApplies, with the reason recorded so nobody re-adds it. Chose skipping over comparing against IdentityID: the fixture's identity_id (identity-456) doesn't match ajp5871 either, so I have no evidence that field would work, and I'd rather check nothing than check the wrong thing.

Also switched the label comparison to fold case, which the rest of the file already did.

[wallrj-cyberark]

Blocker. This test passes with the guard removed, so it does not defend the thing it is named for.

I checked by deleting the if u.Scheme != "https" || !hostOnAllowedRootDomain(...) block at discovery.go:247-249 and rerunning. All three subtests still PASS. They only assert require.Error, and without the guard each one errors for an unrelated reason:

• host with no scheme fails with unsupported protocol scheme
• disallowed domain fails on DNS for attacker.example
• plain HTTP makes a real outbound GET to platform-discovery.cyberark.cloud and fails on the response, taking 6.83s of the run

That last one is worth fixing on its own account: a unit test should not be reaching the internet.

Two changes make it real. Assert the error text, require.ErrorContains(t, err, "refusing to bootstrap trust"). And pass a client whose Transport fails the test if Do is ever called, which pins the "we never dialled" property that the guard exists to provide.

[roeezis]

You're right, and checking it empirically rather than by eye caught something I'd missed entirely — the 6.83s. A unit test reaching platform-discovery.cyberark.cloud is its own bug.

Took both suggestions. The test asserts refusing to bootstrap trust, and the client's Transport is now a failOnDial that fails the test if Do is ever called, which pins the never-dialled property directly. Verified the way you did: deleted both guards and all four cases fail, with failOnDial catching the outbound request. Restored, all pass.

Since the check now also runs in New(), there's a second test covering the DiscoverServices re-check for a Client not built through New().

[wallrj-cyberark]

Not blocking, but I think this whole registry can be about eleven lines, and I would rather it were before it becomes load-bearing.

mockdial.go, launderIfLoopback, fakeHostCounter, mustHostPort and the WrapMockTransport edits in the conjur, dataupload and identity mocks can all go if instead you add a var allowLoopbackForTests bool beside allowedRootDomains, have hostOnAllowedRootDomain return true when it is set and net.ParseIP(host).IsLoopback(), and set it in MockDiscoveryServer with a t.Cleanup reset, keeping ARK_DISCOVERY_API = server.URL. One guard is enough because both check sites go through hostOnAllowedRootDomain.

I implemented this rather than just suggesting it. Result: 6 files changed, +13 / -118. No InsecureSkipVerify, no dial rewriting, no process-wide map, and the three foreign mocks revert to plain srv.Client(). All ten packages under ./internal/... and ./pkg/testutil/... pass, including under -race. The attacker.example and http:// rejection tests still fail correctly, because those are not loopback, and the real allowlist path is still exercised by the mock*APIURL constants. Happy to push the patch to the branch if useful.

One caveat, in fairness to the alternative: a bypass boolean sitting beside the allowlist it bypasses, in a file that compiles into the production binary, is the sort of thing a reviewer will object to on sight, even though it is unexported and only reachable through a function taking testing.TB. A build tag, or a comment that says plainly why it cannot be reached in production, would head that off.

[roeezis]

Taken, as your design. mockdial.go is deleted, launderIfLoopback/fakeHostCounter/mustHostPort are gone, and the conjur, dataupload and identity mocks are back to a plain srv.Client(). MockDiscoveryServer sets an allowLoopbackHosts flag with a t.Cleanup reset and keeps ARK_DISCOVERY_API = server.URL. My diff comes out at 14 files, +173/-206 — larger than your +13/-118 because it also carries the three blocker fixes.

This is strictly better than what I had, and it also resolves your earlier point about InsecureSkipVerify shipping: there's no InsecureSkipVerify, no dial rewriting and no process-wide map left anywhere in the tree. It removes the real-DNS fallthrough you raise in the comment below too, since nothing publishes a fake hostname any more.

On your own caveat about a bypass flag sitting next to the allowlist: agreed it needs to not read as a hole. It's unexported, only MockDiscoveryServer (which takes a testing.TB) sets it, and it widens the allowlist only to loopback, never to a named host — with a comment saying so. I did try a //go:build tag first and backed it out: Go has no way to make a tag default-on for tests, so every go test invocation would need -tags, and a bare go test ./... breaks with an undefined-symbol error instead of a real failure.

There are still 3 inert httptest symbols in the binary, from the mock.go files being ordinary build files. That predates this PR and is the structural half of your earlier point; worth a follow-up rather than more churn here.

[wallrj-cyberark]

If the registry stays, this is worth tightening. WrapMockTransport disables TLS verification on every mock client, and falls through to a real dial for any host that is not registered, while the mocks now publish fake hostnames under a real, CyberArk-controlled zone.

Before this PR, mock addresses were 127.0.0.1:port, so a mis-wired or unwrapped client failed fast and locally. Now ARK_DISCOVERY_API and the laundered service URLs are mock-N.integration-cyberark.cloud, which any unwrapped client resolves through real DNS. pkg/agent/run.go:301 into keyfetch.NewClient(ctx, ..., nil) into http_client.NewDefaultClient is such a path. Nothing tests loadEncryptor today, so the first agent-level test that pairs it with the CyberArk fakes would dial that zone.

I checked: mock-1.integration-cyberark.cloud currently answers NXDOMAIN, but the zone is live (platform-discovery.integration-cyberark.cloud resolves). So a wildcard record added by whoever owns the zone would turn a test into an outbound request with no change here.

Two small fixes. Use TLSClientConfig.ServerName = "example.com" instead of InsecureSkipVerify, since every httptest.NewTLSServer shares the testcert whose SANs include example.com. And have DialContext return an error for any unregistered *cyberark.cloud host rather than dialling it.

[roeezis]

Moot now — the registry is gone, so there's no InsecureSkipVerify, no dial fallthrough, and nothing publishing a fake hostname under a live zone. Mocks are plain loopback httptest servers again, so a mis-wired or unwrapped client fails fast and locally as it did before this PR. Thanks for chasing the NXDOMAIN/wildcard detail; that's what made it clear the laundering approach was the wrong trade.

[wallrj-cyberark]

Two small things here, both moot if the simplification on mockdial.go is taken.

server.URL is always https://127.0.0.1:port, so lines 114-116 are doing by hand exactly what launderIfLoopback already does. They collapse to t.Setenv("ARK_DISCOVERY_API", launderIfLoopback(server.URL)), and mustHostPort goes with them.

More substantively, this site clones the transport on line 111, but conjur/mock.go:18, dataupload/mock.go:86 and identity/mock.go:89 mutate theirs in place. httptest.Server.Client() returns the same client every call, so internal/cyberark/auth_select_test.go:71 and :100, which call conjurSrv.Client() directly, silently receive the non-verifying, dial-redirected client rather than a clean one.

A single helper in internal/cyberark/testing (say MockClient(srv) doing Client(), wrap, and the debugging round tripper) would remove all four copies and make the clone-or-mutate choice once.

[roeezis]

Both resolved by dropping the registry. launderIfLoopback and mustHostPort are gone and ARK_DISCOVERY_API is just server.URL, so there's no hand-rolled duplication left. The clone-or-mutate inconsistency goes with it: nothing wraps a transport any more, so auth_select_test.go's direct conjurSrv.Client() calls get an ordinary client rather than a silently non-verifying one. Good catch — that one was invisible from the diff.

[wallrj-cyberark]

Worth a comment rather than a change. The deleted isAllowedServiceHost doc noted that ARK_DISCOVERY_API is "not guaranteed to be the host the request actually landed on after redirects", and that this ticket was the fix for both gaps. This diff closes the scheme and domain half. grep CheckRedirect over the repo returns nothing, so the redirect half is still open, and it is no longer written down anywhere.

It is not hypothetical. The allowlist admits any tenant's host on an allowed domain, which the test at discovery_test.go:257 makes explicit. A secrets_manager host on an allowed domain that answers 307 or 308 causes Go's default client to re-send the POST body to whatever Location names, including a host outside allowedRootDomains. That body is the agent's SA JWT as a form field (conjur.go:70-72; GetBody is set because the body is a strings.NewReader). Go strips Authorization across a domain change but never strips bodies.

Either set CheckRedirect on the clients that carry credentials, to refuse or to re-validate the target host, or restore a line marking the redirect gap as still open.

[roeezis]

Went further than the comment, because the attack path you spelled out is real and I could confirm each step: GetBody is set (the body is a strings.NewReader), Go strips Authorization across a host change but never strips bodies, and grep CheckRedirect over the repo returned nothing.

conjur.New now takes a shallow copy of the caller's client and sets CheckRedirect to refuse, so a 3xx on the exchange fails instead of re-sending the SA token to whatever Location names. Shallow copy, so the Transport and its connection pool are still shared; only the exchange uses that client.

Also left the note you asked for, since this only covers the credential-carrying path: the comment on the base-URL check now says plainly that it validates the host we address, not the host that answers, and that a 3xx elsewhere can still reach an unvalidated host.

[wallrj-cyberark]

The rewritten comment reads better, but it drops the provenance paragraph, and I would keep the substance of that even if the wording goes.

The point it recorded was that this list is a copy of an authoritative upstream allowlist, and that an earlier partial source omitted the gov-cloud entries entirely. That mattered more once the list started gating the bootstrap URL, because drift is now fatal rather than degrading: when a new root domain appears upstream, every agent pointed at it fails with "refusing to bootstrap trust from it", and whoever debugs that has no pointer to where the real list lives or to the gov-cloud trap.

I would not restore it verbatim. This repository is public, and the original paragraph named an internal package, its version and an internal source file. Two sentences saying that the list mirrors an internal CyberArk allowlist, that the gov-cloud domains must be kept, and that a change upstream needs a matching change here, with the ticket as the pointer, keeps the warning without publishing the internals. I suspect that is what "trim internal jargon from comments" was aiming at, in which case this is just asking for the safety note to survive the trim.

[roeezis]

Fair — that was over-trimming, not deliberate. You read the intent right: the paragraph named an internal package, its version and an internal source file, and this repo is public.

Restored the substance without the internals: that the list mirrors an authoritative allowlist maintained outside this repo and must be kept in step with it, that drift is now fatal rather than degrading because it gates the bootstrap URL, and that the gov-cloud entries must be kept because an earlier draft omitted them. No ticket pointer — same reason as the rest of the trim.

[roeezis]

Rebase looks right — verified go build/vet/test myself too, matches what you found.

On the two open follow-ups from the previous review:

Untrusted URL in the identity-rejection error (discovery.go): confirmed — the raw discovery-response value was embedded verbatim in a returned error, and that error reaches a Kubernetes Event (same path as the Conjur error-body leak: DiscoverServices -> pkg/client -> postData's retry loop -> eventf). Removed the raw value from the error; the parsed hostname is still visible via the existing Info log line, which is what actually failed the allowlist check anyway.

Response bodies in the upload path (dataupload.go): confirmed, same leak class, two spots — PutSnapshot's and retrievePresignedUploadURL's non-2xx branches both returned the response body (bounded to 500 bytes, but still unvetted) directly in the error. Both reach the same Event path. Moved both to a V(2) log line and added a test proving the body is still visible there, not just absent from the error — same pattern as the earlier Conjur fix.

Commit 7a20418.

[roeezis]

All three blockers fixed, plus the seven non-blocking points, in 6ad5a78. Detail is inline; this is the summary.

Blockers

1. Base-URL error no longer prints the URL — scheme and host only. Traced the path you named and confirmed it reaches a Pod Event.
2. Tenant-subdomain warning now skips identity_administration, whose host is keyed on the Identity tenant's identifier rather than the platform subdomain. Also folds case.
3. Base-URL test asserts the error text and supplies a transport that fails the test on any request. Verified by deleting the guards and watching all four cases fail — including the outbound request to the real internet you found.

Non-blocking

• Took your registry simplification as your design: mockdial.go deleted, no InsecureSkipVerify, no dial rewriting, no process-wide map, three mocks back to srv.Client(). That also closes your earlier point about InsecureSkipVerify shipping, and the real-DNS fallthrough.
• servicediscovery.New returns an error now, so a bad ARK_DISCOVERY_API fails at startup rather than crash-looping through PushingErr. Runtime check kept as defence in depth. One production caller, which already returned an error.
• Refused redirects on the Conjur exchange rather than only documenting the gap — the SA-token-in-body path you described checks out end to end.
• Provenance note restored without the internal detail; the three stale escape-hatch comments corrected.

Two follow-ups I'm deliberately not doing here

• The 3 remaining inert httptest symbols in the binary come from the mock.go files being ordinary build files, which predates this PR. Worth its own change.
• The gosec exclusion in .golangci.yaml has no path scoping, so G402 never fires anywhere in the repo, not just here. Also its own change.

On the rebase: verified independently before building on it — go build, go vet, and go test across ./internal/... and ./pkg/testutil/... all clean, and the only pkg/client failure is the pre-existing jsontext.Value message drift. Thanks for doing it rather than leaving the PR unreadable.

Full suite: 457 tests, 4 skipped, that one pre-existing failure. -race clean across ./internal/... and ./pkg/testutil/.... golangci-lint clean.

[roeezis]

Went and measured both of the follow-ups I'd waved off, rather than leaving them as assertions. Both should stay out of this PR, but for better reasons than I gave.

The httptest symbols are provably inert, not merely unused. net/http/httptest's init only registers the flag if -httptest.serve= is already in os.Args:

func init() {
	if strSliceContainsPrefix(os.Args, "-httptest.serve=") || strSliceContainsPrefix(os.Args, "--httptest.serve=") {
		flag.StringVar(&serveFlag, "httptest.serve", "", ...)
	}
}

Confirmed against the built binary: no httptest flag in agent --help or at the root. So the three symbols register nothing, listen on nothing, and are unreachable.

I also tried the structural fix properly and it doesn't work cleanly. Moving the mocks to their own package means they can no longer set allowLoopbackHosts, which is unexported — and the ways out are all worse: importing testing into a production package, or exporting a setter that disables a security control. A //go:build tag fails for the reason I hit yesterday: Go has no default-on tag for tests, so a bare go test ./... breaks with undefined symbols instead of a real failure. Happy to be told I'm missing an option here.

The gosec exclusion is one line to remove, but it uncovers 16 findings, 15 of them in code this PR never touches:

   3 pkg/testutil/envtest.go              G705
   3 pkg/client/client_ngts_test.go       G117
   1 pkg/client/client_venafi_cloud.go    G101
   1 pkg/client/client_oauth.go           G115
   1 pkg/client/client_file.go            G306
   1 pkg/client/client_file_test.go       G306
   1 pkg/agent/run.go                     G112
   1 pkg/agent/config.go                  G101
   1 cmd/agent_test.go                    G204
   1 internal/cyberark/servicediscovery/mock.go  G101

The mix needs judgement per finding rather than a bulk fix — G112 on run.go looks real, G306 and G115 are arguable, and the G101s are false positives on URLs and paths (the one in mock.go is a pre-existing constant flagged because the hostname contains "secretsmgr"). Turning gosec on and triaging 16 findings across pkg/client, pkg/agent and cmd doesn't belong in a service-discovery change, and I'd rather not unilaterally set your lint policy.

Worth noting for the original concern though: G402 is absent from that list entirely, because InsecureSkipVerify no longer exists anywhere in the tree. So the regression you were worried about has nothing left to catch in this code — the lint gap is still real for a future reintroduction, but it isn't papering over anything today.

I'll raise both separately. Say the word if you'd rather see either folded in and I'll do it.

[wallrj-cyberark]

Approving. All three blockers are fixed, and you have taken most of the non-blocking points as well.

I re-checked the vacuous-test one by mutation rather than by reading. Neutering the guard now fails all five subtests and trips failOnDial, where previously all three passed. The suite also drops from 6.83s to 0.00s, so the accidental outbound request is gone too. The loopback when not set case is a good addition — it pins that the test seam is off by default, which is the property that makes the seam safe.

The base-URL error now prints only scheme and u.Hostname(), which strips userinfo and port, so the Event path is closed. subdomainCheckApplies fixes the identity warning and the case folding went in alongside it.

Also picked up beyond what was asked: the registry is gone entirely (no InsecureSkipVerify or RegisterMockHost left anywhere, mock.go down 60 lines), CheckRedirect on the Conjur exchange with a shallow client copy so the connection pool survives, New returning an error and propagated through pkg/client, and the three stale comments rewritten accurately. The provenance note came back with the safety warning intact and the internal identifiers left out, which is exactly right for a public repository.

One thing left, not blocking and not a regression: PutSnapshot's own non-2xx branch at dataupload.go:173-177 still has no test. Every assertion in that file matches while retrieving snapshot upload URL, so only retrievePresignedUploadURL is covered. I confirmed by mutation — changing that branch's error string leaves the suite passing. Worth one more subtest whenever you are next in there.

Verification

Checked against 6ad5a78.

All ten packages under ./internal/... and ./pkg/testutil/... pass, including under -race. pkg/client shows only the pre-existing KUBEBUILDER_ASSETS failure, unchanged from before this PR.

I have not run the e2e suites, so this says nothing about behaviour under load or on retry paths.