getsops / sops

Simple and flexible tool for managing secrets
https://getsops.io/
Mozilla Public License 2.0
17.11k stars 879 forks source link

Fix `--config` being ignored by `loadConfig` #1613

Closed lopter closed 2 months ago

lopter commented 2 months ago

Use GlobalString in loadConfig since --config is not a sub-command flag.

This fixes --config being ignored and the CLI erroring out in a maddening way:

% sops --config config/sops.yaml encrypt --input-type json --output-type yaml /dev/stdin < data/test.json | head -n 3
config file not found, or has no creation rules, and no keys provided through command line options
% tree
./
├── config/
│   └── sops.yaml
└── data/
    └── test.json

3 directories, 2 files
% cat config/sops.yaml
creation_rules:
- path_regex: .*
  pgp: ADB6276965590A096004F6D1E114CBAE8FA29165
% cat data/test.json
{
    "key": 42
}
%

Maybe there is more to this fix, and I am happy to hear about it.


FWIW and future reference, here is how I got a debug build on Nix to debug this issue with delve:

$ cat ~/snippets/sops-debug-build.nix
sops.overrideAttrs(final: prev: {
  ldflags = [ "-X github.com/getsops/sops/v3/version.Version=${prev.version}" ];
  GOFLAGS = prev.GOFLAGS ++ [ "'-gcflags=all=-N -l'" ];
  dontStrip = true;
})
$ nix-store --realise $(nix-instantiate ~/snippets/sops-debug-build.nix)
felixfontein commented 2 months ago

Can you elaborate on

CLI erroring out in a maddening way.

? Do you mean an error such as [CMD] FATA[0000] flag provided but not defined: -config when using a subcommand?

Did you try passing --config before the subcommand, like sops --config /path/to/config edit /path/to/file? That works fine for me.

felixfontein commented 2 months ago

Or more precisely, that doesn't produce an error. It seems to be ignored, though. Is that what you mean, and what is fixed by using GlobalString?

felixfontein commented 2 months ago

There are two more places that should change from context.String("config") to context.GlobalString("config") in lines 2044 and 2045.

Also you need to sign-off the commits (see the failing DCO check).

felixfontein commented 2 months ago

While looking at #559 I noticed that this issue was already fixed once in #672. Either that fix was not correct, or got broken by a behavior change in the argument parsing library.

lopter commented 2 months ago

Can you elaborate on

CLI erroring out in a maddening way.

? Do you mean an error such as [CMD] FATA[0000] flag provided but not defined: -config when using a subcommand?

Did you try passing --config before the subcommand, like sops --config /path/to/config edit /path/to/file? That works fine for me.

I apologize for not including the error message:

% nix --experimental-features "nix-command flakes" run "nixpkgs#sops" -- --config config/sops.yaml encrypt --input-type json --output-type yaml /dev/stdin < data/test.json | head -n 3
config file not found, or has no creation rules, and no keys provided through command line options
% nix --experimental-features "nix-command flakes" run "github:lopter/nixpkgs/739ff1113708f7f5dea6a2dea423853d2800e42b#sops" -- --config config/sops.yaml encrypt --input-type json --output-type yaml /dev/stdin < data/test.json | head -n 3
key: ENC[AES256_GCM,data:hds=,iv:o4nE57sN3Hrqj4yTqiEeQR+/wqGcSuLJTuxIPrOxJVA=,tag:wmf30k4Kp8ZprWgxdAdX1g==,type:float]
sops:
    kms: []
% tree
./
├── config/
│   └── sops.yaml
└── data/
    └── test.json

3 directories, 2 files
% cat config/sops.yaml
creation_rules:
- path_regex: .*
  pgp: ADB6276965590A096004F6D1E114CBAE8FA29165
% cat data/test.json
{
    "key": 42
}
%

Looking at the blame for main.go, I am wondering if sub-commands were added down the road, and when they did the config loading code was not updated to look at --config as a global flag.

Let me update those two lines and commit with --signoff.

Thank you for the quick follow-up :)

felixfontein commented 2 months ago

Looking at the blame for main.go, I am wondering if sub-commands were added down the road, and when they did the config loading code was not updated to look at --config as a global flag.

I guess it's a mixture... In any case, I think we should add some tests that make sure that all subcommands correctly support this option :)

felixfontein commented 2 months ago

Thanks a lot for fixing this!