feat(agent): wire Conjur JWT config into the top-level CyberArk client - #823
feat(agent): wire Conjur JWT config into the top-level CyberArk client#823roeezis wants to merge 6 commits into
Conversation
4e2daeb to
c2d0dcf
Compare
Introduces a small, isolated interface for reading a JWT from a file path — the first piece of the upcoming Conjur JWT authentication path, split out on its own since nothing else in this PR depends on it yet.
identity.go mixed the shared client/token-cache plumbing with the CyberArk Identity username/password (UP) login flow. Move the UP-specific code into username_password.go so the shared plumbing stays easy to find once a second login mechanism (Conjur JWT) is added alongside it. No behavior change — pure extraction, plus exporting the mock's success credentials for other packages' tests.
c2d0dcf to
9b587c8
Compare
mladen-rusev-cyberark
left a comment
mladen-rusev-cyberark
left a comment
There was a problem hiding this comment.
Config plumbing is straightforward, and the reasoning for combining the call-site and signature change in one commit is right. Two things ride along that I'd separate, or at least call out — inline.
Minor points:
NewCyberArknow takes four adjacent same-typed string params (pkg/client/client_cyberark.go:45) — classic transposition hazard, and the compiler can't help.CyberArkConfigalready exists and is already being destructured at the call site; passing the struct would be one self-documenting parameter, and adding a fifth field later wouldn't be a signature change.pkg/agent/config.go:783: after the block above it,clusterName == ""already impliescfg.ClusterID == "", so the second conjunct is dead.res.CyberArk = arkonly runs in MachineHub mode, so acyberark:block in a Venafi Cloud config is silently ignored. Everywhere else in this function ignored fields get a log line (see theorganization_idhandling just above).
c304519 to
a696848
Compare
|
Fixed — added On the signature note: kept |
|
Fixed — added the check you suggested (same code, |
|
Confirmed accurate, and I'm not resolving this one — it's a product decision, not something I should silently pick:
An existing install with I've reworded the log line (point 3 — it said "for backwards compatibility" describing what is actually new behavior). Points 1 and 2 need a human call: @rzisholz — is a cluster-name change on upgrade acceptable here, and does whoever owns the MachineHub data model need to weigh in or get a migration note? I don't have the context to decide this myself, and I don't want to guess and silently ship a behavior change to production cluster identity. |
a696848 to
8aa61e4
Compare
The Service Discovery API returns several independently-hosted services; the authn-jwt exchange this PR series adds is served by secrets_manager, a different host from identity_administration. Add a SecretsManager field to Services and parse it, so callers have it available — nothing reads it yet, that lands in a later PR alongside the client that needs it. secrets_manager and discoveryContext are deliberately not required here unlike identity, since not every caller needs them and requiring secrets_manager would break every existing username/password install on a tenant not yet onboarded to Conjur — each caller validates what it needs at its own point of use instead. identity, by contrast, is required unconditionally: it's present and active for every healthy tenant, so callers may rely on it without re-checking. Factor the repeated "find the first active main endpoint" loop into a mainActiveAPI helper now that there are three near-identical copies.
362d7b2 to
88f83bf
Compare
|
Resolved with a compromise from @rzisholz: cluster_id still takes priority over ARK_USERNAME when both are set (needed since ARK_USERNAME doesn't exist on the Conjur JWT path), but ARK_USERNAME is restored as a further fallback rather than dropped — order is now cluster_name > cluster_id > ARK_USERNAME > empty. This means:
Added three tests pinning this exact order ( |
88f83bf to
db68257
Compare
|
The fallback now depends on which auth method is actually active, mirroring selectAuthenticator's own precedence, instead of one universal order:
This fully closes the original concern: an existing username/password install now sees zero behavior change, in any configuration. cluster_id is only ever consulted on the new Conjur path, which no existing install could be on. |
|
Follow-up on the "confirm with whoever owns the MachineHub data model" ask — checked directly against discoverycontext-file-ingestor-service rather than guessing.
So there's no data/identity fragmentation risk from this fallback-order change, in either direction — worst case is a cosmetic display-label flicker for one upgrade, since |
mladen-rusev-cyberark
left a comment
mladen-rusev-cyberark
left a comment
There was a problem hiding this comment.
Round 3 — approve, with one code gap and one behaviour proposal inline.
The two-upload regression test is the right test and it genuinely works: I reintroduced the defer wipe in username_password.go and it fails on the second upload with the original Identity error; reverting makes it pass. Its doc comment explains the []byte-via-closure sharing clearly and says outright that a single upload structurally can't catch it. The envtest.go reword cites the discovery contract properly now.
Two things inline:
config.go:806still missesARK_SECRET. This one is mine — you added the check exactly as I wrote it in round 1, and I got it wrong. My round-2 review flagging it was never posted, so you've had no chance to fix it. Details inline.- The MachineHub cluster-name change — I think this can preserve existing behaviour and meet the new requirement, without the product decision you escalated. Concrete proposal inline.
Minor, all fine to leave:
res.CyberArk = arkonly runs in MachineHub mode, so acyberark:block in a Venafi Cloud config is silently ignored — unlikeorganization_idjust above, which logs.NewCyberArk's four adjacent string params: passing a struct would meanpkg/clientimportingpkg/agent, so leaving it is defensible — worth a comment saying so.
db68257 to
3704306
Compare
|
Took this exactly as proposed — verified both supporting claims first ( |
|
Confirmed and fixed — added the |
5e21356 to
5ac99f3
Compare
5ac99f3 to
6a31ea8
Compare
Exchanges a projected ServiceAccount JWT (via jwtsource) for a Conjur access token through the authn-jwt endpoint, and authenticates requests with it as identity.RequestAuthenticator. Nothing wires this in yet — that's the next PR, once both this and the legacy identity client exist side by side. The identity returned for audit tagging is the token's own sub claim when it can be extracted, falling back to the configured service ID otherwise. The cache expiry is driven by the token's own exp claim when present, falling back to a guessed TTL only when it isn't — a fixed TTL stamped after the exchange returns would otherwise serve a token past its real expiry under latency or clock skew. Exposes Invalidate() so a caller that gets a 401 from the resource server can force a fresh exchange instead of waiting out the cache.
Adds NewRequestAuthenticator, choosing between the new Conjur JWT exchange and the legacy CyberArk Identity username/password login based on which config is present — Conjur JWT takes priority when both are set. Conjur JWT requires service_id and resolves its base URL from the secrets_manager service discovered in the prior PR, not identity_administration; the two are different hosts. The identity API is only required on the username/password path now; Conjur JWT never uses it. Since service discovery already errors on a missing identity_administration API before selectAuthenticator ever runs, the username/password branch no longer re-checks it — that check could never actually fire. Switches keyfetch's client over to the new authenticator selection instead of constructing a username/password identity client directly. Doing so dropped keyfetch's own per-fetch LoginUsernamePassword call, which was the only thing keeping the cached identity token from aging out — identity.Client.AuthenticateRequest never refreshed on its own. Give it the same self-refreshing behavior conjur.Client already has: it now re-logs-in internally once its cached token passes tokenTTL (a field, not a const, so tests can shrink it), using a durable copy of the credentials it captured at the last LoginUsernamePassword call. A refresh that fails falls back to whatever's cached rather than failing the request outright, since the next call will retry. LoginUsernamePassword no longer zeroes the caller's password slice: it already keeps its own durable copy (needed for the self-refresh above), so wiping the caller's copy bought nothing and actively broke callers that reuse one ClientConfig across multiple logins — cfg.Secret is a []byte shared across every upload cycle via NewCyberArk's configLoader closure, so the second call received an already-zeroed password and failed to authenticate. Reproduced live before fixing: a second PostDataReadingsWithOptions call on the username/password path failed with "Authentication ... has failed" every time. Hoists the jwt_source validation and the "file"/"conjur" literals this and the agent config layer both encode separately into shared JWTSourceFile/DefaultAccount consts and a ValidateJWTSource helper, and drops the "POC" wording from the operator-facing error.
NewCyberArk and the agent config schema were still legacy-username/password-only at the top level, even though the underlying authenticator selection (previous PR) already supports Conjur JWT. Threads service_id/account/jwt_source/jwt_file_path through cyberark.service_id in the agent config down to NewCyberArk, so the config file is the single place an operator chooses which authentication method to use. config.go's call site and client_cyberark.go's signature change together — splitting them across two PRs would leave one non-building at every commit in between. jwt_source validation now delegates to the shared cyberark.ValidateJWTSource (previous PR) instead of re-implementing the same rule with different wording. Requiring an auth method is now checked at config-validation time rather than left to cyberark.selectAuthenticator alone — that only runs at first upload, so a misconfigured agent could otherwise report healthy for up to a full config.period before failing. Checks both ARK_USERNAME and ARK_SECRET, since selectAuthenticator's hasUP requires both — checking only ARK_USERNAME would pass validation and still fail at runtime for the one credential missing. MachineHub's cluster_name fallback order is cluster_name > cluster_id > ARK_USERNAME > empty. Explicit config always wins over the env var; the ARK_USERNAME fallback exists only for pre-Conjur installs that set neither config field — the chart never emits cluster_id, so a chart-installed agent can't have set it. This also keeps a migrating install's reported name unchanged: adding service_id to test Conjur alongside still-present ARK_USERNAME, before also setting cluster_id, doesn't blank out the name. Adds FakeCyberArkUsernamePassword and matching integration tests: the only existing test exercising NewCyberArk's username/password path set ARK_USERNAME/ARK_SECRET but also passed a non-empty serviceID, which selectAuthenticator prioritises — so it silently tested Conjur regardless of those env vars, leaving the legacy path with no coverage through this seam. Also adds a two-upload regression test for the previous PR's cfg.Secret zeroing fix, which a single-upload test can't catch.
6a31ea8 to
f42e98b
Compare
2 participants
Summary
Part 6 of the SMS/Conjur JWT authentication series (split out of #817). Stacked on the prior 5 PRs in this series — diff will shrink as they merge.
NewCyberArkand the agent config schema were still legacy-username/password-only at the top level, even though the underlying authenticator selection (#822) already supports Conjur JWT. Threadsservice_id/account/jwt_source/jwt_file_paththroughcyberark.service_idin the agent config down toNewCyberArk, so the config file is the single place an operator chooses which authentication method to use.config.go's call site andclient_cyberark.go's signature change together — splitting them across two PRs would leave one non-building at every commit in between, so they're one PR.Test plan
go build ./...go test ./pkg/agent/... ./pkg/client/... ./pkg/testutil/...