Skip to content
This repository has been archived by the owner on Jun 23, 2023. It is now read-only.

Don't write jwks to disc #93

Open
nsklikas opened this issue Jun 9, 2021 · 15 comments
Open

Don't write jwks to disc #93

nsklikas opened this issue Jun 9, 2021 · 15 comments
Milestone

Comments

@nsklikas
Copy link
Contributor

nsklikas commented Jun 9, 2021

The code in https://github.com/IdentityPython/oidc-op/blob/master/src/oidcop/token/handler.py#L172 doesn't make much sense to me.

Why do we always have to create that jwks? Why do we always write it to disc?

If I understand it correctly this jwks will not be used if that tokens are JWKS.
Also I see no reason to force writing to disc.

Is there something I'm missing?

@peppelinux
Copy link
Member

It depends by read_only, if True it won't create/write anything

read_only = defs.get("read_only", read_only)

Probably we could handle this jwks initialization in a single,way, in a unique function.
Often we have this features re-implemented in many places

@nsklikas
Copy link
Contributor Author

nsklikas commented Jun 9, 2021

Yes but it also forces the private_path

if kwargs.get("jwks_def"):

If this jwks is not needed (access token and ID token can be JWT) why force a private path? I don't need to keep those keys

@peppelinux
Copy link
Member

Actually we deal with jwks forgery in

./endpoint_context.py:79:        jwks_def = {
./endpoint_context.py:84:        th_args = {"jwks_def": jwks_def}
./token/handler.py:172:    if kwargs.get("jwks_def"):
./token/handler.py:173:        defs = kwargs["jwks_def"]
./configure.py:527:        "jwks_def": {

I'm wondering if it could be better to handle only in endpoint_context and not also in token handler BUT that's not the question.

If I have well understand you're are saying that we should need an option that forces, optionally, the token minting in JWE format and also this parameter would handle the jwks defs.

overall, I have to be honest, I'd like to have an example about how to get tokens in JWE format, standing of an example configuration. @rohe what did I miss this time? :)

@rohe
Copy link
Collaborator

rohe commented Jun 10, 2021

So, we're witnessing a change in usage here.
Back when, we had keys that was only used for constructing access/refresh JWS tokens.

Now, token handler uses the same keys as are used for signing/encrypting messages so there are really no need for a set of special keys for just access/refresh JWS tokens.
Lately I've started to use JWKS for initiating and handling symmetric keys as can be seen in L182- in ./token/handler.py.
The reason for having read_only=False and using private_path comes from this. The symmetric keys will persist over restarts of the server. Presently these symmetric keys are not part of the dump/load paradigm.

I'm leaning against having 2 JWKS definitions, one for endpoint_context which then will be used for all type of JWT based tokens an all type of messages and one for symmetric keys that needs to be persistent over restarts (here the kid is used to distinguished between the keys). Allowing both sets of keys to be constructed before the first start of the server.

I want to get away from having lines like this in the configuration:

key: "A long rambling set of characters"

We should never trust people to come up with reasonable keys.
If we use JWKS for symmetric keys we could also enforce things like key length.

@nsklikas
Copy link
Contributor Author

I agree that these keys should be persisted in a production environment.

But when I'm running it on my machine for development/testing I don't want the keys to be written in the disc. I should be able to choose whether I want them saved.

@rohe
Copy link
Collaborator

rohe commented Jun 11, 2021

OK, sounds fair

@peppelinux
Copy link
Member

Sounds good, we Need a roadmap now 🐕

@rohe
Copy link
Collaborator

rohe commented Jun 12, 2021

So what keyword should we use in the configuration to flag this ?

Ephemeral ? Like this:

    "token_handler_args": {
        "ephemeral_keys": True,
        "code": {"kwargs": {"lifetime": 600}},
        "token": {"class": "oidcop.token.jwt_token.JWTToken", "kwargs": {"lifetime": 3600}, },
        "refresh": {"class": "oidcop.token.jwt_token.JWTToken", "kwargs": {"lifetime": 86400}, },
        "id_token": {"class": "oidcop.token.id_token.IDToken", "kwargs": {}},
    },

@peppelinux
Copy link
Member

Ephemeral? Ok, but not without documentation!!!

Let's see.
Is there the possibility to deprecate token handler jwks and have It in endpoint_context?

Anyway, I follow you

@rohe
Copy link
Collaborator

rohe commented Jun 12, 2021

What are the use cases ?:

  1. Test deployment. You don't expect any persistence between server runs.
  2. You expect the server to create all necessary keys and to make them persist over runs.
    The first time up you just start the server, it creates all the necessary files and writes them to disc such that
    for the following stop/start sequences will always get you a server that uses the same key set.
  3. You provide a JWKS file with the keys you want the server to use.

There might be something in-between 2 and 3. Where you provide some but not all of the necessary keys.
To make it easier we might want to out-law this.

@rohe
Copy link
Collaborator

rohe commented Jun 12, 2021

Token handler is part of the endpoint context so I don't see the need for having it higher up.
If the configuration is part of token_handler_args then the result of the configuration should be in token_handler.

@peppelinux
Copy link
Member

Good to me @rohe, few things not so important right now.
I'd spend energies to have JWE tokens, optionally, with some paramenters in token_handler. That's another topic, I'll open an Issue

@rohe
Copy link
Collaborator

rohe commented Jun 12, 2021

I guess the policy for doing JWE instead of JWS could be quite complex.
Definitely per audience (RP/RS).
Now, if there is more then one entity as audience then you suddenly has to do JSON serialisation instead of compact serialisation. More complexity. Which means you really should be sure you need this before venturing down that path.

@peppelinux
Copy link
Member

I guess the policy for doing JWE instead of JWS could be quite complex.

@rohe thread moved to #102

@peppelinux
Copy link
Member

This issue causes to satosa oidcop frontend to write private files related to cookies to disk, even if satosa frontend doesn't use oidcop cookies.

Do we have a way to put them in a custom storage? (/dev/null for cookies in satosa)

@peppelinux peppelinux added this to the 2.2.0 milestone Sep 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants