From: Petr Špaček <petr.spacek@nic.cz>
Date: Tue, 24 Jul 2018 16:12:26 +0000 (+0200)
Subject: Fix regression in HTTP module which broke custom certs.
X-Git-Tag: v2.4.1~5^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ba7eb057b2ead118e093b1aa72e8a394bd5325c1;p=thirdparty%2Fknot-resolver.git

Fix regression in HTTP module which broke custom certs.

This is now covered by test suite.
fixup! b2cefdcf350e846492579e3308f234a696350e01 (regressed in 2.4.0).

Parameter cert=false did not work even in 2.3.0 so it was replaced with cleaner
tls=false.
---

diff --git a/modules/http/README.rst b/modules/http/README.rst
index 5a64b81c4..f5af30d33 100644
--- a/modules/http/README.rst
+++ b/modules/http/README.rst
@@ -42,15 +42,18 @@ Now you can reach the web services and APIs, done!
 	$ curl -k https://localhost:8053
 	$ curl -k https://localhost:8053/stats
 
-It is possible to disable HTTPS altogether by passing ``cert = false`` option.
+It is possible to disable HTTPS altogether by passing ``tls = false`` option.
 While it's not recommended, it could be fine for localhost tests as, for example,
 Safari doesn't allow WebSockets over HTTPS with a self-signed certificate.
 Major drawback is that current browsers won't do HTTP/2 over insecure connection.
 
+Alternatively you can disable unecrypted HTTP and enforce HTTPS by passing
+``tls = true`` option.
+
 .. code-block:: lua
 
 	http = {
-		cert = false,
+		tls = true,
 	}
 
 If you want to provide your own certificate and key, you're welcome to do so:
diff --git a/modules/http/http.lua b/modules/http/http.lua
index 11b8c9cae..f3c4e4936 100644
--- a/modules/http/http.lua
+++ b/modules/http/http.lua
@@ -274,32 +274,32 @@ end
 -- @function Listen on given HTTP(s) host
 function M.add_interface(conf)
 	local crt, key, ephemeral
-	if conf.crtfile ~= false then
-		-- Check if the cert file exists
-		if not conf.crtfile then
-			conf.crtfile = 'self.crt'
-			conf.keyfile = 'self.key'
+	if conf.tls ~= false then
+		-- Check if a cert file was specified
+		if not conf.cert then
+			conf.cert = 'self.crt'
+			conf.key = 'self.key'
 			ephemeral = true
-		elseif not conf.keyfile then
+		elseif not conf.key then
 			error('certificate provided, but missing key')
 		end
 		-- Read or create self-signed x509 certificate
-		local f = io.open(conf.crtfile, 'r')
+		local f = io.open(conf.cert, 'r')
 		if f then
 			crt = assert(x509.new(f:read('*all')))
 			f:close()
 			-- Continue reading key file
 			if crt then
-				f = io.open(conf.keyfile, 'r')
+				f = io.open(conf.key, 'r')
 				key = assert(pkey.new(f:read('*all')))
 				f:close()
 			end
 		elseif ephemeral then
-			crt, key = updatecert(conf.crtfile, conf.keyfile)
+			crt, key = updatecert(conf.cert, conf.key)
 		end
 		-- Check loaded certificate
 		if not crt or not key then
-			panic('failed to load certificate "%s"', conf.crtfile)
+			panic('failed to load certificate "%s"', conf.cert)
 		end
 	end
 	-- Compose server handler
@@ -320,6 +320,7 @@ function M.add_interface(conf)
 		reuseport = reuseport,
 		client_timeout = conf.client_timeout or 5,
 		ctx = crt and tlscontext(crt, key),
+		tls = conf.tls,
 		onstream = routes,
 	}
 	-- Manually call :listen() so that we are bound before calling :localname()
@@ -336,7 +337,7 @@ function M.add_interface(conf)
 		expiry = math.max(0, expiry - (os.time() - 3 * 24 * 3600))
 		event.after(expiry, function ()
 			log('[http] refreshed ephemeral certificate')
-			crt, key = updatecert(conf.crtfile, conf.keyfile)
+			crt, key = updatecert(conf.cert, conf.key)
 			s.ctx = tlscontext(crt, key)
 		end)
 	end
@@ -348,8 +349,8 @@ function M.interface(host, port, endpoints, crtfile, keyfile)
 		host = host,
 		port = port,
 		endpoints = endpoints,
-		crtfile = crtfile,
-		keyfile = keyfile,
+		cert = crtfile,
+		key = keyfile,
 	}
 end
 
