-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow multiple sopsFiles for easier common secrets sharing across multiple configuration #417
base: master
Are you sure you want to change the base?
Conversation
Current changes are just a rough draft |
Only question is how you order priority between those in case keys overlap? Otherwise it would make sense to throw an error so that users don't accidentally shadow secrets. |
My idea was to have the files in increasing priority, In other words search the list of sops files in reverse order and use the first found secret. In the example above vpn_user and vpn_password in secrets/common.yaml would be shadowed by vpn_user and vpn_password in secrets/<system>/default.yaml. |
It's just hard to enforce an order in lists if you those lists get defined in different nixos modules. |
Ah. I think one can use lib.mkBefore for host files. |
modules/sops/default.nix
Outdated
description = '' | ||
Sops file the secret is loaded from. | ||
''; | ||
}; | ||
sopsFileHash = mkOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should deprecated this one than.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecate sopsFiles? Just added that this PR, do you mean deprecate sopsFile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't really need both sopsFileHash and sopsFilesHash, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They could be point to the same value.
@@ -172,6 +189,13 @@ in { | |||
Default sops file used for all secrets. | |||
''; | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this one with a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this?
modules/sops/default.nix
Outdated
sopsFile = lib.mkOptionDefault cfg.defaultSopsFile; | ||
sopsFileHash = mkOptionDefault (optionalString cfg.validateSopsFiles "${builtins.hashFile "sha256" config.sopsFile}"); | ||
sopsFile = mkOptionDefault cfg.defaultSopsFile; | ||
sopsFiles = if options.sopsFile.isDefined then warn "`sops.secrets.<name>.sopsFile` is being deprecated, use `sops.secrets.<name>.sopsFiles` instead" [ config.sopsFile ] else (lib.mkOptionDefault cfg.defaultSopsFiles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultSopsFile/sopsFile takes priority over defaultSopsFiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not add defaultSopsFile
to the new defaultSopsFiles
list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not be better to keep defaultSopsFile
and defaultSopsFiles
separate and place sopsFile
before sopsFiles
per secret?
If you merge in default you would need to remove sopsFile
otherwise sopsFiles
would become [ sopsFile
defaultSopsFile
] ++ defaultSopsFiles
.
The only way I was able to implement this feels however a bit hacky
config = mkMerge [{
sopsFile = mkOptionDefault cfg.defaultSopsFile;
sopsFiles = mkOptionDefault cfg.defaultSopsFiles;
sopsFilesHash = mkOptionDefault (optionals cfg.validateSopsFiles (forEach config.sopsFiles (builtins.hashFile "sha256")));
}
{
sopsFiles = mkIf (config.sopsFile != null) (mkOverride options.sopsFile.highestPrio (mkBefore [config.sopsFile]));
}];
The effect of this is that when both sopsFile
and sopsFiles
are defined (and not null) with different priorities only one will make it into config.sopsFiles
sopsFile
or sopsFiles
also take priority over defaultSopsFile
and defaultSopsFiles
pkgs/sops-install-secrets/main.go
Outdated
for i := len(files) - 1; i >= 0; i-- { | ||
err := app.validateSopsFile(secret, &files[i]) | ||
if err == nil { | ||
// Found valid sopsFile | ||
break | ||
} else if i == 0 { | ||
// No valid sopsFile found in sopsFiles | ||
return err | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error "secret %s defined the format of %s as %s, but it was specified as %s in %s before" from validateSopsFile
is ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm. I would build a new error message that would give you all files where the secret has been looked for instead.
default: | ||
return fmt.Errorf("Secret of type %s in %s is not supported", s.Format, s.SopsFiles[i]) | ||
} | ||
} | ||
switch s.Format { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming all sopsFiles have the same format
pkgs/sops-install-secrets/main.go
Outdated
@@ -393,40 +400,40 @@ func lookupKeysGroup() (int, error) { | |||
return 0, fmt.Errorf("Can't find group 'keys' nor 'nogroup' (%w).", err2) | |||
} | |||
|
|||
func (app *appContext) loadSopsFile(s *secret) (*secretFile, error) { | |||
func (app *appContext) loadSopsFile(s *secret, sopsFileIndex int) (*secretFile, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not pass in the sopsFile as a pointer here rather than using an index?
pkgs/sops-install-secrets/main.go
Outdated
if err != nil { | ||
return err | ||
files := []secretFile {}; | ||
for index, sopsFile := range secret.SopsFiles { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already have you reference here.
is this dead in the water? is there another way to share secrets among hosts without copying secrets between individual hosts secrets.yaml files? |
Allow multiple sopsFiles for easier secret sharing across multiple configurations.
For example you have files
secrets/common.yaml
secrets/<system>/default.yaml
with
sops.defaultSopsFiles = [ ./secrets/common.yaml ./secrets/<system>/default.yaml ];
in the configuration for<system>
.This would allow overriding of shared secrets across hosts without manually checking and changing the required sopsFile per secret.