Subject: [secdir] review of draft-ietf-oauth-introspection-09

From: Stephen Kent <[email-masked]>
Date: Mon, 01 Jun 2015 15:05:16 -0400

I reviewed this document as part of the security directorate's ongoing 
effort to review all IETF documents being processed by the IESG.These 
comments were written with the intent of improving security requirements 
and considerations in IETF drafts.Comments not addressed in last call 
may be included in AD reviews during the IESG review.Document editors 
and WG chairs should treat these comments just like any other last call 
comments.

This document describes an API for use with OAuth 2.0. The API enables 
the recipient of a token to query an authorization server to determine 
the set of metadata presented to the server (by the OAuth client) when 
the token was issued. This API is intended to overcome the lack of a 
standard way to convey token metadata in the OAuth 2.0 context.

I didn’t find any significant security problems with the document, but 
there are a number of places where the exposition needs improvement.

Specific comments

Section 2

The text says:

The introspection endpoint MUST be protected by a transport-layer 
security mechanism as described in Section 4.

It might be clearer to say here that the token endpoint MUST communicate 
security with the introspection endpoint, and that TLS 1.2 is the 
mandatory-to-implement mechanism for such security. Also, the text 
should be clearer about the relationship between an authorization server 
and an introspection endpoint. In some places the two terms seem to be 
used interchangeably.

Section 2.1

The texts says:

The endpoint MAY allow other parameters to provide further context to 
the query.

How about:

The endpoint MAY *accept* other parameters to provide further context to 
the query.

How does a protected resource know which other parameters an 
introspection endpoint requires/accepts?

This section mandates (MUST) protection against “unauthorized token 
scanning” but mandates no standard way to do so. [Also, when would a 
scanning “attack” be authorized ;-)]

The text says:

For example, the following example shows a protected resource calling 
the token introspection endpoint to query about an OAuth 2.0 bearer.

How about:

For example, the following example shows a protected resource calling 
the token introspection endpoint to query about an OAuth 2.0 bearer *token*.

Section 2.2

He text says:

The authorization server MAY respond differently to different

protected resources making the same request.For instance, an

authorization server MAY limit which scopes from a given token are

returned for each protected resource in order to prevent protected

resources from learning more about the larger network than is

necessary for their function.

How about:

*An*authorization server MAY respond differently to different

protected resources making the same request.For instance, an

authorization server MAY limit which scopes from a given token are

returned *to* each protected resource in order to prevent *a *protected

resources from learning more about the larger network than is

necessary for *its* *operation*.

Section 3.1

Experience suggests a longer review period is appropriate, e.g., to 
accommodate holidays, vacations, etc.

The text says:

IANA must only accept registry updates from the Designated Expert(s)

and should direct all requests for registration to the review mailing

list.

How about:

IANA *MUST* accept registry updates *only* from the Designated Expert(s)

and *SHOULD* direct all requests for registration to the review mailing

list.

Section 3.1.1

If a proposed Name matches one already registered as per 7519, ought it 
not have an “equivalent” (vs. “comparable”) definition if it is to be 
accepted?

Section 4

The text lists four checks to be performed by an authorization server, 
yet the introduction to this list says “For instance:” A more normative 
introduction would seem appropriate here. I realize that the text says 
that not all of these checks may be applicable in all OAuth 2.0 
contexts, but since each check begins with “If the token …” isn’t that a 
sufficient caveat?

The text says:

If an authorization server fails to perform any applicable check, the

resource server could make an errant security decision based on that

response.

How about:

If an authorization server fails to perform any applicable check, the

resource server could make an *erroneous* security decision based on 
that response.

The text says:

per RFC 6125 [RFC6125].

How about:

as specified in [RFC6125].

The discussion of introspection endpoint response caching seems to omit 
a simple, useful heuristic. Since the expiration time for a token is an 
optional parameter in the response, why not say that IF this parameter 
is present, the response MUST NOT be cached beyond that time?

