From 6b74043b85ee03215705e789cebf79bc6f604bda Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Thu, 31 Oct 2024 23:28:25 +0800 Subject: [PATCH] Fix `missing signature key` error when pulling Docker images with `SERVE_DIRECT` enabled (#32365) Fix #28121 I did some tests and found that the `missing signature key` error is caused by an incorrect `Content-Type` header. Gitea correctly sets the `Content-Type` header when serving files. https://github.com/go-gitea/gitea/blob/348d1d0f322ca57c459acd902f54821d687ca804/routers/api/packages/container/container.go#L712-L717 However, when `SERVE_DIRECT` is enabled, the `Content-Type` header may be set to an incorrect value by the storage service. To fix this issue, we can use query parameters to override response header values. https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html In this PR, I introduced a new parameter to the `URL` method to support additional parameters. ``` URL(path, name string, reqParams url.Values) (*url.URL, error) ``` --- Most S3-like services support specifying the content type when storing objects. However, Gitea always use `application/octet-stream`. Therefore, I believe we also need to improve the `Save` method to support storing objects with the correct content type. https://github.com/go-gitea/gitea/blob/b7fb20e73e63b8edc9b90c52073e248bef428fcc/modules/storage/minio.go#L214-L221 (cherry picked from commit 0690cb076bf63f71988a709f62a9c04660b51a4f) Conflicts: - modules/storage/azureblob.go Dropped the change, as we do not support Azure blob storage. - modules/storage/helper.go Resolved by adjusting their `discardStorage` to our `DiscardStorage` - routers/api/actions/artifacts.go routers/api/actions/artifactsv4.go routers/web/repo/actions/view.go routers/web/repo/download.go Resolved the conflicts by manually adding the new `nil` parameter to the `storage.Attachments.URL()` calls. Originally conflicted due to differences in the if expression above these calls. --- modules/packages/content_store.go | 4 ++-- modules/storage/helper.go | 2 +- modules/storage/helper_test.go | 2 +- modules/storage/local.go | 2 +- modules/storage/minio.go | 8 ++++++-- modules/storage/storage.go | 2 +- routers/api/actions/artifacts.go | 2 +- routers/api/actions/artifactsv4.go | 2 +- routers/api/packages/container/container.go | 4 +++- routers/api/packages/maven/maven.go | 2 +- routers/api/v1/repo/file.go | 4 ++-- routers/web/base.go | 2 +- routers/web/repo/actions/view.go | 3 ++- routers/web/repo/attachment.go | 2 +- routers/web/repo/download.go | 4 ++-- routers/web/repo/repo.go | 2 +- services/lfs/server.go | 2 +- services/packages/packages.go | 6 +++--- 18 files changed, 31 insertions(+), 24 deletions(-) diff --git a/modules/packages/content_store.go b/modules/packages/content_store.go index da93e6cf6b..6438fb174f 100644 --- a/modules/packages/content_store.go +++ b/modules/packages/content_store.go @@ -37,8 +37,8 @@ func (s *ContentStore) ShouldServeDirect() bool { return setting.Packages.Storage.MinioConfig.ServeDirect } -func (s *ContentStore) GetServeDirectURL(key BlobHash256Key, filename string) (*url.URL, error) { - return s.store.URL(KeyToRelativePath(key), filename) +func (s *ContentStore) GetServeDirectURL(key BlobHash256Key, filename string, reqParams url.Values) (*url.URL, error) { + return s.store.URL(KeyToRelativePath(key), filename, reqParams) } // FIXME: Workaround to be removed in v1.20 diff --git a/modules/storage/helper.go b/modules/storage/helper.go index 95f1c7b9a8..8bec3a0042 100644 --- a/modules/storage/helper.go +++ b/modules/storage/helper.go @@ -30,7 +30,7 @@ func (s DiscardStorage) Delete(_ string) error { return fmt.Errorf("%s", s) } -func (s DiscardStorage) URL(_, _ string) (*url.URL, error) { +func (s DiscardStorage) URL(_, _ string, _ url.Values) (*url.URL, error) { return nil, fmt.Errorf("%s", s) } diff --git a/modules/storage/helper_test.go b/modules/storage/helper_test.go index 60a7c61289..dd30c9b8ac 100644 --- a/modules/storage/helper_test.go +++ b/modules/storage/helper_test.go @@ -38,7 +38,7 @@ func Test_discardStorage(t *testing.T) { require.Error(t, err, string(tt)) } { - got, err := tt.URL("path", "name") + got, err := tt.URL("path", "name", nil) assert.Nil(t, got) require.Errorf(t, err, string(tt)) } diff --git a/modules/storage/local.go b/modules/storage/local.go index 9bb532f1df..00c7f668aa 100644 --- a/modules/storage/local.go +++ b/modules/storage/local.go @@ -114,7 +114,7 @@ func (l *LocalStorage) Delete(path string) error { } // URL gets the redirect URL to a file -func (l *LocalStorage) URL(path, name string) (*url.URL, error) { +func (l *LocalStorage) URL(path, name string, reqParams url.Values) (*url.URL, error) { return nil, ErrURLNotSupported } diff --git a/modules/storage/minio.go b/modules/storage/minio.go index d0c2dec65b..b02eec7aa0 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -276,8 +276,12 @@ func (m *MinioStorage) Delete(path string) error { } // URL gets the redirect URL to a file. The presigned link is valid for 5 minutes. -func (m *MinioStorage) URL(path, name string) (*url.URL, error) { - reqParams := make(url.Values) +func (m *MinioStorage) URL(path, name string, serveDirectReqParams url.Values) (*url.URL, error) { + // copy serveDirectReqParams + reqParams, err := url.ParseQuery(serveDirectReqParams.Encode()) + if err != nil { + return nil, err + } // TODO it may be good to embed images with 'inline' like ServeData does, but we don't want to have to read the file, do we? reqParams.Set("response-content-disposition", "attachment; filename=\""+quoteEscaper.Replace(name)+"\"") u, err := m.client.PresignedGetObject(m.ctx, m.bucket, m.buildMinioPath(path), 5*time.Minute, reqParams) diff --git a/modules/storage/storage.go b/modules/storage/storage.go index b83b1c7929..9cc6949256 100644 --- a/modules/storage/storage.go +++ b/modules/storage/storage.go @@ -63,7 +63,7 @@ type ObjectStorage interface { Save(path string, r io.Reader, size int64) (int64, error) Stat(path string) (os.FileInfo, error) Delete(path string) error - URL(path, name string) (*url.URL, error) + URL(path, name string, reqParams url.Values) (*url.URL, error) IterateObjects(path string, iterator func(path string, obj Object) error) error } diff --git a/routers/api/actions/artifacts.go b/routers/api/actions/artifacts.go index bc29e4481d..405686a058 100644 --- a/routers/api/actions/artifacts.go +++ b/routers/api/actions/artifacts.go @@ -437,7 +437,7 @@ func (ar artifactRoutes) getDownloadArtifactURL(ctx *ArtifactContext) { for _, artifact := range artifacts { var downloadURL string if setting.Actions.ArtifactStorage.MinioConfig.ServeDirect { - u, err := ar.fs.URL(artifact.StoragePath, artifact.ArtifactName) + u, err := ar.fs.URL(artifact.StoragePath, artifact.ArtifactName, nil) if err != nil && !errors.Is(err, storage.ErrURLNotSupported) { log.Error("Error getting serve direct url: %v", err) } diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 677e89da2f..0417f98242 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -530,7 +530,7 @@ func (r *artifactV4Routes) getSignedArtifactURL(ctx *ArtifactContext) { respData := GetSignedArtifactURLResponse{} if setting.Actions.ArtifactStorage.MinioConfig.ServeDirect { - u, err := storage.ActionsArtifacts.URL(artifact.StoragePath, artifact.ArtifactPath) + u, err := storage.ActionsArtifacts.URL(artifact.StoragePath, artifact.ArtifactPath, nil) if u != nil && err == nil { respData.SignedUrl = u.String() } diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index f376e7bc59..9c9da38424 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -689,7 +689,9 @@ func DeleteManifest(ctx *context.Context) { } func serveBlob(ctx *context.Context, pfd *packages_model.PackageFileDescriptor) { - s, u, _, err := packages_service.GetPackageBlobStream(ctx, pfd.File, pfd.Blob) + serveDirectReqParams := make(url.Values) + serveDirectReqParams.Set("response-content-type", pfd.Properties.GetByName(container_module.PropertyMediaType)) + s, u, _, err := packages_service.GetPackageBlobStream(ctx, pfd.File, pfd.Blob, serveDirectReqParams) if err != nil { apiError(ctx, http.StatusInternalServerError, err) return diff --git a/routers/api/packages/maven/maven.go b/routers/api/packages/maven/maven.go index 4181577454..521ef2209a 100644 --- a/routers/api/packages/maven/maven.go +++ b/routers/api/packages/maven/maven.go @@ -217,7 +217,7 @@ func servePackageFile(ctx *context.Context, params parameters, serveContent bool return } - s, u, _, err := packages_service.GetPackageBlobStream(ctx, pf, pb) + s, u, _, err := packages_service.GetPackageBlobStream(ctx, pf, pb, nil) if err != nil { apiError(ctx, http.StatusInternalServerError, err) return diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 1fa44d50c4..50d2786ec8 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -214,7 +214,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) { if setting.LFS.Storage.MinioConfig.ServeDirect { // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.LFS.URL(pointer.RelativePath(), blob.Name()) + u, err := storage.LFS.URL(pointer.RelativePath(), blob.Name(), nil) if u != nil && err == nil { ctx.Redirect(u.String()) return @@ -341,7 +341,7 @@ func download(ctx *context.APIContext, archiveName string, archiver *repo_model. rPath := archiver.RelativePath() if setting.RepoArchive.Storage.MinioConfig.ServeDirect { // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.RepoArchives.URL(rPath, downloadName) + u, err := storage.RepoArchives.URL(rPath, downloadName, nil) if u != nil && err == nil { ctx.Redirect(u.String()) return diff --git a/routers/web/base.go b/routers/web/base.go index 78dde57fa6..285d1ecddc 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -39,7 +39,7 @@ func storageHandler(storageSetting *setting.Storage, prefix string, objStore sto rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/") rPath = util.PathJoinRelX(rPath) - u, err := objStore.URL(rPath, path.Base(rPath)) + u, err := objStore.URL(rPath, path.Base(rPath), nil) if err != nil { if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) { log.Warn("Unable to find %s %s", prefix, rPath) diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index a343f60a98..e7dbb6d975 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -688,7 +688,8 @@ func ArtifactsDownloadView(ctx *context_module.Context) { if len(artifacts) == 1 && artifacts[0].ArtifactName+".zip" == artifacts[0].ArtifactPath && artifacts[0].ContentEncoding == "application/zip" { art := artifacts[0] if setting.Actions.ArtifactStorage.MinioConfig.ServeDirect { - u, err := storage.ActionsArtifacts.URL(art.StoragePath, art.ArtifactPath) + u, err := storage.ActionsArtifacts.URL(art.StoragePath, art.ArtifactPath, nil) + if u != nil && err == nil { ctx.Redirect(u.String()) return diff --git a/routers/web/repo/attachment.go b/routers/web/repo/attachment.go index b42effd8c3..b5078e1f63 100644 --- a/routers/web/repo/attachment.go +++ b/routers/web/repo/attachment.go @@ -134,7 +134,7 @@ func ServeAttachment(ctx *context.Context, uuid string) { if setting.Attachment.Storage.MinioConfig.ServeDirect { // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.Attachments.URL(attach.RelativePath(), attach.Name) + u, err := storage.Attachments.URL(attach.RelativePath(), attach.Name, nil) if u != nil && err == nil { ctx.Redirect(u.String()) diff --git a/routers/web/repo/download.go b/routers/web/repo/download.go index c4a8baecca..1e87bbf015 100644 --- a/routers/web/repo/download.go +++ b/routers/web/repo/download.go @@ -54,8 +54,8 @@ func ServeBlobOrLFS(ctx *context.Context, blob *git.Blob, lastModified *time.Tim } if setting.LFS.Storage.MinioConfig.ServeDirect { - // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.LFS.URL(pointer.RelativePath(), blob.Name()) + // If we have a signed url (S3, object storage, blob storage), redirect to this directly. + u, err := storage.LFS.URL(pointer.RelativePath(), blob.Name(), nil) if u != nil && err == nil { ctx.Redirect(u.String()) return nil diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 9562491440..8036bcae67 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -505,7 +505,7 @@ func download(ctx *context.Context, archiveName string, archiver *repo_model.Rep rPath := archiver.RelativePath() if setting.RepoArchive.Storage.MinioConfig.ServeDirect { // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.RepoArchives.URL(rPath, downloadName) + u, err := storage.RepoArchives.URL(rPath, downloadName, nil) if u != nil && err == nil { if archiver.ReleaseID != 0 { err = repo_model.CountArchiveDownload(ctx, ctx.Repo.Repository.ID, archiver.ReleaseID, archiver.Type) diff --git a/services/lfs/server.go b/services/lfs/server.go index 225dfdb024..51d6f42776 100644 --- a/services/lfs/server.go +++ b/services/lfs/server.go @@ -485,7 +485,7 @@ func buildObjectResponse(rc *requestContext, pointer lfs_module.Pointer, downloa var link *lfs_module.Link if setting.LFS.Storage.MinioConfig.ServeDirect { // If we have a signed url (S3, object storage), redirect to this directly. - u, err := storage.LFS.URL(pointer.RelativePath(), pointer.Oid) + u, err := storage.LFS.URL(pointer.RelativePath(), pointer.Oid, nil) if u != nil && err == nil { // Presigned url does not need the Authorization header // https://github.com/go-gitea/gitea/issues/21525 diff --git a/services/packages/packages.go b/services/packages/packages.go index a5b84506de..72ab19ee27 100644 --- a/services/packages/packages.go +++ b/services/packages/packages.go @@ -602,12 +602,12 @@ func GetPackageFileStream(ctx context.Context, pf *packages_model.PackageFile) ( return nil, nil, nil, err } - return GetPackageBlobStream(ctx, pf, pb) + return GetPackageBlobStream(ctx, pf, pb, nil) } // GetPackageBlobStream returns the content of the specific package blob // If the storage supports direct serving and it's enabled, only the direct serving url is returned. -func GetPackageBlobStream(ctx context.Context, pf *packages_model.PackageFile, pb *packages_model.PackageBlob) (io.ReadSeekCloser, *url.URL, *packages_model.PackageFile, error) { +func GetPackageBlobStream(ctx context.Context, pf *packages_model.PackageFile, pb *packages_model.PackageBlob, serveDirectReqParams url.Values) (io.ReadSeekCloser, *url.URL, *packages_model.PackageFile, error) { key := packages_module.BlobHash256Key(pb.HashSHA256) cs := packages_module.NewContentStore() @@ -617,7 +617,7 @@ func GetPackageBlobStream(ctx context.Context, pf *packages_model.PackageFile, p var err error if cs.ShouldServeDirect() { - u, err = cs.GetServeDirectURL(key, pf.Name) + u, err = cs.GetServeDirectURL(key, pf.Name, serveDirectReqParams) if err != nil && !errors.Is(err, storage.ErrURLNotSupported) { log.Error("Error getting serve direct url: %v", err) }