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

Run as non-privilege and non-root user #2037

Merged
merged 3 commits into from
Jun 29, 2022
Merged

Run as non-privilege and non-root user #2037

merged 3 commits into from
Jun 29, 2022

Conversation

hjiawei
Copy link
Contributor

@hjiawei hjiawei commented Jun 22, 2022

Description

This changeset set securityContext as non-privilege and non-root user
for IDS controller, prometheus, and kibana pods. It fixes an enterprise
component developmend issue on CIS profile hardened RKE2 cluster. When
the container is running as root or non-numeric user (nobody or kibana),
it failed non-root validation.

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

@marvin-tigera marvin-tigera added this to the v1.28.0 milestone Jun 22, 2022
@hjiawei hjiawei force-pushed the non-root-security-context branch 2 times, most recently from 1a69e50 to 4673e1a Compare June 22, 2022 06:24
@hjiawei hjiawei marked this pull request as ready for review June 22, 2022 16:01
@hjiawei hjiawei requested a review from a team as a code owner June 22, 2022 16:01
hjiawei
hjiawei commented Jun 22, 2022
View reviewed changes
pkg/render/common/securitycontext/security_context.go
// Non-system UID and GID range is normally from 1000 to 60000 (Debian derived systems define this
// in /etc/login.defs). On a normal Linux host, it is unlikely to have more than 10k non-system users.
// 10001 is chosen based on this assumption.
RunAsUserID int64 = 10001
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Preferably, I want this UID and GID consistent across all components. It is not possible to make the Operator only change because some components chown folders to a hard-coded UID in Dockerfile. When running as a different user, I got permission errors. 10001 is chosen for the reason mentioned in the comments. The UIDs we are currently using (999 or 1001 etc.) may not be desired because they will collide with system or non-system users. Do we want to make them consistent? @caseydavenport or @tmjd WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Consistency would be nice, but like you point out it would require some extra work. I don't think we have any special attachment to 999 or 1001, but there are probably other places where we (or users) make assumptions based on those, so I might be inclined to leave them as-is for now. I don't think we've had any reports of those values causing issues.

Maybe just make these configurable for now and each component can pass in the user / group that it expects to be run as, and we can then do a follow-up series of PRs to standardize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made uid and gid configurable in NewBaseContext. The UID and GID are taken from Dockerfiles (unchanged) for each component.

hjiawei
hjiawei commented Jun 22, 2022
View reviewed changes
pkg/render/common/podsecuritycontext/pod_security_context.go
@@ -1,15 +0,0 @@
package podsecuritycontext
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed. This is a SecurityContext not PodSecurityContext.

@caseydavenport
Copy link
Member

CC @mgleung

hjiawei
hjiawei commented Jun 24, 2022
View reviewed changes
pkg/render/intrusion_detection.go
@@ -532,7 +534,7 @@ func (c *intrusionDetectionComponent) intrusionDetectionControllerContainer() co
},
}

privileged := false
sc := securitycontext.NewBaseContext(securitycontext.RunAsUserID, securitycontext.RunAsGroupID)
Copy link
Contributor Author

@hjiawei hjiawei Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nothing is set in IDS controller Dockerfile so we are safe to set UID and GID to 10001. This is also fixed in IDS master as a fallback.

@hjiawei hjiawei force-pushed the non-root-security-context branch 5 times, most recently from 7d61dd8 to 79ef6c2 Compare June 25, 2022 06:02
@hjiawei
Copy link
Contributor Author

hjiawei commented Jun 25, 2022

Changes are tests on a CIS-1.6 profile hardened RKE2 cluster and normal GCP kubeadm cluster.

@hjiawei
Copy link
Contributor Author

hjiawei commented Jun 27, 2022

The UID/GID for cloud controller, dex, guardian, packetcapture are copied from Dockerfiles. I need these due to the interface change in NewBaseContext as suggested by @caseydavenport.

@rene-dekker
Copy link
Member

The PR lgtm, except that it would be good to have a check in each _test.go to verify the security context.

@hjiawei hjiawei force-pushed the non-root-security-context branch 3 times, most recently from ce743a2 to 8407442 Compare June 28, 2022 21:56
@hjiawei
Copy link
Contributor Author

hjiawei commented Jun 28, 2022

UTs for SecurityContext changes are added or updated in 8407442.

rene-dekker
rene-dekker approved these changes Jun 28, 2022
View reviewed changes
Copy link
Member

@rene-dekker rene-dekker left a comment

Choose a reason for hiding this comment

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

lgtm

@hjiawei hjiawei force-pushed the non-root-security-context branch 2 times, most recently from 8463de4 to 0ddc7be Compare June 29, 2022 15:40
@hjiawei
Run as non-privilege and non-root user
cae94b8 
This changeset set `securityContext` as non-privilege and non-root user
for IDS controller, prometheus, and kibana pods. It fixes an enterprise
component developmend issue on CIS profile hardened RKE2 cluster. When
the container is running as root or non-numeric user (nobody or kibana),
it failed non-root validation.
@hjiawei
Make UID and GID configurable in security context
2cb3055
@hjiawei
Update UTs for SecurityContext changes
d9f3110
@rene-dekker
Copy link
Member

/merge-when-ready delete-branch

@marvin-tigera
Copy link
Contributor

OK, I will merge the pull request when it's ready, leave the commits as is when I merge it, and delete the branch after I've merged it.

@rene-dekker rene-dekker reopened this Jun 29, 2022
@rene-dekker rene-dekker merged commit 5304d7b into tigera:master Jun 29, 2022
@hjiawei hjiawei mentioned this pull request Jun 29, 2022
Merged
5 tasks
@hjiawei hjiawei deleted the non-root-security-context branch June 29, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rene-dekker rene-dekker rene-dekker approved these changes

@caseydavenport caseydavenport Awaiting requested review from caseydavenport

Assignees
No one assigned
Labels
docs-pr-required release-note-required
Projects
None yet
Milestone
v1.28.0
Development

Successfully merging this pull request may close these issues.
4 participants
@hjiawei @caseydavenport @rene-dekker @marvin-tigera