diff --git a/pkg/handlers/handlers.go b/pkg/handlers/handlers.go index 214fe9a..c63a147 100644 --- a/pkg/handlers/handlers.go +++ b/pkg/handlers/handlers.go @@ -53,7 +53,9 @@ type Handlers struct { gh *github.Client pinProxyURL string // pins caches the pinned commit by module@version: the proxy never changes it. - pins *sync.Map + pins *sync.Map + // tagHashes caches the hash of moved tag archives by owner/repo@commit. + tagHashes *sync.Map httpClient *http.Client tracer trace.Tracer } @@ -67,6 +69,7 @@ func New(store Storer, goProxy *goproxy.Client, gh *github.Client, pinProxyURL s gh: gh, pinProxyURL: pinProxyURL, pins: &sync.Map{}, + tagHashes: &sync.Map{}, httpClient: &http.Client{Timeout: 10 * time.Second}, tracer: otel.GetTracerProvider().Tracer("handler"), } diff --git a/pkg/handlers/module.go b/pkg/handlers/module.go index e3ebab8..380f63d 100644 --- a/pkg/handlers/module.go +++ b/pkg/handlers/module.go @@ -236,7 +236,7 @@ func (h Handlers) downloadGitHub(ctx context.Context, moduleName, version string logger := log.With().Str("module_name", moduleName).Str("module_version", version).Logger() - _, errHash := h.store.GetHashByName(ctxDownload, moduleName, version) + ph, errHash := h.store.GetHashByName(ctxDownload, moduleName, version) if errHash != nil && !errors.As(errHash, &db.NotFoundError{}) { span.RecordError(errHash) logger.Error().Err(errHash).Msg("Failed to get plugin hash") @@ -247,7 +247,12 @@ func (h Handlers) downloadGitHub(ctx context.Context, moduleName, version string recorded := errHash == nil - request, err := h.getArchiveLinkRequest(ctxDownload, moduleName, version, recorded) + var recordedHash string + if recorded { + recordedHash = ph.Hash + } + + request, err := h.getArchiveLinkRequest(ctxDownload, moduleName, version, recordedHash) if err != nil { span.RecordError(err) logger.Error().Err(err).Msg("Failed to get archive link") @@ -481,26 +486,37 @@ func (h Handlers) downloadGitHubFromAssets(ctx context.Context, moduleName, vers } } -func (h Handlers) getArchiveLinkRequest(ctx context.Context, moduleName, version string, recorded bool) (*http.Request, error) { +// getArchiveLinkRequest builds the request downloading the GitHub zipball to serve for moduleName@version. +// recordedHash is the hash recorded for this version, empty if none. +func (h Handlers) getArchiveLinkRequest(ctx context.Context, moduleName, version, recordedHash string) (*http.Request, error) { ctx, span := h.tracer.Start(ctx, "handler_getArchiveLinkRequest") defer span.End() owner, repoName := path.Split(strings.TrimPrefix(moduleName, "github.com/")) owner = strings.TrimSuffix(owner, "/") - ref, err := h.archiveRef(ctx, owner, repoName, moduleName, version, recorded) + ref, err := h.archiveRef(ctx, owner, repoName, moduleName, version, recordedHash) if err != nil { span.RecordError(err) return nil, fmt.Errorf("failed to get archive ref: %w", err) } + req, err := h.zipballRequest(ctx, owner, repoName, ref) + if err != nil { + span.RecordError(err) + + return nil, err + } + + return req, nil +} + +func (h Handlers) zipballRequest(ctx context.Context, owner, repoName, ref string) (*http.Request, error) { opts := &github.RepositoryContentGetOptions{Ref: ref} link, _, err := h.gh.Repositories.GetArchiveLink(ctx, owner, repoName, github.Zipball, opts, 3) if err != nil { - span.RecordError(err) - return nil, fmt.Errorf("failed to get archive link: %w", err) } diff --git a/pkg/handlers/pinned.go b/pkg/handlers/pinned.go index b644c3b..5d2f9b8 100644 --- a/pkg/handlers/pinned.go +++ b/pkg/handlers/pinned.go @@ -2,6 +2,7 @@ package handlers import ( "context" + "crypto/sha256" "encoding/json" "errors" "fmt" @@ -22,11 +23,12 @@ var errNotPinned = errors.New("no pinned commit") // and the pinned commit once the tag moved or was deleted. // Requesting the tag keeps the archive bytes: the zipball of an annotated tag is named after the tag object, not the commit. // The pinned commit of a moved annotated tag cannot give those bytes back, nor match a hash recorded after the move: -// those hashes are realigned with `piceus realign --only`, reviewed one by one. +// a moved tag whose archive still matches the recorded hash is served, since validate accepts exactly those bytes. // Without an answer from the Go module proxy, it falls back to the tag, as before pinning. // When GitHub no longer has the pinned commit, the moved tag is served only if a hash is recorded: // validate then rejects any other bytes, and no hash is ever recorded from code the proxy did not pin. -func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, version string, recorded bool) (string, error) { +// recordedHash is the hash recorded for this version, empty if none. +func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, version, recordedHash string) (string, error) { if h.pinProxyURL == "" { return version, nil } @@ -54,18 +56,31 @@ func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, v case unchanged: return version, nil case moved: - return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, sha, recorded) + return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, sha, recordedHash) case deleted: - return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, "", recorded) + return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, "", recordedHash) default: return "", fmt.Errorf("resolve tag %s: %w", version, err) } } // refAfterTagChange returns the ref to serve once the tag moved to tagCommit, or was deleted when tagCommit is empty. -func (h Handlers) refAfterTagChange(ctx context.Context, owner, repoName, moduleName, version, pinned, tagCommit string, recorded bool) (string, error) { +func (h Handlers) refAfterTagChange(ctx context.Context, owner, repoName, moduleName, version, pinned, tagCommit, recordedHash string) (string, error) { logger := log.With().Str("module_name", moduleName).Str("module_version", version).Str("pinned_commit", pinned).Logger() + if tagCommit != "" && recordedHash != "" { + matches, err := h.tagMatchesRecorded(ctx, owner, repoName, version, tagCommit, recordedHash) + if err != nil { + logger.Warn().Err(err).Str("tag_commit", tagCommit).Msg("Failed to check the moved tag against the recorded hash, serving as if it does not match") + } + + if matches { + logger.Info().Str("tag_commit", tagCommit).Msg("Tag moved after publication, serving the tag: its archive matches the recorded hash") + + return version, nil + } + } + gone, err := h.commitGone(ctx, owner, repoName, pinned) if err != nil { return "", err @@ -80,7 +95,7 @@ func (h Handlers) refAfterTagChange(ctx context.Context, owner, repoName, module logger.Info().Msg("Tag not found, serving the pinned commit") return pinned, nil - case tagCommit != "" && recorded: + case tagCommit != "" && recordedHash != "": logger.Warn().Str("tag_commit", tagCommit).Msg("Pinned commit gone from GitHub, serving the moved tag, checked against the recorded hash") return version, nil @@ -89,6 +104,36 @@ func (h Handlers) refAfterTagChange(ctx context.Context, owner, repoName, module } } +// tagMatchesRecorded reports whether the zipball of the tag version, pointing at tagCommit, has the recorded hash. +func (h Handlers) tagMatchesRecorded(ctx context.Context, owner, repoName, version, tagCommit, recordedHash string) (bool, error) { + // Cache the hash, not the answer: the recorded hash can be realigned. + // The zipball of an annotated tag names the tag object, so two tags on one commit differ. + key := owner + "/" + repoName + "@" + version + "@" + tagCommit + + if cached, ok := h.tagHashes.Load(key); ok { + if sum, ok := cached.(string); ok { + return sum == recordedHash, nil + } + } + + req, err := h.zipballRequest(ctx, owner, repoName, version) + if err != nil { + return false, fmt.Errorf("tag %s: %w", version, err) + } + + hash := sha256.New() + + if _, err = h.gh.Do(ctx, req, hash); err != nil { + return false, fmt.Errorf("download tag %s: %w", version, err) + } + + sum := fmt.Sprintf("%x", hash.Sum(nil)) + + h.tagHashes.Store(key, sum) + + return sum == recordedHash, nil +} + // commitGone reports whether GitHub no longer has the commit sha. func (h Handlers) commitGone(ctx context.Context, owner, repoName, sha string) (bool, error) { // A conditional request answered 304 is not counted in the GitHub rate limit. diff --git a/pkg/handlers/pinned_test.go b/pkg/handlers/pinned_test.go index 022ffb8..27983e1 100644 --- a/pkg/handlers/pinned_test.go +++ b/pkg/handlers/pinned_test.go @@ -1,6 +1,7 @@ package handlers import ( + "crypto/sha256" "fmt" "net/http" "net/http/httptest" @@ -28,7 +29,7 @@ func TestArchiveRef(t *testing.T) { rw.WriteHeader(http.StatusGone) }) - for _, repo := range []string{"stable", "moved", "deleted", "notfound", "ghdown", "movedgone", "deletedgone", "checkdown"} { + for _, repo := range []string{"stable", "moved", "deleted", "notfound", "ghdown", "movedgone", "deletedgone", "checkdown", "movedmatch", "archivedown", "archivedowngone"} { proxy.HandleFunc("/github.com/o/"+repo+"/@v/v1.0.0.info", func(rw http.ResponseWriter, _ *http.Request) { _, _ = fmt.Fprint(rw, `{"Version":"v1.0.0","Origin":{"VCS":"git","Hash":"pinned"}}`) }) @@ -55,6 +56,9 @@ func TestArchiveRef(t *testing.T) { } } + // The URL of the fake GitHub, set once it starts: the zipball endpoint redirects to an absolute URL. + var ghURL string + gh := http.NewServeMux() gh.HandleFunc("/repos/o/stable/commits/v1.0.0", commit("pinned")) gh.HandleFunc("/repos/o/moved/commits/v1.0.0", commit("other")) @@ -64,28 +68,50 @@ func TestArchiveRef(t *testing.T) { gh.HandleFunc("/repos/o/movedgone/commits/v1.0.0", commit("other")) gh.HandleFunc("/repos/o/deletedgone/commits/v1.0.0", status(http.StatusUnprocessableEntity)) gh.HandleFunc("/repos/o/checkdown/commits/v1.0.0", commit("other")) + gh.HandleFunc("/repos/o/movedmatch/commits/v1.0.0", commit("other")) + gh.HandleFunc("/repos/o/archivedown/commits/v1.0.0", commit("other")) + gh.HandleFunc("/repos/o/archivedowngone/commits/v1.0.0", commit("other")) // The pinned commit, still on GitHub or gone. - for _, repo := range []string{"moved", "deleted", "notfound"} { + for _, repo := range []string{"moved", "deleted", "notfound", "movedmatch", "archivedown"} { gh.HandleFunc("/repos/o/"+repo+"/commits/pinned", commit("pinned")) } + // The zipball of the moved tags. + for _, repo := range []string{"moved", "movedgone", "checkdown", "movedmatch"} { + gh.HandleFunc("/repos/o/"+repo+"/zipball/v1.0.0", func(rw http.ResponseWriter, _ *http.Request) { + rw.Header().Set("Location", ghURL+"/archives/"+repo) + rw.WriteHeader(http.StatusFound) + }) + } + + gh.HandleFunc("/archives/", func(rw http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprint(rw, "tag archive") + }) + gh.HandleFunc("/repos/o/archivedown/zipball/v1.0.0", status(http.StatusServiceUnavailable)) + gh.HandleFunc("/repos/o/archivedowngone/zipball/v1.0.0", status(http.StatusServiceUnavailable)) + gh.HandleFunc("/repos/o/movedgone/commits/pinned", status(http.StatusUnprocessableEntity)) + gh.HandleFunc("/repos/o/archivedowngone/commits/pinned", status(http.StatusUnprocessableEntity)) gh.HandleFunc("/repos/o/deletedgone/commits/pinned", status(http.StatusUnprocessableEntity)) gh.HandleFunc("/repos/o/checkdown/commits/pinned", status(http.StatusServiceUnavailable)) ghSrv := httptest.NewServer(gh) t.Cleanup(ghSrv.Close) + ghURL = ghSrv.URL + ghClient := github.NewClient(nil) ghClient.BaseURL, _ = url.Parse(ghSrv.URL + "/") h := New(nil, nil, ghClient, proxySrv.URL) + tagArchive := fmt.Sprintf("%x", sha256.Sum256([]byte("tag archive"))) + testCases := []struct { desc string repo string - recorded bool + recorded string ref string wantErr bool }{ @@ -99,12 +125,20 @@ func TestArchiveRef(t *testing.T) { {repo: "proxydown", ref: "v1.0.0"}, {repo: "proxygone", ref: "v1.0.0"}, {repo: "Bad!Path", ref: "v1.0.0"}, + // The pinned commit, the recorded hash is from other bytes. + {desc: "moved recorded", repo: "moved", recorded: "other", ref: "pinned"}, + // validate accepts exactly the bytes of the moved tag. + {desc: "moved recorded from the tag", repo: "movedmatch", recorded: tagArchive, ref: "v1.0.0"}, + {desc: "moved not recorded", repo: "movedmatch", ref: "pinned"}, + // The check failed: served as if the tag does not match. + {desc: "tag archive down", repo: "archivedown", recorded: tagArchive, ref: "pinned"}, + {desc: "tag archive down, pinned commit gone", repo: "archivedowngone", recorded: tagArchive, ref: "v1.0.0"}, // validate checks the moved tag against the recorded hash. - {desc: "movedgone recorded", repo: "movedgone", recorded: true, ref: "v1.0.0"}, + {desc: "movedgone recorded", repo: "movedgone", recorded: "other", ref: "v1.0.0"}, // Serving the moved tag would record a hash from code the proxy did not pin. {desc: "movedgone not recorded", repo: "movedgone", wantErr: true}, - {desc: "deletedgone recorded", repo: "deletedgone", recorded: true, wantErr: true}, - {repo: "checkdown", recorded: true, wantErr: true}, + {desc: "deletedgone recorded", repo: "deletedgone", recorded: "other", wantErr: true}, + {repo: "checkdown", recorded: "other", wantErr: true}, } for _, test := range testCases { @@ -150,8 +184,60 @@ func TestPinnedCommit_cached(t *testing.T) { func TestArchiveRef_noPinProxy(t *testing.T) { h := New(nil, nil, github.NewClient(nil), "") - ref, err := h.archiveRef(t.Context(), "o", "moved", "github.com/o/moved", "v1.0.0", false) + ref, err := h.archiveRef(t.Context(), "o", "moved", "github.com/o/moved", "v1.0.0", "") require.NoError(t, err) assert.Equal(t, "v1.0.0", ref) } + +func TestTagMatchesRecorded_cached(t *testing.T) { + var downloads atomic.Int32 + + var ghURL string + + gh := http.NewServeMux() + gh.HandleFunc("/repos/o/r/zipball/v1.0.0", func(rw http.ResponseWriter, _ *http.Request) { + rw.Header().Set("Location", ghURL+"/archive") + rw.WriteHeader(http.StatusFound) + }) + gh.HandleFunc("/repos/o/r/zipball/v1.0.1", func(rw http.ResponseWriter, _ *http.Request) { + rw.Header().Set("Location", ghURL+"/archive-v1.0.1") + rw.WriteHeader(http.StatusFound) + }) + gh.HandleFunc("/archive", func(rw http.ResponseWriter, _ *http.Request) { + downloads.Add(1) + _, _ = fmt.Fprint(rw, "tag archive") + }) + // An annotated tag on the same commit: its zipball names another tag object. + gh.HandleFunc("/archive-v1.0.1", func(rw http.ResponseWriter, _ *http.Request) { + downloads.Add(1) + _, _ = fmt.Fprint(rw, "other tag archive") + }) + + ghSrv := httptest.NewServer(gh) + t.Cleanup(ghSrv.Close) + + ghURL = ghSrv.URL + + ghClient := github.NewClient(nil) + ghClient.BaseURL, _ = url.Parse(ghSrv.URL + "/") + + h := New(nil, nil, ghClient, "") + + tagArchive := fmt.Sprintf("%x", sha256.Sum256([]byte("tag archive"))) + + matches, err := h.tagMatchesRecorded(t.Context(), "o", "r", "v1.0.0", "tagcommit", tagArchive) + require.NoError(t, err) + assert.True(t, matches) + + // A realigned hash no longer matches the cached archive. + matches, err = h.tagMatchesRecorded(t.Context(), "o", "r", "v1.0.0", "tagcommit", "realigned") + require.NoError(t, err) + assert.False(t, matches) + + matches, err = h.tagMatchesRecorded(t.Context(), "o", "r", "v1.0.1", "tagcommit", fmt.Sprintf("%x", sha256.Sum256([]byte("other tag archive")))) + require.NoError(t, err) + assert.True(t, matches) + + assert.Equal(t, int32(2), downloads.Load()) +}