A new plugin to allow SCP config backups to a remote host. #458

dharrigan · 2018-01-02T15:44:16Z

This simple plugin takes a few parameters, such as hostname and username and
schedules a cron job to remotely scp the config.xml file at regular intervals.
It uses the public/private keypair of the built-in root user as the source of
the key exchange. This means that the public key must be copied to the remote
host and added to the authorized_keys file (for the defined user).

The remote file is backed up as config-YYYY-DD-MM-HH-MM.xml, for example:
config-2018-01-02-15-40.xml. It's possible to change the remote location of
where the config file is backed up to.

The cron job can be modified under System/Settings/Cron and the schedule
adjusted to suit the backup frequency requirements.

-=david=-

closes #457

fabianfrz

In my opinion this should not be here but included into the core page for backups. As long as there is a RCE this must not be merged.

See comments for other stuff.

fabianfrz · 2018-01-02T18:33:15Z

sysutils/scp-backup/src/opnsense/scripts/OPNsense/ScpBackup/ScpBackup.php

+    }
+
+    $remoteLocationFullPath = $remoteLocation . "config-" . date('Y-m-d-H-i') . ".xml";
+    $command = "scp -P $port -i $identifyFile /conf/config.xml $username@$hostname:$remoteLocationFullPath";


Looks like RCE

I don't believe it is - can you show if it is? Happy to change if it's provable. I do escape the command as per PHP guidelines.

You are using data from config.xml unescaped in the shell command - escape them with https://secure.php.net/manual/en/function.escapeshellarg.php

the variables that need escaping are

$username

$hostname

$remoteLocationFullPath

Fantastic! Thank you. I'll do that :-)

-=david=-

fabianfrz · 2018-01-02T18:34:40Z

sysutils/scp-backup/src/opnsense/scripts/OPNsense/ScpBackup/ScpBackup.php

+    }
+}
+
+exit(0);


fabianfrz · 2018-01-02T18:35:41Z

sysutils/scp-backup/src/opnsense/scripts/OPNsense/ScpBackup/ScpBackup.php

+if(isset($scpBackup) && isset($scpBackup->enabled) && $scpBackup->enabled == 1) {
+    $hostname = $scpBackup->hostname;
+    $port = empty(trim($scpBackup->port)) ? "22" : $scpBackup->port;
+    $username = empty(trim($scpBackup->username)) ? "root" : $scpBackup->username;


usernames cannot include whitespace

That is true, however, someone may submit the form with a blank username, thus doing additional check here just to be safe and sure and to default to "root" if nothing is supplied.

yes but you won't need trim if you would do proper validation

fabianfrz · 2018-01-02T18:37:09Z

sysutils/scp-backup/src/opnsense/mvc/app/models/OPNsense/ScpBackup/General.xml

+    <items>
+        <enabled type="BooleanField">
+            <default>0</default>
+            <Required>Y</Required>


indent issue

fabianfrz · 2018-01-02T18:39:20Z

...ils/scp-backup/src/opnsense/mvc/app/controllers/OPNsense/ScpBackup/Api/GeneralController.php

+        $result = array();
+        if ($this->request->isGet()) {
+            $mdlGeneral = $this->getModel();
+            $publicKey = fopen("/conf/sshd/ssh_host_rsa_key.pub", "r");


this might be the wrong key - it is only or the server to show its identity to the client. It should not be used for that.

I believe it is the correct key, as this is the only available to me - "root" on OPNsense doesn't have a default generated public/private keypair - Fitch pointed me to this particular file.

fabianfrz · 2018-01-02T18:40:56Z

sysutils/scp-backup/Makefile

+PLUGIN_NAME=		scp-backup
+PLUGIN_VERSION=		1.0.0
+PLUGIN_COMMENT=		Perform config backups using SCP.
+PLUGIN_MAINTAINER=	dharrigan@gmail.com


Should be devel first

Updated as per Fitch's comment below.

fabianfrz · 2018-01-02T18:42:38Z

sysutils/scp-backup/src/opnsense/mvc/app/controllers/OPNsense/ScpBackup/forms/general.xml

+  </field>
+  <field>
+    <id>general.remoteLocation</id>
+    <label>Remote Location</label>


fabianfrz · 2018-01-02T18:44:07Z

sysutils/scp-backup/src/opnsense/mvc/app/controllers/OPNsense/ScpBackup/forms/general.xml

+  </field>
+  <field>
+    <id>general.publicKey</id>
+    <label>Local Root's Public Key</label>


Should not be the one of root or anyone else. Should be an autogenerated key for this service. Should also support passwords as well.

Maybe the fingerprint should be configurable as well for the remote server.

This was agreed with Fitch.

Happy to accept PR's for passwords, fingerprints etc...

fabianfrz · 2018-01-02T18:45:37Z

sysutils/scp-backup/src/opnsense/mvc/app/models/OPNsense/ScpBackup/General.xml

+        </username>
+        <remoteLocation type="TextField">
+          <Required>N</Required>
+          <ValidationMessage>Please provide a remote location.</ValidationMessage>


Happy to change it to RemoteDirectory :-)

please no camelcase in strings - that's already hard to read in the code ;)

fabianfrz · 2018-01-02T18:45:58Z

sysutils/scp-backup/src/opnsense/mvc/app/models/OPNsense/ScpBackup/General.xml

+          <Required>N</Required>
+          <MinimumValue>1</MinimumValue>
+          <MaximumValue>65535</MaximumValue>
+          <ValidationMessage>Please provide a valid port number between 1 and 65535. Port 22 is usually the default.</ValidationMessage>


remove usually

fichtner · 2018-01-02T19:18:32Z

@dharrigan thanks for this, I'll do further review tomorrow!

Just a thought: this is probably core-worthy, especially since it has no external dependencies. As such it would be better to start with PLUGIN_DEVEL and a version 0.x so we can add it to the plugins, but have a bit of time to decide what we are going to do with it mid-term. How does that sound? :)

