Skip to content

Commit

Permalink
initial changes in settings, app, account, admin, rbac, doc and tests
Browse files Browse the repository at this point in the history
Signed-off-by: reggie-k <reginakagan@gmail.com>
  • Loading branch information
reggie-k committed Feb 16, 2022
1 parent f059c99 commit 5a5a851
Show file tree
Hide file tree
Showing 19 changed files with 443 additions and 8 deletions.
1 change: 1 addition & 0 deletions assets/builtin-policy.csv
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ p, role:readonly, repositories, get, *, allow
p, role:readonly, projects, get, *, allow
p, role:readonly, accounts, get, *, allow
p, role:readonly, gpgkeys, get, *, allow
p, role:readonly, logs, get, */*, allow

p, role:admin, applications, create, */*, allow
p, role:admin, applications, update, */*, allow
Expand Down
3 changes: 3 additions & 0 deletions cmd/argocd/commands/admin/settings_rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ var resourceMap map[string]string = map[string]string{
"cluster": rbacpolicy.ResourceClusters,
"gpgkey": rbacpolicy.ResourceGPGKeys,
"key": rbacpolicy.ResourceGPGKeys,
"log": rbacpolicy.ResourceLogs,
"logs": rbacpolicy.ResourceLogs,
"proj": rbacpolicy.ResourceProjects,
"projs": rbacpolicy.ResourceProjects,
"project": rbacpolicy.ResourceProjects,
Expand All @@ -48,6 +50,7 @@ var validRBACResources map[string]bool = map[string]bool{
rbacpolicy.ResourceCertificates: true,
rbacpolicy.ResourceClusters: true,
rbacpolicy.ResourceGPGKeys: true,
rbacpolicy.ResourceLogs: true,
rbacpolicy.ResourceProjects: true,
rbacpolicy.ResourceRepositories: true,
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ data:
p, role:user, applications, create, */*, allow
p, role:user, applications, delete, *, allow
p, role:user, applications, delete, */guestbook, deny
p, role:user, logs, get, */*, allow
g, test, role:user
policy.default: role:unknown
kind: ConfigMap
Expand Down
1 change: 1 addition & 0 deletions cmd/argocd/commands/admin/testdata/rbac/policy.csv
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ p, role:user, applications, create, */*, allow
p, role:user, applications, delete, *, allow
p, role:user, applications, delete, */guestbook, deny
p, role:test, certificates, get, *, allow
p, role:test, logs, get, */*, allow
g, test, role:user
2 changes: 1 addition & 1 deletion docs/user-guide/commands/argocd_account_can-i.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ argocd account can-i update projects 'default'
argocd account can-i create clusters '*'
Actions: [get create update delete sync override]
Resources: [clusters projects applications repositories certificates]
Resources: [clusters projects applications repositories certificates logs]
```

Expand Down
15 changes: 15 additions & 0 deletions server/account/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,21 @@ func (s *Server) CanI(ctx context.Context, r *account.CanIRequest) (*account.Can
if !slice.ContainsString(rbacpolicy.Resources, r.Resource, nil) {
return nil, status.Errorf(codes.InvalidArgument, "%v does not contain %s", rbacpolicy.Resources, r.Resource)
}

// Temporarily, logs RBAC will be enforced only if an intermediate var serverRBACLogEnforceEnable (representing ) is defined and has a "true" value
// Otherwise, no RBAC enforcement for logs will take place (meaning, can-i request on a logs resource will result in "yes")
// In the future, logs RBAC will be always enforced and the parameter along with this check will be removed
if r.Resource == "logs" {
logsRBACEnforceEnable, err := s.settingsMgr.GetServerRBACLogEnforceEnable()
if err != nil {
return nil, err
}

if !logsRBACEnforceEnable {
return &account.CanIResponse{Value: "yes"}, nil
}
}

ok := s.enf.Enforce(ctx.Value("claims"), r.Resource, r.Action, r.Subresource)
if ok {
return &account.CanIResponse{Value: "yes"}, nil
Expand Down
50 changes: 50 additions & 0 deletions server/account/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,3 +311,53 @@ func TestDeleteToken_SuccessfullyRemoved(t *testing.T) {

assert.Len(t, acc.Tokens, 0)
}

func TestCanI_GetLogsAllowNoSwitch(t *testing.T) {

accountServer, _ := newTestAccountServer(context.Background(), func(cm *v1.ConfigMap, secret *v1.Secret) {
})

ctx := projTokenContext(context.Background())
resp, err := accountServer.CanI(ctx, &account.CanIRequest{Resource: "logs", Action: "get", Subresource: ""})
assert.NoError(t, err)
assert.EqualValues(t, "yes", resp.Value)
}

func TestCanI_GetLogsDenySwitchOn(t *testing.T) {
enforcer := func(claims jwt.Claims, rvals ...interface{}) bool {
return false
}

accountServer, _ := newTestAccountServerExt(context.Background(), enforcer, func(cm *v1.ConfigMap, secret *v1.Secret) {
cm.Data["server.rbac.log.enforce.enable"] = "true"
})

ctx := projTokenContext(context.Background())
resp, err := accountServer.CanI(ctx, &account.CanIRequest{Resource: "logs", Action: "get", Subresource: "*/*"})
assert.NoError(t, err)
assert.EqualValues(t, "no", resp.Value)
}

