Allow bind mounts prefixed with /mnt/ for ephemeral storage #320

zaheerm · 2024-12-16T14:13:45Z

Issue number:

Closes #

Description of changes:

Testing done:

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

bcressey

Thanks for picking this up!

bcressey · 2024-12-17T20:33:37Z

sources/api/apiserver/src/server/ephemeral_storage.rs

@@ -114,10 +114,13 @@ pub fn bind(variant: &str, dirs: Vec<String>) -> Result<()> {

    let mount_point = format!("/mnt/{}", EPHEMERAL_MNT);
    let mount_point = Path::new(&mount_point);
-    let allowed_dirs = allowed_bind_dirs(variant);
+    let (allowed_exact, allowed_prefixes) = allowed_bind_dirs(variant);


Two concerns:

We should disallow paths starting with /mnt/.ephemeral (the value in mount_point) since that directory and its subdirectories are used for keeping state and siloing content of the different mount points.

We should reject any paths that contain .. as that could traverse outside /mnt or over-mount other paths within it.

@bcressey thanks for the feedback, I have added checks for presence of .. and /mnt/.ephemeral in the next commit.

bcressey

@zaheerm left a couple of nits but LGTM overall. Can you squash these commits together?

@KCSesh or @yeazelm could one of you review and/or test?

sources/api/apiserver/src/server/ephemeral_storage.rs

sources/api/apiserver/src/server/mod.rs

zaheer-merali · 2025-01-06T14:19:38Z

@yeazelm Let me know if I should make any more changes to this PR. I marked it as draft initially but I'm happy with it being merged as is if the PR code is acceptable.

yeazelm · 2025-01-06T16:03:57Z

Hey @zaheer-merali, I just saw this was out for testing, sorry for the delay! I'll test it out now and come back as soon as I'm done.

yeazelm · 2025-01-06T19:59:57Z

I was able to validate this works (mostly) like we expect. I can now pass a directory in /mnt and it works, and if I pass something outside of the allowlist, it fails. The curious part is that I marked these as essential = true and the failure continued to boot. That isn't a bug in this PR, but we'll need to sort out why this didn't fail the boot. Here are my test results.

For a basic test:
This input works:

[settings.bootstrap-commands.k8s-ephemeral-storage]
commands = [
    ["apiclient", "ephemeral-storage", "init"],
    ["apiclient", "ephemeral-storage" ,"bind", "--dirs", "/var/lib/containerd", "/var/lib/kubelet", "/var/log/pods", "/mnt/testdir"]
]
essential = true
mode = "always"

And the directory exists

bash-5.1# ls -al /mnt
total 4
drwxr-xr-x.  4 root root   39 Jan  6 19:04 .
drwxr-xr-x. 19 root root 4096 Jan  6 18:45 ..
drwxr-xr-x.  6 root root  102 Jan  6 19:04 .ephemeral
drwxr-xr-x.  2 root root   22 Jan  6 19:06 testdir

If I try to put something in /var, it fails:

[settings.bootstrap-commands.k8s-ephemeral-storage]
commands = [
    ["apiclient", "ephemeral-storage", "init"],
    ["apiclient", "ephemeral-storage" ,"bind", "--dirs", "/var/lib/containerd", "/var/lib/kubelet", "/var/log/pods", "/var/lib/testdir"]
]
essential = true
mode = "always"

Output

Jan 06 19:09:54 localhost systemd[1]: Starting Bootstrap Commands...
Jan 06 19:09:54 ip-192-168-62-156.us-west-2.compute.internal bootstrap-commands[1769]: 19:09:54 [INFO] Processing bootstrap command 'k8s-ephemeral-storage' ...
Jan 06 19:09:56 ip-192-168-62-156.us-west-2.compute.internal bootstrap-commands[1785]: Failed to initialize ephemeral storage: Failed POST request to '/actions/ephemeral-storage/bind': Status 500 when POSTing /actions/ephemeral-storage/bind: Unable to bind ephemeral storage: Invalid Parameter '/var/lib/testdir', specified bind directory not in allow list
Jan 06 19:09:56 ip-192-168-62-156.us-west-2.compute.internal bootstrap-commands[1769]: 19:09:56 [INFO] Successfully ran bootstrap command 'k8s-ephemeral-storage'
Jan 06 19:09:56 ip-192-168-62-156.us-west-2.compute.internal systemd[1]: Finished Bootstrap Commands.

I also tried to sneak in under a path and that is blocked:

[root@admin]# journalctl -D /.bottlerocket/rootfs/var/log/journal/ -u bootstrap-commands
-- Logs begin at Mon 2025-01-06 20:02:19 UTC, end at Mon 2025-01-06 20:04:08 UTC. --
Jan 06 20:02:28 localhost systemd[1]: Starting Bootstrap Commands...
Jan 06 20:02:28 ip-192-168-32-137.us-west-2.compute.internal bootstrap-commands[1768]: 20:02:28 [INFO] Processing bootstrap command 'k8s-ephemeral-storage' ...
Jan 06 20:02:30 ip-192-168-32-137.us-west-2.compute.internal bootstrap-commands[1784]: Failed to initialize ephemeral storage: Failed POST request to '/actions/ephemeral-storage/bind': Status 500 when POSTing /actions/ephemeral-storage/bind: Unable to bind ephemeral storage: Invalid Parameter '/var/lib/containerd/sneaky', specified bind directory not in allow list
Jan 06 20:02:30 ip-192-168-32-137.us-west-2.compute.internal systemd[1]: Finished Bootstrap Commands.
Jan 06 20:02:30 ip-192-168-32-137.us-west-2.compute.internal bootstrap-commands[1768]: 20:02:30 [INFO] Successfully ran bootstrap command 'k8s-ephemeral-storage'

With it being marked essential, we should be failing but this behavior is happening with the current code so we can track that problem separately.

sources/api/apiserver/src/server/ephemeral_storage.rs

yeazelm · 2025-01-07T16:27:58Z

The changes look good, can we get the commits squashed again into one?

yeazelm

LGTM! Thanks for the contribution!

zaheer-merali · 2025-01-07T16:46:10Z

@yeazelm it missed the struct having to be made public to allow the struct to be exposed to mod.rs. So I resquashed the 4 character change.

yeazelm · 2025-01-07T17:50:59Z

@yeazelm it missed the struct having to be made public to allow the struct to be exposed to mod.rs. So I resquashed the 4 character change.

I caught the same thing when testing the build and you beat me to it! Nice catch!z

bcressey reviewed Dec 17, 2024

View reviewed changes

bcressey approved these changes Dec 21, 2024

View reviewed changes

sources/api/apiserver/src/server/ephemeral_storage.rs Outdated Show resolved Hide resolved

sources/api/apiserver/src/server/mod.rs Outdated Show resolved Hide resolved

zaheerm force-pushed the ephemeral-storage-mnt-prefix branch from af1b7ec to 2d5c361 Compare December 21, 2024 08:31

yeazelm self-requested a review December 23, 2024 19:51

yeazelm marked this pull request as ready for review January 6, 2025 20:00

yeazelm reviewed Jan 6, 2025

View reviewed changes

sources/api/apiserver/src/server/ephemeral_storage.rs Outdated Show resolved Hide resolved

sources/api/apiserver/src/server/ephemeral_storage.rs Outdated Show resolved Hide resolved

zaheerm force-pushed the ephemeral-storage-mnt-prefix branch from f976003 to 8e0e4b7 Compare January 6, 2025 20:41

yeazelm reviewed Jan 7, 2025

View reviewed changes

sources/api/apiserver/src/server/ephemeral_storage.rs Outdated Show resolved Hide resolved

zaheerm force-pushed the ephemeral-storage-mnt-prefix branch from a6acee5 to 8a6aee2 Compare January 7, 2025 16:29

yeazelm approved these changes Jan 7, 2025

View reviewed changes

Allow bind mounts prefixed with /mnt/ for ephemeral storage

8e028ee

zaheerm force-pushed the ephemeral-storage-mnt-prefix branch from 8a6aee2 to 8e028ee Compare January 7, 2025 16:36

yeazelm approved these changes Jan 7, 2025

View reviewed changes

yeazelm merged commit 5902dc0 into bottlerocket-os:develop Jan 7, 2025
2 checks passed

vigh-m mentioned this pull request Jan 15, 2025

Fix errors with apiserver and bootstrap-commands #350

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow bind mounts prefixed with /mnt/ for ephemeral storage #320

Allow bind mounts prefixed with /mnt/ for ephemeral storage #320

zaheerm commented Dec 16, 2024

bcressey left a comment

bcressey Dec 17, 2024

zaheer-merali Dec 17, 2024

bcressey left a comment

zaheer-merali commented Jan 6, 2025

yeazelm commented Jan 6, 2025

yeazelm commented Jan 6, 2025 •

edited

Loading

yeazelm commented Jan 7, 2025

yeazelm left a comment

zaheer-merali commented Jan 7, 2025

yeazelm commented Jan 7, 2025

Allow bind mounts prefixed with /mnt/ for ephemeral storage #320

Allow bind mounts prefixed with /mnt/ for ephemeral storage #320

Conversation

zaheerm commented Dec 16, 2024

bcressey left a comment

Choose a reason for hiding this comment

bcressey Dec 17, 2024

Choose a reason for hiding this comment

zaheer-merali Dec 17, 2024

Choose a reason for hiding this comment

bcressey left a comment

Choose a reason for hiding this comment

zaheer-merali commented Jan 6, 2025

yeazelm commented Jan 6, 2025

yeazelm commented Jan 6, 2025 • edited Loading

yeazelm commented Jan 7, 2025

yeazelm left a comment

Choose a reason for hiding this comment

zaheer-merali commented Jan 7, 2025

yeazelm commented Jan 7, 2025

yeazelm commented Jan 6, 2025 •

edited

Loading