search

githubixx / ansible-role-wireguard

Ansible role for installing WireGuard VPN. Supports Ubuntu, Debian, Archlinx, Fedora and CentOS.
https://www.tauceti.blog/post/kubernetes-the-not-so-hard-way-with-ansible-wireguard/
576 stars 181 forks source link

Put private key into separate file instead of main config #78

Open ypid opened 4 years ago

ypid commented 4 years ago

Reasons:

Example:

PostUp = wg set %i private-key /etc/wireguard/%i.privkey

Downsides:

What do you think?

The same should then be done for PSKs, although I have no file naming schema yet.

ypid commented 4 years ago

The bad thing in that case would be that you basically need to parse the configuration file with every run if the private key file isn't stored in a separate file. Of course it would be possible to create a file with the private key if it don't exist and derive the public key from it.

Ref: https://github.com/githubixx/ansible-role-wireguard/issues/18#issuecomment-532408488

Why didn’t you go for this approach back then?

githubixx commented 4 years ago

Well... Being able to show someone a config file doesn't really justify PR to move the private key out of the WireGuard config TBH. :wink: That totally depends on your point of view. If you talk to one of the cloud native guys and tell him that you ssh into a VM/container/pod/whatever to show someone a config file that contains a secret he/she will jump straight out of the window. :smile:

I mean that's the WireGuard config, right? I personally would absolutely expect that there is a secret in it. You need to store it somewhere. Other enterprise people will tell you "you're absolutely crazy" because secrets belong into Hashicorp's Vault and what not. But even then sooner or later you need to store that key somewhere. It's the same with OpenVPN, PeerVPN, and so on. And using a regex to extract some information from a file is perfectly valid IMHO. The config file and the secret are entities that belongs together. Personally I think that's not worth the effort. While it's maybe nice to have it, it's at least a change that requires to migrate the current state to another state. And that might introduce problems and errors. For me it's important to keep the thing stable, maintainable and usable.That's my No. 1 priorities. Not all fancy things make sense. Breaking or "state transition" changes are needed from time to time that's for sure but they are not justified every time.

And regarding your previous comment: This role is a few years old already. Different people have different needs. So various PRs were introduced that where useful at that point of time. You can always ask why did you that, why this... If I merged something in the past then it was either useful or needed or I just liked the idea at THAT point of time. If I would start again I probably would do it differently. But the "why" question is pointless for small open source projects. Just assume that at a specific point of time a change was okay and maybe even needed. If it still makes sense today is a completely different story. wg syncconf wasn't there at the beginning e.g. So a restart was needed basically.

That being said now shortly regarding #80: Why an external dependency to crudini should be introduced? E.g. for Archlinux it's not even in the standard Arch repository, You would need to install it from AUR and that's definitely a no go. AUR is still needed for older kernels to install the WireGuard DKMS package. But in this case it's just the best option. I've no idea about Fedora and other distributions. I definitely want one handler for all distributions and not a different one for every distribution. That adds complexity that needs to be avoided. If Ansible can handle it then I normally try to implement it the Ansible "native" way. There were cases in the past (and you fixed some of them) and maybe there are still cases were this isn't true but as already said roles or software in general evolves over time and things you did in the past, might be done differently if you do it years later :wink: But still you've to consider what's already out there and used possibly in production.

ypid commented 4 years ago

Thanks for the input. I think the why question is relevant in the case when the reasons back in the day are still valid today. So that I could reevaluate your decision making process so that I have the full context. That is why I asked.

I consider this still experimental. I am working on a full POC including PSK support to see how difficult it is (which will then probably target DebOps too much for you to consider merging it). Even I am not yet fully convinced that this change is the right approach. That is why I opened the issue. The advantages just look too interesting for me to ignore them just because they might be difficult. Also consider my direction in #65 and that I now see it as unavoidable to fork the role in the near future.

The cloud people, that is a different story :) I think the shoulder surfing aspect is also relevant here, but maybe in a more automated/less obvious way where machines see things they should not see. Consider cloud native people running their daily config drift checks with Ansible and --diff. This the can/will include private keys. This log might end up in an unprotected log collection system. VPN tokes are sensible where because they are often involved in the first stages of attacking an organization. (I am just teasing the cloud people a bit, I hope you forgive me :) )

