thephpleague / oauth2-server

A spec compliant, secure by default PHP OAuth 2.0 Server
https://oauth2.thephpleague.com
MIT License
6.54k stars 1.12k forks source link

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

Open marc-mabe opened 5 years ago

marc-mabe commented 5 years ago

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:

Thoughts?

sg3s commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/web-token/jwt-framework/issues/185 https://github.com/web-token/jwt-framework/pull/187

bradjones1 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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

JackKora commented 7 months ago

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

Sephster commented 7 months ago

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