From df6914d92982fa41a6405d0a340363f4993fc00e Mon Sep 17 00:00:00 2001 From: Alex Collins Date: Mon, 27 Jan 2020 16:53:09 -0800 Subject: [PATCH 1/4] fix(security): Fixes an issue that allowed you to list archived workflows you were not allowed to. Fixes #2049 --- server/auth/authorizer.go | 18 +++- .../archived_workflow_server.go | 7 +- test/e2e/argo_server_test.go | 101 ++++++++++-------- 3 files changed, 72 insertions(+), 54 deletions(-) diff --git a/server/auth/authorizer.go b/server/auth/authorizer.go index bee70ab2d97c..51d167eda175 100644 --- a/server/auth/authorizer.go +++ b/server/auth/authorizer.go @@ -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 } diff --git a/server/workflowarchive/archived_workflow_server.go b/server/workflowarchive/archived_workflow_server.go index 652159714d28..8dc5ebb32d4e 100644 --- a/server/workflowarchive/archived_workflow_server.go +++ b/server/workflowarchive/archived_workflow_server.go @@ -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.CronWorkflowPlural, 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.CronWorkflowPlural, wf.Namespace, wf.Name) if err != nil { return nil, err } diff --git a/test/e2e/argo_server_test.go b/test/e2e/argo_server_test.go index e61c66e78f83..5c24c747d8f2 100644 --- a/test/e2e/argo_server_test.go +++ b/test/e2e/argo_server_test.go @@ -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,32 @@ func (s *ArgoServerSuite) TestPermission() { Status(403) }) - if s.Persistence.IsEnabled() { + // Simply wait 10 seconds for the wf to be completed + 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 +304,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 get delete 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 get delete archived wf with bad 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() { From 8eff099c18bbbf8245d4f549492e8459d415da81 Mon Sep 17 00:00:00 2001 From: Alex Collins Date: Mon, 27 Jan 2020 16:54:15 -0800 Subject: [PATCH 2/4] fix: typo --- server/workflowarchive/archived_workflow_server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/workflowarchive/archived_workflow_server.go b/server/workflowarchive/archived_workflow_server.go index 8dc5ebb32d4e..682f76bfc575 100644 --- a/server/workflowarchive/archived_workflow_server.go +++ b/server/workflowarchive/archived_workflow_server.go @@ -87,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", workflow.CronWorkflowPlural, wf.Namespace, wf.Name) + allowed, err := auth.CanI(ctx, "get", workflow.WorkflowPlural, wf.Namespace, wf.Name) if err != nil { return nil, err } @@ -102,7 +102,7 @@ func (w *archivedWorkflowServer) DeleteArchivedWorkflow(ctx context.Context, req if err != nil { return nil, err } - allowed, err := auth.CanI(ctx, "delete", workflow.CronWorkflowPlural, wf.Namespace, wf.Name) + allowed, err := auth.CanI(ctx, "delete", workflow.WorkflowPlural, wf.Namespace, wf.Name) if err != nil { return nil, err } From 19eb22ea9940987261329dfd0f5c99f8556c6ab7 Mon Sep 17 00:00:00 2001 From: Alex Collins Date: Tue, 28 Jan 2020 10:29:32 -0800 Subject: [PATCH 3/4] Update argo_server_test.go --- test/e2e/argo_server_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/e2e/argo_server_test.go b/test/e2e/argo_server_test.go index 5c24c747d8f2..c092796d9dbb 100644 --- a/test/e2e/argo_server_test.go +++ b/test/e2e/argo_server_test.go @@ -265,7 +265,6 @@ func (s *ArgoServerSuite) TestPermission() { Status(403) }) - // Simply wait 10 seconds for the wf to be completed s.Given(). WorkflowName("test-wf-good"). When(). @@ -333,14 +332,14 @@ func (s *ArgoServerSuite) TestPermission() { Status(403) }) - // Test get delete archived wf with bad token + // 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 get delete archived wf with bad token + // Test deleting archived wf with bad token s.bearerToken = goodToken s.Run("DeleteArchivedWFsGoodToken", func(t *testing.T) { s.e(t).DELETE("/api/v1/archived-workflows/" + uid). From 95683a4c630f25782115476bbe7fd03ae750d364 Mon Sep 17 00:00:00 2001 From: Derek Wang Date: Tue, 28 Jan 2020 10:32:14 -0800 Subject: [PATCH 4/4] Update argo_server_test.go --- test/e2e/argo_server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/argo_server_test.go b/test/e2e/argo_server_test.go index c092796d9dbb..f6b9badfe072 100644 --- a/test/e2e/argo_server_test.go +++ b/test/e2e/argo_server_test.go @@ -339,7 +339,7 @@ func (s *ArgoServerSuite) TestPermission() { Expect(). Status(403) }) - // Test deleting archived wf with bad token + // 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).