Add githubAppClientID as an alternative to githubAppID - #1296
Merged
Merged
Conversation
matheuscscp
reviewed
Sep 16, 2026
Erik-Schuetze
force-pushed
the
githubapp-client-id
branch
from
September 22, 2026 12:16
3a1ed36 to
9fce150
Compare
Contributor
Author
|
appID is now a WithGitHubAppID option. I left the validation unchanged as it was already checking for exactly one of appID/clientID. As discussed this is a breaking change to the exported MakeGitHubAppSecret signature (positional appID dropped), so downstream callers will need updating. The force push was just to add the missing DCO sign-off to the commit. |
Member
|
@Erik-Schuetze Please rebase with main (no merge commits!), squash all commits into one, and force-push |
GitHub recommends using the App client ID (e.g. Iv23li...) rather than the numeric App ID as the JWT iss claim; both remain valid. Exactly one of the two must be provided (mirroring the existing installationOwner/installationID pattern) — supplying both or neither is an error at every layer. auth/githubapp: new KeyAppClientID const and clientID field; WithAppData reads it as an opaque string; New enforces exactly one of appID/clientID; createJWT uses clientID as the JWT iss when set, else the numeric appID; buildCacheKey includes clientID to avoid cache-key collisions for client-ID-only clients. runtime/secrets: new KeyGitHubAppClientID const; WithGitHubAppID and WithGitHubAppClientID options (both symmetric, positional appID removed); MakeGitHubAppSecret requires exactly one of the two; GitHubAppDataFromSecret enforces exactly one on the read path. Breaking change: MakeGitHubAppSecret no longer takes appID as a positional argument — callers must use WithGitHubAppID instead. Refs: fluxcd#1291 Signed-off-by: Erik Schuetze <erik.schuetze@sap.com> Assisted-by: claude-code/claude-opus-4-8
Erik-Schuetze
force-pushed
the
githubapp-client-id
branch
from
September 22, 2026 13:19
9fce150 to
7446dc3
Compare
Contributor
Author
|
Done |
matheuscscp
approved these changes
Sep 22, 2026
matheuscscp
left a comment
matheuscscp
left a comment
Member
There was a problem hiding this comment.
LGTM! 🚀
Thanks @Erik-Schuetze, nice addition!
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.
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.