Skip to content

security/openconnect: New plugin#462

Merged
fichtner merged 55 commits intoopnsense:masterfrom
mimugmail:openconnect
Jan 6, 2018
Merged

security/openconnect: New plugin#462
fichtner merged 55 commits intoopnsense:masterfrom
mimugmail:openconnect

Conversation

@mimugmail
Copy link
Copy Markdown
Member

New plugin, allows in this stage to edit server/user/pw, creates a tab in firewall for rules management.
Until stable I'd need to a status tab, but the application itself doesn't offer logging or a socket to connect to.

The scripting could need some fixes, especially the sleep in the rc script, I need a way that the command waits until the interface is there. When the VPN server is a bit slow this could take 1-3 seconds. Any ideas?

Also @fichtner already mentioned the problem with piping the password, but the application doesn't offer a config file. All the rest should be ok/standard and mutable codebase.

Ah, I added "sh" to actions since the chmod is always wrong after templating. You could remove this after merge/chmod.

@fichtner fichtner self-assigned this Jan 3, 2018
# start openconnect
openconnect_start()
{
{% if helpers.exists('OPNsense.openconnect.general.enabled') and OPNsense.openconnect.general.enabled == '1' %}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can't template this script, templating the /etc/rc.conf.d/opnsense-openconnect file should be enough to transport the settings for this

Password could be dumped as a separate file in 500 mode and then cat $file | pushed..

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is still true, we cannot template this script, it needs to go to /usr/local/etc/rc.d/ directly

@mimugmail
Copy link
Copy Markdown
Member Author

Ok, I added some validations and reworked the config and password process.
The password file needs a chmod 500.

{% if helpers.exists('OPNsense.openconnect.general.password') and OPNsense.openconnect.general.password != '' %}
echo "starting openconnect"
echo "{{ OPNsense.openconnect.general.password }}" | /usr/local/sbin/openconnect -u {{ OPNsense.openconnect.general.user }} {{ OPNsense.openconnect.general.server }} --pid-file=/var/run/openconnect.pid -b -q -i tun30000 2>&1 > /dev/null
cat "$secret" | /usr/local/sbin/openconnect --config=/usr/local/etc/openconnect.conf {{ OPNsense.openconnect.general.server }} 2>&1 > /dev/null
Copy link
Copy Markdown
Member

@fabianfrz fabianfrz Jan 5, 2018

Choose a reason for hiding this comment

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

can you try < as a pipe (place it behind the command as well )

@mimugmail
Copy link
Copy Markdown
Member Author

Good idea @fabianfrz .. done.

@mimugmail
Copy link
Copy Markdown
Member Author

@fichtner I reworked the rc stuff, now there's only templating in rc.d

@fabianfrz
Copy link
Copy Markdown
Member

@mimugmail the RC file should not be a template. You can fix this by moving it directly to etc and remove it from +TARGETS

@mimugmail
Copy link
Copy Markdown
Member Author

@fabianfrz oh .. yes :) fixed

<?php

/**
* Copyright (C) 2015 - 2017 Deciso B.V.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove

namespace OPNsense\Openconnect\Api;

use OPNsense\Base\ApiMutableServiceControllerBase;
use OPNsense\Core\Backend;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove

Copy link
Copy Markdown
Member

@fichtner fichtner left a comment

Choose a reason for hiding this comment

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

very good work

two more things after merge:

  • will look into allowing to use any interface name so we don't have to rename afterwards
  • the rc script will likely be moved to FreeBSD ports in a not so distant future


$services = array();

if (isset($config['OPNsense']['openconnect']['general']['enabled']) && $config['OPNsense']['openconnect']['general']['enabled'] == 1) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I forgot: this is basically "if (openconnect_enabled()) {"

in that case "global $config;" should be removed, too

function openconnect_enabled()
{
$model = new \OPNsense\Openconnect\General();
if ((string)$model->enabled == '1') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

code simplification:

return (string)$model->enabled == '1';

@fichtner fichtner merged commit db06a4d into opnsense:master Jan 6, 2018
@fichtner
Copy link
Copy Markdown
Member

fichtner commented Jan 6, 2018

Merged, will tweak as stated. Thanks!

fichtner pushed a commit that referenced this pull request Jan 6, 2018
(cherry picked from commit db06a4d)
(cherry picked from commit a2091db)
(cherry picked from commit 76a866f)
fichtner pushed a commit that referenced this pull request Jan 6, 2018
(cherry picked from commit db06a4d)
(cherry picked from commit a2091db)
(cherry picked from commit 76a866f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Adding new functionality

Development

Successfully merging this pull request may close these issues.

3 participants