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

Agenix integration #279

Merged
merged 3 commits into from
May 20, 2021
Merged

Agenix integration #279

merged 3 commits into from
May 20, 2021

Conversation

Pacman99
Copy link
Member

@Pacman99 Pacman99 commented May 13, 2021

AFAICT This is mostly a documentation thing. But with divnix/devlib#2 we can now add agenix to the devshell.

@Pacman99 Pacman99 requested a review from nrdxp May 13, 2021 17:39
@Pacman99 Pacman99 requested a review from blaggacao May 13, 2021 17:53
doc/secrets.md Outdated Show resolved Hide resolved
@Pacman99 Pacman99 force-pushed the secrets branch 2 times, most recently from 901aff0 to d727e7a Compare May 13, 2021 18:10
Comment on lines 1 to 4
* filter=git-crypt diff=git-crypt
.gitattributes !filter !diff
secrets.nix !filter !diff
README.md !filter !diff
Copy link
Member Author

@Pacman99 Pacman99 May 13, 2021

Choose a reason for hiding this comment

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

git-crypt is a little pointless if your using agenix properly. I also don't really like using it in general, agenix is just a much better solution for nix based setups.

We should probably leave it in the template for backwards compatibility - users might be storing secrets with the assumption that they are going to be encrypted.

But I would be in favor of removing it at some point.

Cool addition if you drop git-crypt: sudo nixos-rebuild switch --flake github:<nix config repo>.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also in favor of dropping it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm late to the party, but it would be good to drop as it actually enourages bad secrets management (putting secrets into Nix store). This was really more of a move of desparation on my part than an actual well thought out secrets strategy.

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely agree, I'm just more worried about someone not realizing git-crypt usage in secrets is dropped and committing their secrets to a public repo. Which is arguably worse.

doc/secrets.md Outdated Show resolved Hide resolved
doc/secrets.md Outdated Show resolved Hide resolved
@Pacman99 Pacman99 force-pushed the secrets branch 3 times, most recently from 4727fc1 to 0305c90 Compare May 13, 2021 19:51
Copy link
Contributor

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

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

Super exciting!

doc/secrets.md Outdated Show resolved Hide resolved
doc/secrets.md Outdated Show resolved Hide resolved
doc/secrets.md Outdated Show resolved Hide resolved
doc/secrets.md Outdated Show resolved Hide resolved
@@ -8,6 +8,7 @@ channels: final: prev: {
discord
element-desktop
manix
rage
Copy link
Contributor

Choose a reason for hiding this comment

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

off topic:

We might want to push for an update to latest release asap: https://github.com/str4d/rage/releases

Because:

Plugin support!

And I see how people want to use https://github.com/str4d/age-plugin-yubikey to seamlessly decrypt with their security key

Comment on lines 1 to 4
* filter=git-crypt diff=git-crypt
.gitattributes !filter !diff
secrets.nix !filter !diff
README.md !filter !diff
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also in favor of dropping it.

@@ -143,6 +143,12 @@ in
'';
};

# For rage encryption, all hosts need a ssh key pair
Copy link
Contributor

@GTrunSec GTrunSec May 13, 2021

Choose a reason for hiding this comment

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

The practice of agenix should enable the ed25519 in ssh services by default. like:

  services.openssh.hostKeys = [
    { type = "rsa"; bits = 4096; path = "/etc/ssh/ssh_host_rsa_key"; }
    { type = "ed25519"; bits = 256; path = "/etc/ssh/ssh_host_ed25519_key"; }
  ];

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this necessary? Looking at man configuration.nix the default for hostKeys is just what you put except without the bits = 256; part.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, Sorry I overlooked the man page. Just ignore this comment.

Copy link
Member Author

@Pacman99 Pacman99 May 13, 2021

Choose a reason for hiding this comment

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

Would be nice if it was possible to generate these files without enabling the openssh daemon. It would require decoupling the key generation parts from the daemon parts of the nixos sshd module.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about if I write a bash to generate these files by executing remote ssh. It would be an alternative choice for generating keys.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a bad idea, but it doesn't feel declarative. I think this seems to be the best solution for now. We could alternatively generate the hostKeys ourself, but that would conflict with the ssh module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For future reference, perhaps we could just make a more minimal module that generates the keys, but doesn't enable ssh.

Copy link
Member Author

Choose a reason for hiding this comment

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

Optimally we could contribute changes to the upstream ssh module to separate host key generation from the daemon itself.

