Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vdbe
Copy link
Contributor

@vdbe vdbe commented Oct 8, 2023

Allow multiple sopsFiles for easier secret sharing across multiple configurations.

For example you have files

secrets/common.yaml

hashed_password: ....
vpn_user: ...
vpn_password: ...
vpn_cert: | ...

secrets/<system>/default.yaml

vpn_user: ...
vpn_password: ...

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.

@vdbe
Copy link
Contributor Author

vdbe commented Oct 8, 2023

Current changes are just a rough draft

@Mic92
Copy link
Owner

Mic92 commented Oct 8, 2023

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.

@vdbe
Copy link
Contributor Author

vdbe commented Oct 8, 2023

My idea was to have the files in increasing priority,
this way you can have a default common file and append system/user specific files.

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.

@Mic92
Copy link
Owner

Mic92 commented Oct 8, 2023

It's just hard to enforce an order in lists if you those lists get defined in different nixos modules.

@Mic92
Copy link
Owner

Mic92 commented Oct 8, 2023

Ah. I think one can use lib.mkBefore for host files.

description = ''
Sops file the secret is loaded from.
'';
};
sopsFileHash = mkOption {
Copy link
Owner

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.

Copy link
Contributor Author

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?

Copy link
Owner

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?

Copy link
Owner

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.
'';
};
Copy link
Owner

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.

Copy link
Contributor Author

@vdbe vdbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this?

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);
Copy link
Contributor Author

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

Copy link
Owner

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?

Copy link
Contributor Author

@vdbe vdbe Oct 28, 2023

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

Comment on lines 517 to 528
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
}
}
Copy link
Contributor Author

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

Copy link
Owner

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 {
Copy link
Contributor Author

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

@@ -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) {
Copy link
Owner

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?

if err != nil {
return err
files := []secretFile {};
for index, sopsFile := range secret.SopsFiles {
Copy link
Owner

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.

@vdbe vdbe marked this pull request as ready for review November 12, 2023 18:55
@vdbe vdbe requested a review from Mic92 December 3, 2023 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants