From f4ff369ed0f0d8118100ae54fbc6dc667588ec1e Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Tue, 20 Oct 2020 15:19:11 +0800 Subject: [PATCH 1/2] Add admin group support to OIDC auth mode Add oidc_admin_group to configuration, and make sure a token with the group name in group claim has the admin authority. Signed-off-by: Daniel Jiang --- src/common/config/metadata/metadatalist.go | 1 + src/common/const.go | 1 + src/common/models/config.go | 1 + src/common/utils/oidc/helper.go | 72 ++++++--- src/common/utils/oidc/helper_test.go | 165 +++++++++++++++++---- src/common/utils/oidc/secret.go | 12 +- src/common/utils/oidc/testutils.go | 13 ++ src/core/config/config.go | 1 + src/core/controllers/oidc.go | 25 +--- src/server/middleware/security/idtoken.go | 23 ++- 10 files changed, 223 insertions(+), 91 deletions(-) diff --git a/src/common/config/metadata/metadatalist.go b/src/common/config/metadata/metadatalist.go index cf3ccc62b..694cb1ae1 100644 --- a/src/common/config/metadata/metadatalist.go +++ b/src/common/config/metadata/metadatalist.go @@ -139,6 +139,7 @@ var ( {Name: common.OIDCCLientID, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}}, {Name: common.OIDCClientSecret, Scope: UserScope, Group: OIDCGroup, ItemType: &PasswordType{}}, {Name: common.OIDCGroupsClaim, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}}, + {Name: common.OIDCAdminGroup, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}}, {Name: common.OIDCScope, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}}, {Name: common.OIDCUserClaim, Scope: UserScope, Group: OIDCGroup, ItemType: &StringType{}}, {Name: common.OIDCVerifyCert, Scope: UserScope, Group: OIDCGroup, DefaultValue: "true", ItemType: &BoolType{}}, diff --git a/src/common/const.go b/src/common/const.go index daaf55e14..47b17ba8d 100755 --- a/src/common/const.go +++ b/src/common/const.go @@ -105,6 +105,7 @@ const ( OIDCCLientID = "oidc_client_id" OIDCClientSecret = "oidc_client_secret" OIDCVerifyCert = "oidc_verify_cert" + OIDCAdminGroup = "oidc_admin_group" OIDCGroupsClaim = "oidc_groups_claim" OIDCAutoOnboard = "oidc_auto_onboard" OIDCScope = "oidc_scope" diff --git a/src/common/models/config.go b/src/common/models/config.go index 806969fd6..c970dd167 100644 --- a/src/common/models/config.go +++ b/src/common/models/config.go @@ -85,6 +85,7 @@ type OIDCSetting struct { ClientID string `json:"client_id"` ClientSecret string `json:"client_secret"` GroupsClaim string `json:"groups_claim"` + AdminGroup string `json:"admin_group"` RedirectURL string `json:"redirect_url"` Scope []string `json:"scope"` UserClaim string `json:"user_claim"` diff --git a/src/common/utils/oidc/helper.go b/src/common/utils/oidc/helper.go index 228376dab..18a220b81 100644 --- a/src/common/utils/oidc/helper.go +++ b/src/common/utils/oidc/helper.go @@ -26,6 +26,8 @@ import ( "time" gooidc "github.com/coreos/go-oidc" + "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/core/config" "github.com/goharbor/harbor/src/lib/log" @@ -119,12 +121,13 @@ type Token struct { // UserInfo wraps the information that is extracted via token. It will be transformed to data object that is persisted // in the DB type UserInfo struct { - Issuer string `json:"iss"` - Subject string `json:"sub"` - Username string `json:"name"` - Email string `json:"email"` - Groups []string `json:"groups"` - hasGroupClaim bool + Issuer string `json:"iss"` + Subject string `json:"sub"` + Username string `json:"name"` + Email string `json:"email"` + Groups []string `json:"groups"` + AdminGroupMember bool `json:"AdminGroupMember"` + hasGroupClaim bool } func getOauthConf() (*oauth2.Config, error) { @@ -247,7 +250,7 @@ func UserInfoFromToken(ctx context.Context, token *Token) (*UserInfo, error) { return nil, err } setting := provider.setting.Load().(models.OIDCSetting) - local, err := userInfoFromIDToken(ctx, token, setting) + local, err := UserInfoFromIDToken(ctx, token, setting) if err != nil { return nil, err } @@ -280,9 +283,11 @@ func mergeUserInfo(remote, local *UserInfo) *UserInfo { } if remote.hasGroupClaim { res.Groups = remote.Groups + res.AdminGroupMember = remote.AdminGroupMember res.hasGroupClaim = true } else if local.hasGroupClaim { res.Groups = local.Groups + res.AdminGroupMember = local.AdminGroupMember res.hasGroupClaim = true } else { res.Groups = []string{} @@ -300,10 +305,11 @@ func userInfoFromRemote(ctx context.Context, token *Token, setting models.OIDCSe if err != nil { return nil, err } - return userInfoFromClaims(u, setting.GroupsClaim, setting.UserClaim) + return userInfoFromClaims(u, setting) } -func userInfoFromIDToken(ctx context.Context, token *Token, setting models.OIDCSetting) (*UserInfo, error) { +// UserInfoFromIDToken extract user info from ID token +func UserInfoFromIDToken(ctx context.Context, token *Token, setting models.OIDCSetting) (*UserInfo, error) { if token.RawIDToken == "" { return nil, nil } @@ -312,34 +318,41 @@ func userInfoFromIDToken(ctx context.Context, token *Token, setting models.OIDCS return nil, err } - return userInfoFromClaims(idt, setting.GroupsClaim, setting.UserClaim) + return userInfoFromClaims(idt, setting) } -func userInfoFromClaims(c claimsProvider, g, u string) (*UserInfo, error) { +func userInfoFromClaims(c claimsProvider, setting models.OIDCSetting) (*UserInfo, error) { res := &UserInfo{} if err := c.Claims(res); err != nil { return nil, err } - if u != "" { + if setting.UserClaim != "" { allClaims := make(map[string]interface{}) if err := c.Claims(&allClaims); err != nil { return nil, err } - username, ok := allClaims[u].(string) + username, ok := allClaims[setting.UserClaim].(string) if !ok { - return nil, fmt.Errorf("OIDC. Failed to recover Username from claim. Claim '%s' is invalid or not a string", u) + return nil, fmt.Errorf("OIDC. Failed to recover Username from claim. Claim '%s' is invalid or not a string", setting.UserClaim) } res.Username = username - } - res.Groups, res.hasGroupClaim = GroupsFromClaims(c, g) + res.Groups, res.hasGroupClaim = groupsFromClaims(c, setting.GroupsClaim) + if len(setting.AdminGroup) > 0 { + for _, g := range res.Groups { + if g == setting.AdminGroup { + res.AdminGroupMember = true + break + } + } + } return res, nil } -// GroupsFromClaims fetches the group name list from claimprovider, such as decoded ID token. +// groupsFromClaims fetches the group name list from claimprovider, such as decoded ID token. // If the claims does not have the claim defined as k, the second return value will be false, otherwise true -func GroupsFromClaims(gp claimsProvider, k string) ([]string, bool) { +func groupsFromClaims(gp claimsProvider, k string) ([]string, bool) { res := make([]string, 0) claimMap := make(map[string]interface{}) if err := gp.Claims(&claimMap); err != nil { @@ -362,6 +375,29 @@ func GroupsFromClaims(gp claimsProvider, k string) ([]string, bool) { return res, true } +type populate func(groupNames []string) ([]int, error) + +func populateGroupsDB(groupNames []string) ([]int, error) { + return group.PopulateGroup(models.UserGroupsFromName(groupNames, common.OIDCGroupType)) +} + +// InjectGroupsToUser populates the group to DB and inject the group IDs to user model. +// The third optional parm is for UT only, when using the func, the third +func InjectGroupsToUser(info *UserInfo, user *models.User, f ...populate) { + var populateGroups populate + if len(f) == 0 { + populateGroups = populateGroupsDB + } else { + populateGroups = f[0] + } + if gids, err := populateGroups(info.Groups); err != nil { + log.Warningf("failed to get group ID, error: %v, skip populating groups", err) + } else { + user.GroupIDs = gids + } + user.AdminRoleInAuth = info.AdminGroupMember +} + // Conn wraps connection info of an OIDC endpoint type Conn struct { URL string `json:"url"` diff --git a/src/common/utils/oidc/helper_test.go b/src/common/utils/oidc/helper_test.go index 96b2970ff..fcc5e2c0c 100644 --- a/src/common/utils/oidc/helper_test.go +++ b/src/common/utils/oidc/helper_test.go @@ -165,7 +165,7 @@ func TestGroupsFromClaim(t *testing.T) { for _, tc := range m { - r, ok := GroupsFromClaims(&fakeClaims{tc.input}, tc.key) + r, ok := groupsFromClaims(&fakeClaims{tc.input}, tc.key) assert.Equal(t, tc.expect, r) assert.Equal(t, tc.ok, ok) } @@ -173,10 +173,9 @@ func TestGroupsFromClaim(t *testing.T) { func TestUserInfoFromClaims(t *testing.T) { s := []struct { - input map[string]interface{} - groupClaim string - userClaim string - expect *UserInfo + input map[string]interface{} + setting models.OIDCSetting + expect *UserInfo }{ { input: map[string]interface{}{ @@ -184,8 +183,12 @@ func TestUserInfoFromClaims(t *testing.T) { "email": "daniel@gmail.com", "groups": []interface{}{"g1", "g2"}, }, - groupClaim: "grouplist", - userClaim: "", + setting: models.OIDCSetting{ + Name: "t1", + GroupsClaim: "grouplist", + UserClaim: "", + AdminGroup: "g1", + }, expect: &UserInfo{ Issuer: "", Subject: "", @@ -201,15 +204,20 @@ func TestUserInfoFromClaims(t *testing.T) { "email": "daniel@gmail.com", "groups": []interface{}{"g1", "g2"}, }, - groupClaim: "groups", - userClaim: "", + setting: models.OIDCSetting{ + Name: "t2", + GroupsClaim: "groups", + UserClaim: "", + AdminGroup: "g1", + }, expect: &UserInfo{ - Issuer: "", - Subject: "", - Username: "Daniel", - Email: "daniel@gmail.com", - Groups: []string{"g1", "g2"}, - hasGroupClaim: true, + Issuer: "", + Subject: "", + Username: "Daniel", + Email: "daniel@gmail.com", + Groups: []string{"g1", "g2"}, + AdminGroupMember: true, + hasGroupClaim: true, }, }, { @@ -220,15 +228,20 @@ func TestUserInfoFromClaims(t *testing.T) { "email": "jack@gmail.com", "groupclaim": []interface{}{}, }, - groupClaim: "groupclaim", - userClaim: "", + setting: models.OIDCSetting{ + Name: "t3", + GroupsClaim: "groupclaim", + UserClaim: "", + AdminGroup: "g1", + }, expect: &UserInfo{ - Issuer: "issuer", - Subject: "subject000", - Username: "jack", - Email: "jack@gmail.com", - Groups: []string{}, - hasGroupClaim: true, + Issuer: "issuer", + Subject: "subject000", + Username: "jack", + Email: "jack@gmail.com", + Groups: []string{}, + hasGroupClaim: true, + AdminGroupMember: false, }, }, { @@ -237,20 +250,25 @@ func TestUserInfoFromClaims(t *testing.T) { "email": "airadier@gmail.com", "groups": []interface{}{"g1", "g2"}, }, - groupClaim: "grouplist", - userClaim: "email", + setting: models.OIDCSetting{ + Name: "t4", + GroupsClaim: "grouplist", + UserClaim: "email", + AdminGroup: "g1", + }, expect: &UserInfo{ - Issuer: "", - Subject: "", - Username: "airadier@gmail.com", - Email: "airadier@gmail.com", - Groups: []string{}, - hasGroupClaim: false, + Issuer: "", + Subject: "", + Username: "airadier@gmail.com", + Email: "airadier@gmail.com", + Groups: []string{}, + hasGroupClaim: false, + AdminGroupMember: false, }, }, } for _, tc := range s { - out, err := userInfoFromClaims(&fakeClaims{tc.input}, tc.groupClaim, tc.userClaim) + out, err := userInfoFromClaims(&fakeClaims{tc.input}, tc.setting) assert.Nil(t, err) assert.Equal(t, *tc.expect, *out) } @@ -347,3 +365,86 @@ func TestMergeUserInfo(t *testing.T) { assert.Equal(t, *tc.expected, *m) } } + +func TestInjectGroupsToUser(t *testing.T) { + cases := []struct { + userInfo *UserInfo + old *models.User + new *models.User + }{ + { + userInfo: &UserInfo{ + Issuer: "issuer-yahoo", + Subject: "subject-jim", + Username: "jim", + Email: "jim@gmail.com", + Groups: []string{}, + hasGroupClaim: true, + AdminGroupMember: false, + }, + old: &models.User{ + Username: "jim", + Email: "jim@gmail.com", + GroupIDs: []int{}, + AdminRoleInAuth: false, + }, + new: &models.User{ + Username: "jim", + Email: "jim@gmail.com", + GroupIDs: []int{}, + AdminRoleInAuth: false, + }, + }, + { + userInfo: &UserInfo{ + Issuer: "issuer-yahoo", + Subject: "subject-jim", + Username: "jim", + Email: "jim@gmail.com", + Groups: []string{"1", "abc"}, + hasGroupClaim: true, + AdminGroupMember: true, + }, + old: &models.User{ + Username: "jim", + Email: "jim@gmail.com", + GroupIDs: []int{}, + AdminRoleInAuth: false, + }, + new: &models.User{ + Username: "jim", + Email: "jim@gmail.com", + GroupIDs: []int{}, + AdminRoleInAuth: true, + }, + }, + { + userInfo: &UserInfo{ + Issuer: "issuer-yahoo", + Subject: "subject-jim", + Username: "jim", + Email: "jim@gmail.com", + Groups: []string{"1", "2"}, + hasGroupClaim: true, + AdminGroupMember: true, + }, + old: &models.User{ + Username: "jim", + Email: "jim@gmail.com", + GroupIDs: []int{}, + AdminRoleInAuth: false, + }, + new: &models.User{ + Username: "jim", + Email: "jim@gmail.com", + GroupIDs: []int{1, 2}, + AdminRoleInAuth: true, + }, + }, + } + for _, c := range cases { + u := c.old + InjectGroupsToUser(c.userInfo, u, mockPopulateGroups) + assert.Equal(t, *c.new, *u) + } +} diff --git a/src/common/utils/oidc/secret.go b/src/common/utils/oidc/secret.go index fd4d31966..b33ea94a3 100644 --- a/src/common/utils/oidc/secret.go +++ b/src/common/utils/oidc/secret.go @@ -4,15 +4,14 @@ import ( "context" "encoding/json" "fmt" - "github.com/goharbor/harbor/src/common" + "sync" + "github.com/goharbor/harbor/src/common/dao" - "github.com/goharbor/harbor/src/common/dao/group" "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/utils" "github.com/goharbor/harbor/src/core/config" "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/log" - "sync" ) // SecretVerifyError wraps the different errors happened when verifying a secret for OIDC user. When seeing this error, @@ -117,12 +116,7 @@ func (dm *defaultManager) VerifySecret(ctx context.Context, username string, sec if err != nil { return nil, verifyError(err) } - gids, err := group.PopulateGroup(models.UserGroupsFromName(info.Groups, common.OIDCGroupType)) - if err != nil { - log.Warningf("failed to get group ID, error: %v, skip populating groups", err) - } else { - user.GroupIDs = gids - } + InjectGroupsToUser(info, user) return user, nil } diff --git a/src/common/utils/oidc/testutils.go b/src/common/utils/oidc/testutils.go index 6ff429825..476bcb4ff 100644 --- a/src/common/utils/oidc/testutils.go +++ b/src/common/utils/oidc/testutils.go @@ -3,6 +3,8 @@ package oidc import ( "context" "fmt" + "strconv" + "github.com/goharbor/harbor/src/common/models" ) import "errors" @@ -24,3 +26,14 @@ func (fv *fakeVerifier) VerifySecret(ctx context.Context, name string, secret st func SetHardcodeVerifierForTest(s string) { m = &fakeVerifier{s} } +func mockPopulateGroups(groupNames []string) ([]int, error) { + res := make([]int, 0) + for _, g := range groupNames { + id, err := strconv.Atoi(g) + if err != nil { + return res, err + } + res = append(res, id) + } + return res, nil +} diff --git a/src/core/config/config.go b/src/core/config/config.go index 0a410f452..6cdc8c53e 100755 --- a/src/core/config/config.go +++ b/src/core/config/config.go @@ -440,6 +440,7 @@ func OIDCSetting() (*models.OIDCSetting, error) { ClientID: cfgMgr.Get(common.OIDCCLientID).GetString(), ClientSecret: cfgMgr.Get(common.OIDCClientSecret).GetString(), GroupsClaim: cfgMgr.Get(common.OIDCGroupsClaim).GetString(), + AdminGroup: cfgMgr.Get(common.OIDCAdminGroup).GetString(), RedirectURL: extEndpoint + common.OIDCCallbackPath, Scope: scope, UserClaim: cfgMgr.Get(common.OIDCUserClaim).GetString(), diff --git a/src/core/controllers/oidc.go b/src/core/controllers/oidc.go index 20f3a971e..184d6c55a 100644 --- a/src/core/controllers/oidc.go +++ b/src/core/controllers/oidc.go @@ -20,8 +20,6 @@ import ( "net/http" "strings" - "github.com/goharbor/harbor/src/common/dao/group" - "github.com/goharbor/harbor/src/common" "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" @@ -153,12 +151,7 @@ func (oc *OIDCController) Callback() { return } } - - gids, err := group.PopulateGroup(models.UserGroupsFromName(info.Groups, common.OIDCGroupType)) - if err != nil { - log.Warningf("Failed to populate groups, error: %v, user will have empty group list, username: %s", err, info.Username) - } - u.GroupIDs = gids + oidc.InjectGroupsToUser(info, u) oidcUser, err := dao.GetOIDCUserByUserID(u.UserID) if err != nil { oc.SendInternalServerError(err) @@ -181,30 +174,24 @@ func userOnboard(oc *OIDCController, info *oidc.UserInfo, username string, token oc.SendInternalServerError(err) return nil, false } - - gids, err := group.PopulateGroup(models.UserGroupsFromName(info.Groups, common.OIDCGroupType)) - if err != nil { - log.Warningf("Failed to populate group user will have empty group list. username: %s", username) - } - oidcUser := models.OIDCUser{ SubIss: info.Subject + info.Issuer, Secret: s, Token: t, } - user := models.User{ + user := &models.User{ Username: username, Realname: username, Email: info.Email, - GroupIDs: gids, OIDCUserMeta: &oidcUser, Comment: oidcUserComment, } + oidc.InjectGroupsToUser(info, user) - log.Debugf("User created: %+v\n", user) + log.Debugf("User created: %+v\n", *user) - err = dao.OnBoardOIDCUser(&user) + err = dao.OnBoardOIDCUser(user) if err != nil { if strings.Contains(err.Error(), dao.ErrDupUser.Error()) { oc.RenderError(http.StatusConflict, "Conflict, the user with same username or email has been onboarded.") @@ -215,7 +202,7 @@ func userOnboard(oc *OIDCController, info *oidc.UserInfo, username string, token return nil, false } - return &user, true + return user, true } // Onboard handles the request to onboard a user authenticated via OIDC provider diff --git a/src/server/middleware/security/idtoken.go b/src/server/middleware/security/idtoken.go index 323065198..5b7574b52 100644 --- a/src/server/middleware/security/idtoken.go +++ b/src/server/middleware/security/idtoken.go @@ -20,8 +20,6 @@ import ( "github.com/goharbor/harbor/src/common" "github.com/goharbor/harbor/src/common/dao" - "github.com/goharbor/harbor/src/common/dao/group" - "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/security" "github.com/goharbor/harbor/src/common/security/local" "github.com/goharbor/harbor/src/common/utils/oidc" @@ -33,14 +31,16 @@ import ( type idToken struct{} func (i *idToken) Generate(req *http.Request) security.Context { - log := log.G(req.Context()) - if lib.GetAuthMode(req.Context()) != common.OIDCAuth { + ctx := req.Context() + log := log.G(ctx) + if lib.GetAuthMode(ctx) != common.OIDCAuth { return nil } if !strings.HasPrefix(req.URL.Path, "/api") { return nil } - claims, err := oidc.VerifyToken(req.Context(), bearerToken(req)) + token := bearerToken(req) + claims, err := oidc.VerifyToken(ctx, token) if err != nil { log.Warningf("failed to verify token: %v", err) return nil @@ -54,19 +54,16 @@ func (i *idToken) Generate(req *http.Request) security.Context { log.Warning("user matches token's claims is not onboarded.") return nil } - settings, err := config.OIDCSetting() + setting, err := config.OIDCSetting() if err != nil { log.Errorf("failed to get OIDC settings: %v", err) return nil } - if groupNames, ok := oidc.GroupsFromClaims(claims, settings.GroupsClaim); ok { - groups := models.UserGroupsFromName(groupNames, common.OIDCGroupType) - u.GroupIDs, err = group.PopulateGroup(groups) - if err != nil { - log.Errorf("failed to get group ID list for OIDC user %s: %v", u.Username, err) - return nil - } + info, err := oidc.UserInfoFromIDToken(ctx, &oidc.Token{RawIDToken: token}, *setting) + if err != nil { + log.Errorf("Failed to get user info from ID token: %v", err) } + oidc.InjectGroupsToUser(info, u) log.Debugf("an ID token security context generated for request %s %s", req.Method, req.URL.Path) return local.NewSecurityContext(u, config.GlobalProjectMgr) } From 649c9814e48c9ad146ab8fd9ad18211806d7df52 Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Thu, 22 Oct 2020 16:53:37 +0800 Subject: [PATCH 2/2] Address review comment by Yan Resolve review comment in PR #13312 Signed-off-by: Daniel Jiang --- src/common/utils/oidc/helper.go | 8 ++++++-- src/server/middleware/security/idtoken.go | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/common/utils/oidc/helper.go b/src/common/utils/oidc/helper.go index 18a220b81..b0ae794fb 100644 --- a/src/common/utils/oidc/helper.go +++ b/src/common/utils/oidc/helper.go @@ -126,7 +126,7 @@ type UserInfo struct { Username string `json:"name"` Email string `json:"email"` Groups []string `json:"groups"` - AdminGroupMember bool `json:"AdminGroupMember"` + AdminGroupMember bool `json:"admin_group_member"` hasGroupClaim bool } @@ -382,8 +382,12 @@ func populateGroupsDB(groupNames []string) ([]int, error) { } // InjectGroupsToUser populates the group to DB and inject the group IDs to user model. -// The third optional parm is for UT only, when using the func, the third +// The third optional parm is for UT only. func InjectGroupsToUser(info *UserInfo, user *models.User, f ...populate) { + if info == nil || user == nil { + log.Warningf("user info or user model is nil, skip the func") + return + } var populateGroups populate if len(f) == 0 { populateGroups = populateGroupsDB diff --git a/src/server/middleware/security/idtoken.go b/src/server/middleware/security/idtoken.go index 5b7574b52..6b9668651 100644 --- a/src/server/middleware/security/idtoken.go +++ b/src/server/middleware/security/idtoken.go @@ -62,6 +62,7 @@ func (i *idToken) Generate(req *http.Request) security.Context { info, err := oidc.UserInfoFromIDToken(ctx, &oidc.Token{RawIDToken: token}, *setting) if err != nil { log.Errorf("Failed to get user info from ID token: %v", err) + return nil } oidc.InjectGroupsToUser(info, u) log.Debugf("an ID token security context generated for request %s %s", req.Method, req.URL.Path)