Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Interoperate with conformant providers that send positive assertions as a POST #25

Open
wants to merge 1 commit into from

2 participants

@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

@jablko 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
8bf3faa
@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 6, 2011
  1. @jablko

    Conform to spec: Support providers which send positive assertions as …

    jablko authored
    …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
This page is out of date. Refresh to see the latest.
Showing with 30 additions and 35 deletions.
  1. +16 −14 openid/consumer/consumer.py
  2. +14 −21 openid/test/test_consumer.py
View
30 openid/consumer/consumer.py
@@ -657,17 +657,20 @@ def _checkReturnTo(self, message, return_to):
against a return_to URL from an application. Return True on
success, False on failure.
"""
- # Check the openid.return_to args against args in the original
- # message.
+ # Check the return_to base URL against the one in the message.
+ msg_return_to = message.getArg(OPENID_NS, 'return_to')
+ if msg_return_to is None:
+ oidutil.log('Response has no return_to')
+ return False
+
+ # Check the openid.return_to args against args in the URL from the
+ # application.
try:
- self._verifyReturnToArgs(message.toPostArgs())
+ self._verifyReturnToArgs(msg_return_to, return_to)
except ProtocolError, why:
oidutil.log("Verifying return_to arguments: %s" % (why[0],))
return False
- # Check the return_to base URL against the one in the message.
- msg_return_to = message.getArg(OPENID_NS, 'return_to')
-
# The URL scheme, authority, and path MUST be the same between
# the two URLs.
app_parts = urlparse(urinorm.urinorm(return_to))
@@ -839,19 +842,17 @@ def _idResCheckForFields(self, message):
raise ProtocolError('"%s" not signed' % (field,))
- def _verifyReturnToArgs(query):
+ def _verifyReturnToArgs(msg_return_to, return_to):
"""Verify that the arguments in the return_to URL are present in this
- response.
+ URL.
"""
- message = Message.fromPostArgs(query)
- return_to = message.getArg(OPENID_NS, 'return_to')
-
- if return_to is None:
- raise ProtocolError('Response has no return_to')
+ parsed_url = urlparse(msg_return_to)
+ rt_query = parsed_url[4]
+ parsed_args = cgi.parse_qsl(rt_query)
parsed_url = urlparse(return_to)
rt_query = parsed_url[4]
- parsed_args = cgi.parse_qsl(rt_query)
+ query = dict(cgi.parse_qsl(rt_query))
for rt_key, rt_value in parsed_args:
try:
@@ -866,6 +867,7 @@ def _verifyReturnToArgs(query):
# Make sure all non-OpenID arguments in the response are also
# in the signed return_to.
+ message = Message.fromPostArgs(query)
bare_args = message.getArgs(BARE_NS)
for pair in bare_args.iteritems():
if pair not in parsed_args:
View
35 openid/test/test_consumer.py
@@ -1075,43 +1075,36 @@ def setUp(self):
self.consumer = GenericConsumer(store)
def test_returnToArgsOkay(self):
- query = {
- 'openid.mode': 'id_res',
- 'openid.return_to': 'http://example.com/?foo=bar',
- 'foo': 'bar',
- }
# no return value, success is assumed if there are no exceptions.
- self.consumer._verifyReturnToArgs(query)
+ self.consumer._verifyReturnToArgs(
+ 'http://example.com/?foo=bar',
+ 'http://example.com/?openid.mode=id_res&openid.return_to=http%3A%2F%2Fexample.com%2F%3Ffoo%3Dbar&foo=bar')
def test_returnToArgsUnexpectedArg(self):
- query = {
- 'openid.mode': 'id_res',
- 'openid.return_to': 'http://example.com/',
- 'foo': 'bar',
- }
# no return value, success is assumed if there are no exceptions.
self.failUnlessRaises(ProtocolError,
- self.consumer._verifyReturnToArgs, query)
+ self.consumer._verifyReturnToArgs,
+ 'http://example.com/',
+ 'http://example.com/?openid.mode=id_res&openid.return_to=http%3A%2F%2Fexample.com%2F&foo=bar')
def test_returnToMismatch(self):
- query = {
- 'openid.mode': 'id_res',
- 'openid.return_to': 'http://example.com/?foo=bar',
- }
# fail, query has no key 'foo'.
self.failUnlessRaises(ValueError,
- self.consumer._verifyReturnToArgs, query)
+ self.consumer._verifyReturnToArgs,
+ 'http://example.com/?foo=bar',
+ 'http://example.com/?openid.mode=id_res&openid.return_to=http%3A%2F%2Fexample.com%2F%3Ffoo%3Dbar')
- query['foo'] = 'baz'
# fail, values for 'foo' do not match.
self.failUnlessRaises(ValueError,
- self.consumer._verifyReturnToArgs, query)
+ self.consumer._verifyReturnToArgs,
+ 'http://example.com/?foo=bar',
+ 'http://example.com/?openid.mode=id_res&openid.return_to=http%3A%2F%2Fexample.com%2F%3Ffoo%3Dbar&foo=baz')
def test_noReturnTo(self):
query = {'openid.mode': 'id_res'}
- self.failUnlessRaises(ValueError,
- self.consumer._verifyReturnToArgs, query)
+ return_to = "http://some.url/path?foo=bar"
+ self.failIf(self.consumer._checkReturnTo(Message.fromPostArgs(query), return_to))
def test_completeBadReturnTo(self):
"""Test GenericConsumer.complete()'s handling of bad return_to
Something went wrong with that request. Please try again.