phusion / passenger

A fast and robust web server and application server for Ruby, Python and Node.js
https://www.phusionpassenger.com/
MIT License
5k stars 547 forks source link

HTTPS is sometimes set to "on" in the environment for HTTP requests #1472

Closed tomhughes closed 9 years ago

tomhughes commented 9 years ago

We have backend server which receive requests via a frontend proxy, which is just another copy of apache using mod_proxy_http to pass on the request. The backend then passes the request to rails running under passenger 5.0.6.

When requests are made to the frontend via https this all works find but when requests to the frontend are made via http the value @env['HTTPS'] is on for some requests, which causes rack to return an https URL which in turn causes signature verification failures for OAuth signed requests.

I have verified with wireshark that the frontend correctly uses http to pass the request to the backend in this case, and added logging to Rack::Request to verify the observed behaviour.

tomhughes commented 9 years ago

It appears that REMOTE_PORT is being set to match, which suggests that req->https is actually true at the apache level...

tomhughes commented 9 years ago

Here's a dump, captured with strace, of a working request being passed to the application process:

[pid  2201] read(11, "REQUEST_URI\0/api/0.6/user/detail"..., 1207) = 1207
 | 00000  52 45 51 55 45 53 54 5f  55 52 49 00 2f 61 70 69  REQUEST_URI./api |
 | 00010  2f 30 2e 36 2f 75 73 65  72 2f 64 65 74 61 69 6c  /0.6/user/detail |
 | 00020  73 3f 6f 61 75 74 68 5f  63 6f 6e 73 75 6d 65 72  s?oauth_consumer |
...
 | 00110  6b 36 59 6f 50 4b 66 39  26 6f 61 75 74 68 5f 76  k6YoPKf9&oauth_v |
 | 00120  65 72 73 69 6f 6e 3d 31  2e 30 00 50 41 54 48 5f  ersion=1.0.PATH_ |
 | 00130  49 4e 46 4f 00 2f 61 70  69 2f 30 2e 36 2f 75 73  INFO./api/0.6/us |
 | 00140  65 72 2f 64 65 74 61 69  6c 73 00 53 43 52 49 50  er/details.SCRIP |
 | 00150  54 5f 4e 41 4d 45 00 00  51 55 45 52 59 5f 53 54  T_NAME..QUERY_ST |
 | 00160  52 49 4e 47 00 6f 61 75  74 68 5f 63 6f 6e 73 75  RING.oauth_consu |
