www/nginx: new features #828

fabianfrz · 2018-09-09T09:13:13Z

found stuff is sent to a special log and can be parsed periodically via a Cron job to update the alias automatically.

Also marked some settings as advanced to decrease the size of some dialogs.
In addition (request per mail), I added web socket support.
Nice feature I've found in the nginx docs: HTTP/2 preloading.
Allow caching

fabianfrz · 2018-09-09T09:15:08Z

@fichtner please check the use of the alias class especially since I don not know it yet and it lacks some documentation.

fabianfrz · 2018-09-15T08:08:27Z

@fichtner review?

AdSchellevis · 2018-09-15T08:23:14Z

www/nginx/src/opnsense/scripts/nginx/ngx_autoblock.php

+$blacklist_element = null;
+foreach ($model->aliases->alias->__items as $alias) {
+    if ((string)$alias->name == $autoblock_alias_name) {
+        if ((string)$alias->type != 'network') {


@fabianfrz you better manage the contents of the list from within your plugin, using the internal type
https://github.com/opnsense/core/blob/master/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.xml#L33

Now you have two processes managing the same table, which is likely resulting in issues somewhere along the line. Only question is if the entries should survive a reboot, in which case you need to store some data in your own model (or parse logs on clean startup)

@AdSchellevis can you explain the external field? It seems to have no validations and the current one works. I want the user to be able to delete an entry if it detected an incorrect one.

The script runs once per minute and appends new findings to the alias and updates the alias by itself when done. The log is purged automatically but the IP addresses keep being stored in the alias.

@fabianfrz external means that it’s managed elsewhere (in this case in your plugin), the alias system won’t touch the contents and trusts the provider to manage it accordingly. With the current construction you created two systems, which try to update the table. The pfct in your code and the regular alias system updating the contents to its configuration at given intervals, which causes a race and can lead to removal of entries just inserted by your tool.

For better separation of concerns it’s highly advisable to keep the blacklist in your plugin or ask the alias system to sync it’s contents, my advise is to keep the blacklist in the plugin since it causes less overhead. People can always drop entries manually using the table tool, same as for ssh an web ui lockout construction.

so external will do nothing except adding a table entry to pf.conf and make it available to the UI?

Yes, that’s it

fabianfrz · 2018-09-15T20:14:41Z

@AdSchellevis I changed the alias functionality and it is now completely handled by the plugin. I've also created an view and unlock page, so the user can unlock an IP address or IPv6 network again if it got banned "by accident".

fabianfrz · 2018-09-17T17:49:14Z

@fichtner did you forget this for 18.7.3?

fichtner · 2018-09-17T17:50:03Z

I did not forget. There was no time.

fabianfrz · 2018-09-17T17:50:58Z

Ok, then I may add new features until next release to this branch.

fichtner · 2018-09-17T17:53:03Z

Sure, I'll be reviewing later this week.

fabianfrz · 2018-09-17T19:46:10Z

now we have a simple cache for upstreams ;)

fichtner

generally: ok!

fichtner · 2018-09-25T18:24:40Z

www/nginx/src/etc/inc/plugins.inc.d/nginx.inc

+            'description' => gettext('nginx Load Balancer, WAF, Reverse Proxy and Web Server'),
+            'configd' => array(
+                'restart' => array('nginx restart'),
+                'start' => array('nginx start')


The original implementation was intended to run the web interface too which obviously should not be stopped.

fichtner · 2018-09-25T18:24:50Z

www/nginx/src/etc/inc/plugins.inc.d/nginx.inc

+
+    return array(
+        array(
+            'description' => gettext('nginx Load Balancer, WAF, Reverse Proxy and Web Server'),


too long ;)

what would you like to have as an alternative?

fichtner · 2018-09-25T18:25:14Z

www/nginx/src/etc/inc/plugins.inc.d/nginx.inc

+{
+    return array(
+        array(
+            'autocron' => array('/usr/local/opnsense/scripts/nginx/ngx_autoblock.php', '*')


can't we have a conditional here?

conditional for what? nginx runs always

fichtner · 2018-09-25T18:26:27Z

www/nginx/src/opnsense/mvc/app/controllers/OPNsense/Nginx/Api/BansController.php

+{
+    static protected $internalModelClass = '\OPNsense\Nginx\Nginx';
+    static protected $internalModelName = 'nginx';
+    public function searchbanAction()


line spacing is whacky. I've held back a larger style update for 1.0 already to not mess with this PR. You could run and fix it yourself (requires plugin os-debug):

# cd /usr/plugins/www/nging && make style-fix

fichtner · 2018-09-25T18:27:34Z

www/nginx/src/opnsense/mvc/app/controllers/OPNsense/Nginx/forms/httpserver.xml

@@ -48,6 +48,7 @@
    <id>httpserver.verify_client</id>
    <label>Verify Client Certificate</label>
    <type>dropdown</type>
+    <advanced>true</advanced>


just a note: nice! advanced is good, happy users due to simpler UI

fichtner · 2018-09-25T18:28:36Z

www/nginx/src/opnsense/mvc/app/models/OPNsense/Nginx/Menu/Menu.xml

@@ -3,6 +3,7 @@
    <Nginx cssClass="fa fa-shield fa-fw">
      <Configuration url="/ui/nginx" />
      <Logs url="/ui/nginx/index/logs" />
+      <Banned url="/ui/nginx/index/ban" />


"Banned" feels misplaced. Logs should be visible as "Log File" and probably go last...

ban is really a ban - it is a list of banned IPs in a special alias which can be referenced by PF rules. This page does not handle log files.

fichtner · 2018-09-25T18:29:08Z

www/nginx/src/opnsense/mvc/app/views/OPNsense/Nginx/logs.volt

-<script src="/ui/js/nginx/lib/lodash.min.js"></script>
-<script src="/ui/js/nginx/lib/backbone-min.js"></script>
-<script src="/ui/js/nginx/dist/bundle.js"></script>
+<script src="{{ cache_safe('/ui/js/nginx/lib/lodash.min.js') }}"></script>


fichtner · 2018-09-25T18:29:30Z

www/nginx/src/opnsense/scripts/nginx/ngx_autoblock.php

@@ -0,0 +1,145 @@
+#!/usr/local/bin/php


does this have to be prefixed "ngx_" ?

no, it does not. Is it a problem? It is just the file name of my cron job which must run as often as possible which means every minute.

Wouldn't it be better to rename the fourth one?

the first one is for header handling, the second and the third one nginx related and the fifth one is the standard setup routine which is called setup in any plugin.

it depends on whether you want csp_ ngx_ and so on to have a special meaning for different call context? I was only suggesting name simplification because "nginx" is implied by the parent directory, but I don't know enough about inner workings of nginx to know if "ngx" is something inside the process or something that you want explicitly separated for a specific reason. :)

csp_report: API endpoint for CSP violation reports
ngx_auth: API endpoint for authentication request (checkbox advanced authentication)
ngx_autoblock: cron job which looks for bad IPs and sends them to PF
read_log: configd call to read the logs for the frontend (logs view)
setup: create files, chmod, export certs

the first one is generic, the 2-3 depend on how nginx or the plugin is working, 4 is generic (would probably also work for other servers), 5 is plugin code

ok, thanks. to the original point read_log is perfectly clear so no need to change that

fichtner · 2018-09-25T18:29:53Z

www/nginx/src/opnsense/scripts/nginx/ngx_autoblock.php

+<?php
+
+/*
+ * Copyright (C) 2018 Fabian Franz


thank you for using proper copyright layout here

most of the time I am just copying the copyright block from my older plugins except some files which I am boilerplate using my rake task.

I'm trying to make all of them one PHP style when I can:

/* * Copyright... * * [...] */

fichtner · 2018-09-25T20:46:45Z

@fabianfrz I ask you to reconsider your recent comments and responses which have been growing rougher and unfriendlier. I don't like cleaning up bugs for work that you consider "done" or reworking style that does not meet project standards because you don't think it's interesting enough. If that trend continues have no reason to continue reviewing your work.

fabianfrz added 4 commits September 6, 2018 21:06

www/nginx: add initial permanent ban feature

dadcee1

www/nginx: write data to model and add blocks to fw

758bf1e

www/nginx: add permanent ban script; change some settings to be advanced

4089cbe

www/nginx: change cron fix

073f691

fabianfrz added the feature Adding new functionality label Sep 9, 2018

fabianfrz self-assigned this Sep 9, 2018

fabianfrz requested a review from fichtner September 9, 2018 09:15

fabianfrz added 2 commits September 10, 2018 19:49

www/nginx: a network field is needed

60e26be

www/nginx: allow ws/wss

12f073c

fabianfrz changed the title ~~nginx permanent ban feature~~ nginx permanent ban feature, allow to proxy WS Sep 11, 2018

fabianfrz added 3 commits September 11, 2018 19:16

www/nginx: hostname bugfix

6a6b314

www/nginx: remove hack to get uuid

8796d51

www/nginx: update blacklist

3937576

fabianfrz changed the title ~~nginx permanent ban feature, allow to proxy WS~~ nginx permanent ban feature, allow to proxy WS, extend bot blacklist Sep 12, 2018

fabianfrz added 2 commits September 12, 2018 22:31

www/nginx: hook cron automatically

febddc5

www/nginx: add support for HTTP/2 preloading

8d3d884

fabianfrz changed the title ~~nginx permanent ban feature, allow to proxy WS, extend bot blacklist~~ nginx permanent ban feature, allow to proxy WS, extend bot blacklist, HTTP/2 preload Sep 15, 2018

AdSchellevis reviewed Sep 15, 2018

View reviewed changes

www/nginx imrove permanent ban feature

43b709b

fabianfrz added 2 commits September 16, 2018 14:43

www/nginx: cleanup and add cache helper

7bf541a

www/nginx: webgui - enable HTTP/2 server push

dbe8ab2

www/nginx: add cache

f08cf9a

www/nginx: more options for caching

ebdb1d9

fabianfrz changed the title ~~nginx permanent ban feature, allow to proxy WS, extend bot blacklist, HTTP/2 preload~~ www/nginx: new features Sep 18, 2018

www/nginx: export fullchain

f4148b7

fabianfrz mentioned this pull request Sep 25, 2018

www/nginx: Export full chain as pem (if present) #863

Closed

fichtner approved these changes Sep 25, 2018

View reviewed changes

fabianfrz added 2 commits September 26, 2018 17:45

www/nginx: run bmake style-fix

bdf774b

www/nginx: shorten name

3418b02

fabianfrz merged commit 8e687ee into master Sep 26, 2018

fabianfrz deleted the nginx-permaban branch September 26, 2018 16:06

www/nginx: new features #828

www/nginx: new features #828

Conversation

fabianfrz commented Sep 9, 2018 • edited Loading

fabianfrz commented Sep 9, 2018

fabianfrz commented Sep 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfrz Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfrz commented Sep 15, 2018

fabianfrz commented Sep 17, 2018

fichtner commented Sep 17, 2018

fabianfrz commented Sep 17, 2018

fichtner commented Sep 17, 2018

fabianfrz commented Sep 17, 2018

fichtner 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

fabianfrz Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fichtner commented Sep 25, 2018

fabianfrz commented Sep 9, 2018 •

edited

Loading

fabianfrz Sep 15, 2018 •

edited

Loading

fabianfrz Sep 25, 2018 •

edited

Loading