Section 5

This section says: “One way to limit disclosure is to require

authorization to call the introspection endpoint and to limit calls

to only registered and trusted protected resource servers.” But

Section 4 says: “… the authorization server MUST require authentication 
of protected resources that need to access the introspection endpoint 
and SHOULD require protected resources to be specifically authorized to 
call the introspection endpoint.”It seems that the requirement imposed 
in Section 4 already mandate what is merely suggested in Section 5.

---

From: Justin Richer <[email-masked]>
Date: Mon, 1 Jun 2015 17:26:58 -0400

Stephen, thanks for the review. Comments inline.

> On Jun 1, 2015, at 3:05 PM, Stephen Kent <[email-masked]> wrote:
> 
> I reviewed this document as part of the security directorate's ongoing effort to review all IETF documents being processed by the IESG.  These comments were written with the intent of improving security requirements and considerations in IETF drafts.  Comments not addressed in last call may be included in AD reviews during the IESG review.  Document editors and WG chairs should treat these comments just like any other last call comments.
> 
> This document describes an API for use with OAuth 2.0. The API enables the recipient of a token to query an authorization server to determine the set of metadata presented to the server (by the OAuth client) when the token was issued. This API is intended to overcome the lack of a standard way to convey token metadata in the OAuth 2.0 context.
> 
> I didn’t find any significant security problems with the document, but there are a number of places where the exposition needs improvement.
> 
> Specific comments
> 
> Section 2
> 
> The text says:
> 
> The introspection endpoint MUST be protected by a transport-layer security mechanism as described in Section 4.
> 
> It might be clearer to say here that the token endpoint MUST         communicate security with the introspection endpoint, and that TLS 1.2 is the mandatory-to-implement mechanism for such security. Also, the text should be clearer about the relationship between an authorization server and an introspection endpoint. In some places the two terms seem to be used interchangeably.

This language was imported from the soon-to-be-published OAuth Dynamic Registration drafts, including the forward reference. We don’t want to repeat the requirement text.

> 
> Section 2.1
> 
> The texts says:
> 
> The endpoint MAY allow other parameters to provide further context to the query.
> 
> How about:
> 
> The endpoint MAY accept other parameters to provide further context to the query.

Thanks, that’s a better wording.

> 
> How does a protected resource know which other parameters an introspection endpoint requires/accepts?
> 
> 

We’re assuming it would be either an extension or deployment-specific.

> 
> This section mandates (MUST) protection against “unauthorized token scanning” but mandates no standard way to do so. [Also, when would a scanning “attack” be authorized ;-)]
> 

Is there something more specific we can recommend here? Perhaps in the security considerations section.

And the attack could actually take place where an authorized protected resource goes fishing for other valid tokens. The attack isn’t authorized, but the client is … and yes that’s not what we meant but I’m sticking to it.  ;)

> 
> The text says:
> 
> For example, the following example shows a protected resource calling the token introspection endpoint to query about an OAuth 2.0 bearer.
> 
> How about:
> 
> For example, the following example shows a protected resource calling the token introspection endpoint to query about an OAuth 2.0 bearer token.
> 

Thanks, that’s a typo. Good catch!

>    protected resources making the same request.  For instance, an
>    An authorization server MAY respond differently to different
>    protected resources making the same request.  For instance, an
>    returned to each protected resource in order to prevent a protected
>    necessary for its operation.

I prefer the definite article in the text, but the later correction is helpful.

> 
> 
> Section 3.1
> 
> Experience suggests a longer review period is appropriate, e.g., to accommodate holidays, vacations, etc.
> 
> The text says:
> 
> IANA MUST accept registry updates only from the Designated Expert(s)
> and SHOULD direct all requests for registration to the review mailing
> list.

This text was imported from a template, I’d be glad to change it if there’s a better one.

