Adding support of private_key_jwt for authentication to OP

[pamiel]

The objective of this PR is to support the “private_key_jwt” authentication mode when accessing to the OP Token Endpoint.

Instead of providing the client_id /client_secret credentials as configuration data of the lua-resty-openidc library, the client_id /client_rsa_private_key shall be provided. client_rsa_private_key is a RSA private key in PEM format.
Usage of the “private_key_jwt” authentication mode shall then be requested by setting the "private_key_jwt" string value into the token_endpoint_auth_method configuration parameter.

When doing so, lua-resty-openidc will generate a JWT token and sign it with the RSA private key provided above. This signed certificate then acts as credential of the client to the OP.
The format of the JWT (i.e. the claims and their content) follows the specification https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication. An additional key id can optionally be specified through the client_rsa_private_key_id configuration data, in order to be set in the JWT.
This PR indeed partially addresses issue #131 as it supports the “private_key_jwt” authentication mode (item 1.), but it does not address the other points mentioned in this Issue (my feeling is that this is not really the responsibility of lua-resty-openidc library to manage these other points).

Computation of a RSA signature is far more time-consuming than simply using a client_secret for the other authentication modes. In order to counter-balance the performance decrease of the “private_key_jwt” authentication mode, the signed JWTs are cached for reuse (for requests made for the same OP, client, key and key_id). Default life duration of the JWT in the cache is 1 hour but can be overwritten through the configuration parameter client_jwt_assertion_expires_in (value is expressed in seconds; default value is 60 * 60 = 1h; value of 0 means: not cached).
Note that the expiration of the signed JWTs (i.e. the exp claim) is set to 60 seconds after the cache life time (if life time in cache is 1h, then token expiration is 1h and 1 minutes, letting at least 1 minute for the OP to check the token). No external configuration of this expiration is possible.

I order to avoid creating a new lua_shared_dict, I reused the one used for jwks, thinking that it would be the most apporpriaire dictionary to be “shared”. Challenging this point might be fully acceptable! :P

[zandbelt]

please compare notes with @bodewig on https://github.com/zmartzone/lua-resty-openidc/tree/support_jwt_auth_methods

[pamiel]

Oops, I did not realize that you were already working on the topic; sorry for that.
I will have a look to the ongoing job here.
@bodewig , in parallel, if you could have a look to my PR as well (that is quite simple so far)

[pamiel]

ok, @bodewig , you implemented the "client_secret_jwt" and I implemented the "private_key_jwt", which is very complementary: thanks to that, we would cover all authentication modes defined in https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication, which is good !!

Putting aside the difference of implementation due to the usage of a secret vs. a RSA key, the most important point is whether we should cache the generated JWT or not. For RSA signature, the impact on performances might really be problematic. That's the reason why I put a cache.
As HMAC SHA-256 is less demanding, maybe it is not required... but maybe it could be useful as well; what is you feeling about that ?

The minor differences I noted between the 2 implementations:

• presence of a the optional iat claim in the JWT in my implementation => could be homogenized
• expiration time of 60sec on my side, 10sec on your side (plus the fact that the expiration time is influenced by the cache mechanism, of course) => could be homogenized
• jti claim set to the request id on your side (fully makes sense), while based on the time on my side (in fact, due to the cache mechanism, the binding of the JWT to the request is less important as the JWT is used for several calls => I took one simple data... but taking the request id (of the current request, at JWT creation time) would be ok as well, even if there will not no direct mapping with the following requests when the JWT is obtained from the cache) => could be homogenized

[bodewig]

Sorry for the delay @pamiel , I've been traveling. By now I've rebased the brach I worked on quite some time ago :-)

I fully agree that our changes are complementary as we are implementing two different authentication schemes here. The spec says the JWT should be a one-time token unless negotiated differently, that's why I never thought about caching at all.

That one would want to cache the token in the case of RSA signature more than for HMAC is understandable, but given the spec we should make it optional then (and off by default?). and stick to short expiration times when not using a cache.

[pamiel]

Hi @bodewig,

Sure, I saw the sentence "These tokens MUST only be used once, unless conditions for reuse were negotiated between the parties; any such negotiation is beyond the scope of this specification" in the spec. Indeed, I was surfing on the "unless conditions for reuse were negotiated between the parties" ;)

In my mind, the requirement of uniqueness was addressed by setting the client_jwt_assertion_expires_in to 0 (no cache), and then, with a value different from 0, reuse could be implemented for "negotiated" situations that want to address performance concerns.

From a pure security standpoint, if we compare the client_secret solution with this one, then even with a cache, the solution is much more secure than a static password that never changes (or that changes once every 3 month, for example... if people are doing a good job...).
At least, here, we can have a JWT renewal every 10min or every hour (depending on the configuration of the cache), and a key renewal policy can also apply to change the certificate regularly. Always better than a static unchanged password...

Is setting the default value of client_jwt_assertion_expires_in to 0 (i.e. token is not reused) instead of 1h a good compromise for you?
I'm ok for having a short expiration time for the JWT... but I'm always a bit afraid with time leeway. I though that setting it to "cache life-time +1 min" would be ok, but I'm open to counter proposals (is 10s enough?) !

[bodewig]

I guess I'd prefer something more explicit than a zero expiration time, but that may just be me.

