Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 28 additions & 12 deletions mocks/vcs/mocks/mock_MockClient.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

52 changes: 42 additions & 10 deletions pkg/archive/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -204,14 +204,16 @@ The manager gets VCS-specific auth headers:

**GitHub**:
```go
// Returns: {"Authorization": "Bearer ghp_xxxxx"}
authHeaders := githubClient.GetAuthHeaders()
// Returns: {"Authorization": "Bearer <token>"}
// For GitHub App auth, the token is fetched fresh from the installation transport,
// so the call may make a network request and must be passed a context.
authHeaders, err := githubClient.GetAuthHeaders(ctx)
```

**GitLab**:
```go
// Returns: {"PRIVATE-TOKEN": "glpat_xxxxx"}
authHeaders := gitlabClient.GetAuthHeaders()
authHeaders, err := gitlabClient.GetAuthHeaders(ctx)
```

### 4. Cache Lookup
Expand Down Expand Up @@ -291,9 +293,19 @@ https://api.github.com/repos/{owner}/{repo}/zipball/{merge_commit_sha}

**Authentication**:
```
Authorization: Bearer ghp_xxxxxxxxxxxxx
Authorization: Bearer <token>
```

Supported token sources:
- Classic PAT (`ghp_*`) or fine-grained PAT (`github_pat_*`) via `KUBECHECKS_VCS_TOKEN`.
- GitHub App installation token (`ghs_*`) — fetched fresh from the installation transport
on each archive download; no static token is needed in config.
- OAuth user tokens (`gho_*`) are also accepted when set via `KUBECHECKS_VCS_TOKEN`.

The REST API zipball endpoint 302s to a signed codeload URL on a different host;
Go's `http.Client` strips the `Authorization` header on that cross-host redirect by
default, so the bearer token is not leaked to the signed URL.

**Merge Commit SHA**:
- GitHub provides `merge_commit_sha` in PR API response
- This is a real commit representing the merged state
Expand All @@ -308,10 +320,24 @@ Authorization: Bearer ghp_xxxxxxxxxxxxx
```go
// pkg/vcs/github_client/client.go

func (c *Client) GetAuthHeaders() map[string]string {
return map[string]string{
"Authorization": fmt.Sprintf("Bearer %s", c.cfg.VcsToken),
func (c *Client) GetAuthHeaders(ctx context.Context) (map[string]string, error) {
token := c.cfg.VcsToken
if c.appTokenSource != nil {
// GitHub App: fetch a fresh installation token.
t, err := c.appTokenSource.Token(ctx)
if err != nil {
return nil, errors.Wrap(err, "failed to fetch GitHub App installation token")
}
token = t
}
if token == "" {
// Surfaces as a typed authError to PostArchiveErrorMessage so the PR comment
// explains it's a config problem, not a transient one to retry.
return nil, errors.New("no GitHub credentials configured for archive download")
}
return map[string]string{
"Authorization": fmt.Sprintf("Bearer %s", token),
}, nil
}

func (c *Client) DownloadArchive(ctx context.Context, pr vcs.PullRequest) (string, error) {
Expand Down Expand Up @@ -375,10 +401,10 @@ PRIVATE-TOKEN: glpat_xxxxxxxxxxxxx
```go
// pkg/vcs/gitlab_client/client.go

func (c *Client) GetAuthHeaders() map[string]string {
func (c *Client) GetAuthHeaders(_ context.Context) (map[string]string, error) {
return map[string]string{
"PRIVATE-TOKEN": c.cfg.VcsToken,
}
}, nil
}

func (c *Client) DownloadArchive(ctx context.Context, pr vcs.PullRequest) (string, error) {
Expand Down Expand Up @@ -716,10 +742,16 @@ Kubechecks will immediately fall back to git mode.

### Authentication

- **GitHub**: Uses Bearer token (PAT or App token)
- **GitHub**: Uses Bearer token. Sources:
- Classic PAT, fine-grained PAT, or OAuth user token from `KUBECHECKS_VCS_TOKEN`.
- GitHub App installation token — fetched fresh from the installation transport at
download time, so rotation is handled automatically and no static token is required.
- **GitLab**: Uses Private token
- **Storage**: Tokens are stored in memory, never written to disk
- **Headers**: Auth headers are added per-request, not globally
- **Cross-host redirects**: The GitHub REST zipball endpoint 302s to a signed codeload
URL. Go's `http.Client` strips `Authorization` on cross-host redirects by default, so
the bearer token is not leaked to the codeload host.

### Archive Integrity

Expand Down
87 changes: 87 additions & 0 deletions pkg/archive/download_test.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
package archive

import (
"archive/zip"
"bytes"
"context"
"fmt"
"net/http"
"net/http/httptest"
"os"
"path/filepath"
"strings"
"testing"

"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestIsRetriableDownloadError(t *testing.T) {
Expand Down Expand Up @@ -136,3 +143,83 @@ func TestIsRetriableDownloadError(t *testing.T) {
})
}
}