func TestCanI_GetLogsAllowSwitchOn(t *testing.T) {

accountServer, _ := newTestAccountServer(context.Background(), func(cm *v1.ConfigMap, secret *v1.Secret) {
cm.Data["server.rbac.log.enforce.enable"] = "true"
})

ctx := projTokenContext(context.Background())
resp, err := accountServer.CanI(ctx, &account.CanIRequest{Resource: "logs", Action: "get", Subresource: ""})
assert.NoError(t, err)
assert.EqualValues(t, "yes", resp.Value)
}

func TestCanI_GetLogsAllowSwitchOff(t *testing.T) {

accountServer, _ := newTestAccountServer(context.Background(), func(cm *v1.ConfigMap, secret *v1.Secret) {
cm.Data["server.rbac.log.enforce.enable"] = "false"
})

ctx := projTokenContext(context.Background())
resp, err := accountServer.CanI(ctx, &account.CanIRequest{Resource: "logs", Action: "get", Subresource: ""})
assert.NoError(t, err)
assert.EqualValues(t, "yes", resp.Value)
}
14 changes: 14 additions & 0 deletions server/application/application.go
Original file line number Diff line number Diff line change
Expand Up @@ -1196,6 +1196,20 @@ func (s *Server) PodLogs(q *application.ApplicationPodLogsQuery, ws application.
return err
}

// Temporarily, logs RBAC will be enforced only if an intermediate env var serverRBACLogEnforceEnable is defined and has a "true" value
// Otherwise, no RBAC enforcement for logs will take place (meaning, can-i request on a logs resource will result in "yes")
// In the future, logs RBAC will be always enforced and the parameter along with this check will be removed
logsRBACEnforceEnable, err := s.settingsMgr.GetServerRBACLogEnforceEnable()
if err != nil {
return err
}

if logsRBACEnforceEnable {
if err := s.enf.EnforceErr(ws.Context().Value("claims"), rbacpolicy.ResourceLogs, rbacpolicy.ActionGet, appRBACName(*a)); err != nil {
return err
}
}

