From 6862173655142c231b0f8da08119dc794ce9ba14 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Sat, 14 Jan 2023 17:21:33 +0000 Subject: [PATCH 1/3] Check quota limits for container uploads. --- routers/api/packages/container/blob.go | 16 +++++++----- routers/api/packages/container/container.go | 29 ++++++++++++++++++--- routers/api/packages/container/manifest.go | 4 +++ services/packages/packages.go | 12 ++++++--- 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/routers/api/packages/container/blob.go b/routers/api/packages/container/blob.go index fd5819d506f3b..457ca564d0ec4 100644 --- a/routers/api/packages/container/blob.go +++ b/routers/api/packages/container/blob.go @@ -26,7 +26,11 @@ var uploadVersionMutex sync.Mutex // saveAsPackageBlob creates a package blob from an upload // The uploaded blob gets stored in a special upload version to link them to the package/image -func saveAsPackageBlob(hsr packages_module.HashedSizeReader, pi *packages_service.PackageInfo) (*packages_model.PackageBlob, error) { +func saveAsPackageBlob(hsr packages_module.HashedSizeReader, pci *packages_service.PackageCreationInfo) (*packages_model.PackageBlob, error) { + if err := packages_service.CheckSizeQuotaExceeded(db.DefaultContext, pci.Creator, pci.Owner, packages_model.TypeContainer, hsr.Size()); err != nil { + return nil, err + } + pb := packages_service.NewPackageBlob(hsr) exists := false @@ -41,10 +45,10 @@ func saveAsPackageBlob(hsr packages_module.HashedSizeReader, pi *packages_servic err := db.WithTx(db.DefaultContext, func(ctx context.Context) error { created := true p := &packages_model.Package{ - OwnerID: pi.Owner.ID, + OwnerID: pci.Owner.ID, Type: packages_model.TypeContainer, - Name: strings.ToLower(pi.Name), - LowerName: strings.ToLower(pi.Name), + Name: strings.ToLower(pci.Name), + LowerName: strings.ToLower(pci.Name), } var err error if p, err = packages_model.TryInsertPackage(ctx, p); err != nil { @@ -57,7 +61,7 @@ func saveAsPackageBlob(hsr packages_module.HashedSizeReader, pi *packages_servic } if created { - if _, err := packages_model.InsertProperty(ctx, packages_model.PropertyTypePackage, p.ID, container_module.PropertyRepository, strings.ToLower(pi.Owner.LowerName+"/"+pi.Name)); err != nil { + if _, err := packages_model.InsertProperty(ctx, packages_model.PropertyTypePackage, p.ID, container_module.PropertyRepository, strings.ToLower(pci.Owner.LowerName+"/"+pci.Name)); err != nil { log.Error("Error setting package property: %v", err) return err } @@ -65,7 +69,7 @@ func saveAsPackageBlob(hsr packages_module.HashedSizeReader, pi *packages_servic pv := &packages_model.PackageVersion{ PackageID: p.ID, - CreatorID: pi.Owner.ID, + CreatorID: pci.Creator.ID, Version: container_model.UploadVersion, LowerVersion: container_model.UploadVersion, IsInternal: true, diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index cbbdf8eb880f5..01e3080cee7d3 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -222,7 +222,16 @@ func InitiateUploadBlob(ctx *context.Context) { return } - if _, err := saveAsPackageBlob(buf, &packages_service.PackageInfo{Owner: ctx.Package.Owner, Name: image}); err != nil { + if _, err := saveAsPackageBlob( + buf, + &packages_service.PackageCreationInfo{ + PackageInfo: packages_service.PackageInfo{ + Owner: ctx.Package.Owner, + Name: image, + }, + Creator: ctx.Doer, + }, + ); err != nil { apiError(ctx, http.StatusInternalServerError, err) return } @@ -353,7 +362,16 @@ func EndUploadBlob(ctx *context.Context) { return } - if _, err := saveAsPackageBlob(uploader, &packages_service.PackageInfo{Owner: ctx.Package.Owner, Name: image}); err != nil { + if _, err := saveAsPackageBlob( + uploader, + &packages_service.PackageCreationInfo{ + PackageInfo: packages_service.PackageInfo{ + Owner: ctx.Package.Owner, + Name: image, + }, + Creator: ctx.Doer, + }, + ); err != nil { apiError(ctx, http.StatusInternalServerError, err) return } @@ -521,7 +539,12 @@ func UploadManifest(ctx *context.Context) { } else if errors.Is(err, container_model.ErrContainerBlobNotExist) { apiErrorDefined(ctx, errBlobUnknown) } else { - apiError(ctx, http.StatusInternalServerError, err) + switch err { + case packages_service.ErrQuotaTotalCount, packages_service.ErrQuotaTypeSize, packages_service.ErrQuotaTotalSize: + apiError(ctx, http.StatusForbidden, err) + default: + apiError(ctx, http.StatusInternalServerError, err) + } } return } diff --git a/routers/api/packages/container/manifest.go b/routers/api/packages/container/manifest.go index 350933f3d2f87..491fb70639435 100644 --- a/routers/api/packages/container/manifest.go +++ b/routers/api/packages/container/manifest.go @@ -327,6 +327,10 @@ func createPackageAndVersion(ctx context.Context, mci *manifestCreationInfo, met } } + if err := packages_service.CheckCountQuotaExceeded(ctx, mci.Creator, mci.Owner); err != nil { + return nil, err + } + if mci.IsTagged { if _, err := packages_model.InsertProperty(ctx, packages_model.PropertyTypeVersion, pv.ID, container_module.PropertyManifestTagged, ""); err != nil { log.Error("Error setting package version property: %v", err) diff --git a/services/packages/packages.go b/services/packages/packages.go index 49f5a2fac4e21..c41711d6b33c0 100644 --- a/services/packages/packages.go +++ b/services/packages/packages.go @@ -173,7 +173,7 @@ func createPackageAndVersion(ctx context.Context, pvci *PackageCreationInfo, all } if versionCreated { - if err := checkCountQuotaExceeded(ctx, pvci.Creator, pvci.Owner); err != nil { + if err := CheckCountQuotaExceeded(ctx, pvci.Creator, pvci.Owner); err != nil { return nil, false, err } @@ -240,7 +240,7 @@ func NewPackageBlob(hsr packages_module.HashedSizeReader) *packages_model.Packag func addFileToPackageVersion(ctx context.Context, pv *packages_model.PackageVersion, pvi *PackageInfo, pfci *PackageFileCreationInfo) (*packages_model.PackageFile, *packages_model.PackageBlob, bool, error) { log.Trace("Adding package file: %v, %s", pv.ID, pfci.Filename) - if err := checkSizeQuotaExceeded(ctx, pfci.Creator, pvi.Owner, pvi.PackageType, pfci.Data.Size()); err != nil { + if err := CheckSizeQuotaExceeded(ctx, pfci.Creator, pvi.Owner, pvi.PackageType, pfci.Data.Size()); err != nil { return nil, nil, false, err } @@ -302,7 +302,9 @@ func addFileToPackageVersion(ctx context.Context, pv *packages_model.PackageVers return pf, pb, !exists, nil } -func checkCountQuotaExceeded(ctx context.Context, doer, owner *user_model.User) error { +// CheckCountQuotaExceeded checks if the owner has more than the allowed packages +// The check is skipped if the doer is an admin. +func CheckCountQuotaExceeded(ctx context.Context, doer, owner *user_model.User) error { if doer.IsAdmin { return nil } @@ -324,7 +326,9 @@ func checkCountQuotaExceeded(ctx context.Context, doer, owner *user_model.User) return nil } -func checkSizeQuotaExceeded(ctx context.Context, doer, owner *user_model.User, packageType packages_model.Type, uploadSize int64) error { +// CheckSizeQuotaExceeded checks if the upload size is bigger than the allowed size +// The check is skipped if the doer is an admin. +func CheckSizeQuotaExceeded(ctx context.Context, doer, owner *user_model.User, packageType packages_model.Type, uploadSize int64) error { if doer.IsAdmin { return nil } From 9000f46c0d1ba78e3f6ccf77d4e21fe1dec60537 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Sun, 15 Jan 2023 15:37:19 +0000 Subject: [PATCH 2/3] Add tests. --- routers/api/packages/container/container.go | 14 ++++- tests/integration/api_packages_test.go | 70 +++++++++++++++------ 2 files changed, 62 insertions(+), 22 deletions(-) diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index 01e3080cee7d3..4fe20101af1c1 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -232,7 +232,12 @@ func InitiateUploadBlob(ctx *context.Context) { Creator: ctx.Doer, }, ); err != nil { - apiError(ctx, http.StatusInternalServerError, err) + switch err { + case packages_service.ErrQuotaTotalCount, packages_service.ErrQuotaTypeSize, packages_service.ErrQuotaTotalSize: + apiError(ctx, http.StatusForbidden, err) + default: + apiError(ctx, http.StatusInternalServerError, err) + } return } @@ -372,7 +377,12 @@ func EndUploadBlob(ctx *context.Context) { Creator: ctx.Doer, }, ); err != nil { - apiError(ctx, http.StatusInternalServerError, err) + switch err { + case packages_service.ErrQuotaTotalCount, packages_service.ErrQuotaTypeSize, packages_service.ErrQuotaTotalSize: + apiError(ctx, http.StatusForbidden, err) + default: + apiError(ctx, http.StatusInternalServerError, err) + } return } diff --git a/tests/integration/api_packages_test.go b/tests/integration/api_packages_test.go index 8346e3bcccaa6..158edc07ae78c 100644 --- a/tests/integration/api_packages_test.go +++ b/tests/integration/api_packages_test.go @@ -5,8 +5,10 @@ package integration import ( "bytes" + "crypto/sha256" "fmt" "net/http" + "strings" "testing" "time" @@ -169,34 +171,62 @@ func TestPackageAccess(t *testing.T) { func TestPackageQuota(t *testing.T) { defer tests.PrepareTestEnv(t)() - limitTotalOwnerCount, limitTotalOwnerSize, limitSizeGeneric := setting.Packages.LimitTotalOwnerCount, setting.Packages.LimitTotalOwnerSize, setting.Packages.LimitSizeGeneric + limitTotalOwnerCount, limitTotalOwnerSize := setting.Packages.LimitTotalOwnerCount, setting.Packages.LimitTotalOwnerSize + // Exceeded quota result in StatusForbidden for normal users but admins are always allowed to upload. admin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 10}) - uploadPackage := func(doer *user_model.User, version string, expectedStatus int) { - url := fmt.Sprintf("/api/packages/%s/generic/test-package/%s/file.bin", user.Name, version) - req := NewRequestWithBody(t, "PUT", url, bytes.NewReader([]byte{1})) - AddBasicAuthHeader(req, doer.Name) - MakeRequest(t, req, expectedStatus) - } + t.Run("Common", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() - // Exceeded quota result in StatusForbidden for normal users but admins are always allowed to upload. + limitSizeGeneric := setting.Packages.LimitSizeGeneric + + uploadPackage := func(doer *user_model.User, version string, expectedStatus int) { + url := fmt.Sprintf("/api/packages/%s/generic/test-package/%s/file.bin", user.Name, version) + req := NewRequestWithBody(t, "PUT", url, bytes.NewReader([]byte{1})) + AddBasicAuthHeader(req, doer.Name) + MakeRequest(t, req, expectedStatus) + } - setting.Packages.LimitTotalOwnerCount = 0 - uploadPackage(user, "1.0", http.StatusForbidden) - uploadPackage(admin, "1.0", http.StatusCreated) - setting.Packages.LimitTotalOwnerCount = limitTotalOwnerCount + setting.Packages.LimitTotalOwnerCount = 0 + uploadPackage(user, "1.0", http.StatusForbidden) + uploadPackage(admin, "1.0", http.StatusCreated) + setting.Packages.LimitTotalOwnerCount = limitTotalOwnerCount - setting.Packages.LimitTotalOwnerSize = 0 - uploadPackage(user, "1.1", http.StatusForbidden) - uploadPackage(admin, "1.1", http.StatusCreated) - setting.Packages.LimitTotalOwnerSize = limitTotalOwnerSize + setting.Packages.LimitTotalOwnerSize = 0 + uploadPackage(user, "1.1", http.StatusForbidden) + uploadPackage(admin, "1.1", http.StatusCreated) + setting.Packages.LimitTotalOwnerSize = limitTotalOwnerSize - setting.Packages.LimitSizeGeneric = 0 - uploadPackage(user, "1.2", http.StatusForbidden) - uploadPackage(admin, "1.2", http.StatusCreated) - setting.Packages.LimitSizeGeneric = limitSizeGeneric + setting.Packages.LimitSizeGeneric = 0 + uploadPackage(user, "1.2", http.StatusForbidden) + uploadPackage(admin, "1.2", http.StatusCreated) + setting.Packages.LimitSizeGeneric = limitSizeGeneric + }) + + t.Run("Container", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + limitSizeContainer := setting.Packages.LimitSizeContainer + + uploadBlob := func(doer *user_model.User, data string, expectedStatus int) { + url := fmt.Sprintf("/v2/%s/quota-test/blobs/uploads?digest=sha256:%x", user.Name, sha256.Sum256([]byte(data))) + req := NewRequestWithBody(t, "POST", url, strings.NewReader(data)) + AddBasicAuthHeader(req, doer.Name) + MakeRequest(t, req, expectedStatus) + } + + setting.Packages.LimitTotalOwnerSize = 0 + uploadBlob(user, "2", http.StatusForbidden) + uploadBlob(admin, "2", http.StatusCreated) + setting.Packages.LimitTotalOwnerSize = limitTotalOwnerSize + + setting.Packages.LimitSizeContainer = 0 + uploadBlob(user, "3", http.StatusForbidden) + uploadBlob(admin, "3", http.StatusCreated) + setting.Packages.LimitSizeContainer = limitSizeContainer + }) } func TestPackageCleanup(t *testing.T) { From 31869ab75a23abe0e1d7f24f029d6cda9bb952c4 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Sun, 29 Jan 2023 16:23:59 +0000 Subject: [PATCH 3/3] Fix lint. --- routers/api/packages/container/blob.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/routers/api/packages/container/blob.go b/routers/api/packages/container/blob.go index 9501d1e8f71a2..f0457c55e19c3 100644 --- a/routers/api/packages/container/blob.go +++ b/routers/api/packages/container/blob.go @@ -37,7 +37,7 @@ func saveAsPackageBlob(hsr packages_module.HashedSizeReader, pci *packages_servi contentStore := packages_module.NewContentStore() - uploadVersion, err := getOrCreateUploadVersion(pi) + uploadVersion, err := getOrCreateUploadVersion(&pci.PackageInfo) if err != nil { return nil, err } @@ -99,10 +99,10 @@ func getOrCreateUploadVersion(pi *packages_service.PackageInfo) (*packages_model err := db.WithTx(db.DefaultContext, func(ctx context.Context) error { created := true p := &packages_model.Package{ - OwnerID: pci.Owner.ID, + OwnerID: pi.Owner.ID, Type: packages_model.TypeContainer, - Name: strings.ToLower(pci.Name), - LowerName: strings.ToLower(pci.Name), + Name: strings.ToLower(pi.Name), + LowerName: strings.ToLower(pi.Name), } var err error if p, err = packages_model.TryInsertPackage(ctx, p); err != nil { @@ -115,7 +115,7 @@ func getOrCreateUploadVersion(pi *packages_service.PackageInfo) (*packages_model } if created { - if _, err := packages_model.InsertProperty(ctx, packages_model.PropertyTypePackage, p.ID, container_module.PropertyRepository, strings.ToLower(pci.Owner.LowerName+"/"+pci.Name)); err != nil { + if _, err := packages_model.InsertProperty(ctx, packages_model.PropertyTypePackage, p.ID, container_module.PropertyRepository, strings.ToLower(pi.Owner.LowerName+"/"+pi.Name)); err != nil { log.Error("Error setting package property: %v", err) return err } @@ -123,7 +123,7 @@ func getOrCreateUploadVersion(pi *packages_service.PackageInfo) (*packages_model pv := &packages_model.PackageVersion{ PackageID: p.ID, - CreatorID: pci.Creator.ID, + CreatorID: pi.Owner.ID, Version: container_model.UploadVersion, LowerVersion: container_model.UploadVersion, IsInternal: true,