Merge pull request #10136 from reasonerjt/rm-authproxy-case-sensitive

Get rid of case-sensitivity in authproxy setting

src/common/config/metadata/metadatalist.go

@@ -139,7 +139,6 @@ var (
 		{Name: common.HTTPAuthProxyVerifyCert, Scope: UserScope, Group: HTTPAuthGroup, DefaultValue: "true", ItemType: &BoolType{}},
 		{Name: common.HTTPAuthProxySkipSearch, Scope: UserScope, Group: HTTPAuthGroup, DefaultValue: "false", ItemType: &BoolType{}},
 		{Name: common.HTTPAuthProxyServerCertificate, Scope: UserScope, Group: HTTPAuthGroup, ItemType: &StringType{}},
-		{Name: common.HTTPAuthProxyCaseSensitive, Scope: UserScope, Group: HTTPAuthGroup, DefaultValue: "true", ItemType: &BoolType{}},
 
 		{Name: common.OIDCName, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}},
 		{Name: common.OIDCEndpoint, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}},

src/common/const.go

@@ -104,7 +104,6 @@ const (
 	HTTPAuthProxyTokenReviewEndpoint = "http_authproxy_tokenreview_endpoint"
 	HTTPAuthProxyVerifyCert          = "http_authproxy_verify_cert"
 	HTTPAuthProxySkipSearch          = "http_authproxy_skip_search"
-	HTTPAuthProxyCaseSensitive       = "http_authproxy_case_sensitive"
 	HTTPAuthProxyServerCertificate   = "http_authproxy_server_certificate"
 	OIDCName                         = "oidc_name"
 	OIDCEndpoint                     = "oidc_endpoint"

src/common/models/config.go

@@ -74,7 +74,6 @@ type HTTPAuthProxy struct {
 	VerifyCert          bool   `json:"verify_cert"`
 	SkipSearch          bool   `json:"skip_search"`
 	ServerCertificate   string `json:"server_certificate"`
-	CaseSensitive       bool   `json:"case_sensitive"`
 }
 
 // OIDCSetting wraps the settings for OIDC auth endpoint

src/core/api/config.go

@@ -126,7 +126,7 @@ func (c *ConfigAPI) validateCfg(cfgs map[string]interface{}) (bool, error) {
 		return true, err
 	}
 	if !flag {
-		if failedKeys := checkUnmodifiable(c.cfgManager, cfgs, common.AUTHMode, common.HTTPAuthProxyCaseSensitive); len(failedKeys) > 0 {
+		if failedKeys := checkUnmodifiable(c.cfgManager, cfgs, common.AUTHMode); len(failedKeys) > 0 {
 			return false, fmt.Errorf("the keys %v can not be modified as new users have been inserted into database", failedKeys)
 		}
 	}

src/core/auth/authproxy/auth.go

@@ -53,10 +53,6 @@ type Auth struct {
 	Endpoint            string
 	TokenReviewEndpoint string
 	SkipSearch          bool
-	// When this attribute is set to false, the name of user/group will be converted to lower-case when onboarded to Harbor, so
-	// as long as the authentication is successful there's no difference in terms of upper or lower case that is used.
-	// It will be mapped to one entry in Harbor's User/Group table.
-	CaseSensitive    bool
 	settingTimeStamp    time.Time
 	client              *http.Client
 }
@@ -83,26 +79,25 @@ func (a *Auth) Authenticate(m models.AuthModel) (*models.User, error) {
 	if err != nil {
 		return nil, err
 	}
-	defer resp.Body.Close()
-	if resp.StatusCode == http.StatusOK {
-		name := a.normalizeName(m.Principal)
-		user := &models.User{Username: name}
 	data, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		log.Warningf("Failed to read response body, error: %v", err)
 		return nil, auth.ErrAuth{}
 	}
+	defer resp.Body.Close()
+	if resp.StatusCode == http.StatusOK {
 		s := session{}
 		err = json.Unmarshal(data, &s)
 		if err != nil {
 			return nil, auth.NewErrAuth(fmt.Sprintf("failed to read session %v", err))
 		}
-		if err := a.tokenReview(s.SessionID, user); err != nil {
+		user, err := a.tokenReview(s.SessionID)
-			return nil, auth.NewErrAuth(err.Error())
+		if err != nil {
+			return nil, auth.NewErrAuth(fmt.Sprintf("failed to do token review, error: %v", err))
 		}
 		return user, nil
 	} else if resp.StatusCode == http.StatusUnauthorized {
-		return nil, auth.ErrAuth{}
+		return nil, auth.NewErrAuth(string(data))
 	} else {
 		data, err := ioutil.ReadAll(resp.Body)
 		if err != nil {
@@ -112,26 +107,24 @@ func (a *Auth) Authenticate(m models.AuthModel) (*models.User, error) {
 	}
 }
 
-func (a *Auth) tokenReview(sessionID string, user *models.User) error {
+func (a *Auth) tokenReview(sessionID string) (*models.User, error) {
 	httpAuthProxySetting, err := config.HTTPAuthProxySetting()
 	if err != nil {
-		return err
+		return nil, err
 	}
 	reviewStatus, err := authproxy.TokenReview(sessionID, httpAuthProxySetting)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	u2, err := authproxy.UserFromReviewStatus(reviewStatus)
+	u, err := authproxy.UserFromReviewStatus(reviewStatus)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	user.GroupIDs = u2.GroupIDs
+	return u, nil
-	return nil
 }
 
 // OnBoardUser delegates to dao pkg to insert/update data in DB.
 func (a *Auth) OnBoardUser(u *models.User) error {
-	u.Username = a.normalizeName(u.Username)
 	return dao.OnBoardUser(u)
 }
 
@@ -154,7 +147,6 @@ func (a *Auth) SearchUser(username string) (*models.User, error) {
 	if err != nil {
 		log.Warningf("Failed to refresh configuration for HTTP Auth Proxy Authenticator, error: %v, the default settings will be used", err)
 	}
-	username = a.normalizeName(username)
 	var u *models.User
 	if a.SkipSearch {
 		u = &models.User{Username: username}
@@ -171,7 +163,6 @@ func (a *Auth) SearchGroup(groupKey string) (*models.UserGroup, error) {
 	if err != nil {
 		log.Warningf("Failed to refresh configuration for HTTP Auth Proxy Authenticator, error: %v, the default settings will be used", err)
 	}
-	groupKey = a.normalizeName(groupKey)
 	var ug *models.UserGroup
 	if a.SkipSearch {
 		ug = &models.UserGroup{
@@ -190,7 +181,6 @@ func (a *Auth) OnBoardGroup(u *models.UserGroup, altGroupName string) error {
 		return errors.New("Should provide a group name")
 	}
 	u.GroupType = common.HTTPGroupType
-	u.GroupName = a.normalizeName(u.GroupName)
 	err := group.OnBoardUserGroup(u)
 	if err != nil {
 		return err
@@ -248,13 +238,6 @@ func getTLSConfig(setting *models.HTTPAuthProxy) (*tls.Config, error) {
 	return &tls.Config{InsecureSkipVerify: !setting.VerifyCert}, nil
 }
 
-func (a *Auth) normalizeName(n string) string {
-	if !a.CaseSensitive {
-		return strings.ToLower(n)
-	}
-	return n
-}
-
 func init() {
 	auth.Register(common.HTTPAuth, &Auth{})
 }

src/core/auth/authproxy/auth_test.go

@@ -44,13 +44,11 @@ func TestMain(m *testing.M) {
 	a = &Auth{
 		Endpoint:            mockSvr.URL + "/test/login",
 		TokenReviewEndpoint: mockSvr.URL + "/test/tokenreview",
-		CaseSensitive:       false,
 	}
 	cfgMap := cut.GetUnitTestConfig()
 	conf := map[string]interface{}{
 		common.HTTPAuthProxyEndpoint:            a.Endpoint,
 		common.HTTPAuthProxyTokenReviewEndpoint: a.TokenReviewEndpoint,
-		common.HTTPAuthProxyCaseSensitive:       a.CaseSensitive,
 		common.HTTPAuthProxyVerifyCert:          false,
 		common.PostGreSQLSSLMode:                cfgMap[common.PostGreSQLSSLMode],
 		common.PostGreSQLUsername:               cfgMap[common.PostGreSQLUsername],
@@ -200,7 +198,7 @@ func TestAuth_OnBoardGroup(t *testing.T) {
 	assert.True(t, input.ID > 0, "The OnBoardGroup should have a valid group ID")
 	g, er := group.GetUserGroup(input.ID)
 	assert.Nil(t, er)
-	assert.Equal(t, "onboardtest", g.GroupName)
+	assert.Equal(t, "OnBoardTest", g.GroupName)
 
 	emptyGroup := &models.UserGroup{}
 	err := a.OnBoardGroup(emptyGroup, "")
@@ -226,7 +224,6 @@ func TestGetTLSConfig(t *testing.T) {
 				VerifyCert:          false,
 				SkipSearch:          false,
 				ServerCertificate:   "",
-				CaseSensitive:       false,
 			},
 			expect: result{
 				hasError:  false,
@@ -241,7 +238,6 @@ func TestGetTLSConfig(t *testing.T) {
 				VerifyCert:          false,
 				SkipSearch:          false,
 				ServerCertificate:   "This does not look like a cert",
-				CaseSensitive:       false,
 			},
 			expect: result{
 				hasError:  false,
@@ -256,7 +252,6 @@ func TestGetTLSConfig(t *testing.T) {
 				VerifyCert:          true,
 				SkipSearch:          false,
 				ServerCertificate:   "This does not look like a cert",
-				CaseSensitive:       false,
 			},
 			expect: result{
 				hasError: true,
@@ -269,7 +264,6 @@ func TestGetTLSConfig(t *testing.T) {
 				VerifyCert:          true,
 				SkipSearch:          false,
 				ServerCertificate:   "",
-				CaseSensitive:       false,
 			},
 			expect: result{
 				hasError:  false,
@@ -315,7 +309,6 @@ pkgODrJUf0p5dhcnLyA2nZolRV1rtwlgJstnEV4JpG1MwtmAZYZUilLvnfpVxTtA
 y1bQusZMygQezfCuEzsewF+OpANFovCTUEs6s5vyoVNP8lk=
 -----END CERTIFICATE-----
 `,
-				CaseSensitive: false,
 			},
 			expect: result{
 				hasError:  false,

src/core/auth/authproxy/test/server.go

@@ -15,14 +15,30 @@
 package test
 
 import (
+	"encoding/json"
+	"fmt"
+	"github.com/goharbor/harbor/src/common/utils"
+	"io/ioutil"
+	"k8s.io/api/authentication/v1beta1"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 )
 
+type userEntry struct {
+	username     string
+	password     string
+	sessionID    string
+	reviewStatus string
+}
+
 type authHandler struct {
+	entries []userEntry
 	m       map[string]string
 }
 
+var reviewStatusTpl = `{"apiVersion": "authentication.k8s.io/v1beta1", "kind": "TokenReview", "status": {"authenticated": true, "user": {"username": "%s", "groups": ["vsphere.local\\users", "vsphere.local\\administrators", "vsphere.local\\caadmins", "vsphere.local\\systemconfiguration.bashshelladministrators", "vsphere.local\\systemconfiguration.administrators", "vsphere.local\\licenseservice.administrators", "vsphere.local\\everyone"], "extra": {"method": ["basic"]}}}}`
+
 // ServeHTTP handles HTTP requests
 func (ah *authHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	if req.Method != http.MethodPost {
@@ -34,27 +50,64 @@ func (ah *authHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	} else if pass, ok := ah.m[u]; !ok || pass != p {
 		http.Error(rw, "", http.StatusUnauthorized)
 	} else {
-		_, e := rw.Write([]byte(`{"session_id": "hgx59wuWI3b0jcbtidv5mU1YCp-DOQ9NKR1iYKACdKCvbVn7"}`))
+		for _, e := range ah.entries {
-		if e != nil {
+			if e.username == strings.ToLower(u) {
-			panic(e)
+				_, err := rw.Write([]byte(fmt.Sprintf(`{"session_id": "%s"}`, e.sessionID)))
+				if err != nil {
+					panic(err)
+				} else {
+					return
 				}
 			}
 		}
+		http.Error(rw, fmt.Sprintf("Do not find entry in entrylist, username: %s", u), http.StatusUnauthorized)
+	}
+}
 
 type reviewTokenHandler struct {
+	entries []userEntry
 }
 
 func (rth *reviewTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	if req.Method != http.MethodPost {
 		http.Error(rw, "", http.StatusMethodNotAllowed)
 	}
-	rw.Write([]byte(`{"apiVersion": "authentication.k8s.io/v1beta1", "kind": "TokenReview", "status": {"authenticated": true, "user": {"username": "administrator@vsphere.local", "groups": ["vsphere.local\\users", "vsphere.local\\administrators", "vsphere.local\\caadmins", "vsphere.local\\systemconfiguration.bashshelladministrators", "vsphere.local\\systemconfiguration.administrators", "vsphere.local\\licenseservice.administrators", "vsphere.local\\everyone"], "extra": {"method": ["basic"]}}}}`))
+	bodyBytes, err := ioutil.ReadAll(req.Body)
+	if err != nil {
+		http.Error(rw, fmt.Sprintf("failed to read request body, error: %v", err), http.StatusBadRequest)
+	}
+	reviewData := &v1beta1.TokenReview{}
+	if err := json.Unmarshal(bodyBytes, reviewData); err != nil {
+		http.Error(rw, fmt.Sprintf("failed to decode request body, error: %v", err), http.StatusBadRequest)
+	}
+	defer req.Body.Close()
+	for _, e := range rth.entries {
+		if reviewData.Spec.Token == e.sessionID {
+			_, err := rw.Write([]byte(fmt.Sprintf(reviewStatusTpl, e.username)))
+			if err != nil {
+				panic(err)
+			} else {
+				return
+			}
+		}
+	}
+	http.Error(rw, fmt.Sprintf("failed to match token: %s, entrylist: %+v", reviewData.Spec.Token, rth.entries), http.StatusUnauthorized)
 }
 
 // NewMockServer creates the mock server for testing
 func NewMockServer(creds map[string]string) *httptest.Server {
 	mux := http.NewServeMux()
-	mux.Handle("/test/login", &authHandler{m: creds})
+	entryList := []userEntry{}
-	mux.Handle("/test/tokenreview", &reviewTokenHandler{})
+	for user, pwd := range creds {
+		e := userEntry{
+			username:     strings.ToLower(user),
+			password:     pwd,
+			sessionID:    utils.GenerateRandomString(),
+			reviewStatus: fmt.Sprintf(reviewStatusTpl, user),
+		}
+		entryList = append(entryList, e)
+	}
+	mux.Handle("/test/login", &authHandler{m: creds, entries: entryList})
+	mux.Handle("/test/tokenreview", &reviewTokenHandler{entries: entryList})
 	return httptest.NewTLSServer(mux)
 }

src/core/config/config.go

@@ -409,7 +409,6 @@ func HTTPAuthProxySetting() (*models.HTTPAuthProxy, error) {
 		VerifyCert:          cfgMgr.Get(common.HTTPAuthProxyVerifyCert).GetBool(),
 		SkipSearch:          cfgMgr.Get(common.HTTPAuthProxySkipSearch).GetBool(),
 		ServerCertificate:   cfgMgr.Get(common.HTTPAuthProxyServerCertificate).GetString(),
-		CaseSensitive:       cfgMgr.Get(common.HTTPAuthProxyCaseSensitive).GetBool(),
 	}, nil
 }
 

src/core/config/config_test.go

@@ -241,7 +241,6 @@ y1bQusZMygQezfCuEzsewF+OpANFovCTUEs6s5vyoVNP8lk=
 	m := map[string]interface{}{
 		common.HTTPAuthProxySkipSearch:        "true",
 		common.HTTPAuthProxyVerifyCert:        "true",
-		common.HTTPAuthProxyCaseSensitive:     "false",
 		common.HTTPAuthProxyEndpoint:          "https://auth.proxy/suffix",
 		common.HTTPAuthProxyServerCertificate: certificate,
 	}
@@ -252,7 +251,6 @@ y1bQusZMygQezfCuEzsewF+OpANFovCTUEs6s5vyoVNP8lk=
 		Endpoint:          "https://auth.proxy/suffix",
 		SkipSearch:        true,
 		VerifyCert:        true,
-		CaseSensitive:     false,
 		ServerCertificate: certificate,
 	})
 }

src/core/filter/security_test.go

@@ -257,7 +257,6 @@ func TestAuthProxyReqCtxModifier(t *testing.T) {
 		Endpoint:            "https://auth.proxy/suffix",
 		SkipSearch:          true,
 		VerifyCert:          false,
-		CaseSensitive:       true,
 		TokenReviewEndpoint: server.URL,
 	})
 

src/pkg/authproxy/http_test.go

@@ -132,7 +132,6 @@ y1bQusZMygQezfCuEzsewF+OpANFovCTUEs6s5vyoVNP8lk=
 				VerifyCert:          false,
 				SkipSearch:          false,
 				ServerCertificate:   "",
-				CaseSensitive:       false,
 			},
 			expect: rest.TLSClientConfig{
 				Insecure: true,