// buildTestZip returns a small in-memory zip mirroring the GitHub archive layout: an
// explicit top-level directory entry `{repo}-{sha}/` followed by a file underneath.
// The directory entry is what the extractor uses to detect and strip the wrapper folder.
func buildTestZip(t *testing.T) []byte {
t.Helper()
var buf bytes.Buffer
zw := zip.NewWriter(&buf)

dirHdr := &zip.FileHeader{Name: "repo-deadbeef/"}
dirHdr.SetMode(0o755 | os.ModeDir)
_, err := zw.CreateHeader(dirHdr)
require.NoError(t, err)

fileHdr := &zip.FileHeader{Name: "repo-deadbeef/README.md"}
fileHdr.SetMode(0o644)
w, err := zw.CreateHeader(fileHdr)
require.NoError(t, err)
_, err = w.Write([]byte("hello from zip"))
require.NoError(t, err)
require.NoError(t, zw.Close())
return buf.Bytes()
}

// TestDownloader_FollowsRedirect locks in the redirect behavior the GitHub zipball flow
// depends on: api.github.com responds with 302 to a signed codeload URL, and the
// downloader must follow that redirect to retrieve the archive. Guards against a future
// http.Client customization (e.g. CheckRedirect: ErrUseLastResponse) silently breaking
// archive downloads.
//
// Note: this test does NOT assert Authorization-header stripping. httptest servers all
// bind to 127.0.0.1, and Go's net/http compares hostnames (not host:port) for the
// sensitive-header decision, so within httptest the header is always forwarded. The
// cross-host stripping for api.github.com → codeload.github.com is a Go stdlib invariant
// covered by the standard library's own tests.
func TestDownloader_FollowsRedirect(t *testing.T) {
zipBytes := buildTestZip(t)

var (
apiHits int
codeloadHits int
)

// "codeload" stand-in: serves the archive bytes.
codeload := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
codeloadHits++
w.Header().Set("Content-Type", "application/zip")
_, _ = w.Write(zipBytes)
}))
defer codeload.Close()

// "api" stand-in: requires Authorization, 302s to codeload (mirroring how
// api.github.com/repos/.../zipball/SHA redirects to a signed codeload URL).
api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
apiHits++
assert.Equal(t, "Bearer test-token", r.Header.Get("Authorization"),
"original request to API must carry the Bearer token")
http.Redirect(w, r, codeload.URL+"/signed-archive.zip?token=abc", http.StatusFound)
}))
defer api.Close()

targetDir := filepath.Join(t.TempDir(), "extract")
d := NewDownloader()
extractedPath, err := d.DownloadAndExtract(
context.Background(),
api.URL+"/repos/owner/repo/zipball/deadbeef",
targetDir,
map[string]string{"Authorization": "Bearer test-token"},
)
require.NoError(t, err)

assert.Equal(t, 1, apiHits, "API endpoint should be hit exactly once")
assert.Equal(t, 1, codeloadHits, "Downloader must follow the 302 and fetch from the redirect target")
assert.True(t, strings.HasSuffix(extractedPath, "repo-deadbeef"),
"extractor should strip the top-level wrapper directory: got %q", extractedPath)

body, err := os.ReadFile(filepath.Join(extractedPath, "README.md"))
require.NoError(t, err)
assert.Equal(t, "hello from zip", string(body))
}
55 changes: 48 additions & 7 deletions pkg/archive/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ type urlParseError struct{ err error }
func (e *urlParseError) Error() string { return e.err.Error() }
func (e *urlParseError) Unwrap() error { return e.err }

// authError wraps failures from vcs.Client.GetAuthHeaders. These are config/permission
// issues (missing credentials, bad App private key, JWT exchange failure) rather than
// transient network blips — retrying via replan won't make the credentials valid.
// Without this typing, such failures would fall through PostArchiveErrorMessage's
// catch-all and show a misleading "transient error, comment to retry" message.
type authError struct{ err error }

func (e *authError) Error() string { return e.err.Error() }
func (e *authError) Unwrap() error { return e.err }