tree, err := s.getAppResources(ws.Context(), a)
if err != nil {
return err
Expand Down
4 changes: 3 additions & 1 deletion server/rbacpolicy/rbacpolicy.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const (
ResourceCertificates = "certificates"
ResourceAccounts = "accounts"
ResourceGPGKeys = "gpgkeys"
ResourceLogs = "logs"

// please add new items to Actions
ActionGet = "get"
Expand All @@ -40,6 +41,7 @@ var (
ResourceApplications,
ResourceRepositories,
ResourceCertificates,
ResourceLogs,
}
Actions = []string{
ActionGet,
Expand Down Expand Up @@ -167,7 +169,7 @@ func (p *RBACPolicyEnforcer) getProjectFromRequest(rvals ...interface{}) *v1alph
if res, ok := rvals[1].(string); ok {
if obj, ok := rvals[3].(string); ok {
switch res {
case ResourceApplications, ResourceRepositories, ResourceClusters:
case ResourceApplications, ResourceRepositories, ResourceClusters, ResourceLogs:
if objSplit := strings.Split(obj, "/"); len(objSplit) >= 2 {
return getProjectByName(objSplit[0])
}
Expand Down
31 changes: 25 additions & 6 deletions server/rbacpolicy/rbacpolicy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func newFakeProj() *argoappv1.AppProject {
Name: "my-role",
Policies: []string{
"p, proj:my-proj:my-role, applications, create, my-proj/*, allow",
"p, proj:my-proj:my-role, logs, get, my-proj/*, allow",
},
Groups: []string{
"my-org:my-team",
Expand All @@ -51,22 +52,30 @@ func TestEnforceAllPolicies(t *testing.T) {
projLister := test.NewFakeProjLister(newFakeProj())
enf := rbac.NewEnforcer(kubeclientset, test.FakeArgoCDNamespace, common.ArgoCDConfigMapName, nil)
enf.EnableLog(true)
_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj/*, allow`)
_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj/*, allow`)
_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj/*, allow` + "\n" + `p, alice, logs, get, my-proj/*, allow`)
_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj/*, allow` + "\n" + `p, bob, logs, get, my-proj/*, allow`)
rbacEnf := NewRBACPolicyEnforcer(enf, projLister)
enf.SetClaimsEnforcerFunc(rbacEnf.EnforceClaims)

claims := jwt.MapClaims{"sub": "alice"}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))

claims = jwt.MapClaims{"sub": "bob"}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))

claims = jwt.MapClaims{"sub": "proj:my-proj:my-role", "iat": 1234}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))

claims = jwt.MapClaims{"groups": []string{"my-org:my-team"}}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))

claims = jwt.MapClaims{"sub": "cathy"}
assert.False(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.False(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))

// AWS cognito returns its groups in cognito:groups
rbacEnf.SetScopes([]string{"cognito:groups"})
Expand Down Expand Up @@ -112,26 +121,36 @@ func TestInvalidatedCache(t *testing.T) {
projLister := test.NewFakeProjLister(newFakeProj())
enf := rbac.NewEnforcer(kubeclientset, test.FakeArgoCDNamespace, common.ArgoCDConfigMapName, nil)
enf.EnableLog(true)
_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj/*, allow`)
_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj/*, allow`)
_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj/*, allow` + "\n" + `p, alice, logs, get, my-proj/*, allow`)
_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj/*, allow` + `p, bob, logs, get, my-proj/*, allow`)
rbacEnf := NewRBACPolicyEnforcer(enf, projLister)
enf.SetClaimsEnforcerFunc(rbacEnf.EnforceClaims)

claims := jwt.MapClaims{"sub": "alice"}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))

claims = jwt.MapClaims{"sub": "bob"}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))

_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj2/*, allow`)
_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj2/*, allow`)
_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj2/*, allow` + "\n" + `p, alice, logs, get, my-proj2/*, allow`)
_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj2/*, allow` + "\n" + `p, bob, logs, get, my-proj2/*, allow`)
claims = jwt.MapClaims{"sub": "alice"}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj2/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj2/my-app"))

claims = jwt.MapClaims{"sub": "bob"}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj2/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj2/my-app"))

claims = jwt.MapClaims{"sub": "alice"}
assert.False(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.False(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))

claims = jwt.MapClaims{"sub": "bob"}
assert.False(t, enf.Enforce(claims, "applications", "create", "my-proj/my-app"))
assert.False(t, enf.Enforce(claims, "logs", "get", "my-proj/my-app"))
}

func TestGetScopes_DefaultScopes(t *testing.T) {
Expand Down
74 changes: 74 additions & 0 deletions test/e2e/accounts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package e2e

import (
"context"
"strings"
"testing"

"github.com/argoproj/pkg/errors"
Expand All @@ -13,6 +14,7 @@ import (
"github.com/argoproj/argo-cd/v2/cmd/argocd/commands/headless"
"github.com/argoproj/argo-cd/v2/pkg/apiclient/account"
"github.com/argoproj/argo-cd/v2/pkg/apiclient/session"
"github.com/argoproj/argo-cd/v2/test/e2e/fixture"
. "github.com/argoproj/argo-cd/v2/test/e2e/fixture"
accountFixture "github.com/argoproj/argo-cd/v2/test/e2e/fixture/account"
"github.com/argoproj/argo-cd/v2/util/io"
Expand All @@ -38,6 +40,78 @@ func TestCreateAndUseAccount(t *testing.T) {
})
}

func TestCanIGetLogsAllowNoSwitch(t *testing.T) {
ctx := accountFixture.Given(t)
ctx.
Name("test").
When().
Create().
Login().
CanIGetLogs().
Then().
AndCLIOutput(func(output string, err error) {
assert.True(t, strings.Contains(output, "yes"))
})
}

func TestCanIGetLogsDenySwitchOn(t *testing.T) {
ctx := accountFixture.Given(t)
ctx.
Name("test").
When().
Create().
Login().
SetParamInSettingConfigMap("server.rbac.log.enforce.enable", "true").
CanIGetLogs().
Then().
AndCLIOutput(func(output string, err error) {
assert.True(t, strings.Contains(output, "no"))
})
}

func TestCanIGetLogsAllowSwitchOn(t *testing.T) {
ctx := accountFixture.Given(t)
ctx.
Name("test").
Project(ProjectName).
When().
Create().
Login().
SetPermissions([]fixture.ACL{
{
Resource: "logs",
Action: "get",
Scope: ProjectName + "/*",
},
{
Resource: "apps",
Action: "get",
Scope: ProjectName + "/*",
},
}, "log-viewer").
SetParamInSettingConfigMap("server.rbac.log.enforce.enable", "true").
CanIGetLogs().
Then().
AndCLIOutput(func(output string, err error) {
assert.True(t, strings.Contains(output, "yes"))
})
}

func TestCanIGetLogsAllowSwitchOff(t *testing.T) {
ctx := accountFixture.Given(t)
ctx.
Name("test").
When().
Create().
Login().
SetParamInSettingConfigMap("server.rbac.log.enforce.enable", "false").
CanIGetLogs().
Then().
AndCLIOutput(func(output string, err error) {
assert.True(t, strings.Contains(output, "yes"))
})
}

func TestCreateAndUseAccountCLI(t *testing.T) {
EnsureCleanState(t)

Expand Down
Loading

0 comments on commit 5a5a851

Please sign in to comment.