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

Add AppArmor Support #8299 #18858

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add AppArmor Support #8299 #18858

wants to merge 11 commits into from

Conversation

nnzv
Copy link

@nnzv nnzv commented May 12, 2024

Implemented AppArmor support for Minikube on both aarch64 and x86_64 architectures. Testing pending, relying on documentation for guidance. Excluded unnecessary extras:

  • BR2_PACKAGE_APPARMOR_PROFILES: Community profiles deemed non-essential for Minikube users.

Copy link

linux-foundation-easycla bot commented May 12, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nnzv
Once this PR has been reviewed and has the lgtm label, please assign prezha for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 12, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @nnzv!

It looks like this is your first PR to kubernetes/minikube 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/minikube has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @nnzv. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 12, 2024
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 12, 2024
@nnzv
Copy link
Author

nnzv commented May 12, 2024

I'm checking if the kernel image from Buildroot supports AppArmor. Following the docs, I tried linux-menuconfig in the Makefile, but it says there's nothing to be done. Am I overlooking any essential steps? Also, for an overview of the kernel modifications required, here's a summary of the necessary options:

General setup --->
    -*- Auditing support

Security options  --->
    -*- Enable the securityfs filesystem
    -*- Socket and Networking Security Hooks
    [*] Enable different security models
    [*] AppArmor support
          [*]   Enable introspection of sha1 hashes for loaded profiles
          [*]      Enable policy hash introspection by default
    [ ] Build AppArmor with debug code
          First legacy 'major LSM' to be initialized (AppArmor)  --->
          "yama,apparmor" Ordered List of enabled LSMs

@nnzv
Copy link
Author

nnzv commented May 12, 2024

CC: @tstromberg

@nnzv
Copy link
Author

nnzv commented May 13, 2024

I've gone through the entire image building process to test out the new environment, following the documentation closely. It instructs to use the target out/minikube-amd64.iso, but it failed near the end. I fixed it by switching "amd64" to "x86_64". If this change gets merged into the master branch, we might need to update the documentation. Check the logs, especially after I switched to the "x86_64" target. The original log file is massive, around 56MB of plain text.

...
make[1]: Leaving directory '/mnt/out/buildroot'
# x86_64 ISO is still BIOS rather than EFI because of AppArmor issues for KVM, and Gen 2 issues for Hyper-V
if [ "x86_64" = "aarch64" ]; then \
                mv /mnt/out/buildroot/output-aarch64/images/boot.iso /mnt/out/minikube-arm64.iso; \
        else \
                mv /mnt/out/buildroot/output-x86_64/images/rootfs.iso9660 /mnt/out/minikube-amd64.iso; \
        fi;
Please enter a valid architecture. Choices are x86_64 and aarch64.
make: *** [Makefile:959: deploy/iso/minikube-iso/board/minikube/amd64/rootfs-overlay/usr/bin/auto-pause] Error 1
make: *** [Makefile:333: out/minikube-amd64.iso] Error 2
...

Seems like the only thing left is to tweak the kernel, you know, enabling those configurations I mentioned earlier. Once that's done, the feature should be ready, although we still need to do some testing regarding the use of AppArmor at the Kubernetes level. I'll roughly leave the final commands I used and some evidence. Note that the last "cat" command results in N, it should be Y.

minikube start --driver=kvm2 --cni calico --kubernetes-version=1.29.0 --feature-gates=AppArmor=true --iso-url=file://$(pwd)/out/minikube-amd64.iso
minikube ssh

2024-05-12_23-49

@medyagh
Copy link
Member

medyagh commented May 14, 2024

@nnzv do you mind sharing what use cases this would help with ? are there any issues that users are already asking for this ?

@nnzv
Copy link
Author

nnzv commented May 14, 2024

Do you mind sharing what use cases this would help with ? are there any issues that users are already asking for this ?

