From 06d084f5f86bb3b4dfc3b0d8cb1dbf61f6fda75c Mon Sep 17 00:00:00 2001 From: Robert Paprocki Date: Tue, 28 Mar 2017 18:10:57 -0700 Subject: [PATCH 1/3] fix(sni) stricter detection of duplicate SNIs Previously, if a duplicate SNI was detected as part of the 'snis' sugar param, we still created the certificate object, as well as any previously non-existant sni encountered before the conflicting element. This commit prevents creation of orphan entries by checking for the existence all posted snis before creating the certificate and sni objects. This commit also cleans up the naming schemes for admin API certificate routes integration tests (no functional change). Signed-off-by: Thibault Charbonnier --- kong/api/routes/certificates.lua | 32 +++ kong/dao/schemas/ssl_servers_names.lua | 2 +- .../07-certificates_routes_spec.lua | 241 ++++++++++++------ 3 files changed, 200 insertions(+), 75 deletions(-) diff --git a/kong/api/routes/certificates.lua b/kong/api/routes/certificates.lua index 6cb45e8a6cf..ad9f7dff4fe 100644 --- a/kong/api/routes/certificates.lua +++ b/kong/api/routes/certificates.lua @@ -12,6 +12,38 @@ return { self.params.snis = nil end + -- dont add the certificate or any snis if we have an SNI conflict + -- its fairly inefficient that we have to loop twice over the datastore + -- but no support for OR queries means we gotsta! + if snis then + local seen = {} + + for i = 1, #snis do + local sni_name = snis[i] + + if seen[sni_name] then + return helpers.responses.send_HTTP_CONFLICT( + "duplicate requested sni name " .. sni_name + ) + end + + local cnt, err = dao_factory.ssl_servers_names:count({ + name = sni_name, + }) + if err then + return helpers.yield_error(err) + end + + if cnt > 0 then + return helpers.responses.send_HTTP_CONFLICT( + "entry already exists with name " .. sni_name + ) + end + + seen[sni_name] = true + end + end + local ssl_cert, err = dao_factory.ssl_certificates:insert(self.params) if err then return helpers.yield_error(err) diff --git a/kong/dao/schemas/ssl_servers_names.lua b/kong/dao/schemas/ssl_servers_names.lua index 2d67a1c3263..b37522cb5b0 100644 --- a/kong/dao/schemas/ssl_servers_names.lua +++ b/kong/dao/schemas/ssl_servers_names.lua @@ -2,7 +2,7 @@ return { table = "ssl_servers_names", primary_key = { "name" }, fields = { - name = { type = "text", required = true }, + name = { type = "text", required = true, unique = true }, ssl_certificate_id = { type = "id", foreign = "ssl_certificates:id" }, created_at = { type = "timestamp", diff --git a/spec/02-integration/03-admin_api/07-certificates_routes_spec.lua b/spec/02-integration/03-admin_api/07-certificates_routes_spec.lua index 392730271b9..bc18a675ab2 100644 --- a/spec/02-integration/03-admin_api/07-certificates_routes_spec.lua +++ b/spec/02-integration/03-admin_api/07-certificates_routes_spec.lua @@ -31,6 +31,99 @@ describe("Admin API", function() helpers.dao:truncate_tables() end) + it("returns a conflict when duplicates snis are present in the request", function() + local res = assert(client:send { + method = "POST", + path = "/certificates", + body = { + cert = ssl_fixtures.cert, + key = ssl_fixtures.key, + snis = "foo.com,bar.com,foo.com", + }, + headers = { ["Content-Type"] = "application/x-www-form-urlencoded" }, + }) + + local body = assert.res_status(409, res) + local json = cjson.decode(body) + assert.equals('duplicate requested sni name foo.com', json.message) + + -- make sure we dont add any snis + res = assert(client:send { + method = "GET", + path = "/snis", + }) + + body = assert.res_status(200, res) + json = cjson.decode(body) + assert.equal(0, #json.data) + assert.equal(0, json.total) + + -- make sure we didnt add the certificate + res = assert(client:send { + method = "GET", + path = "/certificates", + }) + + body = assert.res_status(200, res) + json = cjson.decode(body) + assert.equal(0, #json) + end) + + it("returns a conflict when a pre-existing sni is detected", function() + local res = assert(client:send { + method = "POST", + path = "/certificates", + body = { + cert = ssl_fixtures.cert, + key = ssl_fixtures.key, + snis = "foo.com", + }, + headers = { ["Content-Type"] = "application/x-www-form-urlencoded" }, + }) + + assert.res_status(201, res) + + res = assert(client:send { + method = "POST", + path = "/certificates", + body = { + cert = ssl_fixtures.cert, + key = ssl_fixtures.key, + snis = "foo.com,bar.com", + }, + headers = { ["Content-Type"] = "application/x-www-form-urlencoded" }, + }) + + local body = assert.res_status(409, res) + local json = cjson.decode(body) + assert.equals('entry already exists with name foo.com', json.message) + + -- make sure we only have one sni + res = assert(client:send { + method = "GET", + path = "/snis", + }) + + body = assert.res_status(200, res) + json = cjson.decode(body) + assert.equal(1, #json.data) + assert.equal(1, json.total) + assert.equal("foo.com", json.data[1].name) + + -- make sure we only have one certificate + res = assert(client:send { + method = "GET", + path = "/certificates", + }) + + body = assert.res_status(200, res) + json = cjson.decode(body) + assert.equal(1, #json) + assert.is_string(json[1].cert) + assert.is_string(json[1].key) + assert.same({ "foo.com" }, json[1].snis) + end) + it_content_types("creates a certificate", function(content_type) return function() local res = assert(client:send { @@ -68,99 +161,99 @@ describe("Admin API", function() assert.same({ "foo.com", "bar.com" }, json[1].snis) end) end) + end) - describe("/certificates/:sni_or_uuid", function() - - describe("GET", function() - it("retrieves a certificate by SNI", function() - local res1 = assert(client:send { - method = "GET", - path = "/certificates/foo.com", - }) + describe("/certificates/:sni_or_uuid", function() - local body1 = assert.res_status(200, res1) - local json1 = cjson.decode(body1) + describe("GET", function() + it("retrieves a certificate by SNI", function() + local res1 = assert(client:send { + method = "GET", + path = "/certificates/foo.com", + }) - local res2 = assert(client:send { - method = "GET", - path = "/certificates/bar.com", - }) + local body1 = assert.res_status(200, res1) + local json1 = cjson.decode(body1) - local body2 = assert.res_status(200, res2) - local json2 = cjson.decode(body2) + local res2 = assert(client:send { + method = "GET", + path = "/certificates/bar.com", + }) - assert.is_string(json1.cert) - assert.is_string(json1.key) - assert.same({ "foo.com", "bar.com" }, json1.snis) - assert.same(json1, json2) - end) - end) + local body2 = assert.res_status(200, res2) + local json2 = cjson.decode(body2) - describe("PATCH", function() - it_content_types("updates a certificate by SNI", function(content_type) - return function() - local res = assert(client:send { - method = "PATCH", - path = "/certificates/foo.com", - body = { - cert = content_type - }, - headers = { ["Content-Type"] = content_type } - }) - - local body = assert.res_status(200, res) - local json = cjson.decode(body) - - assert.equal(content_type, json.cert) - end - end) + assert.is_string(json1.cert) + assert.is_string(json1.key) + assert.same({ "foo.com", "bar.com" }, json1.snis) + assert.same(json1, json2) end) + end) - describe("DELETE", function() - it("deletes a certificate and all related SNIs", function() + describe("PATCH", function() + it_content_types("updates a certificate by SNI", function(content_type) + return function() local res = assert(client:send { - method = "DELETE", - path = "/certificates/foo.com", + method = "PATCH", + path = "/certificates/foo.com", + body = { + cert = content_type + }, + headers = { ["Content-Type"] = content_type } }) - assert.res_status(204, res) + local body = assert.res_status(200, res) + local json = cjson.decode(body) - res = assert(client:send { - method = "GET", - path = "/certificates/foo.com", - }) + assert.equal(content_type, json.cert) + end + end) + end) - assert.res_status(404, res) + describe("DELETE", function() + it("deletes a certificate and all related SNIs", function() + local res = assert(client:send { + method = "DELETE", + path = "/certificates/foo.com", + }) - res = assert(client:send { - method = "GET", - path = "/certificates/bar.com", - }) + assert.res_status(204, res) - assert.res_status(404, res) - end) + res = assert(client:send { + method = "GET", + path = "/certificates/foo.com", + }) - it("deletes a certificate by id", function() - local res = assert(client:send { - method = "POST", - path = "/certificates", - body = { - cert = "foo", - key = "bar", - }, - headers = { ["Content-Type"] = "application/json" } - }) + assert.res_status(404, res) - local body = assert.res_status(201, res) - local json = cjson.decode(body) + res = assert(client:send { + method = "GET", + path = "/certificates/bar.com", + }) - res = assert(client:send { - method = "DELETE", - path = "/certificates/" .. json.id, - }) + assert.res_status(404, res) + end) - assert.res_status(204, res) - end) + it("deletes a certificate by id", function() + local res = assert(client:send { + method = "POST", + path = "/certificates", + body = { + cert = "foo", + key = "bar", + }, + headers = { ["Content-Type"] = "application/json" } + }) + + local body = assert.res_status(201, res) + local json = cjson.decode(body) + + res = assert(client:send { + method = "DELETE", + path = "/certificates/" .. json.id, + }) + + assert.res_status(204, res) end) end) end) From 64a66b409cc4d46cb887544e831c67a03e141352 Mon Sep 17 00:00:00 2001 From: Thibault Charbonnier Date: Mon, 3 Apr 2017 13:48:57 -0700 Subject: [PATCH 2/3] test(sni) run on both DBs + enforce uniqueness * new test to enforce the uniqueness of a SNI by its name * run `/certificates` and `/snis` tests with both PostgreSQL and Cassandra. --- .../07-certificates_routes_spec.lua | 95 ++++++++++++++----- 1 file changed, 72 insertions(+), 23 deletions(-) diff --git a/spec/02-integration/03-admin_api/07-certificates_routes_spec.lua b/spec/02-integration/03-admin_api/07-certificates_routes_spec.lua index bc18a675ab2..b85326b6931 100644 --- a/spec/02-integration/03-admin_api/07-certificates_routes_spec.lua +++ b/spec/02-integration/03-admin_api/07-certificates_routes_spec.lua @@ -1,4 +1,6 @@ local ssl_fixtures = require "spec.fixtures.ssl" +local dao_helpers = require "spec.02-integration.02-dao.helpers" +local DAOFactory = require "kong.dao.factory" local helpers = require "spec.helpers" local cjson = require "cjson" @@ -11,24 +13,35 @@ local function it_content_types(title, fn) end +dao_helpers.for_each_dao(function(kong_config) + describe("Admin API", function() local client + local dao setup(function() - assert(helpers.start_kong()) + dao = assert(DAOFactory.new(kong_config)) + + assert(helpers.start_kong({ + database = kong_config.database + })) + client = assert(helpers.admin_client()) end) teardown(function() - if client then client:close() end + if client then + client:close() + end + helpers.stop_kong() end) - describe("/certificates", function() + describe("/certificates with " .. kong_config.database, function() describe("POST", function() before_each(function() - helpers.dao:truncate_tables() + dao:truncate_tables() end) it("returns a conflict when duplicates snis are present in the request", function() @@ -96,7 +109,7 @@ describe("Admin API", function() local body = assert.res_status(409, res) local json = cjson.decode(body) - assert.equals('entry already exists with name foo.com', json.message) + assert.equals("entry already exists with name foo.com", json.message) -- make sure we only have one sni res = assert(client:send { @@ -259,14 +272,14 @@ describe("Admin API", function() end) - describe("/snis", function() + describe("/snis with " .. kong_config.database, function() local ssl_certificate describe("POST", function() before_each(function() - helpers.dao:truncate_tables() + dao:truncate_tables() - ssl_certificate = assert(helpers.dao.ssl_certificates:insert { + ssl_certificate = assert(dao.ssl_certificates:insert { cert = ssl_fixtures.cert, key = ssl_fixtures.key, }) @@ -308,6 +321,27 @@ describe("Admin API", function() assert.equal(ssl_certificate.id, json.ssl_certificate_id) end end) + + it("returns a conflict when an SNI already exists", function() + assert(dao.ssl_servers_names:insert { + name = "foo.com", + ssl_certificate_id = ssl_certificate.id, + }) + + local res = assert(client:send { + method = "POST", + path = "/snis", + body = { + name = "foo.com", + ssl_certificate_id = ssl_certificate.id, + }, + headers = { ["Content-Type"] = "application/json" }, + }) + + local body = assert.res_status(409, res) + local json = cjson.decode(body) + assert.equals("already exists with value 'foo.com'", json.name) + end) end) describe("GET", function() @@ -346,26 +380,39 @@ describe("Admin API", function() local ssl_certificate_2 setup(function() - ssl_certificate_2 = assert(helpers.dao.ssl_certificates:insert { + ssl_certificate_2 = assert(dao.ssl_certificates:insert { cert = "foo", key = "bar", }) end) - it("updates a SNI", function() - local res = assert(client:send { - method = "PATCH", - path = "/snis/foo.com", - body = { - ssl_certificate_id = ssl_certificate_2.id, - }, - headers = { ["Content-Type"] = "application/json" }, - }) - - local body = assert.res_status(200, res) - local json = cjson.decode(body) - assert.equal(ssl_certificate_2.id, json.ssl_certificate_id) - end) + do + local test = it + if kong_config.database == "cassandra" then + test = pending + end + + test("updates a SNI", function() + -- SKIP: this test fails with Cassandra because the PRIMARY KEY + -- used by the C* table is a composite of (name, + -- ssl_certificate_id), and hence, we cannot update the + -- ssl_certificate_id field because it is in the `SET` part of the + -- query built by the DAO, but in C*, one cannot change a value + -- from the clustering key. + local res = assert(client:send { + method = "PATCH", + path = "/snis/foo.com", + body = { + ssl_certificate_id = ssl_certificate_2.id, + }, + headers = { ["Content-Type"] = "application/json" }, + }) + + local body = assert.res_status(200, res) + local json = cjson.decode(body) + assert.equal(ssl_certificate_2.id, json.ssl_certificate_id) + end) + end end) describe("DELETE", function() @@ -381,3 +428,5 @@ describe("Admin API", function() end) end) end) + +end) From d1b196dbc8d233e546cdfb79e09f4b2443be8a45 Mon Sep 17 00:00:00 2001 From: Thibault Charbonnier Date: Mon, 3 Apr 2017 14:29:07 -0700 Subject: [PATCH 3/3] refactor(sni) simpler idioms for /snis endpoint In such cold code paths, we can make the code more readable. Let's also not call SNIs "sni_name" since "sni" already carries this meaning. --- kong/api/routes/certificates.lua | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/kong/api/routes/certificates.lua b/kong/api/routes/certificates.lua index ad9f7dff4fe..3c95668d782 100644 --- a/kong/api/routes/certificates.lua +++ b/kong/api/routes/certificates.lua @@ -12,23 +12,21 @@ return { self.params.snis = nil end - -- dont add the certificate or any snis if we have an SNI conflict - -- its fairly inefficient that we have to loop twice over the datastore - -- but no support for OR queries means we gotsta! if snis then + -- dont add the certificate or any snis if we have an SNI conflict + -- its fairly inefficient that we have to loop twice over the datastore + -- but no support for OR queries means we gotsta! local seen = {} - for i = 1, #snis do - local sni_name = snis[i] - - if seen[sni_name] then + for _, sni in ipairs(snis) do + if seen[sni] then return helpers.responses.send_HTTP_CONFLICT( - "duplicate requested sni name " .. sni_name + "duplicate requested sni name " .. sni ) end local cnt, err = dao_factory.ssl_servers_names:count({ - name = sni_name, + name = sni, }) if err then return helpers.yield_error(err) @@ -36,11 +34,11 @@ return { if cnt > 0 then return helpers.responses.send_HTTP_CONFLICT( - "entry already exists with name " .. sni_name + "entry already exists with name " .. sni ) end - seen[sni_name] = true + seen[sni] = true end end @@ -51,12 +49,12 @@ return { -- insert SNIs if given - if type(snis) == "table" then + if snis then ssl_cert.snis = {} - for i = 1, #snis do + for _, sni in ipairs(snis) do local ssl_server_name = { - name = snis[i], + name = sni, ssl_certificate_id = ssl_cert.id, }