Project

General

Profile

Bug #8438

haproxy: can't use ACL for cert with http-response actions

Added by Petr H almost 2 years ago. Updated almost 2 years ago.

Status:
New
Priority:
High
Assignee:
-
Category:
haproxy
Target version:
-
Start date:
04/05/2018
Due date:
% Done:

0%

Estimated time:
Affected Version:
2.4.3
Affected Architecture:
amd64

Description

pfSense 2.4.3, pfSense-pkg-haproxy 0.54_2, haproxy 1.7.10

1. Primary frontend used by other shared ones
2. SSL-enabled
3. Option "Add ACL for certificate Subject Alternative Names." enabled
4. Action "http-response header replace" used

Upon saving such configuration I'm getting:
"[WARNING] 094/235036 (95194) : parsing [/var/etc/haproxy/haproxy.cfg:46] : acl 'aclcrt_main-SSL' will never match because it only involves keywords that are incompatible with 'frontend http-response header rule'"

And the configuration doesn't work as expected.

- If I deselect the "Add ACL for certificate Subject Alternative Names." option the issue doesn't occur.
- Or If I move the rule to any of the shared ones where the ACL option doesn't have effect the issue doesn't occur.
- Or (just for test purposes) if I change the action from "http-response header replace" to "http-request header replace" the issue doesn't occur (however the configuration doesn't work properly of course).

I think that ACLs generated via the "Add ACL for certificate Subject Alternative Names." (or "Add ACL for certificate CommonName.") option shouldn't be applied to actions affecting HTTP (or TCP) responses.

haproxy.cfg (2.56 KB) haproxy.cfg sample haproxy.cfg, warning at lines 48 and 49 Petr H, 04/06/2018 02:29 PM

History

#1 Updated by Pi Ba almost 2 years ago

Can you show/attach the complete haproxy.conf itself? I'm not yet seeing when this would occur.. And or perhaps a screenshot of the configured actions? (obfuscate names/ips where needed, but make sure it stays structurally the same)

These aclcrt acl's arent automatically applied to anything but the use_backend action iirc..

#2 Updated by Petr H almost 2 years ago

Attached sample haproxy.cfg that demonstrates the problem. With this file the warnings occur at lines 48 and 49:

[WARNING] 095/211743 (34585) : parsing [/var/etc/haproxy/haproxy.cfg:48] : acl 'aclcrt_main-SSL' will never match because it only involves keywords that are incompatible with 'frontend http-response header rule'
[WARNING] 095/211743 (34585) : parsing [/var/etc/haproxy/haproxy.cfg:49] : acl 'aclcrt_main-SSL' will never match because it only involves keywords that are incompatible with 'frontend http-response header rule'

Those certificate ACL conditions are applied to almost all actions. Only those defined within the shared frontend (use_backend in this particular case) seem to be excluded.

And I just tried creating a simple (and standalone) test frontend "test2" with just the following specified:
- Listen Port: 444
- SSL Offloading: enabled
- Action1: http-response header add: X-Test TEST2 (no conditions)
- Action2: Use Backend: TEST1_80 (same as in the attached haproxy.cfg) (no conditions)
- Use "forwardfor" option: enabled
- Use "httpclose" option: http-server-close
- Certificate: custom certificate selected (it's a wildcard one)
- Add ACL for certificate Subject Alternative Names: enabled

And the warning occurs for the following line where the certificate ACL condition is added again:
http-response add-header X-Test TEST2 if aclcrt_test2

For reference, the whole haproxy.cfg entry for this frontend is as follows:

frontend test2
        bind                    1.2.3.4:444 name 1.2.3.4:444 ssl  crt /var/etc/haproxy/test2.pem
        mode                    http
        log                     global
        option                  http-server-close
        option                  forwardfor
        acl https ssl_fc
        http-request set-header         X-Forwarded-Proto http if !https
        http-request set-header         X-Forwarded-Proto https if https
        timeout client          30000
        acl                     aclcrt_test2    hdr_reg(host) -i ^([^\.]*)\.domain\.net(:([0-9]){1,5})?$
        acl                     aclcrt_test2    hdr_reg(host) -i ^domain\.net(:([0-9]){1,5})?$
        http-response add-header X-Test TEST2  if   aclcrt_test2
        use_backend TEST1_80_http_ipvANY  if   aclcrt_test2

#3 Updated by Pi Ba almost 2 years ago

Ok thanks can reproduce it now. Ill check why that happens.

#4 Updated by Pi Ba almost 2 years ago

Actually that the condition is added to all actions in the frontend probably is the 'right thing' to do.. (my previous reply about only doing so on use_backend was wrong) however your correct that it wont work for hdr(Host) like fetches directly..

I'm going to make a patch by using set-var, i think that should cover it nicely:

acl                     aclcrt_test2    var(txn.txnhost) -m reg -i ^([^\.]*)\.domain\.net(:([0-9]){1,5})?$
acl                     aclcrt_test2    var(txn.txnhost) -m reg -i ^domain\.net(:([0-9]){1,5})?$
http-request set-var(txn.txnhost) hdr(host)
http-response add-header X-Test TEST2  if   aclcrt_test2
use_backend TEST1_80_http_ipvANY  if   aclcrt_test2

Unless you can tell that would be completely wrong for some reason ;) Will probably send a pull-request against haproxy-devel tomorrow..