@GTrunSec
Copy link
Contributor

I did testing of secrets between agenix and sops-nix.

Maybe those commands will help users to use agenix or sops easily.
https://github.com/GTrunSec/nixops-infra-land/blob/b1fb9983f9ce6a58691e87410827876167fcfc35/devshell.toml#L16

Default setting of ssh:

what do you think of this suggestion?

@Pacman99
Copy link
Member Author

Thanks for the suggestions!

https://github.com/GTrunSec/nixops-infra-land/blob/b1fb9983f9ce6a58691e87410827876167fcfc35/devshell.toml#L16

That looks useful, but for agenix I don't know if its warranted to add an entire command in the template to get a hosts public key. Its not something most people would do often. Although more pressing, this version of the template doesn't allow as much devshell customization. Something I would like to fix eventually.

Pacman99 and others added 2 commits May 14, 2021 18:38
@Pacman99
Copy link
Member Author

I think we can do this here too
bors try

bors bot added a commit that referenced this pull request May 15, 2021
@bors
Copy link
Contributor

bors bot commented May 15, 2021

try

Build succeeded:

@Pacman99
Copy link
Member Author

https://github.com/GTrunSec/nixops-infra-land/blob/b1fb9983f9ce6a58691e87410827876167fcfc35/devshell.toml#L16

I'm thinking about this again. And I realize the command is not agenix specific and generally pretty useful. I think it would be a good addition to the shell and can be done in in the library itself. Would you mind making a PR there to add it?

@GTrunSec
Copy link
Contributor

I think it would be a good addition to the shell and can be done in in the library itself. Would you mind making a PR there to add it?

Sure, by the way, Do you have a plan to add the sops-nix module for extending secrets?

@bors bors bot closed this May 15, 2021
@bors bors bot deleted the branch divnix:develop May 15, 2021 04:25
@Pacman99 Pacman99 reopened this May 15, 2021
@Pacman99
Copy link
Member Author

Sure, by the way, Do you have a plan to add the sops-nix module for extending secrets?

Not currently, I also don't entirely understand how to use sops-nix. I opted for agenix here due to simplicity.

For the most part secrets management is documentation, so I'm sure a doc addition to explain sops-nix usage with devos would be very welcome. We could discuss adding sops-nix to the template by default with such a change.

@blaggacao
Copy link
Contributor

When comparing sops-nix and agenix, I cannot see how sops-nix could outperform agenix or even complement with additional features.

Maybe there is a complementary element which I havn't been able to recognize, yet.

As such, I would think we can relatively safely limit our attention to agenix. No?

@Pacman99
Copy link
Member Author

Maybe there is a complementary element which I havn't been able to recognize, yet.

This is what I was thinking. And it looks like @GTrunSec's repo shows that. They used agenix and sops-nix together. At the very least I think documentation on how to use sops-nix with devos would be good. And I would be open to discussion on how it can be integrated in the template along with agenix.

Also I know we are considering multiple templates in the future. So maybe one of those could have sops-nix.

@blaggacao
Copy link
Contributor

blaggacao commented May 15, 2021

Agree, while let's remember:

As long as we maintain clear, and well-argumented preferences, any documented alternative still is pareto-dominated, as it doesn't burden an unfeasible choice especially on our newcomer user persona (let's call him "Novatus" from now on).

@blaggacao
Copy link
Contributor

blaggacao commented May 20, 2021

I'm putting on my "Novatus" cap, while I start to review our documentation.

What's holing this back? I think it is a very useful improvement.

@Pacman99
Copy link
Member Author

What's holing this back? I think it is a very useful improvement.

I think its ready to go, feel free to merge. I think devshell additions and any other agenix features can be added in separate PR's.

I'm putting on my "Novatus" cap, while I start to review our documentation.

I think more extensive doc changes should target develop. A little to prevent merge conflicts, but also to improve the review process of doc changes.

@divnix divnix deleted a comment from bors bot May 20, 2021
@blaggacao
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented May 20, 2021

Build succeeded:

@bors bors bot merged commit da9f14c into divnix:develop May 20, 2021
@blaggacao
Copy link
Contributor

blaggacao commented May 21, 2021

Looking at it from #20 angle, we probably wanted to implement some of the very nice devshell enhancements through #11 for agenix. 😜

@Pacman99
Copy link
Member Author

Pacman99 commented May 21, 2021

@GTrunSec's comments were one of my main motivations for #11. I would like to see a devshell.toml in this repo.

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.

4 participants