From 884acba9677402bb018658e6c50ce8661c2ccdd5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 Dec 2024 08:00:32 -0600 Subject: [PATCH] chore: revert back to FileServer returning an http handler Allows bytes to be copied without reading them all into memory first. --- api/api.go | 2 +- go.mod | 5 ++- go.sum | 10 ++--- storage/artifactory.go | 50 +++++++++--------------- storage/local.go | 4 ++ storage/signature.go | 85 ++++++++++++++--------------------------- storage/storage.go | 18 ++------- storage/storage_test.go | 2 +- testutil/mockstorage.go | 13 ------- 9 files changed, 63 insertions(+), 126 deletions(-) diff --git a/api/api.go b/api/api.go index 91a92df..50f07c5 100644 --- a/api/api.go +++ b/api/api.go @@ -112,7 +112,7 @@ func New(options *Options) *API { r.Post("/api/extensionquery", api.extensionQuery) // Endpoint for getting an extension's files or the extension zip. - r.Mount("/files", http.StripPrefix("/files", storage.HTTPFileServer(options.Storage))) + r.Mount("/files", http.StripPrefix("/files", options.Storage.FileServer())) // VS Code can use the files in the response to get file paths but it will // sometimes ignore that and use requests to /assets with hardcoded types to diff --git a/go.mod b/go.mod index 52220e5..06d49a0 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,6 @@ require ( github.com/go-chi/httprate v0.14.1 github.com/google/uuid v1.6.0 github.com/lithammer/fuzzysearch v1.1.8 - github.com/spf13/afero v1.11.0 github.com/spf13/cobra v1.8.1 github.com/stretchr/testify v1.9.0 golang.org/x/mod v0.19.0 @@ -18,6 +17,7 @@ require ( ) require ( + cloud.google.com/go/logging v1.8.1 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/charmbracelet/lipgloss v0.7.1 // indirect @@ -38,6 +38,9 @@ require ( golang.org/x/sys v0.17.0 // indirect golang.org/x/term v0.17.0 // indirect golang.org/x/text v0.14.0 // indirect + google.golang.org/genproto v0.0.0-20231106174013-bbf56f31fb17 // indirect + google.golang.org/genproto/googleapis/api v0.0.0-20231106174013-bbf56f31fb17 // indirect + google.golang.org/genproto/googleapis/rpc v0.0.0-20231120223509-83a465c0220f // indirect google.golang.org/protobuf v1.32.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index f19c00c..9dac1e7 100644 --- a/go.sum +++ b/go.sum @@ -5,10 +5,10 @@ cloud.google.com/go/compute v1.23.3 h1:6sVlXXBmbd7jNX0Ipq0trII3e4n1/MsADLK6a+aiV cloud.google.com/go/compute v1.23.3/go.mod h1:VCgBUoMnIVIR0CscqQiPJLAG25E3ZRZMzcFZeQ+h8CI= cloud.google.com/go/compute/metadata v0.2.3 h1:mg4jlk7mCAj6xXp9UJ4fjI9VUI5rubuGBW5aJ7UnBMY= cloud.google.com/go/compute/metadata v0.2.3/go.mod h1:VAV5nSsACxMJvgaAuX6Pk2AawlZn8kiOGuCv6gTkwuA= -cloud.google.com/go/logging v1.7.0 h1:CJYxlNNNNAMkHp9em/YEXcfJg+rPDg7YfwoRpMU+t5I= -cloud.google.com/go/logging v1.7.0/go.mod h1:3xjP2CjkM3ZkO73aj4ASA5wRPGGCRrPIAeNqVNkzY8M= -cloud.google.com/go/longrunning v0.5.1 h1:Fr7TXftcqTudoyRJa113hyaqlGdiBQkp0Gq7tErFDWI= -cloud.google.com/go/longrunning v0.5.1/go.mod h1:spvimkwdz6SPWKEt/XBij79E9fiTkHSQl/fRUUQJYJc= +cloud.google.com/go/logging v1.8.1 h1:26skQWPeYhvIasWKm48+Eq7oUqdcdbwsCVwz5Ys0FvU= +cloud.google.com/go/logging v1.8.1/go.mod h1:TJjR+SimHwuC8MZ9cjByQulAMgni+RkXeI3wwctHJEI= +cloud.google.com/go/longrunning v0.5.4 h1:w8xEcbZodnA2BbW6sVirkkoC+1gP8wS57EUUgGS0GVg= +cloud.google.com/go/longrunning v0.5.4/go.mod h1:zqNVncI0BOP8ST6XQD1+VcvuShMmq7+xFSzOL++V0dI= github.com/aymanbagabas/go-osc52/v2 v2.0.1 h1:HwpRHbFMcZLEVr42D4p7XBqjyuxQH5SMiErDT4WkJ2k= github.com/aymanbagabas/go-osc52/v2 v2.0.1/go.mod h1:uYgXzlJ7ZpABp8OJ+exZzJJhRNQ2ASbcXHWsFqH8hp8= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= @@ -56,8 +56,6 @@ github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJ github.com/rivo/uniseg v0.4.4 h1:8TfxU8dW6PdqD27gjM8MVNuicgxIjxpm4K7x4jp8sis= github.com/rivo/uniseg v0.4.4/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= -github.com/spf13/afero v1.11.0 h1:WJQKhtpdm3v2IzqG8VMqrr6Rf3UYpEF239Jy9wNepM8= -github.com/spf13/afero v1.11.0/go.mod h1:GH9Y3pIexgf1MTIWtNGyogA5MwRIDXGUr+hbWNoBjkY= github.com/spf13/cobra v1.8.1 h1:e5/vxKd/rZsfSJMUX1agtjeTDf+qv1/JdBF8gg5k9ZM= github.com/spf13/cobra v1.8.1/go.mod h1:wHxEcudfqmLYa8iTfL+OuZPbBZkmvliBWKIezN3kD9Y= github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= diff --git a/storage/artifactory.go b/storage/artifactory.go index ae4ea88..9b15e20 100644 --- a/storage/artifactory.go +++ b/storage/artifactory.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "io" - "io/fs" "net/http" "os" "path" @@ -16,7 +15,6 @@ import ( "sync" "time" - "github.com/spf13/afero/mem" "golang.org/x/sync/errgroup" "golang.org/x/xerrors" @@ -277,37 +275,25 @@ func (s *Artifactory) AddExtension(ctx context.Context, manifest *VSIXManifest, return s.uri + dir, nil } -// Open returns a file from Artifactory. -// TODO: Since we only extract a subset of files perhaps if the file does not -// exist we should download the vsix and extract the requested file as a -// fallback. Obviously this seems like quite a bit of overhead so we would -// then emit a warning so we can notice that VS Code has added new asset types -// that we should be extracting to avoid that overhead. Other solutions could -// be implemented though like extracting the VSIX to disk locally and only -// going to Artifactory for the VSIX when it is missing on disk (basically -// using the disk as a cache). -func (s *Artifactory) Open(ctx context.Context, fp string) (fs.File, error) { - resp, code, err := s.read(ctx, fp) - if code != http.StatusOK || err != nil { - switch code { - case http.StatusNotFound: - return nil, fs.ErrNotExist - case http.StatusForbidden: - return nil, fs.ErrPermission - default: - return nil, err +func (s *Artifactory) FileServer() http.Handler { + // TODO: Since we only extract a subset of files perhaps if the file does not + // exist we should download the vsix and extract the requested file as a + // fallback. Obviously this seems like quite a bit of overhead so we would + // then emit a warning so we can notice that VS Code has added new asset types + // that we should be extracting to avoid that overhead. Other solutions could + // be implemented though like extracting the VSIX to disk locally and only + // going to Artifactory for the VSIX when it is missing on disk (basically + // using the disk as a cache). + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + reader, code, err := s.read(r.Context(), r.URL.Path) + if err != nil { + http.Error(rw, err.Error(), code) + return } - } - - // TODO: Do no copy the bytes into memory, stream them rather than - // storing the entire file into memory. - f := mem.NewFileHandle(mem.CreateFile(fp)) - _, err = io.Copy(f, resp) - if err != nil { - return nil, err - } - - return f, nil + defer reader.Close() + rw.WriteHeader(http.StatusOK) + _, _ = io.Copy(rw, reader) + }) } func (s *Artifactory) Manifest(ctx context.Context, publisher, name string, version Version) (*VSIXManifest, error) { diff --git a/storage/local.go b/storage/local.go index 5293e39..beb908f 100644 --- a/storage/local.go +++ b/storage/local.go @@ -142,6 +142,10 @@ func (s *Local) AddExtension(ctx context.Context, manifest *VSIXManifest, vsix [ return dir, nil } +func (s *Local) FileServer() http.Handler { + return http.FileServer(http.Dir(s.extdir)) +} + func (s *Local) Open(_ context.Context, fp string) (fs.File, error) { return http.Dir(s.extdir).Open(fp) } diff --git a/storage/signature.go b/storage/signature.go index ce917bc..6bf5199 100644 --- a/storage/signature.go +++ b/storage/signature.go @@ -4,15 +4,15 @@ import ( "context" "crypto" "encoding/json" - "io" - "io/fs" - "path/filepath" + "net/http" + "strconv" "strings" - "github.com/spf13/afero/mem" "golang.org/x/xerrors" "cdr.dev/slog" + "github.com/coder/code-marketplace/api/httpapi" + "github.com/coder/code-marketplace/api/httpmw" "github.com/coder/code-marketplace/extensionsign" ) @@ -113,7 +113,7 @@ func (s *Signature) Manifest(ctx context.Context, publisher, name string, versio return manifest, nil } -// Open will intercept requests for signed extensions payload. +// FileServer will intercept requests for signed extensions payload. // It does this by looking for 'SigzipFileExtension' or p7s.sig. // // The signed payload and signing process is taken from: @@ -125,61 +125,32 @@ func (s *Signature) Manifest(ctx context.Context, publisher, name string, versio // the signature. Meaning the signature could be empty, incorrect, or a // picture of cat and it would work. There is no signature verification. // -// - VSCode requires a signature payload to exist, but the context appear -// to be somewhat optional. -// Following another open source implementation, it appears the '.signature.p7s' -// file must exist, but it can be empty. -// The signature is stored in a '.signature.sig' file, although it is unclear -// is VSCode ever reads this file. -// TODO: Properly implement the p7s file, and diverge from the other open -// source implementation. Ideally this marketplace would match Microsoft's -// marketplace API. -func (s *Signature) Open(ctx context.Context, fp string) (fs.File, error) { - if s.SigningEnabled() && strings.HasSuffix(filepath.Base(fp), SigzipFileExtension) { - base := filepath.Base(fp) - vsixPath := strings.TrimSuffix(base, SigzipFileExtension) - - // hijack this request, sign the sig manifest - manifest, err := s.Storage.Open(ctx, filepath.Join(filepath.Dir(fp), sigManifestName)) - if err != nil { - // If this file is missing, it means the extension was added before - // signatures were handled by the marketplace. - // TODO: Generate the sig manifest payload and insert it? - return nil, xerrors.Errorf("open signature manifest: %w", err) - } - defer manifest.Close() - - manifestData, err := io.ReadAll(manifest) - if err != nil { - return nil, xerrors.Errorf("read signature manifest: %w", err) - } - - vsix, err := s.Storage.Open(ctx, filepath.Join(filepath.Dir(fp), vsixPath+".vsix")) - if err != nil { - // If this file is missing, it means the extension was added before - // signatures were handled by the marketplace. - // TODO: Generate the sig manifest payload and insert it? - return nil, xerrors.Errorf("open signature manifest: %w", err) - } - defer vsix.Close() +// - VSCode requires a signature payload to exist, but the content is optional +// for linux users. +// For windows (maybe mac?) users, the signature must be valid, and this +// implementation will not work. +func (s *Signature) FileServer() http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + if s.SigningEnabled() && strings.HasSuffix(r.URL.Path, SigzipFileExtension) { + // hijack this request, return an empty signature payload + signed, err := extensionsign.IncludeEmptySignature() + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, httpapi.ErrorResponse{ + Message: "Unable to generate empty signature for extension", + Detail: err.Error(), + RequestID: httpmw.RequestID(r), + }) + return + } - vsixData, err := io.ReadAll(vsix) - if err != nil { - return nil, xerrors.Errorf("read signature manifest: %w", err) + rw.Header().Set("Content-Length", strconv.FormatInt(int64(len(signed)), 10)) + rw.WriteHeader(http.StatusOK) + _, _ = rw.Write(signed) + return } - // TODO: Fetch the VSIX payload from the storage - signed, err := s.SigZip(ctx, vsixData, manifestData) - if err != nil { - return nil, xerrors.Errorf("sign and zip manifest: %w", err) - } - - f := mem.NewFileHandle(mem.CreateFile(fp)) - _, err = f.Write(signed) - return f, err - } - - return s.Storage.Open(ctx, fp) + s.Storage.FileServer().ServeHTTP(rw, r) + }) } func (s *Signature) SigZip(ctx context.Context, vsix []byte, sigManifest []byte) ([]byte, error) { diff --git a/storage/storage.go b/storage/storage.go index ad5ed13..da21f68 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -213,10 +213,9 @@ type Storage interface { // for verification purposes. Extra files can be included, but not required. // All extra files will be placed relative to the manifest outside the vsix. AddExtension(ctx context.Context, manifest *VSIXManifest, vsix []byte, extra ...File) (string, error) - // Open mirrors the fs.FS interface of Open, except with a context. - // The Open should return files from the extension storage, and used for - // serving extensions. - Open(ctx context.Context, name string) (fs.File, error) + // FileServer provides a handler for fetching extension repository files from + // a client. + FileServer() http.Handler // Manifest returns the manifest bytes for the provided extension. The // extension asset itself (the VSIX) will be included on the manifest even if // it does not exist on the manifest on disk. @@ -239,17 +238,6 @@ type Storage interface { WalkExtensions(ctx context.Context, fn func(manifest *VSIXManifest, versions []Version) error) error } -// HTTPFileServer creates an http.Handler that serves files from the provided -// storage. -func HTTPFileServer(s Storage) http.Handler { - return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - http.FileServerFS(&contextFs{ - ctx: r.Context(), - open: s.Open, - }).ServeHTTP(rw, r) - }) -} - type File struct { RelativePath string Content []byte diff --git a/storage/storage_test.go b/storage/storage_test.go index 8bb649c..d3c4739 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -201,7 +201,7 @@ func testFileServer(t *testing.T, factory storageFactory) { req := httptest.NewRequest("GET", test.path, nil) rec := httptest.NewRecorder() - server := storage.HTTPFileServer(f.storage) + server := f.storage.FileServer() server.ServeHTTP(rec, req) resp := rec.Result() diff --git a/testutil/mockstorage.go b/testutil/mockstorage.go index 0066c40..c1f6249 100644 --- a/testutil/mockstorage.go +++ b/testutil/mockstorage.go @@ -3,14 +3,10 @@ package testutil import ( "context" "errors" - "io/fs" "net/http" "os" - "path/filepath" "sort" - "github.com/spf13/afero/mem" - "github.com/coder/code-marketplace/storage" ) @@ -26,15 +22,6 @@ func NewMockStorage() *MockStorage { func (s *MockStorage) AddExtension(ctx context.Context, manifest *storage.VSIXManifest, vsix []byte, extra ...storage.File) (string, error) { return "", errors.New("not implemented") } -func (s *MockStorage) Open(ctx context.Context, path string) (fs.File, error) { - if filepath.Base(path) == "nonexistent" { - return nil, fs.ErrNotExist - } - - f := mem.NewFileHandle(mem.CreateFile(path)) - _, _ = f.Write([]byte("foobar")) - return f, nil -} func (s *MockStorage) FileServer() http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {