From 150858a17ab8e76f15a13f799f9c3de576b9b7f0 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Sat, 8 Sep 2018 09:42:40 +1000 Subject: [PATCH] feat(webhook whitelist): allow hosts to be whitelisted using * domains Closes: https://github.com/pact-foundation/pact_broker/issues/223 --- lib/pact_broker/doc/views/webhooks.markdown | 6 ++- .../webhooks/check_host_whitelist.rb | 34 +++++++++++++---- .../webhooks/check_host_whitelist_spec.rb | 38 +++++++++++++++++++ 3 files changed, 69 insertions(+), 9 deletions(-) diff --git a/lib/pact_broker/doc/views/webhooks.markdown b/lib/pact_broker/doc/views/webhooks.markdown index 35f0667e3..4fd9a0272 100644 --- a/lib/pact_broker/doc/views/webhooks.markdown +++ b/lib/pact_broker/doc/views/webhooks.markdown @@ -123,7 +123,7 @@ Pact Broker Github repository. * **Host**: If the `webhook_host_whitelist` contains any entries, the host must match one or more of the entries. By default, it is empty. For security purposes, if the host whitelist is empty, the response details will not be logged to the UI (though they can be seen in the application logs at debug level). - The host whitelist may contain hostnames (eg `"github.com"`), IPs (eg `"192.0.345.4"`), network ranges (eg `"10.0.0.0/8"`) or regular expressions (eg `/.*\.foo\.com$/`). Note that IPs are not resolved, so if you specify an IP range, you need to use the IP in the webhook URL. If you wish to allow webhooks to any host (not recommended!), you can set `webhook_host_whitelist` to `[/.*/]`. Beware of any sensitive endpoints that may be exposed within the same network. + The host whitelist may contain hostnames (eg `"github.com"`), domains beginning with `*` (eg. `"*.foo.com"`), IPs (eg `"192.0.345.4"`), network ranges (eg `"10.0.0.0/8"`) or regular expressions (eg `/.*\.foo\.com$/`). Note that IPs are not resolved, so if you specify an IP range, you need to use the IP in the webhook URL. If you wish to allow webhooks to any host (not recommended!), you can set `webhook_host_whitelist` to `[/.*/]`. Beware of any sensitive endpoints that may be exposed within the same network. The recommended set of values to start with are: @@ -131,7 +131,9 @@ Pact Broker Github repository. * your company chat (eg. Slack, for publishing notifications) * your code repository (eg. Github, for sending commit statuses) - Alternatively, you could use a regular expression to limit requests to your company's domain. eg `/.*\.foo\.com$/` (don't forget the end of string anchor). You can test Ruby regular expressions at [rubular.com](http://rubular.com). + Alternatively, you could use a domain beginning with a `*` to limit requests to your company's domain. + + Note that the hostname/domain matching follows that used for SSL certificate hostnames, so `*.foo.com` will match `a.foo.com` but not `a.b.foo.com`. If you need more flexible matching because you have domains with variable "parts" (eg `a.b.foo.com`), you can use a regular expression (eg `/.*\.foo\.com$/` - don't forget the end of string anchor). You can test Ruby regular expressions at [rubular.com](http://rubular.com). ### Testing diff --git a/lib/pact_broker/webhooks/check_host_whitelist.rb b/lib/pact_broker/webhooks/check_host_whitelist.rb index 1ac0e522c..5f2d19b8a 100644 --- a/lib/pact_broker/webhooks/check_host_whitelist.rb +++ b/lib/pact_broker/webhooks/check_host_whitelist.rb @@ -1,3 +1,5 @@ +require 'openssl' + module PactBroker module Webhooks class CheckHostWhitelist @@ -7,16 +9,34 @@ def self.call(host, whitelist = PactBroker.configuration.webhook_host_whitelist) end def self.match?(host, whitelist_host) - if whitelist_host.is_a?(Regexp) - host =~ whitelist_host + if parse_ip_address(host) + ip_address_matches_range(host, whitelist_host) + elsif whitelist_host.is_a?(Regexp) + host_matches_regexp(host, whitelist_host) + elsif whitelist_host.start_with?("*") + OpenSSL::SSL.verify_hostname(host, whitelist_host) else - begin - IPAddr.new(whitelist_host) === IPAddr.new(host) - rescue IPAddr::Error - host == whitelist_host - end + host == whitelist_host end end + + def self.parse_ip_address(addr) + IPAddr.new(addr) + rescue IPAddr::Error + nil + end + + def self.ip_address_matches_range(host, maybe_whitelist_range) + parse_ip_address(maybe_whitelist_range) === parse_ip_address(host) + end + + def self.host_matches_regexp(host, whitelist_regexp) + host =~ whitelist_regexp + end + + def self.host_matches_domain_with_wildcard(host, whitelist_domain) + OpenSSL::SSL.verify_hostname(host, whitelist_domain) + end end end end diff --git a/spec/lib/pact_broker/webhooks/check_host_whitelist_spec.rb b/spec/lib/pact_broker/webhooks/check_host_whitelist_spec.rb index edd83c411..99edd51a9 100644 --- a/spec/lib/pact_broker/webhooks/check_host_whitelist_spec.rb +++ b/spec/lib/pact_broker/webhooks/check_host_whitelist_spec.rb @@ -23,6 +23,44 @@ module Webhooks end end + context "when the whitelist includes *.foo.bar" do + let(:whitelist) { ["*.foo.bar"] } + + it "matches host a.foo.bar" do + expect(CheckHostWhitelist.call("a.foo.bar", whitelist)).to eq whitelist + end + + it "does not matche host a.b.foo.bar" do + expect(CheckHostWhitelist.call("a.b.foo.bar", whitelist)).to eq [] + end + + it "does not match a.foo.bar.b" do + expect(CheckHostWhitelist.call("a.foo.bar.b", whitelist)).to eq [] + end + + it "does not match foo.bar" do + expect(CheckHostWhitelist.call("foo.bar", whitelist)).to eq [] + end + + it "does not match 10.0.0.2" do + expect(CheckHostWhitelist.call("10.0.0.2", whitelist)).to eq [] + end + end + + context "when the whitelist includes *.2" do + it "does not match 10.0.0.2 as that's the wrong way to declare an IP range" do + expect(CheckHostWhitelist.call("10.0.0.2", ["*.0.0.2"])).to eq [] + end + end + + context "when the whitelist includes *.foo.*.bar" do + let(:whitelist) { ["*.foo.*.bar"] } + + it "does not match host a.foo.b.bar, according to RFC 6125, section 6.4.3, subitem 1" do + expect(CheckHostWhitelist.call("a.foo.b.bar", whitelist)).to eq [] + end + end + context "when the host is localhost" do let(:host) { "localhost" }