RFC: libcontainer: remove dependency on libapparmor #1675
Conversation
tklauser
changed the title
|
As noted in #1674, I did not fully test this yet. I'm trying to get runc running with an apparmor profile to test this first. |
libcontainer/apparmor/apparmor.go
Outdated
|
|
||
| // mimick aa_change_onexec from libapparmor in Go | ||
| value := "exec " + name | ||
| if err := setprocattr(unix.Gettid(), "exec", value); err != nil { |
There was a problem hiding this comment.
This looks /okay/ overall, but I would prefer if we had a function called ChangeOnExec (or whatever) which wraps these two lines -- to make it clearer what we are mimicing.
There was a problem hiding this comment.
Agree. Does it need to be exported or could it also be changeOnExec?
There was a problem hiding this comment.
Yeah, there's no need to export it I guess.
| _, err = fmt.Fprintf(f, "%s", value) | ||
| return err | ||
| } | ||
|
|
||
| // ApplyProfile will apply the profile with the specified name to the process after | ||
| // the next exec. | ||
| func ApplyProfile(name string) error { | ||
| if name == "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Should check !strings.Contains(name, "/")
There was a problem hiding this comment.
Why? Profile names can have / in them IIRC (and they can also not have / in them as well).
There was a problem hiding this comment.
Would something like the following be sufficient?
if strings.Contains(name, "/") {
return fmt.Errorf("apparmor profile name cannot contain '/'")
}
There was a problem hiding this comment.
At least ".." should be prohibited to prevent potential filesystem contamination
|
This looks fine. I will test it though (though I imagine it will work fine -- the only really ugly parts of the AppArmor API are related to profile loading and unloading). |
libcontainer/apparmor/apparmor.go
Outdated
| @@ -24,16 +21,34 @@ func IsEnabled() bool { | |||
| return false | |||
| } | |||
|
|
|||
| func setprocattr(tid int, attr, value string) error { | |||
| path := fmt.Sprintf("/proc/%d/attr/%s", tid, attr) | |||
There was a problem hiding this comment.
While this works, I think this should be /proc/self/attr/%s. Under AppArmor you can only change your own attr, so I think /proc/self makes more sense than providing a tid argument.
libapparmor is integrated in libcontainer using cgo but is only used to call a single function: aa_change_onexec. It turns out this function is simple enough (writing a string to a file in /proc/<n>/attr/...) to be re-implemented locally in libcontainer in plain Go. This allows to drop the dependency on libapparmor and the corresponding cgo integration. Fixes #1674 Signed-off-by: Tobias Klauser <[email protected]>
|
LGTM. Tested this with a few profiles and they are applied properly. |
|
LGTM |
libapparmor is integrated in libcontainer using cgo but is only used to
call a single function: aa_change_onexec. It turns out this function is
simple enough (writing a string to a file in /proc//attr/...) to be
re-implemented locally in libcontainer in plain Go.
This allows to drop the dependency on libapparmor and the corresponding
cgo integration.
Fixes #1674
Signed-off-by: Tobias Klauser [email protected]