Add support for wireguard secrets. #2109
Conversation
Remove secret source and inline secret handling for simplicity.
|
+1 for this notation. It is commonly used by many docker images, they have a reference to it in the docs. And it is a more secure pattern to use files. Only suggestion would be adding more env vars to be able to be set by this mechanism |
|
Any thoughts on the existing mechanism: #2024 (comment) ? |
|
I saw the existing implementation which is non standard and does not work for wireguard. I orignally planned to just add support for wireguard using the existing implementation but found the code quite difficult and complicated to follow. That there are two pull requests regarding this feature and an issue should be quite telling that there is some room for improvement here. I also wrote my thoughts on the top. This PR reduces code quite a bit. The comments and longer test actually have it roughly even out with locs. What you do with this information and PR is up to you. |
There was a problem hiding this comment.
Thanks for the contribution, including test and comments 💯
However, I feel like this is trying to fix a non-existent problem (those _SECRETFILE variables do work, right?), EXCEPT adding a secret file reading for Wireguard.
I had lots of trouble following all the different sources.
I don't think they add much value and are overly complex.
I would advise to remove the others too and use just one simple config loading module.
Fair point, and I agree and disagree:
- it was a single loading module a few years ago, and it was unmanageable. That's why it was split up, and source reading was separated from settings definitions (and defaults, validation etc.)
- I'm planning one day to migrate to https://github.com/qdm12/gosettings/tree/v0.4.0-rc9 which handles internally merging of sources and transformation of keys (i.e.
SOME_ENVbecomessome-envfor flags), I've done the migration for all my projects but Gluetun because there is a lot of nifty details and retro-compatibility that makes this hard. That would result in a single packageinternal/configurationand no subpackages at all 😉
Anyway, for now we should keep the existing structure with those 3 sources.
On top of this, doing major refactor of the configuration directory (here this is a bit of a major ones for a few configuration keys) usually results in mayhem, since it's really easy to miss details (even for me) given retro-compatibility, lowercasing, etc. so I prefer to avoid unless very necessary and rare.
| WIREGUARD_PRIVATE_KEY_FILE=/run/secrets/wireguard_private_key \ | ||
| WIREGUARD_PRESHARED_KEY= \ | ||
| WIREGUARD_PRESHARED_KEY_FILE=/run/secrets/wireguard_preshared_key \ | ||
| WIREGUARD_PUBLIC_KEY= \ | ||
| WIREGUARD_PUBLIC_KEY_FILE=/run/secrets/wireguard_public_key \ |
There was a problem hiding this comment.
Let's load in a toml wireguard config file instead? See prior related discussion on #610 (comment)
| httpProxy.User = s.env.Get("HTTPPROXY_USER", | ||
| env.RetroKeys("PROXY_USER", "TINYPROXY_USER"), | ||
| env.ForceLowercase(false)) | ||
| httpProxy.User, err = s.readSecretFileAsStringPtr( | ||
| "HTTPPROXY_USER", | ||
| "/run/secrets/httpproxy_user", | ||
| []string{"HTTPPROXY_USER_SECRETFILE", "PROXY_USER", "TINYPROXY_USER"}, | ||
| ) | ||
| if err != nil { | ||
| return httpProxy, fmt.Errorf("reading HTTP proxy user secret file: %w", err) | ||
| } | ||
|
|
||
| httpProxy.Password = s.env.Get("HTTPPROXY_PASSWORD", | ||
| env.RetroKeys("PROXY_PASSWORD", "TINYPROXY_PASSWORD"), | ||
| env.ForceLowercase(false)) | ||
| httpProxy.Password, err = s.readSecretFileAsStringPtr( | ||
| "HTTPPROXY_PASSWORD", | ||
| "/run/secrets/httpproxy_password", | ||
| []string{"HTTPPROXY_PASSWORD_SECRETFILE", "PROXY_PASSWORD", "TINYPROXY_PASSWORD"}, | ||
| ) | ||
| if err != nil { | ||
| return httpProxy, fmt.Errorf("reading HTTP proxy password secret file: %w", err) | ||
| } |
There was a problem hiding this comment.
This is rather messy and mixes up too many things.
Let's leave the env source as it was, however you could merge the secrets source in the files source, although that can get messy too eventually. We should do this in a v4 release really, where the user specifies each file path with an env variable, whether the file is a secret file or a standard file, Feel free to add to #1113
|
Also see previous PR: #1348 |
|
#1348 got merged so now Wireguard secrets (as config file or as individual field-files) are supported. Therefore this PR becomes a bit unneeded, EXCEPT to add a comment to #1113 about using just |
qdm12
force-pushed
the
master
branch
2 times, most recently
from
bba6a9f
to
23b0320
Compare
Closing this since it's no longer necessary, but thanks for the discussion and coding put into it 💯 |
|
Awesome! I will try it soon. |
Removed the secret Source and inlined secret handling for simplicity.
Secrets are important because I like to commit my env and yml files.
Addresses: #2024
Note:
I had lots of trouble following all the different sources.
I don't think they add much value and are overly complex.
I would advise to remove the others too and use just one simple config loading module.
This was my first time writing go code so if you want any changes let me know.
Please test it against your setup for the various providers.
I can also update the documentation/wiki but first I want to be sure this is satisfactory.