Closed davidgtonge closed 6 years ago
Merging #122 into master will decrease coverage by
0.12%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #122 +/- ##
==========================================
- Coverage 100% 99.87% -0.13%
==========================================
Files 17 17
Lines 795 798 +3
==========================================
+ Hits 795 797 +2
- Misses 0 1 +1
As discussed in #120
Just missing one important piece. Asserting, based on response_type used, that s_hash is even returned in the payload where it MUST be.
Now i’m thinking we’ll have to introduce a flag for a client instance. Smth like client.PROFILE with values OIDC, FAPI, OB with their specific requirements in. Wdyt?
Oops - just pushed the tls_client_auth to the wrong branch - will fix that and add the assertion.
Flag for the profile sounds good.
I’m travelling atm. Will take a look at all of this tomorrow evening.
sounds good - I also probably wont get a chance to finish these off until then.
@panva so I've pushed a commit that checks the presence of s_hash, however how do you want to define the different profiles.
Should it be something like clientInstance.PROFILE = Issuer.PROFILES.FAPI2
Or just clientInstance.PROFILE = "FAPI2"
Also this is the relevant clause:
shall include state hash, s_hash, in the ID Token to protect the state value if the client supplied a value for state. s_hash may be omitted from the ID Token returned from the Token Endpoint when s_hash is present in the ID Token returned from the Authorization Endpoint;
I think it can be simplified (and made clearer) FAPI Part 2 requires hybrid mode, therefore there is always an id token returned in the auth response, therefore if state is provided, the s_hash must be present in that id_token.
I'm not sure if there is any benefit from it being included in the id token from the token endpoint.
@davidgtonge this is already looking good, but let's see what differences does FAPI/OB/pure OIDC make in the mtls case to nail down the profile option.
The options I see
1) define the profile constants and export them similar to your suggestion above 2) define a new set of constructors FAPIIssuer, FAPIClient (or similar) which have these extra checks enabled by default, for instance - they require s_hash from the authorization endpoint, they only allow hybrid response_type to be used and they require that checks.response_type be provided so that the response parameter presence gets verified.
Furthermore support for JARM signed/encrypted authorization response parsing is needed.
@davidgtonge does FAPI make the use of state parameter required?
does FAPI make the use of state parameter required?
Not at the moment. The tricky thing is that FAPI is trying to walk a line between OAuth 2.0 and OIDC and its bringing some confusion. For example in OIDC with a hybrid flow, the nonce
should be enough. But we want FAPI to be able to support https://tools.ietf.org/html/draft-ietf-oauth-jwsreq-16 and JARM. In that case, state should be required.
I'll have a think about the differences required for MTLS between the different profiles (hopefully there aren't any).
@davidgtonge seeing that FAPI requires hybrid to be used, is adding the validation to token endpoint necessary for your case? The profile is something i'll get to once i get per-endpoint http options and mtls going.
@davidgtonge I'm going to go ahead and close this for now, mtls is still on my radar. Enforcing s_hash presence in the id_token from authorization endpoint is something the app can do on its own, but will land in openid-client eventually too, just need to find a good structure for these profiles.
This is required in the OpenID Foundation's FAPI profile if s_hash hasn’t been returned from the authorization endpoint.