fix(sni) stricter detection and handling of duplicate snis #2285

p0pr0ck5 · 2017-03-29T03:24:42Z

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).

Fixes issue #2275, among other behavioral inconsistencies.

Tieske · 2017-03-30T13:00:03Z

if some of this is based on the unique constraint as mentioned in #2275 then there should also be a migration adding that constraint to the postgres schema. No?

p0pr0ck5 · 2017-03-30T15:10:27Z

Err. The name field is a PRIMARY KEY in postgres. Does this not make it unique?

p0pr0ck5 · 2017-03-30T16:28:33Z

if some of this is based on the unique constraint as mentioned in #2275 then there should also be a migration adding that constraint to the postgres schema. No?

Just clarifying, the point there was that the constraint is not applied to cassandra.

btw, this PR also fixes a few other misbehaviors, like creating certs and some SNI objects even when a PK violation is detected (yet still returning a 409).

Tieske · 2017-03-30T21:11:25Z

Just clarifying, the point there was that the constraint is not applied to cassandra.

then we need to add a schema change then. Right?

p0pr0ck5 · 2017-03-31T03:34:47Z

From what I can tell, the behavior seen is #2275 is a result of this PK definition for the cassandra ssl_servers_names table: https://github.com/Mashape/kong/blob/master/kong/dao/migrations/cassandra.lua#L238

Prior to this PR, we always created a new Certificate object, regardless of SNI name duplicates, which means that we never had a PK violation for cassandra when we try to insert the duplicate SNI (indeed, that's one behavior this PR fixes). So it seems the cassandra schema change here under the hood would actually be to alter the PK- which, from what I can tell, is not an option (it would involve copying and duplicating the table with the new PK- this sounds awful ;) ).

It seems that we can handle this ourselves by adding the unique flag to the schema, as discussed @Tieske. Took me a while to wrap my head around this. It does indeed seem a bit odd to add this flag to the primary_key, but given the circumstances, it seems appropriate- It does resolve the buggy behavior, with the detriment of slightly worse (but acceptable) performance on SNI insert (since the cassandra db driver has to mock the constraint behavior).

Tieske · 2017-03-31T14:16:26Z

kong/api/routes/certificates.lua

+      local seen = {}
+      if type(snis) == "table" then
+        for i = 1, #snis do
+          local sni_name = snis[i]


@p0pr0ck5 btw: why not use ipairs and implictly declare and assign sni_name? any specific reason you avoid that? (just wondering)

ipairs is a bit slower than an iterative loop, due to the function call overhead, but given the likely scope of this, it probably wouldn't make much difference either way :) let me know if this is a blocker (doesnt seem like it)

(also the existing code below follows the same pattern; maintaining consistency inside a module is important imo)

+1 for i = 1, #t

thibaultcha · 2017-03-31T20:38:43Z

kong/api/routes/certificates.lua

+      -- 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


It seems this check is extraneous here. Following the beginning of this function, I believe snis can either be nil or a table. Hence, if snis then seems simpler and reasonable enough.

thibaultcha · 2017-03-31T20:38:59Z

kong/api/routes/certificates.lua

+      local seen = {}
+      if type(snis) == "table" then
+        for i = 1, #snis do
+          local sni_name = snis[i]


+1 for i = 1, #t

thibaultcha · 2017-03-31T20:42:21Z

kong/api/routes/certificates.lua

+      -- 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 = {}


I believe this seen table does not have to belong to this scope, but can be scoped under the if snis then block?

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 <thibaultcha@me.com>

* new test to enforce the uniqueness of a SNI by its name * run `/certificates` and `/snis` tests with both PostgreSQL and Cassandra.

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.

p0pr0ck5 added the pr/status/needs review label Mar 29, 2017

p0pr0ck5 force-pushed the fix/sni-conflict branch from a61a5ff to 6572e00 Compare March 31, 2017 03:36

Tieske reviewed Mar 31, 2017

View reviewed changes

thibaultcha reviewed Mar 31, 2017

View reviewed changes

thibaultcha force-pushed the fix/sni-conflict branch 2 times, most recently from 6a92e26 to 6da94a8 Compare April 3, 2017 20:53

thibaultcha changed the base branch from next to master April 3, 2017 20:53

p0pr0ck5 and others added 2 commits April 3, 2017 13:57

test(sni) run on both DBs + enforce uniqueness

64a66b4

* new test to enforce the uniqueness of a SNI by its name * run `/certificates` and `/snis` tests with both PostgreSQL and Cassandra.

thibaultcha force-pushed the fix/sni-conflict branch from 6da94a8 to 64a66b4 Compare April 3, 2017 20:59

thibaultcha approved these changes Apr 3, 2017

View reviewed changes

thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/needs review labels Apr 3, 2017

refactor(sni) simpler idioms for /snis endpoint

d1b196d

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.

thibaultcha force-pushed the fix/sni-conflict branch from 60aa903 to d1b196d Compare April 3, 2017 21:31

thibaultcha merged commit 57ad9ff into master Apr 3, 2017

thibaultcha deleted the fix/sni-conflict branch April 3, 2017 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sni) stricter detection and handling of duplicate snis #2285

fix(sni) stricter detection and handling of duplicate snis #2285

p0pr0ck5 commented Mar 29, 2017 •

edited

Loading

Tieske commented Mar 30, 2017

p0pr0ck5 commented Mar 30, 2017

p0pr0ck5 commented Mar 30, 2017 •

edited

Loading

Tieske commented Mar 30, 2017

p0pr0ck5 commented Mar 31, 2017

Tieske Mar 31, 2017

p0pr0ck5 Mar 31, 2017

p0pr0ck5 Mar 31, 2017

thibaultcha Mar 31, 2017

thibaultcha Mar 31, 2017

thibaultcha Mar 31, 2017

thibaultcha Mar 31, 2017

fix(sni) stricter detection and handling of duplicate snis #2285

fix(sni) stricter detection and handling of duplicate snis #2285

Conversation

p0pr0ck5 commented Mar 29, 2017 • edited Loading

Tieske commented Mar 30, 2017

p0pr0ck5 commented Mar 30, 2017

p0pr0ck5 commented Mar 30, 2017 • edited Loading

Tieske commented Mar 30, 2017

p0pr0ck5 commented Mar 31, 2017

Tieske Mar 31, 2017

Choose a reason for hiding this comment

p0pr0ck5 Mar 31, 2017

Choose a reason for hiding this comment

p0pr0ck5 Mar 31, 2017

Choose a reason for hiding this comment

thibaultcha Mar 31, 2017

Choose a reason for hiding this comment

thibaultcha Mar 31, 2017

Choose a reason for hiding this comment

thibaultcha Mar 31, 2017

Choose a reason for hiding this comment

thibaultcha Mar 31, 2017

Choose a reason for hiding this comment

p0pr0ck5 commented Mar 29, 2017 •

edited

Loading

p0pr0ck5 commented Mar 30, 2017 •

edited

Loading