From a72cffddfaa03f429736b5bd47f986c06721ed09 Mon Sep 17 00:00:00 2001 From: Andreas Ahlenstorf Date: Sun, 8 Mar 2026 02:56:44 +0100 Subject: [PATCH] [v14.0/forgejo] fix: extend basic auth to /v2, always include WWW-Authenticate header (#11393) (#11557) Forgejo's OCI container registry did not enable basic authentication for the top-level endpoint `/v2`. Furthermore, it did not include the `WWW-Authenticate` header when returning the status code 401 as mandated by [RFC 7235](https://datatracker.ietf.org/doc/html/rfc7235#section-3.1), "Hypertext Transfer Protocol (HTTP/1.1): Authentication", section 3.1. Those deficiencies made it impossible for Apple's [container](https://github.com/apple/container) to log into Forgejo OCI container registry. This has been rectified. The problem did not occur with most other tools because they do not include credentials when sending the initial request to `/v2`. Forgejo's reply then included `WWW-Authenticate` as expected. Enabling basic authentication for `/v2` has the side effect that Apple's container uses username and password for all successive requests and not the bearer token. If that is a problem, it's up to Apple to change container's behaviour. If invalid credentials are passed to `container registry login`, then container enters an infinite loop. The same happens with quay.io, but not ghcr.io (returns 403) or docker.io (returns 401 but _without_ `WWW-Authenticate`). As this is invalid behaviour on container's side, it's up to Apple to change container. Docker and Podman handle it correctly. Login and pushing have been tested manually with Docker 29.1.3, Podman 5.7.1, and Apple's container 0.9.0. Resolves https://codeberg.org/forgejo/forgejo/issues/11297. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11557 Reviewed-by: Mathieu Fenniak (cherry picked from commit 3e849b4b5021136f927f7a616112b3cf6e790b30) Co-authored-by: Andreas Ahlenstorf Co-committed-by: Andreas Ahlenstorf --- routers/api/packages/api.go | 1 + routers/api/packages/container/container.go | 9 +-- services/auth/auth.go | 4 +- services/auth/auth_test.go | 64 +++++++++++++++++++ ..._packages_container_cleanup_sha256_test.go | 3 +- .../api_packages_container_test.go | 30 ++++++++- 6 files changed, 104 insertions(+), 7 deletions(-) diff --git a/routers/api/packages/api.go b/routers/api/packages/api.go index 47e582eb92..3b51ddc406 100644 --- a/routers/api/packages/api.go +++ b/routers/api/packages/api.go @@ -118,6 +118,7 @@ func verifyAuth(r *web.Route, authMethods []auth.Method) { ctx.Doer, err = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session) if err != nil { log.Info("Failed to verify user: %v", err) + container.APIUnauthorizedError(ctx) ctx.Error(http.StatusUnauthorized, "authGroup.Verify") return } diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index 873ce5c23a..373847b3ab 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -125,15 +125,16 @@ func apiErrorDefined(ctx *context.Context, err *namedError) { }) } -func apiUnauthorizedError(ctx *context.Context) { - ctx.Resp.Header().Add("WWW-Authenticate", `Bearer realm="`+setting.AppURL+`v2/token",service="container_registry",scope="*"`) +func APIUnauthorizedError(ctx *context.Context) { + ctx.Resp.Header().Set("WWW-Authenticate", `Bearer realm="`+setting.AppURL+`v2/token",service="container_registry",scope="*",`+ + `Basic realm="`+setting.AppURL+`v2",service="container_registry",scope="*"`) apiErrorDefined(ctx, errUnauthorized) } // ReqContainerAccess is a middleware which checks the current user valid (real user or ghost if anonymous access is enabled) func ReqContainerAccess(ctx *context.Context) { if ctx.Doer == nil || (setting.Service.RequireSignInView && ctx.Doer.IsGhost()) { - apiUnauthorizedError(ctx) + APIUnauthorizedError(ctx) } } @@ -158,7 +159,7 @@ func Authenticate(ctx *context.Context) { u := ctx.Doer if u == nil { if setting.Service.RequireSignInView { - apiUnauthorizedError(ctx) + APIUnauthorizedError(ctx) return } diff --git a/services/auth/auth.go b/services/auth/auth.go index 52f1f65cce..14d78cdbc0 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -33,7 +33,9 @@ func isAttachmentDownload(req *http.Request) bool { // isContainerPath checks if the request targets the container endpoint func isContainerPath(req *http.Request) bool { - return strings.HasPrefix(req.URL.Path, "/v2/") + // Go's URL omits trailing slashes from `Path`. That means that `/v2/`, the top-level endpoint, appears as `/v2`. + // strings.HasPrefix(req.URL.Path, "/v2") would be inappropriate because it would match paths like `/v2-abcd`, too. + return req.URL.Path == "/v2" || strings.HasPrefix(req.URL.Path, "/v2/") } var ( diff --git a/services/auth/auth_test.go b/services/auth/auth_test.go index a6c6c74022..82308402d2 100644 --- a/services/auth/auth_test.go +++ b/services/auth/auth_test.go @@ -6,9 +6,13 @@ package auth import ( "net/http" + "net/url" "testing" "forgejo.org/modules/setting" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_isGitRawOrLFSPath(t *testing.T) { @@ -132,3 +136,63 @@ func Test_isGitRawOrLFSPath(t *testing.T) { } setting.LFS.StartServer = origLFSStartServer } + +func TestAuth_isContainerPath(t *testing.T) { + testCases := []struct { + name string + input string + isContainerPath bool + }{ + { + name: "without trailing slash", + input: "https://example.com/v2", + isContainerPath: true, + }, + { + name: "with trailing slash", + input: "https://example.com/v2/", + isContainerPath: true, + }, + { + name: "with additional path components", + input: "https://example.com/v2/example/blobs/uploads/", + isContainerPath: true, + }, + { + name: "without v2", + input: "https://example.com/", + isContainerPath: false, + }, + { + name: "v2 not at the beginning", + input: "https://example.com/something/v2/", + isContainerPath: false, + }, + { + name: "v2 with prefix", + input: "https://example.com/abcd-v2/", + isContainerPath: false, + }, + { + name: "v2 with suffix", + input: "https://example.com/v2-abcd/", + isContainerPath: false, + }, + { + name: "v1", + input: "https://example.com/v1/", + isContainerPath: false, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + inputURL, err := url.Parse(testCase.input) + require.NoError(t, err) + + request := http.Request{URL: inputURL} + + assert.Equal(t, testCase.isContainerPath, isContainerPath(&request)) + }) + } +} diff --git a/tests/integration/api_packages_container_cleanup_sha256_test.go b/tests/integration/api_packages_container_cleanup_sha256_test.go index 19b73f7698..d672504509 100644 --- a/tests/integration/api_packages_container_cleanup_sha256_test.go +++ b/tests/integration/api_packages_container_cleanup_sha256_test.go @@ -63,7 +63,8 @@ func TestPackageContainerCleanupSHA256(t *testing.T) { Token string `json:"token"` } - authenticate := []string{`Bearer realm="` + setting.AppURL + `v2/token",service="container_registry",scope="*"`} + authenticate := []string{`Bearer realm="` + setting.AppURL + `v2/token",service="container_registry",scope="*",` + + `Basic realm="` + setting.AppURL + `v2",service="container_registry",scope="*"`} t.Run("User", func(t *testing.T) { req := NewRequest(t, "GET", fmt.Sprintf("%sv2", setting.AppURL)) diff --git a/tests/integration/api_packages_container_test.go b/tests/integration/api_packages_container_test.go index b961415feb..9143ff973b 100644 --- a/tests/integration/api_packages_container_test.go +++ b/tests/integration/api_packages_container_test.go @@ -86,7 +86,8 @@ func TestPackageContainer(t *testing.T) { Token string `json:"token"` } - authenticate := []string{`Bearer realm="` + setting.AppURL + `v2/token",service="container_registry",scope="*"`} + authenticate := []string{`Bearer realm="` + setting.AppURL + `v2/token",service="container_registry",scope="*",` + + `Basic realm="` + setting.AppURL + `v2",service="container_registry",scope="*"`} t.Run("Anonymous", func(t *testing.T) { defer tests.PrintCurrentTest(t)() @@ -166,6 +167,33 @@ func TestPackageContainer(t *testing.T) { MakeRequest(t, req, http.StatusOK) }) }) + + t.Run("No token issued if credentials are invalid", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", fmt.Sprintf("%sv2/token", setting.AppURL)) + // Setting the header explicitly instead of using AddBasicAuth to supply an invalid password. + req.Request.Header.Set("Authorization", "Basic "+base64.StdEncoding.EncodeToString([]byte("user2:very-invalid"))) + resp := MakeRequest(t, req, http.StatusUnauthorized) + + assert.Equal(t, authenticate, resp.Header().Values("WWW-Authenticate")) + }) + + t.Run("Basic authentication", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", fmt.Sprintf("%sv2", setting.AppURL)) + req.AddBasicAuth("does-not-exist") + resp := MakeRequest(t, req, http.StatusUnauthorized) + + assert.Equal(t, authenticate, resp.Header().Values("WWW-Authenticate")) + + req = NewRequest(t, "GET", fmt.Sprintf("%sv2", setting.AppURL)) + req.AddBasicAuth(user.Name) + resp = MakeRequest(t, req, http.StatusOK) + + assert.Empty(t, resp.Header().Get("WWW-Authenticate")) + }) }) t.Run("DetermineSupport", func(t *testing.T) {