fix coraza configuration to use the action variable

[JanHolger]

This is a fix for #1085

The snippet used in the fix was provided by the issue's op (@tomklapka)

It uses txn.coraza.action instead of the txn.coraza.fail and allow for a more fine grained control about how requests should be rejected and fixes the issue that with the current coraza version all requests are rejected (txn.coraza.fail is most likely not set anymore).

I've based this fix of 0.14 because I wasn't able to get master running in our testing environment. Please let me know if I need to change anything here.

[jcmoraisjr]

Thanks for the contribution! A few questions:

• Why did you add both the http-request and http-response? The former should be used if you want the proxy to block requests to you server, or modify how the request is changed before reach your server, and the later is used to change the response your server made before being delivered to the client/user. In the case of denies, redirects and drops, we're always talking about blocking when using in requests, so without the request reaching the underlying server, which apparently makes the http-response useless.
• What sort of problems did you have with master? Maybe you found another incompatibility what worth to be improved in code, or better documented as a known incompatibility. Despite of that, that part of the code didn't change (that much) from v0.14 so I'd say it's safe to be sent to master. We'll probably cherry-pick it to v0.14 as well, just need to better understand the impact.

[h2o-commits]

Thanks for the contribution! A few questions:

* Why did you add both the http-request and http-response? The former should be used if you want the proxy to block requests to you server, or modify how the request is changed before reach your server, and the later is used to change the response your server made before being delivered to the client/user. In the case of denies, redirects and drops, we're always talking about blocking when using in requests, so without the request reaching the underlying server, which apparently makes  the http-response useless.

Not tested, but I thing you get the point. http-response is useless then.

* What sort of problems did you have with master? Maybe you found another incompatibility what worth to be improved in code, or better documented as a known incompatibility. Despite of that, that part of the code didn't change (that much) from v0.14 so I'd say it's safe to be sent to master. We'll probably cherry-pick it to v0.14 as well, just need to better understand the impact.

The problem with master has nothing to do with this fix. We were not able to test this fix on master (v0.15) as haproxy wasn't able to be bind to network socket there.

[JanHolger]

• Why did you add both the http-request and http-response? The former should be used if you want the proxy to block requests to you server, or modify how the request is changed before reach your server, and the later is used to change the response your server made before being delivered to the client/user. In the case of denies, redirects and drops, we're always talking about blocking when using in requests, so without the request reaching the underlying server, which apparently makes the http-response useless.

Like I mentioned, we used the snippet from the issue's op and just verified that it doesn't do anything harmful and is actually working but we didn't really check what he did in the snippet. If I check the official coraza docs (https://coraza.io/connectors/coraza-spoa/#haproxy-configuration) they also do it for both request and response and according to the flow diagram on their website (https://owasp.org/www-project-coraza-web-application-firewall/) coraza will not only check the request but also the response.

• What sort of problems did you have with master? Maybe you found another incompatibility what worth to be improved in code, or better documented as a known incompatibility. Despite of that, that part of the code didn't change (that much) from v0.14 so I'd say it's safe to be sent to master. We'll probably cherry-pick it to v0.14 as well, just need to better understand the impact.

The first issue was with our configuration and the newer haproxy version used in master (the http-use-htx option was removed). Since this option is the default value in newer versions, I was able to fix that by simply removing the option from our config.

The second issue (that I wasn't able to resolve) is a permission denied error while trying to bind 80/443, even after manually adding the NET_BIND_SERVICE capability. Do we need to change anything besides replacing the image version to make the new version work?

I've switched the base to master

[jcmoraisjr]

coraza will not only check the request but also the response.

Ah this really makes sense, thanks for pointing it out.

http-use-htx option was removed

Great thanks, we in fact need to remove this deprecated option.

permission denied error while trying to bind 80/443, even after manually adding the NET_BIND_SERVICE capability

This is somewhat expected since we started to run haproxy ingress as non root on v0.15. How did you add the capability? Are you running the embedded haproxy or configuring an external one via a sidecar?

[JanHolger]

permission denied error while trying to bind 80/443, even after manually adding the NET_BIND_SERVICE capability

This is somewhat expected since we started to run haproxy ingress as non root on v0.15. How did you add the capability? Are you running the embedded haproxy or configuring an external one via a sidecar?

We are running with the internal haproxy instance. I guess you are already providing a fix for this in #1096?

[jcmoraisjr]

I guess you are already providing a fix for this in #1096?

This is more like a "best effort" than a fix, because capabilities rely on extended attributes, and it doesn't work properly on all file systems and controller runtime environments. I'll provide a proper doc and link in the v0.15 release notes, with all the options and their issues.

[jcmoraisjr]

Lgtm, merging now.