charm-helpers icon indicating copy to clipboard operation
charm-helpers copied to clipboard

Adding send-proxy-v2 to see actual client ip

Open xtrusia opened this issue 9 months ago • 8 comments

currently, log shows proxy ip but need actual client ip

This PR suggest to add required module and configuration method to setup options for Apache2 templates and options for haproxy templates

#918

xtrusia avatar Mar 20 '25 04:03 xtrusia

you are putting this feature behind a config option , does this mean all charms will be updated to include a new option in their config.yaml? ... can't we just make this the default behavior?, is there any cons on making this the default?

You're right. I actually thought the same at first, and it seemed fine to just make it the default behavior.

But I had a question during testing. For example, in the keystone charm, there are its own templates for apache2. e.g wsgi-openstack-api.conf and openstack_https_frontend.conf.

I tested this, and it turns out that if a charm includes its own template files, it uses those instead of the ones from charmhelpers. So even if we make this the default behavior in charmhelpers, we'd still need to update each individual charm that overrides the templates.

Also, fixes would need to be applied across haproxy, apache templates, and modules all at once, since they're all interdependent. So charmhelpers(sync with charm) and the keystone charm itself(templates update) should be updated together, otherwise things will break I think.

If we adopt conf option, we can split commit to sync with charmhelpers and change each charms.

But yes. making this the default also makes sense to me as well.

xtrusia avatar Mar 25 '25 00:03 xtrusia

On Mon, 2025-03-24 at 17:55 -0700, Seyeong Kim wrote:

you are putting this feature behind a config option , does this mean all charms will be updated to include a new option in their config.yaml? ... can't we just make this the default behavior?, is there any cons on making this the default? You're right. I actually thought the same at first, and it seemed fine to just make it the default behavior. But I had a question during testing. For example, in the keystone charm, there are its own templates for apache2. e.g wsgi-openstack-api.conf and openstack_https_frontend.conf. I tested this, and it turns out that if a charm includes its own template files, it uses those instead of the ones from charmhelpers. So even if we make this the default behavior in charmhelpers, we'd still need to update each individual charm that overrides the templates. Also, fixes would need to be applied across haproxy, apache templates, and modules all at once, since they're all interdependent. So charmhelpers(sync with charm) and the keystone charm itself(templates update) should be updated together, otherwise things will break I think. If we adopt conf option, we can split commit to sync with charmhelpers and change each charms. But yes. making this the default also makes sense to me as well. — Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. You are receiving this because you commented.Message ID: <juju/charm- @.***> xtrusiaxtrusia left a comment (juju/charm-helpers#919) [1] you are putting this feature behind a config option , does this mean all charms will be updated to include a new option in their config.yaml? ... can't we just make this the default behavior?, is there any cons on making this the default? You're right. I actually thought the same at first, and it seemed fine to just make it the default behavior. But I had a question during testing. For example, in the keystone charm, there are its own templates for apache2. e.g wsgi-openstack-api.conf and openstack_https_frontend.conf. This is true, although not that common. AFAIK charm-keystone is an outlier here rather the norm. I tested this, and it turns out that if a charm includes its own template files, it uses those instead of the ones from charmhelpers. So even if we make this the default behavior in charmhelpers, we'd still need to update each individual charm that overrides the templates. we would need the usual "charm-helper sync" dance, yes. Also, fixes would need to be applied across haproxy, apache templates, and modules all at once, since they're all interdependent. So charmhelpers(sync with charm) and the keystone charm itself(templates update) should be updated together, otherwise things will break I think. If we adopt conf option, we can split commit to sync with charmhelpers and change each charms. I'm not following you on this one, you could have a charmhelpers sync for keystone and a subsequent patch that modifies the keystone templates, what makes you think things will break?

[1] view it on GitHub https://github.com/juju/charm-helpers/pull/919#issuecomment-2749759211 [2] unsubscribe https://github.com/notifications/unsubscribe-auth/AAAYLH7VUVJWBGAOXTTFH632WCSRVAVCNFSM6AAAAABZMOX736VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONBZG42TSMRRGE

freyes avatar Mar 25 '25 13:03 freyes

@freyes If we can do both at once, no issue. if haproxy.conf has send-proxy-v2 but apache2 conf doesn't have it (vice versa) it doesn't seem to work. so I mentioned it.

So, would you like me to change it to default instead of adding config option? I'll change the fix tomorrow.

Thanks so much

xtrusia avatar Mar 25 '25 13:03 xtrusia

something we haven't talked about is what happens when you have only side of the connection with the proxy protocol enabled, specifically:

  1. haproxy set with send-proxy-v2, but apache doesn't have remoteip enabled
  2. haproxy is NOT set with send-proxy-v2, but apache has enabled remoteip

I would assume in the case of (2) nothing happens, and the old behavior where the client ip address is not received by apache, right?, but what about in the case of (1)?

This is why I mentioned in the beginning they should be fixed at the same time.(for keystone) but sorry my explanation was not enough. I remember that (1) didn't work. keystone command returned error. But I'll double check it today and share the result here.

  1. 400 error
  2. connection aborted

Test

  1. deploy keystone and mysql charm
  2. openstack endpoint list ( works fine )
  3. a2enmod remoteip
  4. change configuration files and restart them

Thanks so much.

xtrusia avatar Mar 26 '25 23:03 xtrusia

@freyes I'm wondering if you might check the latest comment and have some time to review this issue. Thanks for your review.

xtrusia avatar Apr 22 '25 00:04 xtrusia

re-added configuration option since we discussed it in sprint.

xtrusia avatar May 27 '25 02:05 xtrusia

  1. I think it would be nice to have a draft PR up for charm-keystone incorporating these changes, so we can review/test the implementation before this is merged. As we may find we later need changes and have to do a second PR. Could you upload such a change in draft mode for review?

It would also be great to explicitly document the following

  1. Do both apache2 and haproxy support these exact same modules/configuration options on focal (for focal-yoga) as well as jammy + noble. The requesting environment is Jammy-Yoga so the intent is to backport this feature to at least Yoga. But the same charm has to support both Jammy-Yoga and Focal-Yoga.

  2. Do we (now) intend for the config option to be disabled or enabled by default. While avoiding an unexpected change on existing environments is ideal, I can't really think of scenarios where people would NOT want this option enabled - if they are using both haproxy and apache2. I agree it would be ideal to have an option to turn it off in case of a problem. If we are going to go ahead with having it disabled by default, can we document exactly why - is it only abundance of caution, or otherwise what specific scenarios may be broken without it.

  3. What happens during either charm upgrades (or just charm config change) when only 1 or 2 units have finished upgrading their config file and restarting. (since you do 1 unit at a time, which hosts both) - in cases where the config option is both off and on. Will it still work, or fail during that time until all units have updated. This may be an issue for managed upgrades where daemon restarts are not being done. When enabled, RemoteIPProxyProtocol requires that clients connect with PROXYv2 by default, so I suspect this won't work.

Lastly, some review notes

  1. When deployed in a non-HA configuration, that this is not enabled. I am not sure this is the case currently? Perhaps by default we would only activate this option when haproxy is in use for a HA deployment with the hacluster relation, but the config option could force it on otherwise.

  2. When enabled, RemoteIPProxyProtocol requires that clients connect with PROXYv2. We should check whether any charms have NRPE checks or other reasons the port is conneted to with a non-PROXYv2 client that will be broken by this. It may also frustrate us at debug time, when you try to directly query the apache2 server when you are trying to debug problems for example. Perhaps it's desirable to configure mod_remoteip to -only- request PROXYv2 from the specific list of haproxy IP addresses. Seems this may be possible with RemoteIPProxyProtocolExceptions.

  3. That other clients cannot speak Proxy V2 and spoof the client source IP. Currently, we don't firewall off apache2 access and many clients of the customer's network can often connect directly to the apache2 port, bypassing haproxy. So we need to prevent this by configuring either a firewall, or using configuration options like RemoteIPTrustedProxy or RemoteIPInternalProxy to limit this access to certain IP addresses - e.g. the IP addresses of the other units haproxy instance.

  4. Looking at the apache2 documentation, it seems it ignores RFC1918/Intranet IP addresses unless a specific RemoteIPInternalProxy is configured. I think in practice, most Charmed OpenStack customers will be using such addresses, and so we may need to configure this option specifically otherwise the config option may be mostly useless.

lathiat avatar Jun 27 '25 05:06 lathiat

image

It seems that haproxy and apache2 are tightly coupled so other components connect via haproxy only, they don't have to access to apache2 directly by proxyv2.

Testing Notes

  1. Test keystone charm

    • master: https://review.opendev.org/c/openstack/charm-keystone/+/955042
    • 2024.1: https://review.opendev.org/c/openstack/charm-keystone/+/962637
    • yoga: https://review.opendev.org/c/openstack/charm-keystone/+/962639
  2. RemoteIPProxyProtocolExceptions

    • Adding RemoteIPProxyProtocolExceptions 127.0.0.1 is sufficient, since the charm itself checks connectivity via localhost during deployment.
    • Tested several components with the updated Keystone and no issues observed so far.
    • Plan: test different combinations (focal-yoga, jammy-yoga, etc.) with both older and new config changes.
  3. Directive availability

    • Supported since Apache 2.4.31.
    • Focal ships Apache 2.4.41 → supported.
    • Ref: https://httpd.apache.org/docs/2.4/mod/mod_remoteip.html
  4. Default value

    • Should be false for compatibility.
  5. NRPE tests

    • Focal-yoga with Nagios web UI monitoring
      • Config changes tested (on/off) → no issues.
    • Jammy-yoga with Nagios web UI monitoring
      • Config changes tested (on/off) → no issues.
  6. zaza test

    • master
      • Ran zaza test with the patch above (although it doesn’t include a conf test, it at least confirms default conf works).
        • Test result(master): https://pastebin.ubuntu.com/p/zTVXzkRtyX/
      • With test https://github.com/openstack-charmers/zaza-openstack-tests/pull/1328/files
      • Test result(master): https://pastebin.ubuntu.com/p/j2VC75X3qf/
    • 2024.1
      • Test result: https://pastebin.ubuntu.com/p/MW5w8bD4VV/
    • yoga
      • Test result: https://pastebin.ubuntu.com/p/T9J9ZT3Kqy/
  7. RemoteIPTrustedProxy 127.0.0.1 doesn't block connection from somewhere else. like below picture. it just checks X-F-F header image

RemoteIPTrustedProxy 127.0.0.1 checks only x-forwarded-for. if I use below script(injecting fake client ip), it just pass. We do need to setup firewall for apache2 port(in this case 4980) as proxyv2 doesn't have any

image

As above structure in HA, I think security issue should be done by new ticket. if we secure them strictly, may set up firewall instead of setting up apache2.

#!/usr/bin/env python3
"""
Send an HTTP request with a PROXY protocol v2 header (IPv4/TCP).
Usage:
  ./send_proxy_v2.py <target_ip> <target_port> <fake_client_ip> <fake_client_port>
"""

import argparse
import socket
import struct


def build_proxy_v2_ipv4(fake_src_ip: str, dst_ip: str, fake_src_port: int, dst_port: int) -> bytes:
    # PROXY v2 signature + version/cmd + family/proto + addr length(IPv4=12)
    sig = b"\r\n\r\n\x00\r\nQUIT\n"
    ver_cmd = b"\x21"   # v2 + PROXY
    fam_proto = b"\x11" # INET + STREAM(TCP)
    addr_len = struct.pack("!H", 12)

    src_addr = socket.inet_aton(fake_src_ip)
    dst_addr = socket.inet_aton(dst_ip)
    src_port_b = struct.pack("!H", fake_src_port)
    dst_port_b = struct.pack("!H", dst_port)

    return sig + ver_cmd + fam_proto + addr_len + src_addr + dst_addr + src_port_b + dst_port_b


def main() -> int:
    p = argparse.ArgumentParser(add_help=True)
    p.add_argument("target_ip")
    p.add_argument("target_port", type=int)
    p.add_argument("fake_client_ip")
    p.add_argument("fake_client_port", type=int)
    p.add_argument("--path", default="/", help="HTTP path (default: /)")
    p.add_argument("--host", default=None, help="Host header (default: target_ip)")
    args = p.parse_args()

    host_header = args.host or args.target_ip
    proxy_hdr = build_proxy_v2_ipv4(args.fake_client_ip, args.target_ip, args.fake_client_port, args.target_port)
    http_req = f"GET {args.path} HTTP/1.1\r\nHost: {host_header}\r\nConnection: close\r\n\r\n".encode("ascii")

    payload = proxy_hdr + http_req

    try:
        with socket.create_connection((args.target_ip, args.target_port), timeout=5) as s:
            s.sendall(payload)
            buf = bytearray()
            while True:
                chunk = s.recv(4096)
                if not chunk:
                    break
                buf += chunk
    except Exception as e:
        print(f"Error: {e}")
        return 1

    print(buf.decode("utf-8", errors="replace"))
    return 0


if __name__ == "__main__":
    raise SystemExit(main())

xtrusia avatar Jul 16 '25 04:07 xtrusia