-
Notifications
You must be signed in to change notification settings - Fork 2
feat(server): add support for minting enterprise-scoped installation access tokens #254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4e9b3b5
81469cb
244c21a
8519760
f800a3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "net/url" | ||
| "strings" | ||
| "time" | ||
|
|
||
|
|
@@ -149,6 +150,74 @@ | |
| return accessToken, nil | ||
| } | ||
|
|
||
| func (g *gitHubSourceSystem) installationForEnterprise(ctx context.Context, app *githubauth.App, enterprise string) (*githubauth.AppInstallation, error) { | ||
| jwtToken, err := app.AppToken() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to generate app JWT: %w", err) | ||
| } | ||
|
|
||
| client := github.NewClient(g.httpClient).WithAuthToken(jwtToken) | ||
| if g.baseURL != "" { | ||
| parsedURL, err := url.Parse(g.baseURL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse base URL: %w", err) | ||
| } | ||
| if !strings.HasSuffix(parsedURL.Path, "/") { | ||
| parsedURL.Path += "/" | ||
| } | ||
| client.BaseURL = parsedURL | ||
| } | ||
|
|
||
| req, err := client.NewRequest(http.MethodGet, fmt.Sprintf("enterprises/%s/installation", enterprise), nil) | ||
|
pdewilde marked this conversation as resolved.
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
|
|
||
| var result github.Installation | ||
| _, err = client.Do(ctx, req, &result) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get enterprise installation: %w", err) | ||
| } | ||
|
|
||
| return app.InstallationForID(ctx, fmt.Sprint(result.GetID())) | ||
|
Check failure on line 182 in pkg/server/source/github.go
|
||
| } | ||
|
|
||
| // MintEnterpriseAccessToken implements SourceSystem. | ||
| func (g *gitHubSourceSystem) MintEnterpriseAccessToken(ctx context.Context, enterprise string, permissions map[string]string) (string, error) { | ||
| var errs []error | ||
| var installation *githubauth.AppInstallation | ||
| var err error | ||
|
|
||
| logger := logging.FromContext(ctx) | ||
| for _, app := range g.apps { | ||
| installation, err = g.installationForEnterprise(ctx, app, enterprise) | ||
| if err != nil { | ||
| errs = append(errs, err) | ||
| } else { | ||
| break | ||
| } | ||
| } | ||
| if len(errs) != 0 { | ||
| return "", fmt.Errorf("errors retrieving GitHub enterprise installation: %w", errors.Join(errs...)) | ||
| } | ||
|
|
||
| allRepoRequest := &githubauth.TokenRequestAllRepos{Permissions: map[string]string{}} | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the right type of token?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Antigravity: Yes, AccessTokenAllRepos sends POST /app/installations/{installation_id}/access_tokens with {"permissions": ...} without specifying a repository array, which is the required GitHub payload for an Enterprise App installation access token. |
||
| if permissions != nil { | ||
| allRepoRequest.Permissions = permissions | ||
| } | ||
|
|
||
| logger.InfoContext(ctx, "sending request for enterprise to GitHub", "request", allRepoRequest) | ||
| accessToken, err := installation.AccessTokenAllRepos(ctx, allRepoRequest) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "invalid http response status (expected 404 to be 201):") || strings.Contains(err.Error(), "invalid http response status (expected 422 to be 201):") { | ||
| logger.WarnContext(ctx, "error generating GitHub enterprise access token", "error", err) | ||
| return "", nil | ||
| } | ||
| return "", fmt.Errorf("error generating GitHub enterprise access token: %w", err) | ||
| } | ||
| return accessToken, nil | ||
| } | ||
|
|
||
| // RetrieveFileContents implements SourceSystem. | ||
| func (g *gitHubSourceSystem) RetrieveFileContents(ctx context.Context, org, repo, filePath, ref string) ([]byte, error) { | ||
| logger := logging.FromContext(ctx) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,7 @@ type TokenMinterServer struct { | |
| // installation access token. | ||
| type TokenRequest struct { | ||
| OrgName string `json:"org_name"` | ||
| Enterprise string `json:"enterprise,omitempty"` | ||
| Repositories []string `json:"repositories"` | ||
| Permissions map[string]string `json:"permissions"` | ||
| Scope string `json:"scope"` | ||
|
|
@@ -156,6 +157,59 @@ func (s *TokenMinterServer) processRequest(r *http.Request) *apiResponse { | |
| return apiError | ||
| } | ||
|
|
||
| // If enterprise is requested, mint an enterprise-scoped access token directly. | ||
| if request.Enterprise != "" { | ||
| if request.OrgName != "" || len(request.Repositories) > 0 { | ||
| return &apiResponse{ | ||
| http.StatusBadRequest, | ||
| "request for 'enterprise' also contained request for 'org_name' or 'repositories' and that is not allowed", | ||
| errors.New("request for 'enterprise' also contained request for 'org_name' or 'repositories' and that is not allowed"), | ||
| } | ||
| } | ||
| scope, _, err := s.configStore.EvalEnterprise(ctx, request.Scope, claims.asMap()) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "not enabled") { | ||
| return &apiResponse{ | ||
| http.StatusForbidden, | ||
| err.Error(), | ||
| err, | ||
| } | ||
| } | ||
| return &apiResponse{ | ||
| http.StatusForbidden, | ||
| fmt.Sprintf("requested scope %q is not found", request.Scope), | ||
| fmt.Errorf("error evaluating enterprise scope: %w", err), | ||
| } | ||
| } | ||
| if len(request.Permissions) == 0 { | ||
| if _, ok := scope.Permissions["*"]; ok { | ||
| request.Permissions = nil | ||
| } else { | ||
| request.Permissions = scope.Permissions | ||
| } | ||
| } | ||
| if err = validatePermissions(scope.Permissions, request.Permissions); err != nil { | ||
|
pdewilde marked this conversation as resolved.
|
||
| return &apiResponse{http.StatusForbidden, "requested permissions are not authorized for this scope", err} | ||
| } | ||
| if s.enforceReadOnly { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is relevant for enterprise scoped tokens
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Antigravity: This logic handles the global --enforce-read-only flag to ensure any minted token (including enterprise tokens) respects read-only mode if enabled globally.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My point is there are a lot of claim scopes besides those two which can mutate. This is also relevant outside of enterprise scoped tokens as well.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Antigravity: Done. Replaced hardcoded contents/metadata map with enforceReadOnlyPermissions, which dynamically downgrades any requested/scope permission to "read" rather than stripping other permission keys.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets undo that for now. I misunderstood the existing state and thought it was only downgrading those two, but it instead just replaces the full permisison list with those two. I'm still not sure it makes sense for enterprise scope, maybe we should just throw an error if that read only permissions flag is set, I don't want to possibly escalate the permisssion we allow right now compared to existing state, as there may be a good reason we do it the way we do.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Antigravity: Done. Reverted standard org read-only behavior back to its original contents/metadata map, and updated enterprise token handling to explicitly return HTTP 403 Forbidden ("read-only mode is not supported for enterprise scoped tokens") when --enforce-read-only is active.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Antigravity: Done. Updated the error response for enterprise token requests in read-only mode to return HTTP 400 Bad Request. |
||
| return &apiResponse{ | ||
| http.StatusBadRequest, | ||
| "read-only mode is not supported for enterprise scoped tokens", | ||
| errors.New("read-only mode is not supported for enterprise scoped tokens"), | ||
| } | ||
| } | ||
| logger.InfoContext(ctx, "generating enterprise token", | ||
| "claims", claims, | ||
| "enterprise", request.Enterprise, | ||
| "request_permissions", request.Permissions, | ||
| ) | ||
| accessToken, err := s.sourceSystem.MintEnterpriseAccessToken(ctx, request.Enterprise, request.Permissions) | ||
| if err != nil { | ||
| return &apiResponse{http.StatusInternalServerError, "error generating enterprise access token", fmt.Errorf("error generating enterprise access token: %w", err)} | ||
| } | ||
| return &apiResponse{http.StatusOK, accessToken, nil} | ||
| } | ||
|
|
||
| // Determine the org name to be used for the request | ||
| requestOrgName, apiError := validateOrgName(request.OrgName, claims.ParsedOrgName) | ||
| if apiError != nil { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic should be unit tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Antigravity: Done. Added dedicated unit tests TestConfigEvaluator_EvalEnterprise in pkg/config/config_evaluator_test.go covering disabled state, multi-org fallback, and all-org failure scenarios.