Prevent Source builders from running as root #4025
Conversation
|
@smarterclayton ptal - this is WIP. I've been trying a few things with this one, and without requiring upstream changes to the SCC code, this is the best I have so far. Would like your input/ideas. |
|
@pweil- ptal |
| // AllowedUIDs is a list of user ranges of users allowed to run the builder image. | ||
| // If a range is specified and the builder image uses a non-numeric user or a user | ||
| // that is outside the specified range, then the build fails. | ||
| AllowedUIDs user.RangeList |
There was a problem hiding this comment.
can this express "anything other than 0"?
There was a problem hiding this comment.
Yes, it can have a list of ranges like 10-20,30,50,1024-. Ideally we'd be able to ask SCC what UID ranges are allowed for that user/sa and pass that on to the STI builder. But the way it is right now, I can only ask whether a particular user is allowed. @pweil - any thoguths?
There was a problem hiding this comment.
We could probably separate out some of the logic in the admission controller to an SCC api to help with calls like this. The relevant components are the total set of SCCs and the getMatchingSecurityContextConstraints method in admission which would provide the applicable sets. That could then be passed to something that would return the blocks of allowable uids supported.
There was a problem hiding this comment.
@pweil- Until we can get the range of users from SCCs, is this an acceptable approach to at least preventing the STI builder from running images that could potentially run as root?
There was a problem hiding this comment.
Yes, I believe this will be sufficient. If I'm understanding the sequence from the code below it is basically
- See if the service account has access to run build pods as root by trying to admit a pod with runAsUser forced to 0
- If the above fails set a range of anything except 0 with 1-
- The STI builder checks the build image to prevent it from running if the uid is not in the range set by the env var.
There was a problem hiding this comment.
Basically yes
On Fri, Aug 7, 2015 at 5:03 PM, Paul Weil notifications@github.com wrote:
In
Godeps/_workspace/src/github.com/openshift/source-to-image/pkg/api/types.go
#4025 (comment):@@ -91,6 +96,11 @@ type Config struct {
// Specify a relative directory inside the application repository that should
// be used as a root directory for the application.
ContextDir string
+
- // AllowedUIDs is a list of user ranges of users allowed to run the builder image.
- // If a range is specified and the builder image uses a non-numeric user or a user
- // that is outside the specified range, then the build fails.
- AllowedUIDs user.RangeList
Yes, I believe this will be sufficient. If I'm understanding the sequence
from the code below it is basically
- See if the service account has access to run build pods as root by
trying to admit a pod with runAsUser forced to 0- If the above fails set a range of anything except 0 with 1-
- The STI builder checks the build image to prevent it from running
if the uid is not in the range set by the env var.—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/4025/files#r36561052.
csrwng
force-pushed
the
prevent_s2i_root
branch
2 times, most recently
from
19e3af5
to
e5bf05d
Compare
|
Depends on #4124 |
csrwng
changed the title
|
[test] |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/4201/) |
|
@pweil- the s2i part is now in a separate PR (that this one depends on). Please let me know if you have any additional comments/questions on the origin commit. |
|
Cool. Other changes look fine to me. Thanks @csrwng |
csrwng
force-pushed
the
prevent_s2i_root
branch
from
e5bf05d
to
9246794
Compare
|
@smarterclayton any comments? |
| @@ -74,6 +74,13 @@ func (s *STIBuilder) Build() error { | |||
| } else if s.build.Spec.Source.Git.Ref != "" { | |||
| config.Ref = s.build.Spec.Source.Git.Ref | |||
| } | |||
| glog.Infof("The value of ALLOWED_UIDS is [%s]", os.Getenv("ALLOWED_UIDS")) | |||
There was a problem hiding this comment.
lower(higher) log level? i don't think we need to print this on every run
There was a problem hiding this comment.
also i'd just get it once and use it for the log and the if.
csrwng
force-pushed
the
prevent_s2i_root
branch
from
9246794
to
9b6a18d
Compare
|
thx @bparees - updates made |
|
Evaluated for origin test up to 9b6a18d |
|
lgtm. |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3008/) (Image: devenv-fedora_2173) |
|
trying again... seems an issue with ui e2e |
|
[merge] |
|
Evaluated for origin merge up to 9b6a18d |
Merged by openshift-bot
Adds SCC checking to Source build controller strategy to prevent builds from running as root.