Re: [secdir] secdir review of draft-ietf-oauth-device-flow

[Christopher Wood <[e-mail]>]

Hi William,

Please see inline below.

On Wed, Aug 1, 2018 at 5:03 PM William Denniss <[e-mail]> wrote:

>
> On Tue, Jun 12, 2018 at 5:55 PM, Christopher Wood <
> [e-mail]> wrote:
>
>> Hello,
>>
>> I have reviewed this document as part of the security directorate's
>> ongoing effort to review all IETF documents being processed by the
>> IESG.  These comments were written primarily for the benefit of the
>> security area directors.  Document editors and WG chairs should treat
>> these comments just like any other last call comments.
>>
>>   The summary of my review is: Ready with nits.
>>
>> Overall, the document is in fine shape. I have a few general comments
>> (not quite nits,
>> though not quite issues either), listed below:
>>
>> - Section 3.5, fifth paragraph: Requiring clients to poll at a
>> “reasonable” polling interval
>> without a suggestion of what is reasonable seems strange. Could you
>> suggest a value that’s
>> within reason, e.g., every second?
>>
>
> We documented a default of 5s in version 12.
>
>
>> - Section 5.1, first paragraph: It might be useful to point to Section
>> 6.1 wherein User Code
>> generation is discussed. Right now minimum entropy “requirements” are
>> listed without further
>> details regarding viable mechanisms.
>>
>
> The authors are still considering this feedback, along with Benjamin
> Kaduk's DISCUSS.
>
>
>> - Section 5.2, second paragraph: The text claims that an end user would
>> end up “on the
>> authorization page of the wrong service.” Can you provide more details
>> here? What stops
>> the malicious MITM from serving an authorization page that’s
>> indistinguishable from the
>> legitimate service page?
>
>
>
>> - Section 5.3, first paragraph: How specifically does the authorization
>> service prevent
>> devices from lying when providing “information about the device”? Or,
>> alternatively, how
>> does the authorization service learn this information?
>>
>
> These 2 also pending.
>
>
>> - Section 5.4: Would it be useful to suggest that clients SHOULD use a
>> secure (encrypted
>> and authenticated) channel when communicating to the user device?
>>
>
> This section is actually referring to real-world spying, i.e. someone in
> the same room as you who can see the TV. Perhaps we need to make that more
> clear?
>

That might help. It seems to be only a matter of clarity, not correctness.


>
>>
>> The remainder of my comments, listed below, are editorial in nature,
>> aimed towards improving
>> readability of the document.
>>
>> - Section 1, step (E): This is the first time client polling is mentioned
>> without
>> discussion of timeouts or server-generated errors. The draft provides
>> such details later
>> on, so it would be helpful to allude or point to them here.
>>
>
> I believe this is covered in detail in the document, this section is
> intending to just be a high-level overview.
>
>
>> - Section 3.3, second paragraph: Please cite TLS upon use (“… in a secure
>> TLS-protected
>> session.”).
>>
>
> Done, thanks!
>
>
>> - Section 3.3, second paragraph: The text suggests that the server
>> informs the user to
>> “return to their device.” Perhaps this should be prefaced with a MAY, as
>> the client will
>> eventually learn that authorization is complete upon polling.
>>
>
> MAY was added, thanks!
>
>
>> - Section 3.3.1, first paragraph: Should it be required that
>> “verification_uri_complete”
>> is constructed in part from the “verification_uri” and “user_code”? I’m
>> not sure this is
>> necessary, though the example given is constructed this way. If not
>> required, this might be
>> worth noting.
>>
>
> The working group considered this, in fact originally we were not going to
> have verification_uri_complete and it would just be defined as a
> composition of those two values. In the end, the work group decided to make
> it separate. The authorization server can combine them to create the
> complete verification URI, or may use something else.
>
> The text currently states the following which I believe covers this:
>
> "A verification URI that includes the "user_code" (or
> other information with the same function as the "user_code"),
> designed for non-textual transmission."
>
> - Section 3.5, first paragraph: s/token endpoint/authentication server?
>>
>
> This section does actually relate to the token endpoint.
>

Ah, okay. I misunderstood. Thanks!


>
>> - Section 5.1, third paragraph: This text is mostly redundant with the
>> preceding paragraphs.
>> I would remove or merge it with the paragraphs above.
>>
>
> I don't agree that it's redundant. Willing to review text if you have a
> concrete proposal.
>

My only recommendation is to remove the paragraph. I think it’s already
covered by the preceding paragraphs. That said, I don’t think this is
necessary. It’s merely a suggestion.


>
> Please let me know if you’ve further questions, comments, or concerns. I
>> hope this helps.
>
>
It does indeed. Thanks for your changes!

Best,
Chris