My initial version used iat_slack to account for any potential leeway, but this is meant to be used for something else. I'm not sure whether 10s will always be enough, hardcoding any value is probably wrong (i.e my 10s are in no way better than would be 60s).

[pamiel]

Yes, I think iat_slack is not a good choice for that: different purpose => different parameter.

We could then have a parameter such as allow_caching_client_jwt (as a boolean) in complement to the client_jwt_assertion_expires_in (that would now not exactly be the life duration in cache, but the value to be set in the exp claim, meaning that in case allow_caching_client_jwt is set to true, then the JWT could also be cached for reuse until the token is about to expire ("about to expire" => still back on the 10sec or 60sec of guard period to avoid expiration on OP side...)).

So, at the end, for private_key_jwt, we would need:

• opts.client_id
• opts.client_rsa_private_key
• opts.client_rsa_private_key_id (optional)
• opts.client_jwt_assertion_expires_in -> integer, in seconds. Eg: 10, 60, 3600...
• opts.allow_caching_client_jwt -> boolean

and for client_secret_jwt:

• opts.client_id
• opts.client_secret

and maybe also, why not (up to you)

• opts.client_jwt_assertion_expires_in
• opts.allow_caching_client_jwt

[bodewig]

This looks good to me - and I'd add caching for client_secret_jwt as well and only because somebody will come and ask for it once it is available for private_key_jwt :-)

[pamiel]

Yes, you are right ! :P

How can we proceed? Are you merging both contributions + applying the changes mentioned above into your own branch, or do you want me to update my PR with the above mentioned updates, and then you will do your own PR on your side ?

[bodewig]

Right now I won't be able to merge your and my work quickly, so we can probably move faster if we make the changes in two steps. If you can wait for a couple of days then I can take care of changing my branch pulling in your ideas instead.

[pamiel]

Ok, no problem: I will update my PR with the agreed changes, and let you rebase yours.

[bodewig]

One thing I just noticed, you are reusing the same cache we use for downloaded truststores, I believe it would be better to create a separate cache for the assertions.

[pamiel]

Yes, I mentioned it in my initial post: I reused the "jwks" cache, thinking that it would avoid creating a fourth dictionary as we already have 3... but honestly I don't know whether it might create issues or over-memory usage to multiply the number of caches or not. Please feel free to tell me.

[bodewig]

To be honest, I'm not that experienced with the impact of shared memory caches myself, just thought it would be cleaner to have separate caches.

@zandbelt what do you think?

[zandbelt]

I don't think we should have a cache at all: creating a JWT from the key material is not that big of an issue but moreover, a correctly generated JWT will have a jti value that makes it good for one time usage only. The OP should reject a JWT with the same jti being used a 2nd time, see: https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

[pamiel]

@zandbelt , yes, both of us already seen this part of the spec, but it says "These tokens MUST only be used once, unless conditions for reuse were negotiated between the parties", and this is this situation I would like to address by allowing caching of JWTs (@bodewig recommended to have a specific configuration parameter to allow caching; by default, no cache so no reuse of token).
I'm quite afraid about the performance issues that we might have without using cache. Running openssl speed sha256 rsa2048 shows a huge difference between RSA signature and HMAC SHA-256 (x1.000 if my computations are correct), and of course even more compared to the usage of a simple client-secret :P ... but I've been requested to use RSA signature :(

[zandbelt]

well, if you are sure that your OP does what you need... but I'm sure there aren't many out there

[bodewig]

That's why I recommended to turn the cache off by default :-)
Assuming there is a cache for those who know what they do, should it be a separate one from the jwks cache? I think it should be.

[pamiel]

Dear all,

Thinking twice on the topic (a long week-end is always good for meditation :P): Ok, computation of RSA signature is much more time consuming than the other authentication means… but how many times will this happen ? Indeed, for a given end-user access, it will happen only at initial authentication time, and then regularly at token refresh time, which mean once every couple of minutes only.
Assuming that the number of end-users remains reasonable, then it is definitely not an heavy load.

So, as this is not the mindset of the specification to reuse tokens, as I see that both of you are not fully convinced of the interest of having a cache, and as I don’t want to over-engineer the solution at the beginning (incl. adding an extra dictionary), then I updated my PR to remove the cache.
If in the future I’m facing some performance issues, then I will submit a new PR at that time to add a cache mechanism ;)

Note that in complement, I added a general declaration of "resty.jwt" because there were only local declarations inside functions (sometimes leading, in some functions, to get 2 declarations of a jwt variable).

I saw that, in parallel to our discussion, other PRs have been merged to master: how should I rebase my local branch so that the merge of the PR to master be automatic on your side? Should I git rebase (which will rewrite the history and set it in the right order; but what about the commits of what I already pushed to you in the PR? Is GitHub managing this transparently?) or should I git merge (which does not rewrite the history… but which will not be compatible with your history, I guess: commits will not be in the correct order at the end) ?
Thanks for your help !

@bodewig , as agreed, I let you then add the support of "client_secret_jwt".

[pamiel]

Oops, sorry: I though I did a good think :P
I will rollback this part

[bodewig]

Thanks, @pamiel !

I don't think we've applied any strict policy about rebases in the past. If the PR can be merged, then this is good enough for me. If you want to rebase your branch that's also fine - and whenever I have done so the github UI just did the right thing.

I'm not sure when i'll find time to add the client_secret_jwt part, but it shouldn't be more than a few days.