OpenID connect: support response_mode=query

[kevinchalet]

response_mode=query support has been a recurrent demand on JabbR, given that the optional response_mode=form_post is not implemented everywhere.

Even if authorization servers should make sure they don't return token or id_token when using response_mode=query, we should reject such requests in the middleware itself.

#202 (comment)

/cc @HaoK @brentschmaltz @Praburaj

[kevinchalet]

@brentschmaltz if you agree with the general idea, I'll submit a PR when yours is merged.

[brentschmaltz]

It's not entirely my call, other folks involved have indicated that 'query_mode' should not be supported by default.

[kevinchalet]

You should maybe ping them so they can participate to this conversation 👷

/cc @selfissued

[brentschmaltz]

@PinpointTownes I don't think there is much support for query mode.

[brentschmaltz]

@PinpointTownes did you ping MikeJones (selfissued)?

[kevinchalet]

@brentschmaltz I mentioned him in my last post, so he probably received a notification from GitHub.

I'll ping him via Twitter 😄

[kevinchalet]

Tweet sent: https://twitter.com/PinpointTownes/status/601111713853890560

[selfissued]

First, response_mode should only be used when a non-default response mode is being requested for the response_type, per http://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#ResponseModes and http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest. Given that it's never safe to use the query response_mode when fragment encoding is the default, this means that it is only ever safe to use response_mode=query when response_type=code is used. But in that case, it's unnecessary and NOT RECOMMENDED, because query is already the default for the code response_mode. So I short, response_mode=query should never be used.

The query response mode is, of course, used by default for the most common response_type - code.

I would close this one as invalid, because it reflects a misunderstanding of the specifications.

[kevinchalet]

@selfissued by response_mode=query, I mean GET callbacks using the query string to flow the OIDC payload (the last being the result of the explicit response_mode or implied from the response_type used), that are currently NOT supported by the OIDC client middleware.

The question is: should we support GET+query string callbacks for response_type=code?

[kevinchalet]

it's never safe to use the query response_mode when fragment encoding is the default

BTW, that's exactly what I said in my first post:

Even if authorization servers should make sure they don't return token or id_token when using response_mode=query, we should reject such requests in the middleware itself.

[selfissued]

The default response modes are specified for each response type in http://openid.net/specs/oauth-v2-multiple-response-types-1_0.html. For instance, about the four combination response types defined in http://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#Combinations, you'll see this language "The default Response Mode for this Response Type is the fragment encoding and the query encoding MUST NOT be used". Likewise, http://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#id_token says "The default Response Mode for this Response Type is the fragment encoding and the query encoding MUST NOT be used". The only response_type for which the query response mode is permitted is "code", which is documented at http://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#ResponseModes says "the default Response Mode for the OAuth 2.0 code Response Type is the query encoding". But since it's the default, response_mode=query should not be explicitly included in any requests.

Therefore, for no response_type value used by OpenID Connect should response_mode=query ever be used. Requests to do so reflect an incomplete understanding of the specifications and the reasoning behind them.

[kevinchalet]

Meh, the thread is not about adding an explicit response_mode=query parameter to the OIDC request or not when using response_type=code but about SUPPORTING GET REQUESTS ON THE CALLBACK ENDPOINT FOR THE CODE FLOW, WHICH IS NOT CURRENTLY THE CASE.

(you know we're not illiterate, right? 😄)

[brentschmaltz]

wow, that took a while. thanks for pushing through.
@tushargupta51 will need to have a look at this since he is working on the 'code' only flow.

[brentschmaltz]

@PinpointTownes @selfissued @tushargupta51
ok, so I think we all agree OIDC handler needs to support a 'code' response returned as a 'query' parameter. We will have to do some work, since OIDC handler only looks at post. I assigned the bug to myself, since tushargupta51 doesn't seem available now.

[kevinchalet]

@brentschmaltz if you want me to, I can submit a PR and ping you to make sure we're adding the right checks to reject unsafe OIDC payloads.

[kevinchalet]

@brentschmaltz https://github.com/PinpointTownes/Security/commit/4125595f89efa3c485c3dff16d770813369515a6

I'll test it with AspNet.Security.OpenIdConnect.Server and send a PR if everything works fine 😄