User @acoulon99 requested this feature some time ago via an issue (#8299). For detailed information on its use cases, you can refer to the Kubernetes documentation at https://kubernetes.io/docs/tutorials/security/apparmor, particularly the example of a profile that blocks all file write operations on nodes. It's surprising that Minikube hasn't incorporated this feature, considering that Kubernetes has supported AppArmor since version 1.14. Implementing it would be a logical next step.

@medyagh
Copy link
Member

medyagh commented May 16, 2024

Do you mind sharing what use cases this would help with ? are there any issues that users are already asking for this ?

User @acoulon99 requested this feature some time ago via an issue (#8299). For detailed information on its use cases, you can refer to the Kubernetes documentation at https://kubernetes.io/docs/tutorials/security/apparmor, particularly the example of a profile that blocks all file write operations on nodes. It's surprising that Minikube hasn't incorporated this feature, considering that Kubernetes has supported AppArmor since version 1.14. Implementing it would be a logical next step.

thank you for sharing that thats a valid reason to add the support, in the past each time apparomor gets enabled it breaks a lot of other things... I curious how it would affect addons or other normal use cases

@medyagh
Copy link
Member

medyagh commented May 16, 2024

ok-to-build-iso

@medyagh
Copy link
Member

medyagh commented May 16, 2024

Do you mind sharing what use cases this would help with ? are there any issues that users are already asking for this ?

User @acoulon99 requested this feature some time ago via an issue (#8299). For detailed information on its use cases, you can refer to the Kubernetes documentation at https://kubernetes.io/docs/tutorials/security/apparmor, particularly the example of a profile that blocks all file write operations on nodes. It's surprising that Minikube hasn't incorporated this feature, considering that Kubernetes has supported AppArmor since version 1.14. Implementing it would be a logical next step.

if we I think we should have a flag to like --apparomor so if users face issues they could disable it.
I am gonna build the ISO on the PR just to see initial tests results

@nnzv
Copy link
Author

nnzv commented May 16, 2024

If we I think we should have a flag to like --apparomor so if users face issues they could disable it. I am gonna build the ISO on the PR just to see initial tests results

You're welcome. Indeed, it does seem like a good idea. I wasn't aware that it was implemented long ago but disabled due to breaking changes. Regarding the new flag, I'm not sure how to dynamically ignore things by buildroot, specifically referring to the iso/minikube-iso/configs/* files depending on the architecture.

@nnzv
Copy link
Author

nnzv commented May 16, 2024

Additionally, keep in mind that this is only partially implemented. The remaining task involves configuring the kernel. You can find details in one of my comments below. @medyagh

@medyagh
Copy link
Member

medyagh commented May 17, 2024

yes I think there was issues with github action running with apparmor (for KIC - docker/podman drivers)
#7658

@nnzv the ISO build failed btw but not related to this PR (yet) the build failure is due to the changes in SHA by containernetworking/plugins#1038 that they unexpectedly built the binary and overwrote the previous ones

for enable disabling it I am thinking if there should be a linux command to turn it on or off after the ISO is built. so if users start minikube --apparmor=true it would be enabled in the kernel module. that way we would not have a lot of new headaches for breaking people's existing workflows

@medyagh
Copy link
Member

medyagh commented May 20, 2024

@nnzv do you mind pulling upstream just merged #18896

@medyagh
Copy link
Member

medyagh commented May 20, 2024

/ok-to-build-iso

nnzv added 2 commits May 24, 2024 15:44
Apparmor configured for aarch64 and x86_64 platforms, requiring
activation via kernel parameters (e.g., via grub.cfg). Debug
settings enabled for future apparmor issue resolution.
Added apparmor utilities and extra utilities to configurations
for both aarch64 and x86_64 platforms. Apparmor utilities such
as aa-status are helpful for exploring this LSM, while additional
utilities require Perl and other dependencies, hopefully already
enabled.
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 24, 2024
Copy link
Author

@nnzv nnzv left a comment

Choose a reason for hiding this comment

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

After researching, I found that apparmor can't be activated as a kernel module. So, with this setup, the kernel won't use apparmor LSM by default. To enable it dynamically, we may need to tweak kernel parameters in the grub.cfg. So, after setting up a node, we'd SSH in, adjust the grub.cfg, or generate it with grub-mkconfig after editing /etc/default/grub.

minikube ssh
# ...
vi /etc/default/grub
# ...
grep CMDLINE_LINUX /etc/default/grub
# GRUB_CMDLINE_LINUX="... apparmor=1 security=apparmor ..."
grub-mkconfig -o /boot/grub/grub.cfg
# ...

If for some reason Minikube doesn't handle the modification of the grub configuration, we can manually edit /boot/grub/grub.cfg or its equivalent on the node to add the required kernel parameters. Afterward, a reboot is required for the kernel to recognize the new LSM. Of course, this explanation is simply to outline the process; ideally, automation could be achieved using a boolean flag variable like --apparmor, as you mentioned.

@nnzv
Copy link
Author

nnzv commented May 24, 2024

@nnzv I didnt see any new change in your commit, if apparmor is added you should be able to verify it locally (try running minikube start --iso-url) and provide local file path as the url to test it.

No worries. It's committed now. Since I don't have my ThinkPad (the one I used for development) with me, I'll wait for Minikube's CI to finish building the ISO. Once that's done, I'll test the POC I mentioned earlier, specifically about enabling apparmor with kernel parameters. Enabling it by default (adding CONFIG_DEFAULT_SECURITY_APPARMOR) in the kernel configuration could pose a problem. Take a look at this quote from a GitHub blog post: https://github.blog/2023-07-05-introduction-to-selinux/

Please note that some Linux distributions have different MAC systems enabled, and running multiple exclusive LSMs at the same time will cause issues. For example, Ubuntu has AppArmor enabled by default, therefore enabling SELinux while AppArmor is enabled will prevent you from booting properly. Please check include/linux/lsm_hooks.h for the LSM_FLAG_EXCLUSIVE to see if your LSM is exclusive.

@minikube-pr-bot

This comment has been minimized.

@minikube-bot
Copy link
Collaborator

Hi @nnzv, we have updated your PR with the reference to newly built ISO. Pull the changes locally if you want to test with them or update your PR further.

@minikube-pr-bot

This comment has been minimized.

@nnzv
Copy link
Author

nnzv commented May 25, 2024

Hey @medyagh, could we give "ok-to-build-iso" another shot? I've checked the ISO created by the bot, but it seems my changes weren't taken into account (494ee1d and e109ed0). By the way, do you happen to know how we can tweak kernel parameters? I'm aware that each architecture in deploy/iso/minikube-iso/board has its own grub.cfg, but I couldn't find anything when I ran find / -name "grub.cfg" -type f from the root after SSHing into the minikube node.

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2024
@medyagh
Copy link
Member

medyagh commented May 29, 2024

@nnzv sure can you plz rebase your PR so I can trigger it one more time ?

@medyagh
Copy link
Member

medyagh commented May 29, 2024

/ok-to-build-iso

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2024
@minikube-pr-bot

This comment has been minimized.

@minikube-bot
Copy link
Collaborator

Hi @nnzv, we have updated your PR with the reference to newly built ISO. Pull the changes locally if you want to test with them or update your PR further.

@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 18858) |
+----------------+----------+---------------------+
| minikube start | 48.0s    | 48.4s               |
| enable ingress | 24.4s    | 24.4s               |
+----------------+----------+---------------------+

Times for minikube ingress: 25.4s 24.0s 25.0s 23.5s 23.9s
Times for minikube (PR 18858) ingress: 24.0s 24.5s 24.9s 24.0s 24.4s

Times for minikube start: 47.5s 45.3s 49.4s 49.8s 47.9s
Times for minikube (PR 18858) start: 49.1s 47.6s 47.2s 49.7s 48.5s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 18858) |
+----------------+----------+---------------------+
| minikube start | 24.1s    | 21.7s               |
| enable ingress | 21.5s    | 21.6s               |
+----------------+----------+---------------------+

Times for minikube ingress: 21.8s 20.8s 21.8s 21.3s 21.8s
Times for minikube (PR 18858) ingress: 20.8s 21.8s 22.3s 21.8s 21.3s

Times for minikube start: 24.2s 24.5s 23.7s 24.3s 23.6s
Times for minikube (PR 18858) start: 21.5s 20.7s 21.0s 21.0s 24.4s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 18858) |
+----------------+----------+---------------------+
| minikube start | 23.1s    | 21.3s               |
| enable ingress | 31.5s    | 35.3s               |
+----------------+----------+---------------------+

