Subject: [OAUTH-WG] I-D Action: draft-ietf-oauth-spop-14.txt

From: [email-masked]
Date: Mon, 06 Jul 2015 16:05:50 -0700

A New Internet-Draft is available from the on-line Internet-Drafts directories.
 This draft is a work item of the Web Authorization Protocol Working Group of the IETF.

        Title           : Proof Key for Code Exchange by OAuth Public Clients
        Authors         : Nat Sakimura
                          John Bradley
                          Naveen Agarwal
	Filename        : draft-ietf-oauth-spop-14.txt
	Pages           : 20
	Date            : 2015-07-06

Abstract:
   OAuth 2.0 public clients utilizing the Authorization Code Grant are
   susceptible to the authorization code interception attack.  This
   specification describes the attack as well as a technique to mitigate
   against the threat through the use of Proof Key for Code Exchange
   (PKCE, pronounced "pixy").

The IETF datatracker status page for this draft is:
https://datatracker.ietf.org/doc/draft-ietf-oauth-spop/

There's also a htmlized version available at:
https://tools.ietf.org/html/draft-ietf-oauth-spop-14

A diff from the previous version is available at:
https://www.ietf.org/rfcdiff?url2=draft-ietf-oauth-spop-14

Please note that it may take a couple of minutes from the time of submission
until the htmlized version and diff are available at tools.ietf.org.

Internet-Drafts are also available by anonymous FTP at:
ftp://ftp.ietf.org/internet-drafts/

---

From: William Denniss <[email-masked]>
Date: Tue, 7 Jul 2015 17:38:20 -0700

In version 14, there's a typo on this line ("deso") in Section 7.2:

`"plain" method deso not protect`

Also, in the 1.1 Protocol Flow diagram, regarding the text:

`+ t(code_verifier), t`

I wonder if it makes more sense to represent as `+ t(code_verifier), "t"`
(note the quotes on the second 't') given that it's a string representation
of the method that's being sent?

On Mon, Jul 6, 2015 at 4:05 PM, <[email-masked]> wrote:

> _______________________________________________
> OAuth mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/oauth
>

---

From: John Bradley <[email-masked]>
Date: Tue, 7 Jul 2015 22:28:46 -0300

Thanks, I fixed my finger dyslexia for the next draft.

I changed it to t_m rather than “t”  I think that is clearer.  If I were to do it the other way XML2RFC would have double quotes in the text version.

John B. 
> On Jul 7, 2015, at 9:38 PM, William Denniss <[email-masked]> wrote:
> 
> I wonder if it makes more sense to represent as `+ t(code_verifier), "t"` (note the quotes on the second 't') given that it's a string representation of the method that's being sent?
> 
> 
> On Mon, Jul 6, 2015 at 4:05 PM, <[email-masked] <mailto:[email-masked]>> wrote:
> 
> https://datatracker.ietf.org/doc/draft-ietf-oauth-spop/ <https://datatracker.ietf.org/doc/draft-ietf-oauth-spop/>
> 
> https://tools.ietf.org/html/draft-ietf-oauth-spop-14 <https://tools.ietf.org/html/draft-ietf-oauth-spop-14>
> 
> https://www.ietf.org/rfcdiff?url2=draft-ietf-oauth-spop-14 <https://www.ietf.org/rfcdiff?url2=draft-ietf-oauth-spop-14>
> 
> 
> until the htmlized version and diff are available at tools.ietf.org <http://tools.ietf.org/>.
> 
> ftp://ftp.ietf.org/internet-drafts/ <ftp://ftp.ietf.org/internet-drafts/>
> 
> [email-masked] <mailto:[email-masked]>
> https://www.ietf.org/mailman/listinfo/oauth <https://www.ietf.org/mailman/listinfo/oauth>
>

---

From: William Denniss <[email-masked]>
Date: Tue, 7 Jul 2015 22:51:49 -0700

t_m works for me, I just think we should have some indication that it's
the name of the transform. Will you also update where it is referenced in
the description below Figure 2?

On Tue, Jul 7, 2015 at 6:28 PM, John Bradley <[email-masked]> wrote:

> I changed it to t_m rather than “t”  I think that is clearer.  If I were
> to do it the other way XML2RFC would have double quotes in the text version.
>
> John B.
>

---

From: William Denniss <[email-masked]>
Date: Wed, 8 Jul 2015 19:22:22 -0700

Following up the discussion on today's NAPPS call, I understand why plain
is not presented as the recommended approach in the spec (though it still
has some value over not doing PKCE at all, in that it mitigates against the
current known attack where a rogue app registers the same custom URI scheme
as another), but I feel that after all the back and forth the picture is a
little confusing.

In particular, 4.2 and 4.4.1 include some examples where plain is supported:

4.2
> Clients SHOULD use the S256 transformation.  The plain transformation is
> for compatibility with existing deployments and for constrained
> environments that can't use the S256 transformation.
>

4.4.1.
> If the client is capable of using "S256", it MUST use "S256", as "S256" is
> Mandatory To Implement (MTI) on the server. Clients are permitted to use
> "plain" only if they cannot support "S256" for some technical reason and
> knows that the server supports "plain".

But then 7.2 is very vocal that it MUST NOT be used for new implementations:

7.2
> Because of this, "plain" SHOULD NOT be used, and exists only
> for compatibility with deployed implementations where the request path
> is already protected.  The "plain" method MUST NOT be used in
> new implementations.

 What if those new implementations are constrained, as indicated in 4.2 and
4.4.1?

Also, while S256 is clearly indicated as MTI, little is said about "plain",
although it's alluded to that it's not MTI in 4.4.1 ("and knows that the
server supports "plain"").

Should we be more explicit upfront that "plain" is optional for servers to
support, if that's the intention?

---

From: Brian Campbell <[email-masked]>
Date: Thu, 9 Jul 2015 12:19:57 -0600

I agree with William that it's a little confusing. I get that there's a
desire to discourage using "plain" but perhaps the language (especially the
MUST NOT in 7.2) should be lightened up just a bit?

On Wed, Jul 8, 2015 at 8:22 PM, William Denniss <[email-masked]> wrote:

>> If the client is capable of using "S256", it MUST use "S256", as "S256"
>> is Mandatory To Implement (MTI) on the server. Clients are permitted to use
>  What if those new implementations are constrained, as indicated in 4.2
> and 4.4.1?
>
>
> Also, while S256 is clearly indicated as MTI, little is said about
> "plain", although it's alluded to that it's not MTI in 4.4.1 ("and knows
> that the server supports "plain"").
>

---

From: John Bradley <[email-masked]>
Date: Thu, 9 Jul 2015 15:46:00 -0300

I have made some edits to make it consistent.  They are checked into the butbucket repo nat and I use, but we can’t update the official draft during the freeze before the IETF meeting.

https://bitbucket.org/Nat/oauth-spop

> On Jul 9, 2015, at 3:19 PM, Brian Campbell <[email-masked]> wrote:
> 
> I agree with William that it's a little confusing. I get that there's a desire to discourage using "plain" but perhaps the language (especially the MUST NOT in 7.2) should be lightened up just a bit? 
> 
> On Wed, Jul 8, 2015 at 8:22 PM, William Denniss <[email-masked] <mailto:[email-masked]>> wrote:
> Following up the discussion on today's NAPPS call, I understand why plain is not presented as the recommended approach in the spec (though it still has some value over not doing PKCE at all, in that it mitigates against the current known attack where a rogue app registers the same custom URI scheme as another), but I feel that after all the back and forth the picture is a little confusing.
> 
> Clients SHOULD use the S256 transformation.  The plain transformation is for compatibility with existing deployments and for constrained environments that can't use the S256 transformation.
>  
> 4.4.1.
> If the client is capable of using "S256", it MUST use "S256", as "S256" is Mandatory To Implement (MTI) on the server. Clients are permitted to use "plain" only if they cannot support "S256" for some technical reason and knows that the server supports "plain".
> 
> Because of this, "plain" SHOULD NOT be used, and exists only for compatibility with deployed implementations where the request path is already protected.  The "plain" method MUST NOT be used in new implementations.
> 
>  What if those new implementations are constrained, as indicated in 4.2 and 4.4.1?
> 
> 
> Also, while S256 is clearly indicated as MTI, little is said about "plain", although it's alluded to that it's not MTI in 4.4.1 ("and knows that the server supports "plain"").
> 
> Should we be more explicit upfront that "plain" is optional for servers to support, if that's the intention?
> 
> 
> On Tue, Jul 7, 2015 at 10:51 PM, William Denniss <[email-masked] <mailto:[email-masked]>> wrote:
> t_m works for me, I just think we should have some indication that it's the name of the transform. Will you also update where it is referenced in the description below Figure 2?
> 
> 
> 
> On Tue, Jul 7, 2015 at 6:28 PM, John Bradley <[email-masked] <mailto:[email-masked]>> wrote:
>> On Jul 7, 2015, at 9:38 PM, William Denniss <[email-masked] <mailto:[email-masked]>> wrote:
>>

---

From: Kathleen Moriarty <[email-masked]>
Date: Fri, 10 Jul 2015 11:29:37 -0400

John,

The updates were included in the version I approved for posting that also
addressed Barry's discuss points, correct?

Are we good with the current version to move forward:
https://datatracker.ietf.org/doc/draft-ietf-oauth-spop/

Thank you,
Kathleen

On Thu, Jul 9, 2015 at 2:46 PM, John Bradley <[email-masked]> wrote:

> I have made some edits to make it consistent.  They are checked into the
> butbucket repo nat and I use, but we can’t update the official draft during
> the freeze before the IETF meeting.
>
>>>> I changed it to t_m rather than “t”  I think that is clearer.  If I
>>>> were to do it the other way XML2RFC would have double quotes in the text
>>>> version.
>>>>
>>>> John B.
>>>>
>>>> I wonder if it makes more sense to represent as `+ t(code_verifier),
>>>> "t"` (note the quotes on the second 't') given that it's a string
>>>> representation of the method that's being sent?
>>>>
>>>>

-- 

Best regards,
Kathleen

---

From: John Bradley <[email-masked]>
Date: Fri, 10 Jul 2015 12:36:18 -0300

Yes I believe I I addressed these comments as part of Barry’s discuss points.  
They were comments on the changes that Barry introduced that caused a inconsistency.   I resolved that in 15.

I think it is good to go.

> On Jul 10, 2015, at 12:29 PM, Kathleen Moriarty <[email-masked]> wrote:
> 
> John,
> 
> The updates were included in the version I approved for posting that also addressed Barry's discuss points, correct?
> 
> On Thu, Jul 9, 2015 at 2:46 PM, John Bradley <[email-masked] <mailto:[email-masked]>> wrote:
> https://bitbucket.org/Nat/oauth-spop <https://bitbucket.org/Nat/oauth-spop>
> 
>> On Jul 9, 2015, at 3:19 PM, Brian Campbell <[email-masked] <mailto:[email-masked]>> wrote:
>>

---

From: Brian Campbell <[email-masked]>
Date: Fri, 10 Jul 2015 10:11:06 -0600

I think -15 does address the inconsistency.

On Fri, Jul 10, 2015 at 9:36 AM, John Bradley <[email-masked]> wrote:

>>> Following up the discussion on today's NAPPS call, I understand why
>>> plain is not presented as the recommended approach in the spec (though it
>>> still has some value over not doing PKCE at all, in that it mitigates
>>> against the current known attack where a rogue app registers the same
>>> custom URI scheme as another), but I feel that after all the back and forth
>>> the picture is a little confusing.
>>>
>>>> Clients SHOULD use the S256 transformation.  The plain transformation
>>>> is for compatibility with existing deployments and for constrained

---

From: Kathleen Moriarty <[email-masked]>
Date: Fri, 10 Jul 2015 12:33:45 -0400

Thanks, Brian!

William? Are you good with this version?

On Fri, Jul 10, 2015 at 12:11 PM, Brian Campbell <[email-masked]

>>>>> If the client is capable of using "S256", it MUST use "S256", as
>>>>> "S256" is Mandatory To Implement (MTI) on the server. Clients are permitted
>>>>> to use "plain" only if they cannot support "S256" for some technical reason
>>>>> and knows that the server supports "plain".
>>>>
>>>>
>>>>> t_m works for me, I just think we should have some indication that
>>>>> it's the name of the transform. Will you also update where it is referenced
>>>>> in the description below Figure 2?
>>>>>
>>>>>
>>>>>

-- 

Best regards,
Kathleen

---

From: William Denniss <[email-masked]>
Date: Fri, 10 Jul 2015 09:57:22 -0700

Looks good to me. I think it's a lot clearer now, thanks for the update
John.

Unrelated, I noticed a typo in "7.5.  TLS security considerations", the
word 'Curent'.

On Fri, Jul 10, 2015 at 9:33 AM, Kathleen Moriarty <
[email-masked]> wrote:

> On Fri, Jul 10, 2015 at 12:11 PM, Brian Campbell <
> [email-masked]> wrote:
>
>>> The updates were included in the version I approved for posting that
>>> also addressed Barry's discuss points, correct?
>>>
>>>> I have made some edits to make it consistent.  They are checked into
>>>> the butbucket repo nat and I use, but we can’t update the official draft
>>>> during the freeze before the IETF meeting.
>>>>
>>>>> Should we be more explicit upfront that "plain" is optional for
>>>>> servers to support, if that's the intention?
>>>>>
>>>>>
>>>>>>> I wonder if it makes more sense to represent as `+
>>>>>>> t(code_verifier), "t"` (note the quotes on the second 't') given
>>>>>>> that it's a string representation of the method that's being sent?
>>>>>>>
>>>>>>>

---

From: Kathleen Moriarty <[email-masked]>
Date: Fri, 10 Jul 2015 12:59:06 -0400

Thank you all for your work on this draft!

On Fri, Jul 10, 2015 at 12:57 PM, William Denniss <[email-masked]>
wrote:

>>>>> I agree with William that it's a little confusing. I get that there's
>>>>> a desire to discourage using "plain" but perhaps the language (especially
>>>>> the MUST NOT in 7.2) should be lightened up just a bit?
>>>>>
>>>>>>> Clients SHOULD use the S256 transformation.  The plain
>>>>>>> transformation is for compatibility with existing deployments and for
>>>>>>> constrained environments that can't use the S256 transformation.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> 4.4.1.
>>>>>> On Tue, Jul 7, 2015 at 10:51 PM, William Denniss <[email-masked]
>>>>>> > wrote:
>>>>>>

-- 

Best regards,
Kathleen