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

Best practice: Container should execute process(es) as a non-root user #182

Merged
merged 54 commits into from
Oct 4, 2021
Merged

Best practice: Container should execute process(es) as a non-root user #182

merged 54 commits into from
Oct 4, 2021

Conversation

taylor
Copy link
Member

@taylor taylor commented Jul 22, 2021 •
edited
Loading

This covers the least privilege best practice of avoiding running processes as root in containers

See least privilege issue #67 and discussion https://github.com/cncf/cnf-wg/discussions/20

Alternative best practice name: "Container should execute process(es) as non-root user"

Co-authored-by: Ian Wells iawells@cisco.com

@taylor @iawells
Initial draft for a no root in containers best practice
ca1e44c 
This covers the least privilege best practice of avoiding running processes as root in containers

Co-authored-by: Ian Wells <iawells@cisco.com>
@taylor taylor self-assigned this Jul 22, 2021
@taylor taylor changed the title Initial draft for a no root in containers best practice WIP: Initial draft for a no root in containers best practice Jul 22, 2021
@taylor taylor added this to the 2021 Best Practices milestone Jul 22, 2021
@vukg
Copy link
Collaborator

vukg commented Jul 23, 2021

/lgtm
one of very important practices that we for example enforce at admission

electrocucaracha
electrocucaracha reviewed Jul 23, 2021
View reviewed changes
Copy link
Collaborator

@electrocucaracha electrocucaracha left a comment

Choose a reason for hiding this comment

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

Are "no root" and "non-root" terms the same? If that's true the document uses those interchangeably so I'll be nice to clarify it.

cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
jeffsaelens
jeffsaelens reviewed Jul 26, 2021
View reviewed changes
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
@lixuna lixuna marked this pull request as draft July 26, 2021 16:47
@iawells @taylor
Remove 'compliance' as it's a loaded word
27bcf14 
Co-authored-by: Taylor Carpenter <taylor@vulk.coop>
iawells
iawells reviewed Jul 26, 2021
View reviewed changes
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
taylor and others added 2 commits July 26, 2021 13:08
@taylor @iawells
Update motivation for cbpps/0002-no-root-in-containers.md
9ef2f0a 
Co-authored-by: Ian Wells <iawells@cisco.com>
@iawells @jeffsaelens
Speeling mistak
367b327 
Co-authored-by: Jeffrey Saelens <nerdengineering@gmail.com>
agentpoyo
agentpoyo approved these changes Sep 30, 2021
View reviewed changes
Copy link
Collaborator

@agentpoyo agentpoyo left a comment

Choose a reason for hiding this comment

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

LGTM

michaelspedersen
michaelspedersen reviewed Sep 30, 2021
View reviewed changes
Copy link
Collaborator

@michaelspedersen michaelspedersen left a comment

Choose a reason for hiding this comment

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

Generally looks good, but I have a couple comments and questions as I am a little unclear about the use of "privileged" and "privileges" throughout the BP

cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
@taylor
Copy link
Member Author

taylor commented Sep 30, 2021

@pgoyal01 please review again. Your items have been resolved and user stories have been added.

ildikov
ildikov reviewed Sep 30, 2021
View reviewed changes
Copy link
Collaborator

@ildikov ildikov left a comment

Choose a reason for hiding this comment

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

Hi,

I have only one nit pick in line, please check.

This patch also seem to contain quite some unrelated changes. I don't know if that's because of a rebase or it's intentionally addressing multiple issues, it would be great to avoid this in the future if possible as it makes reviewing harder.

Thanks,
Ildikó

cbpps/0002-no-root-in-containers.md Outdated Show resolved Hide resolved
taylor and others added 5 commits September 30, 2021 15:43
@taylor
removed accidental comment from non-root best practice
7c5c61e
@taylor @pgoyal01
Update cbpps/0002-no-root-in-containers.md
ddb17a1 
Co-authored-by: Pankaj Goyal <52107136+pgoyal01@users.noreply.github.com>
@taylor
Update 0002-no-root-in-containers.md
a3568cc
@taylor @pgoyal01
Apply suggestions from code review
ebb4093 
typos and grammar from @pgoyal01

Co-authored-by: Pankaj Goyal <52107136+pgoyal01@users.noreply.github.com>
@taylor
Update 0002-no-root-in-containers.md
2afdfeb
@taylor
Update 0002-no-root-in-containers.md
e479a58 
capabilities instead of privileges to avoid confusion
@taylor taylor mentioned this pull request Sep 30, 2021
Merged
taylor referenced this pull request Sep 30, 2021
@taylor
Updating best practice user story index links
69d3879 
ref:
- https://github.com/cncf/cnf-wg/pull/182
- https://github.com/cncf/cnf-wg/pull/193
@taylor
Copy link
Member Author

taylor commented Sep 30, 2021

@ildikov

I have only one nit pick in line, please check.

fixed thanks.

This patch also seem to contain quite some unrelated changes. I don't know if that's because of a rebase or it's intentionally addressing multiple issues, it would be great to avoid this in the future if possible as it makes reviewing harder.

Yes, that was a rebase. I hoped to avoid updates that were already accepted. Unfortunately, we ended up with more changes showing :/

ildikov
ildikov approved these changes Sep 30, 2021
View reviewed changes
Copy link
Collaborator

@ildikov ildikov left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix, it looks good to me now.

I also think it might be worthwhile to merge this and have follow up PRs in case there are still other bits to fix to avoid it picking up more unrelated changes.

taylor and others added 2 commits October 1, 2021 07:51
@taylor
Update 0002-no-root-in-containers.md
9df082b
@taylor @iawells
@CsatariGergely @michaelspedersen
Adding supply chain attack user stories (#193)
d986488 
- added a set of supply chain attack stories
- reference these stories from the non-root best practice
* adding links to user stories
* Updating best practice user story index links
* Apply suggestions from code review

ref:
- https://github.com/cncf/cnf-wg/pull/182
- https://github.com/cncf/cnf-wg/pull/193

Sign-off: Taylor Carpenter <taylor@vulk.coop>
Co-Authored-by: Ian Wells <iawells@cisco.com>
Co-authored-by: Gergely Csatari <gergely.csatari@nokia.com>
Co-authored-by: Michael Sølvkær Pedersen <michaelx.pedersen@intel.com>
electrocucaracha
electrocucaracha requested changes Oct 4, 2021
View reviewed changes
user-stories/supply-chain-attacks.md Outdated Show resolved Hide resolved
user-stories/supply-chain-attacks.md Outdated Show resolved Hide resolved
@taylor @electrocucaracha
Apply suggestions from code review - typos
46c551f 
Co-authored-by: Victor Morales <v.morales@samsung.com>
@taylor taylor merged commit afbbfe2 into main Oct 4, 2021
@taylor taylor deleted the best-practice-no-root-in-containers branch October 4, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgoyal01 pgoyal01 pgoyal01 approved these changes

@michaelspedersen michaelspedersen michaelspedersen approved these changes

@xmulligan xmulligan xmulligan approved these changes

@denverwilliams denverwilliams denverwilliams approved these changes

@agentpoyo agentpoyo agentpoyo approved these changes

@ildikov ildikov ildikov approved these changes

@electrocucaracha electrocucaracha electrocucaracha approved these changes

@wvwatson wvwatson wvwatson approved these changes

@CsatariGergely CsatariGergely Awaiting requested review from CsatariGergely

@fkautz fkautz Awaiting requested review from fkautz

@nickolaev nickolaev Awaiting requested review from nickolaev

@TamasZsiros TamasZsiros Awaiting requested review from TamasZsiros

@tliron tliron Awaiting requested review from tliron

@vukg vukg Awaiting requested review from vukg

@iawells iawells Awaiting requested review from iawells

@jeffsaelens jeffsaelens Awaiting requested review from jeffsaelens

Assignees

@taylor taylor

@iawells iawells

Labels
best practice proposal best-practice documentation Improvements or additions to documentation use-case
Projects
None yet
Milestone
Q3 Best Practices
Development

Successfully merging this pull request may close these issues.