Subject: Re: [OAUTH-WG] Gen Art LC review of draft-hammer-oauth-03

From: Eran Hammer-Lahav <[email-masked]>
Date: Sun, 15 Nov 2009 15:29:25 -0700

First, thanks!

The following changes have been submitted as a -04 revision.

> -----Original Message-----
> From: Avshalom Houri [mailto:[email-masked]]
> Sent: Tuesday, November 10, 2009 2:25 AM
> To: Eran Hammer-Lahav; [email-masked]; General Area Review Team
> Cc: .Lisa Dusseault
> Subject: Gen Art LC review of draft-hammer-oauth-03
> 
> I have been selected as the General Area Review Team (Gen-ART)
> reviewer for this draft (for background on Gen-ART, please see
> http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).
> 
> Please resolve these comments along with any other Last Call comments
> you may receive.
> 
> Document: draft-hammer-oauth-03
> Reviewer: Avshalom Houri
> Review Date: 2009-11-10
> IETF LC End Date: 2009-11-06
> IESG Telechat date: (if known)
> 
> Summary: Draft is almost ready. Needs some more work to improve
> readability and structure.
> 
> Major issues:
> 
> Section 3.3.1.1.  Collect Request Parameters. It is very hard to
> understand this whole section. It seems that it belongs more to the
> later parts of the document.

Entire section reorganized and rewritten to align the narrative with the protocol workflow. No changes to the normative language.

> Minor issues:
> 
> Lines 421-423:
>    nor does it include most HTTP entity-headers.  The importance of the
>    signature base string scope is that the authenticity of the excluded
>    components cannot be verified using the signature.
> 
> Could not understand the sentence starting with "The importance"

Replaced with: "It is important to note that the server cannot verify the authenticity of the excluded request components without using additional protections such as SSL/TLS or other methods."

> Lines 627-636
>    4.  If the URI includes an empty path, it MUST be included as "/".
> 
>    For example:
> 
>    +----------------------------------+-------------------------------+
>    | The request URI                  | Is included in base string as |
>    +----------------------------------+-------------------------------+
>    | HTTP://EXAMPLE.com:80/r/x?id=123 | http://example.com/r/x        |
>    | https://example.net:8080?q=1#top | https://example.net:8080/     |
>    +----------------------------------+-------------------------------+
> 
> Does it mean that the granularity here is only for whole resource? If
> so it should be mentioned somewhere.

No. This was a result of the way the spec was written, discussing resource URIs instead of HTTP requests. Since HTTP requires normalizing empty paths as '/', the spec had to mention that when discussing a URI such as http://example.com. However, with the new revision, the examples have been replaced with HTTP requests (instead of URIs), and the entire spec has been defined in the context of HTTP 1.1.

> Section 4. Redirection-Based Authorization seems to be more correctly
> placed in the beginning of the document.

This seems like a good idea. I am going to try it but not sure how the document will read to fresh eyes (anyone?).

> Section 6. Security Considerations
> 
> I like the detailed explanations but it may be good to have some
> preface that will describe the class of threats described etc.

I am happy for text suggestions. Writing security text is not my area.

> Appendix should be part of the document as an example.

The document is full of examples. This is just a complete walkthrough. I think it is fine as an appendix.

> Nits/editorial comments:
> 
> Line: 360:
>    A nonce is a random string, uniquely generated to allows the server
> ->    A nonce is a random string, uniquely generated to allow the
> server
> 
> Line 378:
>   client needs to prove it is the rightful owner of the credentials.
> ->    client needs to prove that it is the rightful owner of the
> credentials.
> 
> Line 405:
>    (or a sting of an equivalent value), and includes it in the
> -> (or a string of an equivalent value), and includes it in the

Yep.

> 
> Thanks & sorry for the late review
> --Avshalom

Thanks again. This was helpful. If you could review the latest revision and confirm the major concern I would greatly appreciate it.

EHL