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.

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

Add HostAuthorization rack-protection middleware #2053

Merged
merged 34 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
fb2d76d
Add `permitted_hosts` setting
dentarg Nov 5, 2024
b5df6e1
Split bad/good cases in `HostAuthorization` specs
dentarg Nov 8, 2024
492589f
Less brittle `HostAuthorization` specs
dentarg Nov 8, 2024
841ee31
Simplify `HostAuthorization` tests for Sinatra
dentarg Nov 8, 2024
d2293d9
Document `HostAuthorization` in rack-protection README
dentarg Nov 8, 2024
c9c2625
Expose all the options for `HostAuthorization`
dentarg Nov 8, 2024
6e3711e
Test with multiple hostnames in headers
dentarg Nov 8, 2024
1701645
Improve test descriptions
dentarg Nov 8, 2024
071358e
Port should not matter for `HostAuthorization`
dentarg Nov 8, 2024
856d8e6
No need for `Rack::Protection::HostAuthorization.forwarded?` now
dentarg Nov 8, 2024
ffaf412
Support for `IPAddr` hosts
dentarg Nov 8, 2024
44dd856
Always look at the `Host` header
dentarg Nov 12, 2024
76e614a
Better grouping of `HostAuthorization` specs
dentarg Nov 12, 2024
e6264be
Test with more exotic header values
dentarg Nov 12, 2024
d179c0c
Support for subdomains
dentarg Nov 12, 2024
4652ad2
Support `Forwarded` header in `#forwarded?`
dentarg Nov 12, 2024
a167d8b
Use the same `require` order as other protections
dentarg Nov 13, 2024
3d5383d
Better `allow_if` example in tests
dentarg Nov 13, 2024
dd73d0e
`HostAuthorization` development settings
dentarg Nov 13, 2024
8466a9e
Test with `nil` `HTTP_HOST`
dentarg Nov 13, 2024
1081bf8
Fix typo in README
dentarg Nov 14, 2024
85d2079
Mention `IPAddr` objects
dentarg Nov 14, 2024
801e821
Add (optional) debug logging in `HostAuthorization`
dentarg Nov 14, 2024
579a2e6
Fix incorrect test description
dentarg Nov 15, 2024
affc961
Use keyword argument for `mock_app` helper
dentarg Nov 15, 2024
543bac2
Better test description
dentarg Nov 15, 2024
b1bf256
Fix typo in test description
dentarg Nov 15, 2024
9f0d130
Reject invalid hostnames
dentarg Nov 17, 2024
5947a6d
Document `host_authorization` defaults
dentarg Nov 17, 2024
b29d698
Lint `HostAuthorization` middleware
dentarg Nov 18, 2024
5ba1250
Lint `HostAuthorization` specs
dentarg Nov 18, 2024
3059727
Avoid `warning: character class has '-' without escape: ...`
dentarg Nov 18, 2024
4dfd160
Silence `Style/SafeNavigationChainLength` cop
dentarg Nov 18, 2024
7697337
No need to silence `SafeNavigationChainLength` cop
dentarg Nov 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Support for subdomains
  • Loading branch information
dentarg committed Nov 12, 2024
commit d179c0c2b948ae7f08fcddf024234e9eb6432180
11 changes: 9 additions & 2 deletions rack-protection/lib/rack/protection/host_authorization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ def initialize(*)
@permitted_hosts = @all_permitted_hosts
.select { |host| host.is_a?(String) }
.map(&:downcase)
@domain_hosts = @permitted_hosts.select { |host| host[0] == "." }
@ip_hosts = @all_permitted_hosts.select { |host| host.is_a?(IPAddr) }
end

Expand Down Expand Up @@ -60,9 +61,15 @@ def extract_host(authority)
end

def host_permitted?(host)
return true if @permitted_hosts.include?(host)
exact_match?(host) || domain_match?(host) || ip_match?(host)
end

def exact_match?(host)
@permitted_hosts.include?(host)
end

ip_match?(host)
def domain_match?(host)
@domain_hosts.any? { |domain_host| host.end_with?(domain_host) }
end

def ip_match?(host)
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@dentarg dentarg Nov 15, 2024

Choose a reason for hiding this comment

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

I will just copy the tests here for easier discussion (no need to open the link every time you visit this convo)

https://github.com/rails/rails/blob/6230bd334ab47f9efc6d97c6627abe76c80d8058/actionpack/test/dispatch/host_authorization_test.rb#L108

  test "does not allow domain name notation in the HOST header itself" do
    @app = build_app(".example.com")

    get "/", env: {
      "HOST" => ".example.com",
      "action_dispatch.show_detailed_exceptions" => true
    }

    assert_response :forbidden
    assert_match "Blocked hosts: .example.com", response.body
  end

https://github.com/rails/rails/blob/6230bd334ab47f9efc6d97c6627abe76c80d8058/actionpack/test/dispatch/host_authorization_test.rb#L449

  test "blocks requests with invalid hostnames" do
    @app = build_app(".example.com")

    get "/", env: {
      "HOST" => "attacker.com#x.example.com",
      "action_dispatch.show_detailed_exceptions" => true
    }

    assert_response :forbidden
    assert_match "Blocked hosts: attacker.com#x.example.com", response.body
  end

Copy link
Member Author

Choose a reason for hiding this comment

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

Both of these are not blocked currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder of they can be taken advantage of 🤔 They are not valid hostnames (Wikipedia, see "LDH rule")

$ host example.com#example.org
Host example.com#example.org not found: 3(NXDOMAIN)

$ host .example.org
host: '.example.org' is not a legal name (empty label)

Copy link
Member Author

Choose a reason for hiding this comment

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

I handled them both in 9f0d130, no longer allowed.

I think there's one difference between Sinatra and Rails now: with .example.com in the list of permitted hosts, Rails will allow bar.example.com but not foo.bar.example.com. Sinatra will allow both. This is intentional, I think it makes sense. WDYT @walro?

Copy link
Contributor

@walro walro Nov 17, 2024

Choose a reason for hiding this comment

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

Both ways seem acceptable to me. I guess for Rails one would need to add .bar.example.com in addition to .example.com to allow for foo.bar.example.com?

In the Sinatra case, to only allow foo.bar.example.com (and its siblings) but not baz.example.com, one could setup .bar.example.com?

Copy link
Member Author

@dentarg dentarg Nov 17, 2024

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

Noticed an interesting comment in rack/rack#1604

Given RFC 2181 relaxes all restrictions on DNS names (except segment and total length), it seems unnecessarily risky to use anything but a wildcard match in that position.

Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,32 @@ def assert_response(outcome:, headers:, last_response:)
# we always specify HTTP_HOST because when HTTP_HOST is not set in env,
# requests are made with env { HTTP_HOST => Rack::Test::DEFAULT_HOST }

describe "when subdomains under .test and .example.com are permitted" do
requests = lambda do
dentarg marked this conversation as resolved.
Show resolved Hide resolved
[
{ "HTTP_HOST" => "foo.test" },
{ "HTTP_HOST" => "foo.example.com" },
{ "HTTP_HOST" => "foo.test", "HTTP_X_FORWARDED_HOST" => "bar.baz.example.com" },
{ "HTTP_HOST" => "foo.test", "HTTP_X_FORWARDED_HOST" => "bar.test, baz.test" },
{ "HTTP_HOST" => "foo.test", "HTTP_FORWARDED" => %(host="baz.test") },
{ "HTTP_HOST" => "foo.test", "HTTP_FORWARDED" => %(host="baz.test" host="baz.example.com") },
]
end

requests.call.each do |headers|
it "stops the request with headers '#{headers}'" do
dentarg marked this conversation as resolved.
Show resolved Hide resolved
permitted_hosts = [".test", ".example.com"]
mock_app do
use Rack::Protection::HostAuthorization, permitted_hosts: permitted_hosts
run DummyApp
end

get("/", {}, headers)
assert_response(outcome: :allowed, headers: headers, last_response: last_response)
end
end
end

describe "requests with bogus values in headers" do
requests = lambda do |allowed_host|
[
Expand Down Expand Up @@ -177,6 +203,8 @@ def assert_response(outcome:, headers:, last_response:)
{ "HTTP_HOST" => allowed_host, "HTTP_FORWARDED" => "host=#{allowed_host}; host=#{bad_host}" },
{ "HTTP_HOST" => allowed_host, "HTTP_X_FORWARDED_HOST" => bad_host },
{ "HTTP_HOST" => allowed_host, "HTTP_FORWARDED" => "host=#{bad_host}" },
{ "HTTP_HOST" => allowed_host, "HTTP_FORWARDED" => %(host=".#{allowed_host}") },
{ "HTTP_HOST" => allowed_host, "HTTP_FORWARDED" => %(host="foo.#{allowed_host}") },
{ "HTTP_HOST" => bad_host, "HTTP_X_FORWARDED_HOST" => allowed_host },
{ "HTTP_HOST" => bad_host, "HTTP_FORWARDED" => "host=#{allowed_host}" },
]
Expand Down