fix(dex): restrict GitHub auth to maintainers - #2709
Merged
devantler merged 12 commits intoAug 17, 2026
Conversation
Contributor
Author
|
Contributor
Author
|
@cursor review |
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a0ff91ea-8114-4f41-934b-e36674aabd9e) |
Contributor
Author
|
Contributor
Author
|
@cursor review |
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_65bc499d-e68b-47e3-aa67-ab59ba018341) |
devantler
commented
Aug 9, 2026
devantler
left a comment
devantler
left a comment
Contributor
Author
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Static exact-head review of 900256b826f19d776149ec8a66064a7222588723.
This was referenced Aug 14, 2026
Contributor
✅
|
… team gate `🔐 Validate EKS Authorization` has failed on this branch since the Dex change landed: narrowing the GitHub connector moves the rendered authorization-surface fingerprint, and the new value was never approved. Approved against the MERGE RESULT after `gh pr update-branch`, so the constant describes what actually merges rather than a seven-commit-stale base. The branch's earlier value (1a4f58dc) was rendered against that older main and is correctly not approved. Evidence: - CI on the approved toolchain and a local render agree exactly on 78490ee9. - The validator reports no per-resource mismatch; only the aggregate moved. - Reverting exactly the three touched manifests to their main versions makes the validator PASS against the previous constant, so the merge carried no unrelated authorization drift. The delta is a privilege reduction: Dex authenticates the maintainers team instead of every devantler-tech org member. No identity, binding, resource, or verb is added, and nothing granted to the aws/aws service account is touched.
Code Coverage OverviewLanguages: Go Go / code-coverage/goThe overall coverage in commit 35c8a3a in the Show a code coverage summary of the most covered files.
Updated |
…ility-in-actual-budget-oidc # Conflicts: # k8s/bases/apps/crossview/helm-release.yaml # scripts/validate-eks-ci-role-policy/main.go
…merge result The branch's previous value was rendered against an older main and did not describe the merge result. Records the value both the required CI job and a local render measured, with the reviewed delta unchanged.
Contributor
Author
The latent shared-connector gate discussed in the resolved thread is now tracked as #3187, so it is not lost when this merges. Nothing blocking remains on that thread — the change excludes no current org member. |
# Conflicts: # scripts/validate-eks-ci-role-policy/main.go
Resolves the conflict in the authorization-fingerprint ledger. Both parents appended to the same ledger region and each described only its own delta -- this branch the Dex GitHub-connector narrowing, main the #2713 gateway-tenant-edit route-kind narrowing plus the #2725 Umami provisioning grant. Neither parent's value describes the merge result, so the conflict is not resolvable by picking a side. Both prose records are retained, because both deltas are present in the merged surface and neither cancels the other. Only one const survives, and it is deliberately left at main's approved value rather than either parent's: NOT RE-APPROVED. The merge-result fingerprint has not been measured here. A local render could not produce an authoritative value -- this host has kubectl v1.36.1 and the ledger's approved renderer is v1.36.2, so the validator refuses fail-closed rather than emitting an unapproved measurement. The required `Validate EKS Authorization` job runs the approved toolchain and is the authoritative renderer; its reported value is what must be approved into the const, with the usual second independent render agreeing before this merges.
Contributor
Author
Conflict resolved — but the fingerprint is deliberately NOT re-approvedThis PR was Why it could not be resolved by picking a side. Both parents appended to the same
Both deltas are present in the merged surface and neither cancels the other, so both prose records That const is left at That refusal is the guard working, not an obstacle to route around. So this PR is parked on a named blocker: the merge-result fingerprint must be measured and Unrelated but worth noting while the render was being checked: the prod overlay renders cleanly from |
… maintainers-team narrowing The required EKS authorization job measured 8773eaf0 for this branch's merge result; the constant still carried main's b5b39418, which was deliberately left un-approved while the value could not be attributed to this branch alone. Taken at head 1d35730 with the branch level against main (behind_by 0), so the digest describes the merge result rather than a stale rendering. Supersedes the two earlier #2709 records, measured on merge states a later main had moved past. Conservation: the aggregate is the ONLY control that moved — zero per-identity mismatches, zero missing, zero duplicate, zero encrypted findings, and the same 35 unresolved-substitution notes clean main reports, so surface membership is unchanged. The single rendering is corroborated by two independent observations that the leveling merge moved nothing: the required job reported this identical digest at 629bdd1 (pre-merge) and 1d35730 (post-merge), and main already contains that merge's only k8s change (umami CronJob Checkov annotations) while passing against the previous digest, so those annotations are not in the surface. Still a privilege reduction on every axis: Dex authenticates the maintainers team instead of every devantler-tech org member, and no identity, binding, ServiceAccount, resource, or verb is added anywhere.
Contributor
Author
CI is green at @coderabbitai review |
Contributor
|
✅ Action performedReview finished.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.{yaml,yml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (6)📚 Learning: 2026-07-01T21:13:36.950ZApplied to files:
📚 Learning: 2026-07-07T18:01:26.730ZApplied to files:
📚 Learning: 2026-08-08T15:10:00.349ZApplied to files:
📚 Learning: 2026-08-08T15:10:00.350ZApplied to files:
📚 Learning: 2026-08-08T21:23:32.529ZApplied to files:
📚 Learning: 2026-08-11T12:41:28.242ZApplied to files:
🔇 Additional comments (5)
📝 WalkthroughWalkthroughThe Dex GitHub connector now allows only the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
Readiness at
|
devantler
marked this pull request as ready for review
August 17, 2026 21:34
devantler
added a commit
that referenced
this pull request
Aug 17, 2026
main merged #2709, which re-approved expectedRenderedSurfaceSHA for the Dex maintainers-team narrowing, while this branch had re-approved it for the Longhorn orphan-reclamation chart value. Both parents therefore moved the same constant and each described only its own delta, so neither value describes the merge result and the conflict cannot be resolved by picking a side. Resolved by retaining BOTH measurement records — both deltas are present in the merged surface — and leaving the constant as an explicitly-flagged UNMEASURED placeholder at main's value. The required job is expected to reject it and report the merge result's actual digest, which must be recorded before promotion. The superseded #3181 digest 489afc66 is kept in full as a record so the measurement taken at 74676d5 is not lost. Both deltas remain individually measured and individually authorization-neutral or privilege-reducing, and neither moved surface membership; only their aggregate is unmeasured.
devantler
added a commit
that referenced
this pull request
Aug 17, 2026
…erge result The required job rejected the carried-through placeholder and reported the merge result's actual digest. Conservation measured against this branch's own pre-merge rendering: 35 substitution notes both sides, zero resources added, removed or duplicated, and exactly two per-identity fingerprints moved - the dex and oauth2-proxy HelmReleases, which are precisely the two manifests #2709 changes and are already approved on main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 tasks
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.
Motivation
public-client, which allowed any member of thedevantler-techGitHub org to log in and potentially become the first-owner, creating an authorization risk.oauth2-proxycannot be claimed by non-maintainer org members.Description
teams: - maintainersunder theorgs: - name: devantler-techentry ink8s/bases/infrastructure/controllers/dex/helm-release.yamlso onlydevantler-tech/maintainerscan authenticate.oauth2-proxy'sallowed_groupsas defense-in-depth by clarifying its comment ink8s/bases/infrastructure/controllers/oauth2-proxy/helm-release.yaml.k8s/bases/apps/crossview/helm-release.yamlto reflect that Dex is now maintainer-scoped for native-OIDC apps.Testing
ruby -e 'require "yaml"; ...'which succeeded.python scripts/validate-naming.pywhich succeeded.git diff --checkwhich reported no whitespace/merge issues.kubectl kustomizeorksail workload validatebecausekubectlandksailare not installed in this environment (not run).Codex Task