Add githubAppClientID as an alternative to githubAppID - #1296
Open
Erik-Schuetze wants to merge 2 commits into
Open
Add githubAppClientID as an alternative to githubAppID#1296Erik-Schuetze wants to merge 2 commits into
githubAppClientID as an alternative to githubAppID#1296Erik-Schuetze wants to merge 2 commits into
Conversation
GitHub recommends using the App client ID (e.g. Iv23li...) rather than the numeric App ID as the JWT iss claim; both remain valid. This adds githubAppClientID across both pkg modules, fully backward compatible. auth/githubapp: new KeyAppClientID const and clientID field; WithAppData reads it as an opaque string; New requires at least one of appID/clientID; createJWT uses clientID as the issuer when set (else the numeric appID); buildCacheKey includes clientID to avoid cache-key collisions for client-ID-only clients. runtime/secrets: new KeyGitHubAppClientID const and WithGitHubAppClientID option; MakeGitHubAppSecret keeps appID positional (non-breaking) and requires exactly one of appID/clientID; GitHubAppDataFromSecret requires at least one and passes both through when present. Refs: fluxcd#1291 Signed-off-by: Erik Schuetze <erik.schuetze@sap.com> Assisted-by: claude-code/claude-opus-4-8
Following review, treat appID and clientID as mutually exclusive identities for one app (mirroring the existing installationOwner/installationID pattern), rather than allowing both with clientID taking precedence. New, MakeGitHubAppSecret, and GitHubAppDataFromSecret now all reject a secret that sets both identities. This removes the JWT issuer precedence question entirely and avoids silently ignoring one identity when a user supplies a valid one alongside an invalid one. The reader names both keys in its error instead of only githubAppID. Backward compatible: appID-only secrets are unaffected. Also fold the jwt/v4 test import into the third-party group. Signed-off-by: Erik Schuetze <erik.schuetze@sap.com> Assisted-by: claude-code/claude-opus-4-8
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.
Refs: #1291
GitHub accepts either the numeric AppID or the ClientID of GH Apps as the JWT
issclaim, and recommends the ClientID in the official docs (GitHub, GHE).pkgcurrently only accepts the numeric AppID, so this PR addsgithubAppClientIDas an alternative acrossauth/githubapp(the JWT consumer) andruntime/secrets(the secret writer and reader). Existing secrets using only AppID are unaffected.AppID and ClientID are treated as mutually exclusive. This is enforced in
New,MakeGitHubAppSecret, andGitHubAppDataFromSecret, mirroring the existinginstallationOwner/installationIDhandling.They identify the same app for a single
issclaim, so this keeps the behaviour unambiguous and avoids silently ignoring one identity when both are supplied.The ClientID is treated as a regular string, since GitHub documents no format for it (The ClientIDs I checked on GHE and GitHub also differed from each other with no common prefix).
As
pkgis a GA dependency consumed by all Flux controllers,MakeGitHubAppSecretkeepsappIDas a positional parameter and addsclientIDvia an option, because any change to the positional parameters of this exported function would break every existing consumer.If the preferred solution is to change to symmetric options (
WithGitHubAppID+WithGitHubAppClientID) and accept the function signature break, I'm happy to switch.Two behaviour changes to note, given the GA compatibility guarantee:
MakeGitHubAppSecretfrom"githubAppID is required"to"exactly one of githubAppID or githubAppClientID must be provided".buildCacheKeynow includes the ClientID, which causes a new token to be requested on version upgrade (a one-time, self-healing re-fetch of short-lived installation tokens).I applied the change across
auth/githubappandruntime/secretsinpkgso the new ClientID is usable end-to-end. If you prefer I can split it into two PRs.I added Tests for client-ID-only, both-set (rejected), and neither-set across writer, reader, and JWT issuer generation;
make test-authandmake test-runtimepass.