dharrigan · 2018-01-02T19:22:30Z

Hi,

Sure, I can update the version. Fabian has raised some points, a lot of which you and I have already discussed off-line privately. Can we co-ordinate on this?

-=david=-

dharrigan · 2018-01-02T19:34:58Z

I do validate it, however, it's down as an optional field and the form may be submitted with this field being blank. This check here, makes sure that if it is blank, I default to root. I could make the form field mandatory, prepopulating it with "root" - would that be better?

…

On 2 January 2018 at 19:25, Fabian Franz, BSc. ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sysutils/scp-backup/src/opnsense/scripts/OPNsense/ ScpBackup/ScpBackup.php <#458 (comment)>: > + * + */ + +require_once("util.inc"); +require_once("config.inc"); + +use OPNsense\Base; +use OPNsense\Core\Config; + +$config = Config::getInstance()->object(); +$scpBackup = $config->OPNsense->ScpBackup; + +if(isset($scpBackup) && isset($scpBackup->enabled) && $scpBackup->enabled == 1) { + $hostname = $scpBackup->hostname; + $port = empty(trim($scpBackup->port)) ? "22" : $scpBackup->port; + $username = empty(trim($scpBackup->username)) ? "root" : $scpBackup->username; yes but you won't need trim if you would do proper validation — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#458 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaEjnkOJy0L2SDr4MbIFB7PFwMJzsjPks5tGoLDgaJpZM4RQwCX> .

-- I prefer encrypted and signed messages. Fingerprint: 110A F423 3647 54E2 880F ADAD 1C52 85BF B20A 22F9 No trees were harmed in the sending of this message, however, a number of electrons were inconvenienced.

fabianfrz · 2018-01-02T19:45:28Z

@dharrigan you can do it however you like but you should provide a mask in any case. This is for no whitespace for example: /\S*/

For usernames it is probably more complex like characters, numbers and a limited set of special chars (dot, dash, underscore, maybe plus) ->/^[a-z0-9\.\-_\+]*$/

dharrigan · 2018-01-02T19:47:35Z

No problem, I'll look around for a valid regex for unixy usernames and apply some sensible default. -=david=-

…

On 2 January 2018 at 19:45, Fabian Franz, BSc. ***@***.***> wrote: @dharrigan <https://github.com/dharrigan> you can do it however you like but you should provide a mask in any case. This is for no whitespace for example: /\S*/ For usernames it is probably more complex like characters, numbers and a limited set of special chars (dot, dash, underscore, maybe plus) ->/^[a-z0-9.-_+]*$/ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#458 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaEjubMMufNhfYCXK4tPMKpo72Dro04ks5tGodZgaJpZM4RQwCX> .

-- I prefer encrypted and signed messages. Fingerprint: 110A F423 3647 54E2 880F ADAD 1C52 85BF B20A 22F9 No trees were harmed in the sending of this message, however, a number of electrons were inconvenienced.

dharrigan · 2018-01-02T21:09:19Z

Hi,

Feedback applied and new push made. I await feedback :-)

-=david=-

fichtner

looks good, but as discussed we will rewrite the backup script to use shell and a dedicated config file.

maybe the cron job handling can be altered as well using the plugin cron hook and a simple setting...

but none of this is pressing. we're aiming for inclusion in an early 18.1.x release.

dharrigan · 2018-01-03T09:37:20Z

Hi @fabianfrz,

I've spoken with @fichtner. He's happy enough with what I've done so far and also with the updates based upon your great review yesterday. He would me to merge in if you're happy - he has said that he's going to rewrite the code slightly to externalise the configuration of the plugin into its own configuration file and to re-write the cron'ed PHP script to be a shell script. He'll do that once this branch has been merged into master.

Are you happy enough with the code for me to merge in? You can always reach out to me on IRC if you want to go over a few things :-)

Speak to you later.

-=david=-

fabianfrz

Only some minor issues left - It's OK now.

fabianfrz · 2018-01-03T16:13:44Z

sysutils/scp-backup/src/opnsense/mvc/app/views/OPNsense/ScpBackup/general.volt

+        $("#saveAct").click(function() {
+            $("#saveAct_progress").addClass("fa fa-spinner fa-pulse");
+            saveFormToEndpoint(url="/api/scpbackup/general/set", formid='frm_general_settings', null);
+            $("#saveAct_progress").removeClass("fa fa-spinner fa-pulse");


this will probably not work because it's async - @fichtner this is for you

nice catch, it needs a return function :)

fabianfrz · 2018-01-03T16:15:00Z

sysutils/scp-backup/src/opnsense/scripts/OPNsense/ScpBackup/ScpBackup.php

+
+    $command = "scp -P $port -i $identifyFile $configFile $username@$hostname:$remoteDirectoryFullPath";
+
+    syslog(LOG_WARNING, "scp_backup command: $command");


warning?

is this needed - looks like a debug code which is still left

Hi, I tried LOG_INFO, but nothing came out on the syslog. I wanted to print out the command being executed, just in case something was blowing up and to help out the user. LOG_ERR seemed inappropriate :)

I would say this is LOG_DEBUG. However it is not risky because it does not spam the log.

dharrigan · 2018-01-03T16:23:37Z

Thank's @fabianfrz, I'll merge in shortly.

-=david=-

dharrigan · 2018-01-03T16:27:08Z

I'll add it in before merging in. -=david=-

…

On 3 January 2018 at 16:24, Franco Fichtner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sysutils/scp-backup/src/opnsense/mvc/app/views/ OPNsense/ScpBackup/general.volt <#458 (comment)>: > +POSSIBILITY OF SUCH DAMAGE. + +#} +<script type="text/javascript"> + $(document).ready(function() { + var data_get_map = {'frm_general_settings':"/api/scpbackup/general/get"}; + + mapDataToFormUI(data_get_map).done(function(data) { + formatTokenizersUI(); + $('.selectpicker').selectpicker('refresh'); + }); + + $("#saveAct").click(function() { + $("#saveAct_progress").addClass("fa fa-spinner fa-pulse"); + saveFormToEndpoint(url="/api/scpbackup/general/set", formid='frm_general_settings', null); + $("#saveAct_progress").removeClass("fa fa-spinner fa-pulse"); nice catch, it needs a return function :) — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#458 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaEjpfIXDmVRwuDCZasKYtp6P2dSoOcks5tG6m0gaJpZM4RQwCX> .

-- I prefer encrypted and signed messages. Fingerprint: 110A F423 3647 54E2 880F ADAD 1C52 85BF B20A 22F9 No trees were harmed in the sending of this message, however, a number of electrons were inconvenienced.

This simple plugin takes a few parameters, such as hostname and username and schedules a cron job to remotely scp the config.xml file at regular intervals. It uses the public/private keypair of the built-in root user as the source of the key exchange. This means that the public key must be copied to the remote host and added to the authorized_keys file (for the defined user). The remote file is backed up as `config-YYYY-DD-MM-HH-MM.xml`, for example: `config-2018-01-02-15-40.xml`. It's possible to change the remote location of where the config file is backed up to. The cron job can be modified under System/Settings/Cron and the schedule adjusted to suit the backup frequency requirements. -=david=- closes #457

fabianfrz suggested changes Jan 2, 2018

View reviewed changes

fichtner self-assigned this Jan 2, 2018

fichtner assigned dharrigan and unassigned fichtner Jan 3, 2018

fichtner approved these changes Jan 3, 2018

View reviewed changes

fabianfrz approved these changes Jan 3, 2018

View reviewed changes

fabianfrz added the feature Adding new functionality label Jan 3, 2018

dharrigan merged commit e59ee70 into opnsense:master Jan 3, 2018

dharrigan deleted the scp-backup branch January 3, 2018 19:25


		$command = "scp -P $port -i $identifyFile $configFile $username@$hostname:$remoteDirectoryFullPath";

		syslog(LOG_WARNING, "scp_backup command: $command");

A new plugin to allow SCP config backups to a remote host. #458

A new plugin to allow SCP config backups to a remote host. #458

Conversation

dharrigan commented Jan 2, 2018

fabianfrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fichtner commented Jan 2, 2018

dharrigan commented Jan 2, 2018

dharrigan commented Jan 2, 2018 via email

fabianfrz commented Jan 2, 2018 • edited Loading

dharrigan commented Jan 2, 2018 via email

dharrigan commented Jan 2, 2018

fichtner left a comment

Choose a reason for hiding this comment

dharrigan commented Jan 3, 2018 • edited Loading

fabianfrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharrigan commented Jan 3, 2018

dharrigan commented Jan 3, 2018 via email

fabianfrz commented Jan 2, 2018 •

edited

Loading

dharrigan commented Jan 3, 2018 •

edited

Loading