Created attachment 34024 [details] safe and functional implementation mod_remoteip had no setting to detect if the client is using HTTPS and set the environment variable accordingly. Tested on our acting reverse-proxy nginx, configured with this on the http listener: proxy_set_header X-Forwarded-For $remote_addr; proxy_set_header Host $http_host; proxy_set_header X-Forwarded-Proto "http"; And this on the https listener: proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header Host $http_host; proxy_set_header X-Forwarded-Proto https; The patch adds a new setting called SecureIndicatorHeader (maybe you can think of a better name). It's used as follows: SecureIndicatorHeader X-Forwarded-Proto https This tells you if the header "X-Forwarded-Proto" is present and matches the value "https" AND we've passed the proxy validation, then we set the "HTTPS" request environment variable to "on". Apache Tomcat has had something similar in it's RemoteIpValve for ages, but because of its configuration format the setting is split into protocolHeader and protocolHeaderHttpsValue.
Created attachment 34249 [details] Same as the other patch, but also change the SERVER_PORT environment variable
Created attachment 34495 [details] like previous patch, but also setting r->server->port for correct redirections Extended the patch proposed by coladict: This extended version changes the scheme and the used port accordingly to protocol set in secure_header. The port to be used can be configured in remoteip.conf ("SecureInidcatorSSLPort", default is 443). Changing scheme and port comes into effect if the server redirects requests. If not changed accordingly, the server redirects an https-Request to an http-location.
This part worries me about your proposal r->server->server_scheme = config->secure_header_value; This is not guaranteed to always be "http" or "https". That variable is set by the user, because you can't guarantee how their existing reverse proxy server reports it. I think it should be changed just to: r->server->server_scheme = "https";
Created attachment 34496 [details] change scheme to "https" if secure header is present Thank you for your feedback! I changed the line. Now every time a secure header is present and the secure header value matches the configured one, the scheme is being changed to "https".
Created attachment 34504 [details] reset scheme and port if next request to be processed is http; set scheme and port only from trusted proxies The existing patches lacks resetting scheme and port if the request object is reused by apache. Additionally, scheme and port should only be modified if the request was received from a trusted proxy. Both modifications may executed independently, e.g. setting the scheme when remote ip is invalid or header is absent.
Created attachment 34506 [details] reset scheme and port if next request to be processed is http; set scheme and port only from trusted proxies patch file; see comment above
Created attachment 34507 [details] modify IP, scheme, server_port and set https env (working with mod_rewrite now) The enhanced patch prevents mod_remoteip from running twice (and setting unexpected values) when mod_rewrite is enabled.
Created attachment 34518 [details] using apr_itoa for integer conversion, removed comment setting remoteip-proxy-ip-list Changed two things compared to previous patch: The note "remoteip-proxy-ip-list" in the request-table is meant to be used by mod_log_config to log the proxy forwarding the request. I removed the comment at this line. Instead of using snprintf for converting the integer value to a character array you can use apr_itoa. Thus the definition of a buffer at the data structure for remoteip_config_t is not necessary. There is no impact on complexity for allocating and deallocating the memory on destruction of the request pool, because the memory isn't really deallocated, but added to a list of free memory managed by the apache core. Source: http://www.fmc-modeling.org/category/projects/apache/amp/4_6Memory_resource.html Unfortunately the apache documentation doesn't go that much into detail regarding memory management.
Its unfortunately still not enough for the HTTPS variable to work with mod_rewrite, such rewriterules will not work and create a redirect loop : RewriteCond %{HTTPS} !=on RewriteRule ^/?(.*) https://%{SERVER_NAME}/$1 [R,L] Its not working because mod_rewrite doesnt rely on the "HTTPS" variable but it does call the mod_ssl function "ssl_is_https()". The code calling this function is located in "lookup_variable()" in mod_rewrite.c where it does : int flag = rewrite_is_https && rewrite_is_https(r->connection); So far i havent found a simple solution to this issue that wouldnt involve modifying mod_rewrite (or mod_ssl).
(In reply to Jean Weisbuch from comment #9) > Its unfortunately still not enough for the HTTPS variable to work with > mod_rewrite, such rewriterules will not work and create a redirect loop : > RewriteCond %{HTTPS} !=on > RewriteRule ^/?(.*) https://%{SERVER_NAME}/$1 [R,L] > > > Its not working because mod_rewrite doesnt rely on the "HTTPS" variable but > it does call the mod_ssl function "ssl_is_https()". > The code calling this function is located in "lookup_variable()" in > mod_rewrite.c where it does : > int flag = rewrite_is_https && rewrite_is_https(r->connection); > > > > So far i havent found a simple solution to this issue that wouldnt involve > modifying mod_rewrite (or mod_ssl). A way to fix it by modifying mod_rewrite (which is not a proper solution) : --- modules/mappers/mod_rewrite.c.orig 2016-12-14 23:27:25.000000000 +0100 +++ modules/mappers/mod_rewrite.c 2017-01-31 11:36:46.162667272 +0100 @@ -1923,7 +1923,8 @@ case 5: if (!strcmp(var, "HTTPS")) { - int flag = rewrite_is_https && rewrite_is_https(r->connection); + int flag = (rewrite_is_https && rewrite_is_https(r->connection)) || apr_table_get(r->connection->notes, "remoteip_env_https"); return apr_pstrdup(r->pool, flag ? "on" : "off"); } break;
Why not rather use %{ENV:remoteip_env_https} in the RewriteCond?
Comment on attachment 34518 [details] using apr_itoa for integer conversion, removed comment setting remoteip-proxy-ip-list From attachment 34518 [details] : >--- httpd-fbc5e20ead005fd3a2bec05924f9e90dfd195406/modules/metadata/mod_remoteip.c 2016-09-13 21:59:18.000000000 +0200 >+++ mod_remoteip.c 2016-12-09 13:51:20.517582087 +0100 [] >@@ -389,38 +458,75 @@ [] >+ if (secure) { >+ apr_table_setn(r->subprocess_env, "HTTPS", "on"); >+ r->server->port = config->secure_port; >+ r->server->server_scheme = config->secure_scheme; We really can't do that, r->server is shared between all threads/requests (i.e. read-only in request processing). >+ r->parsed_uri.port = r->server->port; >+ >+ /* Header available, matched and processed. May be unset now. >+ When using mod_rewrite, the HTTPS env will be unset, so set >+ a flag for setting the env var again. */ >+ apr_table_unset(r->headers_in, config->secure_header_name); >+ apr_table_set(r->connection->notes, "remoteip_env_https", "1"); >+ >+ ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, >+ "Setting scheme https and server's port %d", >+ r->server->port); > } > else { >- apr_table_unset(r->headers_in, config->header_name); >- } >- if (req->proxy_ips) { >- apr_table_setn(r->notes, "remoteip-proxy-ip-list", req->proxy_ips); >- if (config->proxies_header_name) { >- apr_table_setn(r->headers_in, config->proxies_header_name, >- req->proxy_ips); >- } >+ r->server->port = config->orig_port; >+ r->server->server_scheme = config->orig_scheme; Likewise. > } >
(In reply to Yann Ylavic from comment #11) > Why not rather use %{ENV:remoteip_env_https} in the RewriteCond? Or %{ENV:HTTPS} since the patch seems to set the "HTTPS" entry in the request's notes (used by mod_rewrite), while "remoteip_env_https" is set in the connection's (why??) notes (not used by mod_rewrite).
(In reply to Jean Weisbuch from comment #9) > Its unfortunately still not enough for the HTTPS variable to work with > mod_rewrite, such rewriterules will not work and create a redirect loop : > RewriteCond %{HTTPS} !=on > RewriteRule ^/?(.*) https://%{SERVER_NAME}/$1 [R,L] > > > Its not working because mod_rewrite doesnt rely on the "HTTPS" variable but > it does call the mod_ssl function "ssl_is_https()". > The code calling this function is located in "lookup_variable()" in > mod_rewrite.c where it does : > int flag = rewrite_is_https && rewrite_is_https(r->connection); > > > > So far i havent found a simple solution to this issue that wouldnt involve > modifying mod_rewrite (or mod_ssl). In the scenario where you use mod_remoteip, you wouldn't have mod_ssl even loaded. Making mod_rewrite to be less dependent on mod_ssl would be the solution for that problem. (In reply to Yann Ylavic from comment #12) > Comment on attachment 34518 [details] > using apr_itoa for integer conversion, removed comment setting > remoteip-proxy-ip-list > > From attachment 34518 [details] : > > >--- httpd-fbc5e20ead005fd3a2bec05924f9e90dfd195406/modules/metadata/mod_remoteip.c 2016-09-13 21:59:18.000000000 +0200 > >+++ mod_remoteip.c 2016-12-09 13:51:20.517582087 +0100 > [] > >@@ -389,38 +458,75 @@ > [] > >+ if (secure) { > >+ apr_table_setn(r->subprocess_env, "HTTPS", "on"); > >+ r->server->port = config->secure_port; > >+ r->server->server_scheme = config->secure_scheme; > > We really can't do that, r->server is shared between all threads/requests > (i.e. read-only in request processing). I have not worked on this in a while, but instead of setting r->server->port, can't we just set the connection environment variable "SERVER_PORT"?
In my case, i would need this for a shared hosting platform, so i cant rely on the users having to modify their htaccess to use %{ENV:HTTPS} instead of %{HTTPS}, it has to be transparent for the users. As for testing the subprocess_env "HTTPS" instead of the note in mod_rewrite, it definitely seems to be a better approach.
Created attachment 34709 [details] Updated patch that sets SERVER_PORT and REQUEST_SCHEME Since you correctly voiced concerns about the SERVER_PORT, I took another shot at it, and discovered that setting it in the environment doesn't work. The correct way was to register call ap_hook_http_scheme and ap_hook_default_port to register overrides. Both hook functions assume are written under the assumption you're not using mod_ssl, because you're behind a proxy. Patch is tested on trunk 2.5 version from about 20 hours ago. Testing via phpinfo() gives these values when NOT using HTTPS: SERVER_PORT 80 REQUEST_SCHEME http With HTTPS not being set, while calling it through a HTTPS request gives: HTTPS on SERVER_PORT 443 REQUEST_SCHEME https When I configured `SecureIndicatorSSLPort 5512` in httpd.conf, the `SERVER_PORT` changed accordingly. I haven't added checks for the validity of the parsed SecureIndicatorSSLPort value, but otherwise it seems complete.
Created attachment 34711 [details] Same as last plus settings validation This patch is made on the current trunk branch of the git mirror git://git.apache.org/httpd.git I find it odd that so many core Apache projects are still using SVN.
I have written something similar for ProxyProtocol instead of X-Forwarded-For, unfortunately I had not seen this issue here as I could have maybe used it as a base. My code is at https://github.com/AntagonistHQ/httpd/tree/remote-ssl I will see if I can rebase it on the patches in this bugzilla.
Created attachment 34741 [details] Fix data loss when using mod_rewrite Fixing patch fixes merge conflicts introduced in the following commit --- commit b960c87946085b8335da3e8b9b8749e3562f87cd Author: Daniel Ruggeri <druggeri@apache.org> Date: Sat Feb 4 20:14:18 2017 +0000 Change tactic for PROXY processing in Optional case git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1781701 13f79535-47bb-0310-9956-ffa450edef68 --- In addition it attempts to detect when the proxy server is reached via it's remote port (set by SecureIndicatorSSLPort or default to 443) when using ProxyProtocol. It also addresses an issue where using mod_rewrite to internally rewrite the request. The HTTPS environment variable had been renamed from 'HTTPS' to 'REDIRECT_HTTPS' in the `internal_internal_redirect` function. Personally I don't see why it needs to be that way, instead of just copying the table. It just forces mod_ssl (and now this mod) to use an extra hook for no good reason. The code that does it has been in there since 2000. Note: this still does not touch on mod_rewrite's problem of relying entirely on mod_ssl being loaded to check the `HTTPS` condition.
I have no way to test the ProxyProtocol addition, but I'm pretty sure I should have converted the server port using ntohs() when the protocol is version 2. That is lines 1056 and 1073 when viewing the diff form. I'm on a Windows machine now, so I can't easily compile it. The .dsp files and their dependencies are so outdated that I have no idea how you manage to compile the official builds.
Created attachment 34774 [details] Implementation for 2.4 branch based on 2.4.25 tag Anyone wanting to use this feature for 2.4 can apply this one, but it requires changes to the core, so rebuilding just the module won't work. The reason is I ran into this https://www.sslshopper.com/apache-redirect-http-to-https.html I found-out that someone using `Redirect / https://mysite.com/` in their VirtualHost will get stuck in a redirect loop, and the only solution is to add a hook called after the headers are parsed, but before VirtualHost is resolved.
Created attachment 34781 [details] Allow multiple SSL ports It came to mind that hosting providers often have their administration system on a different port than the main, so allowing for multiple ports to be detected as secure would give them that flexibility. For example port 443 is the default, but maybe they also use 8443 (common secondary HTTPS port). That also means they should probably send a the port information. Tested via ProxyProtocol v2, as optional setting, both with and without ProxyProtocol. Settings names have been made more consistent to be obvious they belong to this mod. Current logic is this after validating proxy: If RemoteIPServerPortHeader is set and given, then port information is available. If using ProxyProtocol, then port information is available. If not using ProxyProtocol, and RemoteIPSecureHeader is sent valid value, set secure = on. If RemoteIPSSLPorts is set, and port info is available { Check if port is in list. Override secure setting from RemoteIPSecureHeader. Ports are given higher priority. } If RemoteIPSSLPorts is not set and port info is available, then SSL port defaults to 443 and overrides RemoteIPSecureHeader setting. If RemoteIPSSLPorts is not set and no port info is available, but RemoteIPSecureHeader has set secure, then port defaults to 443. I believe all that's left to do now is documentation.
Tested on our acting reverse-proxy nginx, configured with this on the http listener: proxy_set_header X-Forwarded-For $remote_addr; proxy_set_header Host $http_host; proxy_set_header X-Forwarded-Proto "http"; And this on the https listener: proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header Host $http_host; proxy_set_header X-Forwarded-Proto https; yes, you ar right http://www.freewing-model.com
Any chance of getting this added into 2.4 or 2.5?