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

added OPNsense functionality #3

Open
wants to merge 3 commits into
base: split-files-clean
Choose a base branch
from

Conversation

pkoevesdi
Copy link

Since on our site the user cannot make the full wg configuration alone on his site without having credentilas for the OPNsense to lookup an IP address, our workflow is different than written in the README.md. Further more, it's ok for us that the admin potentially knows the user's PrivateKey. So, our admin makes the client config, settles the new client in the OPNsense webgui, and passes the client config via secure way (USB stick or Signal messenger) to the client computer.
But putting the new client to the OPNsense is very entangled, especially when the admin himself is connected via wireguard to the webgui, since the webgui cannot restart the wireguard service - System -> Diagnostics -> Services -> Restart doesn't adapt the configuration changes. Instead, it can only be stopped and then started. Since he himself is connected via wireguard, after stopping, he cannot start again. So, he has to connect via an SSH route to a virtual admin machine on the OPNsense server side... takes all together like 15 minutes.
That's why I wrote this OPNsense bridge part using the OPNsense wireguard API.

@pkoevesdi pkoevesdi force-pushed the opnsensebridge branch 2 times, most recently from 66beb19 to e9acd40 Compare April 29, 2023 19:16
@jcarrano
Copy link
Owner

jcarrano commented Aug 5, 2023

From what I gather, this feature may be useful for other OPNsense users (I myself have no idea since I never used OPNSense).In that case, it makse sese to include it. However:

  • I'd rather have it under a ./contrib directory to clearly indicate it is an "add-on" functionality.
  • Next to the script, README with the same information contained in this PR description would help users understand what it does.
  • As it stands now, the modifications feel quite intrusive to the original tool. I'm trying to think how the tool can be extended without such heavy modifications.

@pkoevesdi
Copy link
Author

I took a look at it. But I see it better in an own branch than in a subfolder. Since this is not a add-on, it's the full program with additional functionality that fully stands alone. If You put it in a subfolder, You cannot cherry-pick fixes etc. With a branch, a user can more easily choose the flavor of the program he wants to use.
Also, the Readme has been already updated with the additional function, also with statement concerning the contacting remoe server.

removed field for subnet prefix, to allow multiple subnets (also with different prefixes)

added direct display of configs, with copylink to clipboard

improved look and feel

established w3c validator compliance

established w3c validator compliance, removed link underlines

added OPNsense functionality

added several error catchings and gui and code improvements

removed admin email address field - it is easier to fill out inside email program with features like autocomplete and address book

cleaned up code

cleaned up code

improved and cleaned up code

removed redundant id tags

some more error catching

added PSK autogenerate function
added missing push of PSK to OPNsense
marked client name also as relevant for client config (it is for the server fragment)

added PSK autogenerate function

added missing push of PSK to OPNsense
marked client name also as relevant for client config (it is for the server fragment)

added PSK autogenerate function
added missing push of PSK to OPNsense
marked client name also as relevant for client config (it is for the server fragment)

lot of refactoring and improving

added Client name as comment into config

removed unused dep

small fix in server config
@pkoevesdi
Copy link
Author

I did a rebase here too.
Actually, we could even think of merging the two branches, since if the OPNSENSE-functionality is not used, it works exactly like the main branch. So why eliminate a function that a user easily can ignore?
Just a thought...

@jcarrano
Copy link
Owner

I took a look at it. But I see it better in an own branch than in a subfolder.

The thing that is complicated about my code is that it is not easy to extend the tool without either invasive modification or making the code bigger in a way that it is more work to audit.

Perhaps it could be improved I rewrote the code to actually follow a definite programming paradigm instead of just a bunch of functions. I'm still thinking about this stuff and maybe having different branches/flavors is the best approach.

One thing I must admit is that I have not been using WG much lately.

Copy link

Github deployment failed. Check out the build logs for debugging. Please reach out if you get stuck - we're here to help!

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.

2 participants