From 08e00744bef8814b245adce7c3d213ef208a4063 Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Thu, 4 Apr 2019 14:59:50 +0800 Subject: [PATCH 1/4] Fix misc bugs for e2e OIDC user onboard process This commit adjust the code and fix some bugs to make onboard process work. Only thing missed is that the UI will need to initiate the redirection, because the request of onboarding a user was sent via ajax call and didn't handle the 302. Signed-off-by: Daniel Jiang --- src/common/utils/oidc/helper.go | 27 ++++++++++++------- src/core/controllers/base.go | 4 ++- src/core/controllers/oidc.go | 48 ++++++++++++++++++++++++--------- 3 files changed, 55 insertions(+), 24 deletions(-) diff --git a/src/common/utils/oidc/helper.go b/src/common/utils/oidc/helper.go index 75a45f6b1..b32e224a7 100644 --- a/src/common/utils/oidc/helper.go +++ b/src/common/utils/oidc/helper.go @@ -90,16 +90,7 @@ func (p *providerHelper) create() error { return errors.New("the configuration is not loaded") } s := p.setting.Load().(models.OIDCSetting) - var client *http.Client - if s.SkipCertVerify { - client = &http.Client{ - Transport: insecureTransport, - } - } else { - client = &http.Client{} - } - ctx := context.Background() - gooidc.ClientContext(ctx, client) + ctx := clientCtx(context.Background(), s.SkipCertVerify) provider, err := gooidc.NewProvider(ctx, s.Endpoint) if err != nil { return fmt.Errorf("failed to create OIDC provider, error: %v", err) @@ -170,6 +161,8 @@ func ExchangeToken(ctx context.Context, code string) (*Token, error) { log.Errorf("Failed to get OAuth configuration, error: %v", err) return nil, err } + setting := provider.setting.Load().(models.OIDCSetting) + ctx = clientCtx(ctx, setting.SkipCertVerify) oauthToken, err := oauth.Exchange(ctx, code) if err != nil { return nil, err @@ -184,5 +177,19 @@ func VerifyToken(ctx context.Context, rawIDToken string) (*gooidc.IDToken, error return nil, err } verifier := p.Verifier(&gooidc.Config{ClientID: provider.setting.Load().(models.OIDCSetting).ClientID}) + setting := provider.setting.Load().(models.OIDCSetting) + ctx = clientCtx(ctx, setting.SkipCertVerify) return verifier.Verify(ctx, rawIDToken) } + +func clientCtx(ctx context.Context, skipCertVerify bool) context.Context { + var client *http.Client + if skipCertVerify { + client = &http.Client{ + Transport: insecureTransport, + } + } else { + client = &http.Client{} + } + return gooidc.ClientContext(ctx, client) +} diff --git a/src/core/controllers/base.go b/src/core/controllers/base.go index 5d239ad65..88c9a7ba5 100644 --- a/src/core/controllers/base.go +++ b/src/core/controllers/base.go @@ -35,6 +35,8 @@ import ( "github.com/goharbor/harbor/src/core/config" ) +const userKey = "user" + // CommonController handles request from UI that doesn't expect a page, such as /SwitchLanguage /logout ... type CommonController struct { beego.Controller @@ -69,7 +71,7 @@ func (cc *CommonController) Login() { if user == nil { cc.CustomAbort(http.StatusUnauthorized, "") } - cc.SetSession("user", *user) + cc.SetSession(userKey, *user) } // LogOut Habor UI diff --git a/src/core/controllers/oidc.go b/src/core/controllers/oidc.go index be1c9ff84..b5d8ff4fa 100644 --- a/src/core/controllers/oidc.go +++ b/src/core/controllers/oidc.go @@ -21,6 +21,7 @@ import ( "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/utils" + "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/common/utils/oidc" "github.com/goharbor/harbor/src/core/api" "github.com/goharbor/harbor/src/core/config" @@ -31,12 +32,17 @@ import ( const idTokenKey = "oidc_id_token" const stateKey = "oidc_state" +const userInfoKey = "oidc_user_info" // OIDCController handles requests for OIDC login, callback and user onboard type OIDCController struct { api.BaseController } +type onboardReq struct { + Username string `json:"username"` +} + type oidcUserData struct { Issuer string `json:"iss"` Subject string `json:"sub"` @@ -94,16 +100,25 @@ func (oc *OIDCController) Callback() { oc.RenderFormatedError(http.StatusInternalServerError, err) return } - oc.SetSession(idTokenKey, string(ouDataStr)) - // TODO: check and trigger onboard popup or redirect user to project page - oc.Data["json"] = d - oc.ServeFormatted() + u, err := dao.GetUserBySubIss(d.Subject, d.Issuer) + if err != nil { + oc.RenderFormatedError(http.StatusInternalServerError, err) + return + } + if u == nil { + oc.SetSession(userInfoKey, string(ouDataStr)) + oc.Controller.Redirect("/oidc-onboard", http.StatusFound) + } else { + oc.SetSession(userKey, *u) + oc.Controller.Redirect("/", http.StatusFound) + } } // Onboard handles the request to onboard an user authenticated via OIDC provider func (oc *OIDCController) Onboard() { - - username := oc.GetString("username") + u := &onboardReq{} + oc.DecodeJSONReq(u) + username := u.Username if utils.IsIllegalLength(username, 1, 255) { oc.RenderFormatedError(http.StatusBadRequest, errors.New("username with illegal length")) return @@ -113,9 +128,14 @@ func (oc *OIDCController) Onboard() { return } - idTokenStr := oc.GetSession(idTokenKey) + userInfoStr, ok := oc.GetSession(userInfoKey).(string) + if !ok { + oc.RenderError(http.StatusBadRequest, "Failed to get OIDC user info from session") + return + } + log.Debugf("User info string: %s\n", userInfoStr) d := &oidcUserData{} - err := json.Unmarshal([]byte(idTokenStr.(string)), &d) + err := json.Unmarshal([]byte(userInfoStr), &d) if err != nil { oc.RenderFormatedError(http.StatusInternalServerError, err) return @@ -126,8 +146,8 @@ func (oc *OIDCController) Onboard() { Secret: utils.GenerateRandomString(), } - var email string - if d.Email == "" { + email := d.Email + if email == "" { email = utils.GenerateRandomString() + "@harbor.com" } user := models.User{ @@ -139,12 +159,14 @@ func (oc *OIDCController) Onboard() { err = dao.OnBoardOIDCUser(&user) if err != nil { if strings.Contains(err.Error(), dao.ErrDupUser.Error()) { - oc.RenderFormatedError(http.StatusConflict, err) + oc.RenderError(http.StatusConflict, "Duplicate username") return } oc.RenderFormatedError(http.StatusInternalServerError, err) + oc.DelSession(userInfoKey) return } - - oc.Controller.Redirect(config.GetPortalURL(), http.StatusMovedPermanently) + user.OIDCUserMeta = nil + oc.SetSession(userKey, user) + oc.DelSession(userInfoKey) } From 0a2343f5422ce6e832224238442d8294d540d24d Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Mon, 8 Apr 2019 19:37:30 +0800 Subject: [PATCH 2/4] Support secret for docker CLI As CLI does not support oauth flow, we'll use secret for help OIDC user to authenticate via CLI. Add column to store secret and token, and add code to support verify/refresh token associates with secret. Such that when the user is removed from OIDC provider the secret will no longer work. Signed-off-by: Daniel Jiang --- .../postgresql/0004_1.8.0_schema.up.sql | 8 ++ src/common/models/oidc_user.go | 10 +- src/common/utils/oidc/helper.go | 17 +++ src/common/utils/oidc/secret.go | 127 ++++++++++++++++++ src/core/api/user.go | 31 ++++- src/core/controllers/oidc.go | 44 +++++- src/core/filter/security.go | 43 +++++- 7 files changed, 271 insertions(+), 9 deletions(-) create mode 100644 src/common/utils/oidc/secret.go diff --git a/make/migrations/postgresql/0004_1.8.0_schema.up.sql b/make/migrations/postgresql/0004_1.8.0_schema.up.sql index 88e4bea5b..be77781e3 100644 --- a/make/migrations/postgresql/0004_1.8.0_schema.up.sql +++ b/make/migrations/postgresql/0004_1.8.0_schema.up.sql @@ -16,6 +16,9 @@ CREATE TRIGGER robot_update_time_at_modtime BEFORE UPDATE ON robot FOR EACH ROW CREATE TABLE oidc_user ( id SERIAL NOT NULL, user_id int NOT NULL, + /* + Encoded secret + */ secret varchar(255) NOT NULL, /* Subject and Issuer @@ -24,9 +27,14 @@ CREATE TABLE oidc_user ( The sub (subject) and iss (issuer) Claims, used together, are the only Claims that an RP can rely upon as a stable identifier for the End-User */ subiss varchar(255) NOT NULL, + /* + Encoded token + */ + token text, creation_time timestamp default CURRENT_TIMESTAMP, update_time timestamp default CURRENT_TIMESTAMP, PRIMARY KEY (id), + FOREIGN KEY (user_id) REFERENCES harbor_user(user_id), UNIQUE (subiss) ); diff --git a/src/common/models/oidc_user.go b/src/common/models/oidc_user.go index 5d6c66c35..e42ae3e54 100644 --- a/src/common/models/oidc_user.go +++ b/src/common/models/oidc_user.go @@ -6,10 +6,14 @@ import ( // OIDCUser ... type OIDCUser struct { - ID int64 `orm:"pk;auto;column(id)" json:"id"` - UserID int `orm:"column(user_id)" json:"user_id"` - Secret string `orm:"column(secret)" json:"secret"` + ID int64 `orm:"pk;auto;column(id)" json:"id"` + UserID int `orm:"column(user_id)" json:"user_id"` + // encrypted secret + Secret string `orm:"column(secret)" json:"-"` + // secret in plain text + PlainSecret string `orm:"-" json:"secret"` SubIss string `orm:"column(subiss)" json:"subiss"` + Token string `orm:"column(token)" json:"-"` CreationTime time.Time `orm:"column(creation_time);auto_now_add" json:"creation_time"` UpdateTime time.Time `orm:"column(update_time);auto_now" json:"update_time"` } diff --git a/src/common/utils/oidc/helper.go b/src/common/utils/oidc/helper.go index b32e224a7..9efe7cd32 100644 --- a/src/common/utils/oidc/helper.go +++ b/src/common/utils/oidc/helper.go @@ -193,3 +193,20 @@ func clientCtx(ctx context.Context, skipCertVerify bool) context.Context { } return gooidc.ClientContext(ctx, client) } + +// RefreshToken refreshes the token passed in parameter, and return the new token. +func RefreshToken(ctx context.Context, token *Token) (*Token, error) { + oauth, err := getOauthConf() + if err != nil { + log.Errorf("Failed to get OAuth configuration, error: %v", err) + return nil, err + } + setting := provider.setting.Load().(models.OIDCSetting) + ctx = clientCtx(ctx, setting.SkipCertVerify) + ts := oauth.TokenSource(ctx, token.Token) + t, err := ts.Token() + if err != nil { + return nil, err + } + return &Token{Token: t, IDToken: t.Extra("id_token").(string)}, nil +} diff --git a/src/common/utils/oidc/secret.go b/src/common/utils/oidc/secret.go new file mode 100644 index 000000000..188cf7a94 --- /dev/null +++ b/src/common/utils/oidc/secret.go @@ -0,0 +1,127 @@ +package oidc + +import ( + "context" + "encoding/json" + "fmt" + "github.com/goharbor/harbor/src/common/dao" + "github.com/goharbor/harbor/src/common/utils" + "github.com/goharbor/harbor/src/common/utils/log" + "github.com/goharbor/harbor/src/core/config" + "github.com/pkg/errors" + "sync" +) + +// SecretVerifyError wraps the different errors happened when verifying a secret for OIDC user. When seeing this error, +// the caller should consider this an authentication error. +type SecretVerifyError struct { + cause error +} + +func (se *SecretVerifyError) Error() string { + return fmt.Sprintf("failed to verify the secret: %v", se.cause) +} + +func verifyError(err error) error { + return &SecretVerifyError{err} +} + +// SecretManager is the interface for store and verify the secret +type SecretManager interface { + // SetSecret sets the secret and token based on the ID of the user, when setting the secret the user has to be + // onboarded to Harbor DB. + SetSecret(userID int, secret string, token *Token) error + // VerifySecret verifies the secret and the token associated with it, it refreshes the token in the DB if it's + // refreshed during the verification + VerifySecret(ctx context.Context, userID int, secret string) error +} + +type defaultManager struct { + sync.Mutex + key string +} + +var m SecretManager = &defaultManager{} + +func (dm *defaultManager) getEncryptKey() (string, error) { + if dm.key == "" { + dm.Lock() + defer dm.Unlock() + if dm.key == "" { + key, err := config.SecretKey() + if err != nil { + return "", err + } + dm.key = key + } + } + return dm.key, nil +} + +// SetSecret sets the secret and token based on the ID of the user, when setting the secret the user has to be +// onboarded to Harbor DB. +func (dm *defaultManager) SetSecret(userID int, secret string, token *Token) error { + key, err := dm.getEncryptKey() + if err != nil { + return fmt.Errorf("failed to load the key for encryption/decryption： %v", err) + } + oidcUser, err := dao.GetOIDCUserByUserID(userID) + if oidcUser == nil { + return fmt.Errorf("failed to get oidc user info, error: %v", err) + } + encSecret, _ := utils.ReversibleEncrypt(secret, key) + tb, _ := json.Marshal(token) + encToken, _ := utils.ReversibleEncrypt(string(tb), key) + oidcUser.Secret = encSecret + oidcUser.Token = encToken + return dao.UpdateOIDCUser(oidcUser) +} + +// VerifySecret verifies the secret and the token associated with it, it tries to update the token in the DB if it's +// refreshed during the verification +func (dm *defaultManager) VerifySecret(ctx context.Context, userID int, secret string) error { + key, err := dm.getEncryptKey() + if err != nil { + return fmt.Errorf("failed to load the key for encryption/decryption： %v", err) + } + oidcUser, err := dao.GetOIDCUserByUserID(userID) + if oidcUser == nil { + return fmt.Errorf("failed to get oidc user info, error: %v", err) + } + plainSecret, err := utils.ReversibleDecrypt(oidcUser.Secret, key) + if err != nil { + return fmt.Errorf("failed to decrypt secret from DB: %v", err) + } + if secret != plainSecret { + return verifyError(errors.New("secret mismatch")) + } + tokenStr, err := utils.ReversibleDecrypt(oidcUser.Token, key) + if err != nil { + return verifyError(err) + } + token := &Token{} + err = json.Unmarshal(([]byte)(tokenStr), token) + if err != nil { + return verifyError(err) + } + _, err = VerifyToken(ctx, token.IDToken) + if err == nil { + return nil + } + log.Infof("Failed to verify ID Token, error: %v, refreshing...", err) + t, err := RefreshToken(ctx, token) + if err != nil { + return verifyError(err) + } + err = dm.SetSecret(oidcUser.UserID, secret, t) + if err != nil { + log.Warningf("Failed to update the token in DB: %v, ignore this error.", err) + } + return nil +} + +// VerifySecret verifies the secret and the token associated with it, it tries to update the token in the DB if it's +// refreshed during the verification +func VerifySecret(ctx context.Context, userID int, secret string) error { + return m.VerifySecret(ctx, userID, secret) +} diff --git a/src/core/api/user.go b/src/core/api/user.go index 765ed9da6..86d70758c 100644 --- a/src/core/api/user.go +++ b/src/core/api/user.go @@ -118,12 +118,19 @@ func (ua *UserAPI) Get() { u, err := dao.GetUser(userQuery) if err != nil { log.Errorf("Error occurred in GetUser, error: %v", err) - ua.CustomAbort(http.StatusInternalServerError, "Internal error.") + ua.RenderFormatedError(http.StatusInternalServerError, err) + return } u.Password = "" if ua.userID == ua.currentUserID { u.HasAdminRole = ua.SecurityCtx.IsSysAdmin() } + o, err := ua.getOIDCUserInfo() + if err != nil { + ua.RenderFormatedError(http.StatusInternalServerError, err) + return + } + u.OIDCUserMeta = o ua.Data["json"] = u ua.ServeJSON() return @@ -429,6 +436,28 @@ func (ua *UserAPI) ListUserPermissions() { return } +func (ua *UserAPI) getOIDCUserInfo() (*models.OIDCUser, error) { + if ua.AuthMode != common.OIDCAuth { + return nil, nil + } + key, err := config.SecretKey() + if err != nil { + return nil, err + } + o, err := dao.GetOIDCUserByUserID(ua.userID) + if err != nil { + return nil, err + } + if len(o.Secret) > 0 { + p, err := utils.ReversibleDecrypt(o.Secret, key) + if err != nil { + return nil, err + } + o.PlainSecret = p + } + return o, nil +} + // modifiable returns whether the modify is allowed based on current auth mode and context func (ua *UserAPI) modifiable() bool { if ua.AuthMode == common.DBAuth { diff --git a/src/core/controllers/oidc.go b/src/core/controllers/oidc.go index b5d8ff4fa..31cabad06 100644 --- a/src/core/controllers/oidc.go +++ b/src/core/controllers/oidc.go @@ -30,7 +30,7 @@ import ( "strings" ) -const idTokenKey = "oidc_id_token" +const tokenKey = "oidc_token" const stateKey = "oidc_state" const userInfoKey = "oidc_user_info" @@ -105,6 +105,13 @@ func (oc *OIDCController) Callback() { oc.RenderFormatedError(http.StatusInternalServerError, err) return } + tokenBytes, err := json.Marshal(token) + if err != nil { + oc.RenderFormatedError(http.StatusInternalServerError, err) + + } + oc.SetSession(tokenKey, tokenBytes) + if u == nil { oc.SetSession(userInfoKey, string(ouDataStr)) oc.Controller.Redirect("/oidc-onboard", http.StatusFound) @@ -112,6 +119,7 @@ func (oc *OIDCController) Callback() { oc.SetSession(userKey, *u) oc.Controller.Redirect("/", http.StatusFound) } + } // Onboard handles the request to onboard an user authenticated via OIDC provider @@ -134,16 +142,26 @@ func (oc *OIDCController) Onboard() { return } log.Debugf("User info string: %s\n", userInfoStr) + tb, ok := oc.GetSession(tokenKey).([]byte) + if !ok { + oc.RenderError(http.StatusBadRequest, "Failed to get OIDC token from session") + return + } + s, t, err := secretAndToken(tb) + if err != nil { + oc.RenderFormatedError(http.StatusInternalServerError, err) + return + } d := &oidcUserData{} - err := json.Unmarshal([]byte(userInfoStr), &d) + err = json.Unmarshal([]byte(userInfoStr), &d) if err != nil { oc.RenderFormatedError(http.StatusInternalServerError, err) return } oidcUser := models.OIDCUser{ SubIss: d.Subject + d.Issuer, - // TODO: get secret with secret manager. - Secret: utils.GenerateRandomString(), + Secret: s, + Token: t, } email := d.Email @@ -166,7 +184,25 @@ func (oc *OIDCController) Onboard() { oc.DelSession(userInfoKey) return } + user.OIDCUserMeta = nil oc.SetSession(userKey, user) oc.DelSession(userInfoKey) } + +func secretAndToken(tokenBytes []byte) (string, string, error) { + key, err := config.SecretKey() + if err != nil { + return "", "", err + } + token, err := utils.ReversibleEncrypt((string)(tokenBytes), key) + if err != nil { + return "", "", err + } + str := utils.GenerateRandomString() + secret, err := utils.ReversibleEncrypt(str, key) + if err != nil { + return "", "", err + } + return secret, token, nil +} diff --git a/src/core/filter/security.go b/src/core/filter/security.go index 9c95182fe..c3a47a8a2 100644 --- a/src/core/filter/security.go +++ b/src/core/filter/security.go @@ -17,6 +17,7 @@ package filter import ( "context" "fmt" + "github.com/goharbor/harbor/src/common/utils/oidc" "net/http" "regexp" @@ -110,6 +111,7 @@ func Init() { // standalone reqCtxModifiers = []ReqCtxModifier{ &secretReqCtxModifier{config.SecretStore}, + &oidcCliReqCtxModifier{}, &authProxyReqCtxModifier{}, &robotAuthReqCtxModifier{}, &basicAuthReqCtxModifier{}, @@ -205,6 +207,46 @@ func (r *robotAuthReqCtxModifier) Modify(ctx *beegoctx.Context) bool { return true } +type oidcCliReqCtxModifier struct{} + +func (oc *oidcCliReqCtxModifier) Modify(ctx *beegoctx.Context) bool { + path := ctx.Request.URL.Path + if path != "/service/token" || strings.HasPrefix(path, "/chartrepo/") { + log.Debug("OIDC CLI modifer only handles request by docker CLI or helm CLI") + return false + } + authMode, err := config.AuthMode() + if err != nil { + log.Errorf("fail to get auth mode, %v", err) + return false + } + if authMode != common.OIDCAuth { + return false + } + username, secret, ok := ctx.Request.BasicAuth() + if !ok { + return false + } + + user, err := dao.GetUser(models.User{ + Username: username, + }) + if user == nil { + if err != nil { + log.Errorf("Failed to get user: %v", err) + } + return false + } + if err := oidc.VerifySecret(ctx.Request.Context(), user.UserID, secret); err != nil { + log.Errorf("Failed to verify secret: %v", err) + return false + } + pm := config.GlobalProjectMgr + sc := local.NewSecurityContext(user, pm) + setSecurCtxAndPM(ctx.Request, sc, pm) + return true +} + type authProxyReqCtxModifier struct{} func (ap *authProxyReqCtxModifier) Modify(ctx *beegoctx.Context) bool { @@ -307,7 +349,6 @@ func (ap *authProxyReqCtxModifier) Modify(ctx *beegoctx.Context) bool { return false } - log.Debug("using local database project manager") pm := config.GlobalProjectMgr log.Debug("creating local database security context for auth proxy...") securCtx := local.NewSecurityContext(user, pm) From 0d18e6c82f9f70a4cb25ff18f7f24bf0fa99c0df Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Wed, 10 Apr 2019 17:05:14 +0800 Subject: [PATCH 3/4] Update according to comments For more context see PR #7335 Signed-off-by: Daniel Jiang --- src/common/utils/oidc/secret.go | 11 +++++++---- src/core/api/user.go | 15 +++++++-------- src/core/controllers/oidc.go | 2 +- src/core/filter/security.go | 7 ++++--- 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/common/utils/oidc/secret.go b/src/common/utils/oidc/secret.go index 188cf7a94..d98037d73 100644 --- a/src/common/utils/oidc/secret.go +++ b/src/common/utils/oidc/secret.go @@ -80,14 +80,17 @@ func (dm *defaultManager) SetSecret(userID int, secret string, token *Token) err // VerifySecret verifies the secret and the token associated with it, it tries to update the token in the DB if it's // refreshed during the verification func (dm *defaultManager) VerifySecret(ctx context.Context, userID int, secret string) error { + oidcUser, err := dao.GetOIDCUserByUserID(userID) + if err != nil { + return fmt.Errorf("failed to get oidc user info, error: %v", err) + } + if oidcUser == nil { + return fmt.Errorf("user is not onboarded as OIDC user") + } key, err := dm.getEncryptKey() if err != nil { return fmt.Errorf("failed to load the key for encryption/decryption： %v", err) } - oidcUser, err := dao.GetOIDCUserByUserID(userID) - if oidcUser == nil { - return fmt.Errorf("failed to get oidc user info, error: %v", err) - } plainSecret, err := utils.ReversibleDecrypt(oidcUser.Secret, key) if err != nil { return fmt.Errorf("failed to decrypt secret from DB: %v", err) diff --git a/src/core/api/user.go b/src/core/api/user.go index 86d70758c..c9a76f09d 100644 --- a/src/core/api/user.go +++ b/src/core/api/user.go @@ -125,12 +125,14 @@ func (ua *UserAPI) Get() { if ua.userID == ua.currentUserID { u.HasAdminRole = ua.SecurityCtx.IsSysAdmin() } - o, err := ua.getOIDCUserInfo() - if err != nil { - ua.RenderFormatedError(http.StatusInternalServerError, err) - return + if ua.AuthMode == common.OIDCAuth { + o, err := ua.getOIDCUserInfo() + if err != nil { + ua.RenderFormatedError(http.StatusInternalServerError, err) + return + } + u.OIDCUserMeta = o } - u.OIDCUserMeta = o ua.Data["json"] = u ua.ServeJSON() return @@ -437,9 +439,6 @@ func (ua *UserAPI) ListUserPermissions() { } func (ua *UserAPI) getOIDCUserInfo() (*models.OIDCUser, error) { - if ua.AuthMode != common.OIDCAuth { - return nil, nil - } key, err := config.SecretKey() if err != nil { return nil, err diff --git a/src/core/controllers/oidc.go b/src/core/controllers/oidc.go index 31cabad06..34fe1eb42 100644 --- a/src/core/controllers/oidc.go +++ b/src/core/controllers/oidc.go @@ -108,7 +108,7 @@ func (oc *OIDCController) Callback() { tokenBytes, err := json.Marshal(token) if err != nil { oc.RenderFormatedError(http.StatusInternalServerError, err) - + return } oc.SetSession(tokenKey, tokenBytes) diff --git a/src/core/filter/security.go b/src/core/filter/security.go index c3a47a8a2..b75df851d 100644 --- a/src/core/filter/security.go +++ b/src/core/filter/security.go @@ -231,10 +231,11 @@ func (oc *oidcCliReqCtxModifier) Modify(ctx *beegoctx.Context) bool { user, err := dao.GetUser(models.User{ Username: username, }) + if err != nil { + log.Errorf("Failed to get user: %v", err) + return false + } if user == nil { - if err != nil { - log.Errorf("Failed to get user: %v", err) - } return false } if err := oidc.VerifySecret(ctx.Request.Context(), user.UserID, secret); err != nil { From 763c5df0109ea860514e2a8c37958b8376cd7ae7 Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Wed, 10 Apr 2019 18:10:47 +0800 Subject: [PATCH 4/4] Add UT Signed-off-by: Daniel Jiang --- src/common/config/keyprovider.go | 10 ++++++ src/common/config/keyprovider_test.go | 10 ++++++ src/common/utils/oidc/helper_test.go | 4 ++- src/common/utils/oidc/secret_test.go | 32 +++++++++++++++++ src/common/utils/oidc/testutils.go | 26 ++++++++++++++ src/core/api/user.go | 2 +- src/core/config/config.go | 7 ++-- src/core/filter/security_test.go | 52 ++++++++++++++++++++++++++- 8 files changed, 138 insertions(+), 5 deletions(-) create mode 100644 src/common/utils/oidc/secret_test.go create mode 100644 src/common/utils/oidc/testutils.go diff --git a/src/common/config/keyprovider.go b/src/common/config/keyprovider.go index eac95612d..163ced369 100644 --- a/src/common/config/keyprovider.go +++ b/src/common/config/keyprovider.go @@ -46,3 +46,13 @@ func (f *FileKeyProvider) Get(params map[string]interface{}) (string, error) { } return string(b), nil } + +// PresetKeyProvider returns the preset key disregarding the parm, this is for testing only +type PresetKeyProvider struct { + Key string +} + +// Get ... +func (p *PresetKeyProvider) Get(params map[string]interface{}) (string, error) { + return p.Key, nil +} diff --git a/src/common/config/keyprovider_test.go b/src/common/config/keyprovider_test.go index 48127d903..c3a025ff2 100644 --- a/src/common/config/keyprovider_test.go +++ b/src/common/config/keyprovider_test.go @@ -15,6 +15,7 @@ package config import ( + "github.com/stretchr/testify/assert" "io/ioutil" "os" "testing" @@ -42,3 +43,12 @@ func TestGetOfFileKeyProvider(t *testing.T) { return } } + +func TestPresetKeyProvider(t *testing.T) { + kp := &PresetKeyProvider{ + Key: "mykey", + } + k, err := kp.Get(nil) + assert.Nil(t, err) + assert.Equal(t, "mykey", k) +} diff --git a/src/common/utils/oidc/helper_test.go b/src/common/utils/oidc/helper_test.go index 6e4f357e2..7ad3266d3 100644 --- a/src/common/utils/oidc/helper_test.go +++ b/src/common/utils/oidc/helper_test.go @@ -16,6 +16,7 @@ package oidc import ( "github.com/goharbor/harbor/src/common" + config2 "github.com/goharbor/harbor/src/common/config" "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/core/config" "github.com/stretchr/testify/assert" @@ -36,8 +37,9 @@ func TestMain(m *testing.M) { common.OIDCClientSecret: "secret", common.ExtEndpoint: "https://harbor.test", } + kp := &config2.PresetKeyProvider{Key: "naa4JtarA1Zsc3uY"} - config.InitWithSettings(conf) + config.InitWithSettings(conf, kp) result := m.Run() if result != 0 { diff --git a/src/common/utils/oidc/secret_test.go b/src/common/utils/oidc/secret_test.go new file mode 100644 index 000000000..1a376f54f --- /dev/null +++ b/src/common/utils/oidc/secret_test.go @@ -0,0 +1,32 @@ +package oidc + +import ( + "context" + "fmt" + "github.com/stretchr/testify/assert" + "testing" +) + +func TestSecretVerifyError(t *testing.T) { + sve := &SecretVerifyError{cause: fmt.Errorf("myerror")} + assert.Equal(t, "failed to verify the secret: myerror", sve.Error()) + err := verifyError(fmt.Errorf("myerror")) + assert.Equal(t, sve, err) +} + +func TestDefaultManagerGetEncryptKey(t *testing.T) { + d := &defaultManager{} + k, err := d.getEncryptKey() + assert.Nil(t, err) + assert.Equal(t, "naa4JtarA1Zsc3uY", k) + d2 := &defaultManager{key: "oldkey"} + k2, err := d2.getEncryptKey() + assert.Nil(t, err) + assert.Equal(t, "oldkey", k2) +} + +func TestPkgVerifySecret(t *testing.T) { + SetHardcodeVerifierForTest("secret") + assert.Nil(t, VerifySecret(context.Background(), 1, "secret")) + assert.NotNil(t, VerifySecret(context.Background(), 1, "not-the-secret")) +} diff --git a/src/common/utils/oidc/testutils.go b/src/common/utils/oidc/testutils.go new file mode 100644 index 000000000..b8ceb624b --- /dev/null +++ b/src/common/utils/oidc/testutils.go @@ -0,0 +1,26 @@ +package oidc + +import "context" +import "errors" + +// This is for testing only +type fakeVerifier struct { + secret string +} + +func (fv *fakeVerifier) SetSecret(uid int, s string, t *Token) error { + return nil +} + +func (fv *fakeVerifier) VerifySecret(ctx context.Context, userID int, secret string) error { + if secret != fv.secret { + return verifyError(errors.New("mismatch")) + } + return nil +} + +// SetHardcodeVerifierForTest overwrite the default secret manager for testing. +// Be reminded this is for testing only. +func SetHardcodeVerifierForTest(s string) { + m = &fakeVerifier{s} +} diff --git a/src/core/api/user.go b/src/core/api/user.go index c9a76f09d..5f1a1c806 100644 --- a/src/core/api/user.go +++ b/src/core/api/user.go @@ -444,7 +444,7 @@ func (ua *UserAPI) getOIDCUserInfo() (*models.OIDCUser, error) { return nil, err } o, err := dao.GetOIDCUserByUserID(ua.userID) - if err != nil { + if err != nil || o == nil { return nil, err } if len(o.Secret) > 0 { diff --git a/src/core/config/config.go b/src/core/config/config.go index 0b6049091..d5259e209 100644 --- a/src/core/config/config.go +++ b/src/core/config/config.go @@ -80,11 +80,14 @@ func Init() error { return nil } -// InitWithSettings init config with predefined configs -func InitWithSettings(cfgs map[string]interface{}) { +// InitWithSettings init config with predefined configs, and optionally overwrite the keyprovider +func InitWithSettings(cfgs map[string]interface{}, kp ...comcfg.KeyProvider) { Init() cfgMgr = comcfg.NewInMemoryManager() cfgMgr.UpdateConfig(cfgs) + if len(kp) > 0 { + keyProvider = kp[0] + } } func initKeyProvider() { diff --git a/src/core/filter/security_test.go b/src/core/filter/security_test.go index 91e2d31a7..bb18f901f 100644 --- a/src/core/filter/security_test.go +++ b/src/core/filter/security_test.go @@ -16,6 +16,8 @@ package filter import ( "context" + "github.com/goharbor/harbor/src/common/utils/oidc" + "github.com/stretchr/testify/require" "log" "net/http" "net/http/httptest" @@ -28,6 +30,7 @@ import ( "github.com/astaxie/beego" beegoctx "github.com/astaxie/beego/context" "github.com/astaxie/beego/session" + config2 "github.com/goharbor/harbor/src/common/config" "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" commonsecret "github.com/goharbor/harbor/src/common/secret" @@ -118,6 +121,53 @@ func TestSecretReqCtxModifier(t *testing.T) { assert.NotNil(t, projectManager(ctx)) } +func TestOIDCCliReqCtxModifier(t *testing.T) { + conf := map[string]interface{}{ + common.AUTHMode: common.OIDCAuth, + common.OIDCName: "test", + common.OIDCEndpoint: "https://accounts.google.com", + common.OIDCSkipCertVerify: "false", + common.OIDCScope: "openid, profile, offline_access", + common.OIDCCLientID: "client", + common.OIDCClientSecret: "secret", + common.ExtEndpoint: "https://harbor.test", + } + + kp := &config2.PresetKeyProvider{Key: "naa4JtarA1Zsc3uY"} + config.InitWithSettings(conf, kp) + + modifier := &oidcCliReqCtxModifier{} + req1, err := http.NewRequest(http.MethodGet, + "http://127.0.0.1/api/projects/", nil) + require.Nil(t, err) + ctx1, err := newContext(req1) + require.Nil(t, err) + assert.False(t, modifier.Modify(ctx1)) + req2, err := http.NewRequest(http.MethodGet, "http://127.0.0.1/service/token", nil) + require.Nil(t, err) + ctx2, err := newContext(req2) + require.Nil(t, err) + assert.False(t, modifier.Modify(ctx2)) + username := "oidcModiferTester" + password := "oidcSecret" + u := &models.User{ + Username: username, + Email: "testtest@test.org", + Password: "12345678", + } + id, err := dao.Register(*u) + require.Nil(t, err) + oidc.SetHardcodeVerifierForTest(password) + req3, err := http.NewRequest(http.MethodGet, "http://127.0.0.1/service/token", nil) + require.Nil(t, err) + req3.SetBasicAuth(username, password) + ctx3, err := newContext(req3) + assert.True(t, modifier.Modify(ctx3)) + o := dao.GetOrmer() + _, err = o.Delete(&models.User{UserID: int(id)}) + assert.Nil(t, err) +} + func TestRobotReqCtxModifier(t *testing.T) { req, err := http.NewRequest(http.MethodGet, "http://127.0.0.1/api/projects/", nil) @@ -135,7 +185,7 @@ func TestRobotReqCtxModifier(t *testing.T) { assert.False(t, modified) } -func TestAutoProxyReqCtxModifier(t *testing.T) { +func TestAuthProxyReqCtxModifier(t *testing.T) { server, err := fiter_test.NewAuthProxyTestServer() assert.Nil(t, err)