> 
> Section 3.1.1
> 
> If a proposed Name matches one already registered as per 7519, ought it not have an “equivalent” (vs. “comparable”) definition if it is to be accepted?
> 
> 

It really is comparable, since the contexts are pretty different.

> 
> Section 4
> 
> The text lists four checks to be performed by an authorization server, yet the introduction to this list says “For instance:” A more normative introduction would seem appropriate here. I realize that the text says that not all of these checks may be applicable in all OAuth 2.0 contexts, but since each check begins with “If the token …” isn’t that a sufficient caveat?
> 
> 

This is not a normative requirement, since everything is contextual to the nature of the tokens issued by the authorization server.

> resource server could make an erroneous security decision based on that response.
> 

Thanks, I think that’s a better word.

> as specified in [RFC6125].

That’s fine with me, thanks.

> The discussion of introspection endpoint response caching seems to omit a simple, useful heuristic. Since the expiration time for a token is an optional parameter in the response, why not say that IF this parameter is present, the         response MUST NOT be cached beyond that time?
> 
> 

That’s a good example, I’ll add that.

> Section 4 says: “… the authorization server MUST require authentication of protected resources that need to access the introspection endpoint and SHOULD require protected resources to be specifically authorized to call the introspection endpoint.”  It seems that the requirement imposed in Section 4 already mandate what is merely suggested in Section 5.

The requirement in section 4 came later than the suggestion in section 5. I’ll reword the security consideration to provide further justification for the requirement.

Thanks for your review!

 — Justin

---

From: Stephen Kent <[email-masked]>
Date: Thu, 04 Jun 2015 12:18:30 -0400

Justin,

>> ...
>> It might be clearer to say here that the token endpoint MUST 
>> communicate security with the introspection endpoint, and that TLS 
>> 1.2 is the mandatory-to-implement mechanism for such security. Also, 
>> the text should be clearer about the relationship between an 
>> authorization server and an introspection endpoint. In some places 
>> the two terms seem to be used interchangeably.
>>
>
> This language was imported from the soon-to-be-published OAuth Dynamic 
> Registration drafts, including the forward reference. We don’t want to 
> repeat the requirement text.
I didn't review the dynamic registration docs, or I would have made the 
same comment there :-).

If you don't want to repeat requirements text, then I suggest you might 
re-word the sentence
I cited so that it's clearer. Saying that an endpoint must be protected 
by a transport layer security mechanism seems needlessly ambiguous.

> ...
an extension to what?

it seems that a primary motivation for this document is to fix problems 
that arose
because of inadequate specifications for OAuth token syntax. In that 
light, this description
of "other parameters" seems to perpetuate this sort of problem.
> security considerations section.
yes, the SC section is where there should be suggestions on how to 
protect against such attacks. if you can't recommend something specific, 
then mandating that something be
done seems possibly vacuous, a MUST that will never be realized in 
practice (see RFC 6919).
>
> And the attack could actually take place where an authorized protected 
> resource goes fishing for other valid tokens. The attack isn’t 
> authorized, but the client is … and yes that’s not what we meant but 
> I’m sticking to it.  ;)
so, as I said, the attack is not authorized, and the phrase 
"unauthorized attack" will
make most security experts cringe. is cringing a goal?
>> ...
one should not assume that a doc that has made it through the RFC 
approval process
is perfect.
>
>> Section 3.1.1
>>
>> If a proposed Name matches one already registered as per 7519, ought 
>> it not have an “equivalent” (vs. “comparable”) definition if it is to 
>> be accepted?
>>
>>
>
if the contexts are so different, and thus the semantics are not 
equivalent, why is the reuse of the name, and the potential confusion 
acceptable?
>
>> Section 4
>>
>> The text lists four checks to be performed by an authorization 
>> server, yet the introduction to this list says “For instance:” A more 
>> normative introduction would seem appropriate here. I realize that 
>> the text says that not all of these checks may be applicable in all 
>> OAuth 2.0 contexts, but since each check begins with “If the token …” 
>> isn’t that a sufficient caveat?
>>
>>
>
> This is not a normative requirement, since everything is contextual to 
> the nature of the tokens issued by the authorization server.
so you're saying that there is no set of checks that every authorization 
server
can be expected to perform? not even token expiration?

