Repository: distribution/distribution (go)

Issue:
‘autoredirect‘ force use 'https'
I'm using nginx to proxy `distribution` and `auth server` in kubernetes, both http and https configured. If i enable `autoredirect`, it always return https location with proxy host, then http port can not be used.


https://github.com/docker/distribution/blob/980d1a696b5c5bed25835f25d06ec2ef3ddc24b0/registry/auth/token/accesscontroller.go#L104

Can we make it smarter? or i disable `autoredirect` to use `realm` option.

Pull request description:
Fix #2902: ‘autoRedirect’ hardcode ‘https’ scheme

Patch:
diff --git a/docs/content/about/configuration.md b/docs/content/about/configuration.md
index 482a40ca..75fb79c5 100644
--- a/docs/content/about/configuration.md
+++ b/docs/content/about/configuration.md
@@ -591,7 +591,8 @@ security.
 | `service` | yes      | The service being authenticated.                      |
 | `issuer`  | yes      | The name of the token issuer. The issuer inserts this into the token so it must match the value configured for the issuer. |
 | `rootcertbundle` | yes | The absolute path to the root certificate bundle. This bundle contains the public part of the certificates used to sign authentication tokens. |
-| `autoredirect`   | no      | When set to `true`, `realm` will automatically be set using the Host header of the request as the domain and a path of `/auth/token/`|
+| `autoredirect`   | no      | When set to `true`, `realm` will automatically be set using the Host header of the request as the domain and a path of `/auth/token/`(or specified by `autoredirectpath`), the `realm` URL Scheme will use `X-Forwarded-Proto` header if set, otherwise it will be set to `https`. |
+| `autoredirectpath`   | no      | The path to redirect to if `autoredirect` is set to `true`, default: `/auth/token/`. |
 
 
 For more information about Token based authentication configuration, see the
diff --git a/registry/auth/token/accesscontroller.go b/registry/auth/token/accesscontroller.go
index 8f610c46..4d54248b 100644
--- a/registry/auth/token/accesscontroller.go
+++ b/registry/auth/token/accesscontroller.go
@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"os"
 	"strings"
 
@@ -83,11 +84,12 @@ var (
 
 // authChallenge implements the auth.Challenge interface.
 type authChallenge struct {
-	err          error
-	realm        string
-	autoRedirect bool
-	service      string
-	accessSet    accessSet
+	err              error
+	realm            string
+	autoRedirect     bool
+	autoRedirectPath string
+	service          string
+	accessSet        accessSet
 }
 
 var _ auth.Challenge = authChallenge{}
@@ -102,13 +104,28 @@ func (ac authChallenge) Status() int {
 	return http.StatusUnauthorized
 }
 
+func buildAutoRedirectURL(r *http.Request, autoRedirectPath string) string {
+	scheme := "https"
+
+	if forwardedProto := r.Header.Get("X-Forwarded-Proto"); len(forwardedProto) > 0 {
+		scheme = forwardedProto
+	}
+
+	u := &url.URL{
+		Scheme: scheme,
+		Host:   r.Host,
+		Path:   autoRedirectPath,
+	}
+	return u.String()
+}
+
 // challengeParams constructs the value to be used in
 // the WWW-Authenticate response challenge header.
 // See https://tools.ietf.org/html/rfc6750#section-3
 func (ac authChallenge) challengeParams(r *http.Request) string {
 	var realm string
 	if ac.autoRedirect {
-		realm = fmt.Sprintf("https://%s/auth/token", r.Host)
+		realm = buildAutoRedirectURL(r, ac.autoRedirectPath)
 	} else {
 		realm = ac.realm
 	}
@@ -134,23 +151,29 @@ func (ac authChallenge) SetHeaders(r *http.Request, w http.ResponseWriter) {
 
 // accessController implements the auth.AccessController interface.
 type accessController struct {
-	realm        string
-	autoRedirect bool
-	issuer       string
-	service      string
-	rootCerts    *x509.CertPool
-	trustedKeys  map[string]crypto.PublicKey
+	realm            string
+	autoRedirect     bool
+	autoRedirectPath string
+	issuer           string
+	service          string
+	rootCerts        *x509.CertPool
+	trustedKeys      map[string]crypto.PublicKey
 }
 
+const (
+	defaultAutoRedirectPath = "/auth/token"
+)
+
 // tokenAccessOptions is a convenience type for handling
 // options to the contstructor of an accessController.
 type tokenAccessOptions struct {
-	realm          string
-	autoRedirect   bool
-	issuer         string
-	service        string
-	rootCertBundle string
-	jwks           string
+	realm            string
+	autoRedirect     bool
+	autoRedirectPath string
+	issuer           string
+	service          string
+	rootCertBundle   string
+	jwks             string
 }
 
 // checkOptions gathers the necessary options
@@ -187,6 +210,19 @@ func checkOptions(options map[string]interface{}) (tokenAccessOptions, error) {
 		}
 		opts.autoRedirect = autoRedirect
 	}
+	if opts.autoRedirect {
+		autoRedirectPathVal, ok := options["autoredirectpath"]
+		if ok {
+			autoRedirectPath, ok := autoRedirectPathVal.(string)
+			if !ok {
+				return opts, fmt.Errorf("token auth requires a valid option string: autoredirectpath")
+			}
+			opts.autoRedirectPath = autoRedirectPath
+		}
+		if opts.autoRedirectPath == "" {
+			opts.autoRedirectPath = defaultAutoRedirectPath
+		}
+	}
 
 	return opts, nil
 }
@@ -287,12 +323,13 @@ func newAccessController(options map[string]interface{}) (auth.AccessController,
 	}
 
 	return &accessController{
-		realm:        config.realm,
-		autoRedirect: config.autoRedirect,
-		issuer:       config.issuer,
-		service:      config.service,
-		rootCerts:    rootPool,
-		trustedKeys:  trustedKeys,
+		realm:            config.realm,
+		autoRedirect:     config.autoRedirect,
+		autoRedirectPath: config.autoRedirectPath,
+		issuer:           config.issuer,
+		service:          config.service,
+		rootCerts:        rootPool,
+		trustedKeys:      trustedKeys,
 	}, nil
 }
 
