theforeman / forklift

Helpful deployment scripts for Foreman and Katello
https://theforeman.github.io/forklift
GNU General Public License v3.0
183 stars 199 forks source link

Problems with unattended, HTTP & localhost:3000 redirection #628

Open dLobatog opened 6 years ago

dLobatog commented 6 years ago

I am only able to fetch kickstarts through HTTPs in Forklift. This happened recently, at cfgmgmtcamp (February 7th) I was able to fetch kickstarts through HTTP.

I inspected the request through Rack, and I think I found the reason:

HTTP from inside (doesn't go through the proxy, querying localhost:3000):

pry(#<Rack::URLMap>)> puts path, script_name, http_host, server_name, server_port, is_same_server
/unattended/provision

****localhost:3000****
****localhost****
3000
true

HTTP from outside (goes through the proxy, querying centos7-devel.lobatolan.home):

gets logged as

::1 - - [25/Feb/2018:18:21:52 UTC] "GET /unattended/provision?token=f53cdcab-0eeb-490c-b31b-0c34a HTTP/1.1" 404 32
- -> /unattended/provision?token=f53cdcab-0eeb-490c-b31b-0c34a
pry(#<Rack::URLMap>)>  puts path, script_name, http_host, server_name, server_port, is_same_server
/unattended/provision

****localhost:3000****
****centos7-devel.lobatolan.home****
80
false

Basically, in HTTP, Rack is unaware that localhost:3000 and centos7-devel.lobatolan.home are the same, therefore it decides to drop the request with 404. To fix this, a possible solution is to make puppet-katello_devel force the SERVER_NAME header is changed on every request to localhost.

Locally, I'm able to get around it by creating a Rack::Middleware that sets SERVER_NAME to localhost and SERVER_PORT to 3000. Any ideas?

cc @ehelms @ekohl

dLobatog commented 6 years ago

The patch I'm using looks like this:

────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
modified: lib/foreman/telemetry_rack.rb
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
@ telemetry_rack.rb:8 @ module Foreman
    end

    def call(env)
      if env["HTTP_X_FORWARDED_PROTO"] != "https"
        env['SERVER_NAME'] = 'localhost'
        env['SERVER_PORT'] = '3000'
      end
      Thread.current[:foreman_telemetry_gcstats] = GC.stat if Foreman::Telemetry.instance.enabled?
      @app.call(env)
    end
lzap commented 6 years ago

So if I understand correctly, Rack use these variables to be able to serve virtual host requests.

Are you using webrick or something else? Is your server bound to 0.0.0.0 or particular interface only?

ekohl commented 6 years ago

@dLobatog it sounds like we need some reverse proxy middleware. The app/controllers/unattended_controller.rb has some manual code but a proper implementation would use X-Forwarded-Host and X-Forwarded-Port too I guess.

san7ket commented 6 years ago

@dLobatog Looks like I am hitting the same issue, but I cannot seem to find the "lib/foreman/telemetry_rack.rb" to try the patch Am I missing something? This is standard devel setup "centos7-devel" box

ls /home/vagrant/foreman/lib/foreman/
cast.rb        dynflow     exception.rb             gettext     http_proxy.rb  model.rb      renderer.rb         telemetry.rb     util.rb
controller.rb  dynflow.rb  foreman_url_renderer.rb  http_proxy  logging.rb     provision.rb  silenced_logger.rb  telemetry_sinks
ekohl commented 6 years ago

@san7ket it was moved in https://github.com/theforeman/foreman/commit/4093de10f0f54c8c943e7df8c173f7fb66e2ef0e

@dLobatog it appears the correct rails way is http://api.rubyonrails.org/classes/ActionDispatch/RemoteIp.html but I don't think it sets the SERVER_NAME and SERVER_PORT. We should use that in favor of the custom code in https://github.com/theforeman/foreman/blob/009e7bbd29f31c3750a8be47b4aecf5278c0db95/app/controllers/unattended_controller.rb#L232-L235 which looks unsafe. Perhaps we need an issue for proper reverse proxy support? It'd open the path to a standalone passenger which would open up the path to using websockets.

cc @timogoebel

timogoebel commented 6 years ago

@ekohl: Sounds good. I think the rails way is the proper choice. We just have to make sure we don't introduce a security vulnerability. Another thing to consider regarding reverse proxies is handling of ssl client certs. Both Foreman core and katello contain custom code to handle that. Maybe we can streamline this in the process.