From 92e6cf76183da540b59d70775dac9d4b1dbc12ee Mon Sep 17 00:00:00 2001 From: Andrei Bora Date: Tue, 11 Aug 2020 17:29:34 +0300 Subject: [PATCH 1/3] Add pre and post validation for users that want to use their own public keys --- resources/prosody-plugins/mod_auth_token.lua | 27 +++++++++++++++----- resources/prosody-plugins/token/util.lib.lua | 5 +++- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/resources/prosody-plugins/mod_auth_token.lua b/resources/prosody-plugins/mod_auth_token.lua index 76db97789..5f3b9f57a 100644 --- a/resources/prosody-plugins/mod_auth_token.lua +++ b/resources/prosody-plugins/mod_auth_token.lua @@ -74,18 +74,28 @@ function provider.delete_user(username) return nil; end +local function validate_result(session, res, error, reason) + if res == false then + log("warn", + "Error verifying token err:%s, reason:%s", error, reason); + session.auth_token = nil; + return res, error, reason; + end +end + function provider.get_sasl_handler(session) local function get_username_from_token(self, message) - local res, error, reason = token_util:process_and_verify_token(session); - if (res == false) then - log("warn", - "Error verifying token err:%s, reason:%s", error, reason); - session.auth_token = nil; - return res, error, reason; + -- retrieve custom public key from server and save it on the session + local event_result = prosody.events.fire_event("pre-jitsi-authentication-fetch-key", session); + if event_result ~= nil then + validate_result(session,event_result.res, event_result.error, event_result.reason) end + local res, error, reason = token_util:process_and_verify_token(session); + validate_result(session, res, error, reason); + local customUsername = prosody.events.fire_event("pre-jitsi-authentication", session); @@ -102,6 +112,11 @@ function provider.get_sasl_handler(session) self.username = message; end + local event_result = prosody.events.fire_event("post-jitsi-authentication", session); + if event_result ~= nil then + validate_result(session,event_result.res, event_result.error, event_result.reason) + end + return res; end diff --git a/resources/prosody-plugins/token/util.lib.lua b/resources/prosody-plugins/token/util.lib.lua index 902a7808f..6c19245d7 100644 --- a/resources/prosody-plugins/token/util.lib.lua +++ b/resources/prosody-plugins/token/util.lib.lua @@ -301,7 +301,10 @@ function Util:process_and_verify_token(session, acceptedIssuers) end local pubKey; - if self.asapKeyServer and session.auth_token ~= nil then + if session.public_key then + module:log("debug","Public key was found on the session"); + pubKey = session.public_key; + elseif self.asapKeyServer and session.auth_token ~= nil then local dotFirst = session.auth_token:find("%."); if not dotFirst then return nil, "Invalid token" end local header, err = json_safe.decode(basexx.from_url64(session.auth_token:sub(1,dotFirst-1))); From b765adca752c5bda95b15791e8421852c8ab7000 Mon Sep 17 00:00:00 2001 From: Andrei Bora Date: Fri, 14 Aug 2020 15:06:14 +0300 Subject: [PATCH 2/3] Solve review issues and add retries for http call --- resources/prosody-plugins/mod_auth_token.lua | 34 +++++----- resources/prosody-plugins/token/util.lib.lua | 63 ++--------------- resources/prosody-plugins/util.lib.lua | 71 ++++++++++++++++++++ 3 files changed, 95 insertions(+), 73 deletions(-) diff --git a/resources/prosody-plugins/mod_auth_token.lua b/resources/prosody-plugins/mod_auth_token.lua index 5f3b9f57a..8b437d2cd 100644 --- a/resources/prosody-plugins/mod_auth_token.lua +++ b/resources/prosody-plugins/mod_auth_token.lua @@ -74,27 +74,26 @@ function provider.delete_user(username) return nil; end -local function validate_result(session, res, error, reason) - if res == false then - log("warn", - "Error verifying token err:%s, reason:%s", error, reason); - session.auth_token = nil; - return res, error, reason; - end -end - function provider.get_sasl_handler(session) local function get_username_from_token(self, message) -- retrieve custom public key from server and save it on the session - local event_result = prosody.events.fire_event("pre-jitsi-authentication-fetch-key", session); - if event_result ~= nil then - validate_result(session,event_result.res, event_result.error, event_result.reason) + local pre_event_result = prosody.events.fire_event("pre-jitsi-authentication-fetch-key", session); + if pre_event_result ~= nil and pre_event_result.res == false then + log("warn", + "Error verifying token on pre authentication stage:%s, reason:%s", pre_event_result.error, pre_event_result.reason); + session.auth_token = nil; + return pre_event_result.res, pre_event_result.error, pre_event_result.reason; end local res, error, reason = token_util:process_and_verify_token(session); - validate_result(session, res, error, reason); + if res == false then + log("warn", + "Error verifying token err:%s, reason:%s", error, reason); + session.auth_token = nil; + return res, error, reason; + end local customUsername = prosody.events.fire_event("pre-jitsi-authentication", session); @@ -112,9 +111,12 @@ function provider.get_sasl_handler(session) self.username = message; end - local event_result = prosody.events.fire_event("post-jitsi-authentication", session); - if event_result ~= nil then - validate_result(session,event_result.res, event_result.error, event_result.reason) + local post_event_result = prosody.events.fire_event("post-jitsi-authentication", session); + if post_event_result ~= nil and post_event_result.res == false then + log("warn", + "Error verifying token on post authentication stage :%s, reason:%s", post_event_result.error, post_event_result.reason); + session.auth_token = nil; + return post_event_result.res, post_event_result.error, post_event_result.reason; end return res; diff --git a/resources/prosody-plugins/token/util.lib.lua b/resources/prosody-plugins/token/util.lib.lua index 6c19245d7..c6a6e2c87 100644 --- a/resources/prosody-plugins/token/util.lib.lua +++ b/resources/prosody-plugins/token/util.lib.lua @@ -5,17 +5,13 @@ local basexx = require "basexx"; local have_async, async = pcall(require, "util.async"); local hex = require "util.hex"; local jwt = require "luajwtjitsi"; -local http = require "net.http"; local jid = require "util.jid"; local json_safe = require "cjson.safe"; local path = require "util.paths"; local sha256 = require "util.hashes".sha256; -local timer = require "util.timer"; +local http_get_with_retry = module:require "util".http_get_with_retry; -local http_timeout = 30; -local http_headers = { - ["User-Agent"] = "Prosody ("..prosody.version.."; "..prosody.platform..")" -}; +local nr_retries = 3; -- TODO: Figure out a less arbitrary default cache size. local cacheSize = module:get_option_number("jwt_pubkey_cache_size", 128); @@ -131,65 +127,18 @@ function Util:get_public_key(keyId) if content == nil then -- If the key is not found in the cache. module:log("debug", "Cache miss for key: "..keyId); - local code; - local timeout_occurred; - local wait, done = async.waiter(); - local keyurl = path.join(self.asapKeyServer, hex.to(sha256(keyId))..'.pem'); - - local function cb(content_, code_, response_, request_) - if timeout_occurred == nil then - content, code = content_, code_; - if code == 200 or code == 204 then - self.cache:set(keyId, content); - else - module:log("warn", "Error on public key request: Code %s, Content %s", - code_, content_); - end - done(); - else - module:log("warn", "public key reply delivered after timeout from: %s",keyurl); - end - end - - -- TODO: Is the done() call racey? Can we cancel this if the request - -- succeedes? - local function cancel() - -- TODO: This check is racey. Not likely to be a problem, but we should - -- still stick a mutex on content / code at some point. - if code == nil then - timeout_occurred = true; - module:log("warn", "Timeout %s seconds fetching public key from: %s",http_timeout,keyurl); - if http.destroy_request ~= nil then - http.destroy_request(request); - end - done(); - end - end - module:log("debug", "Fetching public key from: "..keyurl); - - -- We hash the key ID to work around some legacy behavior and make - -- deployment easier. It also helps prevent directory - -- traversal attacks (although path cleaning could have done this too). - local request = http.request(keyurl, { - headers = http_headers or {}, - method = "GET" - }, cb); - - timer.add_task(http_timeout, cancel); - wait(); - - if code == 200 or code == 204 then - return content; + content = http_get_with_retry(keyurl, nr_retries); + if content ~= nil then + self.cache:set(keyId, content); end + return content; else -- If the key is in the cache, use it. module:log("debug", "Cache hit for key: "..keyId); return content; end - - return nil; end --- Verifies issuer part of token diff --git a/resources/prosody-plugins/util.lib.lua b/resources/prosody-plugins/util.lib.lua index fe74f02ad..e9b99476f 100644 --- a/resources/prosody-plugins/util.lib.lua +++ b/resources/prosody-plugins/util.lib.lua @@ -1,5 +1,13 @@ local jid = require "util.jid"; +local timer = require "util.timer"; +local http = require "net.http"; + +local http_timeout = 30; local have_async, async = pcall(require, "util.async"); +local http_headers = { + ["User-Agent"] = "Prosody ("..prosody.version.."; "..prosody.platform..")" +}; + local muc_domain_prefix = module:get_option_string("muc_mapper_domain_prefix", "conference"); @@ -208,6 +216,68 @@ function is_healthcheck_room(room_jid) return false; end +-- Utility function to make an http get request and +-- retry @param retry number of times +-- @param url endpoint to be called +-- @param retry nr of retries, if retry is +-- nil there will be no retries +-- @returns result of the http call or nil if +-- the external call failed after the last retry +function http_get_with_retry(url, retry) + local content, code; + local wait, done = async.waiter(); + local function cb(content_, code_, response_, request_) + code = code_; + if code == 200 or code == 204 then + module:log("debug", "External call was successful, content %s", content_); + content = content_ + else + module:log("warn", "Error on public key request: Code %s, Content %s", + code_, content_); + end + done(); + end + + local function call_http() + return http.request(url, { + headers = http_headers or {}, + method = "GET" + }, cb); + end + + local request = call_http(); + + local function cancel() + -- TODO: This check is racey. Not likely to be a problem, but we should + -- still stick a mutex on content / code at some point. + if code == nil then + -- no longer present in prosody 0.11, so check before calling + if http.destroy_request ~= nil then + http.destroy_request(request); + end + if retry == nil then + module:log("debug", "External call failed and retry policy is not set"); + done(); + elseif retry ~= nil and retry < 1 then + module:log("debug", "External call failed after retry") + done(); + else + module:log("debug", "External call failed, retry nr %s", retry) + retry = retry - 1; + request = call_http() + return http_timeout; + end + end + end + timer.add_task(http_timeout, cancel); + wait(); + + if code == 200 or code == 204 then + return content; + end + return nil; +end + return { is_feature_allowed = is_feature_allowed; is_healthcheck_room = is_healthcheck_room; @@ -217,4 +287,5 @@ return { room_jid_split_subdomain = room_jid_split_subdomain; internal_room_jid_match_rewrite = internal_room_jid_match_rewrite; update_presence_identity = update_presence_identity; + http_get_with_retry = http_get_with_retry; }; From af71d80150609fecdfbd4a3538c26001174e35ff Mon Sep 17 00:00:00 2001 From: Andrei Bora Date: Wed, 19 Aug 2020 17:38:40 +0300 Subject: [PATCH 3/3] Fix call after timeout --- resources/prosody-plugins/util.lib.lua | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/resources/prosody-plugins/util.lib.lua b/resources/prosody-plugins/util.lib.lua index e9b99476f..9c90553fd 100644 --- a/resources/prosody-plugins/util.lib.lua +++ b/resources/prosody-plugins/util.lib.lua @@ -225,17 +225,22 @@ end -- the external call failed after the last retry function http_get_with_retry(url, retry) local content, code; + local timeout_occurred; local wait, done = async.waiter(); local function cb(content_, code_, response_, request_) - code = code_; - if code == 200 or code == 204 then - module:log("debug", "External call was successful, content %s", content_); - content = content_ + if timeout_occurred == nil then + code = code_; + if code == 200 or code == 204 then + module:log("debug", "External call was successful, content %s", content_); + content = content_ + else + module:log("warn", "Error on public key request: Code %s, Content %s", + code_, content_); + end + done(); else - module:log("warn", "Error on public key request: Code %s, Content %s", - code_, content_); + module:log("warn", "External call reply delivered after timeout from: %s", url); end - done(); end local function call_http() @@ -251,6 +256,8 @@ function http_get_with_retry(url, retry) -- TODO: This check is racey. Not likely to be a problem, but we should -- still stick a mutex on content / code at some point. if code == nil then + timeout_occurred = true; + module:log("warn", "Timeout %s seconds making the external call to: %s", http_timeout, url); -- no longer present in prosody 0.11, so check before calling if http.destroy_request ~= nil then http.destroy_request(request); @@ -272,10 +279,7 @@ function http_get_with_retry(url, retry) timer.add_task(http_timeout, cancel); wait(); - if code == 200 or code == 204 then - return content; - end - return nil; + return content; end return {