Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(security): properly authorize list archived workflows #2079

Merged
merged 4 commits into from
Jan 28, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions server/auth/authorizer.go
Original file line number Diff line number Diff line change
@@ -2,10 +2,11 @@ package auth

import (
"context"
"sort"

log "github.com/sirupsen/logrus"
authorizationv1 "k8s.io/api/authorization/v1"

"github.com/argoproj/argo/pkg/apis/workflow"
)

func CanI(ctx context.Context, verb, resource, namespace, name string) (bool, error) {
@@ -49,9 +50,9 @@ func (a Authorizer) CanI(verb, resource, namespace, name string) (bool, error) {
for _, rule := range a.status[namespace].ResourceRules {
if allowed(rule.Verbs, verb) &&
allowed(rule.Resources, resource) &&
allowed(rule.APIGroups, "argoproj.io") &&
allowed(rule.APIGroups, workflow.Group) &&
allowed(rule.ResourceNames, name) {
logCtx.WithField("allowed", true).Debug("CanI")
logCtx.WithFields(log.Fields{"rule": rule, "allowed": true}).Debug("CanI")
return true, nil
}
}
@@ -64,5 +65,14 @@ func NewAuthorizer(ctx context.Context) *Authorizer {
}

func allowed(values []string, value string) bool {
return len(values) == 0 || sort.SearchStrings(values, "*") >= 0 || sort.SearchStrings(values, value) >= 0
return len(values) == 0 || contains(values, "*") || contains(values, value)
}

func contains(values []string, value string) bool {
for _, s := range values {
if value == s {
return true
}
}
return false
}
7 changes: 4 additions & 3 deletions server/workflowarchive/archived_workflow_server.go
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/argoproj/argo/persist/sqldb"
"github.com/argoproj/argo/pkg/apis/workflow"
wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
"github.com/argoproj/argo/server/auth"
)
@@ -57,7 +58,7 @@ func (w *archivedWorkflowServer) ListArchivedWorkflows(ctx context.Context, req
return nil, err
}
for _, wf := range moreItems {
allowed, err := authorizer.CanI("get", "workflow", wf.Namespace, wf.Name)
allowed, err := authorizer.CanI("get", workflow.WorkflowPlural, wf.Namespace, wf.Name)
if err != nil {
return nil, err
}
@@ -86,7 +87,7 @@ func (w *archivedWorkflowServer) GetArchivedWorkflow(ctx context.Context, req *G
if wf == nil {
return nil, status.Error(codes.NotFound, "not found")
}
allowed, err := auth.CanI(ctx, "get", "workflows", wf.Namespace, wf.Name)
allowed, err := auth.CanI(ctx, "get", workflow.WorkflowPlural, wf.Namespace, wf.Name)
if err != nil {
return nil, err
}
@@ -101,7 +102,7 @@ func (w *archivedWorkflowServer) DeleteArchivedWorkflow(ctx context.Context, req
if err != nil {
return nil, err
}
allowed, err := auth.CanI(ctx, "delete", "workflows", wf.Namespace, wf.Name)
allowed, err := auth.CanI(ctx, "delete", workflow.WorkflowPlural, wf.Namespace, wf.Name)
if err != nil {
return nil, err
}
100 changes: 53 additions & 47 deletions test/e2e/argo_server_test.go
Original file line number Diff line number Diff line change
@@ -222,7 +222,7 @@ func (s *ArgoServerSuite) TestPermission() {
Path("$.items").
Array().
Length().
Gt(0)
Equal(1)
})

// Test creating workflow with bad token
@@ -265,14 +265,31 @@ func (s *ArgoServerSuite) TestPermission() {
Status(403)
})

if s.Persistence.IsEnabled() {
s.Given().
WorkflowName("test-wf-good").
When().
WaitForWorkflow(30 * time.Second)

// Simply wait 10 seconds for the wf to be completed
s.Given().
WorkflowName("test-wf-good").
When().
WaitForWorkflow(10 * time.Second)
// Test delete workflow with bad token
s.bearerToken = badToken
s.Run("DeleteWFWithBadToken", func(t *testing.T) {
s.e(t).DELETE("/api/v1/workflows/" + nsName + "/test-wf-good").
Expect().
Status(403)
})

// Test delete workflow with good token
s.bearerToken = goodToken
s.Run("DeleteWFWithGoodToken", func(t *testing.T) {
s.e(t).DELETE("/api/v1/workflows/" + nsName + "/test-wf-good").
Expect().
Status(200)
})

// we've now deleted the workflow, but it is still in the archive, testing the archive
// after deleting the workflow makes sure that we are no dependant of the workflow for authorization

if s.Persistence.IsEnabled() {
// Test list archived WFs with good token
s.bearerToken = goodToken
s.Run("ListArchivedWFsGoodToken", func(t *testing.T) {
@@ -286,61 +303,50 @@ func (s *ArgoServerSuite) TestPermission() {
Array().Length().Gt(0)
})

s.bearerToken = badToken
s.Run("ListArchivedWFsBadToken", func(t *testing.T) {
s.e(t).GET("/api/v1/archived-workflows").
WithQuery("listOptions.labelSelector", "argo-e2e").
WithQuery("listOptions.fieldSelector", "metadata.namespace="+nsName).
Expect().
Status(200).
JSON().
Path("$.items").
Null()
})

// Test get archived wf with good token
s.bearerToken = goodToken
s.Run("GetArchivedWFsGoodToken", func(t *testing.T) {
s.e(t).GET("/api/v1/archived-workflows/"+uid).
WithQuery("listOptions.labelSelector", "argo-e2e").
Expect().
Status(200).
JSON().
Path("$.metadata.name").
Equal("test-wf-good")
Status(200)
})

// Test list archived WFs with bad token
// TODO: Uncomment following code after https://github.com/argoproj/argo/issues/2049 is resolved.

// s.bearerToken = badToken
// s.Run("ListArchivedWFsBadToken", func(t *testing.T) {
// s.e(t).GET("/api/v1/archived-workflows").
// WithQuery("listOptions.labelSelector", "argo-e2e").
// WithQuery("listOptions.fieldSelector", "metadata.namespace="+nsName).
// Expect().
// Status(200).
// JSON().
// Path("$.items").
// Array().
// Length().
// Equal(0)
// })

// Test get archived wf with bad token
s.bearerToken = badToken
s.Run("ListArchivedWFsBadToken", func(t *testing.T) {
s.e(t).GET("/api/v1/archived-workflows/"+uid).
WithQuery("listOptions.labelSelector", "argo-e2e").
s.Run("GetArchivedWFsBadToken", func(t *testing.T) {
s.e(t).GET("/api/v1/archived-workflows/" + uid).
Expect().
Status(403)
})

// Test deleting archived wf with bad token
s.bearerToken = badToken
s.Run("DeleteArchivedWFsBadToken", func(t *testing.T) {
s.e(t).DELETE("/api/v1/archived-workflows/" + uid).
Expect().
Status(403)
})
// Test deleting archived wf with good token
s.bearerToken = goodToken
s.Run("DeleteArchivedWFsGoodToken", func(t *testing.T) {
s.e(t).DELETE("/api/v1/archived-workflows/" + uid).
Expect().
Status(200)
})
}

// Test delete workflow with bad token
s.bearerToken = badToken
s.Run("DeleteWFWithBadToken", func(t *testing.T) {
s.e(t).DELETE("/api/v1/workflows/" + nsName + "/test-wf-good").
Expect().
Status(403)
})

// Test delete workflow with good token
s.bearerToken = goodToken
s.Run("DeleteWFWithGoodToken", func(t *testing.T) {
s.e(t).DELETE("/api/v1/workflows/" + nsName + "/test-wf-good").
Expect().
Status(200)
})
}

func (s *ArgoServerSuite) TestLintWorkflow() {