// Manager manages archive-based repository access
// It provides a similar interface to git.RepoManager but uses VCS archives instead
type Manager struct {
Expand Down Expand Up @@ -61,10 +71,12 @@ func (m *Manager) Clone(ctx context.Context, cloneURL, branchName string, pr vcs
return nil, errors.Wrap(err, "failed to get archive URL from VCS")
}

// Extract merge commit SHA from archive URL for cache key
// Archive URLs contain the SHA: https://github.com/owner/repo/archive/{sha}.zip
// Extract merge commit SHA from archive URL for cache key.
// Archive URLs contain the SHA, e.g.:
// GitHub: https://api.github.com/repos/owner/repo/zipball/{sha}
// GitLab: https://gitlab.com/api/v4/projects/{id}/repository/archive.zip?sha={ref}
// IMPORTANT: Must use merge commit SHA, not HEAD SHA, as cache key!
// Otherwise, cache returns stale archives when new commits are pushed to existing PR
// Otherwise, cache returns stale archives when new commits are pushed to existing PR.
mergeCommitSHA, err := extractSHAFromArchiveURL(archiveURL)
if err != nil {
return nil, &urlParseError{err: errors.Wrap(err, "failed to extract merge commit SHA from archive URL")}
Expand All @@ -77,8 +89,12 @@ func (m *Manager) Clone(ctx context.Context, cloneURL, branchName string, pr vcs
Str("head_sha", pr.SHA).
Msg("extracted merge commit SHA from archive URL")

// Get authentication headers for archive download
authHeaders := m.vcsClient.GetAuthHeaders()
// Get authentication headers for archive download. For GitHub App auth this fetches
// a fresh installation token, so the call needs the request context.
authHeaders, err := m.vcsClient.GetAuthHeaders(ctx)
if err != nil {
return nil, &authError{err: errors.Wrap(err, "failed to get archive auth headers")}
}
Comment thread
fasher marked this conversation as resolved.

// Download and extract archive (or get from cache)
extractedPath, err := m.cache.GetOrDownload(ctx, archiveURL, mergeCommitSHA, authHeaders)
Expand Down Expand Up @@ -186,6 +202,7 @@ func (m *Manager) PostArchiveErrorMessage(ctx context.Context, pr vcs.PullReques
// Classify by the error itself first; ctx.Err() is a fallback for cases where the
// context was cancelled for an unrelated reason after Clone returned.
var urlErr *urlParseError
var authErr *authError
var httpErr *HTTPError
hasHTTP := errors.As(cloneErr, &httpErr)

Expand Down Expand Up @@ -241,6 +258,15 @@ func (m *Manager) PostArchiveErrorMessage(ctx context.Context, pr vcs.PullReques
message = "⚠️ Access denied downloading the repository archive (HTTP 403 Forbidden).\n\n" +
"Check that the kubechecks VCS token has sufficient repository permissions."

case errors.As(cloneErr, &authErr):
// Auth header construction failed before any HTTP call (e.g. no credentials
// configured, GitHub App installation token fetch failed, malformed PEM).
// Permanent — retrying via replan won't fix the configuration.
message = "⚠️ Kubechecks could not obtain credentials to download the repository archive.\n\n" +
"This is a configuration problem — check that the kubechecks VCS token or " +
"GitHub App credentials are valid. Comment `" + replan + "` will not help until " +
"the configuration is fixed."

case errors.As(cloneErr, &urlErr):
// Unrecognized archive URL format — this is a bug, not something the user can retry
message = "⚠️ Kubechecks could not parse the archive URL returned by the VCS.\n\n" +
Expand Down Expand Up @@ -301,10 +327,25 @@ func (m *Manager) PostArchiveErrorMessage(ctx context.Context, pr vcs.PullReques

// extractSHAFromArchiveURL extracts the commit SHA from an archive URL
// Supports GitHub and GitLab archive URL formats:
// - GitHub: https://github.com/owner/repo/archive/{sha}.zip
// - GitHub REST API zipball: https://api.github.com/repos/owner/repo/zipball/{sha}
// - GitHub web archive (legacy): https://github.com/owner/repo/archive/{sha}.zip
// - GitLab: https://gitlab.com/api/v4/projects/{encoded}/repository/archive.zip?sha={ref}
func extractSHAFromArchiveURL(archiveURL string) (string, error) {
// Try GitHub format first: /archive/{sha}.zip or /archive/{sha}.tar.gz
// GitHub REST API zipball/tarball: /zipball/{sha} or /tarball/{sha} (no extension)
for _, marker := range []string{"/zipball/", "/tarball/"} {
if !strings.Contains(archiveURL, marker) {
continue
}
parts := strings.Split(archiveURL, marker)
// Strip any query string that may follow the SHA.
sha := strings.SplitN(parts[len(parts)-1], "?", 2)[0]
if sha == "" {
return "", fmt.Errorf("empty SHA extracted from archive URL: %s", archiveURL)
}
return sha, nil
}

// GitHub web archive (legacy): /archive/{sha}.zip or /archive/{sha}.tar.gz
if strings.Contains(archiveURL, "/archive/") {
// Extract filename from URL path
parts := strings.Split(archiveURL, "/archive/")
Expand Down
Loading