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

add warning about root user in images #6414

Merged
merged 1 commit into from
Jan 8, 2016
Merged

add warning about root user in images #6414

merged 1 commit into from
Jan 8, 2016

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Dec 18, 2015

addresses part of #6348

pweil-
pweil- reviewed Dec 18, 2015
View reviewed changes
pkg/generate/app/cmd/newapp.go
@@ -498,7 +498,10 @@ func (c *AppConfig) inferBuildTypes(components app.ComponentReferences) (app.Com
case input.ResolvedMatch.Score != 0.0:
errs = append(errs, fmt.Errorf("component %q had only a partial match of %q - if this is the value you want to use, specify it explicitly", input.From, input.ResolvedMatch.Name))
continue
case input.ResolvedMatch.Image != nil && (len(input.ResolvedMatch.Image.Config.User) == 0 || input.ResolvedMatch.Image.Config.User == "root"):
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also check for the value "0" here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call.

@bparees bparees force-pushed the user_check branch 4 times, most recently from 62b5053 to ac83a8a Compare December 18, 2015 19:40
@bparees
Copy link
Contributor Author

bparees commented Dec 18, 2015

@smarterclayton ptal.
[test]

@bparees bparees mentioned this pull request Dec 18, 2015
Closed
smarterclayton
smarterclayton reviewed Dec 18, 2015
View reviewed changes
pkg/generate/app/cmd/newapp.go
@@ -498,6 +498,8 @@ func (c *AppConfig) inferBuildTypes(components app.ComponentReferences) (app.Com
case input.ResolvedMatch.Score != 0.0:
errs = append(errs, fmt.Errorf("component %q had only a partial match of %q - if this is the value you want to use, specify it explicitly", input.From, input.ResolvedMatch.Name))
continue
case input.ResolvedMatch.Image != nil && (len(input.ResolvedMatch.Image.Config.User) == 0 || input.ResolvedMatch.Image.Config.User == "root" || input.ResolvedMatch.Image.Config.User == "0"):
glog.Warningf("Image %q runs as the 'root' user which may not be permitted by your OpenShift cluster configuration", input.ResolvedMatch.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't say OpenShift, this could be in AEP. "may be restricted by your cluster administrator". I think we might want to mention SCCs somehow - hard to say for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm - this shouldn't be here - this should be in the describe function.

Copy link
Contributor

Choose a reason for hiding this comment

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

We stopped printing these errors here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't call describe when dumping yaml, so you don't want a warning if they are outputting to yaml/json?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope.

@bparees
Copy link
Contributor Author

bparees commented Dec 19, 2015

@smarterclayton well i moved it to describe, but you're going to get a warning under the following condition:

you specify a docker type build with an image that "runs as root" and your dockerfile for the build sets the USER to non-root

meaning the image that actually gets deployed isn't going to be running as root.

@bparees
Copy link
Contributor Author

bparees commented Dec 20, 2015

[test]

@bparees
Copy link
Contributor Author

bparees commented Jan 6, 2016

@smarterclayton @csrwng ptal.

csrwng
csrwng reviewed Jan 6, 2016
View reviewed changes
pkg/generate/app/cmd/describe.go
@@ -143,6 +143,9 @@ func describeBuildPipelineWithImage(out io.Writer, ref app.ComponentReference, p
fmt.Fprintf(out, " * This image declares volumes and will default to use non-persistent, host-local storage.\n")
fmt.Fprintf(out, " You can add persistent volumes later by running 'volume dc/%s --add ...'\n", pipeline.Deployment.Name)
}
if pipeline.Image.Info != nil && (len(pipeline.Image.Info.Config.User) == 0 || pipeline.Image.Info.Config.User == "root" || pipeline.Image.Info.Config.User == "0") {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to give you a separate warning text for len(user) == 0 (technically it's not the same as specifying user root)... and given that we'll run you with an arbitrary user, maybe it's what's preferred?
Also if you do specify a specific user ... say 1001. Do we want to let you know that we won't run you with the user specified in your image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm going to remove the len==0 check, not sure why i put that in, as you say, it doesn't necessarily mean root.

and no, we don't want a warning for if they specified a specific(non-root) user. I would assume (almost?) every image specifies a specific user, either root or something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check re-added per discussion.

@csrwng
Copy link
Contributor

csrwng commented Jan 6, 2016

LGTM

@bparees
Copy link
Contributor Author

bparees commented Jan 6, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to d44d1bb

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8221/)

@bparees
Copy link
Contributor Author

bparees commented Jan 7, 2016

[merge]

3 similar comments
@bparees
Copy link
Contributor Author

bparees commented Jan 7, 2016

[merge]

@bparees
Copy link
Contributor Author

bparees commented Jan 7, 2016

[merge]

@bparees
Copy link
Contributor Author

bparees commented Jan 7, 2016

[merge]

@bparees
Copy link
Contributor Author

bparees commented Jan 7, 2016

[merge]
going for a record

@bparees
Copy link
Contributor Author

bparees commented Jan 7, 2016

[merge]

1 similar comment
@bparees
Copy link
Contributor Author

bparees commented Jan 8, 2016

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8221/) (Image: devenv-rhel7_3101)

@bparees
Copy link
Contributor Author

bparees commented Jan 8, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to d44d1bb

openshift-bot pushed a commit that referenced this pull request Jan 8, 2016
Merge pull request #6414 from bparees/user_check
205d716 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 205d716 into openshift:master Jan 8, 2016
@bparees bparees deleted the user_check branch January 17, 2016 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bparees @csrwng @openshift-bot @smarterclayton @pweil-