From cfff4d6d592b91c0fdbfe8fe16a067ace1ad0cbe Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Mon, 25 Nov 2019 20:31:22 +0800 Subject: [PATCH] populate group list when doing token review This commit fixes #9869 It has some refactor to make sure the group is populated when user is authenticated via tokenreview workflow. Signed-off-by: Daniel Jiang --- src/core/auth/authproxy/auth.go | 43 ++++++---------- src/core/filter/security.go | 15 +++--- src/pkg/authproxy/http.go | 44 +++++++++++++---- src/pkg/authproxy/http_test.go | 87 +++++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 44 deletions(-) create mode 100644 src/pkg/authproxy/http_test.go diff --git a/src/core/auth/authproxy/auth.go b/src/core/auth/authproxy/auth.go index e9a1414b9..b6be87304 100644 --- a/src/core/auth/authproxy/auth.go +++ b/src/core/auth/authproxy/auth.go @@ -34,7 +34,6 @@ import ( "github.com/goharbor/harbor/src/core/auth" "github.com/goharbor/harbor/src/core/config" "github.com/goharbor/harbor/src/pkg/authproxy" - k8s_api_v1beta1 "k8s.io/api/authentication/v1beta1" ) const refreshDuration = 2 * time.Second @@ -101,31 +100,12 @@ func (a *Auth) Authenticate(m models.AuthModel) (*models.User, error) { s := session{} err = json.Unmarshal(data, &s) if err != nil { - log.Errorf("failed to read session %v", err) + return nil, auth.NewErrAuth(fmt.Sprintf("failed to read session %v", err)) } - - reviewResponse, err := a.tokenReview(s.SessionID) - if err != nil { - return nil, err - } - if reviewResponse == nil { - return nil, auth.ErrAuth{} - } - - // Attach user group ID information - ugList := reviewResponse.Status.User.Groups - log.Debugf("user groups %+v", ugList) - if len(ugList) > 0 { - userGroups := models.UserGroupsFromName(ugList, common.HTTPGroupType) - groupIDList, err := group.PopulateGroup(userGroups) - if err != nil { - return nil, err - } - log.Debugf("current user's group ID list is %+v", groupIDList) - user.GroupIDs = groupIDList + if err := a.tokenReview(s.SessionID, user); err != nil { + return nil, auth.NewErrAuth(err.Error()) } return user, nil - } else if resp.StatusCode == http.StatusUnauthorized { return nil, auth.ErrAuth{} } else { @@ -134,17 +114,24 @@ func (a *Auth) Authenticate(m models.AuthModel) (*models.User, error) { log.Warningf("Failed to read response body, error: %v", err) } return nil, fmt.Errorf("failed to authenticate, status code: %d, text: %s", resp.StatusCode, string(data)) - } - } -func (a *Auth) tokenReview(sessionID string) (*k8s_api_v1beta1.TokenReview, error) { +func (a *Auth) tokenReview(sessionID string, user *models.User) error { httpAuthProxySetting, err := config.HTTPAuthProxySetting() if err != nil { - return nil, err + return err } - return authproxy.TokenReview(sessionID, httpAuthProxySetting) + reviewStatus, err := authproxy.TokenReview(sessionID, httpAuthProxySetting) + if err != nil { + return err + } + u2, err := authproxy.UserFromReviewStatus(reviewStatus) + if err != nil { + return err + } + user.GroupIDs = u2.GroupIDs + return nil } // OnBoardUser delegates to dao pkg to insert/update data in DB. diff --git a/src/core/filter/security.go b/src/core/filter/security.go index f6d6b887a..86c1a568f 100644 --- a/src/core/filter/security.go +++ b/src/core/filter/security.go @@ -325,16 +325,16 @@ func (ap *authProxyReqCtxModifier) Modify(ctx *beegoctx.Context) bool { log.Errorf("fail to get auth proxy settings, %v", err) return false } - tokenReviewResponse, err := authproxy.TokenReview(proxyPwd, httpAuthProxyConf) + tokenReviewStatus, err := authproxy.TokenReview(proxyPwd, httpAuthProxyConf) if err != nil { log.Errorf("fail to review token, %v", err) return false } - - if !tokenReviewResponse.Status.Authenticated { - log.Errorf("fail to auth user: %s", rawUserName) + if rawUserName != tokenReviewStatus.User.Username { + log.Errorf("user name doesn't match with token: %s", rawUserName) return false } + user, err := dao.GetUser(models.User{ Username: rawUserName, }) @@ -346,11 +346,12 @@ func (ap *authProxyReqCtxModifier) Modify(ctx *beegoctx.Context) bool { log.Errorf("User: %s has not been on boarded yet.", rawUserName) return false } - if rawUserName != tokenReviewResponse.Status.User.Username { - log.Errorf("user name doesn't match with token: %s", rawUserName) + u2, err := authproxy.UserFromReviewStatus(tokenReviewStatus) + if err != nil { + log.Errorf("Failed to get user information from token review status, error: %v", err) return false } - + user.GroupIDs = u2.GroupIDs pm := config.GlobalProjectMgr log.Debug("creating local database security context for auth proxy...") securCtx := local.NewSecurityContext(user, pm) diff --git a/src/pkg/authproxy/http.go b/src/pkg/authproxy/http.go index baeed17cf..61356fd3f 100644 --- a/src/pkg/authproxy/http.go +++ b/src/pkg/authproxy/http.go @@ -2,6 +2,9 @@ package authproxy import ( "encoding/json" + "fmt" + "github.com/goharbor/harbor/src/common" + "github.com/goharbor/harbor/src/common/dao/group" "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/utils/log" k8s_api_v1beta1 "k8s.io/api/authentication/v1beta1" @@ -13,8 +16,9 @@ import ( ) // TokenReview ... -func TokenReview(sessionID string, authProxyConfig *models.HTTPAuthProxy) (*k8s_api_v1beta1.TokenReview, error) { +func TokenReview(rawToken string, authProxyConfig *models.HTTPAuthProxy) (k8s_api_v1beta1.TokenReviewStatus, error) { + emptyStatus := k8s_api_v1beta1.TokenReviewStatus{} // Init auth client with the auth proxy endpoint. authClientCfg := &rest.Config{ Host: authProxyConfig.TokenReviewEndpoint, @@ -22,14 +26,14 @@ func TokenReview(sessionID string, authProxyConfig *models.HTTPAuthProxy) (*k8s_ GroupVersion: &schema.GroupVersion{}, NegotiatedSerializer: serializer.DirectCodecFactory{CodecFactory: scheme.Codecs}, }, - BearerToken: sessionID, + BearerToken: rawToken, TLSClientConfig: rest.TLSClientConfig{ Insecure: !authProxyConfig.VerifyCert, }, } authClient, err := rest.RESTClientFor(authClientCfg) if err != nil { - return nil, err + return emptyStatus, err } // Do auth with the token. @@ -39,27 +43,49 @@ func TokenReview(sessionID string, authProxyConfig *models.HTTPAuthProxy) (*k8s_ APIVersion: "authentication.k8s.io/v1beta1", }, Spec: k8s_api_v1beta1.TokenReviewSpec{ - Token: sessionID, + Token: rawToken, }, } res := authClient.Post().Body(tokenReviewRequest).Do() err = res.Error() if err != nil { log.Errorf("fail to POST auth request, %v", err) - return nil, err + return emptyStatus, err } resRaw, err := res.Raw() if err != nil { log.Errorf("fail to get raw data of token review, %v", err) - return nil, err + return emptyStatus, err } // Parse the auth response, check the user name and authenticated status. tokenReviewResponse := &k8s_api_v1beta1.TokenReview{} - err = json.Unmarshal(resRaw, &tokenReviewResponse) + err = json.Unmarshal(resRaw, tokenReviewResponse) if err != nil { log.Errorf("fail to decode token review, %v", err) - return nil, err + return emptyStatus, err } - return tokenReviewResponse, nil + return tokenReviewResponse.Status, nil + +} + +// UserFromReviewStatus transform a review status to a user model. +// Group entries will be populated if needed. +func UserFromReviewStatus(status k8s_api_v1beta1.TokenReviewStatus) (*models.User, error) { + if !status.Authenticated { + return nil, fmt.Errorf("failed to authenticate the token, error in status: %s", status.Error) + } + user := &models.User{ + Username: status.User.Username, + } + if len(status.User.Groups) > 0 { + userGroups := models.UserGroupsFromName(status.User.Groups, common.HTTPGroupType) + groupIDList, err := group.PopulateGroup(userGroups) + if err != nil { + return nil, err + } + log.Debugf("current user's group ID list is %+v", groupIDList) + user.GroupIDs = groupIDList + } + return user, nil } diff --git a/src/pkg/authproxy/http_test.go b/src/pkg/authproxy/http_test.go new file mode 100644 index 000000000..24374043e --- /dev/null +++ b/src/pkg/authproxy/http_test.go @@ -0,0 +1,87 @@ +package authproxy + +import ( + "github.com/goharbor/harbor/src/common/dao" + "github.com/goharbor/harbor/src/common/dao/group" + "github.com/stretchr/testify/assert" + "k8s.io/api/authentication/v1beta1" + "os" + "testing" +) + +func TestMain(m *testing.M) { + dao.PrepareTestForPostgresSQL() + result := m.Run() + if result != 0 { + os.Exit(result) + } +} + +func TestUserFromReviewStatus(t *testing.T) { + type result struct { + hasErr bool + username string + groupLen int + } + cases := []struct { + input v1beta1.TokenReviewStatus + expect result + }{ + { + input: v1beta1.TokenReviewStatus{ + Authenticated: false, + Error: "connection error", + }, + expect: result{ + hasErr: true, + }, + }, + { + input: v1beta1.TokenReviewStatus{ + Authenticated: true, + User: v1beta1.UserInfo{ + Username: "jack", + UID: "u-1", + }, + }, + expect: result{ + hasErr: false, + username: "jack", + groupLen: 0, + }, + }, + { + input: v1beta1.TokenReviewStatus{ + Authenticated: true, + User: v1beta1.UserInfo{ + Username: "daniel", + Groups: []string{"group1", "group2"}, + }, + Error: "", + }, + expect: result{ + hasErr: false, + username: "daniel", + groupLen: 2, + }, + }, + } + for _, c := range cases { + u, err := UserFromReviewStatus(c.input) + if c.expect.hasErr == true { + assert.NotNil(t, err) + } else { + assert.Nil(t, err) + assert.Equal(t, c.expect.username, u.Username) + assert.Equal(t, c.expect.groupLen, len(u.GroupIDs)) + } + if u != nil { + for _, gid := range u.GroupIDs { + t.Logf("Deleting group %d", gid) + if err := group.DeleteUserGroup(gid); err != nil { + panic(err) + } + } + } + } +}