...
 | 00260  68 5f 76 65 72 73 69 6f  6e 3d 31 2e 30 00 52 45  h_version=1.0.RE |
 | 00270  51 55 45 53 54 5f 4d 45  54 48 4f 44 00 47 45 54  QUEST_METHOD.GET |
 | 00280  00 53 45 52 56 45 52 5f  4e 41 4d 45 00 74 68 6f  .SERVER_NAME.tho |
 | 00290  72 6e 2d 30 33 2e 6f 70  65 6e 73 74 72 65 65 74  rn-03.openstreet |
 | 002a0  6d 61 70 2e 6f 72 67 00  53 45 52 56 45 52 5f 50  map.org.SERVER_P |
 | 002b0  4f 52 54 00 38 30 00 53  45 52 56 45 52 5f 53 4f  ORT.80.SERVER_SO |
 | 002c0  46 54 57 41 52 45 00 41  70 61 63 68 65 2f 32 2e  FTWARE.Apache/2. |
 | 002d0  34 2e 37 20 28 55 62 75  6e 74 75 29 20 50 68 75  4.7 (Ubuntu) Phu |
 | 002e0  73 69 6f 6e 5f 50 61 73  73 65 6e 67 65 72 2f 35  sion_Passenger/5 |
 | 002f0  2e 30 2e 36 00 53 45 52  56 45 52 5f 50 52 4f 54  .0.6.SERVER_PROT |
 | 00300  4f 43 4f 4c 00 48 54 54  50 2f 31 2e 31 00 52 45  OCOL.HTTP/1.1.RE |
 | 00310  4d 4f 54 45 5f 41 44 44  52 00 31 32 37 2e 30 2e  MOTE_ADDR.127.0. |
 | 00320  30 2e 31 00 52 45 4d 4f  54 45 5f 50 4f 52 54 00  0.1.REMOTE_PORT. |
 | 00330  33 35 38 36 32 00 50 41  53 53 45 4e 47 45 52 5f  35862.PASSENGER_ |
 | 00340  43 4f 4e 4e 45 43 54 5f  50 41 53 53 57 4f 52 44  CONNECT_PASSWORD |
 | 00350  00 65 64 4b 78 31 33 50  66 4d 73 79 41 6a 62 4e  .edKx13PfMsyAjbN |
 | 00360  4b 00 48 54 54 50 5f 55  53 45 52 5f 41 47 45 4e  K.HTTP_USER_AGEN |
 | 00370  54 00 6c 69 62 77 77 77  2d 70 65 72 6c 2f 36 2e  T.libwww-perl/6. |
 | 00380  30 35 00 48 54 54 50 5f  54 45 00 64 65 66 6c 61  05.HTTP_TE.defla |
 | 00390  74 65 2c 67 7a 69 70 3b  71 3d 30 2e 33 00 48 54  te,gzip;q=0.3.HT |
 | 003a0  54 50 5f 48 4f 53 54 00  74 68 6f 72 6e 2d 30 33  TP_HOST.thorn-03 |
 | 003b0  2e 6f 70 65 6e 73 74 72  65 65 74 6d 61 70 2e 6f  .openstreetmap.o |
 | 003c0  72 67 00 53 43 52 49 50  54 5f 55 52 4c 00 2f 61  rg.SCRIPT_URL./a |
 | 003d0  70 69 2f 30 2e 36 2f 75  73 65 72 2f 64 65 74 61  pi/0.6/user/deta |
 | 003e0  69 6c 73 00 53 43 52 49  50 54 5f 55 52 49 00 68  ils.SCRIPT_URI.h |
 | 003f0  74 74 70 3a 2f 2f 74 68  6f 72 6e 2d 30 33 2e 6f  ttp://thorn-03.o |
 | 00400  70 65 6e 73 74 72 65 65  74 6d 61 70 2e 6f 72 67  penstreetmap.org |
 | 00410  2f 61 70 69 2f 30 2e 36  2f 75 73 65 72 2f 64 65  /api/0.6/user/de |
 | 00420  74 61 69 6c 73 00 53 45  43 52 45 54 5f 4b 45 59  tails.SECRET_KEY |
 | 00430  5f 42 41 53 45 00 2b 6b  6b 43 61 46 6d 63 68 6e  _BASE.+kkCaFmchn |
...
 | 004a0  4c 6e 4e 52 72 57 32 4e  6b 63 6a 7a 46 37 4d 64  LnNRrW2NkcjzF7Md |
 | 004b0  39 70 6c 44 45 79 00                              9plDEy.          |

and then here is a failing one:

[pid  2201] read(11, "REQUEST_URI\0/api/0.6/user/detail"..., 1164) = 1164
 | 00000  52 45 51 55 45 53 54 5f  55 52 49 00 2f 61 70 69  REQUEST_URI./api |
 | 00010  2f 30 2e 36 2f 75 73 65  72 2f 64 65 74 61 69 6c  /0.6/user/detail |
 | 00020  73 3f 6f 61 75 74 68 5f  63 6f 6e 73 75 6d 65 72  s?oauth_consumer |
...
 | 00110  50 4b 66 39 26 6f 61 75  74 68 5f 76 65 72 73 69  PKf9&oauth_versi |
 | 00120  6f 6e 3d 31 2e 30 00 50  41 54 48 5f 49 4e 46 4f  on=1.0.PATH_INFO |
 | 00130  00 2f 61 70 69 2f 30 2e  36 2f 75 73 65 72 2f 64  ./api/0.6/user/d |
 | 00140  65 74 61 69 6c 73 00 53  43 52 49 50 54 5f 4e 41  etails.SCRIPT_NA |
 | 00150  4d 45 00 00 51 55 45 52  59 5f 53 54 52 49 4e 47  ME..QUERY_STRING |
 | 00160  00 6f 61 75 74 68 5f 63  6f 6e 73 75 6d 65 72 5f  .oauth_consumer_ |
