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

nginx WAF #696

Merged
merged 65 commits into from
Jul 13, 2018
Merged

nginx WAF #696

merged 65 commits into from
Jul 13, 2018

Conversation

fabianfrz
Copy link
Member

@fabianfrz fabianfrz commented May 31, 2018

ready for master

@fabianfrz fabianfrz added the feature Adding new functionality label May 31, 2018
@fabianfrz fabianfrz self-assigned this May 31, 2018
@fabianfrz fabianfrz changed the title WIP: Nginx WIP: nginx May 31, 2018
@fabianfrz fabianfrz changed the title WIP: nginx WIP: nginx WAF May 31, 2018
@fichtner fichtner self-requested a review May 31, 2018 18:07
@fichtner
Copy link
Member

@fabianfrz thanks, I'll review when I have a bit more time

@fabianfrz
Copy link
Member Author

@fichtner this is a work in progress - please don't review now.

@fichtner
Copy link
Member

@fabianfrz sure, just let me know when

@fabianfrz
Copy link
Member Author

@fichtner can we make nginx the default webserver, so It could also be in core some day?

@AdSchellevis
Copy link
Member

@fichtner @fabianfrz I'm not sure we should replace lighttpd with nginx or apache, having a dedicated and small webserver for configuration purposes can be practical and the gain is likely not very high. At the moment I rather stick with lighttpd. (adding nginx as waf is nice by the way)

@fichtner
Copy link
Member

fichtner commented May 31, 2018 via email

@AdSchellevis
Copy link
Member

sure, we can always allow to disable lighttpd, as long as that remains the standard I'm obviously fine with it.

@fabianfrz
Copy link
Member Author

fabianfrz commented May 31, 2018

at the moment we do have nginx, HAProxy, relayd and lighttpd using the same ports in OPNsense…

@mimugmail
Copy link
Member

What is the current state of the code base? I installed the plugin, but it's a bit confusing:

First, I'd remove email stuff, do you really want to support it from the beginning?
Are all relations finished? For me upstream and upstream server is confusing. Now I tried to build some upstream servers, group them in upstream and then link to http servers, is that correct? Is there something missing?

@fabianfrz
Copy link
Member Author

@mimugmail it is an early stage. I made it available to allow others to help or test (for example feedback).

In case of nginx, an upstream is a group of servers to load balance. A HTTP Server may contain multiple locations. A location may contain an upstream reference.
The upstream may contain multiple servers, but it is not bound to anything.

The WAF is mixed between HTTP blocks and locations. Headers are injected depending on what they are intended to do.

There is currently a lot of stuff missing. The mail stuff does not do anything at the moment - I will comment it out before release. Also the WAF Rules cannot be added currently - there is only a stub in the model. Also some properties may be still missing.

@mimugmail
Copy link
Member

I think the user should change so something low privileged when used as a reverse proxy sind it doesn't need PHP and only redirects traffic. In the current setup it sets the user to root. Perhaps it makes sense for a replacement for lighttpd but for a WAF not needed.

@fabianfrz
Copy link
Member Author

@mimugmail yes, that's possible. We can also run nginx as root and assign it a php instance running as root which makes it effectively root - so not a real difference. I am open for ideas.

@fichtner nbs-system/naxsi#423 seems to fix an issue we have here. Can you push it with the next release as an updated package?

@fabianfrz fabianfrz changed the title nginx WAF (developer preview) nginx WAF Jun 22, 2018
@sempervictus
Copy link

So the privilege issue can be addressed with configd later, properly anyway. Far as running the webui, can we keep that as light/nginx on a high port and use the TCP rewrite capability to force webui traffic thru an unprivileged WAF enabled proxy socket first? If we can do inline defense for web apps, it would be great to eat our own dogfood adding defensive options for the web ui, IMHO. Which vendor system had a WAF before their web ui? Seems marketable :-)

@fabianfrz
Copy link
Member Author

@sempervictus At the moment I cannot activate the WAF for the webgui - I have to wait for the next release because I need a more up to date version of naxsi. I also don't need to rewrite something as I can pass the connection through directly - but this does not really have an advantage since we would not really change the attack surface. If an attacker gets access to the front proxy, he can read the TLS certificates for MITM and could hook to process to read its memory for encryption keys and plain text traffic which at some time will contain an access token or password which will give root via SSH. It is also more dangerous to put an lighttpd behind because you have the risk of compromising nginx OR lighttpd and nginx is definitely wider used therefor better audited since huge enterprises depend on it.

@fabianfrz
Copy link
Member Author

applied patches for opnsense/core#2494

@fabianfrz
Copy link
Member Author

fabianfrz commented Jul 8, 2018

@fichtner the plugin should get its review. If you want to disable web interface handling for now, you can comment out two lines (user and the include) since root is only required for the web interface

@@ -0,0 +1,5 @@
<menu>
<Services>
<Nginx cssClass="fa fa-shield fa-fw" url="/ui/nginx/" />
Copy link
Member

Choose a reason for hiding this comment

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

trailing slash for nginx/ is no longer needed

message:starting nginx

[stop]
command:/usr/local/etc/rc.d/nginx stop; exit 0
Copy link
Member

Choose a reason for hiding this comment

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

no ";exit 0"

@fichtner
Copy link
Member

@fabianfrz ok so far, just small amendments for inclusion. and, yes, please disable web interface handling for now.

@fabianfrz
Copy link
Member Author

@fichtner I have added your feedback and will merge; in addition I added the directives for brotli which exist since today (brotli on; and brotli_static on;)

@fabianfrz fabianfrz merged commit da2ec7e into master Jul 13, 2018
@fabianfrz fabianfrz deleted the nginx branch July 13, 2018 15:28
@fichtner
Copy link
Member

@fabianfrz ok, for merges in the future please wait for "approval". github makes this easy, we should use it...

@fabianfrz
Copy link
Member Author

@fichtner I misinterpreted your comment as approval ;)

@fichtner
Copy link
Member

@fabianfrz it was, but now I can't push the green button anymore. my favourite part ;)

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.

6 participants