Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces/seccomp: add bind to default seccomp template for hooks #3091
Conversation
| @@ -500,3 +500,10 @@ pwritev | ||
| # of socket(), bind(), connect(), etc individually. | ||
| socketcall | ||
| `) | ||
| + | ||
| +// defaultHookTemplate contains default seccomp template for hooks. | ||
| +// It can be overridden for testing using MockTemplate(). |
zyga
Mar 28, 2017
Contributor
Please add rationale for this and reference the golang bug and the seccomp bug report
| } | ||
| return content, nil | ||
| } | ||
| -func addContent(securityTag string, opts interfaces.ConfinementOptions, snippetForTag string, content map[string]*osutil.FileState) { | ||
| +func addContent(securityTag string, isHook bool, opts interfaces.ConfinementOptions, snippetForTag string, content map[string]*osutil.FileState) { |
mvo5
Mar 28, 2017
Collaborator
My personal pet-peeve: I don't like bool in a function signature. I much prefer flags, even if its just a single option for these flags. I think bool makes it harerd to read, e.g: addContext(securityTag, false..) vs addContext(securityTag, isHook|needsJuice...).
zyga
changed the title from
interfaces/seccomp: add bind as part of the default seccomp policy for hooks
to
interfaces/seccomp: add bind to default seccomp template for hooks
Mar 28, 2017
zyga
requested a review
from
jdstrand
Mar 28, 2017
Some checks were not successful
zyga
added this to the
2.23.6 milestone
Mar 28, 2017
niemeyer
merged commit 6cbfe15
into
snapcore:master
Mar 28, 2017
4 of 6 checks passed
|
I guess was fine to commit, but it may mask the bigger problem that is https://bugs.launchpad.net/snapd/+bug/1675812. I think what is happening is that when only seccomp is enabled, /run/snapd.socket is not blocked and then the bind() is needed. This means that the configure hook is talking to /run/snapd.socket when it seems it should instead be talking to /run/snap-snapd.socket. If this is all true, then it seems the correct fix is instead to fix https://bugs.launchpad.net/snapd/+bug/1675812 and adjust snapctl to use /run/snapd-snap.socket by default (which doesn't seem to need to use the bind()). Someone from the snappy team should confirm this. |
added a commit
to zyga/snapd
that referenced
this pull request
Mar 28, 2017
zyga
referenced this pull request
Mar 28, 2017
Closed
interfaces/seccomp: add bind as part of the default seccomp policy (backport) #3097
|
@mvo5 - yes, that looks great and should fix https://bugs.launchpad.net/snapd/+bug/1675812 and make sure that the correct socket is used regardless of confinement. Does it fix the bind? If not, that's ok; with #3098 this merged PR is fine (though we may want to tweak the comment to be less workaround-y and more 'this is what we need'). If it does fix the bind when using snapctl, this seems we can revert this PR and just commit/backport to 2.23 #3098. |
added a commit
to mvo5/snappy
that referenced
this pull request
Mar 29, 2017
morphis commentedMar 28, 2017
•
Edited 1 time
-
morphis
Mar 28, 2017
This is have a mid term for for LP #1674193 which occurs when AppArmor confinement is disabled and Seccomp is enabled. See also the other referenced bugs in the bug report for more details.