Fix bug which prevents the at_hash claim from appearing in the ID token from an authcode grant for some storage implementations

[cfryanr]

fosite has a bug which may prevent the at_hash claim from appearing in the ID token from an authcode grant. The bug is in the PopulateTokenEndpointResponse method of OpenIDConnectExplicitHandler. The bug will only impact systems which implement the storage interfaces by returning different struct instances.

There are two sessions at play the PopulateTokenEndpointResponse method of OpenIDConnectExplicitHandler:

1. The requester parameter contains a session. In typical usage of fosite, requester is created by a custom implementation of the token endpoint to represent the current request to the token endpoint. The token endpoint calls NewAccessRequest, which calls HandleTokenEndpointRequest on each token endpoint handler. The AuthorizeExplicitGrantHandler’s implementation of that function performs a storage lookup using c.CoreStorage.GetAuthorizeCodeSession(ctx, signature, request.GetSession()) to find the authorize request for the given authcode. It then sets that session from storage onto the new request using request.SetSession(authorizeRequest.GetSession()) (see flow_authorize_code_token.go).
2. The second session is looked up directly by the PopulateTokenEndpointResponse method by calling c.OpenIDConnectRequestStorage.GetOpenIDConnectSession(ctx, requester.GetRequestForm().Get("code"), requester).

These two sessions may not necessarily be the same instance (same pointer), depending on the implementation of the storage interfaces. Fosite should not assume that they will be the same instance, since they came from different calls to storage interfaces.

The last line of the PopulateTokenEndpointResponse method of OpenIDConnectExplicitHandler uses the authorize variable to create the ID token. The access token hash which is computed a few lines above must be set onto the authorize variable’s session in order to have it available during the creation of the ID token. The fix is to simply use the authorize variable’s session.

Note that the bug did not impact the refresh grant, where the ID token does correctly include the at_hash claim.

The unit tests in flow_explicit_token_test.go have been updated to use two different instances of sessions to demonstrate the issue. They have also been updated to be able to run independently without polluting each other, by moving the test setup into the loop and by having each test perform its own test setup. Before the fix, many of these tests fail when run individually because they were accidentally relying on the pollution caused by each previous test in the same file. A new test case was also added to backfill for a case (unrelated to the bug) which was not previously covered.

The integration tests were not changed. They do not catch the bug because they use the storage implementation in memory.go which always returns the same session instance for the two relevant sessions. It might be nice if this storage implementation always returned a copy of the session (always different instances) but it is not obvious how to enhance the code in this way.

Note that the unit and integration tests of the Pinniped project demonstrate the bug. Pinniped uses fosite to implement a partial OIDC provider which supports the OIDC authcode and refresh flows. The ID tokens returned by the authcode grant never includes the at_hash claim, but the ID tokens returned by the refresh grant do include the claim. The changes that would be made to the project’s tests if this bug fix is accepted into fosite are shown in Pinniped PR 1165. The Pinniped project has a unit test which performs an authcode grant and then performs a refresh grant, and the behavior of the fosite library can easily be observed by adding breakpoints in the relevant fosite code and then running the test in the debugger.

Related Issue or Design Document

None.

Checklist

• I have read the contributing guidelines and signed the CLA.
• I have referenced an issue containing the design document if my change introduces a new feature.
• I have read the security policy.
• I confirm that this pull request does not address a security vulnerability.
  If this pull request addresses a security vulnerability,
  I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
• I have added tests that prove my fix is effective or that my feature works.
• I have added necessary documentation within the code base (if appropriate).

Further comments

None.

[aeneasr]

Awesome, thank you! 🎉 Your contribution makes Ory better :)

[cfryanr]

@aeneasr, just curious when this might get shipped in a release? Looking forward to incorporating it downstream.

[james-d-elliott]

If not aware there are actually a few methods to integrate this downstream already:

1. Create a fork and create a branch based on 5dbfa9a and cherry-pick c3b7bab into it. Then in your go.mod use the replace directive to forcible pick your fork over fosite latest release. You don't have to change code, the go module system does everything for you.
2. Use the respective commit as the version, i.e. go get -u github.com/ory/fosite@c3b7bab41db24b000f8e1416e1475e0aae4c310c

Option one is more deliberate, and you will get the latest release plus the final commit.

[cfryanr]

Thanks for the tip, @james-d-elliott. Aware of those options, but I prefer to pull in the official release whenever possible.

The first option that you mentioned may prevent Dependabot from noticing future official releases, and the second option could pull in intermediate commits that are maybe not ready for release (assuming that the maintainers would have shipped an official release if they felt that all new commits to master are ready for release).

[james-d-elliott]

Thanks for the tip, @james-d-elliott. Aware of those options, but I prefer to pull in the official release whenever possible.

Yeah no worries. I figured it was the case but if you didn't know then it may have been a viable option for you. Also I agree the second option is not a desirable option for production software.