disallow implicit docker builds if user can't run normal docker builds#8664
disallow implicit docker builds if user can't run normal docker builds#8664bparees wants to merge 1 commit intoopenshift:masterfrom
Conversation
bparees
force-pushed
the
implicit_build
branch
5 times, most recently
from
5d33c3f to
4251157
Compare
|
@liggitt @csrwng @smarterclayton ok tests are still coming but i think this is ready for an implementation review. Note that as discussed with @liggitt there is still an exploit path here:
|
|
(and actually the way this is currently implemented, "oc start-build" would also work on step (4), because i'm not auditing the buildrequest object itself. |
| // ForcePull describes if the builder should pull the images from registry prior to building. | ||
| ForcePull bool `json:"forcePull,omitempty"` | ||
|
|
||
| // disableImplicitBuild prevents source-to-image from performing a Docker build operation |
There was a problem hiding this comment.
s/disableImplicitBuild/DisableImplicitBuild/
There was a problem hiding this comment.
my understanding from @smarterclayton is that this is how we're supposed to do it now so it matches the json field name.
There was a problem hiding this comment.
ic ... good to know... then the other fields are wrong :)
There was a problem hiding this comment.
yeah, @gabemontero already has a pull open that's updating them all though:
https://github.com/openshift/origin/pull/8477/files#diff-43dd40b4c2b85b388a3d9116729032e4L14
There was a problem hiding this comment.
I can't think of a single reason, other than security, that a user would
ever select DisableImplicitBuild themselves.
On Thu, Apr 28, 2016 at 2:14 PM, Ben Parees notifications@github.com
wrote:
In pkg/build/api/v1/types.go
#8664 (comment):@@ -402,6 +402,11 @@ type SourceBuildStrategy struct {
// ForcePull describes if the builder should pull the images from registry prior to building. ForcePull bool `json:"forcePull,omitempty"`
- // disableImplicitBuild prevents source-to-image from performing a Docker build operation
yeah, @gabemontero https://github.com/gabemontero already has a pull
open that's updating them all though:https://github.com/openshift/origin/pull/8477/files#diff-43dd40b4c2b85b388a3d9116729032e4L14
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8664/files/4251157e4e6c6d4b865d530bb0de37eb6fd75b3a#r61476003
There was a problem hiding this comment.
@csrwng suggested moving it to an annotation. I can do that, but i'd like to talk through this a little more before I do..I kinda hate the number of magic annotations we are accumulating and it makes the implementation code uglier. Our APIs serve two customers: our users, and ourselves. Making our code more complex (and less performant) impacts those users too, and per above, there is a(weak) case for why this is useful to an end user anyway.
There was a problem hiding this comment.
Why do you need to do it on a per user basis? Adding something to the API
is burdensome, it doesn't match cleanly to actual security requirements,
and it's just adding the wrong sorts of complexity. If we had proper
network support then this would not be an issue.
I'm not in favor of opening it up as an API field.
On Thu, Apr 28, 2016 at 2:21 PM, Ben Parees notifications@github.com
wrote:
In pkg/build/api/v1/types.go
#8664 (comment):@@ -402,6 +402,11 @@ type SourceBuildStrategy struct {
// ForcePull describes if the builder should pull the images from registry prior to building. ForcePull bool `json:"forcePull,omitempty"`
- // disableImplicitBuild prevents source-to-image from performing a Docker build operation
@smarterclayton https://github.com/smarterclayton fair enough, but I
want to be able to control this behavior on a per user basis so we need
some way to convey to s2i what is/isn't allowed. I'm open to suggestions,
but I started by putting it on the build pod directly until I realized the
build pods are managed by the controller which means they can't check
permissions in a useful way.—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8664/files/4251157e4e6c6d4b865d530bb0de37eb6fd75b3a#r61477153
There was a problem hiding this comment.
Basically - you need to make a stronger case for why an end user wants this.
On Thu, Apr 28, 2016 at 4:04 PM, Clayton Coleman ccoleman@redhat.com
wrote:
Why do you need to do it on a per user basis? Adding something to the API
is burdensome, it doesn't match cleanly to actual security requirements,
and it's just adding the wrong sorts of complexity. If we had proper
network support then this would not be an issue.I'm not in favor of opening it up as an API field.
On Thu, Apr 28, 2016 at 2:21 PM, Ben Parees notifications@github.com
wrote:In pkg/build/api/v1/types.go
#8664 (comment):@@ -402,6 +402,11 @@ type SourceBuildStrategy struct {
// ForcePull describes if the builder should pull the images from registry prior to building.
ForcePull booljson:"forcePull,omitempty"
+
- // disableImplicitBuild prevents source-to-image from performing a Docker build operation
@smarterclayton https://github.com/smarterclayton fair enough, but I
want to be able to control this behavior on a per user basis so we need
some way to convey to s2i what is/isn't allowed. I'm open to suggestions,
but I started by putting it on the build pod directly until I realized the
build pods are managed by the controller which means they can't check
permissions in a useful way.—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8664/files/4251157e4e6c6d4b865d530bb0de37eb6fd75b3a#r61477153
There was a problem hiding this comment.
An end user wants this so they are not blocked from doing this if their admin trusts them but is not willing to open it up to the entire cluster.
And an end user wants this if they want to protect themselves from having to inspect every s2i builder image to see if it contains rogue ONBUILD commands that amount to trojan horse exploits.
There was a problem hiding this comment.
in other words, being able to disable the implicit build behavior of s2i is absolutely an end user concern.
| // prevent s2i from doing an implicit docker build during the s2i build process. | ||
| // Mutates strategy argument. | ||
| func (a *buildByStrategy) setImplicitBuildPermission(strategy *buildapi.BuildStrategy, attr admission.Attributes) error { | ||
| if strategy.SourceStrategy != nil { |
There was a problem hiding this comment.
you may want to avoid the check if DisableImplicitBuild is already true
|
@csrwng comments addressed, test cases added, ptal |
|
Evaluated for origin test up to af3ff6f |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3420/) |
|
End users only select s2i images, they are not responsible for creating On Thu, Apr 28, 2016 at 4:20 PM, OpenShift Bot notifications@github.com
|
|
I'm pretty sure that disabling implicit build behavior is going to break On Thu, Apr 28, 2016 at 4:27 PM, Clayton Coleman ccoleman@redhat.com
|
4 participants
this is probably going away in deference to #8679