@@ -300,10 +337,11 @@ func newAccessController(options map[string]interface{}) (auth.AccessController,
 // for actions on resources described by the given access items.
 func (ac *accessController) Authorized(req *http.Request, accessItems ...auth.Access) (*auth.Grant, error) {
 	challenge := &authChallenge{
-		realm:        ac.realm,
-		autoRedirect: ac.autoRedirect,
-		service:      ac.service,
-		accessSet:    newAccessSet(accessItems...),
+		realm:            ac.realm,
+		autoRedirect:     ac.autoRedirect,
+		autoRedirectPath: ac.autoRedirectPath,
+		service:          ac.service,
+		accessSet:        newAccessSet(accessItems...),
 	}
 
 	prefix, rawToken, ok := strings.Cut(req.Header.Get("Authorization"), " ")


Test patch:
diff --git a/registry/auth/token/accesscontroller_test.go b/registry/auth/token/accesscontroller_test.go
new file mode 100644
index 00000000..fde20311
--- /dev/null
+++ b/registry/auth/token/accesscontroller_test.go
@@ -0,0 +1,89 @@
+package token
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestBuildAutoRedirectURL(t *testing.T) {
+	cases := []struct {
+		name             string
+		reqGetter        func() *http.Request
+		autoRedirectPath string
+		expectedURL      string
+	}{{
+		name: "http",
+		reqGetter: func() *http.Request {
+			req := httptest.NewRequest("GET", "http://example.com/", nil)
+			return req
+		},
+		autoRedirectPath: "/auth",
+		expectedURL:      "https://example.com/auth",
+	}, {
+		name: "x-forwarded",
+		reqGetter: func() *http.Request {
+			req := httptest.NewRequest("GET", "http://example.com/", nil)
+			req.Header.Set("X-Forwarded-Proto", "http")
+			return req
+		},
+		autoRedirectPath: "/auth/token",
+		expectedURL:      "http://example.com/auth/token",
+	}}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			req := tc.reqGetter()
+			result := buildAutoRedirectURL(req, tc.autoRedirectPath)
+			if result != tc.expectedURL {
+				t.Errorf("expected %s, got %s", tc.expectedURL, result)
+			}
+		})
+	}
+}
+
+func TestCheckOptions(t *testing.T) {
+	realm := "https://auth.example.com/token/"
+	issuer := "test-issuer.example.com"
+	service := "test-service.example.com"
+
+	options := map[string]interface{}{
+		"realm":            realm,
+		"issuer":           issuer,
+		"service":          service,
+		"rootcertbundle":   "",
+		"autoredirect":     true,
+		"autoredirectpath": "/auth",
+	}
+
+	ta, err := checkOptions(options)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if ta.autoRedirect != true {
+		t.Fatal("autoredirect should be true")
+	}
+	if ta.autoRedirectPath != "/auth" {
+		t.Fatal("autoredirectpath should be /auth")
+	}
+
+	options = map[string]interface{}{
+		"realm":                        realm,
+		"issuer":                       issuer,
+		"service":                      service,
+		"rootcertbundle":               "",
+		"autoredirect":                 true,
+		"autoredirectforcetlsdisabled": true,
+	}
+
+	ta, err = checkOptions(options)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if ta.autoRedirect != true {
+		t.Fatal("autoredirect should be true")
+	}
+	if ta.autoRedirectPath != "/auth/token" {
+		t.Fatal("autoredirectpath should be /auth/token")
+	}
+}