Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AppArmor profile includes files if they exist #4173

Merged
merged 2 commits into from
May 15, 2024

Conversation

tinnywang
Copy link
Contributor

Summary

ECS agent fails to start on SUSE Linux 15 because AppArmor can't load the ECS agent profile.

level=info time=2024-05-10T23:31:45Z msg="pre-start"
level=info time=2024-05-10T23:31:45Z msg="Successfully created docker client with API version 1.25"
level=info time=2024-05-10T23:31:45Z msg="pre-start: setting up ecs-agent-default AppArmor profile"
level=error time=2024-05-10T23:31:45Z msg="error loading apparmor profile /etc/apparmor.d/ecs-agent-default: running `/sbin/apparmor_parser apparmor_parser -Kr /etc/apparmor.d/ecs-agent-default` failed with output: AppArmor parser error for /etc/apparmor.d/ecs-agent-default in profile /etc/apparmor.d/ecs-agent-default at line 2: Could not open 'tunables/global'\n\nerror: exit status 1"

This is happening because SUSE Linux 15 doesn't ship with the AppArmor files the ECS agent profile depends on (/etc/apparmor.d/tunables/global and /etc/apparmor.d/abstractions/base).

Implementation details

To fix this, only write #include statements to the profile if the corresponding AppArmor files exist.
Always write @{PROC}=/proc/ to the profile If /etc/apparmor.d/tunables/global does not exist, because the profile uses @{PROC}.

Prior art: https://github.com/moby/moby/blob/ac71ac1c92326635fffb5423f70891e53dc769e4/profiles/apparmor/apparmor.go#L38-L42

Testing

Built the .rpm per these instructions.

On a SUSE Linux 15 EC2 instance,

  1. installed the .rpm with zypper install -y --allow-unsigned-rpm
  2. started ECS agent with systemctl start ecs
  3. verified ECS agent was running with systemctl status ecs
  4. verified that the EC2 instance joined my ECS cluster
  5. verified the contents of the AppArmor profile at /etc/apparmor.d/ecs-agent-default
@{PROC}=/proc/

profile ecs-agent-default flags=(attach_disconnected,mediate_deleted) {

  network inet,
  network inet6,
  network netlink,
  network unix,
  ...

On an AL2 EC2 instance,

  1. installed the .rpm with yum install -y
  2. started ECS agent with systemctl start ecs
  3. verified ECS agent was running with systemctl status ecs
  4. verified that the EC2 instance joined my ECS cluster
  5. verified that the AppArmor profile at /etc/apparmor.d/ecs-agent-default did not exist (because AL2 does not support AppArmor)

Built the .deb per these instructions.

On an Ubuntu 20.04 LTS EC2 instance,

  1. installed the .deb with apt install -y
  2. started ECS agent with systemctl start ecs
  3. verified ECS agent was running with systemctl status ecs
  4. verified that the EC2 instance joined my ECS cluster
  5. verified the contents of the AppArmor profile at /etc/apparmor.d/ecs-agent-default
#include <tunables/global>

profile ecs-agent-default flags=(attach_disconnected,mediate_deleted) {

  #include <abstractions/base>

  network inet,
  network inet6,
  network netlink,
  network unix,
  ...

New tests cover the changes: yes

Description for the changelog

Bug - Fixed a bug that could prevent ECS agent from starting on SUSE Linux 15

Does this PR include breaking model changes? If so, Have you added transformation functions?
No

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@tinnywang tinnywang marked this pull request as ready for review May 11, 2024 01:56
@tinnywang tinnywang requested a review from a team as a code owner May 11, 2024 01:56
}()
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
if !apparmor.HostSupports() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, could you provide a bit more context as to why we're removing this check? (sorry not quite clear to me)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoadDefaultProfile reads/writes files in /etc/apparmor.d, which only exists from the get-go if the OS ships with AppArmor. The check would normally protect against accessing a directory that doesn't exist, but since we stub out the filesystem functions in the tests, the tests will never fail if the OS doesn't have AppArmor.

Copy link
Contributor

@mye956 mye956 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to change the destination branch to dev!

@tinnywang tinnywang changed the base branch from master to dev May 14, 2024 00:31
@tinnywang tinnywang requested review from amogh09 and mye956 May 14, 2024 16:59
@@ -87,19 +101,45 @@ func LoadDefaultProfile(profileName string) error {
return err
}

includeTunablesGlobal, err := fileExists(filepath.Join(appArmorProfileDir, "tunables/global"))
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: If we return an error here if tunables/global doesn't exists, wouldn't this mean we'll never write @{PROC}=/proc/ to the profile? (please correct me if this is incorrect/the expected behavior!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the file doesn't exist, fileExists will return false and no error, so we will still write the profile.

if errors.Is(err, os.ErrNotExist) {
return false, nil
}

These 3 test cases check for this (statErrors are the errors os.Stat is stubbed to return, and profileContent is what's expected to be written to the profile):

{
name: "MissingTunablesGlobal",
profileName: "testProfile.txt",
statErrors: map[string]error{
"tunables/global": os.ErrNotExist,
},
profileContent: []string{"@{PROC}=/proc/", "#include <abstractions/base>"},
},
{
name: "MissingAbstractionsBase",
profileName: "testProfile.txt",
statErrors: map[string]error{
"abstractions/base": os.ErrNotExist,
},
profileContent: []string{"#include <tunables/global>"},
},
{
name: "MissingIncludes",
profileName: "testProfile.txt",
statErrors: map[string]error{
"tunables/global": os.ErrNotExist,
"abstractions/base": os.ErrNotExist,
},
profileContent: []string{"@{PROC}=/proc/"},
},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see, thanks Tiffany!

@tinnywang tinnywang merged commit 58464c6 into aws:dev May 15, 2024
40 checks passed
@tinnywang tinnywang deleted the include_apparmor branch May 15, 2024 19:37
@Yiyuanzzz Yiyuanzzz mentioned this pull request May 28, 2024
jiuchoe4 pushed a commit to saurabhc123/amazon-ecs-agent that referenced this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants