Re: [OAUTH-WG] AD Review of -22 (part II)

Eran Hammer <eran@hueniverse.com> Sat, 21 January 2012 01:04 UTC

Return-Path: <eran@hueniverse.com>
X-Original-To: oauth@ietfa.amsl.com
Delivered-To: oauth@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id BE10921F856F for <oauth@ietfa.amsl.com>; Fri, 20 Jan 2012 17:04:29 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.871
X-Spam-Level:
X-Spam-Status: No, score=-1.871 tagged_above=-999 required=5 tests=[AWL=-0.472, BAYES_00=-2.599, J_CHICKENPOX_22=0.6, J_CHICKENPOX_35=0.6]
Received: from mail.ietf.org ([12.22.58.30]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id QGREvWqegynR for <oauth@ietfa.amsl.com>; Fri, 20 Jan 2012 17:04:28 -0800 (PST)
Received: from p3plex1out02.prod.phx3.secureserver.net (p3plex1out02.prod.phx3.secureserver.net [72.167.180.18]) by ietfa.amsl.com (Postfix) with SMTP id A407621F856C for <oauth@ietf.org>; Fri, 20 Jan 2012 17:04:28 -0800 (PST)
Received: (qmail 13919 invoked from network); 21 Jan 2012 01:04:27 -0000
Received: from unknown (HELO smtp.ex1.secureserver.net) (72.167.180.21) by p3plex1out02.prod.phx3.secureserver.net with SMTP; 21 Jan 2012 01:04:27 -0000
Received: from P3PW5EX1MB01.EX1.SECURESERVER.NET ([10.6.135.20]) by P3PW5EX1HT003.EX1.SECURESERVER.NET ([72.167.180.21]) with mapi; Fri, 20 Jan 2012 18:04:27 -0700
From: Eran Hammer <eran@hueniverse.com>
To: OAuth WG <oauth@ietf.org>
Date: Fri, 20 Jan 2012 18:04:17 -0700
Thread-Topic: AD Review of -22 (part II)
Thread-Index: AczXzezFz/6KwlD5Sz6HqyDpokz7nA==
Message-ID: <90C41DD21FB7C64BB94121FBBC2E723453AAB96544@P3PW5EX1MB01.EX1.SECURESERVER.NET>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
acceptlanguage: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [OAUTH-WG] AD Review of -22 (part II)
X-BeenThere: oauth@ietf.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: OAUTH WG <oauth.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/oauth>, <mailto:oauth-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/oauth>
List-Post: <mailto:oauth@ietf.org>
List-Help: <mailto:oauth-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/oauth>, <mailto:oauth-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sat, 21 Jan 2012 01:04:29 -0000

> -----Original Message-----
> From: oauth-bounces@ietf.org [mailto:oauth-bounces@ietf.org] On Behalf
> Of Stephen Farrell
> Sent: Thursday, October 13, 2011 10:13 AM

> Suggested non-trivial clarifications:
> -------------------------------------
> 
> (1) 1.3.4 - "previously arranged" might trigger discusses on the document
> since it implies that this spec might not be suited for broad use. I think that
> making it clear that the normal mode for client developers is to work against
> an existing service (AS and resource server) would help to clarify that such
> arrangements are ok here.

Added new 'Interoperability' section to the introduction:

          OAuth 2.0 provides a rich authorization framework with well-defined security properties.
          However, as a rich and highly extensible framework with many optional components, this
          specification is likely to produce a wide range of non-interoperable implementations.
          In addition, this specification leaves a few required components partially or fully
          undefined (e.g. client registration, authorization server capabilities, endpoint
          discovery).

          This protocol was design with the clear expectation that future work will define
          prescriptive profiles and extensions necessary to achieve full web-scale
          interoperability.          

There is no way to sugar coat reality and hopefully by being blunt about it upfront, we will avoid a prolonged debate about the protocol's failings in that regard.

> (2) p11, in step (F) is there a way to distinguish between an access token that
> is invalid due to expiry vs. e.g. data corruption? Section 6 refers to 5.2 for the
> error codes but its not clear to me which one is returned for this case. I think
> clarifying that in section 6 or 5.2 is needed.

That depends on the token specification. Steps C-F are outside the scope of this document. I'll note that.
 
> (3) p13, 2.2 and 2.3 - these things happens at registration time right? I think
> making that clear is needed since we don't specify a registration protocol
> here.

The entire section 2 is 'Client Registration' which is described as out of scope for implementation details.
 
> (4) 2.3.1 uses the term "token endpoint" without definition (its defined in
> section 3) and in particular without making it clear if both access and refresh
> token issuance is covered (I guess it is).

Changed to 'when sending requests using password authentication'.

> (5) The same text about x-www-form-urlencoded is repeated various times,
> it'd be better to do that once and refer to it where necessary.

Not enough to be worth the change.

> (6) 3.1.2.2 states the rules for when AS'es are to require registration of
> redirection URIs. I think you need to clarify that some. First, you use the term
> "redirection_uri" for both a "complete" URI and for a scheme/authority/path
> triple that can be added to via a query component which is confusing.
> Second, its overall a very complex rule with a MUST, two MAYs and 3
> SHOULDs.  I do think it could be made clearer by putting the MUST up front
> and separating issues related to complete URI and triples separately from the
> when something MUST be registered.

New text:

   The authorization server MUST require the following clients to
   register their redirection endpoint:

   o  Public clients.
   o  Confidential clients utilizing the implicit grant type.

   The authorization server SHOULD require all clients to register their
   redirection endpoint prior to utilizing the authorization endpoint

   The authorization server SHOULD require the client to provide the
   complete redirection URI (the client MAY use the "state" request
   parameter to achieve per-request customization).  If requiring the
   registration of the complete redirection URI is not possible, the
   authorization server SHOULD require the registration of the URI
   scheme, authority, and path (allowing the client to dynamically vary
   only the query component of the redirection URI when requesting
   authorization).

   The authorization server MAY allow the client to register multiple
   redirection endpoints.

   Lack of a redirection URI registration requirement can enable an
   attacker to use the authorization endpoint as open redirector as
   described in Section 10.15.

> (7) 4.2.1 and elsewhere - refers back to 3.1.2 for the way in which the
> redirection-uri is OPTIONAL - I'm not sure that's sufficiently clear, 3.1.2 is
> quite long and discusses a bunch of things - couldn't it be made clearer when
> the messages are defined?

The reference is not for the OPTIONAL definition. I changed the coma to a period.

>  More generally, is there no way to avoid the
> extensive cross-referencing in the message field definitions? E.g. 4.2.2 has
> references to 7.1 and 3.3, and others are similar. Organizing the text for the
> benefit of the reader is a good thing so would it be worthwhile to do an
> editing pass for just this purpose - to reduce the number of forward
> references and minimize the number of pointers in general?

We've gone back and forth on this for 22 drafts and this is the best balance between readability and consistency we found. I'm inclined to avoid another reshuffle.

> On a similar note, 4.1 & 4.2 call out specific errors, but 4.3 defers to 5.2, why?
> Be good if that could be made more consistent at the TOC level.

Because 4.3 doesn't use the authorization endpoint.

> (8) How can the AS protect against brute force attacks in 4.3.2?  I think you
> could give a bit more guidance, e.g. say to rate-limit or generate alerts or
> whatever, but not as normative text, just good hints.

Ok.

> (9) In 10.12 you say how a client can protect against CSRF via the state field
> but you don't say how the AS can do the same in order to satisfy the MUST in
> the last para of 10.12.  Can you not add a hint or reference here?

The WG sentiment was that server developers do not require as much hand holding as the client developers. If the authors of the original text want to propose text, I'm happy to include it.

> 
> Some nits: (Stuff that seemed more serious at first:-)
> ---------
> 
> (1) In 2.3.1 I think you're ruling out putting the client_id and client_secret in
> the request URL - is that right? If so, that's good, but I think it needs calling
> out since people do that all the time and it'd be good to say why its bad to do
> that.

Added:

           The parameters can only be transmitted in the request body and MUST NOT be included in the
            request URI

> (2) The redirection endpoint is introduced in 3.2.1 but is not listed at the start
> of section 3 which only lists two endpoints.

I think you mean 3.1.2. Added.

> (3) In 4.1.2 what does it mean to "attempt" to revoke tokens?  Why can't the
> AS just revoke them? (Where revoke == not accept them when they are
> next presented, right?)

Can't revoke self-encoded tokens (e.g. stateless on the server). Changed 'attempt' to 'when possible'

> (4) I think this is just language but wanna check. 4.1.2.1 and
> 4.2.2.1 errors have a state field. Text says: "If a valid "state"
> parameter was present..." which would imply that state variables are either
> valid or invalid according to the AS. I don't think that's the case, and nor
> should it be. (If it were, I'd have a security concern that I could use otherwise
> crap requests to probe for good state values.) s/valid// I think?

Yep.
 
> (5) I don't get the benefit of saying the client SHOULD ignore unknown fields
> in the response in 4.2.2 - what's effectively the difference between that and
> "MUST ignore" - I don't get it, and hence don't see why you don't say MUST
> ignore.

Changed all 'ignore' to MUST.
 
> (6) Why say "MUST NOT issue a refresh token" in 4.2.2? Are you making an
> assumption that access & refresh tokens are distinguishable to anyone other
> than the issuer? If not, then is this just saying "don't make a mistake"?

No. It's a security concern that developer might decide to be flexible and provider a refresh token in addition to an access token using this flow.

> (7) 5.1 says that the client SHOULD ignore "unrecognized response
> parameters" - does that mean unrecognized parameters in the JSON entity
> body? Is it clear enough as-is that those are "response parameters"?

Changed 'parameters' to 'value names in the response'.

> (8) How does the use of TLS on endpoints used for end-user interaction
> "reduce the risk of phishing attacks"? I don't get that. Maybe you mean that
> TLS+users actually checking server identity reduces the risk of successful
> phishes? I think that's a bit different. (I do like the MUST though.)

Dropped 'phishing'. If the authors of the original text would like to explain, I'll put it back in.

(To be continued...)

EHL