fix(sni) stricter detection and handling 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).

kong/api/routes/certificates.lua

@@ -12,6 +12,37 @@ 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!
+      local seen = {}
+      if type(snis) == "table" then
+        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)

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)