feat(oauth2) authenticate consumer on token provision

[jjustus2]

Summary

Old Related PR: #4197

Would really like this functionality to make it into mainstream Kong so I can stop maintaining a side patch file of access.lua I drop into our code every time to achieve this enhanced functionality for token generation around finding which customers abuse token generation. Cool with renaming functions or w/e is necessary too. Unsure if the unit test will work(didn't test it, but I know the code works currently as we run it in production).

Full changelog

• Implement authenticating the consumer during the token provision process which helps with logging analytics on who generates too many tokens.
• Validate the consumer context post token generation in unit tests using the kong.client pdk.

Issues resolved

Fix #5958

[jjustus2]

Arg... I really need to learn how to run the unit tests locally:

     __________
        FAILED  spec/03-plugins/25-oauth2/03-access_spec.lua:2603: Plugin: oauth2 [#cassandra] access Making a request works when a correct access_token is being sent in the querystring
spec/03-plugins/25-oauth2/03-access_spec.lua:88: Expected objects to be the same.
Passed in:
(nil)
Expected:
type table
stack traceback:
	spec/03-plugins/25-oauth2/03-access_spec.lua:88: in function 'provision_token'
	spec/03-plugins/25-oauth2/03-access_spec.lua:2604: in function <spec/03-plugins/25-oauth2/03-access_spec.lua:2603>

Will try to fix this up, I imagine an if statement around when this token_provision() call is made there is one odd edge case here.

Meh trying to build it locally on mac, seems I have lua 5.3 and need 5.1 might be why I got this error:

luabitop 1.0.1-1 depends on lua >= 5.1 (5.3-1 provided by VM)
Do not use 'module' as a build type. Use 'builtin' instead.
env MACOSX_DEPLOYMENT_TARGET=10.8 gcc -O2 -fPIC -I/usr/local/opt/lua/include/lua5.3 -c bit.c -o bit.o
bit.c:79:2: error: "Unknown number type, check LUA_NUMBER_* in luaconf.h"
#error "Unknown number type, check LUA_NUMBER_* in luaconf.h"
 ^
bit.c:82:5: warning: implicit declaration of function 'luaL_typerror' is invalid
      in C99 [-Wimplicit-function-declaration]
    luaL_typerror(L, idx, "number");
    ^
bit.c:175:3: warning: implicit declaration of function 'luaL_register' is
      invalid in C99 [-Wimplicit-function-declaration]
  luaL_register(L, "bit", bit_funcs);
  ^
2 warnings and 1 error generated.

Honestly faster for me to commit here and watch the Kong pipeline go so yeah gonna do that until all systems go.

[jjustus2]

Running into the same problems as last time when I tried to deliver this feature. @Kong/team-core anyone know why trying to use the pdk kong.client seems to not function in the context of unit tests when runtime functional testing you would see my changes to the access.lua code do indeed work? I even see kong.table used in this unit test without issue, and its not the call doesn't work, (kong.client.get_consumer() must be returning nil vs the expected table, which can't be the case in a real world test, if a token is generated successfully on an /oauth2/token call then the associated consumer to those creds would be authenticated in the context and logged in the http logging and other log plugins appropriately for that network call.

cc @kikito @bungle @hishamhm if yall know? Or if you have an alt. approach idea to what I am doing that does not involve the pdk?

Edit - glancing at the lazy setup(), certainly seems a consumer is associated to these creds which is how real world Kong works(bob is our consumer for the clientid123 / secret123 used in the unit test check I try to get the consumer auth context):

      local consumer = admin_api.consumers:insert {
        username = "bob"
      }

      local anonymous_user = admin_api.consumers:insert {
        username = "no-body"
      }

      client1 = admin_api.oauth2_credentials:insert {
        client_id      = "clientid123",
        client_secret  = "secret123",
        hash_secret    = true,
        redirect_uris  = { "http://google.com/kong" },
        name           = "testapp",
        consumer       = { id = consumer.id },
      }

So I think there has to be a deficiency in the unit testing framework around some uses of the pdk right?

[jeremyjpj0916]

@thibaultcha you see what I mean in this PR? Am I off base thinking something in the pdk in the context of unit testing does not behave as it would during standard kong runtime? Using pdk to get the consumer context should return bob here not nil. The modifications to the oauth2 access.lua plugin works in the wild too for us which gives me further confidence that this is the case.

[codifierr]

@thibaultcha @jeremyjpj0916 Is there a way to rate-limit this endpoint "/oauth2/token" via a rate-limiting plugin maybe?

[jeremyjpj0916]

@thibaultcha @jeremyjpj0916 Is there a way to rate-limit this endpoint "/oauth2/token" via a rate-limiting plugin maybe?

@codifierr

Complexity there is when you have LB's in-front of Kong and no sure way to get originating client IP(if rate limiting by IP). On the flip side if you wanted to rate limit by consumer other issue is not knowing who the consumer is at token gen time(this PR idea is to solve it for logging purposes at least), and if that logic was added I would want a way to rate limit them once the consumer is known(well client_id and client_secret I think is best way to rate limit by, don't wanna see to many of the same cred pair used too frequently), and prior to injecting a fresh token value in the db so DB r/w could be prevented. Would certainly be a good thing.

Another way we have internally solved token gen abuse delimas is we made this "cached" token gen endpoint, that if a client calls the token gen endpoint prior to a valid token they have expiry, it will return them the same token and its ever decreasing ttl(preventing the need for DB r/w). Might be something we could also PR sometime but dunno if Kong would be cool with it since its probably not "RFC" behavior, but I care more about a stable offering that is protective in nature than appealing to an RFC if I have to haha.

Hopefully I can get back to this PR and fix up the test case like Tibo mentioned. Have not had time to dig into it just yet, other big items on my plate internally at the moment.

[codifierr]

@thibaultcha @jeremyjpj0916 Is there a way to rate-limit this endpoint "/oauth2/token" via a rate-limiting plugin maybe?

@codifierr

Complexity there is when you have LB's in-front of Kong and no sure way to get originating client IP(if rate limiting by IP). On the flip side if you wanted to rate limit by consumer other issue is not knowing who the consumer is at token gen time(this PR idea is to solve it for logging purposes at least), and if that logic was added I would want a way to rate limit them once the consumer is known(well client_id and client_secret I think is best way to rate limit by, don't wanna see to many of the same cred pair used too frequently), and prior to injecting a fresh token value in the db so DB r/w could be prevented. Would certainly be a good thing.