Sure, the secrets need to be stored somewhere. I am not of those manager smartypants who says we need to "magically" encrypt it all (but I do know Hashicorp's Vault). Some secrets need to be accessible to hosts, one way or the other.

crudini is currently part of this POC. I have used it before, also on different distros and found it useful.

I updated the pro arguments in my initial comment.

githubixx commented 4 years ago

TBH I really think it makes sense for you to fork this project. That way you can really change it to your needs. Your way of thinking is totally the DebOps way and this doesn't really fit to this role. While DebOps tries to be the "one thing for everything" - at least that's my impression - this role is the exact opposite :wink: I really want to solve exactly one thing and only that one thing with least dependencies as possible. And my impression is that that's also true for most of the people who currently use this role. DebOps users are probably completely different and that's perfectly okay. Users should have choices and take the solution that fits their needs best.

In DebOps you've the concept of a controller if I get that right. While this concept doesn't really fit for this role I'm pretty sure some people out there run this role on a Jenkins server or other kind of continuous delivery software which you can also consider kinda controller. Normally this entity is considered a trusted resource and therefore also suited to store private keys IMHO. People can also use Vault or ansible-vault there if they want to even more security. So personally I think it's this role can also be be used to generate private keys for unmanaged devices e.g. But that's a different story and refers more to #70 .

ypid commented 4 years ago

TBH I really think it makes sense for you to fork this project.

Works for me. My idea is just to make less invasive changes first so that you can also profit from them directly. And I think that worked with #67. I guess now come the more opinionated parts :)

(I already work on DebOps specific stuff in https://github.com/ypid/ansible-wireguard/tree/prepare-for-debops)

I will report back how my little experiment goes.

ypid commented 4 years ago

Why an external dependency to crudini should be introduced?

You got me thinking. As I need proper support to also cover PSKs and WireGuard seems to fall short here, I am looking into supporting this in wg-quick directly with the intend of upstreaming it. Then everybody can benefit.

ypid commented 4 years ago

I have a POC. https://github.com/ypid/wireguard-tools/tree/feature/wg-quick/strip-and-eval https://github.com/ypid/ansible-wireguard/tree/prepare-for-debops

znerol commented 3 years ago

Slightly OT but the strip-and-eval stuff is not really necessary. @ypid you might want to simply omit the private-key from the interface section in the wireguard config file. And instead add a systemd unit drop-in to load the it using ExecStartPost. I.e.:

In /etc/systemd/system/wg-quick@.service.d/override.conf:

[Service]
ExecStartPost=/usr/bin/wg set %i private-key /etc/wireguard/wg-%i-private.key
ypid commented 3 years ago

That is an idea. Have you tested if this causes a new handshake? This is my reason I want to stick with wg syncconf.

About the OT. The issue is till open here. We can move to https://github.com/ypid/ansible-wireguard/issues if @githubixx prefers that.

znerol commented 3 years ago

Verified that. So the thing is that you need to load the private key again after syncconf. Thus we need to add that to the override:

/etc/systemd/system/wg-quick@.service.d/override.conf:

[Service]
ExecStartPost=/usr/bin/wg set %i private-key /etc/wireguard/%i-private.key
ExecReload=/usr/bin/wg set %i private-key /etc/wireguard/%i-private.key

And yes, that does trigger a handshake.

ypid commented 3 years ago

And yes, that does trigger a handshake.

We probably don’t need to care about reload triggering a handshake, right.

But depending on how many peers you have, the override.conf will get rather long for PresharedKey. Considering that I already implemented it, I don’t see the benefit of switching to this suggestion.

rbeede commented 3 years ago

It could be useful to offer two possible value types in a wg0.conf for PrivateKey or PublicKey.

PrivateKey = U29tZSBCYXNlNjQgRW5jb2RlZCBUZXh0IEZvciBZb3U

or

PrivateKey = file:///etc/wireguard/secrets/peer-acme.key

Although maybe that gets into not having a single .conf for all peers/interfaces but a directory for each section and something that just assembles all the conf files into one. Might be more value in just writing a tutorial on how to automate that instead.