Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Respect X-Forwarded-Proto header #87

Closed
wants to merge 4 commits into from
Closed

Respect X-Forwarded-Proto header #87

wants to merge 4 commits into from

Conversation

jellybob
Copy link

@jellybob jellybob commented Nov 12, 2019 •
edited
Loading

This is an attempt to resolve #86

@coveralls
Copy link

coveralls commented Nov 12, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 82.627% when pulling 9f1501b on neos-uk:no-forwarded-proto-overwrite into 52e57c0 on socketry:master.

@jellybob jellybob force-pushed the no-forwarded-proto-overwrite branch from cc0b873 to ad0c43e Compare November 12, 2019 16:11
@jellybob jellybob changed the title No forwarded proto overwrite Respect X-Forwarded-Proto header Nov 12, 2019
@jellybob
Copy link
Author

All three of these commits actually have the same effect (I was thrown off by #88), so take your pick which one you'd like and I'll rebase things.

@jellybob jellybob marked this pull request as ready for review November 12, 2019 20:13
ioquatix
ioquatix reviewed Nov 13, 2019
View reviewed changes
lib/falcon/adapters/rack.rb
@@ -105,6 +105,7 @@ def unwrap_request(request, env)
# https://tools.ietf.org/html/rfc7239#section-5.4
# https://github.com/rack/rack/issues/1310
env[HTTP_X_FORWARDED_PROTO] ||= request.scheme
env[RACK_URL_SCHEME] = env[HTTP_X_FORWARDED_PROTO]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we set RACK_URL_SCHEME anywhere else or did I just completely miss it?

Also, is this checked by rack linter?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RACK_URL_SCHEME defaults to request.scheme, however that's not accurate in cases where we're listening on HTTP but have SSL terminated upstream.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user sent the X-Forwarded-Proto: https could that break the application that's running on http? Just wondering if there are security implications to this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, here's how puma sets RACK_URL_SCHEME:

env[RACK_URL_SCHEME] = default_server_port(env) == PORT_443 ? HTTPS : HTTP

Here's default_server_port:

def default_server_port(env)
  if ['on', HTTPS].include?(env[HTTPS_KEY]) || env[HTTP_X_FORWARDED_PROTO].to_s[0...5] == HTTPS || env[HTTP_X_FORWARDED_SCHEME] == HTTPS || env[HTTP_X_FORWARDED_SSL] == "on"
    PORT_443
  else
    PORT_80
  end
end

In theory if the application was exposed directly, you could bypass middleware to force ssl by sending X-Forwarded-Proto: https. Whether or not that has security implications I don't know, since it could only affect the current user's session.

I'm not certain on this, but I would think it's up to nginx, haproxy, or whatever is used in front of the application to set or reject these headers. Here's how Heroku handles it:

Copy link

@sfnelson sfnelson Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ioquatix the purpose of the scheme check is to protect the user from sending information over unencrypted connections. If the user provides the X-Forwarded-Proto: https it will only impact them. I don't think this is a security concern.

@ioquatix
Copy link
Member

Thanks @bryanp - X-Forwarded-Proto is also not standardised... there is another header which is specified by the RFC... I need to review it.

The security issues are less of a concern, but I do want to hear more about it if anyone has any other concerns. Using the user facing headers... should probably be something explicit in the app... as it stands, the protocol reported by falcon is the last hop... but in this case we want the first hop...

@jellybob
Copy link
Author

Typically this header will get explicitly set by the server doing SSL termination. I've tested this on Heroku and even if I send X-Forwarded-Proto: https on an HTTP request by the time it gets to my application server its been rewritten to http by their load balancer. In other places I've injected the header in nginx with similar behaviour.

@ioquatix
Copy link
Member

ioquatix commented Jan 8, 2020

Sorry, I have not come back to this and it's been sitting for a while.

There are a few options:

  • This PR, which is great.
  • Prefer Forwarded header.
  • Something more elaborate like Puma.

There is one other issue.

The HTTP/2 pseudo header :scheme should probably be preferred which comes via request.scheme.

I don't really have a strong feeling on this right now, except that what we are doing is already the simplest and most straight forward approach.

Should RACK_URL_SCHEME reflect the pseudo header? Or should we allow other headers to override it?

What's stopping applications from parsing out the X-Forwarded-Proto if that's what they want? Should we do this by default?

I'm all for making those headers easier to work with, and I don't even mind if we should use a more elaborate method for setting the default RACK_URL_SCHEME (e.g. this PR). I'm also wary of imposing more complex logic into the field, which then becomes a bit of "technical debt" going forward, and will only get more complex when we need to deal with Forwarded: (multi-hop) as well as X-Forwarded-Proto.

Thoughts?

@ioquatix
Copy link
Member

ioquatix commented Jan 8, 2020 •
edited
Loading

Bearing in mind, that if the client connection is HTTP, but the internal connection is HTTPS, that's also confusing.

What is RACK_URL_SCHEME used for? Determining if the connection is secure from end to end? Generating URLs?

@ioquatix ioquatix force-pushed the master branch 4 times, most recently from 77ce8ce to 98a0ae6 Compare April 8, 2020 15:12
@ioquatix ioquatix force-pushed the master branch 2 times, most recently from 71effb6 to 827cf81 Compare April 18, 2020 12:06
@ioquatix ioquatix force-pushed the master branch 6 times, most recently from 8a55246 to f785c4d Compare September 19, 2020 02:31
@neos-uk neos-uk closed this by deleting the head repository Dec 9, 2024
@ioquatix
Copy link
Member

ioquatix commented Dec 10, 2024 •
edited
Loading

I discussed this with @jeremyevans and we are going to clarify rack itself: rack/rack#2266

The current behaviour of Falcon is the only safe default.

Overriding rack.url_scheme with values from the forwarded header is unsafe as this is user provided and cannot be trusted.

In principle, you can use Rack::Request.new(env).ssl? to check whether the request was secure or not. If you want to have an http -> https redirect at the application level, that would be the best indicator of a request using the http scheme.

However, in Falcon, if you use falcon virtual, this redirect is handled for you automatically, because the server that binds to port 80 does not serve application traffic and is only configured to redirect to https.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfnelson sfnelson sfnelson left review comments

@ioquatix ioquatix ioquatix left review comments

@bryanp bryanp bryanp left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rack.url_scheme is being set incorrectly on Heroku
6 participants
@jellybob @coveralls @ioquatix @bryanp @sfnelson @neos-uk