Re: [TLS] WGLC for "Guidance for External PSK Usage in TLS"

[Russ Housley <[e-mail]>]

I made a PR with these changes.

Russ


> On Dec 21, 2020, at 9:30 AM, Jonathan Hammell <[e-mail]> wrote:
> 
> Hi Russ,
> 
> On Fri, Dec 18, 2020 at 1:55 PM Russ Housley <[e-mail]> wrote:
>>> 1. Section 4 covers a lot of ground and is difficult to follow.  I
>>> suggest it be split into subsections (e.g. "4.1 PSKs Shared with
>>> Multiple Group Members" and "4.2 Weak Entropy PSKs") and move the
>>> attack description to an appendix.
>> 
>> I just reread it, and I think that the flow is okay.  It is about 2 pages.  That said, I can see it dividing into two clean subsections.  I'd prefer to leave the attack in the body of the first subsection.
> 
> Okay.  Just an editorial suggestion.
> 
>>> 2. In Section 4, para 2, the reference "As discussed in Section 6"
>>> refers to the use case of provisioning where multiple clients or
>>> servers share the same PSK, but Section 6 covers all use cases (and
>>> also provisioning). I suggest to create a couple more subsections for
>>> clarity and accurate referencing: "6.1 Use Cases for Pair-wise
>>> External PSKs" and "6.2 Use Cases for PSKs Shared with Multiple
>>> Entities", shifting the provisioning sections to 6.3 and 6.4.  Then
>>> the Section 4 citation can refer to Section 6.2.
>> 
>> I think your only concern is the granularity of the forward reference. I'm not sure most people would be confused.
> 
> Yes, it is the granularity of the reference.  If you think it is okay,
> then leave it as is.
> 
>>> 3. Section 7, item 4 is missing a word.  s/This protects an attacker
>>> from/This protects against an attacker from/
>> 
>> Against the original text: s/protects/prevents/
> 
> That is better than my suggestion.  Thanks.
> 
>>> 4. Since there is no mitigation against revealing PSK identity, it is
>>> more accurate to rename Section 5 "Privacy Concerns".
>> 
>> How about "Privacy Considerations"?
> 
> Even better.
> 
>>> 5. Section 4, para starting with "Finally, in addition to these...":
>>> s/may negatively affects deployments/may negatively affect
>>> deployments/
>> 
>> Agreed.
>> 
>>> 6. Section 5, para 1: I don't think "oppress" is the right word to use
>>> here.  Perhaps, "suppress" would be better.
>> 
>> Agreed.
>> 
>>> 7. In Section 6, the last paragraph refers to Section 7 as the final
>>> sentence.  I assume this reference is for the recommendation to not
>>> share a PSK between more than one node, but it is not clear.  The
>>> previous sentence says do not share PSKs "even if other accommodations
>>> are made", but this conflicts with item 2 of Section 7 which says do
>>> not share PSKs "unless other accommodations are made".
>> 
>> How about:
>> 
>>   The exact provisioning process depends on the system requirements and
>>   threat model.  Whenever possible, avoid sharing a PSK between nodes;
>>   however, sharing a PSK among several node is sometimes unavoidable.
>>   When PSK sharing happens, other accommodations as needed as
>>   discussed in Section 7.
> 
> That is better, except that I think you meant "are needed as
> discussed" near the end.  But I wonder if that should be written in
> requirements language, such as "other accommodations SHOULD be used as
> discussed in Section 7."  A SHOULD is maybe not as strong as "are
> needed", but I'm not sure if a MUST will be acceptable here.
> 
>>> 8. It is not clear why "client certificate authentication after
>>> PSK-based connection establishment", mentioned at the end of Section
>>> 6, is not a sufficient accommodation.  Should it be added to Section
>>> 7, item 2?
>> 
>> Agreed,
>> 
>>> 9. Section 7.1.2, first para, s/clash/collide
>> 
>> That is better.
>> 
>>> 10. Section 7.1.2 describes a possible concern regarding PSK identity
>>> collisions, but it does not provide a recommendation/mitigation for
>>> vendors or users.  What should the reader do with this information?
>> 
>> It means the application, not the stack need to handle the collision by following the advice in the document.
>> 
>>> 11. Section 6, item 2: the term "logical nodes" is not defined.
>> 
>> I suggest dropping "logical"
>> 
> 
> Thanks,
> Jonathan