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

net/haproxy: Action rules are not sorted properly in haproxy.conf #1925

Closed
astrandb opened this issue Jul 16, 2020 · 7 comments
Closed

net/haproxy: Action rules are not sorted properly in haproxy.conf #1925

astrandb opened this issue Jul 16, 2020 · 7 comments
Assignees
Labels
bug Production bug

Comments

@astrandb
Copy link

Describe the bug
Redirect rules are not processed in proper order which leads to unpredictable behaviour. This is problematic e.g. when using haproxy and Let's encrypt combined.

I have looked into the generated haproxy.conf and I can see that ACL:s are sorted according to the order of entries in frontend gui (Public services). But linked actions are generated in wrong order. In fact the order is not changed if the rule entries are shuffled in gui.

Screenshots
Example 1
Anteckning 2020-07-16 130411

frontend the_front_80
    bind 0.0.0.0:80 name 0.0.0.0:80
    mode http
    option http-keep-alive
    # tuning options
    timeout client 30s

    # logging options
    # ACL: find_acme_challenge
    acl acl_5ec7b26972aa81.78795642 path_beg -i /.well-known/acme-challenge/
    # ACL: cam1_example_nu
    acl acl_5ec7baf41f9470.70661915 hdr(host) -i cam1.example.nu
    # ACL: is_http
    acl acl_5ec7b2fadbd856.27310243 ssl_fc
    # ACL: ull_example_nu
    acl acl_5ec7b3263f2652.69975208 hdr(host) -i ull.example.nu

    # ACTION: redirect_ha_http2https
    http-request redirect scheme https code 301 if !acl_5ec7b2fadbd856.27310243 acl_5ec7b3263f2652.69975208

    # ACTION: redirect_acme_challenges
    use_backend acme_challenge_backend if acl_5ec7b26972aa81.78795642
    # ACTION: redirect_cam1
    use_backend pool_cam1 if acl_5ec7baf41f9470.70661915

Example 2
Anteckning 2020-07-16 131001

frontend the_front_80
    bind 0.0.0.0:80 name 0.0.0.0:80
    mode http
    option http-keep-alive
    # tuning options
    timeout client 30s

    # logging options
    # ACL: find_acme_challenge
    acl acl_5ec7b26972aa81.78795642 path_beg -i /.well-known/acme-challenge/
    # ACL: is_http
    acl acl_5ec7b2fadbd856.27310243 ssl_fc
    # ACL: ull_example_nu
    acl acl_5ec7b3263f2652.69975208 hdr(host) -i ull.example.nu
    # ACL: cam1_example_nu
    acl acl_5ec7baf41f9470.70661915 hdr(host) -i cam1.example.nu

    # ACTION: redirect_ha_http2https
    http-request redirect scheme https code 301 if !acl_5ec7b2fadbd856.27310243 acl_5ec7b3263f2652.69975208

    # ACTION: redirect_acme_challenges
    use_backend acme_challenge_backend if acl_5ec7b26972aa81.78795642
    # ACTION: redirect_cam1
    use_backend pool_cam1 if acl_5ec7baf41f9470.70661915

Environment
OPNsense 20.1.8_1-amd64

@fraenki fraenki self-assigned this Jul 16, 2020
@fraenki fraenki added the bug Production bug label Jul 20, 2020
@fraenki
Copy link
Member

fraenki commented Jul 20, 2020

I was able to reproduce this issue. I think this was caused by the changes in opnsense/core#4070 and opnsense/core@3731bd1.

@AdSchellevis
Copy link
Member

@fraenki these commits are likely not related (grid sorting shouldn't be used here). If you sort the config.xml, the output is as expected. My guess would be it's something related to Javascript (could be jquery).

@fraenki
Copy link
Member

fraenki commented Jul 20, 2020

@AdSchellevis You're right, it looks like this was already broken in 20.1.4 (the oldest testbox I have available). :-/

@fraenki
Copy link
Member

fraenki commented Jul 24, 2020

I did a new test on both 20.1.8 and 20.7-RC1 and I have to correct my previous statement: rule sorting works as expected.

However, there is one notable exception: use_[backend|server] rules are always added to the end of the ruleset. This was introduced back in 2018 with 1f9250e. It was meant as a workaround to make these rules work (because rule sorting was not yet available back then). I have to say this is pretty unexpected behaviour and even though I've merged this change back then I was really surprised to see this now. Apparently this workaround was obsoleted by #582.

I plan to remove the special handling of use_[backend|server] rules, so that the sort order is always honored.

fraenki added a commit to fraenki/plugins that referenced this issue Jul 24, 2020
@fraenki
Copy link
Member

fraenki commented Jul 24, 2020

@astrandb Could you give 1685999 a try?

opnsense-patch -c plugins 1685999b9dd5cc659d99901d2eada79a8fa98c16

@astrandb
Copy link
Author

astrandb commented Jul 25, 2020

@fraenki The patch works as expected, the rules are created in order, but "Test syntax" results in this message:

[WARNING] 206/084823 (97809) : parsing [/usr/local/etc/haproxy.conf:75] : a 'http-request' rule placed after a 'use_backend' rule will still be processed before.
Configuration file is valid

After digging into HAProxy docs and other sources it seems that HAProxy processes http-request rules before use_backend. So in order to avoid the warning message it is probably best to create the use_backend rules after the http-request rules.

My use case is to redirect all http requests to https except those related to acme_challenges. When I got unpredictable results my first thought was that the order of the rules was the root of the problem. Perhaps I have to think of alternate ways to handle my use case, e.g. to process the http-https redirect on the backend instead.

@fraenki
Copy link
Member

fraenki commented Jul 25, 2020

The HAProxy warning simply tells the user that the rules are placed in the wrong order. However, that's not a big issue, because (as stated by the warning message) HAProxy corrects this specific error internally.

I am aware that these messages will now appear for some users. They can easily be fixed by changing the order of these rules.

Still, I think it's a change for the better to have full control over the sort order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Production bug
Development

No branches or pull requests

3 participants