Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(851)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 7314065: Aud can be a list. Fixes issue #236.

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 2 months ago by jcgregorio_google
Modified:
11 years, 2 months ago
Reviewers:
dhermes
CC:
google-api-python-client_googlegroups.com
Visibility:
Public.

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -12 lines) Patch
M oauth2client/crypt.py View 2 chunks +9 lines, -5 lines 1 comment Download
M tests/test_oauth2client_jwt.py View 5 chunks +34 lines, -7 lines 4 comments Download

Messages

Total messages: 11
jcgregorio_google
11 years, 2 months ago (2013-02-12 15:45:14 UTC) #1
dhermes
https://codereview.appspot.com/7314065/diff/1/oauth2client/crypt.py File oauth2client/crypt.py (right): https://codereview.appspot.com/7314065/diff/1/oauth2client/crypt.py#newcode377 oauth2client/crypt.py:377: if set(audience) != set(aud): I don't think these need ...
11 years, 2 months ago (2013-02-12 16:46:39 UTC) #2
jcgregorio_google
http://openid.net/specs/openid-connect-basic-1_0.html#id.token.validation """The ID Token MUST be rejected if the ID Token does not list the ...
11 years, 2 months ago (2013-02-12 16:54:53 UTC) #3
dhermes
Great, this definitely agrees with my notion of subset, not equality of sets. On Tue, ...
11 years, 2 months ago (2013-02-12 16:56:55 UTC) #4
jcgregorio_google
On 2013/02/12 16:56:55, dhermes wrote: > Great, this definitely agrees with my notion of subset, ...
11 years, 2 months ago (2013-02-12 17:14:03 UTC) #5
dhermes
If the allowed audiences are [a, b] then both {a} and {b} are subsets of ...
11 years, 2 months ago (2013-02-12 17:24:12 UTC) #6
jcgregorio_google
If the allowed audience is [a], and the aud found in id_token is [a,b], the ...
11 years, 2 months ago (2013-02-12 18:41:18 UTC) #7
dhermes
Yes, I agree with that, and set(aud) <= set(audiences) i.e. set(['a', 'b']) <= set(['a']) is ...
11 years, 2 months ago (2013-02-12 18:59:50 UTC) #8
jcgregorio_google
Let's rewrite the comparisons as: set(aud in id_token) <= set(expected audience) and set(expected audience) <= ...
11 years, 2 months ago (2013-02-12 19:10:05 UTC) #9
dhermes
So what I'm saying is that "audience" isn't an "expected" audience/audience list, but rather a ...
11 years, 2 months ago (2013-02-12 19:15:36 UTC) #10
jcgregorio_google
11 years, 2 months ago (2013-02-12 19:19:51 UTC) #11 
On Tue, Feb 12, 2013 at 2:15 PM, Danny Hermes <dhermes@google.com> wrote:
> So what I'm saying is that "audience" isn't an "expected" audience/audience
> list, but rather a whitelist of allowed audiences.
>

Right, and this gets me back to where we started when I said:

"""
But, the discussion may be moot because the spec text I quoted from
above is from the OpenID spec, the spec for Signed JWTs implies that the
'aud' value is application dependent, so there may be cases where a
subset match may be appropriate. So we may need both kinds of checks
depending on what kind of id_token is being validated.
"""

So we need two calls, one that takes a whitelist of allowed audiences,
and a second verifier call that that takes the exact list of audiences
that should be present.

  -joe



>
> On Tue, Feb 12, 2013 at 11:09 AM, Joe Gregorio <jcgregorio@google.com>
> wrote:
>>
>> Let's rewrite the comparisons as:
>>
>>    set(aud in id_token) <= set(expected audience)
>>
>> and
>>
>>   set(expected audience)  <= set(aud in id_token)
>>
>> We can see that no matter which way you write it, it's wrong.
>>
>> If you use the comparison
>>     set(aud in id_token) <= set(expected audience)
>> and we have
>>     set(aud in id_token) = [a] and set(expected audience) = [a, b]
>> then the expression evaluates to True, but that's not right since 'b'
>> wasn't found
>> in the id_token and so this id_token is invalid.
>>
>> If you use the expression in the opposite order
>>     set(expected audience) <= set(aud in id_token)
>> and we have
>>     set(expected audience) = [a] and set(aud in id_token) = [a,b]
>> then this comparison evaluates to True, but that's not right
>> since 'b' is present in the id_token, is not in the expected audience
>> and is thus an 'unexpected' scope, which means this id_token
>> should be rejected.
>>
>>
>>
>>
>> On Tue, Feb 12, 2013 at 1:59 PM, Danny Hermes <dhermes@google.com> wrote:
>> > Yes, I agree with that, and
>> >
>> > set(aud) <= set(audiences)
>> >
>> > i.e.
>> >
>> > set(['a', 'b']) <= set(['a'])
>> >
>> > is False.
>> >
>> > Looking back, I now see that I mistakenly typed
>> >
>> > if not set(aud) <= set(audience):
>> >
>> > and I think that's what was causing the confusing.
>> >
>> >
>> >
>> > On Tue, Feb 12, 2013 at 10:40 AM, Joe Gregorio <jcgregorio@google.com>
>> > wrote:
>> >>
>> >> If the allowed audience is [a], and the aud found in id_token is
>> >> [a,b], the the id_token should be rejected.
>> >>
>> >>
>> >> On Tue, Feb 12, 2013 at 12:23 PM, Danny Hermes <dhermes@google.com>
>> >> wrote:
>> >> > If the allowed audiences are [a, b] then both {a} and {b} are subsets
>> >> > of
>> >> > this allowed audience list.
>> >> >
>> >> > I'm not sure what you're getting at by saying "...we only check for
>> >> > (a)
>> >> > then
>> >> > b is an unknown audience".
>> >> >
>> >> > From "(a) is a subset of (a, b)" it seems you mean the "audiences"
>> >> > value
>> >> > (server side) is [a, b] and from "but if an id_token contains b" it
>> >> > seems
>> >> > you mean the "aud" value from the token is b. Since {b} is a subset
>> >> > of
>> >> > {a,
>> >> > b}, this should be just fine.
>> >> >
>> >> > On Tue, Feb 12, 2013 at 9:14 AM, <jcgregorio@google.com> wrote:
>> >> >>
>> >> >>
>> >> >>
>> >> >> On 2013/02/12 16:56:55, dhermes wrote:
>> >> >>>
>> >> >>> Great, this definitely agrees with my notion of subset, not
>> >> >>> equality
>> >> >>
>> >> >> of
>> >> >>>
>> >> >>> sets.
>> >> >>
>> >> >>
>> >> >> That's not incorrect, (a) is a subset of (a, b), but if an id_token
>> >> >> contains b and we only check for (a) then b is an unknown audience
>> >> >> and
>> >> >> the id_token MUST be rejected.
>> >> >>
>> >> >> But, the discussion may be moot because the spec text I quoted from
>> >> >> above is from the OpenID spec, the spec for Signed JWTs implies that
>> >> >> the
>> >> >> 'aud' value is application dependent, so there may be cases where a
>> >> >> subset match may be appropriate. So we may need both kinds of checks
>> >> >> depending on what kind of id_token is being validated.
>> >> >>
>> >> >> I'll hold off on this change until we get some clarity from the
>> >> >> oauth
>> >> >> team.
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>> On Tue, Feb 12, 2013 at 8:54 AM, <mailto:jcgregorio@google.com>
>> >> >>> wrote:
>> >> >>
>> >> >>
>> >> >>> > http://openid.net/specs/**openid-connect-basic-1_0.html#**
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
id.token.validation<http://openid.net/specs/openid-connect-basic-1_0.html#id.token.validation>
>> >> >>
>> >> >>> >
>> >> >>> > """The ID Token MUST be rejected if the ID Token does not list
>> >> >>> > the
>> >> >>> > Client as a valid audience, or if it contains additional
>> >> >>> > audiences
>> >> >>
>> >> >> not
>> >> >>>
>> >> >>> > trusted by the client."""
>> >> >>> >
>> >> >>> >
>> >> >>> >
>> >> >>> > On 2013/02/12 16:46:39, dhermes wrote:
>> >> >>> >
>> >> >>> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
https://codereview.appspot.**com/7314065/diff/1/**oauth2client/crypt.py%3Chtt...>
>> >> >>>
>> >> >>> >> File oauth2client/crypt.py (right):
>> >> >>> >>
>> >> >>> >
>> >> >>> >
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
https://codereview.appspot.**com/7314065/diff/1/**oauth2client/crypt.py#**
>> >> >>>
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
newcode377<https://codereview.appspot.com/7314065/diff/1/oauth2client/crypt.py#newcode377>
>> >> >>
>> >> >>> >
>> >> >>> >> oauth2client/crypt.py:377: if set(audience) != set(aud):
>> >> >>> >> I don't think these need to be equal. I believe it should be
>> >> >>
>> >> >> something
>> >> >>>
>> >> >>> >>
>> >> >>> > like:
>> >> >>> >
>> >> >>> >  if not set(aud) <= set(audience):
>> >> >>> >>
>> >> >>> >
>> >> >>> >  In other words, the token can be minted with any subset of the
>> >> >>> >>
>> >> >>> > audiences in
>> >> >>> >
>> >> >>> >> mind.
>> >> >>> >>
>> >> >>> >
>> >> >>> >
>> >> >>> > https://codereview.appspot.**com/7314065/diff/1/tests/test_**
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
oauth2client_jwt.py<https://codereview.appspot.com/7314065/diff/1/tests/test_oauth2client_jwt.py>
>> >> >>>
>> >> >>> >
>> >> >>> >> File tests/test_oauth2client_jwt.py (right):
>> >> >>> >>
>> >> >>> >
>> >> >>> >
>> >> >>> > https://codereview.appspot.**com/7314065/diff/1/tests/test_**
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
oauth2client_jwt.py#newcode75<https://codereview.appspot.com/7314065/diff/1/tests/test_oauth2client_jwt.py#newcode75>
>> >> >>>
>> >> >>> >
>> >> >>> >> tests/test_oauth2client_jwt.**py:75: def
>> >> >>> >> _check_jwt_failure(self,
>> >> >>
>> >> >> jwt,
>> >> >>>
>> >> >>> >> expected_error, audience =
>> >> >>> >> Move
>> >> >>> >>
>> >> >>> >
>> >> >>> >
>> >> >>> mailto:audience=>
>> >> >>>
>> >> >>> >> '
>> >> >>> >>
>> >> >>> >
>> >> >>> >  to the same line and remove the spaces between the = sign since
>> >> >>> > a
>> >> >>> >>
>> >> >>> > keyword arg.
>> >> >>> >
>> >> >>> >  (I realize this is only a test, but still.)
>> >> >>> >>
>> >> >>> >
>> >> >>> >
>> >> >>> > https://codereview.appspot.**com/7314065/diff/1/tests/test_**
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
oauth2client_jwt.py#newcode88<https://codereview.appspot.com/7314065/diff/1/tests/test_oauth2client_jwt.py#newcode88>
>> >> >>>
>> >> >>> >
>> >> >>> >> tests/test_oauth2client_jwt.**py:88:
>> >> >>> >>
>> >> >>> mailto:>
>> >> >>> >> '):
>> >> >>> >> Ditto here.
>> >> >>> >>
>> >> >>> >
>> >> >>> >
>> >> >>> > https://codereview.appspot.**com/7314065/diff/1/tests/test_**
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
oauth2client_jwt.py#newcode120<https://codereview.appspot.com/7314065/diff/1/tests/test_oauth2client_jwt.py#newcode120>
>> >> >>>
>> >> >>> >
>> >> >>> >> tests/test_oauth2client_jwt.**py:120:
>> >> >>>
>> >> >>> >> Can you add an audience proper subset (i.e. subset but not
>> >> >>> >> equal)
>> >> >>
>> >> >> test
>> >> >>>
>> >> >>> >>
>> >> >>> > as well
>> >> >>> >
>> >> >>> >> if my comment on crypt.py checks out?
>> >> >>> >>
>> >> >>> >
>> >> >>> >
>> >> >>> > https://codereview.appspot.**com/7314065/diff/1/tests/test_**
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
oauth2client_jwt.py#newcode213<https://codereview.appspot.com/7314065/diff/1/tests/test_oauth2client_jwt.py#newcode213>
>> >> >>>
>> >> >>> >
>> >> >>> >> tests/test_oauth2client_jwt.**py:213: jwt =
>> >> >>>
>> >> >>> >>
>> >> >>> > crypt.make_signed_jwt(signer, {
>> >> >>> >
>> >> >>> >> This test may need to change pending my question in crypt.py.
>> >> >>> >>
>> >> >>> >
>> >> >>> >
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
https://codereview.appspot.**com/7314065/%3Chttps://codereview.appspot.com/73...>
>> >> >>
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>> --
>> >> >>> Danny Hermes
>> >> >>> Developer Programs Engineer
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >> https://codereview.appspot.com/7314065/
>> >> >
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Danny Hermes
>> >> > Developer Programs Engineer
>> >
>> >
>> >
>> >
>> > --
>> > Danny Hermes
>> > Developer Programs Engineer
>
>
>
>
> --
> Danny Hermes
> Developer Programs Engineer
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b