Bug 59829 - Detect HTTPS marker from reverse proxy
Summary: Detect HTTPS marker from reverse proxy
Status: NEW
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_remoteip (show other bugs)
Version: 2.5-HEAD
Hardware: PC Linux
: P2 enhancement with 4 votes (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2016-07-08 14:05 UTC by Yordan Gigov
Modified: 2020-04-16 16:05 UTC (History)
4 users (show)



Attachments
safe and functional implementation (3.94 KB, patch)
2016-07-08 14:05 UTC, Yordan Gigov
Details | Diff
Same as the other patch, but also change the SERVER_PORT environment variable (4.02 KB, patch)
2016-09-14 13:47 UTC, Yordan Gigov
Details | Diff
like previous patch, but also setting r->server->port for correct redirections (4.66 KB, patch)
2016-12-01 13:57 UTC, Peter H.
Details | Diff
change scheme to "https" if secure header is present (4.64 KB, patch)
2016-12-02 05:43 UTC, Peter H.
Details | Diff
reset scheme and port if next request to be processed is http; set scheme and port only from trusted proxies (21.37 KB, patch)
2016-12-07 16:07 UTC, martin.kofahl
Details | Diff
reset scheme and port if next request to be processed is http; set scheme and port only from trusted proxies (10.74 KB, patch)
2016-12-07 19:51 UTC, martin.kofahl
Details | Diff
modify IP, scheme, server_port and set https env (working with mod_rewrite now) (11.59 KB, patch)
2016-12-08 08:41 UTC, martin.kofahl
Details | Diff
using apr_itoa for integer conversion, removed comment setting remoteip-proxy-ip-list (11.28 KB, patch)
2016-12-12 09:22 UTC, Peter H.
Details | Diff
Updated patch that sets SERVER_PORT and REQUEST_SCHEME (6.29 KB, patch)
2017-02-02 11:24 UTC, Yordan Gigov
Details | Diff
Same as last plus settings validation (6.71 KB, patch)
2017-02-03 14:32 UTC, Yordan Gigov
Details | Diff
Fix data loss when using mod_rewrite (11.07 KB, patch)
2017-02-09 16:39 UTC, Yordan Gigov
Details | Diff
Implementation for 2.4 branch based on 2.4.25 tag (12.89 KB, patch)
2017-02-22 13:17 UTC, Yordan Gigov
Details | Diff
Allow multiple SSL ports (21.69 KB, patch)
2017-02-27 11:43 UTC, Yordan Gigov
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yordan Gigov 2016-07-08 14:05:22 UTC
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.
Comment 1 Yordan Gigov 2016-09-14 13:47:43 UTC
Created attachment 34249 [details]
Same as the other patch, but also change the SERVER_PORT environment variable
Comment 2 Peter H. 2016-12-01 13:57:32 UTC
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.
Comment 3 Yordan Gigov 2016-12-01 19:51:05 UTC
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";
Comment 4 Peter H. 2016-12-02 05:43:01 UTC
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".
Comment 5 martin.kofahl 2016-12-07 16:07:09 UTC
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.
Comment 6 martin.kofahl 2016-12-07 19:51:14 UTC
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
Comment 7 martin.kofahl 2016-12-08 08:41:56 UTC
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.
Comment 8 Peter H. 2016-12-12 09:22:27 UTC
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.
Comment 9 Jean Weisbuch 2017-01-30 18:08:45 UTC
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).
Comment 10 Jean Weisbuch 2017-01-31 10:42:54 UTC
(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;
Comment 11 Yann Ylavic 2017-01-31 11:36:39 UTC
Why not rather use %{ENV:remoteip_env_https} in the RewriteCond?
Comment 12 Yann Ylavic 2017-01-31 11:43:18 UTC
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.

>     }
>
Comment 13 Yann Ylavic 2017-01-31 12:08:39 UTC
(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).
Comment 14 Yordan Gigov 2017-01-31 12:52:23 UTC
(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"?
Comment 15 Jean Weisbuch 2017-01-31 13:22:29 UTC
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.
Comment 16 Yordan Gigov 2017-02-02 11:24:50 UTC
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.
Comment 17 Yordan Gigov 2017-02-03 14:32:17 UTC
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.
Comment 18 Sander Hoentjen 2017-02-07 19:05:24 UTC
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.
Comment 19 Yordan Gigov 2017-02-09 16:39:26 UTC
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.
Comment 20 Yordan Gigov 2017-02-09 17:28:33 UTC
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.
Comment 21 Yordan Gigov 2017-02-22 13:17:51 UTC
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.
Comment 22 Yordan Gigov 2017-02-27 11:43:21 UTC
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.
Comment 23 atten 2019-06-28 03:30:37 UTC
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
Comment 24 Tom Sommer 2020-04-16 16:05:53 UTC
Any chance of getting this added into 2.4 or 2.5?