Deuxfleurs
/
garage
41
37
Fork 55
Code Issues 70 Pull Requests 6 Releases 29 Wiki Activity

Add allow_world_readable_secrets option to config file #663

Merged
lx merged 1 commits from PicNoir/garage:nin/world-readable-conf-file into main-0.8.x 2024-01-15 15:20:16 +00:00
Conversation 8 Commits 1 Files Changed 2 +89 -5
PicNoir commented 2023-10-25 12:45:42 +00:00
Contributor
Copy Link
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
lx commented 2023-10-26 09:07:20 +00:00
Owner
Copy Link

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
Copy Link

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
Copy Link

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
Copy Link

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
Copy Link

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

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
Copy Link

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.

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
Copy Link

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

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 issue from a commit 2024-01-16 11:13:07 +00:00
Garage v0.8.5
PicNoir commented 2024-01-22 09:38:15 +00:00
Author
Contributor
Copy Link

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.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
AdminAPI

Issues concerning the administration API

  
Bug

   
Check AWS

The behaviour should be checked against AWS S3

  
CI

Issues with Nix, Drone, Rust Compiler, etc.

  
Correctness

Issues of theoretical correctness that may impact edge-case scenarios

  
Critical

   
Documentation

   
Ideas

For abstract ideas/proposal to make Garage better, to better communicate our not yet achieved aims

  
Improvement

   
Low priority

For issues that we don't want to spend time on until made absolutely necessary

  
Newcomer

A bug that should be newcomer friendly.

  
Performance

   
S3 Compatibility

   
Testing

   
Usability

Propositions to make Garage easier to operate, by improving error reporting, CLI, etc.

No Label
AdminAPI
Bug
Check AWS
CI
Correctness
Critical
Documentation
Ideas
Improvement
Low priority
Newcomer
Performance
S3 Compatibility
Testing
Usability
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Deuxfleurs/garage#663
No description provided.
Delete Branch "PicNoir/garage:nin/world-readable-conf-file"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?