diff --git a/modules/http/test_tls/broken.crt b/modules/http/test_tls/broken.crt
new file mode 100644
index 000000000..d93d1f856
--- /dev/null
+++ b/modules/http/test_tls/broken.crt
@@ -0,0 +1,3 @@
+Æ¿¯ˆž
³psâ$Ðëí¯Ö¡«Æȼ[ŒîØ1´Ç	=fl:°Êl=z†=M}iÉ»¤Ñ­†÷*7ƒ)ÄÑ5
+ÈóîÒj¨šIÜWÈÀ­®·´
MwÚf[´HïŽíÙÑÅ-Á¯ËìýEȼë€fû¦Ö
		2fTKÊqýFѬUÖ	ƒ“(jÀ¤œaà,՜¥*²X:lFÌÍ¿£Mî©>ó3Œš
+¬‘áD<^O™q
k¥ˆQŽÉý‘κM…gÄ]pUNÉö¥Mݝ>ŒŸÂÿº¹á(EI”®µ'ÌGŀ÷mÕÅ÷Ã
:ž3
º_”!‚ÁΒ?3Œú$H[„E†¡M¡4èòR¨†ÒA+0w¥ö0ÏÓÁ“%é¸eo¤ašåëØó(öw;¥oÇ¥î¯$—!ZèÀˆr%Äùàü&ßýh;1¾@ˆñý-9((Ób7±á\„UâoÃJÊ`»:Þª€†¼™~ŸdÎa•œƒìÜЃÇŒEœ—PB*l
«Î}!q7ü;+QRL‘¢¶vÉûËQ‡[KãYûXðRà2(¬íùªå7ÀÜ+$ëÃE,óõ­Ûý³IRÄÙï·
§^4µD ¼_r…,išÁèÁ\ðhξ
\ No newline at end of file
diff --git a/modules/http/test_tls/broken.key b/modules/http/test_tls/broken.key
new file mode 100644
index 000000000..ebcbfcfe9
Binary files /dev/null and b/modules/http/test_tls/broken.key differ
diff --git a/modules/http/test_tls/test.crt b/modules/http/test_tls/test.crt
new file mode 100644
index 000000000..01c36f8d9
--- /dev/null
+++ b/modules/http/test_tls/test.crt
@@ -0,0 +1,10 @@
+-----BEGIN CERTIFICATE-----
+MIIBXDCCAQKgAwIBAgIRAONzB6ou1Lh79QSlofsBtBMwCgYIKoZIzj0EAwIwGTEX
+MBUGA1UEAwwOcm9oYW4tcmVzb2x2ZXIwHhcNMTgwNzIzMTA1MjE4WhcNMTgxMDIx
+MTA1MjE4WjAZMRcwFQYDVQQDDA5yb2hhbi1yZXNvbHZlcjBZMBMGByqGSM49AgEG
+CCqGSM49AwEHA0IABJPYWceFJkbjORCrO8aIhMk3Bw2PpTzuPC27O/rjojBjmadO
+vZyFxIKgfYiHp4uMr1z81K+cqq1s/q0+kW+tNaejKzApMBkGA1UdEQQSMBCCDnJv
+aGFuLXJlc29sdmVyMAwGA1UdEwEB/wQCMAAwCgYIKoZIzj0EAwIDSAAwRQIhAL+Z
+IElUAmI0nQdaSRLZw5LCZeC/OIFx9JfaoDzMNkW5AiABXCWYzR+/uyYV7KDucwtW
+LGh/LrjC/FZGK3Drefbu0A==
+-----END CERTIFICATE-----
diff --git a/modules/http/test_tls/test.key b/modules/http/test_tls/test.key
new file mode 100644
index 000000000..1256b5ab8
--- /dev/null
+++ b/modules/http/test_tls/test.key
@@ -0,0 +1,9 @@
+-----BEGIN PUBLIC KEY-----
+MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEk9hZx4UmRuM5EKs7xoiEyTcHDY+l
+PO48Lbs7+uOiMGOZp069nIXEgqB9iIeni4yvXPzUr5yqrWz+rT6Rb601pw==
+-----END PUBLIC KEY-----
+-----BEGIN PRIVATE KEY-----
+MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgtgNJHFS7+jlibs3b
+4NMYVLgZvVgOh5ouMn/ujQrAbouhRANCAAST2FnHhSZG4zkQqzvGiITJNwcNj6U8
+7jwtuzv646IwY5mnTr2chcSCoH2Ih6eLjK9c/NSvnKqtbP6tPpFvrTWn
+-----END PRIVATE KEY-----
diff --git a/modules/http/test_tls/tls.test.lua b/modules/http/test_tls/tls.test.lua
new file mode 100644
index 000000000..9eac38298
--- /dev/null
+++ b/modules/http/test_tls/tls.test.lua
@@ -0,0 +1,164 @@
+-- check prerequisites
+local has_http = pcall(require, 'http') and pcall(require, 'http.request')
+if not has_http then
+	pass('skipping http module test because its not installed')
+	done()
+else
+	local request = require('http.request')
+	local openssl_ctx = require('openssl.ssl.context')
+
+	local function setup_module(desc, config)
+		if http then
+			modules.unload('http')
+		end
+		modules.load('http')
+		same(http.config(config), nil, desc .. ' can be configured')
+
+		local server = http.servers[1]
+		ok(server ~= nil, desc .. ' creates server instance')
+		local _, host, port = server:localname()
+		ok(host and port, desc .. ' binds to an interface')
+		return host, port
+	end
+
+	local function http_get(uri)
+		-- disable certificate verification in this test
+		local req = request.new_from_uri(uri)
+		local idxstart = string.find(uri, 'https://')
+		if idxstart == 1 then
+			req.ctx = openssl_ctx.new()
+			assert(req.ctx, 'OpenSSL cert verification must be disabled')
+			req.ctx:setVerify(openssl_ctx.VERIFY_NONE)
+		end
+
+		local headers = assert(req:go())
+		return tonumber(headers:get(':status'))
+	end
+
+	-- test whether http interface responds and binds
+	local function check_protocol(uri, description, ok_expected)
+		if ok_expected then
+			local code = http_get(uri)
+			same(code, 200, description)
+		else
+			boom(http_get, {uri}, description)
+		end
+	end
+
+	local function test_defaults()
+		local host, port = setup_module('HTTP module default config', {})
+
+		local uri = string.format('http://%s:%d', host, port)
+		check_protocol(uri, 'HTTP is enabled by default', true)
+		uri = string.format('https://%s:%d', host, port)
+		check_protocol(uri, 'HTTPS is enabled by default', true)
+
+		modules.unload('http')
+		uri = string.format('http://%s:%d', host, port)
+		check_protocol(uri, 'HTTP stops working after module unload', false)
+		uri = string.format('https://%s:%d', host, port)
+		check_protocol(uri, 'HTTPS stops working after module unload', false)
+
+	end
+
+	local function test_http_only()
+		local desc = 'HTTP-only config'
+		local host, port = setup_module(desc,
+			{
+				port = 0, -- Select random port
+				tls = false,
+			})
+
+		local uri = string.format('http://%s:%d', host, port)
+		check_protocol(uri, 'HTTP works in ' .. desc, true)
+		uri = string.format('https://%s:%d', host, port)
+		check_protocol(uri, 'HTTPS does not work in ' .. desc, false)
+	end
+
+	local function test_https_only()
+		local desc = 'HTTPS-only config'
+		local host, port = setup_module(desc,
+			{
+				port = 0, -- Select random port
+				tls = true,
+			})
+
+		local uri = string.format('http://%s:%d', host, port)
+		check_protocol(uri, 'HTTP does not work in ' .. desc, false)
+		uri = string.format('https://%s:%d', host, port)
+		check_protocol(uri, 'HTTPS works in ' .. desc, true)
+	end
+
+	local function test_custom_cert()
+		desc = 'config with custom certificate'
+		local host, port = setup_module(desc, {{
+				host = host,
+				port = port,
+				cert = 'test.crt',
+				key = 'test.key'
+			}})
+
+		uri = string.format('https://%s:%d', host, port)
+		check_protocol(uri, 'HTTPS works for ' .. desc, true)
+	end
+
+	local function test_nonexistent_cert()
+		desc = 'config with non-existing certificate file'
+		boom(http.config, {{
+				port = 0,
+				cert = '/tmp/surely_nonexistent_cert_1532432095',
+				key = 'test.key'
+			}}, desc)
+	end
+
+	local function test_nonexistent_key()
+		desc = 'config with non-existing key file'
+		boom(http.config, {{
+				port = 0,
+				cert = 'test.crt',
+				key = '/tmp/surely_nonexistent_cert_1532432095'
+			}}, desc)
+	end
+
+	local function test_missing_key_param()
+		desc = 'config with missing key= param'
+		boom(http.config, {{
+				port = 0,
+				cert = 'test.crt'
+			}}, desc)
+	end
+
+	local function test_broken_cert()
+		desc = 'config with broken file in cert= param'
+		boom(http.config, {{
+				port = 0,
+				cert = 'broken.crt',
+				key = 'test.key'
+			}}, desc)
+	end
+
+	local function test_broken_key()
+		desc = 'config with broken file in key= param'
+		boom(http.config, {{
+				port = 0,
+				cert = 'test.crt',
+				key = 'broken.key'
+			}}, desc)
+	end
+
+
+	-- plan tests
+	local tests = {
+		test_defaults,
+		test_http_only,
+		test_https_only,
+		test_custom_cert,
+		test_nonexistent_cert,
+		test_nonexistent_key,
+		test_missing_key_param,
+		test_broken_cert,
+		test_broken_key
+	}
+
+	return tests
+end