From 020c77440e991ea0043b0230b144c5979e5472c1 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 31 May 2022 15:24:04 +1000 Subject: [PATCH] FEATURE: allow for overlapping DiscourseConnect secrets per domain (#16915) Previously we limited Discourse Connect provider to 1 secret per domain. This made it pretty awkward to cycle secrets in environments where config takes time to propagate This change allows for the same domain to have multiple secrets Also fixes internal implementation on DiscourseConnectProvider which was not thread safe as it leaned on class variables to ferry data around Co-authored-by: Alan Guo Xiang Tan Co-authored-by: David Taylor --- app/controllers/session_controller.rb | 2 +- lib/discourse_connect_base.rb | 6 ++- lib/discourse_connect_provider.rb | 65 ++++++++++++++++-------- spec/requests/session_controller_spec.rb | 24 ++++++++- 4 files changed, 71 insertions(+), 26 deletions(-) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 26299bcb8fd..15f7d4368f2 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -79,7 +79,7 @@ class SessionController < ApplicationController end rescue DiscourseConnectProvider::BlankSecret render plain: I18n.t("discourse_connect.missing_secret"), status: 400 - rescue DiscourseConnectProvider::ParseError => e + rescue DiscourseConnectProvider::ParseError # Do NOT pass the error text to the client, it would give them the correct signature render plain: I18n.t("discourse_connect.login_error"), status: 422 rescue DiscourseConnectProvider::BlankReturnUrl diff --git a/lib/discourse_connect_base.rb b/lib/discourse_connect_base.rb index f2e7a7b8207..3a503ddcbe3 100644 --- a/lib/discourse_connect_base.rb +++ b/lib/discourse_connect_base.rb @@ -122,9 +122,13 @@ class DiscourseConnectBase @custom_fields ||= {} end + def self.sign(payload, secret) + OpenSSL::HMAC.hexdigest("sha256", secret, payload) + end + def sign(payload, secret = nil) secret = secret || sso_secret - OpenSSL::HMAC.hexdigest("sha256", secret, payload) + self.class.sign(payload, secret) end def to_json diff --git a/lib/discourse_connect_provider.rb b/lib/discourse_connect_provider.rb index 8ddf51975c4..218d081b10a 100644 --- a/lib/discourse_connect_provider.rb +++ b/lib/discourse_connect_provider.rb @@ -4,38 +4,59 @@ class DiscourseConnectProvider < DiscourseConnectBase class BlankSecret < RuntimeError; end class BlankReturnUrl < RuntimeError; end - def self.parse(payload, sso_secret = nil) - set_return_sso_url(payload) - if sso_secret.blank? && self.sso_secret.blank? - host = URI.parse(@return_sso_url).host - Rails.logger.warn("SSO failed; website #{host} is not in the `discourse_connect_provider_secrets` site settings") + def self.parse(payload, sso_secret = nil, **init_kwargs) + parsed_payload = Rack::Utils.parse_query(payload) + return_sso_url = lookup_return_sso_url(parsed_payload) + + raise ParseError if !return_sso_url + + sso_secret ||= lookup_sso_secret(return_sso_url, parsed_payload) + + if sso_secret.blank? + begin + host = URI.parse(return_sso_url).host + Rails.logger.warn("SSO failed; website #{host} is not in the `discourse_connect_provider_secrets` site settings") + rescue StandardError => e + # going for StandardError cause URI::Error may not be enough, eg it parses to something not + # responding to host + Discourse.warn_exception(e, message: "SSO failed; invalid or missing return_sso_url in SSO payload") + end + raise BlankSecret end - super + super(payload, sso_secret, **init_kwargs) end - def self.set_return_sso_url(payload) - parsed = Rack::Utils.parse_query(payload) - decoded = Base64.decode64(parsed["sso"]) + def self.lookup_return_sso_url(parsed_payload) + decoded = Base64.decode64(parsed_payload["sso"]) decoded_hash = Rack::Utils.parse_query(decoded) - - raise ParseError unless decoded_hash.key? 'return_sso_url' - @return_sso_url = decoded_hash['return_sso_url'] + decoded_hash['return_sso_url'] end - def self.sso_secret - return nil unless @return_sso_url && SiteSetting.enable_discourse_connect_provider + def self.lookup_sso_secret(return_sso_url, parsed_payload) + return nil unless return_sso_url && SiteSetting.enable_discourse_connect_provider - provider_secrets = SiteSetting.discourse_connect_provider_secrets.split(/[|\n]/) - provider_secrets_hash = Hash[*provider_secrets] - return_url_host = URI.parse(@return_sso_url).host - # moves wildcard domains to the end of hash - sorted_secrets = provider_secrets_hash.sort_by { |k, _| k }.reverse.to_h + return_url_host = URI.parse(return_sso_url).host - secret = sorted_secrets.select do |domain, _| - WildcardDomainChecker.check_domain(domain, return_url_host) + provider_secrets = SiteSetting + .discourse_connect_provider_secrets + .split("\n") + .map { |row| row.split("|", 2) } + .sort_by { |k, _| k } + .reverse + + first_domain_match = nil + + pair = provider_secrets.find do |domain, configured_secret| + if WildcardDomainChecker.check_domain(domain, return_url_host) + first_domain_match ||= configured_secret + sign(parsed_payload["sso"], configured_secret) == parsed_payload["sig"] + end end - secret.present? ? secret.values.first : nil + + # falls back to a secret which will fail to validate in DiscourseConnectBase + # this ensures error flow is correct + pair.present? ? pair[1] : first_domain_match end end diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index d40472dd8d2..b3a160864ab 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -1188,7 +1188,9 @@ describe SessionController do "*|secret,forAll", "*.rainbow|wrongSecretForOverRainbow", "www.random.site|secretForRandomSite", + "somewhere.over.rainbow|oldSecretForOverRainbow", "somewhere.over.rainbow|secretForOverRainbow", + "somewhere.over.rainbow|newSecretForOverRainbow", ].join("\n") @sso = DiscourseConnectProvider.new @@ -1245,9 +1247,28 @@ describe SessionController do expect(sso2.no_2fa_methods).to eq(nil) end + it "correctly logs in for secondary domain secrets" do + sign_in @user + + get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("newSecretForOverRainbow")) + expect(response.status).to eq(302) + redirect_uri = URI.parse(response.location) + expect(redirect_uri.host).to eq("somewhere.over.rainbow") + redirect_query = CGI.parse(redirect_uri.query) + expected_sig = DiscourseConnectBase.sign(redirect_query["sso"][0], "newSecretForOverRainbow") + expect(redirect_query["sig"][0]).to eq(expected_sig) + + get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("oldSecretForOverRainbow")) + expect(response.status).to eq(302) + redirect_uri = URI.parse(response.location) + expect(redirect_uri.host).to eq("somewhere.over.rainbow") + redirect_query = CGI.parse(redirect_uri.query) + expected_sig = DiscourseConnectBase.sign(redirect_query["sso"][0], "oldSecretForOverRainbow") + expect(redirect_query["sig"][0]).to eq(expected_sig) + end + it "it fails to log in if secret is wrong" do get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("secretForRandomSite")) - expect(response.status).to eq(422) end @@ -1263,7 +1284,6 @@ describe SessionController do it "returns a 422 if no return_sso_url" do SiteSetting.discourse_connect_provider_secrets = "abcdefghij" - sso = DiscourseConnectProvider.new get "/session/sso_provider?sso=asdf&sig=abcdefghij" expect(response.status).to eq(422) end