KeyProvider makes reliable implementation challenging

[wfhartford]

Describe the problem

The KeyProvider interface allows users to easily implement signing key rotation. This interface includes two methods used when signing a JWT: getPrivateKeyId() and getPrivateKey(). When key rotation is being used, subsequent invocation of these two methods may return different results as one signing key is swapped for a new one. This creates a race condition when signing keys around the time of a key rotation. The getPrivateKeyId() method is called first, when building the JWT header, getPrivateKey() is called shortly after to sign the JWT. If a key rotation occurs between these two method calls, the resulting JWT will have the key ID of the older key while it will have been signed by the newer key, making verification of the JWT impossible.

What was the expected behavior?

Key ID and signing key should always match.

Reproduction

I have not produced this issue, but examining the code, and imagining a typical KeyProvider implementation with key rotation should make the issue obvious. Reproduction should be possible by simply generating a large number of keys using a KeyProvider implementation that rotates keys frequently.

Proposed Fix

This issue should be fixed by deprecating the two methods mentioned above and adding a new method to the interface which returns an instance of a new class (lets call it PrivateKeyDetails for now) which contains both the key ID and the private key. This can be made backwards compatible by including a default implementation of the new method which builds the PrivateKeyDetails by invoking the getPrivateKeyId() and getPrivateKey() methods. This default implementation will exhibit the same race condition, but allow a user to build a safe implementation.

If a pull request would likely be accepted, I would volunteer to contribute the fix.

[wfhartford]

Removed reference to thread-safe from title as this is not really a threading issue and is quite possible in a single threaded use-case.

[lbalmaceda]

Thanks for offering to create a PR. In theory, it seems that that inconsistency could happen. We would like to see the solution you propose but would be good to first demonstrate that the issue is present, so we can have something to compare against and see if it's fixed when the solution is implemented. We will wait for you to provide that first. Cheers.

[wfhartford]

I've just submitted BR #517 which adds a test class highlighting the issue described above. This test defines a ECDSAKeyProvider implementation which implements time based key rotation. The four test methods use this provider to generate and verify 10000 tokens using various key rotation frequencies (10s, 1s, 100ms, 10ms). The test with a rotation frequency of 10ms fails reliably, normally on the first key verification. The other rotation frequencies are less likely to fail, but can still fail.

The key rotation frequencies used in this test are certainly not realistic, but even more reasonable rotation frequencies any chosen key rotation frequency is likely to experience this issue eventually in a high traffic system.

[lbalmaceda]

Thanks for supplying the PR.

The key rotation frequencies used in this test are certainly not realistic, but even more reasonable rotation frequencies any chosen key rotation frequency is likely to experience this issue eventually in a high traffic system.

We agree, but as a proof of concept, it still helps to illustrate the issue. When we tackle this we will probably test against higher values closer to minutes to be a bit more realistic.

[wfhartford]

Testing with realistic values like minutes is likely to hide any problems rather than expose them. A valid solution to this problem would make the issue impossible regardless of the rotation frequency.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

[poovamraj]

Hi @wrhartford,

We evaluated addressing this in this PR, but
have decided to not move forward with it at this time for the following reasons:

• The issue can be worked around by using flags to ensure rotation doesn't happen while signing,
  as demonstrated here.
• The DX to address it would be a non-trivial breaking change.

We are going to close the issue but will continue to monitor it for any updates. Thanks again
for your contribution including the test sample.