#5 Updated by Petr H almost 2 years ago

http-response set-var(txn.txnhost) hdr(host)

That seems to set that variable only during the response processing. If there would be any http-request action added, you will add a similar set-var entry in the scope of http-request?
EDIT: Actually use_backend with the aclcrt would need the variable to be set in during the request processing as well, or not? In that case perhaps just

http-request set-var(txn.txnhost) hdr(host)
may do it. txn prefix should set the variable for the scope of the whole request/response cycle AFAIK.

And by the way this regex (generated for the wildcard certificate):

^([^\.]*)\.domain\.net(:([0-9]){1,5})?$
may be slightly optimized for better performance by using *? instead of *:
^([^\.]*?)\.domain\.net(:([0-9]){1,5})?$

#6 Updated by Pi Ba almost 2 years ago

Petr H wrote:

http-response set-var(txn.txnhost) hdr(host)

That seems to set that variable only during the response processing.

Oh 'typo' that should have only been on http-request, as only the client sets the Host header.. And yes the 'txn.' should remember the header for use during response processing..

As for regex performance.. I'm not exactly sure how to 'measure' that.. However regex101.com tells me

^([^\.]*)\.domain\.net(:([0-9]){1,5})?$ needs 19 steps

While
^([^\.]*?)\.domain\.net(:([0-9]){1,5})?$ needs 27 steps

My guess is the first one is faster.?.

#7 Updated by Petr H almost 2 years ago

Re regex - yes you're right.
I was living with the false assumption (based on some tests that I remember from the past) that the lazy quantifier should usually help in that particular place, however that isn't the case because your current approach with matching as much as possible before the dot character ([^\.]*)\. is specific enough to match the required string in almost 1 step and thus perform the best here.
It would help in the case of a simple (.*)\. where the lazy form {.*?}\. should help if the string to match wouldn't be too long.
It makes sense when one thinks about it - at least I learnt something today.

#8 Updated by Pi Ba almost 2 years ago

If you can perhaps test/validate my changes again haproxy-devel version that would be great.
Either the full thing (remove the 7th patch before applying through patches package and set pathstrip to 4)
https://patch-diff.githubusercontent.com/raw/pfsense/FreeBSD-ports/pull/510.patch

#9 Updated by Petr H almost 2 years ago

It seems to be fine, good.

While I'm at it, few more glitches I found:

1. Backend: Timeout / retry settings
Unable to effectively set Retries to 0 - when I set it to 0 and then check the haproxy.cfg it's actually 3, I believe it should be possible to set this to 0 to disable retrying

2. Dynamically updated web page content doesn't work properly at several places. I have to force the page to reload by either attempting to save changes while intentionally missing out some required field, or save and reopen the page in order to have the page displayed properly (all relevant options shown etc). Some examples:

- Frontend
Shared Frontend: This can be used to host a second or more website on the same IP:Port combination. - checking this option doesn't allow me to specify the Primary Frontend and some other settings aren't adjusted properly as well, for example the whole Advanced Settings don't disappear and in the SSL Offloading the Use Offloading: Specify additional certificates for this shared-frontend. option is missing, selecting/deselecting this option also doesn't refresh option doesn't appear for the very first time

Advanced settings: Use "httpclose" option - the description isn't dynamically updated when this option is modified

SSL Offloading: Use Offloading: Specify additional certificates for this shared-frontend. - checking this doesn't show all the additional certificate settings don't appear dynamically

- Backend
Health checking: Health check method - if the check method is modified the whole Health checking section including description and options isn't updated to match the newly selected method

Agent checks: Agent checks: Use agent checks - checking this doesn't show the agent checks settings

Cookie persistence: Cookie Enabled: Enables cookie based persistence. (only used on "http" frontends) - checking this doesn't dynamically show the cookie persistence settings

Cookie persistence: Cookie Mode - the description isn't dynamically updated when this option is modified

Stick-table persistence: Stick tables - if the method is modified the whole Stick-table persistence section including description and options isn't updated to match the newly selected method

Advanced settings: Transparent ClientIP: Use Client-IP to connect to backend servers. - the interface selection doesn't dynamically show up when this option is selected

There could be other places with similar problem, I don't think i tested all of them and all their possible combinations.

3. "Dummy" Frontend
Starting with empty configuration (basically no frontends and no backends is enough).
Create a simple backend with just some minimal configuration.
Go to the list of Frontends - there will be some "dummy" frontend shown now. What's that?

4. "Dummy" Backend (similar to the "dummy" frontend case)
Starting with empty configuration (basically no frontends and no backends is enough).
Create a simple frontend with just some minimal configuration
Go to the list of Backends - there will be some "dummy" backend shown now. What's that?

Note: I may open separate bug reports for these if required.

#10 Updated by Pi Ba almost 2 years ago

Thanks for testing and reporting about these issues.
1. found&fixed

2. these items seem to work properly for me on Windows10 with all browsers i tried Chrome/Firefox/Edge/IE
Ive verified the 'shared frontend', the 'transparent clientip' feature in both cases the proper sections get shown/hidden immediately after checking the box. Also when changing the HealthCheckMethod the description and options below it show properly
What browser / OS are you using? Make sure to clear cache just in case, also don't have any script blockers active or something? Can you try another browser?

3. found&fixed
4. found&fixed

#11 Updated by Petr H almost 2 years ago

Re 2: I usually use Firefox @ Windows 10 and yes with some blockers such as NoScript, uBlock and few user scripts in Tampermonkey. I tried disabling them all before but that didn't help. However after reading your reply I tried some more experiments including creating a new Firefox profile and testing it in there and finally was able to identify where the issue comes from.

It's somehow caused by the extension Download Manager (S3) - https://addons.mozilla.org/en-US/firefox/addon/s3download-statusbar/ which partially breaks those dynamic page updates. When that happens I can see the following in the developer console (that's an example for ticking the SSL Offloading checkbox in the Frontend configuration):

SecurityError: The operation is insecure.    haproxy_listeners_edit.php:261
setCSSdisplay                    https://localhost/haproxy/haproxy_listeners_edit.php:261:8
updatevisibility                https://localhost/haproxy/haproxy_listeners_edit.php:297:3
table_extaddr_listitem_change            https://localhost/haproxy/haproxy_listeners_edit.php:2189:6
html_listitem_change                https://localhost/haproxy/haproxy_listeners_edit.php:2207:4
onclick                        https://localhost/haproxy/haproxy_listeners_edit.php:1:1
Don't know what exactly that extension does there and how to effectively report it to its developer though.

#12 Updated by Pi Ba almost 2 years ago

2. found&fixed

The plugin 'injects' extra stylesheets, and the setCSSdisplay function searches for a particular stylesheet rule, FF doesnt allow reading 3rd party stylesheet rules and throws a SecurityException on that.. I'm not sure if this is a bug or feature, but anyhow its now 'catched'..

#13 Updated by Pi Ba almost 2 years ago

Okay 0.56 haproxy-devel package is available now through normal pfSense packages. If you can check 'everything' now works as expected that would be great :)

#14 Updated by Petr H almost 2 years ago

Updated and tested all of the above - looks alright.

Only right after the update I encountered one issue:
- I was updating from that patched version (could it be related to this issue?), update went fine.
- After the update I could see 1 dummy backend on the Backends page, so I deleted it.
- Then attempting to load the Settings page resulted in the following error:

Apr  9 22:11:18 pfSense php-fpm[310]: /haproxy/haproxy_global.php: PHP ERROR: Type: 1, File: /usr/local/www/haproxy/haproxy_global.php, Line: 79, Message: Call to undefined function haproxy_config_init()
- In the end I uninstalled & reinstalled the haproxy-devel package and the issue didn't occur again.

#15 Updated by Pi Ba almost 2 years ago

The haproxy_config_init() is a new function added in the second last commit. Not sure why that wouldn't exist after updating when the edit pages themselves do try to use it and have apparently been updated.. What functions are in the php is not dependent on a possibly empty item in the config.. I dont think ive seen a pkg update apply partially before which seems to be the case. The function is meant to be part of the solution avoiding the dummy/empty frontend/backend..
For now lets call it a anomaly..

#16 Updated by Petr H almost 2 years ago

I've got one minor feature proposal:

Notes/Description/Comments for each ACL or action entry

Normally if I'd use HAProxy somewhere directly and thus also directly edit haproxy.conf, I'd use some comments here and there in order to describe why there are certain entries so that someone else can figure them out easily.
With the pfSense HAProxy frontend it isn't necessary to have such comments in haproxy.conf, but having something like that in the web interface would be essential.
There's a "Description" field in each Frontend, however I'd appreciate having sometihng similar for each ACL and action entry as well. For example when I create set of few ACLs and actions which are supposed to work together, I'd like to add a note to each of them describing that they form a one "unit" so whoever looks at them can easily understand their purpose and also if they're going to be deleted later, it would be easy to identify all of them easily for deletion.

And you may add such field to some more items such as Backend configuration, or to each "External address" entry in the Frontend configuration.

#17 Updated by Petr H almost 2 years ago

Also:

1. Add "path_dir" to the default list of ACL expressions
2. Your current ACLs are case insensitive (-i) by default. Add a checkbox to disable this.

With that I'll need to use custom ACLs less which will result in more straightforward configuration.

#18 Updated by Petr H almost 2 years ago

One more minor issue:

In the Shared Frontend configuration, the Default Backend option shouldn't be configurable.
It should be configurable only in the main frontend. Or am I missing something?

#19 Updated by Petr H almost 2 years ago

Another one on the web interface consistency:

There are several different ways how to reorder certain entries depending on what kind entries these are, for example:

Frontends list - check entry & use anchor icon
Frontend / External address, ACLs, Actions - arrow icons to move up/down, or check entry & use anchor icon
Frontend / Error files - check entry & use anchor icon
Backends list - drag & drop
Backend / Server list - arrow icons to move up/down, or check entry & use anchor icon

It would be nice to have it consistent at all places.

#20 Updated by Pi Ba almost 2 years ago

I havn't forgotten, but as you might have seen (on haproxy mailinglist) i've been busy with some bugs bugging me in the binary (lua) side of haproxy..

0 - adding 'description' items to all the 'table items' could be nice.. need to think of how to do that..
Should it be part of the list items themselves, or become a separate item like the 'separator' in firewall/rules.?.
1 - path_dir << done
2 - acl case-sensitive option << done
3 - Default backend in secondary frontend (in the gui) can make sense when combined with automatic sni acl's from certificates. In the config itself that would get written as use_backend X if cert-sni-value-matches.. removing it now is um tricky to not break existing configs using this feature..
4 - Inconsistent list editing options, i did correct a few of them, but frontend list is special. It wont be exactly the same as the others.. << done mostly..

Ill try and commit the things that are done soon..

#21 Updated by Petr H almost 2 years ago

0 - Should it be part of the list items themselves, or become a separate item like the 'separator' in firewall/rules.?.

Not really sure. Perhaps the horizontal separation would look better because there's already a lot to display in each row..?

One more thing :-)
I wanted to setup a sort of dummy backend that would always return HTTP 403, e.g.

backend deny-all
    http-request deny
(which is basically everything that should be needed in that backend)

This backend, when configured as default for some frontend, would ensure that HTTP 403 is returned to all non-matching requests (instead of HTTP 503).
This approach works, however the web interface doesn't allow such minimalistic backend. I had to add some dummy server to it, otherwise it wouldn't be added to the generated haproxy.conf (I could create and save it, but it wouldn't appear in haproxy.conf), so I had to add some dummy server and ended up with something more complex which I think is unnecessary:

backend deny-all_http_ipvANY
    mode            http
    log            global
    timeout connect        1
    timeout server        1
    retries            0
    http-request deny 
    server            dummy 127.0.0.1:-1 disabled

Could you make it possible to use such minimalistic backends without any servers?

#22 Updated by Petr H almost 2 years ago

And about http-request deny - it has an optional argument deny_status <status>
Currently if I want to specify it, I have to use custom action. It would be nice to have an additional deny_status field to appear if the http-request deny action is selected.

#23 Updated by Petr H almost 2 years ago

One more UI glitch:
Frontends - if I use the On toggle to enable/disable the frontend and save the config, the frontend efective status changes correctly, however when opening the Edit page of a frontend disabled this way, its Status remains Active on the Edit page (so if I don't notice that I can accidentally reenable such frontend when saving its configuration).

If I disable the frontend by changing its Status to Disable on its Edit page, this issue doesn't occur and the frontend Status will remain Disable after saving the changes and reopening its Edit page.

However there's another glitch - if I disable the frontend by changing its Status to Disable on its Edit page and save the changes, the frontend will be effectively disabled and its entry will be grayed out, however its On icon will still show enabled status.

It seems like the On toggle (on the frontends list) and the Status (on the frontend edit page) are not currently connected and work sort of independently.

Try to play with each several times and watch what happens - it's quite strange.

#24 Updated by Pi Ba almost 2 years ago

Ive added a set of commits to this branche for now..: https://github.com/PiBa-NL/FreeBSD-ports/tree/20180521-haproxy-0.57/
I want to do a little more testing before sending a pullrequest for the whole bunch to the official repository..Perhaps you can take a look already.?.

Make sure to make a backup! of your configuration though, as the new package version performs a little automatic 'conversion' of existing configuration..

Ive also uploaded it as a package that you could download and install with

pkg add -f http://piba-nl.nl/files/pfSense-pkg-haproxy-devel-0.57.txz

http://piba-nl.nl/files/pfSense-pkg-haproxy-devel-0.57.txz

If you would be able to test, i would really appreciate it :), i think ive got almost all items taken care of.. (no descriptions yet though..)

#25 Updated by Petr H almost 2 years ago

Done some quick test and it seems mostly fine, even the configuration was "migrated" successfully.
Just few things I noticed that may have been missed or those I'm not sure about:

- Inconsistent method of reordering list entries
Frontend / External address - still different than others, or is it this one the special one that's supposed to be this way?

- path_dir - I noticed a new option "Path contains within slashes" which does:

Path_CTX1    var(txn.txnpath) -m dir /ctx1
Even though as far as I understand it it behaves pretty much the same as the expected variant:
Path_CTX1    path_dir /ctx1
I'm wondering why you implemented it the 1st way - is there any particular reason to capture the path into the variable and keep it around for the whole request/response cycle? Because if not, then I believe the 2nd variant (also without the capture part that isn't shown here) should be slightly more performant (no need to store the path into the variable and keep it in memory during the whole request processing cycle).
EDIT: I see you also converted path_reg to a similar approach.
is it because of the same reasons as capturing the Host header to be usable in response processing as well - as that was one of the earlier issues?

- http-request deny deny_status
Couldn't find that one

#26 Updated by Pi Ba almost 2 years ago

Thanks for checking.

- Inconsistent method of reordering list entries
I thought i removed those up/down arrows. They are all created by the same 'htmltable' object but forgot to remove them editable state.. Done now.

- path_dir
Yes i put the item in to a variable to be able to use the same acl also for 'response actions'. I expect the performance penalty by doing this is likely not really measurable.. Is it.? It does simplify the code/ configuration to not have to think about when to use this and when the other method.

- http-request deny deny_status
Forgot to look at that, ill check it out now.

- description item possibility in htmllist
Still todo.

Also available in: Atom PDF