From cb5ed1ce9d6832d8df4dc524db7512f12fc51132 Mon Sep 17 00:00:00 2001 From: Xianglin Gao Date: Wed, 7 Dec 2016 19:32:50 +0800 Subject: [PATCH 1/3] reload default apparmor profile if it is unloaded Signed-off-by: Xianglin Gao --- server/apparmor/apparmor.go | 16 +++---- server/apparmor/apparmor_unsupported.go | 7 ++- server/container_create.go | 5 ++ server/server.go | 2 +- test/apparmor.bats | 62 ++++++++++++++++++++----- test/helpers.bash | 10 ++-- test/seccomp.bats | 40 ++++++++-------- test/testdata/fake_ocid_default | 1 + 8 files changed, 95 insertions(+), 48 deletions(-) create mode 100644 test/testdata/fake_ocid_default diff --git a/server/apparmor/apparmor.go b/server/apparmor/apparmor.go index 32c778c1..824be5ec 100644 --- a/server/apparmor/apparmor.go +++ b/server/apparmor/apparmor.go @@ -17,8 +17,8 @@ import ( ) const ( - // defaultApparmorProfile is the name of default apparmor profile name. - defaultApparmorProfile = "ocid-default" + // DefaultApparmorProfile is the name of default apparmor profile name. + DefaultApparmorProfile = "ocid-default" // profileDirectory is the file store for apparmor profiles and macros. profileDirectory = "/etc/apparmor.d" @@ -47,13 +47,11 @@ type profileData struct { Version int } -// InstallDefaultAppArmorProfile installs default apparmor profile. -func InstallDefaultAppArmorProfile() { - if err := InstallDefault(defaultApparmorProfile); err != nil { - // Allow daemon to run if loading failed, but are active - // (possibly through another run, manually, or via system startup) - if !IsLoaded(defaultApparmorProfile) { - logrus.Errorf("AppArmor enabled on system but the %s profile could not be loaded.", defaultApparmorProfile) +// LoadDefaultAppArmorProfile loads default apparmor profile, if it is not loaded. +func LoadDefaultAppArmorProfile() { + if !IsLoaded(DefaultApparmorProfile) { + if err := InstallDefault(DefaultApparmorProfile); err != nil { + logrus.Errorf("AppArmor enabled on system but the %s profile could not be loaded:%v", DefaultApparmorProfile, err) } } } diff --git a/server/apparmor/apparmor_unsupported.go b/server/apparmor/apparmor_unsupported.go index ea9b6d08..c98e6dc7 100644 --- a/server/apparmor/apparmor_unsupported.go +++ b/server/apparmor/apparmor_unsupported.go @@ -3,6 +3,9 @@ package apparmor const ( + // DefaultApparmorProfile is the name of default apparmor profile name. + DefaultApparmorProfile = "ocid-default" + // ContainerAnnotationKeyPrefix is the prefix to an annotation key specifying a container profile. ContainerAnnotationKeyPrefix = "container.apparmor.security.beta.kubernetes.io/" @@ -17,8 +20,8 @@ func IsEnabled() bool { return false } -// InstallDefaultAppArmorProfile dose nothing, when build without apparmor build tag. -func InstallDefaultAppArmorProfile() { +// LoadDefaultAppArmorProfile dose nothing, when build without apparmor build tag. +func LoadDefaultAppArmorProfile() { } // GetProfileNameFromPodAnnotations dose nothing, when build without apparmor build tag. diff --git a/server/container_create.go b/server/container_create.go index f0cf96d2..ed7faa7c 100644 --- a/server/container_create.go +++ b/server/container_create.go @@ -393,6 +393,11 @@ func (s *Server) getAppArmorProfileName(annotations map[string]string, ctrName s } if profile == apparmor.ProfileRuntimeDefault { + // reload default apparmor profile if it is unloaded. + if s.appArmorProfile == apparmor.DefaultApparmorProfile { + apparmor.LoadDefaultAppArmorProfile() + } + // If the value is runtime/default, then return default profile. return s.appArmorProfile } diff --git a/server/server.go b/server/server.go index 317b9499..42d7fcac 100644 --- a/server/server.go +++ b/server/server.go @@ -299,7 +299,7 @@ func New(config *Config) (*Server, error) { s.seccompProfile = seccompConfig if s.appArmorEnabled { - apparmor.InstallDefaultAppArmorProfile() + apparmor.LoadDefaultAppArmorProfile() } s.appArmorProfile = config.ApparmorProfile diff --git a/test/apparmor.bats b/test/apparmor.bats index d4300c92..94bc3de4 100644 --- a/test/apparmor.bats +++ b/test/apparmor.bats @@ -15,8 +15,8 @@ function teardown() { fi # this test requires apparmor, so skip this test if apparmor is not enabled. - enabled=is_apparmor_enabled - if [[ "$enabled" = "0" ]]; then + enabled=$(is_apparmor_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since apparmor is not enabled." fi @@ -52,12 +52,12 @@ function teardown() { fi # this test requires apparmor, so skip this test if apparmor is not enabled. - enabled=is_apparmor_enabled - if [[ "$enabled" -eq "0" ]]; then + enabled=$(is_apparmor_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since apparmor is not enabled." fi - load_apparmor_test_profile + load_apparmor_profile "$APPARMOR_TEST_PROFILE_PATH" start_ocid "" "$APPARMOR_TEST_PROFILE_NAME" sed -e 's/%VALUE%/,"container\.apparmor\.security\.beta\.kubernetes\.io\/testname2": "apparmor-test-deny-write"/g' "$TESTDATA"/sandbox_config_seccomp.json > "$TESTDIR"/apparmor2.json @@ -79,7 +79,7 @@ function teardown() { cleanup_ctrs cleanup_pods stop_ocid - remove_apparmor_test_profile + remove_apparmor_profile "$APPARMOR_TEST_PROFILE_PATH" } # 3. test running with loading a specific apparmor profile but not as ocid default apparmor profile. @@ -91,12 +91,12 @@ function teardown() { fi # this test requires apparmor, so skip this test if apparmor is not enabled. - enabled=is_apparmor_enabled - if [[ "$enabled" -eq "0" ]]; then + enabled=$(is_apparmor_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since apparmor is not enabled." fi - load_apparmor_test_profile + load_apparmor_profile "$APPARMOR_TEST_PROFILE_PATH" start_ocid sed -e 's/%VALUE%/,"container\.apparmor\.security\.beta\.kubernetes\.io\/testname3": "apparmor-test-deny-write"/g' "$TESTDATA"/sandbox_config_seccomp.json > "$TESTDIR"/apparmor3.json @@ -118,7 +118,7 @@ function teardown() { cleanup_ctrs cleanup_pods stop_ocid - remove_apparmor_test_profile + remove_apparmor_profile "$APPARMOR_TEST_PROFILE_PATH" } # 4. test running with wrong apparmor profile name. @@ -130,8 +130,8 @@ function teardown() { fi # this test requires apparmor, so skip this test if apparmor is not enabled. - enabled=is_apparmor_enabled - if [[ "$enabled" -eq "0" ]]; then + enabled=$(is_apparmor_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since apparmor is not enabled." fi @@ -149,6 +149,44 @@ function teardown() { [[ "$output" =~ "Creating container failed" ]] + cleanup_ctrs + cleanup_pods + stop_ocid +} + +# 5. test running with default apparmor profile unloaded. +# test that we can will fail when running a ctr with rong apparmor profile name. +@test "run a container after unloading default apparmor profile" { + # this test requires docker, thus it can't yet be run in a container + if [ "$TRAVIS" = "true" ]; then # instead of $TRAVIS, add a function is_containerized to skip here + skip "cannot yet run this test in a container, use sudo make localintegration" + fi + + # this test requires apparmor, so skip this test if apparmor is not enabled. + enabled=$(is_apparmor_enabled) + if [[ "$enabled" -eq 0 ]]; then + skip "skip this test since apparmor is not enabled." + fi + + start_ocid + remove_apparmor_profile "$FAKE_OCID_DEFAULT_PROFILE_PATH" + + sed -e 's/%VALUE%/,"container\.apparmor\.security\.beta\.kubernetes\.io\/testname5": "runtime\/default"/g' "$TESTDATA"/sandbox_config_seccomp.json > "$TESTDIR"/apparmor5.json + + run ocic pod create --name apparmor5 --config "$TESTDIR"/apparmor5.json + echo "$output" + [ "$status" -eq 0 ] + pod_id="$output" + run ocic ctr create --name testname5 --config "$TESTDATA"/container_redis.json --pod "$pod_id" + echo "$output" + [ "$status" -eq 0 ] + ctr_id="$output" + [ "$status" -eq 0 ] + run ocic ctr execsync --id "$ctr_id" touch test.txt + echo "$output" + [ "$status" -eq 0 ] + + cleanup_ctrs cleanup_pods stop_ocid diff --git a/test/helpers.bash b/test/helpers.bash index 3d61d1a5..a4c32108 100644 --- a/test/helpers.bash +++ b/test/helpers.bash @@ -28,6 +28,8 @@ RUNC_BINARY=${RUNC_PATH:-/usr/local/sbin/runc} APPARMOR_PARSER_BINARY=${APPARMOR_PARSER_BINARY:-/sbin/apparmor_parser} # Path of the apparmor profile for test. APPARMOR_TEST_PROFILE_PATH=${APPARMOR_TEST_PROFILE_PATH:-${TESTDATA}/apparmor_test_deny_write} +# Path of the apparmor profile for unloading ocid-default. +FAKE_OCID_DEFAULT_PROFILE_PATH=${FAKE_OCID_DEFAULT_PROFILE_PATH:-${TESTDATA}/fake_ocid_default} # Name of the apparmor profile for test. APPARMOR_TEST_PROFILE_NAME=${APPARMOR_TEST_PROFILE_NAME:-apparmor-test-deny-write} # Path of boot config. @@ -148,12 +150,12 @@ function cleanup_test() { } -function load_apparmor_test_profile() { - "$APPARMOR_PARSER_BINARY" -r "$APPARMOR_TEST_PROFILE_PATH" +function load_apparmor_profile() { + "$APPARMOR_PARSER_BINARY" -r "$1" } -function remove_apparmor_test_profile() { - "$APPARMOR_PARSER_BINARY" -R "$APPARMOR_TEST_PROFILE_PATH" +function remove_apparmor_profile() { + "$APPARMOR_PARSER_BINARY" -R "$1" } function is_seccomp_enabled() { diff --git a/test/seccomp.bats b/test/seccomp.bats index d7aed036..8b26d729 100644 --- a/test/seccomp.bats +++ b/test/seccomp.bats @@ -15,8 +15,8 @@ function teardown() { fi # this test requires seccomp, so skip this test if seccomp is not enabled. - enabled=is_seccomp_enabled - if [[ "$enabled" =~ "0" ]]; then + enabled=$(is_seccomp_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since seccomp is not enabled." fi @@ -56,8 +56,8 @@ function teardown() { fi # this test requires seccomp, so skip this test if seccomp is not enabled. - enabled=is_seccomp_enabled - if [[ "$enabled" =~ "0" ]]; then + enabled=$(is_seccomp_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since seccomp is not enabled." fi @@ -97,8 +97,8 @@ function teardown() { fi # this test requires seccomp, so skip this test if seccomp is not enabled. - enabled=is_seccomp_enabled - if [[ "$enabled" =~ "0" ]]; then + enabled=$(is_seccomp_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since seccomp is not enabled." fi @@ -133,8 +133,8 @@ function teardown() { fi # this test requires seccomp, so skip this test if seccomp is not enabled. - enabled=is_seccomp_enabled - if [[ "$enabled" =~ "0" ]]; then + enabled=$(is_seccomp_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since seccomp is not enabled." fi @@ -158,8 +158,8 @@ function teardown() { fi # this test requires seccomp, so skip this test if seccomp is not enabled. - enabled=is_seccomp_enabled - if [[ "$enabled" =~ "0" ]]; then + enabled=$(is_seccomp_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since seccomp is not enabled." fi @@ -202,8 +202,8 @@ function teardown() { fi # this test requires seccomp, so skip this test if seccomp is not enabled. - enabled=is_seccomp_enabled - if [[ "$enabled" =~ "0" ]]; then + enabled=$(is_seccomp_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since seccomp is not enabled." fi @@ -243,8 +243,8 @@ function teardown() { fi # this test requires seccomp, so skip this test if seccomp is not enabled. - enabled=is_seccomp_enabled - if [[ "$enabled" =~ "0" ]]; then + enabled=$(is_seccomp_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since seccomp is not enabled." fi @@ -284,8 +284,8 @@ function teardown() { fi # this test requires seccomp, so skip this test if seccomp is not enabled. - enabled=is_seccomp_enabled - if [[ "$enabled" =~ "0" ]]; then + enabled=$(is_seccomp_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since seccomp is not enabled." fi @@ -325,8 +325,8 @@ function teardown() { fi # this test requires seccomp, so skip this test if seccomp is not enabled. - enabled=is_seccomp_enabled - if [[ "$enabled" =~ "0" ]]; then + enabled=$(is_seccomp_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since seccomp is not enabled." fi @@ -362,8 +362,8 @@ function teardown() { fi # this test requires seccomp, so skip this test if seccomp is not enabled. - enabled=is_seccomp_enabled - if [[ "$enabled" =~ "0" ]]; then + enabled=$(is_seccomp_enabled) + if [[ "$enabled" -eq 0 ]]; then skip "skip this test since seccomp is not enabled." fi diff --git a/test/testdata/fake_ocid_default b/test/testdata/fake_ocid_default new file mode 100644 index 00000000..9c6e6b84 --- /dev/null +++ b/test/testdata/fake_ocid_default @@ -0,0 +1 @@ +profile ocid-default flags=(attach_disconnected) {} From 6977b3e88d5ebb21504284088e4c8cafa06de910 Mon Sep 17 00:00:00 2001 From: Xianglin Gao Date: Wed, 7 Dec 2016 20:46:38 +0800 Subject: [PATCH 2/3] move duplicated consts to apparmor_common.go Signed-off-by: Xianglin Gao --- server/apparmor/apparmor_common.go | 14 ++++++++++++++ .../{apparmor.go => apparmor_supported.go} | 11 ----------- server/apparmor/apparmor_unsupported.go | 13 ------------- 3 files changed, 14 insertions(+), 24 deletions(-) create mode 100644 server/apparmor/apparmor_common.go rename server/apparmor/{apparmor.go => apparmor_supported.go} (86%) diff --git a/server/apparmor/apparmor_common.go b/server/apparmor/apparmor_common.go new file mode 100644 index 00000000..43670865 --- /dev/null +++ b/server/apparmor/apparmor_common.go @@ -0,0 +1,14 @@ +package apparmor + +const ( + // DefaultApparmorProfile is the name of default apparmor profile name. + DefaultApparmorProfile = "ocid-default" + + // ContainerAnnotationKeyPrefix is the prefix to an annotation key specifying a container profile. + ContainerAnnotationKeyPrefix = "container.apparmor.security.beta.kubernetes.io/" + + // ProfileRuntimeDefault is he profile specifying the runtime default. + ProfileRuntimeDefault = "runtime/default" + // ProfileNamePrefix is the prefix for specifying profiles loaded on the node. + ProfileNamePrefix = "localhost/" +) diff --git a/server/apparmor/apparmor.go b/server/apparmor/apparmor_supported.go similarity index 86% rename from server/apparmor/apparmor.go rename to server/apparmor/apparmor_supported.go index 824be5ec..ff9205ad 100644 --- a/server/apparmor/apparmor.go +++ b/server/apparmor/apparmor_supported.go @@ -17,20 +17,9 @@ import ( ) const ( - // DefaultApparmorProfile is the name of default apparmor profile name. - DefaultApparmorProfile = "ocid-default" - // profileDirectory is the file store for apparmor profiles and macros. profileDirectory = "/etc/apparmor.d" - // ContainerAnnotationKeyPrefix is the prefix to an annotation key specifying a container profile. - ContainerAnnotationKeyPrefix = "container.apparmor.security.beta.kubernetes.io/" - - // ProfileRuntimeDefault is he profile specifying the runtime default. - ProfileRuntimeDefault = "runtime/default" - // ProfileNamePrefix is the prefix for specifying profiles loaded on the node. - ProfileNamePrefix = "localhost/" - // readConfigTimeout is the timeout of reading apparmor profiles. readConfigTimeout = 10 ) diff --git a/server/apparmor/apparmor_unsupported.go b/server/apparmor/apparmor_unsupported.go index c98e6dc7..b4c107c0 100644 --- a/server/apparmor/apparmor_unsupported.go +++ b/server/apparmor/apparmor_unsupported.go @@ -2,19 +2,6 @@ package apparmor -const ( - // DefaultApparmorProfile is the name of default apparmor profile name. - DefaultApparmorProfile = "ocid-default" - - // ContainerAnnotationKeyPrefix is the prefix to an annotation key specifying a container profile. - ContainerAnnotationKeyPrefix = "container.apparmor.security.beta.kubernetes.io/" - - // ProfileRuntimeDefault is he profile specifying the runtime default. - ProfileRuntimeDefault = "runtime/default" - // ProfileNamePrefix is the prefix for specifying profiles loaded on the node. - ProfileNamePrefix = "localhost/" -) - // IsEnabled returns false, when build without apparmor build tag. func IsEnabled() bool { return false From ca7d5c77c29de1b95e8c749c089c7b1dd2cc047d Mon Sep 17 00:00:00 2001 From: Xianglin Gao Date: Mon, 12 Dec 2016 15:55:17 +0800 Subject: [PATCH 3/3] Do not load ocid-default if configured apparmor profile is set up. Signed-off-by: Xianglin Gao --- server/apparmor/apparmor_supported.go | 66 +++++++++++++------------ server/apparmor/apparmor_unsupported.go | 5 +- server/container_create.go | 12 +++-- server/server.go | 8 +-- 4 files changed, 50 insertions(+), 41 deletions(-) diff --git a/server/apparmor/apparmor_supported.go b/server/apparmor/apparmor_supported.go index ff9205ad..d765c9de 100644 --- a/server/apparmor/apparmor_supported.go +++ b/server/apparmor/apparmor_supported.go @@ -4,14 +4,13 @@ package apparmor import ( "bufio" + "fmt" "io" "io/ioutil" "os" "path" "strings" - "time" - "github.com/Sirupsen/logrus" "github.com/docker/docker/utils/templates" "github.com/opencontainers/runc/libcontainer/apparmor" ) @@ -19,9 +18,6 @@ import ( const ( // profileDirectory is the file store for apparmor profiles and macros. profileDirectory = "/etc/apparmor.d" - - // readConfigTimeout is the timeout of reading apparmor profiles. - readConfigTimeout = 10 ) // profileData holds information about the given profile for generation. @@ -36,13 +32,26 @@ type profileData struct { Version int } -// LoadDefaultAppArmorProfile loads default apparmor profile, if it is not loaded. -func LoadDefaultAppArmorProfile() { - if !IsLoaded(DefaultApparmorProfile) { +// EnsureDefaultApparmorProfile loads default apparmor profile, if it is not loaded. +func EnsureDefaultApparmorProfile() error { + if apparmor.IsEnabled() { + loaded, err := IsLoaded(DefaultApparmorProfile) + if err != nil { + return fmt.Errorf("Could not check if %s AppArmor profile was loaded: %s", DefaultApparmorProfile, err) + } + + // Nothing to do. + if loaded { + return nil + } + + // Load the profile. if err := InstallDefault(DefaultApparmorProfile); err != nil { - logrus.Errorf("AppArmor enabled on system but the %s profile could not be loaded:%v", DefaultApparmorProfile, err) + return fmt.Errorf("AppArmor enabled on system but the %s profile could not be loaded.", DefaultApparmorProfile) } } + + return nil } // IsEnabled returns true if apparmor is enabled for the host. @@ -77,35 +86,30 @@ func InstallDefault(name string) error { return LoadProfile(f.Name()) } -// IsLoaded checks if a passed profile has been loaded into the kernel. -func IsLoaded(name string) bool { +// IsLoaded checks if a profile with the given name has been loaded into the +// kernel. +func IsLoaded(name string) (bool, error) { file, err := os.Open("/sys/kernel/security/apparmor/profiles") if err != nil { - return false + return false, err } defer file.Close() - ch := make(chan bool, 1) - - go func() { - r := bufio.NewReader(file) - for { - p, err := r.ReadString('\n') - if err != nil { - ch <- false - } - if strings.HasPrefix(p, name+" ") { - ch <- true - } + r := bufio.NewReader(file) + for { + p, err := r.ReadString('\n') + if err == io.EOF { + break + } + if err != nil { + return false, err + } + if strings.HasPrefix(p, name+" ") { + return true, nil } - }() - - select { - case <-time.After(time.Duration(readConfigTimeout) * time.Second): - return false - case enabled := <-ch: - return enabled } + + return false, nil } // generateDefault creates an apparmor profile from ProfileData. diff --git a/server/apparmor/apparmor_unsupported.go b/server/apparmor/apparmor_unsupported.go index b4c107c0..fbd1d87a 100644 --- a/server/apparmor/apparmor_unsupported.go +++ b/server/apparmor/apparmor_unsupported.go @@ -7,8 +7,9 @@ func IsEnabled() bool { return false } -// LoadDefaultAppArmorProfile dose nothing, when build without apparmor build tag. -func LoadDefaultAppArmorProfile() { +// EnsureDefaultApparmorProfile dose nothing, when build without apparmor build tag. +func EnsureDefaultApparmorProfile() error { + return nil } // GetProfileNameFromPodAnnotations dose nothing, when build without apparmor build tag. diff --git a/server/container_create.go b/server/container_create.go index ed7faa7c..6a5b93fb 100644 --- a/server/container_create.go +++ b/server/container_create.go @@ -188,6 +188,13 @@ func (s *Server) createSandboxContainer(containerID string, containerName string if s.appArmorEnabled { appArmorProfileName := s.getAppArmorProfileName(sb.annotations, metadata.GetName()) if appArmorProfileName != "" { + // reload default apparmor profile if it is unloaded. + if s.appArmorProfile == apparmor.DefaultApparmorProfile { + if err := apparmor.EnsureDefaultApparmorProfile(); err != nil { + return nil, err + } + } + specgen.SetProcessApparmorProfile(appArmorProfileName) } } @@ -393,11 +400,6 @@ func (s *Server) getAppArmorProfileName(annotations map[string]string, ctrName s } if profile == apparmor.ProfileRuntimeDefault { - // reload default apparmor profile if it is unloaded. - if s.appArmorProfile == apparmor.DefaultApparmorProfile { - apparmor.LoadDefaultAppArmorProfile() - } - // If the value is runtime/default, then return default profile. return s.appArmorProfile } diff --git a/server/server.go b/server/server.go index 42d7fcac..b2066f76 100644 --- a/server/server.go +++ b/server/server.go @@ -287,6 +287,7 @@ func New(config *Config) (*Server, error) { }, seccompEnabled: seccompEnabled(), appArmorEnabled: apparmor.IsEnabled(), + appArmorProfile: config.ApparmorProfile, } seccompProfile, err := ioutil.ReadFile(config.SeccompProfile) if err != nil { @@ -298,10 +299,11 @@ func New(config *Config) (*Server, error) { } s.seccompProfile = seccompConfig - if s.appArmorEnabled { - apparmor.LoadDefaultAppArmorProfile() + if s.appArmorEnabled && s.appArmorProfile == apparmor.DefaultApparmorProfile { + if err := apparmor.EnsureDefaultApparmorProfile(); err != nil { + return nil, fmt.Errorf("ensuring the default apparmor profile is installed failed: %v", err) + } } - s.appArmorProfile = config.ApparmorProfile s.podIDIndex = truncindex.NewTruncIndex([]string{}) s.podNameIndex = registrar.NewRegistrar()