From 8d8a7358f8f9ad22906c3cabfbe44546bb6c7169 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Wed, 23 Sep 2020 23:39:44 -0400 Subject: [PATCH] Add AdditionalVerification function (#423) --- internal/auth/additional_verification_test.go | 153 ++++++++++++++++++ internal/auth/auth.go | 117 +++++++++++++- internal/auth/password/argon2_test.go | 3 +- internal/cmd/config/config_test.go | 2 - internal/servers/controller/testing.go | 18 +++ 5 files changed, 282 insertions(+), 11 deletions(-) create mode 100644 internal/auth/additional_verification_test.go diff --git a/internal/auth/additional_verification_test.go b/internal/auth/additional_verification_test.go new file mode 100644 index 0000000000..61b595a213 --- /dev/null +++ b/internal/auth/additional_verification_test.go @@ -0,0 +1,153 @@ +package auth_test + +import ( + "context" + "strings" + "testing" + + "github.com/hashicorp/boundary/internal/auth" + "github.com/hashicorp/boundary/internal/authtoken" + "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/servers" + "github.com/hashicorp/boundary/internal/servers/controller" + "github.com/hashicorp/boundary/internal/types/action" + "github.com/hashicorp/boundary/internal/types/resource" + "github.com/stretchr/testify/require" +) + +func TestAdditionalVerification(t *testing.T) { + tc := controller.NewTestController(t, nil) + defer tc.Shutdown() + + conn := tc.DbConn() + client := tc.Client() + token := tc.Token() + client.SetToken(token.Token) + org, proj := iam.TestScopes(t, tc.IamRepo(), iam.WithUserId(token.UserId), iam.WithSkipRoleCreation(true)) + + iamRepoFn := func() (*iam.Repository, error) { + return tc.IamRepo(), nil + } + serversRepoFn := func() (*servers.Repository, error) { + return tc.ServersRepo(), nil + } + authTokenRepoFn := func() (*authtoken.Repository, error) { + return tc.AuthTokenRepo(), nil + } + + orgRole := iam.TestRole(t, conn, org.GetPublicId()) + iam.TestUserRole(t, conn, orgRole.PublicId, token.UserId) + iam.TestRoleGrant(t, conn, orgRole.PublicId, "id=ampw_1234567890;actions=read,list") + + orgRoleInProj := iam.TestRole(t, conn, org.GetPublicId(), iam.WithGrantScopeId(proj.GetPublicId())) + iam.TestUserRole(t, conn, orgRoleInProj.PublicId, token.UserId) + iam.TestRoleGrant(t, conn, orgRoleInProj.PublicId, "id=hcst_1234567890;type=host-set;actions=create,update") + + projRole := iam.TestRole(t, conn, proj.GetPublicId()) + iam.TestUserRole(t, conn, projRole.PublicId, token.UserId) + iam.TestRoleGrant(t, conn, projRole.PublicId, "id=ttcp_1234567890;actions=authorize") + + type additionalCase struct { + name string + opts []auth.Option + allowed bool + } + cases := []struct { + name string + initialOpts []auth.Option + additionalCases []additionalCase + }{ + { + name: "base", + initialOpts: []auth.Option{ + auth.WithId("hsst_1234567890"), + auth.WithAction(action.Create), + auth.WithScopeId(proj.PublicId), + auth.WithType(resource.HostSet), + auth.WithPin("hcst_1234567890"), + }, + additionalCases: []additionalCase{ + { + name: "same as base", + opts: []auth.Option{ + auth.WithId("hsst_1234567890"), + auth.WithAction(action.Create), + auth.WithScopeId(proj.PublicId), + auth.WithType(resource.HostSet), + auth.WithPin("hcst_1234567890"), + }, + allowed: true, + }, + { + name: "no pin off from base", + opts: []auth.Option{ + auth.WithId("hsst_1234567890"), + auth.WithAction(action.Create), + auth.WithScopeId(proj.PublicId), + auth.WithType(resource.HostSet), + }, + }, + { + name: "good target", + opts: []auth.Option{ + auth.WithId("ttcp_1234567890"), + auth.WithAction(action.Authorize), + auth.WithScopeId(proj.PublicId), + auth.WithType(resource.Target), + }, + allowed: true, + }, + { + name: "cross scope", + opts: []auth.Option{ + auth.WithId("ampw_1234567890"), + auth.WithAction(action.List), + auth.WithScopeId(org.PublicId), + auth.WithType(resource.AuthMethod), + }, + allowed: true, + }, + { + name: "cross scope, bad action", + opts: []auth.Option{ + auth.WithId("ampw_1234567890"), + auth.WithAction(action.Update), + auth.WithScopeId(org.PublicId), + auth.WithType(resource.AuthMethod), + }, + }, + }, + }, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + req := require.New(t) + ctx := auth.NewVerifierContext( + context.Background(), + tc.Logger(), + iamRepoFn, + authTokenRepoFn, + serversRepoFn, + tc.Kms(), + auth.RequestInfo{ + PublicId: token.Id, + EncryptedToken: strings.Split(token.Token, "_")[2], + TokenFormat: auth.AuthTokenTypeBearer, + }) + res := auth.Verify(ctx, tt.initialOpts...) + req.NoError(res.Error) + + for _, c := range tt.additionalCases { + t.Run(c.name, func(t *testing.T) { + req = require.New(t) + res = res.AdditionalVerification(ctx, c.opts...) + if c.allowed { + req.NoError(res.Error) + } else { + req.Error(res.Error) + } + }) + } + }) + } +} diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 52ac0ddbd8..44996c1a85 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -67,6 +67,9 @@ type VerifyResults struct { AuthTokenId string Error error Scope *scopes.ScopeInfo + + // Used for additional verification + v *verifier } type verifier struct { @@ -79,6 +82,7 @@ type verifier struct { res *perms.Resource act action.Type ctx context.Context + acl perms.ACL } // NewVerifierContext creates a context that carries a verifier object from the @@ -115,6 +119,9 @@ func Verify(ctx context.Context, opt ...Option) (ret VerifyResults) { // context we won't catch in tests panic("no verifier information found in context") } + + ret.v = v + v.ctx = ctx opts := getOpts(opt...) @@ -154,13 +161,14 @@ func Verify(ctx context.Context, opt ...Option) (ret VerifyResults) { v.decryptToken() } - var authResults *perms.ACLResults + var authResults perms.ACLResults var err error - authResults, ret.UserId, ret.Scope, err = v.performAuthCheck() + authResults, ret.UserId, ret.Scope, v.acl, err = v.performAuthCheck() if err != nil { v.logger.Error("error performing authn/authz check", "error", err) return } + ret.AuthTokenId = v.requestInfo.PublicId if !authResults.Allowed { if v.requestInfo.DisableAuthzFailures { @@ -182,7 +190,102 @@ func Verify(ctx context.Context, opt ...Option) (ret VerifyResults) { return } -func (v verifier) performAuthCheck() (aclResults *perms.ACLResults, userId string, scopeInfo *scopes.ScopeInfo, retErr error) { +// AdditionalVerification is used to perform checks of additional resources for +// actions that need to touch more than one. +func (r *VerifyResults) AdditionalVerification(ctx context.Context, opt ...Option) (ret VerifyResults) { + v := r.v + + ret.Error = handlers.ForbiddenError() + + // Set other parameters the same to start with + ret.Scope = r.Scope + ret.UserId = r.UserId + ret.AuthTokenId = r.AuthTokenId + ret.v = r.v + + opts := getOpts(opt...) + + act := opts.withAction + res := perms.Resource{ + ScopeId: opts.withScopeId, + Id: opts.withId, + Pin: opts.withPin, + Type: opts.withType, + } + // Global scope has no parent ID; account for this + if opts.withId == scope.Global.String() && opts.withType == resource.Scope { + res.ScopeId = scope.Global.String() + } + + // Only perform lookup if it's actually different, otherwise use cached info + if res.ScopeId != r.Scope.Id { + iamRepo, err := v.iamRepoFn() + if err != nil { + v.logger.Error("additional verification: failed to get iam repo", "error", err) + return + } + + // Look up scope details to return. We can skip a lookup when using the + // global scope + switch res.ScopeId { + case "global": + ret.Scope = &scopes.ScopeInfo{ + Id: scope.Global.String(), + Type: scope.Global.String(), + Name: scope.Global.String(), + Description: "Global Scope", + ParentScopeId: "", + } + + default: + scp, err := iamRepo.LookupScope(v.ctx, v.res.ScopeId) + if err != nil { + v.logger.Error("additional verification: failed to get look up scope", "error", err) + return + } + if scp == nil { + v.logger.Error("additional verification: non-existent scope", "error", err) + return + } + ret.Scope = &scopes.ScopeInfo{ + Id: scp.GetPublicId(), + Type: scp.GetType(), + Name: scp.GetName(), + Description: scp.GetDescription(), + ParentScopeId: scp.GetParentId(), + } + } + } + + // Always allowed + if v.requestInfo.TokenFormat == AuthTokenTypeRecoveryKms { + ret.Error = nil + return + } + + aclResults := v.acl.Allowed(res, act) + + if !aclResults.Allowed { + if v.requestInfo.DisableAuthzFailures { + ret.Error = nil + // TODO: Decide whether to remove this + v.logger.Info("failed authz info for request", "resource", pretty.Sprint(v.res), "user_id", ret.UserId, "action", v.act.String()) + } else { + // If the anon user was used (either no token, or invalid (perhaps + // expired) token), return a 401. That way if it's an authn'd user + // that is not authz'd we'll return 403 to be explicit. + if ret.UserId == "u_anon" { + ret.Error = handlers.UnauthenticatedError() + } + return + } + } + + ret.Error = nil + return +} + +func (v verifier) performAuthCheck() (aclResults perms.ACLResults, userId string, scopeInfo *scopes.ScopeInfo, retAcl perms.ACL, retErr error) { // Ensure we return an error by default if we forget to set this somewhere retErr = errors.New("unknown") // Make the linter happy @@ -261,7 +364,7 @@ func (v verifier) performAuthCheck() (aclResults *perms.ACLResults, userId strin // At this point we don't need to look up grants since it's automatically allowed if v.requestInfo.TokenFormat == AuthTokenTypeRecoveryKms { - aclResults = &perms.ACLResults{Allowed: true} + aclResults.Allowed = true retErr = nil return } @@ -286,10 +389,8 @@ func (v verifier) performAuthCheck() (aclResults *perms.ACLResults, userId strin parsedGrants = append(parsedGrants, parsed) } - acl := perms.NewACL(parsedGrants...) - allowed := acl.Allowed(*v.res, v.act) - - aclResults = &allowed + retAcl = perms.NewACL(parsedGrants...) + aclResults = retAcl.Allowed(*v.res, v.act) retErr = nil return } diff --git a/internal/auth/password/argon2_test.go b/internal/auth/password/argon2_test.go index ce263e68a2..56d6da5685 100644 --- a/internal/auth/password/argon2_test.go +++ b/internal/auth/password/argon2_test.go @@ -407,7 +407,8 @@ func TestArgon2Credential_New(t *testing.T) { require.NoError(err) require.NotNil(got) - got.encrypt(context.Background(), wrapper) + err = got.encrypt(context.Background(), wrapper) + require.NoError(err) err = rw.Create(context.Background(), got) assert.NoError(err) diff --git a/internal/cmd/config/config_test.go b/internal/cmd/config/config_test.go index 2b391c9765..3454e6f657 100644 --- a/internal/cmd/config/config_test.go +++ b/internal/cmd/config/config_test.go @@ -38,7 +38,6 @@ func TestDevController(t *testing.T) { { Type: "tcp", Purpose: []string{"cluster"}, - TLSDisable: true, ProxyProtocolBehavior: "allow_authorized", ProxyProtocolAuthorizedAddrs: []*sockaddr.SockAddrMarshaler{ {SockAddr: addr}, @@ -114,7 +113,6 @@ func TestDevWorker(t *testing.T) { Listeners: []*configutil.Listener{ { Type: "tcp", - TLSDisable: true, Purpose: []string{"proxy"}, ProxyProtocolBehavior: "allow_authorized", ProxyProtocolAuthorizedAddrs: []*sockaddr.SockAddrMarshaler{ diff --git a/internal/servers/controller/testing.go b/internal/servers/controller/testing.go index 1dff38f890..36714183a4 100644 --- a/internal/servers/controller/testing.go +++ b/internal/servers/controller/testing.go @@ -9,10 +9,12 @@ import ( "github.com/hashicorp/boundary/api" "github.com/hashicorp/boundary/api/authmethods" "github.com/hashicorp/boundary/api/authtokens" + "github.com/hashicorp/boundary/internal/authtoken" "github.com/hashicorp/boundary/internal/cmd/base" "github.com/hashicorp/boundary/internal/cmd/config" "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/servers" "github.com/hashicorp/go-hclog" wrapping "github.com/hashicorp/go-kms-wrapping" @@ -60,6 +62,10 @@ func (tc *TestController) Context() context.Context { return tc.ctx } +func (tc *TestController) Kms() *kms.Kms { + return tc.c.kms +} + func (tc *TestController) IamRepo() *iam.Repository { repo, err := tc.c.IamRepoFn() if err != nil { @@ -68,6 +74,14 @@ func (tc *TestController) IamRepo() *iam.Repository { return repo } +func (tc *TestController) AuthTokenRepo() *authtoken.Repository { + repo, err := tc.c.AuthTokenRepoFn() + if err != nil { + tc.t.Fatal(err) + } + return repo +} + func (tc *TestController) ServersRepo() *servers.Repository { repo, err := tc.c.ServersRepoFn() if err != nil { @@ -96,6 +110,10 @@ func (tc *TestController) DbConn() *gorm.DB { return tc.b.Database } +func (tc *TestController) Logger() hclog.Logger { + return tc.b.Logger +} + func (tc *TestController) Token() *authtokens.AuthToken { if tc.opts.DisableAuthMethodCreation { tc.t.Error("no default auth method ID configured")