...
 | 00250  4b 66 39 26 6f 61 75 74  68 5f 76 65 72 73 69 6f  Kf9&oauth_versio |
 | 00260  6e 3d 31 2e 30 00 52 45  51 55 45 53 54 5f 4d 45  n=1.0.REQUEST_ME |
 | 00270  54 48 4f 44 00 47 45 54  00 53 45 52 56 45 52 5f  THOD.GET.SERVER_ |
 | 00280  4e 41 4d 45 00 74 68 6f  72 6e 2d 30 33 2e 6f 70  NAME.thorn-03.op |
 | 00290  65 6e 73 74 72 65 65 74  6d 61 70 2e 6f 72 67 00  enstreetmap.org. |
 | 002a0  53 45 52 56 45 52 5f 50  4f 52 54 00 38 30 00 53  SERVER_PORT.80.S |
 | 002b0  45 52 56 45 52 5f 53 4f  46 54 57 41 52 45 00 41  ERVER_SOFTWARE.A |
 | 002c0  70 61 63 68 65 2f 32 2e  34 2e 37 20 28 55 62 75  pache/2.4.7 (Ubu |
 | 002d0  6e 74 75 29 20 50 68 75  73 69 6f 6e 5f 50 61 73  ntu) Phusion_Pas |
 | 002e0  73 65 6e 67 65 72 2f 35  2e 30 2e 36 00 53 45 52  senger/5.0.6.SER |
 | 002f0  56 45 52 5f 50 52 4f 54  4f 43 4f 4c 00 48 54 54  VER_PROTOCOL.HTT |
 | 00300  50 2f 31 2e 31 00 52 45  4d 4f 54 45 5f 41 44 44  P/1.1.REMOTE_ADD |
 | 00310  52 00 31 32 37 2e 30 2e  30 2e 31 00 52 45 4d 4f  R.127.0.0.1.REMO |
 | 00320  54 45 5f 50 4f 52 54 00  33 35 38 36 33 00 50 41  TE_PORT.35863.PA |
 | 00330  53 53 45 4e 47 45 52 5f  43 4f 4e 4e 45 43 54 5f  SSENGER_CONNECT_ |
 | 00340  50 41 53 53 57 4f 52 44  00 65 64 4b 78 31 33 50  PASSWORD.edKx13P |
 | 00350  66 4d 73 79 41 6a 62 4e  4b 00 48 54 54 50 5f 55  fMsyAjbNK.HTTP_U |
 | 00360  53 45 52 5f 41 47 45 4e  54 00 6c 69 62 77 77 77  SER_AGENT.libwww |
 | 00370  2d 70 65 72 6c 2f 36 2e  30 35 00 48 54 54 50 5f  -perl/6.05.HTTP_ |
 | 00380  54 45 00 64 65 66 6c 61  74 65 2c 67 7a 69 70 3b  TE.deflate,gzip; |
 | 00390  71 3d 30 2e 33 00 48 54  54 50 5f 48 4f 53 54 00  q=0.3.HTTP_HOST. |
 | 003a0  74 68 6f 72 6e 2d 30 33  2e 6f 70 65 6e 73 74 72  thorn-03.openstr |
 | 003b0  65 65 74 6d 61 70 2e 6f  72 67 00 53 43 52 49 50  eetmap.org.SCRIP |
 | 003c0  54 5f 55 52 4c 00 2f 00  53 43 52 49 50 54 5f 55  T_URL./.SCRIPT_U |
 | 003d0  52 49 00 68 74 74 70 73  3a 2f 2f 77 77 77 2e 6f  RI.https://www.o |
 | 003e0  70 65 6e 73 74 72 65 65  74 6d 61 70 2e 6f 72 67  penstreetmap.org |
 | 003f0  2f 00 53 45 43 52 45 54  5f 4b 45 59 5f 42 41 53  /.SECRET_KEY_BAS |
...
 | 00480  45 79 00 48 54 54 50 53  00 6f 6e 00              Ey.HTTPS.on.     |

I've elided some bits that have secret keys and things but the key thing is that the second has HTTPS=on and also SCRIPT_URI=https://www.openstreetmap.org when the request was actually sent to http://thorn-03.openstreetmap.org. The SERVER_NAME and SERVER_PORT values are correct in both cases.

Note that these were sent locally, avoiding the proxy server that normally sits in front of the server.

tomhughes commented 9 years ago

Think I understand it... The problem is config->envvarsCache in ext/apache2/Hooks.cpp which is sticky and if the first request processed by a given apache process/thread (not sure of the exact scope) is https then apache has HTTPS set in the request environment and that then gets baked into passenger's cache. Same for SCRIPT_URI etc.

tomhughes commented 9 years ago

Argh, so I was looking at an apparently old version of master last night, but now I look at 5.0.6 I see that cache is gone....

tomhughes commented 9 years ago

New theory, is that poolOptionsCache in InitRequest.cpp is responsible, because it attaches the PASSENGER_ENV_VARS value to the options, but is then cached so future requests can get the wrong value.

I have debug logs showing that the base64 received by HttpServer.h does not match what is on the request object when SendRequest.cpp comes to decode it.

OnixGH commented 9 years ago

Thanks for the info/debugging effort so far! We indeed removed the SetEnv cache in 5.0.6 to solve https://github.com/phusion/passenger/issues/1446. We'll look into your theory!

FooBarWidget commented 9 years ago

It might be the case that poolOptionsCache is indeed caching the HTTPS variable. The Apache module is written with the assumption that Apache doesn't set the HTTPS variable. The Apache module passes the https flag to the PassengerAgent server through the !~FLAGS header, which isn't cached. But if Apache passes the HTTPS variable then it may indeed cause the issue you describe.

I'll investigate this and report back.

I think this may also cause the problems jrochkind is still experiencing in issue #1446.

tomhughes commented 9 years ago

Well http://httpd.apache.org/docs/2.4/mod/mod_ssl.html implies that HTTPS is only set if you turn on the StdEnvVars option but in fact I think that one is always set.

There are plenty of other ways to get environment variables that vary per-request anyway, and as you say #1446 looks likely to be the same issue.

jrochkind commented 9 years ago

I'd echo @tomhuges that there are all sorts of ways to get environment variables that vary per-request, including arbitrary ones. It's a fairly standard apache technique. http://httpd.apache.org/docs/2.2/env.html#setting

I know I have used mod_rewrite with a [E] flag before in my apache conf in order to set env per-request, although I forget why.

A design that assumes otherwise is, in my opinion, problematic. Or assumes certain env variables that can be cached across requests and others that can't, just seems like asking for weird edge case bugs -- when they end up appearing 'non-deterministic' and different from identical request-to-request because of Passenger caching, it's especially frustrating. I have always appreciated how everything pretty much just works as expected with Passenger with few surprises, I've never had to fight with it before.

FooBarWidget commented 9 years ago

The problem has been confirmed, and has been fixed in commit a0843dac9. This should also fix jrochkind's problem.

@tomhughes could you test it? Can you git clone, checkout the stable-5.0 branch and run ./bin/passenger-install-apache2-module?

tomhughes commented 9 years ago

Yes (once I remember it was the agent I needed to update ;-)) that does seem to fix it.

jrochkind commented 9 years ago

a0843da does not reproduce my problem either (that is, it works) -- but then, 5.0.6 when built from git cloned source didn't seem to reproduce either, while 5.0.6 installed as a gem did, for reasons that remain mysterious. I'll test again when there's another release! Thanks @FooBarWidget !

jrochkind commented 9 years ago

I'm afraid the problem is reproducing for me again in an apache passenger built from source checked out a0843da .

It's tricky because some requests it works, some requests it doesn't. Sometimes it appears to be working for a while, then stops working again.

