feat: Add mode to require Workflows to use workflowTemplateRef #3149

simster7 · 2020-06-02T20:55:39Z

Currently called "reference mode" and enabled as a flag to the workflow-controller binary.

workflow/controller/operator.go

Procfile

simster7 · 2020-06-08T20:14:23Z

cmd/workflow-controller/main.go

@@ -44,6 +44,7 @@ func NewRootCommand() *cobra.Command {
 qps float32
 namespaced bool // --namespaced
 managedNamespace string // --managed-namespace
+ referenceMode bool


Name is not final yet.. still thinking of better options

simster7 · 2020-06-08T20:15:48Z

workflow/controller/operator.go

+ } else if woc.controller.referenceMode {
+ // If the controller is in reference mode, ensure that the stored spec is identical to the reference spec at every operation
+ wftSpec, err := woc.fetchWorkflowSpec()
+ if err != nil {
+ return nil, wfv1.Arguments{}, err
+ }
+ if woc.wf.Status.StoredWorkflowSpec.String() != wftSpec.String() {
+ return nil, wfv1.Arguments{}, fmt.Errorf("workflowTemplateRef reference may not change during execution when the controller is in reference mode")
+ }


Notably, we need to check that the WorkflowTemplate and our own StoredWorkflowSpec have not changed. We do this to prevent attackers from specifying arbitrary code in wf.Status.StoredWorkflowSpec and simply running it by using workflowTemplateRef.

if the can change the status, surely they could have changed the spec?

Sure, but the key is that you can only change the workflowTemplateRef to another (already existing) WorkflowTemplate. The idea with this feature is to not allow an arbitrary WorkflowSpec from being executed, but instead restricting them to an already present WorkflowTemplate

sonarcloud · 2020-06-08T20:33:01Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

68.8% Coverage
0.0% Duplication

alexec · 2020-06-09T21:20:20Z

Could we better enforce this when you submit a workflow?

simster7 · 2020-06-10T14:50:47Z

Could we better enforce this when you submit a workflow?

I think this would make the feature easy to circumvent if the user has edit access to Workflows. Although we could take the official position that for this to work we expect users to not have edit access to existing Workflows.

alexec · 2020-06-10T16:25:30Z

I think as a user I would want to know that my workflow is invalid - because I've used inline templates - I'd like to know that sooner rather than later - rejected at submission.

I'm assuming we lint the workflow before we execute it (as a result of kubectl create -f my-wf.yaml. Can we have both for free?

simster7 · 2020-06-10T16:46:52Z

Since the restriction is enabled in the controller, we won't know if the controller will reject a Workflow before it is submitted. We could add an endpoint for the controller to tell us if it's in reference mode. The CLI/API would then query the endpoint when doing client-side validation and act appropriately.

However, I think this might be unnecessary since this logic is implemented at the beginning of the operation logic, meaning that a Workflow will fail immediately with a message displayed with the cause:

$ argo submit --watch examples/hello-world.yaml
Name:                hello-world-spth8
Namespace:           argo
ServiceAccount:      default
Status:              Error
Message:             workflows must use workflowTemplateRef to be executed when the controller is in reference mode
Conditions:
 Completed           True
Created:             Mon Jun 08 12:19:43 -0700 (now)
Started:             Mon Jun 08 12:19:43 -0700 (now)
Finished:            Mon Jun 08 12:19:43 -0700 (now)
Duration:            0 seconds

sarabala1979

LGTM, only one suggestion

sarabala1979 · 2020-06-18T23:12:42Z

workflow/controller/operator.go

 }

 func (woc *wfOperationCtx) loadExecutionSpec() (wfv1.TemplateReferenceHolder, wfv1.Arguments, error) {

 executionParameters := woc.wf.Spec.Arguments

 if woc.wf.Spec.WorkflowTemplateRef == nil {
- tmplRef := &wfv1.WorkflowStep{Template: woc.wfSpec.Entrypoint}
+ if woc.controller.Config.WorkflowRequirements.MustUseReference() {
+ return nil, wfv1.Arguments{}, fmt.Errorf("workflows must use workflowTemplateRef to be executed when the controller is in reference mode")


You can return the executionParameters arguments instead of returning new empty structs

sarabala1979 · 2020-06-18T23:13:08Z

workflow/controller/operator.go

+ return nil, wfv1.Arguments{}, err
+ }
+ if woc.wf.Status.StoredWorkflowSpec.String() != wftSpec.String() {
+ return nil, wfv1.Arguments{}, fmt.Errorf("workflowTemplateRef reference may not change during execution when the controller is in reference mode")


same here too for arguments

alexec · 2020-06-19T18:30:21Z

config/config.go

+
+type WorkflowRequirements struct {
+ ReferenceOnly bool `json:"referenceOnly"`
+ StrictReferenceOnly bool `json:"strictReferenceOnly"`


what about {reference: {strict:true}}?

@jessesuen and I settled on:

data: workflowRestrictions: templateReferencing: Strict

simster7 added 2 commits June 2, 2020 13:54

feat: Add mode to restrict Workflows to workflowTemplateRef

77333fb

Don't trust status

6c0909d

simster7 commented Jun 2, 2020

View reviewed changes

workflow/controller/operator.go Outdated Show resolved Hide resolved

simster7 added 6 commits June 2, 2020 15:16

minor

79af0fe

Merge branch 'master' into rest

5a12cc4

minor

c4095a7

Merge branch 'master' into rest

e965c85

Prog

41c3b87

Tests

8e54851

simster7 commented Jun 8, 2020

View reviewed changes

simster7 marked this pull request as ready for review June 8, 2020 20:15

Revert Procfile

1f36fe0

simster7 requested review from alexec and sarabala1979 June 9, 2020 16:03

simster7 added 6 commits June 18, 2020 10:03

Merge branch 'master' into rest

eaeb898

Switch to config

90abfa7

Merge branch 'master' into rest

b0d34ad

simplify

e1523b1

Done

0281b9a

fix

5629a07

simster7 changed the title ~~feat: Add mode to restrict Workflows to workflowTemplateRef~~ feat: Add mode to require Workflows to use workflowTemplateRef Jun 18, 2020

changes

19cccc5

sarabala1979 approved these changes Jun 18, 2020

View reviewed changes

alexec reviewed Jun 19, 2020

View reviewed changes

Merge branch 'master' into rest

d3b4286

simster7 added 2 commits June 20, 2020 11:16

Done

7f751c8

Codegen

5a7a987

simster7 merged commit 2dae724 into argoproj:master Jun 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add mode to require Workflows to use workflowTemplateRef #3149

feat: Add mode to require Workflows to use workflowTemplateRef #3149

simster7 commented Jun 2, 2020

simster7 Jun 8, 2020

simster7 Jun 8, 2020

alexec Jun 9, 2020

simster7 Jun 10, 2020

sonarcloud bot commented Jun 8, 2020

alexec commented Jun 9, 2020

simster7 commented Jun 10, 2020

alexec commented Jun 10, 2020

simster7 commented Jun 10, 2020

sarabala1979 left a comment

sarabala1979 Jun 18, 2020

sarabala1979 Jun 18, 2020

alexec Jun 19, 2020

simster7 Jun 20, 2020

feat: Add mode to require Workflows to use workflowTemplateRef #3149

feat: Add mode to require Workflows to use workflowTemplateRef #3149

Conversation

simster7 commented Jun 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 8, 2020

alexec commented Jun 9, 2020

simster7 commented Jun 10, 2020

alexec commented Jun 10, 2020

simster7 commented Jun 10, 2020

sarabala1979 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment