-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Reimplementation of remoteip-rpaf for 2.4.48 and up (updated to trunk) #191
base: trunk
Are you sure you want to change the base?
Conversation
Changes to 2.4.41-46 version: - No more obscure hacks of SSL module and internal metadata for HTTPS simulation handling and port alteration, rely fully on existing Apache hooks (including new 2.4.48 ssl_conn_is_ssl() hook) - Use apr_table_nset() for connection annotation and header changes, we strdup() anyways - RemoteHTTPSEnableProto option omitted now means setting HTTPS mode simulation on any RemoteProtoHeader header presence, without verifying header contents, for compatibility with the previous default "RemoteHTTPSEnableProto https" needs to be used
Added 'static' declaration missing from the initial commit, build passes now. |
I think this also needs some justification about how it's usable. This one was made with shared hosting clusters and frontend-to-backend splits in mind.
|
Should the options not be prefixed RemoteIP*? So Would love to see this added to master 👍 |
Well, they are technically part of remoteip module, but as they do not exactly affect/specify remote IP but remote Host and Port, remote protocol flag or everything at once (in case of RemoteAllowOnlyInternalProxies), I've named them accordingly. |
I tried this patch. It works, but there is something with But excellent patch otherwise, thank you :) |
If you please can share a bit of configuration that allows to reproduce the issue, I'll definitely look into it as I want this to be as clean as possible. If it's not exactly publicly sharable but possible to send in private, I can be reached by mail at alex [meow] alex-at.net |
It's really hard to debug, but after some time (many reloads) it's like things leak from one request to another. Suddenly %HTTPS in mod_rewrite doesn't return the correct value, where on the previous request it did. After a httpd restart it works again for a little while. I'm using
|
Hello, @tomsommer I have been using this on my massive shared hostings for years, especially with mod_rewrite for customers, and am not seeing this issue. HTTPS state adjustment relies on the internal Apache hook, so I wonder. Nevertheless, if there is any possibility of this happening, I want to investigate it as it potentially may shoot us in the leg as well. First things first, please state the MPM and list of loaded modules you are using, also the nature of the request (to static files, to dynamic code). Having full mod_rewrite rules (without sensitive data) you are able to reproduce this on also would be very helpful. That's because I suspect this could be mpm_event or other threaded MPM, then I would have to thoroughly doublecheck the local flags scope in the code and dig in that direction (we are using mpm_event at places for it as well, but not for shared hostings, still had never seen the issue, but yes, anyways I want to thoroughly check that). |
Okay, knowing it's mpm_event already points me to the test set creation. Not promising it would be fast to diagnose, but I'll surely post if I find (or do not) something. |
There is definitely some leakage between requests on mpm_event, the Don't know if gnif/mod_rpaf#42 is relevant |
I turned off keep-alive on the upstream in nginx and that solved the problem, but of course I would like to use keep-alive :) |
Yes, I see where it may happen now. It is always spurious 'https on' state that should not be there, am I correct? |
Thanks to @tomsommer for reporting that bug On keepalive connections when using specific RemoteIP "HTTPS" header content matching, connection note of remote_https is not reset when there is no match and so "HTTPS on" flag leaks between subsequent requests inside single connection in this case. Reproduce: RemoteHTTPSEnableProto option used, single keepalive connection, virtual "HTTP" connection follows virtual "HTTPS" one Fix: unset connection note on no header content match path.
Please test with the latest commit added. Should fix the leakage of HTTPS flag under the conditions stated in the commit description. |
@AlexAT Seems to be fixed now 👍 |
IIUC the scenario fixed here is httpd with So Btw, it seems that |
Hello, @ylavic Thanks for the review, I'll do the changes accordingly today or tomorrow. |
For the hook order and emulating off mode when it's SSL module handling requests, I assume I will have to use APR_HOOK_REALLY_FIRST given the nature of the function then if and then only if config->proto_header_name is set, return DONE instead of DECLINED so the process is complete, but the result is not OK (as it's compared with OK in ap_ssl_conn_is_ssl() @ ssl.c). Will first check what adverse effects it may have. [glancing through the code only shows mod_nw_ssl may be not compatible] |
I was talking about the remoteip_hook_http_scheme() actually, I don't think mod_remote_ip should hook into ssl_conn_is_ssl, that one should stay local IMO (we need a way in httpd to know for sure whether the local connection is TLS or not). I didn't look closely at all the ap_ssl_conn_is_ssl() usages, but the two local and remote indicators shouldn't be mixed.. |
This may mean adding a new ssl_conn_remote_is_ssl hook and using ap_ssl_conn_remote_is_ssl() where ap_ssl_conn_is_ssl() is currently misused if/when these changes to mod_remote_ip are in. It could be an APR_OPTIONAL_FN too, provided by mod_remote_ip and uses by e.g. ap_conn_is_remote_ssl() if it exists. |
Not in scope of the pull request itself but for those who use it, the variant for 2.4.56 (lacks APLOGNO(), lacks some docs changes). httpd-2.4-remoteip-2.4.56-20230327.patch Take care this can still contain bugs I do not know about. I've put this on a single production cluster and it has no issues atm, but anyways, take care, test before using. |
Is this working in 2.4.57? Maybe change the title of the pull-request :) |
Alas, maintaining this onwards for next versions has become harder as it contains more changes after reviewing :) Here we go: httpd-2.4-remoteip-2.4.57-20220531.patch Totally the same as 2.4.56 one above, adjusted for changed ap_mmn.h. Builds well, should work all the same as there are no related changes to the core it seems. EDIT @ 31/05/2023: geez, uploaded wrong file 30/05/2023. Please use the one I placed here 31/05/2023 for builds. |
I think this can already be merged actually - but we need maintainers to somehow check and re-review it beforehand. |
I have by mistake uploaded adjusted older version of the patch in variant for 2.4.57. Terribly sorry for that. |
This post is just to confirm latest patch version is running in my production environment @ version 2.4.57 without any new or noticeable issues for a month already. |
I too would love to see this merged, this would be exceptionally handy for our platform. |
Maybe this has been debunked already, but is the new hook really needed vs existing scheme hook -- and trusting the scheme for things like %{HTTPS}e? |
@covener Yes, the point was remote scheme should not affect local modules behavior and be separate, as it indeed can be HTTP/HTTPS flag over HTTP or HTTPS or whatever. |
@AlexAT I don't suppose you've got some binaries built for this? I'm currently stuck between a rock and a hard place with RPAF not liking keepalive and another bug that goes away with keepalive enabled. |
@shaneshort Upd. It seems OL 9.3 would come with Apache 2.4.57 out of the box, for this I can build the replacement module rather seamlessly I think. I don't know if 'CentOS compatible' would be ok for you or you can wait for it, but once it's out, I'll post links to the 64-bit RPM with mod_remoteip_alexat (with a different name so it can be used along but instead of original module). 2.4.57/58 is what I actually run the modified module with, so should be more or less safe to use. |
@shaneshort Okay, here we go, there is a build for OL 9.3 release where native httpd is altered and rebuilt. Few points beforehand:
Two additional minor and totally non-intrusive / non-default-behavior-changing patches are included as they are in my 'hosting' patch subset which I based the build on (I'm actually going to use this build myself). Both patches are of course public and can obviously be found in the source RPM.
Related repository for OL9.3 x86_64 can be browsed at: https://yum.alex-at.net/ol9/native/ If .repo file is desired, it is at https://yum.alex-at.net/alex-at-ol9-native.repo , but take care I don't gurantee uptime and overall availability of the repository Source RPM is provided so you can check what is inside and build yourself if you (pretty much obviously) do not trust the binaries built, at https://yum.alex-at.net/ol9/native/httpd-2.4.57-5.0.1.alex.1.el9.x86_64.rpm |
To maintainers: please please check if there are any chances this pull request can be re-reviewed and possibly accepted. |
@AlexAT Thanks for all this. Could you make a patch for 2.4.58? |
Hello @tomsommer , sure, I'll look into that this weekend as I have to build it for myself anyways. |
Hello @tomsommer |
@AlexAT and 2.4.59? :) Thanks for all the work on this, I hope this gets merged ASAP. otherwise maybe you could create a new mod_remoteip so it's not just a patch? |
Here we go: (only a minor 'magic number' change since the previous patch, it is running for me for a good while, no issues it seems) |
And here we go for 2.4.62. Again only a minor 'magic number' change. |
This is related to #68 and is the reimplementation of it to 2.4.48, removing all the hacky parts.
Please tell if anything else needs to be adjusted. If someone is using my older variant (from #68), please test in production to make sure I did not introduce any new bugs (I did the initial testing myself though).
The feature description is:
mod_remoteip is extended by multiple options handy for frontend proxies moving traffic to Apache backends
Changes to 2.4.41-46 version in #68: