diff options
author | Hordur Freyr Yngvason <hfyngvason@gitlab.com> | 2019-09-27 13:35:37 +0200 |
---|---|---|
committer | Hordur Freyr Yngvason <hfyngvason@gitlab.com> | 2019-11-21 10:09:57 -0500 |
commit | 729040717e887d33f776497eaefb8b8530dbe130 (patch) | |
tree | 5408aea2e573a7cd12b615031bb5b241952580ab | |
parent | b5ad06174bb1de39438c90847abb86ac6988e944 (diff) | |
download | gitlab-ce-729040717e887d33f776497eaefb8b8530dbe130.tar.gz |
Use Gitlab::HTTP for all chat notifications
12 files changed, 294 insertions, 181 deletions
diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb index ecea1a5b630..b84a79453c1 100644 --- a/app/models/project_services/chat_notification_service.rb +++ b/app/models/project_services/chat_notification_service.rb @@ -113,12 +113,9 @@ class ChatNotificationService < Service private + # every notifier must implement this independently def notify(message, opts) - Slack::Notifier.new(webhook, opts).ping( - message.pretext, - attachments: message.attachments, - fallback: message.fallback - ) + raise NotImplementedError end def custom_data(data) diff --git a/app/models/project_services/mattermost_service.rb b/app/models/project_services/mattermost_service.rb index b8bc83b870e..c1055db78e5 100644 --- a/app/models/project_services/mattermost_service.rb +++ b/app/models/project_services/mattermost_service.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class MattermostService < ChatNotificationService + include ::SlackService::Notifier + def title 'Mattermost notifications' end diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb index 482808255f9..7290964f442 100644 --- a/app/models/project_services/slack_service.rb +++ b/app/models/project_services/slack_service.rb @@ -30,4 +30,28 @@ class SlackService < ChatNotificationService def webhook_placeholder 'https://hooks.slack.com/services/…' end + + module Notifier + private + + def notify(message, opts) + # See https://github.com/stevenosloan/slack-notifier#custom-http-client + notifier = Slack::Notifier.new(webhook, opts.merge(http_client: HTTPClient)) + + notifier.ping( + message.pretext, + attachments: message.attachments, + fallback: message.fallback + ) + end + + class HTTPClient + def self.post(uri, params = {}) + params.delete(:http_options) # these are internal to the client and we do not want them + Gitlab::HTTP.post(uri, body: params) + end + end + end + + include Notifier end diff --git a/changelogs/unreleased/security-dns-rebind-ssrf-in-slack-notifications.yml b/changelogs/unreleased/security-dns-rebind-ssrf-in-slack-notifications.yml new file mode 100644 index 00000000000..5f9713ef844 --- /dev/null +++ b/changelogs/unreleased/security-dns-rebind-ssrf-in-slack-notifications.yml @@ -0,0 +1,5 @@ +--- +title: Limit potential for DNS rebind SSRF in chat notifications +merge_request: +author: +type: security diff --git a/config/initializers/hangouts_chat_http_override.rb b/config/initializers/hangouts_chat_http_override.rb new file mode 100644 index 00000000000..4fd886697e4 --- /dev/null +++ b/config/initializers/hangouts_chat_http_override.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module HangoutsChat + class Sender + class HTTP + module GitlabHTTPOverride + extend ::Gitlab::Utils::Override + + attr_reader :uri + + # see https://github.com/enzinia/hangouts-chat/blob/6a509f61a56e757f8f417578b393b94423831ff7/lib/hangouts_chat/http.rb + override :post + def post(payload) + httparty_response = Gitlab::HTTP.post( + uri, + body: payload.to_json, + headers: { 'Content-Type' => 'application/json' }, + parse: nil # disables automatic response parsing + ) + net_http_response = httparty_response.response + # The rest of the integration expects a Net::HTTP response + net_http_response + end + end + + prepend GitlabHTTPOverride + end + end +end diff --git a/lib/microsoft_teams/notifier.rb b/lib/microsoft_teams/notifier.rb index a7dcd322e27..340bf709f5e 100644 --- a/lib/microsoft_teams/notifier.rb +++ b/lib/microsoft_teams/notifier.rb @@ -14,7 +14,6 @@ module MicrosoftTeams response = Gitlab::HTTP.post( @webhook.to_str, headers: @header, - allow_local_requests: true, body: body(options) ) diff --git a/spec/initializers/hangouts_chat_http_override_spec.rb b/spec/initializers/hangouts_chat_http_override_spec.rb new file mode 100644 index 00000000000..0eee891799f --- /dev/null +++ b/spec/initializers/hangouts_chat_http_override_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'HangoutsChat::Sender Gitlab::HTTP override' do + describe 'HangoutsChat::Sender::HTTP#post' do + it 'calls Gitlab::HTTP.post with default protection settings' do + webhook_url = 'https://example.gitlab.com' + payload = { key: 'value' } + http = HangoutsChat::Sender::HTTP.new(webhook_url) + mock_response = double(response: 'the response') + + expect(Gitlab::HTTP).to receive(:post) + .with( + URI.parse(webhook_url), + body: payload.to_json, + headers: { 'Content-Type' => 'application/json' }, + parse: nil + ) + .and_return(mock_response) + + expect(http.post(payload)).to eq(mock_response.response) + end + + it_behaves_like 'a request using Gitlab::UrlBlocker' do + let(:http_method) { :post } + let(:url_blocked_error_class) { Gitlab::HTTP::BlockedUrlError } + + def make_request(uri) + HangoutsChat::Sender::HTTP.new(uri).post({}) + end + end + end +end diff --git a/spec/initializers/rest-client-hostname_override_spec.rb b/spec/initializers/rest-client-hostname_override_spec.rb index 90a0305c9a9..7e36656ba1c 100644 --- a/spec/initializers/rest-client-hostname_override_spec.rb +++ b/spec/initializers/rest-client-hostname_override_spec.rb @@ -3,147 +3,12 @@ require 'spec_helper' describe 'rest-client dns rebinding protection' do - include StubRequests + it_behaves_like 'a request using Gitlab::UrlBlocker' do + let(:http_method) { :get } + let(:url_blocked_error_class) { ArgumentError } - context 'when local requests are not allowed' do - it 'allows an external request with http' do - request_stub = stub_full_request('http://example.com', ip_address: '93.184.216.34') - - RestClient.get('http://example.com/') - - expect(request_stub).to have_been_requested - end - - it 'allows an external request with https' do - request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - - it 'raises error when it is a request that resolves to a local address' do - stub_full_request('https://example.com', ip_address: '172.16.0.0') - - expect { RestClient.get('https://example.com') } - .to raise_error(ArgumentError, - "URL 'https://example.com' is blocked: Requests to the local network are not allowed") - end - - it 'raises error when it is a request that resolves to a localhost address' do - stub_full_request('https://example.com', ip_address: '127.0.0.1') - - expect { RestClient.get('https://example.com') } - .to raise_error(ArgumentError, - "URL 'https://example.com' is blocked: Requests to localhost are not allowed") - end - - it 'raises error when it is a request to local address' do - expect { RestClient.get('http://172.16.0.0') } - .to raise_error(ArgumentError, - "URL 'http://172.16.0.0' is blocked: Requests to the local network are not allowed") - end - - it 'raises error when it is a request to localhost address' do - expect { RestClient.get('http://127.0.0.1') } - .to raise_error(ArgumentError, - "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed") - end - end - - context 'when port different from URL scheme is used' do - it 'allows the request' do - request_stub = stub_full_request('https://example.com:8080', ip_address: '93.184.216.34') - - RestClient.get('https://example.com:8080/') - - expect(request_stub).to have_been_requested - end - - it 'raises error when it is a request to local address' do - expect { RestClient.get('https://172.16.0.0:8080') } - .to raise_error(ArgumentError, - "URL 'https://172.16.0.0:8080' is blocked: Requests to the local network are not allowed") - end - - it 'raises error when it is a request to localhost address' do - expect { RestClient.get('https://127.0.0.1:8080') } - .to raise_error(ArgumentError, - "URL 'https://127.0.0.1:8080' is blocked: Requests to localhost are not allowed") - end - end - - context 'when DNS rebinding protection is disabled' do - before do - stub_application_setting(dns_rebinding_protection_enabled: false) - end - - it 'allows the request' do - request_stub = stub_request(:get, 'https://example.com') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - end - - context 'when http(s) proxy environment variable is set' do - before do - stub_env('https_proxy' => 'https://my.proxy') - end - - it 'allows the request' do - request_stub = stub_request(:get, 'https://example.com') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - end - - context 'when local requests are allowed' do - before do - stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) - end - - it 'allows an external request' do - request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - - it 'allows an external request that resolves to a local address' do - request_stub = stub_full_request('https://example.com', ip_address: '172.16.0.0') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - - it 'allows an external request that resolves to a localhost address' do - request_stub = stub_full_request('https://example.com', ip_address: '127.0.0.1') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - - it 'allows a local address request' do - request_stub = stub_request(:get, 'http://172.16.0.0') - - RestClient.get('http://172.16.0.0') - - expect(request_stub).to have_been_requested - end - - it 'allows a localhost address request' do - request_stub = stub_request(:get, 'http://127.0.0.1') - - RestClient.get('http://127.0.0.1') - - expect(request_stub).to have_been_requested + def make_request(uri) + RestClient.get(uri) end end end diff --git a/spec/models/project_services/chat_notification_service_spec.rb b/spec/models/project_services/chat_notification_service_spec.rb index 6f4ddd223f6..e8c5f5d611a 100644 --- a/spec/models/project_services/chat_notification_service_spec.rb +++ b/spec/models/project_services/chat_notification_service_spec.rb @@ -30,7 +30,8 @@ describe ChatNotificationService do end describe '#execute' do - let(:chat_service) { described_class.new } + subject(:chat_service) { described_class.new } + let(:user) { create(:user) } let(:project) { create(:project, :repository) } let(:webhook_url) { 'https://example.gitlab.com/' } @@ -53,12 +54,7 @@ describe ChatNotificationService do subject.project = project data = Gitlab::DataBuilder::Push.build_sample(project, user) - expect(Slack::Notifier).to receive(:new) - .with(webhook_url, {}) - .and_return( - double(:slack_service).as_null_object - ) - + expect(chat_service).to receive(:notify).and_return(true) expect(chat_service.execute(data)).to be true end end @@ -68,12 +64,7 @@ describe ChatNotificationService do subject.project = create(:project, :empty_repo) data = Gitlab::DataBuilder::Push.build_sample(subject.project, user) - expect(Slack::Notifier).to receive(:new) - .with(webhook_url, {}) - .and_return( - double(:slack_service).as_null_object - ) - + expect(chat_service).to receive(:notify).and_return(true) expect(chat_service.execute(data)).to be true end end diff --git a/spec/models/project_services/slack_service_spec.rb b/spec/models/project_services/slack_service_spec.rb index f751dd6ffb9..93036ac7ec4 100644 --- a/spec/models/project_services/slack_service_spec.rb +++ b/spec/models/project_services/slack_service_spec.rb @@ -3,5 +3,5 @@ require 'spec_helper' describe SlackService do - it_behaves_like "slack or mattermost notifications", "Slack" + it_behaves_like "slack or mattermost notifications", 'Slack' end diff --git a/spec/support/shared_examples/slack_mattermost_notifications_shared_examples.rb b/spec/support/shared_examples/slack_mattermost_notifications_shared_examples.rb index f15128d3e13..2b68e7bfa82 100644 --- a/spec/support/shared_examples/slack_mattermost_notifications_shared_examples.rb +++ b/spec/support/shared_examples/slack_mattermost_notifications_shared_examples.rb @@ -1,11 +1,13 @@ # frozen_string_literal: true RSpec.shared_examples 'slack or mattermost notifications' do |service_name| + include StubRequests + let(:chat_service) { described_class.new } - let(:webhook_url) { 'https://example.gitlab.com/' } + let(:webhook_url) { 'https://example.gitlab.com' } def execute_with_options(options) - receive(:new).with(webhook_url, options) + receive(:new).with(webhook_url, options.merge(http_client: SlackService::Notifier::HTTPClient)) .and_return(double(:slack_service).as_null_object) end @@ -38,9 +40,13 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.branches_to_be_notified = branches_to_be_notified if branches_to_be_notified end + let!(:stubbed_resolved_hostname) do + stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s + end + it "notifies about #{event_type} events" do chat_service.execute(data) - expect(WebMock).to have_requested(:post, webhook_url) + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname) end end @@ -49,9 +55,13 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.branches_to_be_notified = branches_to_be_notified if branches_to_be_notified end + let!(:stubbed_resolved_hostname) do + stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s + end + it "notifies about #{event_type} events" do chat_service.execute(data) - expect(WebMock).not_to have_requested(:post, webhook_url) + expect(WebMock).not_to have_requested(:post, stubbed_resolved_hostname) end end @@ -66,6 +76,10 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| Gitlab::DataBuilder::Push.build_sample(project, user) end + let!(:stubbed_resolved_hostname) do + stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s + end + before do allow(chat_service).to receive_messages( project: project, @@ -74,8 +88,6 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| webhook: webhook_url ) - WebMock.stub_request(:post, webhook_url) - issue_service = Issues::CreateService.new(project, user, issue_service_options) @issue = issue_service.execute @issues_sample_data = issue_service.hook_data(@issue, 'open') @@ -107,25 +119,25 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| it "calls #{service_name} API for push events" do chat_service.execute(data) - expect(WebMock).to have_requested(:post, webhook_url).once + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once end it "calls #{service_name} API for issue events" do chat_service.execute(@issues_sample_data) - expect(WebMock).to have_requested(:post, webhook_url).once + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once end it "calls #{service_name} API for merge requests events" do chat_service.execute(@merge_sample_data) - expect(WebMock).to have_requested(:post, webhook_url).once + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once end it "calls #{service_name} API for wiki page events" do chat_service.execute(@wiki_page_sample_data) - expect(WebMock).to have_requested(:post, webhook_url).once + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once end it "calls #{service_name} API for deployment events" do @@ -133,14 +145,14 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.execute(deployment_event_data) - expect(WebMock).to have_requested(:post, webhook_url).once + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once end it 'uses the username as an option for slack when configured' do allow(chat_service).to receive(:username).and_return(username) expect(Slack::Notifier).to receive(:new) - .with(webhook_url, username: username) + .with(webhook_url, username: username, http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -151,7 +163,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| it 'uses the channel as an option when it is configured' do allow(chat_service).to receive(:channel).and_return(channel) expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: channel) + .with(webhook_url, channel: channel, http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -163,7 +175,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.update(push_channel: "random") expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: "random") + .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -175,7 +187,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.update(merge_request_channel: "random") expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: "random") + .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -187,7 +199,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.update(issue_channel: "random") expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: "random") + .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -219,7 +231,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.update(wiki_page_channel: "random") expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: "random") + .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -238,7 +250,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| note_data = Gitlab::DataBuilder::Note.build(issue_note, user) expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: "random") + .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -286,7 +298,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| webhook: webhook_url ) - WebMock.stub_request(:post, webhook_url) + stub_full_request(webhook_url, method: :post) end context 'on default branch' do @@ -461,7 +473,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| webhook: webhook_url ) - WebMock.stub_request(:post, webhook_url) + stub_full_request(webhook_url, method: :post) end context 'when commit comment event executed' do @@ -535,7 +547,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| webhook: webhook_url ) - WebMock.stub_request(:post, webhook_url) + stub_full_request(webhook_url, method: :post) end context 'with succeeded pipeline' do diff --git a/spec/support/shared_examples/uses_gitlab_url_blocker_examples.rb b/spec/support/shared_examples/uses_gitlab_url_blocker_examples.rb new file mode 100644 index 00000000000..59c119e6d96 --- /dev/null +++ b/spec/support/shared_examples/uses_gitlab_url_blocker_examples.rb @@ -0,0 +1,155 @@ +# frozen_string_literal: true + +shared_examples 'a request using Gitlab::UrlBlocker' do + # Written to test internal patches against 3rd party libraries + # + # Expects the following to be available in the example contexts: + # + # make_request(uri): Wraps the request we want to test goes through Gitlab::HTTP + # http_method: :get, :post etc + # url_blocked_error_class: Probably just Gitlab::HTTP::BlockedUrlError + + include StubRequests + + context 'when local requests are not allowed' do + it 'allows an external request with http' do + request_stub = stub_full_request('http://example.com', method: http_method, ip_address: '93.184.216.34') + + make_request('http://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows an external request with https' do + request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '93.184.216.34') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'raises error when it is a request that resolves to a local address' do + stub_full_request('https://example.com', method: http_method, ip_address: '172.16.0.0') + + expect { make_request('https://example.com') } + .to raise_error(url_blocked_error_class, + "URL 'https://example.com' is blocked: Requests to the local network are not allowed") + end + + it 'raises error when it is a request that resolves to a localhost address' do + stub_full_request('https://example.com', method: http_method, ip_address: '127.0.0.1') + + expect { make_request('https://example.com') } + .to raise_error(url_blocked_error_class, + "URL 'https://example.com' is blocked: Requests to localhost are not allowed") + end + + it 'raises error when it is a request to local address' do + expect { make_request('http://172.16.0.0') } + .to raise_error(url_blocked_error_class, + "URL 'http://172.16.0.0' is blocked: Requests to the local network are not allowed") + end + + it 'raises error when it is a request to localhost address' do + expect { make_request('http://127.0.0.1') } + .to raise_error(url_blocked_error_class, + "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed") + end + end + + context 'when port different from URL scheme is used' do + it 'allows the request' do + request_stub = stub_full_request('https://example.com:8080', method: http_method, ip_address: '93.184.216.34') + + make_request('https://example.com:8080/') + + expect(request_stub).to have_been_requested + end + + it 'raises error when it is a request to local address' do + expect { make_request('https://172.16.0.0:8080') } + .to raise_error(url_blocked_error_class, + "URL 'https://172.16.0.0:8080' is blocked: Requests to the local network are not allowed") + end + + it 'raises error when it is a request to localhost address' do + expect { make_request('https://127.0.0.1:8080') } + .to raise_error(url_blocked_error_class, + "URL 'https://127.0.0.1:8080' is blocked: Requests to localhost are not allowed") + end + end + + context 'when DNS rebinding protection is disabled' do + before do + stub_application_setting(dns_rebinding_protection_enabled: false) + end + + it 'allows the request' do + request_stub = stub_request(http_method, 'https://example.com') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + end + + context 'when http(s) proxy environment variable is set' do + before do + stub_env('https_proxy' => 'https://my.proxy') + end + + it 'allows the request' do + request_stub = stub_request(http_method, 'https://example.com') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + end + + context 'when local requests are allowed' do + before do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) + end + + it 'allows an external request' do + request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '93.184.216.34') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows an external request that resolves to a local address' do + request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '172.16.0.0') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows an external request that resolves to a localhost address' do + request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '127.0.0.1') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows a local address request' do + request_stub = stub_request(http_method, 'http://172.16.0.0') + + make_request('http://172.16.0.0') + + expect(request_stub).to have_been_requested + end + + it 'allows a localhost address request' do + request_stub = stub_request(http_method, 'http://127.0.0.1') + + make_request('http://127.0.0.1') + + expect(request_stub).to have_been_requested + end + end +end |