search

thephpleague / oauth2-server

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

BC-break by removing temporary file with private key #1240

Closed tjveldhuizen closed 2 years ago

tjveldhuizen commented 3 years ago

Since https://github.com/thephpleague/oauth2-server/pull/1180 an in memory key is not written to a temporary file anymore. That's problematic for https://github.com/steverhoades/oauth2-openid-connect-server because https://github.com/steverhoades/oauth2-openid-connect-server/blob/77d31399b1cf6bab6eef544c5c6089e6c5913880/src/IdTokenResponse.php#L94 depends on the file on disk. I think such change should be done in a major version.

Sephster commented 3 years ago

Thanks for reporting this. What is the error message you are receiving? Is it when trying to retrieve the key path?

If you could specify the exact error message I can take a look and see if we should be applying a fix. Thank you

tjveldhuizen commented 3 years ago

The error I get is The path "" does not contain a valid key file, but that's not an exception in this package. The problem is that until version 8.3.1 this line did return a file path, even when the CryptKey was instantiated with the key string as a parameter. Now it returns an empty string.

In my opinion, a null value return would have made sense in that situation, making it more clear a BC break occurs because the return type of getKeyPath() changes from string to string | null.

Sephster commented 3 years ago

I see what you mean now and am sorry this is causing issues. Are you passing a key into the AuthenticationServer as a string or a file path? I think the change we have made means if you pass a string (effectively key contents), we will only ever store the key in memory where as previously we wrote it to the /tmp file regardless.

I didn't believe this was a BC break at the time as it wasn't changing any of the interfaces on the library but the behaviour has changed somewhat and we didn't pick this up during testing sorry.

Sephster commented 3 years ago

I should add that the reason this change was made was because a number of users are using the server in a read only docker container, so the write to a /tmp file was not possible or necessary for them.

tjveldhuizen commented 3 years ago

I'm passing the key as a string (from an environment variable).

I think it's okay to do this change in a major version and also think it's better not to save it in a temporary file, but it should be done in a major version (preferably with some kind of deprecation notice in a minor version).

Cayan commented 3 years ago

I also have the same issue

I think it may have had to do with this PR: https://github.com/thephpleague/oauth2-server/pull/1215/files

Sephster commented 2 years ago

My apologies for taking so long to respond. My intention had been to leave this open for a little while and see if this issue got traction and possibly look at a work around rather than reverting the change in its entirety.

It is never my intention to release a breaking change in a non breaking release but unfortunately, this one slipped through the cracks despite my best effort. Because so much time has passed and we haven't had too many reports of this affecting people, I'm going to leave the release as is.

Please do accept my apologies for any inconvenience caused with this and thank you for reporting and fixing the issue in Steve Rhoade's library.

I trust you have both now found a workaround but if this is not the case and I am missing something, please do get back in touch and we will see if there is any workaround we can bake in.