Another way we have internally solved token gen abuse delimas is we made this "cached" token gen endpoint, that if a client calls the token gen endpoint prior to a valid token they have expiry, it will return them the same token and its ever decreasing ttl(preventing the need for DB r/w). Might be something we could also PR sometime but dunno if Kong would be cool with it since its probably not "RFC" behavior, but I care more about a stable offering that is protective in nature than appealing to an RFC if I have to haha.

Hopefully I can get back to this PR and fix up the test case like Tibo mentioned. Have not had time to dig into it just yet, other big items on my plate internally at the moment.

@jeremyjpj0916 I am actually looking for rate-limiting just by client-id and client-secret which is a term known as rate-limit by credentials. I tried to do that. But looks like the rate-limit plugin only executes if the consumer is authenticated or it never executes on this API could be because it is not an actual API or route for upstream not sure about that. In our env, we have seen customers abusing this API bringing too much load on our DB. I have tried the second one which replays the previous access and refresh token. This will be an interim solution for me to protect infra. Till I come up with something better. Meanwhile, I think with a config of oauth2 which defines time interval in which the oauth2 user can use refresh token API could be done. Within interval, Kong can send back the same access and refresh token to protect against infra abuse.

[hbagdi]

Closing because of lack of activity. We would like to accept this contribution if anyone wishes to pick this back up.