feat(serde): support Git repo cloning via GitHub app#2246
feat(serde): support Git repo cloning via GitHub app#2246ivaaaan wants to merge 1 commit intoredpanda-data:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for authenticating Git repository access using GitHub Apps, providing a more secure and organization-friendly authentication method compared to Personal Access Tokens (PATs) or SSH keys. The implementation follows GitHub's App authentication flow by generating short-lived installation access tokens using JWT-signed requests.
Changes:
- Added GitHub App authentication configuration with validation for App ID, Installation ID, and private key
- Implemented token generation and refresh logic with proper caching and concurrency controls
- Integrated the new authentication method into the Git service alongside existing SSH and BasicAuth options
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| backend/pkg/config/github_auth_app.go | New configuration struct for GitHub App authentication with validation |
| backend/pkg/config/git.go | Integration of GitHub App config into main Git configuration |
| backend/pkg/config/git_auth_ssh.go | Unintended whitespace changes |
| backend/pkg/git/github_app_auth.go | Core implementation of GitHub App authentication with JWT signing and token management |
| backend/pkg/git/github_app_auth_test.go | Comprehensive test suite covering key parsing, token refresh, and authentication |
| backend/pkg/git/service.go | Integration of GitHub App auth into the service's authentication switch |
| backend/go.mod | Updated golang-jwt dependency from indirect to direct |
Comments suppressed due to low confidence (6)
backend/pkg/config/github_auth_app.go:46
- The error messages use lowercase "github" while the brand name is "GitHub" (capital H) as seen in other comments and log messages throughout the codebase. Consider updating the error messages to use "GitHub" for consistency with the brand name.
return errors.New("github app authentication is enabled but appId is not set or invalid")
}
if c.InstallationID <= 0 {
return errors.New("github app authentication is enabled but installationId is not set or invalid")
}
if c.PrivateKey == "" && c.PrivateKeyFilePath == "" {
return errors.New("github app authentication is enabled but neither privateKey nor privateKeyFilepath is set")
backend/pkg/config/git_auth_ssh.go:29
- This file has unnecessary blank lines added at the end. These lines appear to be accidental and should be removed to keep the file clean and consistent with the rest of the codebase.
backend/pkg/config/git.go:68 - The Git configuration allows multiple authentication methods to be enabled simultaneously (SSH, BasicAuth, and GithubApp), but the switch statement in service.go will silently use only the first enabled method. Consider adding validation to the Git.Validate method to ensure only one authentication method is enabled at a time, similar to how SASL mechanisms are validated. This would prevent configuration errors and make the behavior more explicit to users.
if err := c.GithubApp.Validate(); err != nil {
return err
}
backend/pkg/git/github_app_auth_test.go:180
- The TestGitHubAppAuth_SetAuth test only covers the success case. Consider adding a test case for when getValidToken fails to ensure the error is properly logged and the request continues without authentication being set. This would verify the error handling behavior matches expectations.
func TestGitHubAppAuth_SetAuth(t *testing.T) {
key := generateTestRSAKey(t)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusCreated)
json.NewEncoder(w).Encode(installationTokenResponse{
Token: "ghs_test_token",
ExpiresAt: time.Now().Add(1 * time.Hour),
})
}))
defer server.Close()
auth := &gitHubAppAuth{
appID: 1,
installationID: 2,
privateKey: key,
logger: slog.Default(),
apiBaseURL: server.URL,
}
req := httptest.NewRequest(http.MethodGet, "https://github.com/test/repo.git", nil)
auth.SetAuth(req)
username, password, ok := req.BasicAuth()
require.True(t, ok)
assert.Equal(t, "x-access-token", username)
assert.Equal(t, "ghs_test_token", password)
}
backend/pkg/git/github_app_auth.go:106
- The refreshToken method uses http.DefaultClient which has no timeout configured. This could cause the application to hang indefinitely if the GitHub API is unresponsive. Consider creating a custom HTTP client with appropriate timeout settings, similar to how backend/pkg/bsr/client.go creates a client with a 30-second timeout. For example, create a client with timeout in the buildGithubAppAuth function and store it in the gitHubAppAuth struct.
resp, err := http.DefaultClient.Do(req)
backend/pkg/git/github_app_auth.go:187
- Consider adding tests for the buildGithubAppAuth function to cover error cases such as invalid private key data and file reading failures. This would ensure the configuration loading logic is properly tested.
func buildGithubAppAuth(cfg config.GitGithubApp, logger *slog.Logger) (*gitHubAppAuth, error) {
pemData, err := loadPrivateKeyPEM(cfg)
if err != nil {
return nil, err
}
privateKey, err := parseRSAPrivateKey(pemData)
if err != nil {
return nil, fmt.Errorf("failed to parse GitHub App private key: %w", err)
}
return &gitHubAppAuth{
appID: cfg.AppID,
installationID: cfg.InstallationID,
privateKey: privateKey,
logger: logger,
apiBaseURL: gitHubAPIBaseURL,
}, nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Currently to use protos from a private GitHub Repo you must setup either PAT or SSH key. Both methods are not recommended for organizations. This PR adds support for authentication via GitHub apps, which is a common way to authenticate apps in organizations.