diff --git a/kong/api/routes/certificates.lua b/kong/api/routes/certificates.lua index 6cb45e8a6cf..3c95668d782 100644 --- a/kong/api/routes/certificates.lua +++ b/kong/api/routes/certificates.lua @@ -12,6 +12,36 @@ return { self.params.snis = nil end + 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 _, sni in ipairs(snis) do + if seen[sni] then + return helpers.responses.send_HTTP_CONFLICT( + "duplicate requested sni name " .. sni + ) + end + + local cnt, err = dao_factory.ssl_servers_names:count({ + name = sni, + }) + 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 + ) + end + + seen[sni] = true + end + end + local ssl_cert, err = dao_factory.ssl_certificates:insert(self.params) if err then return helpers.yield_error(err) @@ -19,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, } 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..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,128 @@ 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() + 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) @@ -68,112 +174,112 @@ describe("Admin API", function() assert.same({ "foo.com", "bar.com" }, json[1].snis) end) end) + end) - describe("/certificates/:sni_or_uuid", function() + 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("GET", function() + it("retrieves a certificate by SNI", function() + local res1 = assert(client:send { + method = "GET", + path = "/certificates/foo.com", + }) - local body1 = assert.res_status(200, res1) - local json1 = cjson.decode(body1) + local body1 = assert.res_status(200, res1) + local json1 = cjson.decode(body1) - local res2 = assert(client:send { - method = "GET", - path = "/certificates/bar.com", - }) + local res2 = assert(client:send { + method = "GET", + path = "/certificates/bar.com", + }) - local body2 = assert.res_status(200, res2) - local json2 = cjson.decode(body2) + local body2 = assert.res_status(200, res2) + local json2 = cjson.decode(body2) - assert.is_string(json1.cert) - assert.is_string(json1.key) - assert.same({ "foo.com", "bar.com" }, json1.snis) - assert.same(json1, json2) - 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("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 } - }) + 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) + local body = assert.res_status(200, res) + local json = cjson.decode(body) - assert.equal(content_type, json.cert) - end - end) + assert.equal(content_type, json.cert) + end end) + end) - describe("DELETE", function() - it("deletes a certificate and all related SNIs", function() - local res = assert(client:send { - method = "DELETE", - path = "/certificates/foo.com", - }) + describe("DELETE", function() + it("deletes a certificate and all related SNIs", function() + local res = assert(client:send { + method = "DELETE", + path = "/certificates/foo.com", + }) - assert.res_status(204, res) + assert.res_status(204, res) - res = assert(client:send { - method = "GET", - path = "/certificates/foo.com", - }) + res = assert(client:send { + method = "GET", + path = "/certificates/foo.com", + }) - assert.res_status(404, res) + assert.res_status(404, res) - res = assert(client:send { - method = "GET", - path = "/certificates/bar.com", - }) + res = assert(client:send { + method = "GET", + path = "/certificates/bar.com", + }) - assert.res_status(404, res) - end) + assert.res_status(404, 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" } - }) + 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) + local body = assert.res_status(201, res) + local json = cjson.decode(body) - res = assert(client:send { - method = "DELETE", - path = "/certificates/" .. json.id, - }) + res = assert(client:send { + method = "DELETE", + path = "/certificates/" .. json.id, + }) - assert.res_status(204, res) - end) + assert.res_status(204, res) end) end) 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, }) @@ -215,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() @@ -253,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" }, - }) + do + local test = it + if kong_config.database == "cassandra" then + test = pending + end - local body = assert.res_status(200, res) - local json = cjson.decode(body) - assert.equal(ssl_certificate_2.id, json.ssl_certificate_id) - 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() @@ -288,3 +428,5 @@ describe("Admin API", function() end) end) end) + +end)