Garage fails with more recent systemd versions using ACLs for service credentials rather than chmod

flokli commented

2023-10-20 11:16:23 +00:00

Contributor

Somewhere between systemd 253.6 and 254.6 systemd changed the way systemd service credentials are provided to services - instead of using plain chmod, they use ACLs to restrict secrets to only the specific (dynamic) user the service is running as.

This breaks the check in garage on whether the file is readable by others, as commented in https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/643/files#issuecomment-7065 (but already was broken before that PR).

As upstream points out in https://github.com/systemd/systemd/issues/29435, there's many ways to ensure files are only accessible by one user, so maybe doing the check inside garage itself is the wrong approach - at least this should become a warning.

Somewhere between systemd 253.6 and 254.6 `systemd` changed the way systemd service credentials are provided to services - instead of using plain chmod, they use ACLs to restrict secrets to only the specific (dynamic) user the service is running as. This breaks the check in garage on whether the file is readable by others, as commented in https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/643/files#issuecomment-7065 (but already was broken before that PR). As upstream points out in https://github.com/systemd/systemd/issues/29435, there's many ways to ensure files are only accessible by one user, so maybe doing the check inside garage itself is the wrong approach - at least this should become a warning.

lx commented

2023-10-20 11:34:47 +00:00

Owner

So the options seem to be:

keep things as is, requiring an environment variable to be set when running in systemd
transform check into a warning
remove the check entirely

Any other options I missed?

So the options seem to be: - keep things as is, requiring an environment variable to be set when running in systemd - transform check into a warning - remove the check entirely Any other options I missed?

flokli commented

2023-10-20 20:56:35 +00:00

Author

Contributor

We could also add a config option to turn off the warning/error message. Having to set the environment variable whenever calling garage status is quite annoying.

We could also add a config option to turn off the warning/error message. Having to set the environment variable whenever calling `garage status` is quite annoying.

👍 2

PicNoir commented

2023-10-24 08:27:54 +00:00

Contributor

FTR, I explored a fourth option: keep the current behavior and modify secret_from_file to be ACL-aware.

Implementation wise, it's more involving though. First of all, ACLs implementations diverged. You have POSIX ACLs, implemented by Linux and FreeBSD. You have extended ACLs, implemented by FreeBSD and Darwin. You basically need a compatibility shim at some point. The exacl crate would be a good candidate.

Let's say we're okay to pull this dependency. Even then, the logic we'd have to implement to make sure the permission check is correctly implemented is quite involving. Darwin and Linux use different semantics to interpret the ACL bits (see https://byllyfish.github.io/exacl/exacl/fn.getfacl.html). Correctly interpreting the ACL bits on Darwin require us to look at the parent directories ACLs to get the inherited stuff.

On both implementation, (and for each directory level on Darwin), we'd have to go through the whole ACL vec and keep some sort of "who can access what" state, altered by folding through the ACL vec.

You get my sentiment by now, it's not un-doable, but this is pretty cursed :)

I'd personally vote for solution 1 (keep things as is) behavior-wise, except we'd transform this env variable to a configuration setting to "unbreak" the garage CLI interactive usage.

I'm up to implement this if you feel like it's the right fix.

FTR, I explored a fourth option: keep the current behavior and modify `secret_from_file` to be ACL-aware. Implementation wise, it's more involving though. First of all, ACLs implementations diverged. You have POSIX ACLs, implemented by Linux and FreeBSD. You have extended ACLs, implemented by FreeBSD and Darwin. You basically need a compatibility shim at some point. The [exacl](https://crates.io/crates/exacl) crate would be a good candidate. Let's say we're okay to pull this dependency. Even then, the logic we'd have to implement to make sure the permission check is correctly implemented is quite involving. Darwin and Linux use different semantics to interpret the ACL bits (see https://byllyfish.github.io/exacl/exacl/fn.getfacl.html). Correctly interpreting the ACL bits on Darwin require us to look at the parent directories ACLs to get the inherited stuff. On both implementation, (and for each directory level on Darwin), we'd have to go through the whole ACL vec and keep some sort of "who can access what" state, altered by folding through the ACL vec. You get my sentiment by now, it's not un-doable, but this is pretty cursed :) -------------------------------- I'd personally vote for solution 1 (keep things as is) behavior-wise, except we'd transform this env variable to a configuration setting to "unbreak" the `garage` CLI interactive usage. I'm up to implement this if you feel like it's the right fix.

lx commented

2023-10-24 11:01:42 +00:00

Owner

@PicNoir Your proposed solution looks fine to me. Maybe we can still keep the env variable as an override of the config file in cases where it is more practical? (e.g. if the config file cannot be easily modified)