Steve

---

From: Justin Richer <[email-masked]>
Date: Thu, 4 Jun 2015 16:14:42 -0400

Stephen,

> On Jun 4, 2015, at 12:18 PM, Stephen Kent <[email-masked]> wrote:
> 
> Justin,
> 
> 
> I didn't review the dynamic registration docs, or I would have made the same comment there :-).
> 
> If you don't want to repeat requirements text, then I suggest you might re-word the sentence
> I cited so that it's clearer. Saying that an endpoint must be protected by a transport layer security mechanism seems needlessly ambiguous.

The section pointed to in the forward reference in that sentence has the details. If you have a better way to word this, please suggest text.

>> We’re assuming it would be either an extension or deployment-specific.

An extension to this specification that defines other parameters and their meanings.

> it seems that a primary motivation for this document is to fix problems that arose
> because of inadequate specifications for OAuth token syntax. In that light, this description

I wouldn’t call OAuth having “inadequate” token syntax specification, it has *no* token syntax, and that’s on purpose. This specification actually doesn’t define a token syntax, either, but it defines a web API for getting token information regardless of the token syntax. The token itself can still be a signed JWT, or a UUID, or a random hex blob; OAuth doesn’t care, and that’s a good thing.

> yes, the SC section is where there should be suggestions on how to protect against such attacks. if you can't recommend something specific, then mandating that something be
> done seems possibly vacuous, a MUST that will never be realized in practice (see RFC 6919).

OK, we’ll try to add some more examples.

Cute reference.

> so, as I said, the attack is not authorized, and the phrase "unauthorized attack" will

We can change the wording to reduce the cringe level.

> one should not assume that a doc that has made it through the RFC approval process
> is perfect.
>> 
>>> 
>>> Section 3.1.1
>>> 
>>> 
> if the contexts are so different, and thus the semantics are not equivalent, why is the reuse of the name, and the potential confusion acceptable?

Besides, if we simply prohibit reuse of the name, we’ll have similar-but-confusing names, like “expires” vs. “exp”, in the two different places. The meanings are close enough, with the details being context-specific, that the names should be the same. Previously it was the same registry, but that was causing even more confusion.

> so you're saying that there is no set of checks that every authorization server

That’s correct. Not all tokens expire, so it doesn’t make sense to mandate checking for expiration. Every recommendation here is something that at least one known implementation of this protocol already does in the wild, but not every implementation does every check because it simply doesn’t make sense to do so. That’s why the checks are phrased as “if the token” and the list is given as a “for instance” example of what to do. We would also expect a server implementation to implement any other checks that it might have in store, like “is my database online and can I find the token in the these-are-valid-tokens table”.

 — Justin

---

From: Justin Richer <[email-masked]>
Date: Mon, 22 Jun 2015 20:51:14 -0400

Stephen,

I’ve uploaded a new draft that should address your comments below:

Draft: https://tools.ietf.org/html/draft-ietf-oauth-introspection-10 <https://tools.ietf.org/html/draft-ietf-oauth-introspection-10>

Diff: https://tools.ietf.org/rfcdiff?url2=draft-ietf-oauth-introspection-10.txt <https://tools.ietf.org/rfcdiff?url2=draft-ietf-oauth-introspection-10.txt>

Please let me know if you have any further questions,
 — Justin

> On Jun 4, 2015, at 4:14 PM, Justin Richer <[email-masked]> wrote:
> 
> Stephen,
> 
>> On Jun 4, 2015, at 12:18 PM, Stephen Kent <[email-masked] <mailto:[email-masked]>> wrote:
>> 
>> Justin,
>> 
>>