CP-26002: validate ARK_DISCOVERY_API itself against the domain allowlist - #830
Open
roeezis wants to merge 5 commits into
Open
CP-26002: validate ARK_DISCOVERY_API itself against the domain allowlist#830roeezis wants to merge 5 commits into
roeezis wants to merge 5 commits into
Conversation
added 5 commits
September 3, 2026 17:55
CP-25960 (AG1, 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 with no allowlist — an SSRF-shaped hole if the discovery response is ever tampered with. Mirrors finding R2 already fixed on the discoverycontext-regional-resources side (token.py's iss-host allowlist). Constrain each discovered host to a known CyberArk root domain, or to the same host we already made the successful, TLS-authenticated discovery call to (covers the ARK_DISCOVERY_API dev/CI override and same-origin test mocks without a per-env map). A dropped host is treated the same as one absent from the response. CP-25964 (A1, Low): the authn-jwt exchange error included up to 4KiB of Conjur's response body, which propagates to a Kubernetes Pod Event (pkg/agent/run.go's PushingErr notification) — readable by anyone with `get events` in the namespace. The body can contain Conjur policy structure, service IDs and host identities. Log it at V(2) instead; the returned error (and therefore the Event) now carries only the status code and the existing troubleshooting hint. SCR: https://ca-il-confluence.il.cyber-ark.com/pages/viewpage.action?pageId=710861530
The allowlist added in the previous commit was copied from the identity authorizer Lambda's local commercial-only clone of everest_env_utils' ROOT_DOMAIN map, not the real package. The real map (everest_env_utils_cyberark, v2.0.117) has 8 more entries for the GOV_* environments (*-cyberarkgov.com/.cloud) that the clone omits. Without them, any gov-cloud tenant's agent would have every discovery-derived host rejected, breaking identity discovery entirely (fatal, since it's required unconditionally).
CI's verify job failed: gci requires third-party imports (k8s.io/klog) grouped separately from and before this module's own imports, per .golangci.yaml's [standard, default, localmodule] section order.
Per the pentest finding CP-23593 ("Broken Trust Chain in Tenant Discovery
Bootstrap"), the domain allowlist alone doesn't require HTTPS -- a discovery
response could point identity/discoverycontext/secrets_manager at a plain
HTTP endpoint on an allowlisted-looking host.
This also closes the loopback-attacker shape of the "same host as
discoveryHost" escape hatch in isAllowedServiceHost: an attacker-controlled
ARK_DISCOVERY_API pointing at e.g. 127.0.0.1 (the PoC in CP-23593) can't
present a certificate this client's TLS verification will accept, so
requiring https closes exactly the vector the PoC demonstrated even for the
same-host case.
Does not touch the discovery bootstrap call's own base URL (ARK_DISCOVERY_API
itself) -- that override is deliberately left alone for dev/CI use, per AG1's
original writeup.
The domain/HTTPS allowlist added for identity/discoverycontext/secrets_manager (CP-25960) had a carve-out: any host equal to the discovery endpoint's own host was trusted automatically, since ARK_DISCOVERY_API itself was never checked. That carve-out is exactly the attack CP-23593's PoC demonstrates -- an attacker with ARK_DISCOVERY_API write access (a tampered pod spec or Helm values) bootstraps the whole trust chain from their own infrastructure. HTTPS-only closed the loopback shape of it (no valid cert for 127.0.0.1), but a rogue host with a real globally-trusted cert wasn't caught. 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" -- so isAllowedServiceHost is gone; only hostOnAllowedRootDomain remains. This breaks every test that feeds a real httptest mock address (127.0.0.1) into a Services value or ARK_DISCOVERY_API, since those addresses aren't on the allowlist either. Fixed by adding a small shared test-only registry (internal/cyberark/testing/mockdial.go): servicediscovery.MockDiscoveryServer launders any loopback address in the Services it's given (and its own ARK_DISCOVERY_API override) into a fake CyberArk-domain-looking hostname, registering a dial redirect to the real address. Every other package's Mock*Server (conjur, dataupload, identity) wraps its own returned client 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
CP-26002commit + the mock-transport rework it needed.CP-25960/#829 allowlisted
identity/discoverycontext/secrets_managerhosts, 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 is exactly the attack CP-23593's PoC demonstrates — an attacker withARK_DISCOVERY_APIwrite access (tampered pod spec/Helm values) bootstraps the whole trust chain from their own infrastructure. HTTPS-only (also in #829) closed the loopback shape of this (no valid cert for127.0.0.1), but a rogue host with a real, globally-trusted certificate wasn't caught.DiscoverServicesnow 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" (isAllowedServiceHostis gone; onlyhostOnAllowedRootDomainremains).Test infrastructure: this broke every test that feeds a real httptest mock address into a
Servicesvalue orARK_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.MockDiscoveryServerlaunders any loopback address it's given (including its ownARK_DISCOVERY_APIoverride) into a fake CyberArk-domain-looking hostname, registering a dial redirect to the real address.conjur/dataupload/identity'sMock*Serverhelpers 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.Does not close CP-23593's third recommendation (JWKS key/fingerprint pinning) — that's a separate onboarding-time design decision, not filed yet.
Test plan
go vet ./...make test-unit— 434 tests, 4 skipped, 1 pre-existing unrelated failure (json.RawMessage/jsontext.Valuemessage-drift inpkg/client, pre-existing onmaster)golangci-lintclean