From 22b4ea0f8916bedfddb231d4c29d01fae4b1df30 Mon Sep 17 00:00:00 2001 From: wang yan Date: Fri, 18 Oct 2019 10:42:56 +0800 Subject: [PATCH 1/5] Enable robot account bypass policy check 1, the commit is for internal robot to bypass policy check, like vul and signature checking. 2, add a bool attribute into registry token, decode it in the harbor core and add the status into request context. 3, add a bool attribut for robot API controller, but API will not use it.y Signed-off-by: wang yan --- src/common/security/admiral/context.go | 5 ++ src/common/security/context.go | 2 + src/common/security/local/context.go | 5 ++ src/common/security/robot/context.go | 21 +++-- src/common/security/robot/context_test.go | 24 ++--- src/common/security/secret/context.go | 5 ++ src/common/token/htoken.go | 87 ------------------- src/common/utils/notary/helper.go | 2 +- .../utils/registry/auth/tokenauthorizer.go | 2 +- src/core/api/robot.go | 1 + src/core/filter/security.go | 13 +-- src/core/middlewares/chain.go | 2 + src/core/middlewares/config.go | 3 +- src/core/middlewares/contenttrust/handler.go | 6 ++ src/core/middlewares/regtoken/handler.go | 55 ++++++++++++ src/core/middlewares/util/util.go | 2 + src/core/middlewares/vulnerable/handler.go | 7 ++ src/core/service/token/authutils.go | 33 ++++--- src/core/service/token/creator.go | 2 +- src/core/service/token/token_test.go | 9 +- src/pkg/robot/controller.go | 21 ++++- src/pkg/robot/model/robot.go | 1 + src/pkg/token/claim/registry.go | 22 +++++ .../claims.go => pkg/token/claim/robot.go} | 15 ++-- .../token/claim/robot_test.go} | 10 +-- .../token/option_test.go} | 4 +- src/{common => pkg}/token/options.go | 40 ++++----- src/pkg/token/token.go | 77 ++++++++++++++++ .../token/token_test.go} | 26 +++++- 29 files changed, 331 insertions(+), 171 deletions(-) delete mode 100644 src/common/token/htoken.go create mode 100644 src/core/middlewares/regtoken/handler.go create mode 100644 src/pkg/token/claim/registry.go rename src/{common/token/claims.go => pkg/token/claim/robot.go} (63%) rename src/{common/token/claims_test.go => pkg/token/claim/robot_test.go} (90%) rename src/{common/token/options_test.go => pkg/token/option_test.go} (86%) rename src/{common => pkg}/token/options.go (83%) create mode 100644 src/pkg/token/token.go rename src/{common/token/htoken_test.go => pkg/token/token_test.go} (80%) diff --git a/src/common/security/admiral/context.go b/src/common/security/admiral/context.go index fcc0a069f..b4e19dbda 100644 --- a/src/common/security/admiral/context.go +++ b/src/common/security/admiral/context.go @@ -64,6 +64,11 @@ func (s *SecurityContext) IsSysAdmin() bool { return s.ctx.IsSysAdmin() } +// PolicyCheck ... +func (s *SecurityContext) PolicyCheck() bool { + return true +} + // IsSolutionUser ... func (s *SecurityContext) IsSolutionUser() bool { return false diff --git a/src/common/security/context.go b/src/common/security/context.go index 4b879a218..2de8b7ed5 100644 --- a/src/common/security/context.go +++ b/src/common/security/context.go @@ -27,6 +27,8 @@ type Context interface { GetUsername() string // IsSysAdmin returns whether the user is system admin IsSysAdmin() bool + // PolicyCheck returns whether the middlerwares apply to the request + PolicyCheck() bool // IsSolutionUser returns whether the user is solution user IsSolutionUser() bool // Get current user's all project diff --git a/src/common/security/local/context.go b/src/common/security/local/context.go index a80130da8..56697f731 100644 --- a/src/common/security/local/context.go +++ b/src/common/security/local/context.go @@ -61,6 +61,11 @@ func (s *SecurityContext) IsSysAdmin() bool { return s.user.HasAdminRole } +// PolicyCheck ... +func (s *SecurityContext) PolicyCheck() bool { + return true +} + // IsSolutionUser ... func (s *SecurityContext) IsSolutionUser() bool { return false diff --git a/src/common/security/robot/context.go b/src/common/security/robot/context.go index 43ebf5921..c1a08fb29 100644 --- a/src/common/security/robot/context.go +++ b/src/common/security/robot/context.go @@ -23,17 +23,19 @@ import ( // SecurityContext implements security.Context interface based on database type SecurityContext struct { - robot *model.Robot - pm promgr.ProjectManager - policy []*rbac.Policy + robot *model.Robot + pm promgr.ProjectManager + policy []*rbac.Policy + polichCheck bool } // NewSecurityContext ... -func NewSecurityContext(robot *model.Robot, pm promgr.ProjectManager, policy []*rbac.Policy) *SecurityContext { +func NewSecurityContext(robot *model.Robot, pm promgr.ProjectManager, policy []*rbac.Policy, polichCheck bool) *SecurityContext { return &SecurityContext{ - robot: robot, - pm: pm, - policy: policy, + robot: robot, + pm: pm, + policy: policy, + polichCheck: polichCheck, } } @@ -56,6 +58,11 @@ func (s *SecurityContext) IsSysAdmin() bool { return false } +// PolicyCheck ... +func (s *SecurityContext) PolicyCheck() bool { + return s.polichCheck +} + // IsSolutionUser robot cannot be a system admin func (s *SecurityContext) IsSolutionUser() bool { return false diff --git a/src/common/security/robot/context_test.go b/src/common/security/robot/context_test.go index a16cd40fe..004a1acb6 100644 --- a/src/common/security/robot/context_test.go +++ b/src/common/security/robot/context_test.go @@ -93,45 +93,45 @@ func TestMain(m *testing.M) { func TestIsAuthenticated(t *testing.T) { // unauthenticated - ctx := NewSecurityContext(nil, nil, nil) + ctx := NewSecurityContext(nil, nil, nil, true) assert.False(t, ctx.IsAuthenticated()) // authenticated ctx = NewSecurityContext(&model.Robot{ Name: "test", Disabled: false, - }, nil, nil) + }, nil, nil, true) assert.True(t, ctx.IsAuthenticated()) } func TestGetUsername(t *testing.T) { // unauthenticated - ctx := NewSecurityContext(nil, nil, nil) + ctx := NewSecurityContext(nil, nil, nil, true) assert.Equal(t, "", ctx.GetUsername()) // authenticated ctx = NewSecurityContext(&model.Robot{ Name: "test", Disabled: false, - }, nil, nil) + }, nil, nil, true) assert.Equal(t, "test", ctx.GetUsername()) } func TestIsSysAdmin(t *testing.T) { // unauthenticated - ctx := NewSecurityContext(nil, nil, nil) + ctx := NewSecurityContext(nil, nil, nil, true) assert.False(t, ctx.IsSysAdmin()) // authenticated, non admin ctx = NewSecurityContext(&model.Robot{ Name: "test", Disabled: false, - }, nil, nil) + }, nil, nil, true) assert.False(t, ctx.IsSysAdmin()) } func TestIsSolutionUser(t *testing.T) { - ctx := NewSecurityContext(nil, nil, nil) + ctx := NewSecurityContext(nil, nil, nil, true) assert.False(t, ctx.IsSolutionUser()) } @@ -147,7 +147,7 @@ func TestHasPullPerm(t *testing.T) { Description: "desc", } - ctx := NewSecurityContext(robot, pm, policies) + ctx := NewSecurityContext(robot, pm, policies, true) resource := rbac.NewProjectNamespace(private.ProjectID).Resource(rbac.ResourceRepository) assert.True(t, ctx.Can(rbac.ActionPull, resource)) } @@ -164,7 +164,7 @@ func TestHasPushPerm(t *testing.T) { Description: "desc", } - ctx := NewSecurityContext(robot, pm, policies) + ctx := NewSecurityContext(robot, pm, policies, true) resource := rbac.NewProjectNamespace(private.ProjectID).Resource(rbac.ResourceRepository) assert.True(t, ctx.Can(rbac.ActionPush, resource)) } @@ -185,20 +185,20 @@ func TestHasPushPullPerm(t *testing.T) { Description: "desc", } - ctx := NewSecurityContext(robot, pm, policies) + ctx := NewSecurityContext(robot, pm, policies, true) resource := rbac.NewProjectNamespace(private.ProjectID).Resource(rbac.ResourceRepository) assert.True(t, ctx.Can(rbac.ActionPush, resource) && ctx.Can(rbac.ActionPull, resource)) } func TestGetMyProjects(t *testing.T) { - ctx := NewSecurityContext(nil, nil, nil) + ctx := NewSecurityContext(nil, nil, nil, true) projects, err := ctx.GetMyProjects() require.Nil(t, err) assert.Nil(t, projects) } func TestGetProjectRoles(t *testing.T) { - ctx := NewSecurityContext(nil, nil, nil) + ctx := NewSecurityContext(nil, nil, nil, true) roles := ctx.GetProjectRoles("test") assert.Nil(t, roles) } diff --git a/src/common/security/secret/context.go b/src/common/security/secret/context.go index 5dc06137a..3482d2aa9 100644 --- a/src/common/security/secret/context.go +++ b/src/common/security/secret/context.go @@ -66,6 +66,11 @@ func (s *SecurityContext) IsSysAdmin() bool { return false } +// PolicyCheck ... +func (s *SecurityContext) PolicyCheck() bool { + return true +} + // IsSolutionUser ... func (s *SecurityContext) IsSolutionUser() bool { return s.IsAuthenticated() diff --git a/src/common/token/htoken.go b/src/common/token/htoken.go deleted file mode 100644 index 897c50467..000000000 --- a/src/common/token/htoken.go +++ /dev/null @@ -1,87 +0,0 @@ -package token - -import ( - "crypto/ecdsa" - "crypto/rsa" - "errors" - "fmt" - "github.com/dgrijalva/jwt-go" - "github.com/goharbor/harbor/src/common/rbac" - "github.com/goharbor/harbor/src/common/utils/log" - "time" -) - -// HToken htoken is a jwt token for harbor robot account, -// which contains the robot ID, project ID and the access permission for the project. -// It used for authn/authz for robot account in Harbor. -type HToken struct { - jwt.Token -} - -// New ... -func New(tokenID, projectID, expiresAt int64, access []*rbac.Policy) (*HToken, error) { - rClaims := &RobotClaims{ - TokenID: tokenID, - ProjectID: projectID, - Access: access, - StandardClaims: jwt.StandardClaims{ - IssuedAt: time.Now().UTC().Unix(), - ExpiresAt: expiresAt, - Issuer: DefaultOptions().Issuer, - }, - } - err := rClaims.Valid() - if err != nil { - return nil, err - } - return &HToken{ - Token: *jwt.NewWithClaims(DefaultOptions().SignMethod, rClaims), - }, nil -} - -// Raw get the Raw string of token -func (htk *HToken) Raw() (string, error) { - key, err := DefaultOptions().GetKey() - if err != nil { - return "", nil - } - raw, err := htk.Token.SignedString(key) - if err != nil { - log.Debugf(fmt.Sprintf("failed to issue token %v", err)) - return "", err - } - return raw, err -} - -// ParseWithClaims ... -func ParseWithClaims(rawToken string, claims jwt.Claims) (*HToken, error) { - key, err := DefaultOptions().GetKey() - if err != nil { - return nil, err - } - token, err := jwt.ParseWithClaims(rawToken, claims, func(token *jwt.Token) (interface{}, error) { - if token.Method.Alg() != DefaultOptions().SignMethod.Alg() { - return nil, errors.New("invalid signing method") - } - switch k := key.(type) { - case *rsa.PrivateKey: - return &k.PublicKey, nil - case *ecdsa.PrivateKey: - return &k.PublicKey, nil - default: - return key, nil - } - }) - if err != nil { - log.Errorf(fmt.Sprintf("parse token error, %v", err)) - return nil, err - } - - if !token.Valid { - log.Errorf(fmt.Sprintf("invalid jwt token, %v", token)) - return nil, errors.New("invalid jwt token") - } - return &HToken{ - Token: *token, - }, nil -} diff --git a/src/common/utils/notary/helper.go b/src/common/utils/notary/helper.go index db80a9450..781ef793a 100644 --- a/src/common/utils/notary/helper.go +++ b/src/common/utils/notary/helper.go @@ -76,7 +76,7 @@ func GetTargets(notaryEndpoint string, username string, fqRepo string) ([]model. Type: "repository", Name: fqRepo, Actions: []string{"pull"}, - }}) + }}, true) if err != nil { return nil, err } diff --git a/src/common/utils/registry/auth/tokenauthorizer.go b/src/common/utils/registry/auth/tokenauthorizer.go index 1f1c95569..c2c5fcbd4 100644 --- a/src/common/utils/registry/auth/tokenauthorizer.go +++ b/src/common/utils/registry/auth/tokenauthorizer.go @@ -346,7 +346,7 @@ type rawTokenGenerator struct { // generate token directly func (r *rawTokenGenerator) generate(scopes []*token.ResourceActions, endpoint string) (*models.Token, error) { - return token_util.MakeToken(r.username, r.service, scopes) + return token_util.MakeToken(r.username, r.service, scopes, true) } func buildPingURL(endpoint string) string { diff --git a/src/core/api/robot.go b/src/core/api/robot.go index ded3a8d58..0304e4b54 100644 --- a/src/core/api/robot.go +++ b/src/core/api/robot.go @@ -108,6 +108,7 @@ func (r *RobotAPI) Post() { return } robotReq.Visible = true + robotReq.PolicyCheck = true robotReq.ProjectID = r.project.ProjectID if err := validateRobotReq(r.project, &robotReq); err != nil { diff --git a/src/core/filter/security.go b/src/core/filter/security.go index b8bbbac5b..9f2cdb620 100644 --- a/src/core/filter/security.go +++ b/src/core/filter/security.go @@ -34,7 +34,6 @@ import ( "github.com/goharbor/harbor/src/common/security/local" robotCtx "github.com/goharbor/harbor/src/common/security/robot" "github.com/goharbor/harbor/src/common/security/secret" - "github.com/goharbor/harbor/src/common/token" "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/core/auth" "github.com/goharbor/harbor/src/core/config" @@ -44,6 +43,8 @@ import ( "github.com/goharbor/harbor/src/pkg/authproxy" "github.com/goharbor/harbor/src/pkg/robot" + pkg_token "github.com/goharbor/harbor/src/pkg/token" + "github.com/goharbor/harbor/src/pkg/token/claim" ) // ContextValueKey for content value @@ -188,15 +189,17 @@ func (r *robotAuthReqCtxModifier) Modify(ctx *beegoctx.Context) bool { if !strings.HasPrefix(robotName, common.RobotPrefix) { return false } - rClaims := &token.RobotClaims{} - htk, err := token.ParseWithClaims(robotTk, rClaims) + rClaims := &claim.Robot{} + opt := pkg_token.DefaultTokenOptions() + rtk, err := pkg_token.Parse(opt, robotTk, rClaims) if err != nil { log.Errorf("failed to decrypt robot token, %v", err) return false } + // Do authn for robot account, as Harbor only stores the token ID, just validate the ID and disable. ctr := robot.RobotCtr - robot, err := ctr.GetRobotAccount(htk.Claims.(*token.RobotClaims).TokenID) + robot, err := ctr.GetRobotAccount(rtk.Claims.(*claim.Robot).TokenID) if err != nil { log.Errorf("failed to get robot %s: %v", robotName, err) return false @@ -215,7 +218,7 @@ func (r *robotAuthReqCtxModifier) Modify(ctx *beegoctx.Context) bool { } log.Debug("creating robot account security context...") pm := config.GlobalProjectMgr - securCtx := robotCtx.NewSecurityContext(robot, pm, htk.Claims.(*token.RobotClaims).Access) + securCtx := robotCtx.NewSecurityContext(robot, pm, rtk.Claims.(*claim.Robot).Access, rClaims.PolicyCheck) setSecurCtxAndPM(ctx.Request, securCtx, pm) return true } diff --git a/src/core/middlewares/chain.go b/src/core/middlewares/chain.go index 79617c735..630a8e8d4 100644 --- a/src/core/middlewares/chain.go +++ b/src/core/middlewares/chain.go @@ -25,6 +25,7 @@ import ( "github.com/goharbor/harbor/src/core/middlewares/listrepo" "github.com/goharbor/harbor/src/core/middlewares/multiplmanifest" "github.com/goharbor/harbor/src/core/middlewares/readonly" + "github.com/goharbor/harbor/src/core/middlewares/regtoken" "github.com/goharbor/harbor/src/core/middlewares/sizequota" "github.com/goharbor/harbor/src/core/middlewares/url" "github.com/goharbor/harbor/src/core/middlewares/vulnerable" @@ -72,6 +73,7 @@ func (b *DefaultCreator) geMiddleware(mName string) alice.Constructor { SIZEQUOTA: func(next http.Handler) http.Handler { return sizequota.New(next) }, COUNTQUOTA: func(next http.Handler) http.Handler { return countquota.New(next) }, IMMUTABLE: func(next http.Handler) http.Handler { return immutable.New(next) }, + REGTOKEN: func(next http.Handler) http.Handler { return regtoken.New(next) }, } return middlewares[mName] } diff --git a/src/core/middlewares/config.go b/src/core/middlewares/config.go index fa2b536f5..d80489312 100644 --- a/src/core/middlewares/config.go +++ b/src/core/middlewares/config.go @@ -26,13 +26,14 @@ const ( SIZEQUOTA = "sizequota" COUNTQUOTA = "countquota" IMMUTABLE = "immutable" + REGTOKEN = "REGTOKEN" ) // ChartMiddlewares middlewares for chart server var ChartMiddlewares = []string{CHART} // Middlewares with sequential organization -var Middlewares = []string{READONLY, URL, MUITIPLEMANIFEST, LISTREPO, CONTENTTRUST, VULNERABLE, SIZEQUOTA, IMMUTABLE, COUNTQUOTA} +var Middlewares = []string{READONLY, URL, REGTOKEN, MUITIPLEMANIFEST, LISTREPO, CONTENTTRUST, VULNERABLE, SIZEQUOTA, IMMUTABLE, COUNTQUOTA} // MiddlewaresLocal ... var MiddlewaresLocal = []string{SIZEQUOTA, IMMUTABLE, COUNTQUOTA} diff --git a/src/core/middlewares/contenttrust/handler.go b/src/core/middlewares/contenttrust/handler.go index bcc4de44a..9dc6d1e69 100644 --- a/src/core/middlewares/contenttrust/handler.go +++ b/src/core/middlewares/contenttrust/handler.go @@ -49,6 +49,12 @@ func (cth contentTrustHandler) ServeHTTP(rw http.ResponseWriter, req *http.Reque cth.next.ServeHTTP(rw, req) return } + // Token bypass policy check + policyCheck := req.Context().Value(util.PolicyCheckCtxKey) + if policyCheck != nil && !policyCheck.(bool) { + cth.next.ServeHTTP(rw, req) + return + } if !util.GetPolicyChecker().ContentTrustEnabled(img.ProjectName) { cth.next.ServeHTTP(rw, req) return diff --git a/src/core/middlewares/regtoken/handler.go b/src/core/middlewares/regtoken/handler.go new file mode 100644 index 000000000..67c2c934a --- /dev/null +++ b/src/core/middlewares/regtoken/handler.go @@ -0,0 +1,55 @@ +package regtoken + +import ( + "context" + "github.com/goharbor/harbor/src/common/utils/log" + "github.com/goharbor/harbor/src/core/config" + "github.com/goharbor/harbor/src/core/middlewares/util" + pkg_token "github.com/goharbor/harbor/src/pkg/token" + "github.com/goharbor/harbor/src/pkg/token/claim" + "net/http" + "strings" +) + +type regTokenHandler struct { + next http.Handler +} + +// New ... +func New(next http.Handler) http.Handler { + return ®TokenHandler{ + next: next, + } +} + +// ServeHTTP ... +func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { + imgRaw := req.Context().Value(util.ImageInfoCtxKey) + if imgRaw == nil || !config.WithClair() { + r.next.ServeHTTP(rw, req) + return + } + img, _ := req.Context().Value(util.ImageInfoCtxKey).(util.ImageInfo) + if img.Digest == "" { + r.next.ServeHTTP(rw, req) + return + } + + parts := strings.Split(req.Header.Get("Authorization"), " ") + if len(parts) != 2 || strings.ToLower(parts[0]) != "bearer" { + r.next.ServeHTTP(rw, req) + return + } + rawToken := parts[1] + opt := pkg_token.DefaultTokenOptions() + rClaims := &claim.Registry{} + rtk, err := pkg_token.Parse(opt, rawToken, rClaims) + if err != nil { + log.Debug("failed to decode reg token: %v, the error is skipped and round the request to native registry.", err) + r.next.ServeHTTP(rw, req) + return + } + ctx := context.WithValue(req.Context(), util.PolicyCheckCtxKey, rtk.Claims.(*claim.Registry).PolicyCheck) + req = req.WithContext(ctx) + r.next.ServeHTTP(rw, req) +} diff --git a/src/core/middlewares/util/util.go b/src/core/middlewares/util/util.go index 9c05b7599..4997bf338 100644 --- a/src/core/middlewares/util/util.go +++ b/src/core/middlewares/util/util.go @@ -49,6 +49,8 @@ type contextKey string const ( // ImageInfoCtxKey the context key for image information ImageInfoCtxKey = contextKey("ImageInfo") + // PolicyCheckCtxKey the context key for image information + PolicyCheckCtxKey = contextKey("PolicyCheck") // TokenUsername ... // TODO: temp solution, remove after vmware/harbor#2242 is resolved. TokenUsername = "harbor-core" diff --git a/src/core/middlewares/vulnerable/handler.go b/src/core/middlewares/vulnerable/handler.go index a1ba6bda9..eaecbae5a 100644 --- a/src/core/middlewares/vulnerable/handler.go +++ b/src/core/middlewares/vulnerable/handler.go @@ -52,6 +52,13 @@ func (vh vulnerableHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) return } + // Token bypass policy check + policyCheck := req.Context().Value(util.PolicyCheckCtxKey) + if policyCheck != nil && !policyCheck.(bool) { + vh.next.ServeHTTP(rw, req) + return + } + // Is vulnerable policy set? projectVulnerableEnabled, projectVulnerableSeverity, wl := util.GetPolicyChecker().VulnerablePolicy(img.ProjectName) if !projectVulnerableEnabled { diff --git a/src/core/service/token/authutils.go b/src/core/service/token/authutils.go index 8de8a50ca..aac46c259 100644 --- a/src/core/service/token/authutils.go +++ b/src/core/service/token/authutils.go @@ -42,6 +42,12 @@ func init() { privateKey = config.TokenPrivateKeyPath() } +// ClaimSet ... +type ClaimSet struct { + token.ClaimSet + PolicyCheck bool `json:"policy_check"` +} + // GetResourceActions ... func GetResourceActions(scopes []string) []*token.ResourceActions { log.Debugf("scopes: %+v", scopes) @@ -100,7 +106,7 @@ func filterAccess(access []*token.ResourceActions, ctx security.Context, } // MakeToken makes a valid jwt token based on parms. -func MakeToken(username, service string, access []*token.ResourceActions) (*models.Token, error) { +func MakeToken(username, service string, access []*token.ResourceActions, policyCheck bool) (*models.Token, error) { pk, err := libtrust.LoadKeyFile(privateKey) if err != nil { return nil, err @@ -110,7 +116,7 @@ func MakeToken(username, service string, access []*token.ResourceActions) (*mode return nil, err } - tk, expiresIn, issuedAt, err := makeTokenCore(issuer, username, service, expiration, access, pk) + tk, expiresIn, issuedAt, err := makeTokenCore(issuer, username, service, expiration, access, pk, policyCheck) if err != nil { return nil, err } @@ -138,7 +144,7 @@ func permToActions(p string) []string { // make token core func makeTokenCore(issuer, subject, audience string, expiration int, - access []*token.ResourceActions, signingKey libtrust.PrivateKey) (t *token.Token, expiresIn int, issuedAt *time.Time, err error) { + access []*token.ResourceActions, signingKey libtrust.PrivateKey, policyCheck bool) (t *token.Token, expiresIn int, issuedAt *time.Time, err error) { joseHeader := &token.Header{ Type: "JWT", @@ -155,15 +161,18 @@ func makeTokenCore(issuer, subject, audience string, expiration int, issuedAt = &now expiresIn = expiration * 60 - claimSet := &token.ClaimSet{ - Issuer: issuer, - Subject: subject, - Audience: audience, - Expiration: now.Add(time.Duration(expiration) * time.Minute).Unix(), - NotBefore: now.Unix(), - IssuedAt: now.Unix(), - JWTID: jwtID, - Access: access, + claimSet := &ClaimSet{ + ClaimSet: token.ClaimSet{ + Issuer: issuer, + Subject: subject, + Audience: audience, + Expiration: now.Add(time.Duration(expiration) * time.Minute).Unix(), + NotBefore: now.Unix(), + IssuedAt: now.Unix(), + JWTID: jwtID, + Access: access, + }, + PolicyCheck: policyCheck, } var joseHeaderBytes, claimSetBytes []byte diff --git a/src/core/service/token/creator.go b/src/core/service/token/creator.go index feca191e6..6ca8036ae 100644 --- a/src/core/service/token/creator.go +++ b/src/core/service/token/creator.go @@ -222,7 +222,7 @@ func (g generalCreator) Create(r *http.Request) (*models.Token, error) { if err != nil { return nil, err } - return MakeToken(ctx.GetUsername(), g.service, access) + return MakeToken(ctx.GetUsername(), g.service, access, ctx.PolicyCheck()) } func parseScopes(u *url.URL) []string { diff --git a/src/core/service/token/token_test.go b/src/core/service/token/token_test.go index 4a8435c2c..5a722c23a 100644 --- a/src/core/service/token/token_test.go +++ b/src/core/service/token/token_test.go @@ -119,7 +119,8 @@ func getPublicKey(crtPath string) (*rsa.PublicKey, error) { type harborClaims struct { jwt.StandardClaims // Private claims - Access []*token.ResourceActions `json:"access"` + Access []*token.ResourceActions `json:"access"` + PolicyCheck bool `json:"policy_check"` } func TestMakeToken(t *testing.T) { @@ -133,7 +134,7 @@ func TestMakeToken(t *testing.T) { }} svc := "harbor-registry" u := "tester" - tokenJSON, err := MakeToken(u, svc, ra) + tokenJSON, err := MakeToken(u, svc, ra, true) if err != nil { t.Errorf("Error while making token: %v", err) } @@ -154,6 +155,7 @@ func TestMakeToken(t *testing.T) { t.Errorf("Error while parsing the token: %v", err) } claims := tok.Claims.(*harborClaims) + assert.True(t, claims.PolicyCheck) assert.Equal(t, *(claims.Access[0]), *(ra[0]), "Access mismatch") assert.Equal(t, claims.Audience, svc, "Audience mismatch") } @@ -242,6 +244,9 @@ func (f *fakeSecurityContext) IsSysAdmin() bool { func (f *fakeSecurityContext) IsSolutionUser() bool { return false } +func (f *fakeSecurityContext) PolicyCheck() bool { + return false +} func (f *fakeSecurityContext) Can(action rbac.Action, resource rbac.Resource) bool { return false } diff --git a/src/pkg/robot/controller.go b/src/pkg/robot/controller.go index 5a6565711..8d132fea6 100644 --- a/src/pkg/robot/controller.go +++ b/src/pkg/robot/controller.go @@ -2,12 +2,14 @@ package robot import ( "fmt" + "github.com/dgrijalva/jwt-go" "github.com/goharbor/harbor/src/common" - "github.com/goharbor/harbor/src/common/token" "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/core/config" "github.com/goharbor/harbor/src/pkg/q" "github.com/goharbor/harbor/src/pkg/robot/model" + "github.com/goharbor/harbor/src/pkg/token" + "github.com/goharbor/harbor/src/pkg/token/claim" "github.com/pkg/errors" "time" ) @@ -76,13 +78,24 @@ func (d *DefaultAPIController) CreateRobotAccount(robotReq *model.RobotCreate) ( // generate the token, and return it with response data. // token is not stored in the database. - jwtToken, err := token.New(id, robotReq.ProjectID, expiresAt, robotReq.Access) + opt := token.DefaultTokenOptions() + rClaims := &claim.Robot{ + TokenID: id, + ProjectID: robotReq.ProjectID, + Access: robotReq.Access, + PolicyCheck: robotReq.PolicyCheck, + StandardClaims: jwt.StandardClaims{ + IssuedAt: time.Now().UTC().Unix(), + ExpiresAt: expiresAt, + Issuer: opt.Issuer, + }, + } + tk, err := token.New(opt, rClaims) if err != nil { deferDel = err return nil, fmt.Errorf("failed to valid parameters to generate token for robot account, %v", err) } - - rawTk, err := jwtToken.Raw() + rawTk, err := tk.Raw() if err != nil { deferDel = err return nil, fmt.Errorf("failed to sign token for robot account, %v", err) diff --git a/src/pkg/robot/model/robot.go b/src/pkg/robot/model/robot.go index db911f11c..c5cf530c6 100644 --- a/src/pkg/robot/model/robot.go +++ b/src/pkg/robot/model/robot.go @@ -50,6 +50,7 @@ type RobotCreate struct { Description string `json:"description"` Disabled bool `json:"disabled"` Visible bool `json:"-"` + PolicyCheck bool `json:"-"` Access []*rbac.Policy `json:"access"` } diff --git a/src/pkg/token/claim/registry.go b/src/pkg/token/claim/registry.go new file mode 100644 index 000000000..a93e499a9 --- /dev/null +++ b/src/pkg/token/claim/registry.go @@ -0,0 +1,22 @@ +package claim + +import ( + "github.com/dgrijalva/jwt-go" + "github.com/docker/distribution/registry/auth/token" +) + +// Registry implements the interface of jwt.Claims +type Registry struct { + jwt.StandardClaims + PolicyCheck bool `json:"policy_check"` + Access []*token.ResourceActions `json:"access"` +} + +// Valid valid the standard claims +func (rc *Registry) Valid() error { + stdErr := rc.StandardClaims.Valid() + if stdErr != nil { + return stdErr + } + return nil +} diff --git a/src/common/token/claims.go b/src/pkg/token/claim/robot.go similarity index 63% rename from src/common/token/claims.go rename to src/pkg/token/claim/robot.go index bec9f5f2f..58a25a9de 100644 --- a/src/common/token/claims.go +++ b/src/pkg/token/claim/robot.go @@ -1,4 +1,4 @@ -package token +package claim import ( "errors" @@ -6,16 +6,17 @@ import ( "github.com/goharbor/harbor/src/common/rbac" ) -// RobotClaims implements the interface of jwt.Claims -type RobotClaims struct { +// Robot implements the interface of jwt.Claims +type Robot struct { jwt.StandardClaims - TokenID int64 `json:"id"` - ProjectID int64 `json:"pid"` - Access []*rbac.Policy `json:"access"` + TokenID int64 `json:"id"` + ProjectID int64 `json:"pid"` + PolicyCheck bool `json:"policy_check"` + Access []*rbac.Policy `json:"access"` } // Valid valid the claims "tokenID, projectID and access". -func (rc RobotClaims) Valid() error { +func (rc Robot) Valid() error { if rc.TokenID < 0 { return errors.New("Token id must an valid INT") } diff --git a/src/common/token/claims_test.go b/src/pkg/token/claim/robot_test.go similarity index 90% rename from src/common/token/claims_test.go rename to src/pkg/token/claim/robot_test.go index dc25a120a..50442f72b 100644 --- a/src/common/token/claims_test.go +++ b/src/pkg/token/claim/robot_test.go @@ -1,4 +1,4 @@ -package token +package claim import ( "github.com/goharbor/harbor/src/common/rbac" @@ -15,7 +15,7 @@ func TestValid(t *testing.T) { policies := []*rbac.Policy{} policies = append(policies, rbacPolicy) - rClaims := &RobotClaims{ + rClaims := &Robot{ TokenID: 1, ProjectID: 2, Access: policies, @@ -32,7 +32,7 @@ func TestUnValidTokenID(t *testing.T) { policies := []*rbac.Policy{} policies = append(policies, rbacPolicy) - rClaims := &RobotClaims{ + rClaims := &Robot{ TokenID: -1, ProjectID: 2, Access: policies, @@ -49,7 +49,7 @@ func TestUnValidProjectID(t *testing.T) { policies := []*rbac.Policy{} policies = append(policies, rbacPolicy) - rClaims := &RobotClaims{ + rClaims := &Robot{ TokenID: 1, ProjectID: -2, Access: policies, @@ -59,7 +59,7 @@ func TestUnValidProjectID(t *testing.T) { func TestUnValidPolicy(t *testing.T) { - rClaims := &RobotClaims{ + rClaims := &Robot{ TokenID: 1, ProjectID: 2, Access: nil, diff --git a/src/common/token/options_test.go b/src/pkg/token/option_test.go similarity index 86% rename from src/common/token/options_test.go rename to src/pkg/token/option_test.go index 5f64fb380..227cf688d 100644 --- a/src/common/token/options_test.go +++ b/src/pkg/token/option_test.go @@ -8,7 +8,7 @@ import ( ) func TestNewOptions(t *testing.T) { - defaultOpt := DefaultOptions() + defaultOpt := DefaultTokenOptions() assert.NotNil(t, defaultOpt) assert.Equal(t, defaultOpt.SignMethod, jwt.GetSigningMethod("RS256")) assert.Equal(t, defaultOpt.Issuer, "harbor-token-issuer") @@ -16,7 +16,7 @@ func TestNewOptions(t *testing.T) { } func TestGetKey(t *testing.T) { - defaultOpt := DefaultOptions() + defaultOpt := DefaultTokenOptions() key, err := defaultOpt.GetKey() assert.Nil(t, err) assert.NotNil(t, key) diff --git a/src/common/token/options.go b/src/pkg/token/options.go similarity index 83% rename from src/common/token/options.go rename to src/pkg/token/options.go index 747d7435d..e1d96d476 100644 --- a/src/common/token/options.go +++ b/src/pkg/token/options.go @@ -11,9 +11,9 @@ import ( ) const ( - ttl = 60 * time.Minute - issuer = "harbor-token-issuer" - signedMethod = "RS256" + defaultTTL = 60 * time.Minute + defaultIssuer = "harbor-token-defaultIssuer" + defaultSignedMethod = "RS256" ) // Options ... @@ -25,23 +25,6 @@ type Options struct { Issuer string } -// DefaultOptions ... -func DefaultOptions() *Options { - privateKeyFile := config.TokenPrivateKeyPath() - privateKey, err := ioutil.ReadFile(privateKeyFile) - if err != nil { - log.Errorf(fmt.Sprintf("failed to read private key %v", err)) - return nil - } - opt := &Options{ - SignMethod: jwt.GetSigningMethod(signedMethod), - PrivateKey: privateKey, - Issuer: issuer, - TTL: ttl, - } - return opt -} - // GetKey ... func (o *Options) GetKey() (interface{}, error) { var err error @@ -76,3 +59,20 @@ func (o *Options) GetKey() (interface{}, error) { return nil, fmt.Errorf(fmt.Sprintf("unsupported sign method, %s", o.SignMethod)) } } + +// DefaultTokenOptions ... +func DefaultTokenOptions() *Options { + privateKeyFile := config.TokenPrivateKeyPath() + privateKey, err := ioutil.ReadFile(privateKeyFile) + if err != nil { + log.Errorf(fmt.Sprintf("failed to read private key %v", err)) + return nil + } + opt := &Options{ + SignMethod: jwt.GetSigningMethod(defaultSignedMethod), + PrivateKey: privateKey, + Issuer: defaultIssuer, + TTL: defaultTTL, + } + return opt +} diff --git a/src/pkg/token/token.go b/src/pkg/token/token.go new file mode 100644 index 000000000..62e1e3fec --- /dev/null +++ b/src/pkg/token/token.go @@ -0,0 +1,77 @@ +package token + +import ( + "crypto/ecdsa" + "crypto/rsa" + "errors" + "fmt" + "github.com/dgrijalva/jwt-go" + "github.com/goharbor/harbor/src/common/utils/log" +) + +// Token is a jwt token for harbor robot account, +type Token struct { + jwt.Token + Opt *Options + Claim jwt.Claims +} + +// New ... +func New(opt *Options, claims jwt.Claims) (*Token, error) { + err := claims.Valid() + if err != nil { + return nil, err + } + return &Token{ + Token: *jwt.NewWithClaims(opt.SignMethod, claims), + Opt: opt, + Claim: claims, + }, nil +} + +// Raw get the Raw string of token +func (tk *Token) Raw() (string, error) { + key, err := tk.Opt.GetKey() + if err != nil { + return "", nil + } + raw, err := tk.Token.SignedString(key) + if err != nil { + log.Debugf(fmt.Sprintf("failed to issue token %v", err)) + return "", err + } + return raw, err +} + +// Parse ... +func Parse(opt *Options, rawToken string, claims jwt.Claims) (*Token, error) { + key, err := opt.GetKey() + if err != nil { + return nil, err + } + token, err := jwt.ParseWithClaims(rawToken, claims, func(token *jwt.Token) (interface{}, error) { + if token.Method.Alg() != opt.SignMethod.Alg() { + return nil, errors.New("invalid signing method") + } + switch k := key.(type) { + case *rsa.PrivateKey: + return &k.PublicKey, nil + case *ecdsa.PrivateKey: + return &k.PublicKey, nil + default: + return key, nil + } + }) + if err != nil { + log.Errorf(fmt.Sprintf("parse token error, %v", err)) + return nil, err + } + + if !token.Valid { + log.Errorf(fmt.Sprintf("invalid jwt token, %v", token)) + return nil, errors.New("invalid jwt token") + } + return &Token{ + Token: *token, + }, nil +} diff --git a/src/common/token/htoken_test.go b/src/pkg/token/token_test.go similarity index 80% rename from src/common/token/htoken_test.go rename to src/pkg/token/token_test.go index 38e187ef7..0dd42d4ff 100644 --- a/src/common/token/htoken_test.go +++ b/src/pkg/token/token_test.go @@ -5,8 +5,10 @@ import ( "testing" "time" + "github.com/dgrijalva/jwt-go" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/core/config" + "github.com/goharbor/harbor/src/pkg/token/claim" "github.com/stretchr/testify/assert" ) @@ -33,7 +35,15 @@ func TestNew(t *testing.T) { projectID := int64(321) tokenExpiration := time.Duration(10) * 24 * time.Hour expiresAt := time.Now().UTC().Add(tokenExpiration).Unix() - token, err := New(tokenID, projectID, expiresAt, policies) + robot := claim.Robot{ + TokenID: tokenID, + ProjectID: projectID, + Access: policies, + StandardClaims: jwt.StandardClaims{ + ExpiresAt: expiresAt, + }, + } + token, err := New(DefaultTokenOptions(), robot) assert.Nil(t, err) assert.Equal(t, token.Header["alg"], "RS256") @@ -54,7 +64,15 @@ func TestRaw(t *testing.T) { tokenExpiration := time.Duration(10) * 24 * time.Hour expiresAt := time.Now().UTC().Add(tokenExpiration).Unix() - token, err := New(tokenID, projectID, expiresAt, policies) + robot := claim.Robot{ + TokenID: tokenID, + ProjectID: projectID, + Access: policies, + StandardClaims: jwt.StandardClaims{ + ExpiresAt: expiresAt, + }, + } + token, err := New(DefaultTokenOptions(), robot) assert.Nil(t, err) rawTk, err := token.Raw() @@ -64,8 +82,8 @@ func TestRaw(t *testing.T) { func TestParseWithClaims(t *testing.T) { rawTk := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJJRCI6MTIzLCJQcm9qZWN0SUQiOjAsIkFjY2VzcyI6W3siUmVzb3VyY2UiOiIvcHJvamVjdC9saWJyYXkvcmVwb3NpdG9yeSIsIkFjdGlvbiI6InB1bGwiLCJFZmZlY3QiOiIifV0sIlN0YW5kYXJkQ2xhaW1zIjp7ImV4cCI6MTU0ODE0MDIyOSwiaXNzIjoiaGFyYm9yLXRva2VuLWlzc3VlciJ9fQ.Jc3qSKN4SJVUzAvBvemVpRcSOZaHlu0Avqms04qzPm4ru9-r9IRIl3mnSkI6m9XkzLUeJ7Kiwyw63ghngnVKw_PupeclOGC6s3TK5Cfmo4h-lflecXjZWwyy-dtH_e7Us_ItS-R3nXDJtzSLEpsGHCcAj-1X2s93RB2qD8LNSylvYeDezVkTzqRzzfawPJheKKh9JTrz-3eUxCwQard9-xjlwvfUYULoHTn9npNAUq4-jqhipW4uE8HL-ym33AGF57la8U0RO11hmDM5K8-PiYknbqJ_oONeS3HBNym2pEFeGjtTv2co213wl4T5lemlg4SGolMBuJ03L7_beVZ0o-MKTkKDqDwJalb6_PM-7u3RbxC9IzJMiwZKIPnD3FvV10iPxUUQHaH8Jz5UZ2pFIhi_8BNnlBfT0JOPFVYATtLjHMczZelj2YvAeR1UHBzq3E0jPpjjwlqIFgaHCaN_KMwEvadTo_Fi2sEH4pNGP7M3yehU_72oLJQgF4paJarsmEoij6ZtPs6xekBz1fccVitq_8WNIz9aeCUdkUBRwI5QKw1RdW4ua-w74ld5MZStWJA8veyoLkEb_Q9eq2oAj5KWFjJbW5-ltiIfM8gxKflsrkWAidYGcEIYcuXr7UdqEKXxtPiWM0xb3B91ovYvO5402bn3f9-UGtlcestxNHA" - rClaims := &RobotClaims{} - _, _ = ParseWithClaims(rawTk, rClaims) + rClaims := &claim.Robot{} + _, _ = Parse(DefaultTokenOptions(), rawTk, rClaims) assert.Equal(t, int64(123), rClaims.TokenID) assert.Equal(t, int64(0), rClaims.ProjectID) assert.Equal(t, "/project/libray/repository", rClaims.Access[0].Resource.String()) From 5996189bb03bafde09bc95c3e953c45516a56b3e Mon Sep 17 00:00:00 2001 From: wang yan Date: Mon, 21 Oct 2019 23:15:51 +0800 Subject: [PATCH 2/5] update per comments and fix govet error Signed-off-by: wang yan --- src/common/rbac/const.go | 3 +- src/common/security/admiral/context.go | 5 --- src/common/security/context.go | 2 - src/common/security/local/context.go | 5 --- src/common/security/robot/context.go | 21 ++++------ src/common/security/robot/context_test.go | 24 +++++------ src/common/security/secret/context.go | 5 --- src/common/utils/notary/helper.go | 2 +- src/common/utils/oidc/helper.go | 6 +-- .../utils/registry/auth/tokenauthorizer.go | 2 +- src/core/api/robot.go | 2 +- src/core/filter/security.go | 9 ++-- src/core/middlewares/config.go | 2 +- src/core/middlewares/contenttrust/handler.go | 10 +++-- src/core/middlewares/regtoken/handler.go | 34 ++++++++++++--- src/core/middlewares/util/util.go | 4 +- src/core/middlewares/vulnerable/handler.go | 10 +++-- src/core/service/token/authutils.go | 36 +++++++--------- src/core/service/token/creator.go | 9 +++- src/core/service/token/token_test.go | 9 +--- src/pkg/robot/controller.go | 18 +++++--- src/pkg/robot/model/robot.go | 14 +++---- src/pkg/token/claim/registry.go | 22 ---------- src/pkg/token/claims/registry/accessset.go | 18 ++++++++ src/pkg/token/claims/registry/actionset.go | 14 +++++++ src/pkg/token/claims/registry/registry.go | 42 +++++++++++++++++++ src/pkg/token/claims/registry/stringset.go | 21 ++++++++++ .../token/{claim => claims/robot}/robot.go | 15 ++++--- .../{claim => claims/robot}/robot_test.go | 10 ++--- src/pkg/token/option_test.go | 2 +- src/pkg/token/token_test.go | 8 ++-- 31 files changed, 229 insertions(+), 155 deletions(-) delete mode 100644 src/pkg/token/claim/registry.go create mode 100644 src/pkg/token/claims/registry/accessset.go create mode 100644 src/pkg/token/claims/registry/actionset.go create mode 100644 src/pkg/token/claims/registry/registry.go create mode 100644 src/pkg/token/claims/registry/stringset.go rename src/pkg/token/{claim => claims/robot}/robot.go (63%) rename src/pkg/token/{claim => claims/robot}/robot_test.go (92%) diff --git a/src/common/rbac/const.go b/src/common/rbac/const.go index 0e96d4cd8..cd30f404e 100755 --- a/src/common/rbac/const.go +++ b/src/common/rbac/const.go @@ -28,7 +28,8 @@ const ( ActionDelete = Action("delete") ActionList = Action("list") - ActionOperate = Action("operate") + ActionOperate = Action("operate") + ActionScannerPull = Action("scannerpull") // for robot account created by scanner to pull image, bypass the policy check ) // const resource variables diff --git a/src/common/security/admiral/context.go b/src/common/security/admiral/context.go index b4e19dbda..fcc0a069f 100644 --- a/src/common/security/admiral/context.go +++ b/src/common/security/admiral/context.go @@ -64,11 +64,6 @@ func (s *SecurityContext) IsSysAdmin() bool { return s.ctx.IsSysAdmin() } -// PolicyCheck ... -func (s *SecurityContext) PolicyCheck() bool { - return true -} - // IsSolutionUser ... func (s *SecurityContext) IsSolutionUser() bool { return false diff --git a/src/common/security/context.go b/src/common/security/context.go index 2de8b7ed5..4b879a218 100644 --- a/src/common/security/context.go +++ b/src/common/security/context.go @@ -27,8 +27,6 @@ type Context interface { GetUsername() string // IsSysAdmin returns whether the user is system admin IsSysAdmin() bool - // PolicyCheck returns whether the middlerwares apply to the request - PolicyCheck() bool // IsSolutionUser returns whether the user is solution user IsSolutionUser() bool // Get current user's all project diff --git a/src/common/security/local/context.go b/src/common/security/local/context.go index 56697f731..a80130da8 100644 --- a/src/common/security/local/context.go +++ b/src/common/security/local/context.go @@ -61,11 +61,6 @@ func (s *SecurityContext) IsSysAdmin() bool { return s.user.HasAdminRole } -// PolicyCheck ... -func (s *SecurityContext) PolicyCheck() bool { - return true -} - // IsSolutionUser ... func (s *SecurityContext) IsSolutionUser() bool { return false diff --git a/src/common/security/robot/context.go b/src/common/security/robot/context.go index c1a08fb29..43ebf5921 100644 --- a/src/common/security/robot/context.go +++ b/src/common/security/robot/context.go @@ -23,19 +23,17 @@ import ( // SecurityContext implements security.Context interface based on database type SecurityContext struct { - robot *model.Robot - pm promgr.ProjectManager - policy []*rbac.Policy - polichCheck bool + robot *model.Robot + pm promgr.ProjectManager + policy []*rbac.Policy } // NewSecurityContext ... -func NewSecurityContext(robot *model.Robot, pm promgr.ProjectManager, policy []*rbac.Policy, polichCheck bool) *SecurityContext { +func NewSecurityContext(robot *model.Robot, pm promgr.ProjectManager, policy []*rbac.Policy) *SecurityContext { return &SecurityContext{ - robot: robot, - pm: pm, - policy: policy, - polichCheck: polichCheck, + robot: robot, + pm: pm, + policy: policy, } } @@ -58,11 +56,6 @@ func (s *SecurityContext) IsSysAdmin() bool { return false } -// PolicyCheck ... -func (s *SecurityContext) PolicyCheck() bool { - return s.polichCheck -} - // IsSolutionUser robot cannot be a system admin func (s *SecurityContext) IsSolutionUser() bool { return false diff --git a/src/common/security/robot/context_test.go b/src/common/security/robot/context_test.go index 004a1acb6..a16cd40fe 100644 --- a/src/common/security/robot/context_test.go +++ b/src/common/security/robot/context_test.go @@ -93,45 +93,45 @@ func TestMain(m *testing.M) { func TestIsAuthenticated(t *testing.T) { // unauthenticated - ctx := NewSecurityContext(nil, nil, nil, true) + ctx := NewSecurityContext(nil, nil, nil) assert.False(t, ctx.IsAuthenticated()) // authenticated ctx = NewSecurityContext(&model.Robot{ Name: "test", Disabled: false, - }, nil, nil, true) + }, nil, nil) assert.True(t, ctx.IsAuthenticated()) } func TestGetUsername(t *testing.T) { // unauthenticated - ctx := NewSecurityContext(nil, nil, nil, true) + ctx := NewSecurityContext(nil, nil, nil) assert.Equal(t, "", ctx.GetUsername()) // authenticated ctx = NewSecurityContext(&model.Robot{ Name: "test", Disabled: false, - }, nil, nil, true) + }, nil, nil) assert.Equal(t, "test", ctx.GetUsername()) } func TestIsSysAdmin(t *testing.T) { // unauthenticated - ctx := NewSecurityContext(nil, nil, nil, true) + ctx := NewSecurityContext(nil, nil, nil) assert.False(t, ctx.IsSysAdmin()) // authenticated, non admin ctx = NewSecurityContext(&model.Robot{ Name: "test", Disabled: false, - }, nil, nil, true) + }, nil, nil) assert.False(t, ctx.IsSysAdmin()) } func TestIsSolutionUser(t *testing.T) { - ctx := NewSecurityContext(nil, nil, nil, true) + ctx := NewSecurityContext(nil, nil, nil) assert.False(t, ctx.IsSolutionUser()) } @@ -147,7 +147,7 @@ func TestHasPullPerm(t *testing.T) { Description: "desc", } - ctx := NewSecurityContext(robot, pm, policies, true) + ctx := NewSecurityContext(robot, pm, policies) resource := rbac.NewProjectNamespace(private.ProjectID).Resource(rbac.ResourceRepository) assert.True(t, ctx.Can(rbac.ActionPull, resource)) } @@ -164,7 +164,7 @@ func TestHasPushPerm(t *testing.T) { Description: "desc", } - ctx := NewSecurityContext(robot, pm, policies, true) + ctx := NewSecurityContext(robot, pm, policies) resource := rbac.NewProjectNamespace(private.ProjectID).Resource(rbac.ResourceRepository) assert.True(t, ctx.Can(rbac.ActionPush, resource)) } @@ -185,20 +185,20 @@ func TestHasPushPullPerm(t *testing.T) { Description: "desc", } - ctx := NewSecurityContext(robot, pm, policies, true) + ctx := NewSecurityContext(robot, pm, policies) resource := rbac.NewProjectNamespace(private.ProjectID).Resource(rbac.ResourceRepository) assert.True(t, ctx.Can(rbac.ActionPush, resource) && ctx.Can(rbac.ActionPull, resource)) } func TestGetMyProjects(t *testing.T) { - ctx := NewSecurityContext(nil, nil, nil, true) + ctx := NewSecurityContext(nil, nil, nil) projects, err := ctx.GetMyProjects() require.Nil(t, err) assert.Nil(t, projects) } func TestGetProjectRoles(t *testing.T) { - ctx := NewSecurityContext(nil, nil, nil, true) + ctx := NewSecurityContext(nil, nil, nil) roles := ctx.GetProjectRoles("test") assert.Nil(t, roles) } diff --git a/src/common/security/secret/context.go b/src/common/security/secret/context.go index 3482d2aa9..5dc06137a 100644 --- a/src/common/security/secret/context.go +++ b/src/common/security/secret/context.go @@ -66,11 +66,6 @@ func (s *SecurityContext) IsSysAdmin() bool { return false } -// PolicyCheck ... -func (s *SecurityContext) PolicyCheck() bool { - return true -} - // IsSolutionUser ... func (s *SecurityContext) IsSolutionUser() bool { return s.IsAuthenticated() diff --git a/src/common/utils/notary/helper.go b/src/common/utils/notary/helper.go index 781ef793a..db80a9450 100644 --- a/src/common/utils/notary/helper.go +++ b/src/common/utils/notary/helper.go @@ -76,7 +76,7 @@ func GetTargets(notaryEndpoint string, username string, fqRepo string) ([]model. Type: "repository", Name: fqRepo, Actions: []string{"pull"}, - }}, true) + }}) if err != nil { return nil, err } diff --git a/src/common/utils/oidc/helper.go b/src/common/utils/oidc/helper.go index 7de72a8b8..7cbec4aa9 100644 --- a/src/common/utils/oidc/helper.go +++ b/src/common/utils/oidc/helper.go @@ -208,7 +208,7 @@ func RefreshToken(ctx context.Context, token *Token) (*Token, error) { return &Token{Token: *t, IDToken: it}, nil } -// GroupsFromToken returns the list of group name in the token, the claim of the group list is set in OIDCSetting. +// GroupsFromToken returns the list of group name in the token, the claims of the group list is set in OIDCSetting. // It's designed not to return errors, in case of unexpected situation it will log and return empty list. func GroupsFromToken(token *gooidc.IDToken) []string { if token == nil { @@ -217,7 +217,7 @@ func GroupsFromToken(token *gooidc.IDToken) []string { } setting := provider.setting.Load().(models.OIDCSetting) if len(setting.GroupsClaim) == 0 { - log.Warning("Group claim is not set in OIDC setting returning empty group list.") + log.Warning("Group claims is not set in OIDC setting returning empty group list.") return []string{} } var c map[string]interface{} @@ -233,7 +233,7 @@ func groupsFromClaim(claimMap map[string]interface{}, k string) []string { var res []string g, ok := claimMap[k].([]interface{}) if !ok { - log.Warningf("Unable to get groups from claims, claims: %+v, groups claim key: %s", claimMap, k) + log.Warningf("Unable to get groups from claims, claims: %+v, groups claims key: %s", claimMap, k) return res } for _, e := range g { diff --git a/src/common/utils/registry/auth/tokenauthorizer.go b/src/common/utils/registry/auth/tokenauthorizer.go index c2c5fcbd4..1f1c95569 100644 --- a/src/common/utils/registry/auth/tokenauthorizer.go +++ b/src/common/utils/registry/auth/tokenauthorizer.go @@ -346,7 +346,7 @@ type rawTokenGenerator struct { // generate token directly func (r *rawTokenGenerator) generate(scopes []*token.ResourceActions, endpoint string) (*models.Token, error) { - return token_util.MakeToken(r.username, r.service, scopes, true) + return token_util.MakeToken(r.username, r.service, scopes) } func buildPingURL(endpoint string) string { diff --git a/src/core/api/robot.go b/src/core/api/robot.go index 0304e4b54..a3157e402 100644 --- a/src/core/api/robot.go +++ b/src/core/api/robot.go @@ -108,7 +108,7 @@ func (r *RobotAPI) Post() { return } robotReq.Visible = true - robotReq.PolicyCheck = true + robotReq.ByPassPolicyCheck = false robotReq.ProjectID = r.project.ProjectID if err := validateRobotReq(r.project, &robotReq); err != nil { diff --git a/src/core/filter/security.go b/src/core/filter/security.go index 9f2cdb620..891fcb5e4 100644 --- a/src/core/filter/security.go +++ b/src/core/filter/security.go @@ -44,7 +44,7 @@ import ( "github.com/goharbor/harbor/src/pkg/authproxy" "github.com/goharbor/harbor/src/pkg/robot" pkg_token "github.com/goharbor/harbor/src/pkg/token" - "github.com/goharbor/harbor/src/pkg/token/claim" + robot_claim "github.com/goharbor/harbor/src/pkg/token/claims/robot" ) // ContextValueKey for content value @@ -189,17 +189,16 @@ func (r *robotAuthReqCtxModifier) Modify(ctx *beegoctx.Context) bool { if !strings.HasPrefix(robotName, common.RobotPrefix) { return false } - rClaims := &claim.Robot{} + rClaims := &robot_claim.Claim{} opt := pkg_token.DefaultTokenOptions() rtk, err := pkg_token.Parse(opt, robotTk, rClaims) if err != nil { log.Errorf("failed to decrypt robot token, %v", err) return false } - // Do authn for robot account, as Harbor only stores the token ID, just validate the ID and disable. ctr := robot.RobotCtr - robot, err := ctr.GetRobotAccount(rtk.Claims.(*claim.Robot).TokenID) + robot, err := ctr.GetRobotAccount(rtk.Claims.(*robot_claim.Claim).TokenID) if err != nil { log.Errorf("failed to get robot %s: %v", robotName, err) return false @@ -218,7 +217,7 @@ func (r *robotAuthReqCtxModifier) Modify(ctx *beegoctx.Context) bool { } log.Debug("creating robot account security context...") pm := config.GlobalProjectMgr - securCtx := robotCtx.NewSecurityContext(robot, pm, rtk.Claims.(*claim.Robot).Access, rClaims.PolicyCheck) + securCtx := robotCtx.NewSecurityContext(robot, pm, rtk.Claims.(*robot_claim.Claim).Access) setSecurCtxAndPM(ctx.Request, securCtx, pm) return true } diff --git a/src/core/middlewares/config.go b/src/core/middlewares/config.go index d80489312..f8147b49d 100644 --- a/src/core/middlewares/config.go +++ b/src/core/middlewares/config.go @@ -26,7 +26,7 @@ const ( SIZEQUOTA = "sizequota" COUNTQUOTA = "countquota" IMMUTABLE = "immutable" - REGTOKEN = "REGTOKEN" + REGTOKEN = "regtoken" ) // ChartMiddlewares middlewares for chart server diff --git a/src/core/middlewares/contenttrust/handler.go b/src/core/middlewares/contenttrust/handler.go index 9dc6d1e69..9decccb56 100644 --- a/src/core/middlewares/contenttrust/handler.go +++ b/src/core/middlewares/contenttrust/handler.go @@ -50,10 +50,12 @@ func (cth contentTrustHandler) ServeHTTP(rw http.ResponseWriter, req *http.Reque return } // Token bypass policy check - policyCheck := req.Context().Value(util.PolicyCheckCtxKey) - if policyCheck != nil && !policyCheck.(bool) { - cth.next.ServeHTTP(rw, req) - return + if bypassPC := req.Context().Value(util.ByPassPolicyCheckCtxKey); bypassPC != nil { + bypassPolicyCheck, ok := bypassPC.(bool) + if ok && bypassPolicyCheck { + cth.next.ServeHTTP(rw, req) + return + } } if !util.GetPolicyChecker().ContentTrustEnabled(img.ProjectName) { cth.next.ServeHTTP(rw, req) diff --git a/src/core/middlewares/regtoken/handler.go b/src/core/middlewares/regtoken/handler.go index 67c2c934a..a31af164e 100644 --- a/src/core/middlewares/regtoken/handler.go +++ b/src/core/middlewares/regtoken/handler.go @@ -2,15 +2,20 @@ package regtoken import ( "context" + "github.com/docker/distribution/registry/auth" + "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/core/config" "github.com/goharbor/harbor/src/core/middlewares/util" pkg_token "github.com/goharbor/harbor/src/pkg/token" - "github.com/goharbor/harbor/src/pkg/token/claim" + "github.com/goharbor/harbor/src/pkg/token/claims/registry" "net/http" "strings" ) +// regTokenHandler is responsible for decoding the registry token in the docker pull request header, +// as harbor adds customized claims action into registry auth token, the middlerware is for decode it and write it into +// request context, then for other middlerwares in chain to use it to bypass request validation. type regTokenHandler struct { next http.Handler } @@ -24,6 +29,7 @@ func New(next http.Handler) http.Handler { // ServeHTTP ... func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { + imgRaw := req.Context().Value(util.ImageInfoCtxKey) if imgRaw == nil || !config.WithClair() { r.next.ServeHTTP(rw, req) @@ -42,14 +48,30 @@ func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { } rawToken := parts[1] opt := pkg_token.DefaultTokenOptions() - rClaims := &claim.Registry{} - rtk, err := pkg_token.Parse(opt, rawToken, rClaims) + regTK, err := pkg_token.Parse(opt, rawToken, ®istry.Claim{}) if err != nil { - log.Debug("failed to decode reg token: %v, the error is skipped and round the request to native registry.", err) + log.Debugf("failed to decode reg token: %v, the error is skipped and round the request to native registry.", err) r.next.ServeHTTP(rw, req) return } - ctx := context.WithValue(req.Context(), util.PolicyCheckCtxKey, rtk.Claims.(*claim.Registry).PolicyCheck) - req = req.WithContext(ctx) + + accessItems := []auth.Access{} + accessItems = append(accessItems, auth.Access{ + Resource: auth.Resource{ + Type: "repository", + Name: img.Repository, + }, + Action: rbac.ActionScannerPull.String(), + }) + + accessSet := regTK.Claims.(*registry.Claim).GetAccessSet() + for _, access := range accessItems { + if accessSet.Contains(access) { + ctx := context.WithValue(req.Context(), util.ByPassPolicyCheckCtxKey, true) + req = req.WithContext(ctx) + r.next.ServeHTTP(rw, req) + return + } + } r.next.ServeHTTP(rw, req) } diff --git a/src/core/middlewares/util/util.go b/src/core/middlewares/util/util.go index 4997bf338..f9ea116c4 100644 --- a/src/core/middlewares/util/util.go +++ b/src/core/middlewares/util/util.go @@ -49,8 +49,8 @@ type contextKey string const ( // ImageInfoCtxKey the context key for image information ImageInfoCtxKey = contextKey("ImageInfo") - // PolicyCheckCtxKey the context key for image information - PolicyCheckCtxKey = contextKey("PolicyCheck") + // ByPassPolicyCheckCtxKey the context key for robot account to bypass the pull policy check. + ByPassPolicyCheckCtxKey = contextKey("ByPassPolicyCheck") // TokenUsername ... // TODO: temp solution, remove after vmware/harbor#2242 is resolved. TokenUsername = "harbor-core" diff --git a/src/core/middlewares/vulnerable/handler.go b/src/core/middlewares/vulnerable/handler.go index eaecbae5a..f0acd7c0d 100644 --- a/src/core/middlewares/vulnerable/handler.go +++ b/src/core/middlewares/vulnerable/handler.go @@ -53,10 +53,12 @@ func (vh vulnerableHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) } // Token bypass policy check - policyCheck := req.Context().Value(util.PolicyCheckCtxKey) - if policyCheck != nil && !policyCheck.(bool) { - vh.next.ServeHTTP(rw, req) - return + if bypassPC := req.Context().Value(util.ByPassPolicyCheckCtxKey); bypassPC != nil { + bypassPolicyCheck, ok := bypassPC.(bool) + if ok && bypassPolicyCheck { + vh.next.ServeHTTP(rw, req) + return + } } // Is vulnerable policy set? diff --git a/src/core/service/token/authutils.go b/src/core/service/token/authutils.go index aac46c259..93e2ee2a4 100644 --- a/src/core/service/token/authutils.go +++ b/src/core/service/token/authutils.go @@ -42,12 +42,6 @@ func init() { privateKey = config.TokenPrivateKeyPath() } -// ClaimSet ... -type ClaimSet struct { - token.ClaimSet - PolicyCheck bool `json:"policy_check"` -} - // GetResourceActions ... func GetResourceActions(scopes []string) []*token.ResourceActions { log.Debugf("scopes: %+v", scopes) @@ -106,7 +100,7 @@ func filterAccess(access []*token.ResourceActions, ctx security.Context, } // MakeToken makes a valid jwt token based on parms. -func MakeToken(username, service string, access []*token.ResourceActions, policyCheck bool) (*models.Token, error) { +func MakeToken(username, service string, access []*token.ResourceActions) (*models.Token, error) { pk, err := libtrust.LoadKeyFile(privateKey) if err != nil { return nil, err @@ -116,7 +110,7 @@ func MakeToken(username, service string, access []*token.ResourceActions, policy return nil, err } - tk, expiresIn, issuedAt, err := makeTokenCore(issuer, username, service, expiration, access, pk, policyCheck) + tk, expiresIn, issuedAt, err := makeTokenCore(issuer, username, service, expiration, access, pk) if err != nil { return nil, err } @@ -139,12 +133,15 @@ func permToActions(p string) []string { if strings.Contains(p, "R") { res = append(res, "pull") } + if strings.Contains(p, "S") { + res = append(res, "scannerpull") + } return res } // make token core func makeTokenCore(issuer, subject, audience string, expiration int, - access []*token.ResourceActions, signingKey libtrust.PrivateKey, policyCheck bool) (t *token.Token, expiresIn int, issuedAt *time.Time, err error) { + access []*token.ResourceActions, signingKey libtrust.PrivateKey) (t *token.Token, expiresIn int, issuedAt *time.Time, err error) { joseHeader := &token.Header{ Type: "JWT", @@ -161,18 +158,15 @@ func makeTokenCore(issuer, subject, audience string, expiration int, issuedAt = &now expiresIn = expiration * 60 - claimSet := &ClaimSet{ - ClaimSet: token.ClaimSet{ - Issuer: issuer, - Subject: subject, - Audience: audience, - Expiration: now.Add(time.Duration(expiration) * time.Minute).Unix(), - NotBefore: now.Unix(), - IssuedAt: now.Unix(), - JWTID: jwtID, - Access: access, - }, - PolicyCheck: policyCheck, + claimSet := &token.ClaimSet{ + Issuer: issuer, + Subject: subject, + Audience: audience, + Expiration: now.Add(time.Duration(expiration) * time.Minute).Unix(), + NotBefore: now.Unix(), + IssuedAt: now.Unix(), + JWTID: jwtID, + Access: access, } var joseHeaderBytes, claimSetBytes []byte diff --git a/src/core/service/token/creator.go b/src/core/service/token/creator.go index 6ca8036ae..91acf218d 100644 --- a/src/core/service/token/creator.go +++ b/src/core/service/token/creator.go @@ -28,6 +28,7 @@ import ( "github.com/goharbor/harbor/src/core/config" "github.com/goharbor/harbor/src/core/filter" "github.com/goharbor/harbor/src/core/promgr" + "net/http/httputil" ) var creatorMap map[string]Creator @@ -154,7 +155,7 @@ type repositoryFilter struct { func (rep repositoryFilter) filter(ctx security.Context, pm promgr.ProjectManager, a *token.ResourceActions) error { - // clear action list to assign to new acess element after perm check. + // clear action list to assign to new access element after perm check. img, err := rep.parser.parse(a.Name) if err != nil { return err @@ -180,6 +181,9 @@ func (rep repositoryFilter) filter(ctx security.Context, pm promgr.ProjectManage } else if ctx.Can(rbac.ActionPull, resource) { permission = "R" } + if ctx.Can(rbac.ActionScannerPull, resource) { + permission = "S" + } a.Actions = permToActions(permission) return nil @@ -200,6 +204,7 @@ func (g generalCreator) Create(r *http.Request) (*models.Token, error) { var err error scopes := parseScopes(r.URL) log.Debugf("scopes: %v", scopes) + httputil.DumpRequest(r, true) ctx, err := filter.GetSecurityContext(r) if err != nil { @@ -222,7 +227,7 @@ func (g generalCreator) Create(r *http.Request) (*models.Token, error) { if err != nil { return nil, err } - return MakeToken(ctx.GetUsername(), g.service, access, ctx.PolicyCheck()) + return MakeToken(ctx.GetUsername(), g.service, access) } func parseScopes(u *url.URL) []string { diff --git a/src/core/service/token/token_test.go b/src/core/service/token/token_test.go index 5a722c23a..4a8435c2c 100644 --- a/src/core/service/token/token_test.go +++ b/src/core/service/token/token_test.go @@ -119,8 +119,7 @@ func getPublicKey(crtPath string) (*rsa.PublicKey, error) { type harborClaims struct { jwt.StandardClaims // Private claims - Access []*token.ResourceActions `json:"access"` - PolicyCheck bool `json:"policy_check"` + Access []*token.ResourceActions `json:"access"` } func TestMakeToken(t *testing.T) { @@ -134,7 +133,7 @@ func TestMakeToken(t *testing.T) { }} svc := "harbor-registry" u := "tester" - tokenJSON, err := MakeToken(u, svc, ra, true) + tokenJSON, err := MakeToken(u, svc, ra) if err != nil { t.Errorf("Error while making token: %v", err) } @@ -155,7 +154,6 @@ func TestMakeToken(t *testing.T) { t.Errorf("Error while parsing the token: %v", err) } claims := tok.Claims.(*harborClaims) - assert.True(t, claims.PolicyCheck) assert.Equal(t, *(claims.Access[0]), *(ra[0]), "Access mismatch") assert.Equal(t, claims.Audience, svc, "Audience mismatch") } @@ -244,9 +242,6 @@ func (f *fakeSecurityContext) IsSysAdmin() bool { func (f *fakeSecurityContext) IsSolutionUser() bool { return false } -func (f *fakeSecurityContext) PolicyCheck() bool { - return false -} func (f *fakeSecurityContext) Can(action rbac.Action, resource rbac.Resource) bool { return false } diff --git a/src/pkg/robot/controller.go b/src/pkg/robot/controller.go index 8d132fea6..e58dc8ea8 100644 --- a/src/pkg/robot/controller.go +++ b/src/pkg/robot/controller.go @@ -4,12 +4,13 @@ import ( "fmt" "github.com/dgrijalva/jwt-go" "github.com/goharbor/harbor/src/common" + "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/core/config" "github.com/goharbor/harbor/src/pkg/q" "github.com/goharbor/harbor/src/pkg/robot/model" "github.com/goharbor/harbor/src/pkg/token" - "github.com/goharbor/harbor/src/pkg/token/claim" + robot_claim "github.com/goharbor/harbor/src/pkg/token/claims/robot" "github.com/pkg/errors" "time" ) @@ -71,6 +72,12 @@ func (d *DefaultAPIController) CreateRobotAccount(robotReq *model.RobotCreate) ( ExpiresAt: expiresAt, Visible: robotReq.Visible, } + if robotReq.ByPassPolicyCheck { + robotReq.Access = append(robotReq.Access, &rbac.Policy{ + Resource: rbac.NewProjectNamespace(robotReq.ProjectID).Resource(rbac.ResourceRepository), + Action: rbac.ActionScannerPull, + }) + } id, err := d.manager.CreateRobotAccount(robot) if err != nil { return nil, err @@ -79,11 +86,10 @@ func (d *DefaultAPIController) CreateRobotAccount(robotReq *model.RobotCreate) ( // generate the token, and return it with response data. // token is not stored in the database. opt := token.DefaultTokenOptions() - rClaims := &claim.Robot{ - TokenID: id, - ProjectID: robotReq.ProjectID, - Access: robotReq.Access, - PolicyCheck: robotReq.PolicyCheck, + rClaims := &robot_claim.Claim{ + TokenID: id, + ProjectID: robotReq.ProjectID, + Access: robotReq.Access, StandardClaims: jwt.StandardClaims{ IssuedAt: time.Now().UTC().Unix(), ExpiresAt: expiresAt, diff --git a/src/pkg/robot/model/robot.go b/src/pkg/robot/model/robot.go index c5cf530c6..16401aab7 100644 --- a/src/pkg/robot/model/robot.go +++ b/src/pkg/robot/model/robot.go @@ -45,13 +45,13 @@ type RobotQuery struct { // RobotCreate ... type RobotCreate struct { - Name string `json:"name"` - ProjectID int64 `json:"pid"` - Description string `json:"description"` - Disabled bool `json:"disabled"` - Visible bool `json:"-"` - PolicyCheck bool `json:"-"` - Access []*rbac.Policy `json:"access"` + Name string `json:"name"` + ProjectID int64 `json:"pid"` + Description string `json:"description"` + Disabled bool `json:"disabled"` + Visible bool `json:"-"` + ByPassPolicyCheck bool `json:"-"` + Access []*rbac.Policy `json:"access"` } // Pagination ... diff --git a/src/pkg/token/claim/registry.go b/src/pkg/token/claim/registry.go deleted file mode 100644 index a93e499a9..000000000 --- a/src/pkg/token/claim/registry.go +++ /dev/null @@ -1,22 +0,0 @@ -package claim - -import ( - "github.com/dgrijalva/jwt-go" - "github.com/docker/distribution/registry/auth/token" -) - -// Registry implements the interface of jwt.Claims -type Registry struct { - jwt.StandardClaims - PolicyCheck bool `json:"policy_check"` - Access []*token.ResourceActions `json:"access"` -} - -// Valid valid the standard claims -func (rc *Registry) Valid() error { - stdErr := rc.StandardClaims.Valid() - if stdErr != nil { - return stdErr - } - return nil -} diff --git a/src/pkg/token/claims/registry/accessset.go b/src/pkg/token/claims/registry/accessset.go new file mode 100644 index 000000000..a4e4dacff --- /dev/null +++ b/src/pkg/token/claims/registry/accessset.go @@ -0,0 +1,18 @@ +package registry + +import ( + "github.com/docker/distribution/registry/auth" +) + +// AccessSet ... +type AccessSet map[auth.Resource]actionSet + +// Contains ... +func (s AccessSet) Contains(access auth.Access) bool { + actionSet, ok := s[access.Resource] + if ok { + return actionSet.contains(access.Action) + } + + return false +} diff --git a/src/pkg/token/claims/registry/actionset.go b/src/pkg/token/claims/registry/actionset.go new file mode 100644 index 000000000..3cc62646c --- /dev/null +++ b/src/pkg/token/claims/registry/actionset.go @@ -0,0 +1,14 @@ +package registry + +// actionSet is a special type of stringSet. +type actionSet struct { + stringSet +} + +func newActionSet(actions ...string) actionSet { + return actionSet{newStringSet(actions...)} +} + +func (s actionSet) contains(action string) bool { + return s.stringSet.contains(action) +} diff --git a/src/pkg/token/claims/registry/registry.go b/src/pkg/token/claims/registry/registry.go new file mode 100644 index 000000000..9b5f50496 --- /dev/null +++ b/src/pkg/token/claims/registry/registry.go @@ -0,0 +1,42 @@ +package registry + +import ( + "github.com/dgrijalva/jwt-go" + "github.com/docker/distribution/registry/auth" + "github.com/docker/distribution/registry/auth/token" +) + +// Claim implements the interface of jwt.Claims +type Claim struct { + jwt.StandardClaims + Access []*token.ResourceActions `json:"access"` +} + +// Valid valid the standard claims +func (rc *Claim) Valid() error { + stdErr := rc.StandardClaims.Valid() + if stdErr != nil { + return stdErr + } + return nil +} + +// GetAccessSet ... +func (rc *Claim) GetAccessSet() AccessSet { + accessSet := make(AccessSet, len(rc.Access)) + for _, resourceActions := range rc.Access { + resource := auth.Resource{ + Type: resourceActions.Type, + Name: resourceActions.Name, + } + set, exists := accessSet[resource] + if !exists { + set = newActionSet() + accessSet[resource] = set + } + for _, action := range resourceActions.Actions { + set.add(action) + } + } + return accessSet +} diff --git a/src/pkg/token/claims/registry/stringset.go b/src/pkg/token/claims/registry/stringset.go new file mode 100644 index 000000000..d1138867e --- /dev/null +++ b/src/pkg/token/claims/registry/stringset.go @@ -0,0 +1,21 @@ +package registry + +// StringSet is a useful type for looking up strings. +type stringSet map[string]struct{} + +func newStringSet(keys ...string) stringSet { + ss := make(stringSet, len(keys)) + ss.add(keys...) + return ss +} + +func (ss stringSet) add(keys ...string) { + for _, key := range keys { + ss[key] = struct{}{} + } +} + +func (ss stringSet) contains(key string) bool { + _, ok := ss[key] + return ok +} diff --git a/src/pkg/token/claim/robot.go b/src/pkg/token/claims/robot/robot.go similarity index 63% rename from src/pkg/token/claim/robot.go rename to src/pkg/token/claims/robot/robot.go index 58a25a9de..44ffa2633 100644 --- a/src/pkg/token/claim/robot.go +++ b/src/pkg/token/claims/robot/robot.go @@ -1,4 +1,4 @@ -package claim +package robot import ( "errors" @@ -6,17 +6,16 @@ import ( "github.com/goharbor/harbor/src/common/rbac" ) -// Robot implements the interface of jwt.Claims -type Robot struct { +// Claim implements the interface of jwt.Claims +type Claim struct { jwt.StandardClaims - TokenID int64 `json:"id"` - ProjectID int64 `json:"pid"` - PolicyCheck bool `json:"policy_check"` - Access []*rbac.Policy `json:"access"` + TokenID int64 `json:"id"` + ProjectID int64 `json:"pid"` + Access []*rbac.Policy `json:"access"` } // Valid valid the claims "tokenID, projectID and access". -func (rc Robot) Valid() error { +func (rc Claim) Valid() error { if rc.TokenID < 0 { return errors.New("Token id must an valid INT") } diff --git a/src/pkg/token/claim/robot_test.go b/src/pkg/token/claims/robot/robot_test.go similarity index 92% rename from src/pkg/token/claim/robot_test.go rename to src/pkg/token/claims/robot/robot_test.go index 50442f72b..054f4b4a8 100644 --- a/src/pkg/token/claim/robot_test.go +++ b/src/pkg/token/claims/robot/robot_test.go @@ -1,4 +1,4 @@ -package claim +package robot import ( "github.com/goharbor/harbor/src/common/rbac" @@ -15,7 +15,7 @@ func TestValid(t *testing.T) { policies := []*rbac.Policy{} policies = append(policies, rbacPolicy) - rClaims := &Robot{ + rClaims := &Claim{ TokenID: 1, ProjectID: 2, Access: policies, @@ -32,7 +32,7 @@ func TestUnValidTokenID(t *testing.T) { policies := []*rbac.Policy{} policies = append(policies, rbacPolicy) - rClaims := &Robot{ + rClaims := &Claim{ TokenID: -1, ProjectID: 2, Access: policies, @@ -49,7 +49,7 @@ func TestUnValidProjectID(t *testing.T) { policies := []*rbac.Policy{} policies = append(policies, rbacPolicy) - rClaims := &Robot{ + rClaims := &Claim{ TokenID: 1, ProjectID: -2, Access: policies, @@ -59,7 +59,7 @@ func TestUnValidProjectID(t *testing.T) { func TestUnValidPolicy(t *testing.T) { - rClaims := &Robot{ + rClaims := &Claim{ TokenID: 1, ProjectID: 2, Access: nil, diff --git a/src/pkg/token/option_test.go b/src/pkg/token/option_test.go index 227cf688d..421bf0cbb 100644 --- a/src/pkg/token/option_test.go +++ b/src/pkg/token/option_test.go @@ -11,7 +11,7 @@ func TestNewOptions(t *testing.T) { defaultOpt := DefaultTokenOptions() assert.NotNil(t, defaultOpt) assert.Equal(t, defaultOpt.SignMethod, jwt.GetSigningMethod("RS256")) - assert.Equal(t, defaultOpt.Issuer, "harbor-token-issuer") + assert.Equal(t, defaultOpt.Issuer, "harbor-token-defaultIssuer") assert.Equal(t, defaultOpt.TTL, 60*time.Minute) } diff --git a/src/pkg/token/token_test.go b/src/pkg/token/token_test.go index 0dd42d4ff..eab75a474 100644 --- a/src/pkg/token/token_test.go +++ b/src/pkg/token/token_test.go @@ -8,7 +8,7 @@ import ( "github.com/dgrijalva/jwt-go" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/core/config" - "github.com/goharbor/harbor/src/pkg/token/claim" + robot_claim "github.com/goharbor/harbor/src/pkg/token/claims/robot" "github.com/stretchr/testify/assert" ) @@ -35,7 +35,7 @@ func TestNew(t *testing.T) { projectID := int64(321) tokenExpiration := time.Duration(10) * 24 * time.Hour expiresAt := time.Now().UTC().Add(tokenExpiration).Unix() - robot := claim.Robot{ + robot := robot_claim.Claim{ TokenID: tokenID, ProjectID: projectID, Access: policies, @@ -64,7 +64,7 @@ func TestRaw(t *testing.T) { tokenExpiration := time.Duration(10) * 24 * time.Hour expiresAt := time.Now().UTC().Add(tokenExpiration).Unix() - robot := claim.Robot{ + robot := robot_claim.Claim{ TokenID: tokenID, ProjectID: projectID, Access: policies, @@ -82,7 +82,7 @@ func TestRaw(t *testing.T) { func TestParseWithClaims(t *testing.T) { rawTk := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJJRCI6MTIzLCJQcm9qZWN0SUQiOjAsIkFjY2VzcyI6W3siUmVzb3VyY2UiOiIvcHJvamVjdC9saWJyYXkvcmVwb3NpdG9yeSIsIkFjdGlvbiI6InB1bGwiLCJFZmZlY3QiOiIifV0sIlN0YW5kYXJkQ2xhaW1zIjp7ImV4cCI6MTU0ODE0MDIyOSwiaXNzIjoiaGFyYm9yLXRva2VuLWlzc3VlciJ9fQ.Jc3qSKN4SJVUzAvBvemVpRcSOZaHlu0Avqms04qzPm4ru9-r9IRIl3mnSkI6m9XkzLUeJ7Kiwyw63ghngnVKw_PupeclOGC6s3TK5Cfmo4h-lflecXjZWwyy-dtH_e7Us_ItS-R3nXDJtzSLEpsGHCcAj-1X2s93RB2qD8LNSylvYeDezVkTzqRzzfawPJheKKh9JTrz-3eUxCwQard9-xjlwvfUYULoHTn9npNAUq4-jqhipW4uE8HL-ym33AGF57la8U0RO11hmDM5K8-PiYknbqJ_oONeS3HBNym2pEFeGjtTv2co213wl4T5lemlg4SGolMBuJ03L7_beVZ0o-MKTkKDqDwJalb6_PM-7u3RbxC9IzJMiwZKIPnD3FvV10iPxUUQHaH8Jz5UZ2pFIhi_8BNnlBfT0JOPFVYATtLjHMczZelj2YvAeR1UHBzq3E0jPpjjwlqIFgaHCaN_KMwEvadTo_Fi2sEH4pNGP7M3yehU_72oLJQgF4paJarsmEoij6ZtPs6xekBz1fccVitq_8WNIz9aeCUdkUBRwI5QKw1RdW4ua-w74ld5MZStWJA8veyoLkEb_Q9eq2oAj5KWFjJbW5-ltiIfM8gxKflsrkWAidYGcEIYcuXr7UdqEKXxtPiWM0xb3B91ovYvO5402bn3f9-UGtlcestxNHA" - rClaims := &claim.Robot{} + rClaims := &robot_claim.Claim{} _, _ = Parse(DefaultTokenOptions(), rawTk, rClaims) assert.Equal(t, int64(123), rClaims.TokenID) assert.Equal(t, int64(0), rClaims.ProjectID) From 2fa85aefca811a4f15239b81827624519cf24de6 Mon Sep 17 00:00:00 2001 From: wang yan Date: Wed, 23 Oct 2019 13:04:15 +0800 Subject: [PATCH 3/5] fix per comments Signed-off-by: wang yan --- src/common/rbac/const.go | 2 +- src/common/security/robot/robot.go | 23 ++++++-- src/common/security/robot/robot_test.go | 3 +- src/core/middlewares/regtoken/handler.go | 7 +-- src/core/middlewares/regtoken/handler_test.go | 55 +++++++++++++++++++ src/core/middlewares/url/handler.go | 2 +- src/core/service/token/authutils.go | 2 +- src/core/service/token/creator.go | 4 +- src/pkg/token/claims/registry/registry.go | 6 +- .../token/claims/registry/registry_test.go | 54 ++++++++++++++++++ 10 files changed, 137 insertions(+), 21 deletions(-) create mode 100644 src/core/middlewares/regtoken/handler_test.go create mode 100644 src/pkg/token/claims/registry/registry_test.go diff --git a/src/common/rbac/const.go b/src/common/rbac/const.go index cd30f404e..6b2ff85f2 100755 --- a/src/common/rbac/const.go +++ b/src/common/rbac/const.go @@ -29,7 +29,7 @@ const ( ActionList = Action("list") ActionOperate = Action("operate") - ActionScannerPull = Action("scannerpull") // for robot account created by scanner to pull image, bypass the policy check + ActionScannerPull = Action("scanner-pull") // for robot account created by scanner to pull image, bypass the policy check ) // const resource variables diff --git a/src/common/security/robot/robot.go b/src/common/security/robot/robot.go index c900f672d..32b5b853f 100644 --- a/src/common/security/robot/robot.go +++ b/src/common/security/robot/robot.go @@ -47,11 +47,7 @@ func filterPolicies(namespace rbac.Namespace, policies []*rbac.Policy) []*rbac.P return results } - mp := map[string]bool{} - for _, policy := range project.GetAllPolicies(namespace) { - mp[policy.String()] = true - } - + mp := getAllPolicies(namespace) for _, policy := range policies { if mp[policy.String()] { results = append(results, policy) @@ -60,3 +56,20 @@ func filterPolicies(namespace rbac.Namespace, policies []*rbac.Policy) []*rbac.P return results } + +// getAllPolicies gets all of supported policies supported in project and external policies supported for robot account +func getAllPolicies(namespace rbac.Namespace) map[string]bool { + mp := map[string]bool{} + for _, policy := range project.GetAllPolicies(namespace) { + mp[policy.String()] = true + } + + scannerPull := &rbac.Policy{Resource: rbac.ResourceRepository, Action: rbac.ActionCreate} + robotExternalPolicies := []*rbac.Policy{} + robotExternalPolicies = append(robotExternalPolicies, scannerPull) + for _, policy := range robotExternalPolicies { + mp[policy.String()] = true + } + + return mp +} diff --git a/src/common/security/robot/robot_test.go b/src/common/security/robot/robot_test.go index 617c4c3fa..16b57735b 100644 --- a/src/common/security/robot/robot_test.go +++ b/src/common/security/robot/robot_test.go @@ -44,10 +44,11 @@ func TestGetPolicies(t *testing.T) { func TestNewRobot(t *testing.T) { policies := []*rbac.Policy{ {Resource: "/project/1/repository", Action: "pull"}, + {Resource: "/project/1/repository", Action: "scanner-pull"}, {Resource: "/project/library/repository", Action: "pull"}, {Resource: "/project/library/repository", Action: "push"}, } robot := NewRobot("test", rbac.NewProjectNamespace(1, false), policies) - assert.Len(t, robot.GetPolicies(), 1) + assert.Len(t, robot.GetPolicies(), 2) } diff --git a/src/core/middlewares/regtoken/handler.go b/src/core/middlewares/regtoken/handler.go index a31af164e..a39c6ea54 100644 --- a/src/core/middlewares/regtoken/handler.go +++ b/src/core/middlewares/regtoken/handler.go @@ -5,7 +5,6 @@ import ( "github.com/docker/distribution/registry/auth" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/common/utils/log" - "github.com/goharbor/harbor/src/core/config" "github.com/goharbor/harbor/src/core/middlewares/util" pkg_token "github.com/goharbor/harbor/src/pkg/token" "github.com/goharbor/harbor/src/pkg/token/claims/registry" @@ -31,7 +30,7 @@ func New(next http.Handler) http.Handler { func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { imgRaw := req.Context().Value(util.ImageInfoCtxKey) - if imgRaw == nil || !config.WithClair() { + if imgRaw == nil { r.next.ServeHTTP(rw, req) return } @@ -50,7 +49,7 @@ func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { opt := pkg_token.DefaultTokenOptions() regTK, err := pkg_token.Parse(opt, rawToken, ®istry.Claim{}) if err != nil { - log.Debugf("failed to decode reg token: %v, the error is skipped and round the request to native registry.", err) + log.Errorf("failed to decode reg token: %v, the error is skipped and round the request to native registry.", err) r.next.ServeHTTP(rw, req) return } @@ -58,7 +57,7 @@ func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { accessItems := []auth.Access{} accessItems = append(accessItems, auth.Access{ Resource: auth.Resource{ - Type: "repository", + Type: rbac.ResourceRepository.String(), Name: img.Repository, }, Action: rbac.ActionScannerPull.String(), diff --git a/src/core/middlewares/regtoken/handler_test.go b/src/core/middlewares/regtoken/handler_test.go new file mode 100644 index 000000000..5736781a2 --- /dev/null +++ b/src/core/middlewares/regtoken/handler_test.go @@ -0,0 +1,55 @@ +package regtoken + +import ( + "fmt" + "github.com/goharbor/harbor/src/core/middlewares/util" + "github.com/stretchr/testify/suite" + "net/http" + "net/http/httptest" + "os" + "testing" +) + +type HandlerSuite struct { + suite.Suite +} + +func doPullManifestRequest(projectName, name, tag string, next ...http.HandlerFunc) int { + repository := fmt.Sprintf("%s/%s", projectName, name) + + url := fmt.Sprintf("/v2/%s/manifests/%s", repository, tag) + req, _ := http.NewRequest("GET", url, nil) + + token := "eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6IkNWUTc6REM3NTpHVEROOkxTTUs6VUFJTjpIUUVWOlZVSDQ6Q0lRRDpRV01COlM0Qzc6U0c0STpGRUhYIn0.eyJpc3MiOiJoYXJib3ItdG9rZW4taXNzdWVyIiwic3ViIjoicm9ib3QkZGVtbzExIiwiYXVkIjoiaGFyYm9yLXJlZ2lzdHJ5IiwiZXhwIjoxNTcxNzYzOTI2LCJuYmYiOjE1NzE3NjM4NjYsImlhdCI6MTU3MTc2Mzg2NiwianRpIjoiTnRaZWx4Z01KTUU1MXlEMCIsImFjY2VzcyI6W3sidHlwZSI6InJlcG9zaXRvcnkiLCJuYW1lIjoibGlicmFyeS9oZWxsby13b3JsZCIsImFjdGlvbnMiOlsicHVzaCIsIioiLCJwdWxsIiwic2Nhbm5lcnB1bGwiXX1dfQ.GlWuvtoxmChnpvbWaG5901Z9-g63DrzyNUREWlDbR5gnNeuOKjLNyE4QpogAQKx2yYtcGxbqNL3VfJkExJ_gMS0Qw8e10utGOawwqD4oqf_J06eKq4HzpZJengZfcjMA4g2RoeOlqdVdwimB_PdX9vkBO1od0wX0Cc2v0p2w5TkibcThKRoeLeVs2oRewkKLuVHNSM8wwRIlAvpWJuNnvRCFlHRkLcZM_KpGXqT7H-PZETTisWCi1pMxeYEwIsDFLlTKdV8LaiDeDmH-RaLOsuyAySYEW9Ynk5K3P_dUl2c_SYQXloPyi0MvXxSn6EWE4eHF2oQDM_SvIzR9sOVB8TtjMjKKMQ4yr_mqgMcfEpnInJATExBR56wmxNdLESncHl8rUYCe2jCjQFuR9NGQA1tGdjI4NoBN-OVD0dBs9rm_mkb2tgD-3gEhyzAw6hg0uzDsF7bj5Aq8scoi42UurhX2bZM89s4-TWBp4DWuBG0HDiwpOiBvB3RMm6MpQxsqrl0hQm_WH18L6QCknAW2e3d_6DJWJ0eBzISrhDr7LkqJKl1J8pv4zqoh_EUVeLyzTmjEULm-VbnpVF4wW5yTLF3S6F7Ox4vwWtVfi1XQNVOcJDB3VPUsRgiTTuCW-ZGcBLw-OdIcwaJ3T_QZkEjUw1f6i1JcGa0Mpgl83aLiSdQ 0xc0003c77c0 map[alg:RS256 kid:CVQ7:DC75:GTDN:LSMK:UAIN:HQEV:VUH4:CIQD:QWMB:S4C7:SG4I:FEHX typ:JWT] 0xc000496000 GlWuvtoxmChnpvbWaG5901Z9-g63DrzyNUREWlDbR5gnNeuOKjLNyE4QpogAQKx2yYtcGxbqNL3VfJkExJ_gMS0Qw8e10utGOawwqD4oqf_J06eKq4HzpZJengZfcjMA4g2RoeOlqdVdwimB_PdX9vkBO1od0wX0Cc2v0p2w5TkibcThKRoeLeVs2oRewkKLuVHNSM8wwRIlAvpWJuNnvRCFlHRkLcZM_KpGXqT7H-PZETTisWCi1pMxeYEwIsDFLlTKdV8LaiDeDmH-RaLOsuyAySYEW9Ynk5K3P_dUl2c_SYQXloPyi0MvXxSn6EWE4eHF2oQDM_SvIzR9sOVB8TtjMjKKMQ4yr_mqgMcfEpnInJATExBR56wmxNdLESncHl8rUYCe2jCjQFuR9NGQA1tGdjI4NoBN-OVD0dBs9rm_mkb2tgD-3gEhyzAw6hg0uzDsF7bj5Aq8scoi42UurhX2bZM89s4-TWBp4DWuBG0HDiwpOiBvB3RMm6MpQxsqrl0hQm_WH18L6QCknAW2e3d_6DJWJ0eBzISrhDr7LkqJKl1J8pv4zqoh_EUVeLyzTmjEULm-VbnpVF4wW5yTLF3S6F7Ox4vwWtVfi1XQNVOcJDB3VPUsRgiTTuCW-ZGcBLw-OdIcwaJ3T_QZkEjUw1f6i1JcGa0Mpgl83aLiSdQ" + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + rr := httptest.NewRecorder() + + var n http.HandlerFunc + if len(next) > 0 { + n = next[0] + } else { + n = func(w http.ResponseWriter, req *http.Request) { + w.WriteHeader(http.StatusNotFound) + } + } + + h := New(http.HandlerFunc(n)) + h.ServeHTTP(util.NewCustomResponseWriter(rr), req) + + return rr.Code +} + +func (suite *HandlerSuite) TestPullManifest() { + code1 := doPullManifestRequest("library", "photon", "release-1.10") + suite.Equal(http.StatusNotFound, code1) +} + +func TestMain(m *testing.M) { + if result := m.Run(); result != 0 { + os.Exit(result) + } +} + +func TestRunHandlerSuite(t *testing.T) { + suite.Run(t, new(HandlerSuite)) +} diff --git a/src/core/middlewares/url/handler.go b/src/core/middlewares/url/handler.go index 07e1a0f3f..8a664e5c4 100644 --- a/src/core/middlewares/url/handler.go +++ b/src/core/middlewares/url/handler.go @@ -37,7 +37,7 @@ func New(next http.Handler) http.Handler { // ServeHTTP ... func (uh urlHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { - log.Debugf("in url handler, path: %s", req.URL.Path) + log.Infof("in url handler, path: %s", req.URL.Path) flag, repository, reference := util.MatchPullManifest(req) if flag { components := strings.SplitN(repository, "/", 2) diff --git a/src/core/service/token/authutils.go b/src/core/service/token/authutils.go index 93e2ee2a4..d04fe264d 100644 --- a/src/core/service/token/authutils.go +++ b/src/core/service/token/authutils.go @@ -134,7 +134,7 @@ func permToActions(p string) []string { res = append(res, "pull") } if strings.Contains(p, "S") { - res = append(res, "scannerpull") + res = append(res, "scanner-pull") } return res } diff --git a/src/core/service/token/creator.go b/src/core/service/token/creator.go index 91acf218d..92c6ee336 100644 --- a/src/core/service/token/creator.go +++ b/src/core/service/token/creator.go @@ -28,7 +28,6 @@ import ( "github.com/goharbor/harbor/src/core/config" "github.com/goharbor/harbor/src/core/filter" "github.com/goharbor/harbor/src/core/promgr" - "net/http/httputil" ) var creatorMap map[string]Creator @@ -182,7 +181,7 @@ func (rep repositoryFilter) filter(ctx security.Context, pm promgr.ProjectManage permission = "R" } if ctx.Can(rbac.ActionScannerPull, resource) { - permission = "S" + permission = fmt.Sprintf("%s%s", permission, "S") } a.Actions = permToActions(permission) @@ -204,7 +203,6 @@ func (g generalCreator) Create(r *http.Request) (*models.Token, error) { var err error scopes := parseScopes(r.URL) log.Debugf("scopes: %v", scopes) - httputil.DumpRequest(r, true) ctx, err := filter.GetSecurityContext(r) if err != nil { diff --git a/src/pkg/token/claims/registry/registry.go b/src/pkg/token/claims/registry/registry.go index 9b5f50496..a40535507 100644 --- a/src/pkg/token/claims/registry/registry.go +++ b/src/pkg/token/claims/registry/registry.go @@ -14,11 +14,7 @@ type Claim struct { // Valid valid the standard claims func (rc *Claim) Valid() error { - stdErr := rc.StandardClaims.Valid() - if stdErr != nil { - return stdErr - } - return nil + return rc.StandardClaims.Valid() } // GetAccessSet ... diff --git a/src/pkg/token/claims/registry/registry_test.go b/src/pkg/token/claims/registry/registry_test.go new file mode 100644 index 000000000..46cededf4 --- /dev/null +++ b/src/pkg/token/claims/registry/registry_test.go @@ -0,0 +1,54 @@ +package registry + +import ( + "github.com/docker/distribution/registry/auth" + "github.com/docker/distribution/registry/auth/token" + "github.com/goharbor/harbor/src/common/rbac" + "github.com/stretchr/testify/assert" + "testing" +) + +func TestValid(t *testing.T) { + access := &token.ResourceActions{ + Type: "type", + Name: "repository", + Actions: []string{"pull", "push"}, + } + accesses := []*token.ResourceActions{} + accesses = append(accesses, access) + rClaims := &Claim{ + Access: accesses, + } + assert.Nil(t, rClaims.Valid()) +} + +func TestGetAccessSet(t *testing.T) { + access := &token.ResourceActions{ + Type: "repository", + Name: "hello-world", + Actions: []string{"pull", "push", "scanner-pull"}, + } + accesses := []*token.ResourceActions{} + accesses = append(accesses, access) + rClaims := &Claim{ + Access: accesses, + } + + auth1 := auth.Access{ + Resource: auth.Resource{ + Type: "repository", + Name: "hello-world", + }, + Action: rbac.ActionScannerPull.String(), + } + auth2 := auth.Access{ + Resource: auth.Resource{ + Type: "repository", + Name: "busubox", + }, + Action: rbac.ActionScannerPull.String(), + } + set := rClaims.GetAccessSet() + assert.True(t, set.Contains(auth1)) + assert.False(t, set.Contains(auth2)) +} From a6ad1b2db8f26544a162ed4096a1217d39daf375 Mon Sep 17 00:00:00 2001 From: wang yan Date: Wed, 23 Oct 2019 18:43:39 +0800 Subject: [PATCH 4/5] update code per review comments Signed-off-by: wang yan --- src/common/security/robot/robot.go | 10 +--- src/core/api/robot.go | 1 - src/core/middlewares/contenttrust/handler.go | 10 ++-- src/core/middlewares/regtoken/handler.go | 16 ++---- src/core/middlewares/url/handler.go | 2 +- src/core/middlewares/util/util.go | 11 +++++ src/core/middlewares/vulnerable/handler.go | 10 ++-- src/core/service/token/creator.go | 7 ++- src/pkg/robot/controller.go | 7 --- src/pkg/robot/model/robot.go | 13 +++-- src/pkg/token/claims/registry/accesses.go | 49 +++++++++++++++++++ src/pkg/token/claims/registry/accessset.go | 18 ------- src/pkg/token/claims/registry/actionset.go | 14 ------ src/pkg/token/claims/registry/registry.go | 14 +++--- .../token/claims/registry/registry_test.go | 2 +- src/pkg/token/claims/registry/stringset.go | 21 -------- 16 files changed, 89 insertions(+), 116 deletions(-) create mode 100644 src/pkg/token/claims/registry/accesses.go delete mode 100644 src/pkg/token/claims/registry/accessset.go delete mode 100644 src/pkg/token/claims/registry/actionset.go delete mode 100644 src/pkg/token/claims/registry/stringset.go diff --git a/src/common/security/robot/robot.go b/src/common/security/robot/robot.go index 32b5b853f..db5fe8970 100644 --- a/src/common/security/robot/robot.go +++ b/src/common/security/robot/robot.go @@ -63,13 +63,7 @@ func getAllPolicies(namespace rbac.Namespace) map[string]bool { for _, policy := range project.GetAllPolicies(namespace) { mp[policy.String()] = true } - - scannerPull := &rbac.Policy{Resource: rbac.ResourceRepository, Action: rbac.ActionCreate} - robotExternalPolicies := []*rbac.Policy{} - robotExternalPolicies = append(robotExternalPolicies, scannerPull) - for _, policy := range robotExternalPolicies { - mp[policy.String()] = true - } - + scannerPull := &rbac.Policy{Resource: rbac.ResourceRepository, Action: rbac.ActionScannerPull} + mp[scannerPull.String()] = true return mp } diff --git a/src/core/api/robot.go b/src/core/api/robot.go index a3157e402..ded3a8d58 100644 --- a/src/core/api/robot.go +++ b/src/core/api/robot.go @@ -108,7 +108,6 @@ func (r *RobotAPI) Post() { return } robotReq.Visible = true - robotReq.ByPassPolicyCheck = false robotReq.ProjectID = r.project.ProjectID if err := validateRobotReq(r.project, &robotReq); err != nil { diff --git a/src/core/middlewares/contenttrust/handler.go b/src/core/middlewares/contenttrust/handler.go index 9decccb56..5a202674e 100644 --- a/src/core/middlewares/contenttrust/handler.go +++ b/src/core/middlewares/contenttrust/handler.go @@ -49,13 +49,9 @@ func (cth contentTrustHandler) ServeHTTP(rw http.ResponseWriter, req *http.Reque cth.next.ServeHTTP(rw, req) return } - // Token bypass policy check - if bypassPC := req.Context().Value(util.ByPassPolicyCheckCtxKey); bypassPC != nil { - bypassPolicyCheck, ok := bypassPC.(bool) - if ok && bypassPolicyCheck { - cth.next.ServeHTTP(rw, req) - return - } + if bypass, ok := util.BypassPolicyCheckFromContext(req.Context()); ok && bypass { + cth.next.ServeHTTP(rw, req) + return } if !util.GetPolicyChecker().ContentTrustEnabled(img.ProjectName) { cth.next.ServeHTTP(rw, req) diff --git a/src/core/middlewares/regtoken/handler.go b/src/core/middlewares/regtoken/handler.go index a39c6ea54..849a3d34a 100644 --- a/src/core/middlewares/regtoken/handler.go +++ b/src/core/middlewares/regtoken/handler.go @@ -1,7 +1,6 @@ package regtoken import ( - "context" "github.com/docker/distribution/registry/auth" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/common/utils/log" @@ -28,13 +27,7 @@ func New(next http.Handler) http.Handler { // ServeHTTP ... func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { - - imgRaw := req.Context().Value(util.ImageInfoCtxKey) - if imgRaw == nil { - r.next.ServeHTTP(rw, req) - return - } - img, _ := req.Context().Value(util.ImageInfoCtxKey).(util.ImageInfo) + img, _ := util.ImageInfoFromContext(req.Context()) if img.Digest == "" { r.next.ServeHTTP(rw, req) return @@ -63,13 +56,10 @@ func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { Action: rbac.ActionScannerPull.String(), }) - accessSet := regTK.Claims.(*registry.Claim).GetAccessSet() + accessSet := regTK.Claims.(*registry.Claim).GetAccess() for _, access := range accessItems { if accessSet.Contains(access) { - ctx := context.WithValue(req.Context(), util.ByPassPolicyCheckCtxKey, true) - req = req.WithContext(ctx) - r.next.ServeHTTP(rw, req) - return + *req = *(req.WithContext(util.NewBypassPolicyCheckContext(req.Context(), true))) } } r.next.ServeHTTP(rw, req) diff --git a/src/core/middlewares/url/handler.go b/src/core/middlewares/url/handler.go index 8a664e5c4..07e1a0f3f 100644 --- a/src/core/middlewares/url/handler.go +++ b/src/core/middlewares/url/handler.go @@ -37,7 +37,7 @@ func New(next http.Handler) http.Handler { // ServeHTTP ... func (uh urlHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { - log.Infof("in url handler, path: %s", req.URL.Path) + log.Debugf("in url handler, path: %s", req.URL.Path) flag, repository, reference := util.MatchPullManifest(req) if flag { components := strings.SplitN(repository, "/", 2) diff --git a/src/core/middlewares/util/util.go b/src/core/middlewares/util/util.go index f9ea116c4..fe4951714 100644 --- a/src/core/middlewares/util/util.go +++ b/src/core/middlewares/util/util.go @@ -445,6 +445,17 @@ func ManifestInfoFromContext(ctx context.Context) (*ManifestInfo, bool) { return info, ok } +// NewBypassPolicyCheckContext returns context with policy check info +func NewBypassPolicyCheckContext(ctx context.Context, bypass bool) context.Context { + return context.WithValue(ctx, ByPassPolicyCheckCtxKey, bypass) +} + +// BypassPolicyCheckFromContext returns whether to bypass policy check +func BypassPolicyCheckFromContext(ctx context.Context) (bool, bool) { + info, ok := ctx.Value(ByPassPolicyCheckCtxKey).(bool) + return info, ok +} + // NewBlobInfoContext returns context with blob info func NewBlobInfoContext(ctx context.Context, info *BlobInfo) context.Context { return context.WithValue(ctx, blobInfoKey, info) diff --git a/src/core/middlewares/vulnerable/handler.go b/src/core/middlewares/vulnerable/handler.go index f0acd7c0d..5b15c9343 100644 --- a/src/core/middlewares/vulnerable/handler.go +++ b/src/core/middlewares/vulnerable/handler.go @@ -52,13 +52,9 @@ func (vh vulnerableHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) return } - // Token bypass policy check - if bypassPC := req.Context().Value(util.ByPassPolicyCheckCtxKey); bypassPC != nil { - bypassPolicyCheck, ok := bypassPC.(bool) - if ok && bypassPolicyCheck { - vh.next.ServeHTTP(rw, req) - return - } + if bypass, ok := util.BypassPolicyCheckFromContext(req.Context()); ok && bypass { + vh.next.ServeHTTP(rw, req) + return } // Is vulnerable policy set? diff --git a/src/core/service/token/creator.go b/src/core/service/token/creator.go index 92c6ee336..ad002e511 100644 --- a/src/core/service/token/creator.go +++ b/src/core/service/token/creator.go @@ -154,7 +154,7 @@ type repositoryFilter struct { func (rep repositoryFilter) filter(ctx security.Context, pm promgr.ProjectManager, a *token.ResourceActions) error { - // clear action list to assign to new access element after perm check. + // clear action list to assign to new acess element after perm check. img, err := rep.parser.parse(a.Name) if err != nil { return err @@ -177,12 +177,11 @@ func (rep repositoryFilter) filter(ctx security.Context, pm promgr.ProjectManage permission = "RWM" } else if ctx.Can(rbac.ActionPush, resource) { permission = "RW" + } else if ctx.Can(rbac.ActionScannerPull, resource) { + permission = "RS" } else if ctx.Can(rbac.ActionPull, resource) { permission = "R" } - if ctx.Can(rbac.ActionScannerPull, resource) { - permission = fmt.Sprintf("%s%s", permission, "S") - } a.Actions = permToActions(permission) return nil diff --git a/src/pkg/robot/controller.go b/src/pkg/robot/controller.go index e58dc8ea8..1d5acc170 100644 --- a/src/pkg/robot/controller.go +++ b/src/pkg/robot/controller.go @@ -4,7 +4,6 @@ import ( "fmt" "github.com/dgrijalva/jwt-go" "github.com/goharbor/harbor/src/common" - "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/core/config" "github.com/goharbor/harbor/src/pkg/q" @@ -72,12 +71,6 @@ func (d *DefaultAPIController) CreateRobotAccount(robotReq *model.RobotCreate) ( ExpiresAt: expiresAt, Visible: robotReq.Visible, } - if robotReq.ByPassPolicyCheck { - robotReq.Access = append(robotReq.Access, &rbac.Policy{ - Resource: rbac.NewProjectNamespace(robotReq.ProjectID).Resource(rbac.ResourceRepository), - Action: rbac.ActionScannerPull, - }) - } id, err := d.manager.CreateRobotAccount(robot) if err != nil { return nil, err diff --git a/src/pkg/robot/model/robot.go b/src/pkg/robot/model/robot.go index 16401aab7..db911f11c 100644 --- a/src/pkg/robot/model/robot.go +++ b/src/pkg/robot/model/robot.go @@ -45,13 +45,12 @@ type RobotQuery struct { // RobotCreate ... type RobotCreate struct { - Name string `json:"name"` - ProjectID int64 `json:"pid"` - Description string `json:"description"` - Disabled bool `json:"disabled"` - Visible bool `json:"-"` - ByPassPolicyCheck bool `json:"-"` - Access []*rbac.Policy `json:"access"` + Name string `json:"name"` + ProjectID int64 `json:"pid"` + Description string `json:"description"` + Disabled bool `json:"disabled"` + Visible bool `json:"-"` + Access []*rbac.Policy `json:"access"` } // Pagination ... diff --git a/src/pkg/token/claims/registry/accesses.go b/src/pkg/token/claims/registry/accesses.go new file mode 100644 index 000000000..3665a415d --- /dev/null +++ b/src/pkg/token/claims/registry/accesses.go @@ -0,0 +1,49 @@ +package registry + +import ( + "github.com/docker/distribution/registry/auth" +) + +// Accesses ... +type Accesses map[auth.Resource]actions + +// Contains ... +func (s Accesses) Contains(access auth.Access) bool { + actionSet, ok := s[access.Resource] + if ok { + return actionSet.contains(access.Action) + } + + return false +} + +type actions struct { + stringSet +} + +func newActions(set ...string) actions { + return actions{newStringSet(set...)} +} + +func (s actions) contains(action string) bool { + return s.stringSet.contains(action) +} + +type stringSet map[string]struct{} + +func newStringSet(keys ...string) stringSet { + ss := make(stringSet, len(keys)) + ss.add(keys...) + return ss +} + +func (ss stringSet) add(keys ...string) { + for _, key := range keys { + ss[key] = struct{}{} + } +} + +func (ss stringSet) contains(key string) bool { + _, ok := ss[key] + return ok +} diff --git a/src/pkg/token/claims/registry/accessset.go b/src/pkg/token/claims/registry/accessset.go deleted file mode 100644 index a4e4dacff..000000000 --- a/src/pkg/token/claims/registry/accessset.go +++ /dev/null @@ -1,18 +0,0 @@ -package registry - -import ( - "github.com/docker/distribution/registry/auth" -) - -// AccessSet ... -type AccessSet map[auth.Resource]actionSet - -// Contains ... -func (s AccessSet) Contains(access auth.Access) bool { - actionSet, ok := s[access.Resource] - if ok { - return actionSet.contains(access.Action) - } - - return false -} diff --git a/src/pkg/token/claims/registry/actionset.go b/src/pkg/token/claims/registry/actionset.go deleted file mode 100644 index 3cc62646c..000000000 --- a/src/pkg/token/claims/registry/actionset.go +++ /dev/null @@ -1,14 +0,0 @@ -package registry - -// actionSet is a special type of stringSet. -type actionSet struct { - stringSet -} - -func newActionSet(actions ...string) actionSet { - return actionSet{newStringSet(actions...)} -} - -func (s actionSet) contains(action string) bool { - return s.stringSet.contains(action) -} diff --git a/src/pkg/token/claims/registry/registry.go b/src/pkg/token/claims/registry/registry.go index a40535507..07d18ce58 100644 --- a/src/pkg/token/claims/registry/registry.go +++ b/src/pkg/token/claims/registry/registry.go @@ -17,22 +17,22 @@ func (rc *Claim) Valid() error { return rc.StandardClaims.Valid() } -// GetAccessSet ... -func (rc *Claim) GetAccessSet() AccessSet { - accessSet := make(AccessSet, len(rc.Access)) +// GetAccess ... +func (rc *Claim) GetAccess() Accesses { + accesses := make(Accesses, len(rc.Access)) for _, resourceActions := range rc.Access { resource := auth.Resource{ Type: resourceActions.Type, Name: resourceActions.Name, } - set, exists := accessSet[resource] + set, exists := accesses[resource] if !exists { - set = newActionSet() - accessSet[resource] = set + set = newActions() + accesses[resource] = set } for _, action := range resourceActions.Actions { set.add(action) } } - return accessSet + return accesses } diff --git a/src/pkg/token/claims/registry/registry_test.go b/src/pkg/token/claims/registry/registry_test.go index 46cededf4..600a07edd 100644 --- a/src/pkg/token/claims/registry/registry_test.go +++ b/src/pkg/token/claims/registry/registry_test.go @@ -48,7 +48,7 @@ func TestGetAccessSet(t *testing.T) { }, Action: rbac.ActionScannerPull.String(), } - set := rClaims.GetAccessSet() + set := rClaims.GetAccess() assert.True(t, set.Contains(auth1)) assert.False(t, set.Contains(auth2)) } diff --git a/src/pkg/token/claims/registry/stringset.go b/src/pkg/token/claims/registry/stringset.go deleted file mode 100644 index d1138867e..000000000 --- a/src/pkg/token/claims/registry/stringset.go +++ /dev/null @@ -1,21 +0,0 @@ -package registry - -// StringSet is a useful type for looking up strings. -type stringSet map[string]struct{} - -func newStringSet(keys ...string) stringSet { - ss := make(stringSet, len(keys)) - ss.add(keys...) - return ss -} - -func (ss stringSet) add(keys ...string) { - for _, key := range keys { - ss[key] = struct{}{} - } -} - -func (ss stringSet) contains(key string) bool { - _, ok := ss[key] - return ok -} From 71c769ec97c95b7715299934f9f5a3bc1bddbb09 Mon Sep 17 00:00:00 2001 From: wang yan Date: Thu, 24 Oct 2019 14:02:25 +0800 Subject: [PATCH 5/5] remvoe bypass to scanner pull Signed-off-by: wang yan --- src/common/security/robot/robot.go | 3 +-- src/core/middlewares/contenttrust/handler.go | 2 +- src/core/middlewares/regtoken/handler.go | 9 +++++++-- src/core/middlewares/util/util.go | 16 ++++++++-------- src/core/middlewares/vulnerable/handler.go | 6 +++--- 5 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/common/security/robot/robot.go b/src/common/security/robot/robot.go index db5fe8970..d01dc1055 100644 --- a/src/common/security/robot/robot.go +++ b/src/common/security/robot/robot.go @@ -53,7 +53,6 @@ func filterPolicies(namespace rbac.Namespace, policies []*rbac.Policy) []*rbac.P results = append(results, policy) } } - return results } @@ -63,7 +62,7 @@ func getAllPolicies(namespace rbac.Namespace) map[string]bool { for _, policy := range project.GetAllPolicies(namespace) { mp[policy.String()] = true } - scannerPull := &rbac.Policy{Resource: rbac.ResourceRepository, Action: rbac.ActionScannerPull} + scannerPull := &rbac.Policy{Resource: namespace.Resource(rbac.ResourceRepository), Action: rbac.ActionScannerPull} mp[scannerPull.String()] = true return mp } diff --git a/src/core/middlewares/contenttrust/handler.go b/src/core/middlewares/contenttrust/handler.go index 5a202674e..049a2a88b 100644 --- a/src/core/middlewares/contenttrust/handler.go +++ b/src/core/middlewares/contenttrust/handler.go @@ -49,7 +49,7 @@ func (cth contentTrustHandler) ServeHTTP(rw http.ResponseWriter, req *http.Reque cth.next.ServeHTTP(rw, req) return } - if bypass, ok := util.BypassPolicyCheckFromContext(req.Context()); ok && bypass { + if scannerPull, ok := util.ScannerPullFromContext(req.Context()); ok && scannerPull { cth.next.ServeHTTP(rw, req) return } diff --git a/src/core/middlewares/regtoken/handler.go b/src/core/middlewares/regtoken/handler.go index 849a3d34a..d1f561e12 100644 --- a/src/core/middlewares/regtoken/handler.go +++ b/src/core/middlewares/regtoken/handler.go @@ -27,7 +27,12 @@ func New(next http.Handler) http.Handler { // ServeHTTP ... func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { - img, _ := util.ImageInfoFromContext(req.Context()) + imgRaw := req.Context().Value(util.ImageInfoCtxKey) + if imgRaw == nil { + r.next.ServeHTTP(rw, req) + return + } + img, _ := req.Context().Value(util.ImageInfoCtxKey).(util.ImageInfo) if img.Digest == "" { r.next.ServeHTTP(rw, req) return @@ -59,7 +64,7 @@ func (r *regTokenHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { accessSet := regTK.Claims.(*registry.Claim).GetAccess() for _, access := range accessItems { if accessSet.Contains(access) { - *req = *(req.WithContext(util.NewBypassPolicyCheckContext(req.Context(), true))) + *req = *(req.WithContext(util.NewScannerPullContext(req.Context(), true))) } } r.next.ServeHTTP(rw, req) diff --git a/src/core/middlewares/util/util.go b/src/core/middlewares/util/util.go index fe4951714..e023fc802 100644 --- a/src/core/middlewares/util/util.go +++ b/src/core/middlewares/util/util.go @@ -49,8 +49,8 @@ type contextKey string const ( // ImageInfoCtxKey the context key for image information ImageInfoCtxKey = contextKey("ImageInfo") - // ByPassPolicyCheckCtxKey the context key for robot account to bypass the pull policy check. - ByPassPolicyCheckCtxKey = contextKey("ByPassPolicyCheck") + // ScannerPullCtxKey the context key for robot account to bypass the pull policy check. + ScannerPullCtxKey = contextKey("ScannerPullCheck") // TokenUsername ... // TODO: temp solution, remove after vmware/harbor#2242 is resolved. TokenUsername = "harbor-core" @@ -445,14 +445,14 @@ func ManifestInfoFromContext(ctx context.Context) (*ManifestInfo, bool) { return info, ok } -// NewBypassPolicyCheckContext returns context with policy check info -func NewBypassPolicyCheckContext(ctx context.Context, bypass bool) context.Context { - return context.WithValue(ctx, ByPassPolicyCheckCtxKey, bypass) +// NewScannerPullContext returns context with policy check info +func NewScannerPullContext(ctx context.Context, scannerPull bool) context.Context { + return context.WithValue(ctx, ScannerPullCtxKey, scannerPull) } -// BypassPolicyCheckFromContext returns whether to bypass policy check -func BypassPolicyCheckFromContext(ctx context.Context) (bool, bool) { - info, ok := ctx.Value(ByPassPolicyCheckCtxKey).(bool) +// ScannerPullFromContext returns whether to bypass policy check +func ScannerPullFromContext(ctx context.Context) (bool, bool) { + info, ok := ctx.Value(ScannerPullCtxKey).(bool) return info, ok } diff --git a/src/core/middlewares/vulnerable/handler.go b/src/core/middlewares/vulnerable/handler.go index 5b15c9343..108d683f4 100644 --- a/src/core/middlewares/vulnerable/handler.go +++ b/src/core/middlewares/vulnerable/handler.go @@ -52,7 +52,7 @@ func (vh vulnerableHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) return } - if bypass, ok := util.BypassPolicyCheckFromContext(req.Context()); ok && bypass { + if scannerPull, ok := util.ScannerPullFromContext(req.Context()); ok && scannerPull { vh.next.ServeHTTP(rw, req) return } @@ -114,10 +114,10 @@ func (vh vulnerableHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) return } - // Print bypass CVE list + // Print scannerPull CVE list if len(summary.CVEBypassed) > 0 { for _, cve := range summary.CVEBypassed { - log.Infof("Vulnerable policy check: bypass CVE %s", cve) + log.Infof("Vulnerable policy check: scannerPull CVE %s", cve) } }