Re: [OAUTH-WG] Barry Leiba's Discuss on draft-ietf-oauth-spop-12: (with DISCUSS and COMMENT)

["Nat Sakimura" <[e-mail]>]

Thanks Barry for the comment.

My (personal) responses inline.
John and Naveen, if you have anything to add, please chime in.

>
> -----Original Message----- 
> From: Barry Leiba
> Sent: Tuesday, June 09, 2015 4:42 AM
> To: The IESG
> Cc: [e-mail] ; [e-mail] ; 
> [e-mail] ; [e-mail] ; 
> [e-mail]
> Subject: [OAUTH-WG] Barry Leiba's Discuss on draft-ietf-oauth-spop-12: 
> (with DISCUSS and COMMENT)
>
> Barry Leiba has entered the following ballot position for
> draft-ietf-oauth-spop-12: Discuss
>
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
>
>
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-oauth-spop/
>
>
>
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
>
> How does "plain" do anything at all to mitigate this attack?  Wouldn't
> anyone who could snag the grant also be able to snag the code verifier as
> well?  Why is "plain" even here?
>

You have to understand that this spec deals with a scenario that the 
communication is conducted over two segments:
(1)    Unprotected: Within the machine through mechanisms like custom 
scheme.
(2)    Protected: Over the network, which is protected by TLS.
The “snagging” happens over the segment (1). For example, an attacker can 
snag the grant over this segment.
However, he cannot snag code verifier because it is sent over (2), which is 
TLS protected.
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> General:
> I would think that this mechanism should never be a substitute for using
> TLS, and that this document should be explicit about that, and should say
> that the proper mitigation for situations where TLS can be used... is to
> use TLS.  Is there a reason we should NOT say that?

OAuth (RFC6749 and RFC6750) mandates the use of TLS over the network.
PKCE inherits this property. We could mention it again here, but it is sort 
of already done by inheritance.

>
> Putting quotation marks around "code_verifier" and "code_challenge" in
> the formulas is confusing: it makes it look as if you're putting in those
> strings themselves, rather than the values of the variables.  It's
> probably unlikely that anyone would make that mistake, but I nevertheless
> suggest removing the quotation marks when you mean to refer to the
> values.

That’s the peculiarity of the current XML2TXT converter.
In XML, it is <spanx style="verb"> to signify strings themselves rather than 
the values of the variables.
It renders nicely on HTML format etc., but TXT seem to have this confusing 
property.
Perhaps RFC Editor can deal with.

>
> -- Section 2 --
> I don't understand the distinction between STRING and ASCII(STRING).  Can
> you please explain it?

It is a notation used by JSON Web Signature, etc.
It is making sure not to conflate the sequence of characters with sequence 
of octets of characters.

>
> -- Section 4.3 --
> If "plain" does stay, why on Earth is it the default?  Even if just for
> form's sake, shouldn't S256 be the default?

It is partly historical. The draft started off there, then kept adding other 
mechanisms. There are many implementations of PKCE now but the largest 
portion of it is using “plain”. For the sake of interoperability with 
them, it needs to stay as it is written.

>
> -- Section 4.4 --
> The word "code" is used for too many things, and "Authorization Grant" is
> already the right name for what we're talking about here.  I suggest that
> in both the section title and body you use that term, to make it clear
> what you mean by the "code".

RFC 6749 defines two types of Authorization Grant: Authorization Code and 
Implicit. In PKCE, we are specifically dealing with Authorization Code so 
replacing them with Authorization Grant loosens it up and is not desirable. 
I agree that we have been a bit lax in the use of the term “code” as an 
abbreviation of “Authorization Code”. Also, looking at the TXT 
representation of the spec, largely due to the formatting peculiarity, it is 
making them even more confusing than compared to other format. Therefore, I 
suggest the following:

- Replace all occurrence of “code” as an abbreviation of “Authorization 
Code” with “Authorization Code”.
- Capitalize the defined term “code challenge” and “code verifier” so 
that there will become “Code Challenge” and “Code Verifier” throughout.
> Similarly, in Section 4.5 please say "code_verifier" rather than "secret".

Good catch. Accept.

>
> -- Section 4.4.1 --
> Please expand "PKCE", which is, at the moment, only expanded in the
> document title.

Suggest the following:
Replace “this extension” of the last paragraph of 1. Introduction by 
“Proof Key for Code Exchange (PKCE) extension”.
Add “3.1 Abbreviation” subsection and collect the abbreviations that are 
used in this specification, namely:
- PKCE – Proof Key for (Authorization) Code Exchange
- Authz -- Authorization
- App – Application
- MITM – Man In The Middle

I guess we do not need to list IESG, IANA, and RFC in it…

>
> -- Section 5 --
> The SHOULD in the first paragraph is wrong.  You already have a MAY
> covering the general behavior.  You should just take out the "SHOULD",
> and just say that severs supporting backwards compatibility revert to the
> normal OAuth protocol.

Accept in principle, but at the same time, I feel that explicitly writing 
that the server is expected to fall back to RFC6749 and RFC6750 mode without 
error probably is useful for developers. For this purpose, this “SHOULD” 
while redundant, could be useful. Perhaps, just replacing “SHOULD” with 
“should” suffice?

>
> -- Section 6.2 --
> I have the same comment here as in the other OAuth document: please shift
> the focus away from telling IANA how to handle tracking of the expert
> review, and make the mailing list something that the designated expert(s)
> keep track of.  Also, please give more instructions to the DEs about what
> they should consider when they're evaluating a request (for example,
> should they approve all requests, or are there criteria they should
> apply?).
>
> For the first, here's a text change that I suggest we move toward for
> this sort of thing:
>
> OLD
> <most of Section 6.2>
>
> NEW
>    Additional code_challenge_method types for use with the authorization
>    endpoint are registered using the Specification Required policy
>    [RFC5226], which includes review of the request by one or more
>    Designated Experts.  The DEs will ensure there is at least a two-week
>    review of the request on the [e-mail] mailing list,
>    and that any discussion on that list converges before they respond to
>    the request.  To allow for the allocation of values prior to
>    publication, the Designated Expert(s) may approve registration once
>    they are satisfied that an acceptable specification will be
> published.
>
>    Discussion on the [e-mail] mailing list should use
>    an appropriate subject, such as "Request for PKCE
>    code_challenge_method: example").
>
>    The Designated Expert(s) should consider the discussion on the
>    mailing list, as well as <<these other things>> when evaluating
>    registration requests.  Denials should include an explanation
>    and, if applicable, suggestions as to how to make the request
>    successful.
> END

Thanks. The editors were just trying to be consistent with the way the 
parent spec was dealing with this kind of things, but if the suggested way 
is preferred, we can certainly do it.

>
> -- Section 7.2 --
> Please rewrite the first paragraph.  Please do not leave it for the RFC
> Editor, as they may inadvertently get it technically wrong when they try.
>
Would something like this work?

OLD
Clients MUST NOT try down grading the algorithm after trying S256 method. If 
the server is PKCE compliant, then S256 method works. If the server does not 
support PKCE, it does not generate error. Only the time that the server 
returns that it does not support S256 is there is a MITM trying the 
algorithm downgrade attack.
NEW
Clients MUST NOT down grade the algorithm after trying the S256 method. 
There is no legitimate case that such a downgrade become necessary since 
S256 method always works if the server supports PKCE and it does not 
generate error if the server does not. Only the time that the server returns 
error that it does not support S256 is that there is a MITM attacker trying 
the algorithm downgrade attack.

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