Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept key content directly instead of require a key file #1007

Open
marc-mabe opened this issue May 9, 2019 · 10 comments
Open

Accept key content directly instead of require a key file #1007

marc-mabe opened this issue May 9, 2019 · 10 comments

Comments

@marc-mabe
Copy link
Contributor

We are running the server in a docker environment with read-only filesystem and we use the same docker image for staging and production.

That's why we configure everything (including keys) using environment variables but this is impossible because the keys are required to be available as files :(

Furthermore you already accept the keys as content but in this case you write them to the filesystem (fails in my case because of r/o filesystem) just to get read again by Lcobucci\JWT\Signer\Key.

Interestingly Lcobucci\JWT\Signer\Key does accept the key content directly and only tries to read it as file if it starts with file://.

As a simple test I have updated League\OAuth2\Server\CryptKey constructor to just take the given key as is in case of matching RSA_KEY_PATTERN and it worked fine for me.
... It's already a bit misleading that $keyPath accepts key content (was the case before) but now getKeyPath() also returns the key content in this case.

I would suggest to:

  • Change it the way I described it
    • rename constructor argument $keyPath to $key and document it accepts both, a file path and key conent
    • deprecate getKeyPath
    • add new method getKey() documented as either key content or file path

Thoughts?

@sg3s
Copy link

sg3s commented May 21, 2019

We are also running a non-root containerized environment with read-only file systems.
Our containers are also never rebuilt between promotions to different environments.
We worked around the issue by always specifying the keys as a file path reference.

But I agree, the library should just take a full key and not try to save it as a file. It tries to coerce you to handle the key safely but the method it uses does not play well with containerized environments. Personally I would ditch all code trying to manage how the key is delivered to the library, maybe only accept strings as keys, not as file paths, and only validate if that string looks like an rsa key.

What you can do to handle this without changing the library
Depending on your container orchestration solution you should be able to mount files in the container somewhere during startup. You can then just reference/configure the file paths in your application. You should not need root privileges or write rights inside the container to mount these files.

This can be configured to work nicely with docker-compose and other local tooling too of course.

@Sephster
Copy link
Member

Thanks for the suggestion @marc-mabe. We've just released a new major version and as this would be a BC change, it would be a while before we'd get this into the system.

As time goes by, I'm wondering more and more if we should have a .env file for the server for all these small options to make them easier to configure such as PKCE enablement, keys etc.

I will mark this as a future feature for the timebeing but don't think we can implement this just now. Thanks for the suggestion though. It's very much appreciated.

@sg3s
Copy link

sg3s commented Jul 15, 2019

I would be strongly against using a .env file directly. This package is still a library and should be implemented in projects managing the config on their own. Defining the use of a separate .env for authorization server variables would also be against recommendations (at least in the node version of the dotenv lib they explicitly say so).

What can be done however is look at what types of key formatting the library accepts as input.

The current library used for creating JWTs is lcobucci/jwt which is fairly straightforward. However it missed the spec in some regards (the aud claim can be one or many StringOrUri objects, a fix for this specific issue is in the v4 alpha3 stage). Some items are to be fixed in future versions, but development is moving slow. As far as I know the library only accepts the keys as strings or a file references to a PEM formatted key.

Recently I came across JWT Framework which seems to have a very good grasp on the standards and expands on the capabilities in lcobucci/jwt by supporting more algorithms, features as described in later/current RFCs. One interesting part is the implementation of JWK and JWKs, which abstracts the formatting of keys in such a way that they are directly usable in a broader set of circumstances.

Feed the library with json strings;
https://web-token.spomky-labs.com/components/key-jwk-and-key-set-jwkset/key-management

Example json formatting and cli helper commands;
https://web-token.spomky-labs.com/console

It would add different/more dependencies however.

The only one I would have an issue with is being worked on currently; edit; it's fixed
web-token/jwt-framework#185
web-token/jwt-framework#187

@bradjones1
Copy link
Contributor

Trying to collect context regarding this and related problem spaces/issues: With the addition of CryptKeyInterface we could continue to abstract out the key handling such that the generation of a JWKs manifest works regardless of the specific cryptography in use.

@ptkoz
Copy link

ptkoz commented Dec 12, 2020

To the original comment, in lcobucci/jwt 4.0.0 you can no longer create key by giving either its contents or path to a file. It now has separate classes for in-file (\Lcobucci\JWT\Signer\Key\LocalFileReference) and in-memory (\Lcobucci\JWT\Signer\Key\InMemory) keys and league/oauth2-server hardcodes the usage of LocalFileReference.

For now it makes us stick to 8.1.*, where it's still possible to provide your own implementation of \League\OAuth2\Server\CryptKey that simply keeps key contents in a variable. Apart of the reasoning @marc-mabe gave in his original comment, the performance impact of additional (and completely redundant) I/O operations is just too big for us to take.

@Sephster
Copy link
Member

The upgrade to php 8 wasn't plain sailing as we didn't want to break BC but the jwt package needed to be a major upgrade.

Unfortunately this was one of the compromises. I will take a look but might need to release a proper solution for v9. Sorry for any inconvenience caused by this.

@ssigwart
Copy link
Contributor

ssigwart commented Jan 26, 2021

I have an update in #1180 that switches from using getKeyPath (hence requiring the temporary file) and replacing it with getKey which can use a file (Lcobucci\JWT\Signer\Key\LocalFileReference) or memory (Lcobucci\JWT\Signer\Key\InMemory).

@ptkoz
Copy link

ptkoz commented Mar 7, 2022

Is it worth closing this issue, given recent versions of the library do support fileless keys (with Lcobucci's InMemory)?

@JackKora
Copy link

JackKora commented Apr 17, 2024

Just confirmed that newer versions do read from env vars @Sephster.

@Sephster
Copy link
Member

I am 90 percent certain this is no longer an issue. You don't need a key file as you can pass in a key string which is then stored in memory. You can get this from a .env file if you wish.

I can't check this just now @JackKora but see issue #1180 which I think resolved this. It is mentioned 2 comments above and someone also states this probably resolves this issue.

As soon as I am able, I will check this and close the ticket accordingly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants