policy TLS_FORWARD: check parameters from user

Policy handling was split into smaller functions to allow easier
checking. The code needs further refactoring, it seems that
net_tls_client is just a thin wrapper around tls_client_params_set in C,
which is unnecessary and error prone.

diff --git a/modules/policy/policy.lua b/modules/policy/policy.lua
index 9d162869332ae95e5c231406fed594d0aaea81ca..6e54de18824f13b363d3b832f8cef1ecd447970a 100644 (file)
--- a/modules/policy/policy.lua
+++ b/modules/policy/policy.lua
@@ -128,10 +128,69 @@ local function forward(target)
        end
 end
 
+-- object must be non-empty string or non-empty table of non-empty strings
+local function is_nonempty_string_or_table(object)
+       if type(object) == 'string' then
+               return #object ~= 0
+       elseif type(object) ~= 'table' or not next(object) then
+               return false
+       end
+       for _, val in pairs(object) do
+               if type(val) ~= 'string' or #val == 0 then
+                       return false
+               end
+       end
+       return true
+end
+
+local function insert_from_string_or_table(source, destination)
+       if type(source) == 'table' then
+               for _, v in pairs(source) do
+                       table.insert(destination, v)
+               end
+       else
+               table.insert(destination, source)
+       end
+end
+
+-- Check for allowed authentication types and return type for the current target
+local function tls_forward_target_authtype(idx, target)
+       if (target.pin and not (target.ca_file or target.hostname or target.insecure)) then
+               if not is_nonempty_string_or_table(target.pin) then
+                       error('TLS_FORWARD target authentication is invalid at position '
+                             .. idx .. '; pin must be string or list of strings')
+               end
+               return 'pin'
+       elseif (target.insecure and not (target.ca_file or target.hostname or target.pin)) then
+               return 'insecure'
+       elseif (target.ca_file and target.hostname and not (target.insecure or target.pin)) then
+               if not (is_nonempty_string_or_table(target.hostname)
+                       and is_nonempty_string_or_table(target.ca_file)) then
+                       error('TLS_FORWARD target authentication is invalid at position '
+                             .. idx .. '; hostname and ca_file must be string or list of strings')
+               end
+               return 'cert'
+       else
+               error('TLS_FORWARD authentication options at position ' .. idx
+                     .. ' are invalid; specify one of: pin / hostname+ca_file / insecure')
+       end
+end
+
+local function tls_forward_target_check_syntax(idx, list_entry)
+       if type(list_entry) ~= 'table' then
+               error('TLS_FORWARD target must be a non-empty table (found '
+                     .. type(list_entry) .. ' at position ' .. idx .. ')')
+       end
+       if type(list_entry[1]) ~= 'string' then
+               error('TLS_FORWARD target must start with an IP address (found '
+                     .. type(list_entry[1]) .. ' at the beginning of target position ' .. idx .. ')')
+       end
+end
+
 -- Forward request and all subrequests to upstream over TLS; validate answers
 local function tls_forward(target)
-       local sockaddr_list = {}
-       local addr_list = {}
+       local sockaddr_c_list = {}
+       local sockaddr_config = {}  -- items: { string_addr=<addr string>, auth_type=<auth type> }
        local ca_files = {}
        local hostnames = {}
        local pins = {}
@@ -139,72 +198,44 @@ local function tls_forward(target)
                error('TLS_FORWARD argument must be a non-empty table')
        end
        for idx, upstream_list_entry in pairs(target) do
-               if type(upstream_list_entry) ~= 'table' then
-                       error('TLS_FORWARD target must be a non-empty table (found '
-                             .. type(upstream_list_entry) .. ' at position ' .. idx .. ')')
-               end
-               local upstream_addr = upstream_list_entry[1]
-               if type(upstream_addr) ~= 'string' then
-                       error('TLS_FORWARD target must start with an IP address (found '
-                             .. type(upstream_addr) .. ' at the beginning of target position ' .. idx .. ')')
-               end
-               table.insert(sockaddr_list, addr2sock(upstream_addr, 853))
-               table.insert(addr_list, upstream_addr)
-               local ca_file = upstream_list_entry['ca_file']
-               if ca_file ~= nil then
-                       local hostname = upstream_list_entry['hostname']
-                       if hostname == nil then
-                               assert(false, 'hostname(s) is absent in TLS_FORWARD target')
-                       end
-                       local ca_files_local = {}
-                       if type(ca_file) == 'table' then
-                               for _, v in pairs(ca_file) do
-                                       table.insert(ca_files_local, v)
-                               end
-                       else
-                               table.insert(ca_files_local, ca_file)
-                       end
-                       local hostnames_local = {}
-                       if type(hostname) == 'table' then
-                               for _, v in pairs(hostname) do
-                                       table.insert(hostnames_local, v)
-                               end
-                       else
-                               table.insert(hostnames_local, hostname)
-                       end
-                       if next(ca_files_local) then
-                               ca_files[upstream_addr] = ca_files_local
-                       end
-                       if next(hostnames_local) then
-                               hostnames[upstream_addr] = hostnames_local
-                       end
-               end
-               local pin = upstream_list_entry['pin']
-               local pins_local = {}
-               if pin ~= nil then
-                       if type(pin) == 'table' then
-                               for _, v in pairs(pin) do
-                                       table.insert(pins_local, v)
-                               end
-                       else
-                               table.insert(pins_local, pin)
-                       end
+               tls_forward_target_check_syntax(idx, upstream_list_entry)
+               local auth_type = tls_forward_target_authtype(idx, upstream_list_entry)
+               local string_addr = upstream_list_entry[1]
+               local sockaddr_c = addr2sock(string_addr, 853)
+               local sockaddr_lua = ffi.string(sockaddr_c, ffi.C.kr_inaddr_len(sockaddr_c))
+               if sockaddr_config[sockaddr_lua] then
+                       error('TLS_FORWARD configuration cannot declare two configs for IP address ' .. string_addr)
                end
-               if next(pins_local) then
-                       pins[upstream_addr] = pins_local
+               table.insert(sockaddr_c_list, sockaddr_c)
+               sockaddr_config[sockaddr_lua] = {string_addr=string_addr, auth_type=auth_type}
+               if auth_type == 'cert' then
+                       ca_files[sockaddr_lua] = {}
+                       hostnames[sockaddr_lua] = {}
+                       insert_from_string_or_table(upstream_list_entry.ca_file, ca_files[sockaddr_lua])
+                       insert_from_string_or_table(upstream_list_entry.hostname, hostnames[sockaddr_lua])
+               elseif auth_type == 'pin' then
+                       pins[sockaddr_lua] = {}
+                       insert_from_string_or_table(upstream_list_entry.pin, pins[sockaddr_lua])
+               elseif auth_type ~= 'insecure' then
+                       -- insecure does nothing, user does not want authentication
+                       assert(false, 'unsupported auth_type')
                end
        end
 
-       -- Update the global table of authentication data.
-       for _, v in pairs(addr_list) do
-               if (pins[v] == nil and ca_files[v] == nil) then
-                       net.tls_client(v)
-               elseif (pins[v] ~= nil and ca_files[v] == nil) then
-                       net.tls_client(v, pins[v])
-               elseif (pins[v] == nil and ca_files[v] ~= nil) then
-                       net.tls_client(v, ca_files[v], hostnames[v])
+       -- Update the global table of authentication data only if all checks above passed
+       for sockaddr_lua, config in pairs(sockaddr_config) do
+               assert(#config.string_addr > 0)
+               if config.auth_type == 'insecure' then
+                       net.tls_client(config.string_addr)
+               elseif config.auth_type == 'pin' then
+                       assert(#pins[sockaddr_lua] > 0)
+                       net.tls_client(config.string_addr, pins[sockaddr_lua])
+               elseif config.auth_type == 'cert' then
+                       assert(#ca_files[sockaddr_lua] > 0)
+                       assert(#hostnames[sockaddr_lua] > 0)
+                       net.tls_client(config.string_addr, ca_files[sockaddr_lua], hostnames[sockaddr_lua])
                else
-                       net.tls_client(v, pins[v], ca_files[v], hostnames[v])
+                       assert(false, 'unsupported auth_type')
                end
        end
 
@@ -218,7 +249,7 @@ local function tls_forward(target)
                qry.flags.AWAIT_CUT = true
                req.options.TCP = true
                qry.flags.TCP = true
-               set_nslist(qry, sockaddr_list)
+               set_nslist(qry, sockaddr_c_list)
                return state
        end
 end

diff --git a/modules/policy/policy_test.lua b/modules/policy/policy_test.lua
index 65d32102758f498b00df1c9370b952cebc2fc8e5..f88e75072593d34b8b949f2531b6026529ca0797 100644 (file)
--- a/modules/policy/policy_test.lua
+++ b/modules/policy/policy_test.lua
@@ -1,5 +1,3 @@
-local utils = require('test_utils')
-
 -- setup resolver
 modules = { 'policy' }
 
@@ -13,21 +11,33 @@ local function test_tls_forward()
 
        boom(policy.TLS_FORWARD, {{'1'}}, 'TLS_FORWARD with invalid IP address')
        -- boom(policy.TLS_FORWARD, {{{'::1', bleble=''}}}, 'TLS_FORWARD with valid IP and invalid parameters')
-       -- boom(policy.TLS_FORWARD, {{{'127.0.0.1'}}}, 'TLS_FORWARD with missing auth parameters')
+       boom(policy.TLS_FORWARD, {{{'127.0.0.1'}}}, 'TLS_FORWARD with missing auth parameters')
+
+       ok(policy.TLS_FORWARD({{'127.0.0.1', insecure=true}}), 'TLS_FORWARD with no authentication')
+       boom(policy.TLS_FORWARD, {{{'100:dead::', insecure=true},
+                                  {'100:DEAD:0::', insecure=true}
+                          }}, 'TLS_FORWARD with duplicate IP addresses is not allowed')
+       ok(policy.TLS_FORWARD({{'100:dead::', insecure=true},
+                              {'100:dead::@443', insecure=true}
+                          }), 'TLS_FORWARD with duplicate IP addresses but different ports is allowed')
 
-       -- boom(policy.TLS_FORWARD, {{{'::1', pin=''}}}, 'TLS_FORWARD with empty pin')
+       boom(policy.TLS_FORWARD, {{{'::1', pin=''}}}, 'TLS_FORWARD with empty pin')
        -- boom(policy.TLS_FORWARD, {{{'::1', pin='č'}}}, 'TLS_FORWARD with bad pin')
-       ok(policy.TLS_FORWARD({{'::1', pin='ZTNiMGM0NDI5OGZjMWMxNDlhZmJmNGM4OTk2ZmI5MjQyN2FlNDFlNDY0OWI5MzRjYTQ5NTk5MWI3ODUyYjg1NQ=='}}), 'TLS_FORWARD with base64 pin')
-       ok(policy.TLS_FORWARD({{'::1', pin={
-                                       'ZTNiMGM0NDI5OGZjMWMxNDlhZmJmNGM4OTk2ZmI5MjQyN2FlNDFlNDY0OWI5MzRjYTQ5NTk5MWI3ODUyYjg1NQ==',
-                                       'MTcwYWUzMGNjZDlmYmE2MzBhZjhjZGE2ODQxZTAwYzZiNjU3OWNlYzc3NmQ0MTllNzAyZTIwYzY5YzQ4OGZmOA=='
-                               }}}), 'TLS_FORWARD with table of pins')
+       ok(policy.TLS_FORWARD({
+                       {'::1', pin='ZTNiMGM0NDI5OGZjMWMxNDlhZmJmNGM4OTk2ZmI5MjQyN2FlNDFlNDY0OWI5MzRjYTQ5NTk5MWI3ODUyYjg1NQ=='}
+               }), 'TLS_FORWARD with base64 pin')
+       ok(policy.TLS_FORWARD({
+               {'::1', pin={
+                       'ZTNiMGM0NDI5OGZjMWMxNDlhZmJmNGM4OTk2ZmI5MjQyN2FlNDFlNDY0OWI5MzRjYTQ5NTk5MWI3ODUyYjg1NQ==',
+                       'MTcwYWUzMGNjZDlmYmE2MzBhZjhjZGE2ODQxZTAwYzZiNjU3OWNlYzc3NmQ0MTllNzAyZTIwYzY5YzQ4OGZmOA=='
+               }}}), 'TLS_FORWARD with table of pins')
 
-       ok(policy.TLS_FORWARD({{'::1', hostname='test.', ca='/tmp/ca.crt'}}), 'TLS_FORWARD with hostname + CA cert')
-       -- boom(policy.TLS_FORWARD, {{{'::1', hostname='test.'}}}, 'TLS_FORWARD with just hostname')
-       -- boom(policy.TLS_FORWARD, {{{'::1', ca='/tmp/ca.crt'}}}, 'TLS_FORWARD with just CA cert')
-       -- boom(policy.TLS_FORWARD, {{{'::1', hostname='', ca='/tmp/ca.crt'}}}, 'TLS_FORWARD with invalid hostname + CA cert')
-       -- boom(policy.TLS_FORWARD, {{{'::1', hostname='test.', ca='/dev/null'}}}, 'TLS_FORWARD with hostname + unreadable CA cert')
+       -- ok(policy.TLS_FORWARD({{'::1', hostname='test.', ca_file='/tmp/ca.crt'}}), 'TLS_FORWARD with hostname + CA cert')
+       boom(policy.TLS_FORWARD, {{{'::1', hostname='test.'}}}, 'TLS_FORWARD with just hostname')
+       boom(policy.TLS_FORWARD, {{{'::1', ca_file='/tmp/ca.crt'}}}, 'TLS_FORWARD with just CA cert')
+       boom(policy.TLS_FORWARD, {{{'::1', hostname='', ca_file='/tmp/ca.crt'}}}, 'TLS_FORWARD with empty hostname + CA cert')
+       boom(policy.TLS_FORWARD, {{{'::1', hostname='test.', ca_file='/dev/a_file_which_surely_does_NOT_exist!'}}},
+               'TLS_FORWARD with hostname + unreadable CA cert')
 end
 
 return {