-
Notifications
You must be signed in to change notification settings - Fork 886
stage1: implement no-new-privs linux isolator #2677
Conversation
func getAppNoNewPrivileges(isolators types.Isolators) (bool, error) { | ||
noNewPrivs := "" | ||
|
||
// TODO(sur): once https://github.com/appc/spec/pull/611 lands, bump appc spec, and use official appc types instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the purpose of testing before appc/spec#611 lands, you could have a commit in this PR that patches Godeps/_workspace/src/github.com/appc/spec/ with the patch in appc/spec#611.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good suggestion! :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done (temporarily)
@alban @jonboulle addresses review comments, PTAL |
@lucab PTAL |
Looks good overall. I see this and the appc spec default to false, making this security feature opt-in. I didn't find any reference to discussions about it: do we have plans for making this opt-out in the long term instead? |
@lucab There was no discussions about the default as far as I know. During the review of appc/spec#611, I asked that the spec should say it defaults to false so that the spec is clear, but I haven't considered about the reverse. I wonder about compatibility if images use setuid files: for example, Ubuntu has some:
|
yeah per my OP I wasn't sure whether this'd be generally safe or not. I suggest we keep it an image-settable feature for now and then we can consider making it a default opt-out later? |
uintptr(0), uintptr(0), | ||
) | ||
|
||
fmt.Printf("r1: %v err: %v\n", r1, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be printed in a more human-friendly way? e.g. "no_new_privs: 1" or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
4a870e1
to
7d8c0e9
Compare
Agreed that we should set this to |
@alban PTAL |
@s-urbaniak LGTM (after appc/spec has been bumped) |
@steveej @alban @jonboulle @iaguis PTAL |
@@ -406,6 +406,11 @@ | |||
"Rev": "a1b8ba5163b7f041b22761461eabd02b70d1f824" | |||
}, | |||
{ | |||
"ImportPath": "github.com/gogo/protobuf/proto", | |||
"Comment": "v0.1-125-g82d16f7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also updated by:
#2691, see:
+ {
+ "ImportPath": "github.com/gogo/protobuf/proto",
+ "Comment": "v0.2",
+ "Rev": "4168943e65a2802828518e95310aeeed6d84c4e5"
+ },
Which one should we take first? :)
It should also use a tagged version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the exact same version as appc/spec https://github.com/appc/spec/blob/v0.8.3/Godeps/Godeps.json#L15
Needs to be rebased on #2697. |
Fixes #1469