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(executor): change script file permissions to 0o644 and add test for non-root user creating a script #6905

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

bobh66
Copy link
Contributor

@bobh66 bobh66 commented Oct 10, 2021

Fixes #6643

This change adds a test to catch the case where the WorkflowExecutor.StageFiles() method creates a script file that is not readable by group/other, so execution of the script by the main container fails when there is a securityContext that specifies runAsNonRoot.

I'm pushing this first to make sure that the E2E test fails as expected before I push the fix, which is to revert a change in StageFiles() that changed the file permissions to 600 from 644.

@alexec
Copy link
Contributor

alexec commented Oct 10, 2021

You'll also need to update one of the e2e Golang files.

@bobh66 bobh66 force-pushed the fix-stage-files-for-nonroot branch from 6316b17 to c8e5ba9 Compare October 11, 2021 01:20
@bobh66
Copy link
Contributor Author

bobh66 commented Oct 12, 2021

@alexec Is anything more required to get approval to run the test?

@bobh66 bobh66 force-pushed the fix-stage-files-for-nonroot branch from c8e5ba9 to 757c0be Compare October 12, 2021 22:35
@bobh66
Copy link
Contributor Author

bobh66 commented Oct 13, 2021

@alexec I fixed a bug in the test file name - please re-run the tests. Thanks

@wdma
Copy link

wdma commented Oct 13, 2021

Hi All,

Just circling back to see if this issue has been fixed yet. I would love to be able to use scripts!

Also, I do not want to be a sap on the community and would love to help, but I am really new to Go. If you have a suggestion on how I a novice can start helping, please let me know!

@bobh66
Copy link
Contributor Author

bobh66 commented Oct 13, 2021

@wdna - this is broken in master and in v3.2.0 but not in v3.1.13, so if you use the v3.1.13 release for your work you should be able to use scripts, until this fix can get released

@wdma
Copy link

wdma commented Oct 13, 2021

@bobh66 Thank you, I will try 3.1.13

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #6905 (9b000a9) into master (2fd4b8a) will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6905      +/-   ##
==========================================
+ Coverage   48.53%   48.54%   +0.01%     
==========================================
  Files         265      265              
  Lines       19259    19263       +4     
