Use a Key object when instantiating AuthorizationServer for 200x-300x faster encryption of refresh token #820
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ption/decryption, when combined with oauth2-server package patch from thephpleague/oauth2-server#814
|
Is this a breaking change for tokens that have already been issued? |
|
@taylorotwell Good catch, in the current state of this PR it would be a breaking change. However, I have found a solution to preserve backwards-compatibility and have made a PR in the OAuth2 server repo for it: thephpleague/oauth2-server#941 As such, it might be worth waiting on that other PR to get merged in and a new version of the package to be released before this PR is merged. |
|
OK. Feel free to re-submit this when that is resolved. |
|
@taylorotwell based on the feedback in the other PR, it doesn't seem like this will be able to be a backwards-compatible change. Do you want to reconsider this for the next major release? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now, it can be quite slow to generate a new access and refresh token due to having to use the PBKFD2 hash function, which can account for over 50% of time spent generating an access token in most cases. I found an issue on the oauth2-server package which describes this issue in more detail: thephpleague/oauth2-server#812
Since the encryption key Laravel uses already comes from the
random_bytesfunction, it is secure enough to use as-is. We are also storing the key server-side, so there isn't a big need to use a password hashing key generation function when trying to encrypt the refresh token.The PR thephpleague/oauth2-server#814 implements a fix (released on version 7.x of the library), by allowing the authorization server to be instantiated with a Key object instead of a string. By taking advantage of this and passing in an instantiated key, I was able to get a 290x speed improvement over 1000 iterations of encrypting a refresh token (2+ mins to 0.5s). Other machines may have similar performance gains.
I have verified that Passport continues to work as normal with this change. This change can be backported to Passport 6.0 as that version also has version 7.x of the OAuth2 library. (I can make a separate PR for the backport once this one is approved)