Re: [OAUTH-WG] I-D Action: draft-ietf-oauth-spop-09.txt

"Manger, James" <James.H.Manger@team.telstra.com> Thu, 05 February 2015 01:44 UTC

Return-Path: <James.H.Manger@team.telstra.com>
X-Original-To: oauth@ietfa.amsl.com
Delivered-To: oauth@ietfa.amsl.com
Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id E47ED1A00BF for <oauth@ietfa.amsl.com>; Wed, 4 Feb 2015 17:44:09 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 0.498
X-Spam-Level:
X-Spam-Status: No, score=0.498 tagged_above=-999 required=5 tests=[BAYES_05=-0.5, HELO_EQ_AU=0.377, HOST_EQ_AU=0.327, RCVD_IN_DNSWL_LOW=-0.7, RELAY_IS_203=0.994] autolearn=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xZOh_jfMLOvF for <oauth@ietfa.amsl.com>; Wed, 4 Feb 2015 17:44:08 -0800 (PST)
Received: from ipxcvo.tcif.telstra.com.au (ipxcvo.tcif.telstra.com.au [203.35.135.208]) by ietfa.amsl.com (Postfix) with ESMTP id E3AC21A000D for <oauth@ietf.org>; Wed, 4 Feb 2015 17:44:06 -0800 (PST)
X-IronPort-AV: E=Sophos;i="5.09,521,1418043600"; d="scan'208";a="68305842"
Received: from unknown (HELO ipcavi.tcif.telstra.com.au) ([10.97.217.200]) by ipocvi.tcif.telstra.com.au with ESMTP; 05 Feb 2015 12:17:45 +1100
X-IronPort-AV: E=McAfee;i="5600,1067,7702"; a="331952063"
Received: from wsmsg3706.srv.dir.telstra.com ([172.49.40.80]) by ipcavi.tcif.telstra.com.au with ESMTP; 05 Feb 2015 12:43:43 +1100
Received: from WSMSG3153V.srv.dir.telstra.com ([172.49.40.159]) by wsmsg3706.srv.dir.telstra.com ([172.49.40.80]) with mapi; Thu, 5 Feb 2015 12:43:42 +1100
From: "Manger, James" <James.H.Manger@team.telstra.com>
To: "oauth@ietf.org" <oauth@ietf.org>
Date: Thu, 5 Feb 2015 12:43:41 +1100
Thread-Topic: [OAUTH-WG] I-D Action: draft-ietf-oauth-spop-09.txt
Thread-Index: AdBA1AEYXq7rjRewS0OREYk72TcMvgAAIblA
Message-ID: <255B9BB34FB7D647A506DC292726F6E12851EBA8C3@WSMSG3153V.srv.dir.telstra.com>
References: <20150204234040.19482.87437.idtracker@ietfa.amsl.com>
In-Reply-To: <20150204234040.19482.87437.idtracker@ietfa.amsl.com>
Accept-Language: en-US, en-AU
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
acceptlanguage: en-US, en-AU
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Archived-At: <http://mailarchive.ietf.org/arch/msg/oauth/WyCAKDnFcE8IobtaTETUoc3N64c>
Subject: Re: [OAUTH-WG] I-D Action: draft-ietf-oauth-spop-09.txt
X-BeenThere: oauth@ietf.org
X-Mailman-Version: 2.1.15
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: Thu, 05 Feb 2015 01:44:10 -0000

>     Title           : Proof Key for Code Exchange by OAuth Public Clients
> 	Filename        : draft-ietf-oauth-spop-09.txt
> https://tools.ietf.org/html/draft-ietf-oauth-spop-09


Some nits on this draft:

1. 42 chars.
The lower limit of 42 chars for code_verifier: is not mentioned in prose (just the upper limit); is too high (128-bits=22-chars is sufficient); and doesn't correspond to 256-bits (BASE64URL-ENCODE(32 bytes) gives 43 chars, not 42).

2. 
Quotes around "code_verifier" and "code_challenge" in prose are okay, though not really necessary as the underscore is enough to distinguish them as technical labels. Quotes around these terms in formula is bad as it looks like the formula applies to the 13 or 14 chars of the label. The quoting is also used inconsistently.
Suggestion: remove all quotes around "code_verifier" and "code_challenge" in prose and formula.
For example, change ASCII("code_verifier") to ASCII(code_verifier).

3.
Two ways to check code_verifier are given in appendix B, whereas only one of these is mentioned in section 4.6.
  SHA256(verifier) === B64-DECODE(challenge)
  B64-ENCODE(SHA256(verifier)) === challenge

I suggest only mentioning the 2nd (change 4.6 to use the 2nd, and drop the 1st from appendix B). It is simpler to mention only one. It also means base64url-decoding is never done, and doesn't need to be mentioned in the spec.


4.
Expand "MTI" to "mandatory to implement".

P.S. Suggesting code challenge method names not exceed 8 chars to be compact is a bit perverse given the field holding these values has the long name "code_challenge_method" ;)

--
James Manger