Times for minikube start: 23.0s 23.3s 23.5s 22.5s 23.4s
Times for minikube (PR 18858) start: 20.9s 19.7s 21.9s 23.3s 20.8s

Times for minikube ingress: 31.3s 30.8s 30.8s 32.3s 32.3s
Times for minikube (PR 18858) ingress: 31.8s 31.8s 32.3s 48.8s 31.8s

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot
Copy link

These are the flake rates of all failed tests.

Environment Failed Tests Flake Rate (%)
KVM_Linux_crio TestFunctional/serial/ExtraConfig (gopogh) 0.00 (chart)
KVM_Linux_containerd TestAddons/parallel/LocalPath (gopogh) 0.64 (chart)
KVM_Linux_crio TestFunctional/parallel/ServiceCmdConnect (gopogh) 0.64 (chart)
KVM_Linux_crio TestFunctional/serial/ComponentHealth (gopogh) 0.64 (chart)
KVM_Linux_containerd TestAddons/parallel/MetricsServer (gopogh) 1.27 (chart)
KVM_Linux_crio TestFunctional/parallel/MountCmd/any-port (gopogh) 1.28 (chart)
KVM_Linux_crio TestFunctional/parallel/ServiceCmd/DeployApp (gopogh) 1.28 (chart)
KVM_Linux_crio TestFunctional/parallel/ServiceCmd/Format (gopogh) 1.28 (chart)
KVM_Linux_crio TestFunctional/parallel/ServiceCmd/HTTPS (gopogh) 1.28 (chart)
KVM_Linux_crio TestFunctional/parallel/ServiceCmd/URL (gopogh) 1.28 (chart)
KVM_Linux_crio TestFunctional/parallel/MySQL (gopogh) 2.56 (chart)
Docker_Linux_containerd TestAddons/parallel/Ingress (gopogh) 2.58 (chart)
KVM_Linux_crio TestFunctional/parallel/DashboardCmd (gopogh) 3.21 (chart)
KVM_Linux_crio TestFunctional/parallel/PersistentVolumeClaim (gopogh) 3.85 (chart)
Docker_Linux_containerd TestAddons/parallel/CloudSpanner (gopogh) 3.87 (chart)
Docker_Windows TestNoKubernetes/serial/StartNoArgs (gopogh) 3.88 (chart)
QEMU_macOS TestAddons/Setup (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/CertSync (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/CpCmd (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/DockerEnv/bash (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/FileSync (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageBuild (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageListJson (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageListShort (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageListTable (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageListYaml (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageLoadDaemon (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageLoadFromFile (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageReloadDaemon (gopogh) 6.78 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageSaveToFile (gopogh) 6.78 (chart)
More tests... Continued...

Too many tests failed - See test logs for more details.

To see the flake rates of all tests by environment, click here.

@nnzv
Copy link
Author

nnzv commented Jun 1, 2024

Alright, so we got the utils binaries in place. But now, we need to activate apparmor using kernel parameters. It's disabled at boot by default to prevent potential issues in case minikube decides to utilize another LSM like Selinux in the future.

minikube ssh
aa-enabled
# No - disabled at boot.

I see there's an isolinux.cfg for each board in deploy/iso/minikube-iso. It looks like you're using syslinux, though I couldn't confirm it in the docs. But the file suggests it. If that's the deal, you might want to create a syslinux.cfg file in the /boot/syslinux directory. It allows you to adjust the kernel parameters necessary to enable apparmor.

minikube ssh
sudo su -
mkdir /boot/syslinux
cat << EOF > /boot/syslinux/syslinux.cfg
APPEND apparmor=1 security=apparmor
EOF
reboot

Data isn't sticking around after a reboot, which is essential for loading "syslinux.cfg" to enable apparmor. From what I gather, there are specific directories that the iso keeps intact after a reboot, like /var/tmp/minikube, /data, /tmp/hostpath-provisioner/ or /tmp/hostpath_pv. I'm not sure if there's a method like a hook that minikube uses to preserve symlinks, so we could symlink the syslinux directory under one of these special directories to /boot. Could you kindly see if any of the other maintainers might have insights on this matter? @medyagh

minikube ssh
ls -l /boot
# total 21956
# -rw-r--r-- 1 root root 22482368 May 30 04:42 bzImage

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2024
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@mbana
Copy link

mbana commented Aug 13, 2024

Hi folks, any updates on when this will get merged?

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants