Add JIT access fallback for GitHub API requests - #176
Conversation
There was a problem hiding this comment.
Pull request overview
Adds JIT access fallback support to GitHubAPIHandler so GitHub API GET/HEAD requests that fail with 401/403/404 can be retried using repository-scoped JIT credentials (requested via the existing API client) and then cached for later requests.
Changes:
- Extend
GitHubAPIHandlerto parsejit_accesscredentials and request repo-scoped JIT tokens after auth failures, retrying the API request. - Wire the shared API client into
GitHubAPIHandlerfromproxy.go. - Add tests covering 404-triggered JIT fallback, JIT-only configs, and JIT token caching behavior.
Show a summary per file
| File | Description |
|---|---|
| proxy.go | Passes the shared API client into GitHubAPIHandler so it can request JIT access. |
| internal/handlers/github_api.go | Adds JIT access configuration, fallback retry logic, and JIT token caching into the existing credential store. |
| internal/handlers/github_api_test.go | Adds test coverage for JIT fallback behavior and caching. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
internal/handlers/github_api.go:162
- The JIT retry always sets an
Authorization: token ...header and does not clear any existing auth headers. If JIT ever returns the Proxima service identity (or if the original request usedX-GitHub-PSI-JWT), the immediate retry will send the wrong/ambiguous auth headers and may not actually exercise the JIT credential.
Mirror the existing credential handling logic here: clear both auth headers on the cloned request and choose between X-GitHub-PSI-JWT vs Authorization based on jitCreds.username.
newReq := ctx.Req.Clone(ctx.Req.Context())
logging.RequestLogf(ctx, "* auth'd github api request failed authentication, retrying with jit access auth")
newReq.Header.Set("Authorization", "token "+jitCreds.password)
newRsp, err := ctx.RoundTrip(newReq)
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Low
|
@thavaahariharangit Helm smoke test is failing. |
|
I'm digging through wether this opens up any access to resources/endpoints that we would otherwise expect to be shielded off for users. I think it's probably fine, but I want to make sure that I fully understand how this changes the access that Dependabot gets to these private repositories. |
jurre
left a comment
There was a problem hiding this comment.
Can we put all of this behind a feature flag so we can test it in isolation, roll it out to customers slowly and revert it quickly if needed?
jurre
left a comment
There was a problem hiding this comment.
Commented by Copilot from a review-session with Jurre.
The JIT plumbing mirrors GitServerHandler closely and the happy-path tests are solid. My concerns are all about what happens when JIT fails rather than when it succeeds. GitServerHandler guards against retry storms with reposAlreadyTried; this handler has no equivalent, and combined with treating 404 as an auth failure that turns routine GitHub API 404s into JIT round trips.
There was a problem hiding this comment.
Review details
Suppressed comments (1)
internal/handlers/github_api.go:154
- In HandleResponse, the code uses ctx.Req.BasicAuth() to avoid retrying the same credentials, but GitHubAPIHandler authenticates via the Authorization/X-GitHub-PSI-JWT headers (not BasicAuth). As a result, the first retry can repeat the exact same token/identity, and retries can also end up sending both Authorization and X-GitHub-PSI-JWT on the same request because the alternate-auth loop sets one header without clearing the other. This can cause ambiguous auth precedence and unnecessary extra round-trips.
Consider detecting the original auth from headers (or storing the selected credential in ctxdata like GitServerHandler does) and always clearing both headers before setting the new auth.
username, password, reqWasAuthed := ctx.Req.BasicAuth()
for _, creds := range getCredentialsForRequest(ctx.Req, h.credentials, gitHubAPIExtractOrgAndRepo) {
// don't retry the request with the same auth that was previously used
if reqWasAuthed && creds.username == username && creds.password == password {
continue
}
newReq := ctx.Req.Clone(ctx.Req.Context())
if creds.username == reservedProximaIdentity {
logging.RequestLogf(ctx, "* auth'd github api request failed authentication, retrying with alternate identity")
newReq.Header.Set("X-GitHub-PSI-JWT", creds.password)
} else {
logging.RequestLogf(ctx, "* auth'd github api request failed authentication, retrying with alternate provided auth")
newReq.Header.Set("Authorization", "token "+creds.password)
}
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
What are you trying to accomplish?
Add JIT access fallback support to GitHubAPIHandler.
When static credentials cannot access a repository and GitHub returns
401,403, or404, the handler now requests a repository-scoped JIT credential and retries the API request. This allows Dependabot to retrieve metadata, such as release information, from internal repositories permitted by the organization’s repository access policy.Successful JIT credentials are cached for subsequent API requests to the same repository.
Anything you want to highlight for special attention from reviewers?
The implementation follows the existing JIT authentication pattern used by GitServerHandler:
GitHubAPIHandler.Reviewers should pay particular attention to treating
404as a potential authentication failure, since GitHub returns it when an authenticated token cannot access a private or internal repository.How will you know you've accomplished your goal?
Tests verify that:
404falls back to JIT authentication.The full test suite passes with:
Checklist
Rollout and failure behavior
PROXY_GITHUB_API_JIT_ACCESS=true.404responses remain potential authentication failures because private repositories can be hidden behind a404; when enabled, an ordinary404can therefore cause one JIT attempt for that repository.