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.

Sign up for GitHub

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

Jump to bottom

feat: Remove binaries from argoexec image. Fixes #7486 #8292

Merged
merged 12 commits into from
Apr 5, 2022
Merged

feat: Remove binaries from argoexec image. Fixes #7486 #8292

merged 12 commits into from
Apr 5, 2022

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Apr 1, 2022 •
edited
Loading

Signed-off-by: Alex Collins alex_collins@intuit.com

Fixes #8291
Fixes #7800
Fixes #7798
Fixes #7796
Fixes #7486

argoexec still runs as root with read-write FS, but is much lower risk.

This PR obviates the following:

Closes #7253
Closes #7799
Closes #7983
Closes #7797

Why? The big security issue with both alpine and debian base images is that, when compromised by an attacker:

  • As root, can install any app you want.
  • Anything installed can be run.

Using distroless/scratch only things pre-installed can be run: argoexec, jq and kubectl. Naturally, kubectl is still an bad escalation if the pod is over-permissioned.

Next steps:

@alexec
fix: Remove most binaries form argoexec image
e2ce388 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
fix: go.mod
2c2942e 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec changed the title fix: Remove most binaries form argoexec image. Fixes #8291 fix: Remove binaries from argoexec image. Fixes #8291 Apr 1, 2022
@alexec alexec changed the title fix: Remove binaries from argoexec image. Fixes #8291 feat: Remove binaries from argoexec image. Fixes #8291 Apr 1, 2022
@terrytangyuan
Copy link
Member

Thanks let me know when ready for review.

@alexec
fix: ok
fc29544 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec marked this pull request as ready for review April 1, 2022 16:19
@alexec
Copy link
Contributor Author

alexec commented Apr 1, 2022

This PR should have a tough review.

@alexec alexec marked this pull request as draft April 4, 2022 15:12
alexec
alexec commented Apr 4, 2022
View reviewed changes
workflow/artifacts/git/git.go Outdated Show resolved Hide resolved
alexec
alexec commented Apr 4, 2022
View reviewed changes
workflow/artifacts/git/git.go Outdated Show resolved Hide resolved
@alexec
Merge branch 'master' into img
4056428 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
fix: git tests
e517971 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
fix: tidy up
2f4ac6d 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
Merge branch 'master' into img
f7ec988 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec changed the title feat: Remove binaries from argoexec image. Fixes #8291 feat: Remove binaries from argoexec image. Fixes #7486 Apr 4, 2022
@alexec
Merge branch 'master' into img
b0715c6 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
fix: change back from 8737
0ad629d 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
fix: gcr.io/distroless/static
899e2e7 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec marked this pull request as ready for review April 5, 2022 01:19
terrytangyuan
terrytangyuan reviewed Apr 5, 2022
View reviewed changes
workflow/artifacts/git/git.go Show resolved Hide resolved
workflow/artifacts/git/git_test.go Show resolved Hide resolved
workflow/artifacts/git/git_test.go Show resolved Hide resolved
workflow/executor/executor.go Outdated Show resolved Hide resolved
@alexec
fix: ok
b8c382b 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
Copy link
Contributor Author

alexec commented Apr 5, 2022

changes made - ready for review again

@alexec alexec enabled auto-merge (squash) April 5, 2022 14:39
terrytangyuan
terrytangyuan approved these changes Apr 5, 2022
View reviewed changes
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

LGTM.

@alexec
fix: lint
cceccbd 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec merged commit 153540f into argoproj:master Apr 5, 2022
@alexec alexec deleted the img branch April 5, 2022 15:47
@sarabala1979 sarabala1979 mentioned this pull request Apr 14, 2022
Closed
85 tasks
@swhite24 swhite24 mentioned this pull request Nov 2, 2022
Closed
3 tasks
@dcd000 dcd000 mentioned this pull request Dec 21, 2022
Closed
3 tasks
@agilgur5 agilgur5 added area/build Build or GithubAction/CI issues type/security Security related area/executor labels Oct 2, 2023
@sigwinch28 sigwinch28 mentioned this pull request Nov 5, 2023
Open
3 tasks
@sigwinch28
Copy link

The removal of the git binary means that file:// scheme git artifacts are no longer supportedbecause go-git is not pure go: it relies on the git binary for the file:// cheme: https://github.com/go-git/go-git/blob/master/COMPATIBILITY.md#transport-schemes

Warning: this is not pure Golang. This shells out to the git binary.

@agilgur5 agilgur5 mentioned this pull request May 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrytangyuan terrytangyuan terrytangyuan approved these changes

Assignees

@sarabala1979 sarabala1979

Labels
area/build Build or GithubAction/CI issues area/executor type/security Security related
Projects
None yet
Milestone
No milestone
5 participants
@alexec @terrytangyuan @sigwinch28 @agilgur5 @sarabala1979