feat(key-auth) support auto-expiring keys

[p0pr0ck5]

Native support in the DAO makes supporting this straightforward :)

• Add ttl = true to keyauth credentials schema
• Add migration for Postgres to add TTL column
• Add API spec for added behavior

Fix #87

[thibaultcha]

Nice :)

[thibaultcha]

I think this needs a little rebasing as well, but then I see no reason why not merging this for our next release!

[thibaultcha]

(Would fix myself if it weren't for the rebasing)

[p0pr0ck5]

@thibaultcha fixed up! (annoying, git rebasing was clean but GitHub just didn't wanna do it...)

[thibaultcha]

I just pushed a test case that demonstrates that this feature isn't ready. Unfortunately, the cache will hold the API key indefinitely. We could return the TTL from the mlcache callback, but said TTL is not returned by the DAO as of today. We should contribute a change to the DAO that returns the TTL if it is present (cc @bungle).

[thibaultcha]

Need to make the integration test pass (requires DAO updates)

[subnetmarco]

Note: It would be nice to have this for all the other authentication plugins too.

[p0pr0ck5]

@thibaultcha do oauth2_tokens entities suffer from the same flaw?

[p0pr0ck5]

(The reason I ask is oauth2 plugin DAO entities are the only bundled entities that define ttl = true like we do in this PR, and they follow the same access pattern in the cache (with a nil opts table in the cache :get call) that's presented here).

[subnetmarco]

@thibaultcha any updates?

[jeremyjpj0916]

Note: It would be nice to have this for all the other authentication plugins too.

Do auto-expiring keys also clean up within the db or just sit out there nilled out? Based on a glance at the code I don't see any ttl checks/ or a column being nil causing a delete on all expired records.

If they are actually pruned from db rather than sitting out there in Kongs C*/Postgres db that would be neat, I think most folks who run Kong use some variant of auth cleanup scripts for ttl auth like:
https://github.com/Optum/kong-expired-token-cleanup

Would be pretty nifty for Kong to self-prune its db on expired worthless auth records(keys/oauth2 bearer tokens etc) if its in scope of this PR. Maybe this PR is just about allowing the functionality of an "expired" auth-key, in which case future enhancements down the road for smarter Kong cleanups would be neat from user experience! Could retire external cleanup scripts and let Kong self-manage.

[thibaultcha]

@subnetmarco There are no updates due from my side.

@jeremyjpj0916 Our new DAO (since 0.13/1.0) has many, many benefits. Including cleaning up rows with expired TTLs: https://github.com/Kong/kong/blob/master/kong/db/strategies/postgres/connector.lua#L280-L293. Of course this requires that entities be updated to make use of the new DAO's TTL feature.

With Cassandra, deleting expired TTL rows will only happen during compaction, which is outside of the scope of Kong, and closer to the realm of database administration.

[subnetmarco]

@jeremyjpj0916, what version of Kong are you using?

[p0pr0ck5]

To clarify, the blocker to merge here is @thibaultcha's comment:

I just pushed a test case that demonstrates that this feature isn't ready. Unfortunately, the cache will hold the API key indefinitely. We could return the TTL from the mlcache callback, but said TTL is not returned by the DAO as of today. We should contribute a change to the DAO that returns the TTL if it is present (cc @bungle).

Making this change is outside the scope of this PR (it requires a change to the DAO, and I've not had time on my side to work on this). The question/blocker here is not related to whether or not TTL'd rows are automatically deleted

Also @thibaultcha it would be great if you can validate/refute my question here: #4239 (comment)

[thibaultcha]

@p0pr0ck5 Sorry I missed this.

@thibaultcha do oauth2_tokens entities suffer from the same flaw?

They are not, since the database-level TTL in this case is more of an optimization than a feature (to cleanup the DB appropriately, as per the aside discussion raised by this thread). Checking for token expiration as a feature was always implemented in the business logic, iirc. As of today after fetching the token, the plugin's logic takes care of checking its expiration date.

[p0pr0ck5]

the database-level TTL in this case is more of an optimization than a feature

Given this, does the approach in this PR even make sense (once the DAO is modified to support the use case)?

[thibaultcha]

@p0pr0ck5 For the use-case we have in mind (bluntly deleting credentials as they reach their TTL), this approach is leaner and cleaner imho. It takes direct advantage of both DAO-native and mlcache TTL support so that we do not have to implement expiration checks in the plugin's business logic.

However, doing so allows for distinguishing an expired token from a non-existent one, which can be useful for oauth2 and key-auth, but it depends on our use-cases. For example, if we were to not delete credentials when they expire, but simply invalidating them and relying on an administrator to refresh then, or remove them entirely. But that is not the use-case this plugin is going for, and I never heard of such an ask so far.

[subnetmarco]

Do we have any action items on this PR?

[p0pr0ck5]

@subnetmarco the action item is, Kong's DAO needs to return the TTL for the objects returned from the DB, so we can cache them for the appropriate amount of time. I do not have the bandwidth to work on the plumbing in the DAO at this point- I don't suspect it will be a large effort, but it won't be able to pick it up personally in the near future.

[CLAassistant]

All committers have signed the CLA.

[eskerda]

@p0pr0ck5 @thibaultcha Added support for DAO returning TTL on select queries (on postgres only). About this approach:

Pros

• Very light, and works as expected

Cons

• 403 != 401
• Adding TTL on select queries for cassandra will not be as simple

After discussion with @bungle, the other option could be adding a ttl function directly on DAO, for both cassandra and postgres, and getting the TTL on another step, ie:
local ttl = kong.db.keyauth_credentials:ttl(id)

EDIT: After seeing https://travis-ci.org/Kong/kong/jobs/538783843#L1556 it seems adding ttl on the select queries is not such a good idea.

[thibaultcha]

@eskerda Have a look at https://docs.datastax.com/en/cql/3.3/cql/cql_reference/cqlSelect.html to see what we could do there (watch out, for we also need to ensure whatever we do for C* also works for C* 2.2). For Postgres, not all tables have a ttl column, but you can assume that if the schema of an entity specifies ttl, then it has the column in its table definition.

[hishamhm]

We're closing this one for now, as clearly there are pending blockers in the DAO to allow this approach to be viable.