==========================================
+ Hits         9348     9352       +4     
+ Misses       8860     8859       -1     
- Partials     1051     1052       +1     
Impacted Files Coverage Δ
workflow/executor/executor.go 24.10% <0.00%> (ø)
cmd/argo/commands/get.go 58.89% <0.00%> (-0.88%) ⬇️
workflow/controller/operator.go 71.08% <0.00%> (-0.13%) ⬇️
workflow/controller/controller.go 22.59% <0.00%> (+0.28%) ⬆️
cmd/argoexec/commands/emissary.go 51.79% <0.00%> (+1.43%) ⬆️
cmd/argo/commands/server.go 31.42% <0.00%> (+1.50%) ⬆️
workflow/metrics/server.go 19.29% <0.00%> (+3.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fd4b8a...9b000a9. Read the comment docs.

@bobh66 bobh66 force-pushed the fix-stage-files-for-nonroot branch from 757c0be to 9db206b Compare October 14, 2021 13:43
@@ -251,7 +251,7 @@ func (we *WorkflowExecutor) StageFiles() error {
default:
return nil
}
err := ioutil.WriteFile(filePath, body, 0o600)
err := ioutil.WriteFile(filePath, body, 0o644)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably get this fix out to v3.1 and v3.2 soon. @sarabala1979

@bobh66 bobh66 force-pushed the fix-stage-files-for-nonroot branch from 9db206b to f3f07aa Compare October 15, 2021 17:22
@bobh66 bobh66 changed the title WIP: fix(executor): add test for non-root user creating a script fix(executor): change script file permissions to 0o644 and add test for non-root user creating a script Oct 15, 2021
@bobh66 bobh66 force-pushed the fix-stage-files-for-nonroot branch from f3f07aa to 33b181d Compare October 15, 2021 18:51
@bobh66
Copy link
Contributor Author

bobh66 commented Oct 16, 2021

@alexec I don't know enough about golang to know what this lint error is trying to tell me.

@alexec
Copy link
Contributor

alexec commented Oct 16, 2021

Running “make lint” will fix the lint

Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
@bobh66 bobh66 force-pushed the fix-stage-files-for-nonroot branch from 33b181d to 9b000a9 Compare October 17, 2021 16:59
@alexec alexec enabled auto-merge (squash) October 18, 2021 16:23
@alexec alexec merged commit 73d6010 into argoproj:master Oct 18, 2021
@sarabala1979 sarabala1979 mentioned this pull request Oct 21, 2021
24 tasks
kriti-sc pushed a commit to kriti-sc/argo-workflows that referenced this pull request Oct 24, 2021
…#6905)

Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
Signed-off-by: kriti-sc <kathuriakriti1@gmail.com>
@valorl
Copy link
Contributor

valorl commented Oct 29, 2021

@alexec What is the plan for getting this released ? The release process is not very transparent to me. Its merged, but seems like it has been skipped in the v3.2.2 cherry pick (#7002) and not present at all in v3.2.3 (#7070), so what state is it now in ?

@alexec
Copy link
Contributor

alexec commented Oct 29, 2021

@sarabala1979 can this make the next patch release please?

@wdma
Copy link

wdma commented Nov 5, 2021

@AlexC or @sarabala1979 Can you tell me if the below error is related to this thread? I am trying to implement a script. Argo seems to compress and write as expected, but produces an error immediately after.

In trying to read through the error, it appears that either the writing failed or Argo cannot speak to the sourcecode. I am guessing that the writing failed, but I have no way to test and debug this.

Suggestions would be greatly appreciated.

time="2021-11-05T17:40:26.113Z" level=info msg="/var/run/argo/outputs/artifacts/raw.tgz -> /tmp/argo/outputs/artifacts/raw.tgz"
time="2021-11-05T17:40:26.120Z" level=error msg="executor error: open /var/run/argo/outputs/artifacts/raw.tgz: no such file or directory\ngit.luolix.top/argoproj/argo-workflows/v3/errors.New\n\t/go/src/github.com/argoproj/argo-workflows/errors/errors.go:49\ngit.luolix.top/argoproj/argo-workflows/v3/workflow/executor/emissary.emissary.CopyFile\n\t/go/src/github.com/argoproj/argo-workflows/workflow/executor/emissary/emissary.go:88\ngit.luolix.top/argoproj/argo-workflows/v3/workflow/executor.(*WorkflowExecutor).stageArchiveFile\n\t/go/src/github.com/argoproj/argo-workflows/workflow/executor/executor.go:400\ngit.luolix.top/argoproj/argo-workflows/v3/workflow/executor.(*WorkflowExecutor).saveArtifact\n\t/go/src/github.com/argoproj/argo-workflows/workflow/executor/executor.go:288\ngit.luolix.top/argoproj/argo-workflows/v3/workflow/executor.(*WorkflowExecutor).SaveArtifacts\n\t/go/src/github.com/argoproj/argo-workflows/workflow/executor/executor.go:274\ngit.luolix.top/argoproj/argo-workflows/v3/cmd/argoexec/commands.waitContainer\n\t/go/src/github.com/argoproj/argo-workflows/cmd/argoexec/commands/wait.go:62\ngit.luolix.top/argoproj/argo-workflows/v3/cmd/argoexec/commands.NewWaitCommand.func1\n\t/go/src/github.com/argoproj/argo-workflows/cmd/argoexec/commands/wait.go:18\ngit.luolix.top/spf13/cobra.(*Command).execute\n\t/go/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:860\ngit.luolix.top/spf13/cobra.(*Command).ExecuteC\n\t/go/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:974\ngit.luolix.top/spf13/cobra.(*Command).Execute\n\t/go/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:902\nmain.main\n\t/go/src/github.com/argoproj/argo-workflows/cmd/argoexec/main.go:15\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:225\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1371"

@alexec alexec mentioned this pull request Nov 5, 2021
25 tasks
alexec pushed a commit that referenced this pull request Nov 17, 2021
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
@sarabala1979 sarabala1979 mentioned this pull request Dec 15, 2021
73 tasks
@sarabala1979 sarabala1979 mentioned this pull request Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

argo run failed: Permission denied
4 participants