It's also tricky in that I haven't been able to reproduce with a simple echo-the-env rack app, at all. Only with my actual Rails app for some reason. (Could the very short response time of the demo rack app, compared to a longer response time from my real rails app, make a difference in ease of reproduction? wind up in different passenger processes more easily? Hmm, maybe I'll try setting MinInstances to a high value and see if that helps me reproduce with the rack demo app)

I'll keep working on making an actual reproduction case where I've succeeded in reproducing the problem that doesn't require you installing my gigantic not-open-source app. Not sure what else to do.

Not sure if it makes a difference that I: 1) deploy multiple apps at different SubURIs, 2) deploy my apps via <location> blocks at top-level apache config, not inside a <virtualhost> block (confirmed with apache docs that it should be fine to do this, and such <location> blocks should then apply to any <virtualhost>. And it does work fine normally. No idea if it's involved in the problem here, I mention it just in case).

Any suggestions for what the heck i should be doing here would be welcome. I've spent a couple days on this, really, mostly going in directions that ended up being red herrings unrelated to the problem. I've resigned myself to upgrading to the latest 4.x on my production servers instead of going all the way to 5.x right now, but I would like to be using 5.x eventually of course.

tomhughes commented 9 years ago

@jrochkind Did you make sure to copy PassengerAgent to /usr/lib/passenger/support-binaries/PassengerAgent or did you just change Apache to load the module from your checkout? You need to make sure it is using the agent you have built from source and not any precompiled one or you won't have the fix.

jrochkind commented 9 years ago

Hm, I ran ./bin/passenger-install-apache2-module from the checkout, and then copied the apache config it reported after running that. Which was a LoadModule pointing inside my checkout to ./buildout/apache2/mod_passenger.so. Is mod_passenger.so the "PassengerAgent"? Is there something else I need to copy somewhere or set up somewhere?

If I need to do more, please provide or point me to instructions for what? This machine does not have a /usr/lib/passenger and never has. I didn't know I needed to do more than passenger-install-apache2-module and then copying the config at the end of the install process, same as I always do to install passenger, except this time running passenger-install-apache2-module from the source ./bin instead of from the gem install as I ordinarily do?

tomhughes commented 9 years ago

This fix is in the agent, which is run as a separate process, so just changing the path to the apache module will not pick it up.

I'm not sure how it finds the agent program - my setup is normally using the precompiled Ubuntu binaries so the location I gave is the one they use. If you normally install from source then that may be different.

Best thing to do is probably to look for a "PassengerAgent Server" process and get it's PID and then do ls -l /proc/PID/exe to find out the location of the agent program. Then replace that with buildout/support-binaries/PassengerAgent from your checkout.

FooBarWidget commented 9 years ago

If jrochkind ran ./bin/passenger-install-apache2-module and copy-pasted the snippet, it should work. As far as I know, with this last commit, we've gotten rid of all the variable caching that would interfere with things. I've even tested with two different virtual hosts and confirmed that variables aren't being cached. Usage of location blocks shouldn't matter.

@jrochkind Sorry for the trouble so far. We're doing everything we can to fix your issue. One other thing I can come up with right now, is turbocaching. What happens if you disable it with PassengerTurbocaching off?

FooBarWidget commented 9 years ago

@jrochkind I could help you directly over Skype if you wish. That's probably faster. Could you add me? My Skype name is 'hongli'.

jrochkind commented 9 years ago

Thanks @FooBarWidget ! Believe it or not, I'm not sure I have a skype account, but I can make one if I don't. But i'm not sure I have time for a synchronous meeting today. I think maybe I can profitably attempt further to make a reproducible test case with a demo app (demo single-file rails app this time instead of rack). I will try with turbocaching off.

Two questions though, that I would appreciate your insight on:

1) I have been switching back and forth between passenger versions simply by commenting/uncommenting LoadModule lines, for already compiled different versions of mod_passenger.so. Should this be sufficient, or am I making a mistake, do I actually have to re-run passenger-install-apache2-module each time I want to switch to a different version of passenger?

2) I realized I accidentally switched back to an 'older' recommended passenger apache setup for SubURI's, using the symlink in webroot instead of passenger Alias. I don't think that should make any difference? But I meant to be using the new one, but forgot to update one of my automation scripts, which re-created the old one. So I'll fix that, but there isn't any reason it ought to matter, is there?

FooBarWidget commented 9 years ago

@jrochkind I'm also on Google Talk. honglilai@gmail.com

1) No, that's not sufficient. You need to change LoadModule and PassengerRoot. LoadModule tells Apache which Passenger Apache module to load, while PassengerRoot tells the Passenger Apache module where to find the PassengerAgent executable. This last fix is inside the PassengerAgent executable, so PassengerRoot must point to the right place.

2) Should not make a difference.

OnixGH commented 9 years ago

@jrochkind just in case you were wondering, what @FooBarWidget is saying under 1) is basically the same as the instructions the passenger-install-apache2-module prints to complete the install (at the end).

