Subject: [Gen-art] Gen Art LC review of draft-hammer-oauth-03

From: Avshalom Houri <[email-masked]>
Date: Tue, 10 Nov 2009 12:25:11 +0200

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.

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"

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.

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

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.

Appendix should be part of the document as an example.

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

Thanks & sorry for the late review
--Avshalom

---

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
> 
> 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.

> 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."

> 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.

> 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?).

> 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.

> -> (or a string of an equivalent value), and includes it in the

Yep.

> Thanks & sorry for the late review

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

EHL