Interoperate with conformant providers that send positive assertions as a POST

[jablko]

Conform to spec: Support providers which send positive assertions as a POST (vs. a GET). Compare any query parameters that are present in the "openid.return_to" URL, with query parameters in the URL that is processing the assertion - vs. with the assertion message. Assertion message is only equivalent to query parameters in the URL that is processing the assertion if the assertion is sent as a GET - it's not equivalent if the assertion is sent as a POST

[keturn]

Something about this is setting off alarm bells in my head. "Doesn't work with POST" is just not the sort of bug that can go unnoticed for three or four years. I may have time to review this patch on Friday, but I don't have push access in any case.

[jablko]

Thanks Kevin, that would be great

[keturn]

Okay, I have a guess at what's happened here: Your program works when there's a GET response or when the return_to data has no query parameters. But when you use POST and there's a query parameter on the return_to, it breaks, is that right?

I think when you're invoking consumer.complete(), the query data you pass in contains GET data or POST data, but not both. That's why you're patching _verifyReturnToArgs to use the values parsed out of the URL instead of the passed in query data. I don't think you want to do that, this will obscure the case where the parameter in the URL says one thing but the POST data has another value for the same key.

I recommend fixing up the calling code instead. See https://github.com/openid/python-openid/blob/master/examples/djopenid/consumer/views.py#L152 for an example of how the two data sources may be merged.

[jablko]

Thanks a lot for reviewing this Kevin. You understand correctly what's happening. However I think consumer.complete() must get invoked with only query data if the request is a GET, and only POST data if the request is a POST, to avoid violating the OpenID specification? Merging the query data and the POST data like you suggest violates the specification in two ways:

When a message is sent as a POST, OpenID parameters MUST only be sent in, and extracted from, the POST body.

Merging the query data and the POST data extracts OpenID parameters from the query string, as well as the POST body

Any query parameters that are present in the "openid.return_to" URL MUST also be present with the same values in the URL of the HTTP request the RP received.

If the query data and the POST data are merged, then _verifyReturnToArgs() might not reject an assertion where a query parameter in "openid.return_to" isn't present or has different value in the URL of the request the RP received

[keturn]

Gaaah.

By which I mean, you have a point about the spec.

I think the right answer is "never mix query and post", but that may not really be practical. For one thing, at least with OpenID v1, the library itself will add query parameters to the return_to. And I fully expect there to be a fair number of users who are tacking args on to the return_to because there's no way to thread data through and get it back in POST.

If the query data and the POST data are merged, then _verifyReturnToArgs() might not reject an assertion where a query parameter in "openid.return_to" isn't present or has different value in the URL of the request the RP received

Is that so?

I'm just going to think out loud here for a bit:

Either way, GET and POST are basically the same thing to the OpenID protocol. POST is just a thing we do because there's a limit to how much data you can fit in a URL. They both come from the OP; for GET, as a link or redirect, for POST, it's the action URL of a form and the data in it, but it's all written by the OP and goes through the user agent. They're approximately equivalent in how easy they are for the user-agent or a malicious third party to tamper with. And the entire OpenID message, including openid.return_to, is signed.

So, given that, how does this play out:

If you combine the two, with Query overriding Post, then ... you could potentially throw in some stuff on the URL, but verifyReturnToArgs will catch it. And none of the query data can override the OpenID assertion data without breaking the signature.

If you combine the two, with Post overriding Query, then POST data might clobber some RP data in the query string. If that was done by the OP, then you're screwed anyway, because the OP could just as easily modify the return_to URL before signing it. If that was done by a MitM, verifyReturnToArgs will catch it as long as the parsed data you pass to consumer.complete is the same as the parsed data your application uses. verifyReturnToArgs can't catch that if you don't let it inspect the post data!

So I think consistency is key here. Just the fact that you ran into this situation means you already are mixing post and query data. Once you've decided to do that, stick with it, and pass the whole ball through to consumer.complete.

The safe play would be to do something like assert set(query.keys()).isdisjoint(post.keys()) before you mix them.

Does that logic hold up?

If it bugs you that this seems to run contrary to the spec, then file a bug against the spec. =)

[jablko]

Thanks a lot for following up Kevin

Just the fact that you ran into this situation means you already are mixing post and query data.

I ran into this situation without mixing query data and POST data: A "janrain_nonce" query parameter is added to return_to. OP sends assertion as a POST, so application invokes consumer.complete() with POST data only. _verifyReturnToArgs() checks that any query parameters in return_to are present with the same values in the message. "jainrain_nonce" query parameter isn't present in POST data, so _verifyReturnToArgs() fails. _verifyReturnToArgs() should instead always check that any query parameters in return_to are present with the same values in the URL of the HTTP request the RP received, as required by the OpenID specification. This patch corrects this behavior

If that was done by a MitM, verifyReturnToArgs will catch it as long as the parsed data you pass to consumer.complete is the same as the parsed data your application uses. verifyReturnToArgs can't catch that if you don't let it inspect the post data!

return_to is unrelated to POST data: "there's no way to thread data through and get it back in POST". To verify that any query parameters in return_to are present with the same values in the URL of the HTTP request the RP received, _verifyReturnToArgs() doesn't need to inspect the POST data, it needs to inspect the "current_url" parameter of consumer.complete(). This patch corrects _verifyReturnToArgs() to always inspect the "current_url" parameter of consumer.complete(), as required by the specification

Thanks again for reviewing this Kevin!

[timcappalli]

This repo is being archived. Closing PR.