Add allow_world_readable_secrets option to config file

PicNoir commented

2023-10-25 12:45:42 +00:00

Contributor

commit 045fb89082960fec47e00546a8de41c0c0eed23c
Date:   Wed Oct 25 11:34:39 2023 +0200

    Add allow_world_readable_secrets option to config file
    
    Sometimes, the secret files permissions checks gets in the way. It's
    by no mean complete, it doesn't take the Posix ACLs into account among
    other things. Correctly checking the ACLs would be too involving (see
    https://git.deuxfleurs.fr/Deuxfleurs/garage/issues/658#issuecomment-7102)
    and would likely still fail in some weird chmod settings.
    
    We're adding a new configuration file key allowing the user to disable
    this permission check altogether.
    
    The (already existing) env variable counterpart always take precedence
    to this config file option. That's useful in cases where the
    configuration file is static and cannot be easily altered.
    
    Fixes https://git.deuxfleurs.fr/Deuxfleurs/garage/issues/658
    
    Co-authored-by: Florian Klink <flokli@flokli.de>

Paired on that with @flokli.

I also took the liberty to add clang to the default devshell. We can't build Garage without it.

This is my first contribution to Garage, I've been shy and tried not to set the house on fire the first time I visit it. I think we should extract the current GARAGE_ALLOW_WORLD_READABLE_SECRETS env variable "ad-hoc" management to clap. Aside from clarity, it'd document the variable in the --help usage for free. I can add that to this PR if you think it's a good idue.

Fixes #658

``` commit 045fb89082960fec47e00546a8de41c0c0eed23c Date: Wed Oct 25 11:34:39 2023 +0200 Add allow_world_readable_secrets option to config file Sometimes, the secret files permissions checks gets in the way. It's by no mean complete, it doesn't take the Posix ACLs into account among other things. Correctly checking the ACLs would be too involving (see https://git.deuxfleurs.fr/Deuxfleurs/garage/issues/658#issuecomment-7102) and would likely still fail in some weird chmod settings. We're adding a new configuration file key allowing the user to disable this permission check altogether. The (already existing) env variable counterpart always take precedence to this config file option. That's useful in cases where the configuration file is static and cannot be easily altered. Fixes https://git.deuxfleurs.fr/Deuxfleurs/garage/issues/658 Co-authored-by: Florian Klink <flokli@flokli.de> ``` Paired on that with @flokli. I also took the liberty to add clang to the default devshell. We can't build Garage without it. ---------------------------------------- This is my first contribution to Garage, I've been shy and tried not to set the house on fire the first time I visit it. I think we should extract the current `GARAGE_ALLOW_WORLD_READABLE_SECRETS` env variable "ad-hoc" management to clap. Aside from clarity, it'd document the variable in the `--help` usage for free. I can add that to this PR if you think it's a good idue. Fixes https://git.deuxfleurs.fr/Deuxfleurs/garage/issues/658

PicNoir force-pushed nin/world-readable-conf-file from 045fb89082 to 930fa08395

2023-10-25 12:47:29 +00:00

Compare

flokli referenced this pull request

2023-10-26 07:54:16 +00:00

Add support for specifying rpc_secret_file, metrics_token_file and admin_token_file using environment variables #643

lx commented

2023-10-26 09:07:20 +00:00

Owner

Could you make this change on the main-0.8.x branch instead of the main branch? PR #643 is also on that branch but I think the history you worked on does not include it so that will cause problems. I will sync the two branches manually when making the next release, but for now let's keep all the changes concerning env variables and cli flags on the 0.8.x branch.

By switching to main-0.8.x you can also remove the clang that you added in the flake.nix and we will make that a separate PR.

I think we should extract the current GARAGE_ALLOW_WORLD_READABLE_SECRETS env variable "ad-hoc" management to clap. Aside from clarity, it'd document the variable in the --help usage for free. I can add that to this PR if you think it's a good idue.

Yes it's a good idea, feel free to do so. It will probably require a bit of refactoring between garage/main.rs and garage_util/config.rs

Could you make this change on the `main-0.8.x` branch instead of the `main` branch? PR #643 is also on that branch but I think the history you worked on does not include it so that will cause problems. I will sync the two branches manually when making the next release, but for now let's keep all the changes concerning env variables and cli flags on the 0.8.x branch. By switching to `main-0.8.x` you can also remove the `clang` that you added in the flake.nix and we will make that a separate PR. > I think we should extract the current GARAGE_ALLOW_WORLD_READABLE_SECRETS env variable "ad-hoc" management to clap. Aside from clarity, it'd document the variable in the --help usage for free. I can add that to this PR if you think it's a good idue. Yes it's a good idea, feel free to do so. It will probably require a bit of refactoring between `garage/main.rs` and `garage_util/config.rs`

👍 1

flokli commented

2023-10-26 14:08:11 +00:00

Contributor

Isn't this bug also affecting 0.9.0 too? I'd assume main should contain all fixes, and we backport from there…

Isn't this bug also affecting 0.9.0 too? I'd assume `main` should contain all fixes, and we backport from there…

lx commented

2023-10-26 14:29:43 +00:00

Owner

Isn't this bug also affecting 0.9.0 too? I'd assume main should contain all fixes, and we backport from there…

I'm doing the opposite, it's not backporting but "forward-porting" :D

Changes that can be applied to the 0.8.x codebase are done on branch main-0.8.x, and then I merge that branch into branch main to have all changes in the 0.9 release as well. This helps me keep track in the git history of the fact that the 0.8.x code is an ancestor of the 0.9.x code (in the commit graph sense).

> Isn't this bug also affecting 0.9.0 too? I'd assume main should contain all fixes, and we backport from there… I'm doing the opposite, it's not backporting but "forward-porting" :D Changes that can be applied to the 0.8.x codebase are done on branch `main-0.8.x`, and then I merge that branch into branch `main` to have all changes in the 0.9 release as well. This helps me keep track in the git history of the fact that the 0.8.x code is an ancestor of the 0.9.x code (in the commit graph sense).

PicNoir force-pushed nin/world-readable-conf-file from 930fa08395 to f83fa02193

2023-10-26 16:29:40 +00:00

Compare

PicNoir changed target branch from main to main-0.8.x

2023-10-26 16:29:49 +00:00

PicNoir commented

2023-10-26 16:31:37 +00:00

Author

Contributor

Rebased on main-0.8.x. Having a stab at the clap refactoring discussed two messages above.

Rebased on `main-0.8.x`. Having a stab at the clap refactoring discussed two messages above.

PicNoir commented

2023-10-31 16:26:27 +00:00

Author

Contributor

Quick status update: started refactoring the thing, but got interrupted midway through. Just put that first thing on tomorrow's todolist.

lx commented

2024-01-11 11:18:38 +00:00

Owner

Hello, can anyone remember the status of this work? I'm going to make a v0.8.5 and v0.9.1 release asap and it would be great to have this included if it is ready.

lx commented

2024-01-15 15:20:10 +00:00

Owner

I will merge this as-is for now, thanks for the contribution @PicNoir and @flokli !

lx merged commit ee7fe27d3d into main-0.8.x

2024-01-15 15:20:16 +00:00

lx referenced this pull request from a commit

2024-01-15 15:20:18 +00:00

Merge pull request 'Add allow_world_readable_secrets option to config file' (#663) from PicNoir/garage:nin/world-readable-conf-file into main-0.8.x

lx referenced this pull request from a commit

2024-01-16 11:13:07 +00:00

Garage v0.8.5

lx referenced this pull request

2024-01-16 11:20:03 +00:00

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

PicNoir commented

2024-01-22 09:38:15 +00:00

Author

Contributor

Damn, I completely missed these messages. Sorry, for not responding: I did not receive any notification.

I have a half-baked refactoring I was talking about above. It required more modifications than I first expected, I had to move around a lot of things.

Anyways, even if it's sub-optimal, I guess this is fine-ish as it is.

Edit: pushed the WIP there https://git.deuxfleurs.fr/PicNoir/garage/src/branch/pic-refactoring just in case it'd be useful to somebody. It's definitely not ready though.

Damn, I completely missed these messages. Sorry, for not responding: I did not receive any notification. I have a half-baked refactoring I was talking about above. It required more modifications than I first expected, I had to move around a lot of things. Anyways, even if it's sub-optimal, I guess this is fine-ish as it is. Edit: pushed the WIP there https://git.deuxfleurs.fr/PicNoir/garage/src/branch/pic-refactoring just in case it'd be useful to somebody. It's definitely not ready though.

Add allow_world_readable_secrets option to config file #663