feat(agent): add Helm chart support for Conjur JWT authentication - #824
Open
roeezis wants to merge 7 commits into
Open
feat(agent): add Helm chart support for Conjur JWT authentication#824roeezis wants to merge 7 commits into
roeezis wants to merge 7 commits into
Conversation
3 tasks
roeezis
force-pushed
the
split/07-helm-chart
branch
from
August 24, 2026 11:42
5ca5c7a to
69ebc67
Compare
added 2 commits
August 24, 2026 14:43
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.
roeezis
force-pushed
the
split/07-helm-chart
branch
from
August 24, 2026 11:45
69ebc67 to
5c30360
Compare
mladen-rusev-cyberark
left a comment
mladen-rusev-cyberark
left a comment
Collaborator
There was a problem hiding this comment.
Chart renders cleanly and the env-var changes are right — ARK_SUBDOMAIN stays required, legacy keys become optional, so JWT-only installs need no username/password. expirationSeconds: 600 is the API-server minimum and works out fine given the kubelet rotates the file in place and jwtsource re-reads per exchange, so the 8-minute Conjur cache never outlives the projected token.
Findings are all in the volume wiring and the docs. One I couldn't leave inline:
Generated docs are stale. The ## Values section of README.md and values.schema.json are both generated by helm-tool (make/_shared/helm/helm.mk:98,106), and the new config.cyberark.* values are absent from the <!-- AUTO-GENERATED --> block (lines 146–520). So either the verify target fails in CI, or the schema was hand-edited into a shape helm-tool wouldn't produce. Please run make generate and commit the result — the existing values.yaml entries carry # +docs:property markers and the new block has none, which may be why generation skipped it.
Also: jwtSource has no enum in the schema, so a typo (fille) renders fine, deploys fine, and CrashLoops. Worth "enum": ["", "file"].
roeezis
force-pushed
the
split/07-helm-chart
branch
3 times, most recently
from
August 24, 2026 12:19
dbe96b8 to
2d4fc64
Compare
Author
|
Blocker fixed — took your recommended second option: dropped the |
Author
|
Fixed — gated both the volume and the mount on a new On audience: documented in the README's troubleshooting table that it's fixed at |
Author
|
Both addressed:
|
added 5 commits
August 24, 2026 19:34
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.
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 (service_id or ARK_USERNAME) 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. 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.
Exposes config.cyberark's Conjur JWT fields (serviceId, account, jwtSource) in values.yaml/values.schema.json, and documents per-cluster Conjur onboarding as a prerequisite in the chart README — the tenant administrator's own Conjur Cloud credentials, no admin credential needed at deploy time. Drops the jwtFilePath knob: it let an operator set a path the chart's projected-volume mount couldn't actually honor (mountPath was hardcoded independently of it), producing a deployment that renders cleanly but never authenticates. The agent's own DefaultTokenPath already matches the chart's fixed mount path, so there's nothing to configure. Gates the projected token volume on serviceId being set, not just jwtSource (which defaults to "file") — a pure username/password install was getting an unused audience=conjur token volume for no reason, unnecessary surface for clusters with admission policies constraining projected-token audiences. Factored the condition into a named template since it's needed in two places and needs to stay in sync. Documents that the volume's audience is fixed at conjur, not a values.yaml setting, and that the config-validation fix in the previous PR makes the README's "fails closed at startup" claim actually true (previously that only held because sendSecretValues:true is the chart default and triggers eager authenticator construction).
roeezis
force-pushed
the
split/07-helm-chart
branch
from
August 24, 2026 16:38
2d4fc64 to
9eeba38
Compare
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
2 participants
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
Part 7 (final) of the SMS/Conjur JWT authentication series (split out of #817). Stacked on the prior 6 PRs in this series — diff will shrink to just this PR's own change once they merge.
Exposes
config.cyberark's new Conjur JWT fields (serviceId,account,jwtSource) invalues.yaml/values.schema.json, and documents per-cluster Conjur onboarding as a prerequisite in the chart README — the tenant administrator's own Conjur Cloud credentials, no admin credential needed at deploy time.Test plan
go build ./...helm lint deploy/charts/disco-agent/