-
Notifications
You must be signed in to change notification settings - Fork 647
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
Autossh plugin for OPNsense #818
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some stuff tried to not repeat myself
| $response = array("status"=>"success", "data" => $model->isConfigChange(), "message"=>null); | ||
| } | ||
|
|
||
| return $response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $response variable is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning early has been applied in all controllers - it's an old habit
|
|
||
| public function startAction() | ||
| { | ||
| $response = array('status'=>'fail', 'message' => 'Invalid request'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the same spacing style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| 'local' => htmlspecialchars($tunnel['local_forward']), | ||
| 'dynamic' => htmlspecialchars($tunnel['dynamic_forward']), | ||
| 'remote' => htmlspecialchars($tunnel['remote_forward']), | ||
| 'tunnel' => htmlspecialchars($tunnel['tunnel_device']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an api, not a html renderer. No escaping here. Especially because of this ugly hack:
opnsense/core#2672
Escaping must be done in the viewer - which is the js code of the frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm aware of opnsense/core#2672 if you take a look at my TunnelsController I specifically have an afterExecuteRoute() function to be able to step outside it in a very limited case - even with me implementing this I guess it did not occur to me that using htmlspecialchars() here is in-fact double-escaping things - either way, resolved
|
|
||
| $response['rows'][] = array( | ||
| 'uuid' => $tunnel['uuid'], | ||
| 'connection' => htmlspecialchars($connection), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see prev
| <id>tunnel.description</id> | ||
| <label>Description</label> | ||
| <type>text</type> | ||
| <help><![CDATA[Free form text field available to describe this connection if required.]]></help> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No cdata needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay - CDATA removed from the fields where it is not needed
| public function setConfigChangeOn() | ||
| { | ||
| touch($this->config_status_filename); | ||
| return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead you can just return nothing
| <div class="col-md-12"> | ||
| <h1>Autossh</h1> | ||
| <p> | ||
| The Autossh plugin for OPNsense is a tool for establishing, maintaining and managing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs translation.
| }, | ||
| code_wrap: function(column, row) { | ||
| if(row[column.id].length > 0) { | ||
| return '<code>' + row[column.id] + '</code>'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the places, where you would have to escape if this issue would not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an excellent catch
|
|
||
| $logfile = '/var/log/autossh.log'; | ||
| $logclog = true; | ||
| // $service_hook = 'autossh'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete dead code
|
@fabianfrz super appreciate the rapid turn around on feedback here, it really impresses me about the OPNsense team - I'm going to be busy on another project for the next week and will not be able to address these in the same quick turn around |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small things. I like the idea of autossh. Normally I'd do this with OpenVPN but SSH as an alternative is also nice
net/autossh/Makefile
Outdated
| PLUGIN_COMMENT= Create and automatically persist SSH based tunnels. | ||
| PLUGIN_MAINTAINER= contact@verbnetworks.com | ||
| PLUGIN_WWW= https://verbnetworks.com | ||
| PLUGIN_DEVEL= no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
net/autossh/src/etc/rc.d/autossh
Outdated
| @@ -0,0 +1,182 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally whene there's no init script it's called opnsense-xxx so there wont be a collision when a new script comes in future by pkg maintainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh - I recall wondering about this - thanks, and sorting it out now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also dont forget in rc.conf.d .. 👍
|
|
||
| public function infoAction($uuid = null) | ||
| { | ||
| $ssh_key_restrictions = 'command="",no-agent-forwarding,no-pty,no-user-rc,no-X11-forwarding'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed in the volt, no idea if this is really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - displaying the ssh public key to the user in a manner that is designed to prevent other security accidents is a deliberate thing as there will be some that may not understand the potential flow on effects - anyone that wants to strip off the key restrictions ought to know what they are doing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, looked a bit strange in UI bit I didn't have a deeper look into it yet.
| <id>key.type</id> | ||
| <label>Type</label> | ||
| <type>dropdown</type> | ||
| <help>Select the type of SSH key to be generated - can't be modified after the key has been generated.</help> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cannot
| <id>key.description</id> | ||
| <label>Description</label> | ||
| <type>text</type> | ||
| <help>Free form input to describe this sshkey (not parsed).</help> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here sshkey, below SSH key ..
| <id>tunnel.user</id> | ||
| <label>User</label> | ||
| <type>text</type> | ||
| <help><![CDATA[Provide the connection username for this connection.]]></help> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove the first connection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sharp eyes 👍
| </optionvalues> | ||
| </macs> | ||
|
|
||
| <port type="IntegerField"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also use PortField, no validation or min/max required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh cool - I did not know
|
|
||
| <div class="alert alert-info hidden" role="alert" id="responseMsg"></div> | ||
| <div class="alert alert-info hidden" role="alert" id="applyChangesMessage"> | ||
| <button class="btn btn-primary pull-right" id="btnApplyChanges" type="button"><b>{{ lang._('Apply changes') }}</b> <i id="btnApplyChangesProgress"></i></button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me of legacy code how to apply changes, searched some time after I found the button :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took this approach because I could not see a better way to handle it
The underlying thing driving this is the need to call a helper script to correctly render the required ssh_config files because as best as I can tell I am not able to get what I need with Jinja alone - go looking for AutosshConfigHelper.py if you are interested
|
Found this in system.log: |
|
These log messages are because you have managed to get yourself into a condition where the |
|
Also - another thing here, it's is not clear to me how to "catch" these particular |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ndejong,
I apologise for the delay. Here are preliminary comments for eventual inclusion:
- The plugin is pretty big which makes initial review difficult. Next step for me would be to use it in action to get a feel for the whole thing.
- Kind of split on the OPNsense vs. vendor namespace as initially suggested. Ideally different vendors should be reserved for other projects, commercial and custom plugins. It doesn't matter that much, but it would be nicer to have a single "open source upstream" namespace. What do you think?
- rc.d script is quite complex and may be nice to have in FreeBSD per default.
Cheers,
Franco
| . /etc/rc.subr | ||
|
|
||
| name=autossh | ||
| rcvar=${name}_enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will eventually clash with freebsd when an rc.d script is added in the package: rename var or maybe add a FreeBSD rc.d script?
| @@ -0,0 +1,24 @@ | |||
| Copyright (c) 2018 Verb Networks Pty Ltd <contact@verbnetworks.com> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is implicit from the code lines. and will be in the main LICENSE file merged with all other contributors, but I don't have issues pulling this in. :)
| @@ -0,0 +1,30 @@ | |||
| # Autossh | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes more sense to move this to pkg-descr -- we use the FreeBSD description for general direction, but if you have more to say about your plugin that is the spot since you can read it from the GUI.
| <menu> | ||
| <VPN> | ||
| <Autossh VisibleName="Autossh" cssClass="fa fa-chain fa-fw"> | ||
| <AutosshTunnels VisibleName="Tunnel Settings" url="/ui/autossh/settings" order="10"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly opting for easier node names for sorting in larger menus, but so not relevant here. just a comment.
|
@ndejong @fabianfrz is there an update on this pull-request so that it might get integrated into the distributed plugins? |
|
@fichtner I think the question is more for you. |
|
More than happy to jump in and give life to this. The biggest bump-in-the-road was the namespacing which in 2018 was not super-clear, I went down the wrong garden path which deflated some of the enthusiasm at the time - all good, if there is an interest pretty easy to fix. The things that probably stand out at the moment are -
As it happens, I've recently started a new fork on the configsync-plugin to address the Python3 and namespace concerns and I'm more likely to get that one out first. |
|
This hasn't been moving since 2018 so after internal discussion this can no longer be included in the current form. Also we are still concerned about sizeable first-time contributions as our community review time is not unlimited. Cheers, |
|
Fair enough, no one could fault the OPNsense team on this @fichtner @fabianfrz For my part, I wrote both #818 (Autossh plugin) and #777 (Configuration Sync) at a time when I had a few months spare time and was interested in supporting anything that was not the "other" pf based firewall distribution. Having pained through the efforts of writing FauxAPI for that other distribution it was awesome to discover OPNsense had managed to introduce an MVC framework which made it much easier to write these two substantial plugins - finally some development sanity! I have another OPNsense plugin in the long-slow cooker around Slack Nebula but I kinda expect someone else might be able to get that to happen before me - perhaps others are already working on this? If I do find time to bring these to life again it'll be a new pull-request via my personal @ndejong Github. Regards, |
This PR implements a UI for the autossh package thus providing the ability to create and persist SSH tunnels from the OPNsense system to remote systems.
Features
Various use cases
Looking forward to feedback