Not having the PassengerAgent fixes might explain the weird difference you've been seeing between the commit checkouts and installs?

jrochkind commented 9 years ago

Thanks! Oops, yes, that was my omission, I am indeed changing both LoadModule and PassengerRoot. I'm just not recompiling anything, changing both of those to point to different already compiled passengers.

But I will double check, and also double-check with ps that my running PassengerAgent is as expected.

jrochkind commented 9 years ago

Okay, so, updating my apache conf to current documented best practice for passenger SubURI's seems to have resolved the problem, using a built-from-source at a0843da. So, hooray?

I think that's why it appeared solved to me at first, then broke again -- it appeared broke again after I accidentally reset the apache conf to the older format using an automated deploy system I forgot hadn't been updated to the new conf format.

It's possible my old conf was actually not quite right, although it caused no problems I could notice except for this one. The old conf was based on documentation from a much older Passenger version, and involved a symlink from sub_uri_path in docroot to /path/to/app/public, and then:

RackBaseURI /sub_uri_path
<Directory /path/to/app>  # this maybe should have been to /path/to/app/public but wasn't?
    RailsEnv demo 
    Options -MultiViews
</Directory>
<Location /sub_uri_path/shibboleth_protected_path>
    AuthType shibboleth
    ShibRequestSetting requireSession 1
    Require valid-user
</Location>

Note the old style method involved no explicit PassengerAppName, so it's possible multiple apps at different SubURI's were sharing the same PassengerAppName -- which you wouldn't think would matter, if the request ENV truly was not being cached. So I don't know. The old conf did work, with no noticable problems except this one related to apache env which it apparently triggered.

At any rate, I have updated to apache conf currently suggested, and as far as I can tell everything is working fine.

Using the built from source at a0843da passenger. Since last time I mysteriously seemed to have different results between gem-installed and built-from-source, I will wait for next official release and test again.

Crossing my fingers that it is indeed working reliably now, and won't start acting up again! Going to do some more testing.

jrochkind commented 9 years ago

Crap, it's back again, although in only one of my two SubURI-deployed apps. I dunno.

jrochkind commented 9 years ago

Nevermind! that was my user error! All is well!

By the way though, you suggested PassengerTurbocaching off before, when I was looking into that I noticed it isn't mentioned in the apache passenger guide, maybe it should be? I also noticed PassengerTurbocaching can only appear at top-level config, when I tried to put it in a <Location> block, it wasn't allowed there. Not sure if that's intentional.

Anyway. Okay. Thanks for Passenger, I have been using it for years and love it.

If it really was my bad config that caused the problem -- it is kind of undesirable that it failed so subtly, it appeared to work, but failed for apache env related stuff one might not notice at first. If that old style of SubURI config is no longer meant to be supported, it would be preferable if it failed fast on apache startup or first app access with a message "Dude, use the new way of setting this up."

FooBarWidget commented 9 years ago

The turbocaching option is currently documented in the Server Optimization Guide, but yes it deserves an entry in the config reference too. It's indeed top-level.

The old style config should still work. I'm surprised to hear that it caused you problems. This is worth investigating, but maybe we should open an entirely new ticket for this.

In the end, was it only changing to the new-style sub-URI config that fixed things? Did turning turbocache on/off affect it at all?

jrochkind commented 9 years ago

With the new style config, it was not neccesary to turn off turbocache. I think with the old-style it did not help to turn it off, but I'm not certain, it got confusing to keep track of all the variables, and when I found something that seemed to work I stuck with it. I can try to find time to keep experimenting/reproducing under alternate configurations.

FooBarWidget commented 9 years ago

@jrochkind Do you have Google Talk, or any other kind of instant messaging program? I would really like to help you with this and it would be much faster if I can talk to you over chat.

jrochkind commented 9 years ago

I am on freenode IRC as jrochkind. Also I do have a skype account although I rarely use it, I've updated my Skype software and logged into my skype account also jrochkind, although I don't know how to use Skype well.

At the moment, everything seems to be working for me, I am not actually in need of help (crosses fingers), but I would like to help you understand what's going on for the good of passenger, but I can only spare so much more time, have to actually get some work done after a couple days on this.