Re: [Lsr] AD review of draft-ietf-lsr-pce-discovery-security-support-09

[Qin Wu <[e-mail]>]

Hi, John:
-----邮件原件-----
发件人: John Scudder [mailto:jgs=[e-mail]] 
发送时间: 2022年8月27日 5:34
收件人: Qin Wu <[e-mail]>
抄送: [e-mail]; [e-mail]; Dhruv Dhody <[e-mail]>; Acee Lindem (acee) <[e-mail]>; Les Ginsberg (ginsberg) <[e-mail]>
主题: Re: [Lsr] AD review of draft-ietf-lsr-pce-discovery-security-support-09

Hi Qin,

Thanks for your reply, and Dhruv, Acee, and Les for your subsequent remarks. I’m rolling all my comments together into this reply to Qin’s message. Anything that’s settled I’ve snipped out, remaining items discussed in line.

> On Aug 25, 2022, at 5:21 AM, Qin Wu <bill.wu=[e-mail]> wrote:

[snip]

> +jgs: I don't find any place where you explicitly mention that the 
> +sub-TLVs you define are identical for both OSPF and IS-IS. Please do 
> +make this explicit.
>  
> 
> [Qin Wu] We authors add explicit statement in section 3 to say
> 
> “
> 
>  
> 
>    In addition, the sub-TLVs defined in this document (i.e.,Section 
> 3.2,
> 
> Section 3.3) are used identically in both OSPF and IS-IS.
> 
> ”
> 
> Hope this addresses your comment.

Les’s subsequent remarks have convinced me that it will be better to explicitly define the sub-TLVs separately for the two protocols. Assuming you do so, this becomes moot.

>  3.2.  KEY-ID Sub-TLV
>  
> -   The KEY-ID sub-TLV specifies a key that can be used by the PCC to
> +   The KEY-ID sub-TLV specifies an identifier that can be used by the 
> + PCC to
>     identify the TCP-AO key [RFC5925].
>  
>     The KEY-ID sub-TLV MAY be present in the PCED sub-TLV carried 
> within @@ -233,6 +267,14 @@
>  
>        Reserved: MUST be set to zero while sending and ignored on
>        receipt.
> +      
> +jgs: what is the size of the Reserved field?  I'm guessing it's three 
> +octets, to align the sub-TLV to a word boundary, but you need to say.
> +[Qin Wu] Yes, it can be inferred from 4 octets length and 1 octet KEY ID.
> 
> But we can make it explicitly.

Actually, why do you have a Reserved field anyway? If it’s just padding to get 32-bit alignment — in OSPF, doesn’t that come “for free” without need for explicitly defining the field (as we covered in our discussion of the key-chain-name sub-TLV), and in IS-IS there is no need for padding, so why burn the bytes?

If you’re reserving the field on purpose for some contemplated future use, that’s fine of course. But otherwise it seems to me you could remove it. However, see also my further comments below under KEY-CHAIN-NAME.

[Qin Wu] I agree to remove reserve field for ISIS.
> +A diagram of the style used in RFC 5088 would make this more evident 
> +since presumably you'd show the Reserved field in the diagram.  I 
> +don't insist you add diagrams, but they do tend to help the 
> +document's readability so maybe consider it for this and also §3.3.
>  [Qin Wu] This has been discussed on the list before, it is 
> intentional not use diagram
> 
> for IGP since OSPF uses diagram for TLV format while ISIS not.

This also becomes moot assuming you follow Les’s recommendation to have separate definitions for the OSPF and IS-IS sub-TLVs. In that case you would presumably follow the OSPF format for the OSPF definition, and the IS-IS format for the IS-IS definition.
[Qin Wu] Agree, will do.
>  3.3.  KEY-CHAIN-NAME Sub-TLV
>  
> @@ -241,9 +283,9 @@
>  
>     The KEY-CHAIN-NAME sub-TLV MAY be present in the PCED sub-TLV carried
>     within the IS-IS Router Information Capability TLV when the
> -   capability flag bit of PCE-CAP-FLAGS sub-TLV in IS-IS is set to
> +   capability flag bit of the PCE-CAP-FLAGS sub-TLV in IS-IS is set 
> + to
>     indicate TCP Authentication Option (TCP-AO) support.  Similarly, this
> -   sub-TLV MAY be present in the PCED TLV carried within OSPF Router
> +   sub-TLV MAY be present in the PCED TLV carried within the OSPF 
> + Router
>     Information LSA when the capability flag bit of PCE-CAP-FLAGS sub-TLV
>     in OSPF is set to indicate TCP-AO support.
>  
> @@ -257,6 +299,57 @@
>        identify the key chain.  It SHOULD be a string of printable ASCII
>        characters, without a NULL terminator.  The sub-TLV MUST be zero-
>        padded so that the sub-TLV is 4-octet aligned.
> +      
> +jgs: Shouldn't your SHOULD be a MUST? If not, what are the non-ASCII 
> +characters that are allowed, and under what circumstances?
> +
> +For that matter, why are you restricting the key chain name to ASCII?
> +You cite RFC 8177 above. Looking at 8177, it defines a key-chain name 
> +as a YANG string, and RFC 7950 tells us that for a YANG string:
> +
> +   Legal characters are the Unicode and ISO/IEC 10646 [ISO.10646]
> +   characters, including tab, carriage return, and line feed but
> +   excluding the other C0 control characters, the surrogate blocks, and
> +   the noncharacters.
> +   
> +If you really do want to restrict the alphabet in your extension, I 
> +think you need to explain why, or otherwise you need to support the 
> +same alphabet RFC 8177 does.
> 
> 
> [Qin Wu]: Okay.We can align with what RFC8177 does.

N.b. I think Dhruv and Acee’s suggestions are on point.
[Qin Wu] Sure, have confirmed this to them on the list.
> +Next question, what does the Length field indicate, exactly?  Is it 
> +the length of the string?  Presumably so, since there is no 
> +termination character.  Please say so, and say what the units are (e.g., "Length:
> +length of the Key Name field, in octets").
> +
> +Finally, at the end of the description of the Key Name field, you say 
> +"The sub-TLV MUST be zero-padded so that the sub-TLV is 4-octet aligned."
> +Since we've already established that the Length field must indicate 
> +the Key Name field length, what tells the parser how many bytes of 
> +padding are present? Is it just supposed to know, based on whether 
> +the Key Name field falls on a word boundary or not? Because, the 
> +Length field isn't going to tell it.
> +
> +Perhaps all OSPF and IS-IS TLV parsers are robust to this kind of 
> +format; if so let's discuss. But Occam's Razor suggests that the 
> +format documented here isn't quite right, and that it should be something more like:
> +
> +       Type: 7
> +       Length: Variable
> +       Key Name:
> +               String Length: one octet whose value is the number of octets 
> +                                          in the Key Name
> +               String: zero or more characters of (whatever the supported
> +                           alphabet is).
> +       Padding: 0-3 octets of padding, such that the sub-TLV is 4-octet
> +                    aligned.
> +
> +So there are two length fields: one is the usual TLV length field, 
> +that tells you the size of the value portion. The other is the string 
> +length, and it's needed because you're going to add 0-3 bytes of 
> +padding. If you didn't use the padding, you could get away with a 
> +single length field, but since you do have padding, you need both fields, it seems.
> +
> +That obviously isn't a finished description but it gives you the idea. 
>  
> 
> [Qin Wu] We authors believe your questions can be interpret as two sub-questions as follow:
> 
> 1. Does it include the Type and Length, or just the Key Name?
> 
> 2. Does it count the number of bytes of Key Name, or also include the trailing pad bytes?
> 
>  
> 
> Our answer is:
> 
>  
> 
> The Length field indicates the length in octets of the Key Name field excluding any trailing zero pad bytes.
> 
>  
> 
> Since we already have the statement of "The sub-TLV MUST be zero-padded so that the sub-TLV is 4-octet aligned."
> 
> We don't need anything further. Although, we presumably don't want an 
> arbitrary number of trailing zero bytes,
> 
> so we could say...
> 
>  
> 
> The sub-TLV MUST be zero-padded with 0, 1, 2, or 3 zero octets so that the sub-TLV is 4-octet aligned.
> 
> Does this address your comment above.

I think rather than that, let’s see what it looks like when divided into OSPF and IS-IS encodings? In that case the IS-IS encoding is straightforward. The OSPF encoding probably still does need to mention the padding I guess, maybe along with something like "padding is not included in the Length field”. This is following the model of RFC 5088, where it describes the OSPF PCED TLV (Section 4):

   The TLV is padded to 4-octet alignment; padding is not included in
   the Length field (so a 3-octet value would have a length of 3, but
   the total size of the TLV would be 8 octets).  Nested TLVs are also
   4-octet aligned.  Unrecognized types are ignored.

In reviewing RFC 5088, I notice that it never explicitly mentions anything about alignment and padding of the sub-TLVs — they all “just happen” to be defined to end on word boundaries, therefore for the sub-TLVs it’s unnecessary to mention the detail about the length not covering the padding. Now that you are adding one sub-TLV for which length does not cover padding (or two, if you follow my earlier suggestion about dropping the Reserved field of the KEY-ID sub-TLV) I think you do need to state this explicitly.
[Qin Wu] Okay, thank for good suggestion.
> +But in any case, since you aren't changing the name of the registry 
> +to be something other than "Path Computation Element (PCE) Capability 
> +Flags", I don't think you need to provide these details.  People 
> +looking for the registry should still be able to find it.
> +
> 
> [Qin Wu] We authors think it is important that readers of RFC 8231 should be able to find the registry originally defined in that document.
> 
> We also believe it is important for implementers of RFC 8306 and RFC 8623 to know that the code points defined in those documents are now housed in a registry.
> 
> You are correct that no other part of this document forms an update to those other RFCs. Here are a few proposed changes to abstract:
> 
> The Abstract should have:
> 
>  
> 
> This document Updates RFC 8231 to move the IANA "PCE Capability Flags" registry to be housed under the IANA Common IGP Parameters registry.
> 
> 2. This document Updates RFC 8306 and 8623 to create a registry to contain the PCE Sub-TLVs that they define.
> 
> This text needs to also be in the Introduction.
> 
> Let us know if addition text or clarifications are needed.

The text, as modified by Dhruv and Acee’s comments, seems fine. (I still don’t feel it’s necessary but this is just my personal opinion, since the WG consensus was to have it, that’s fine.)
[Qin Wu] Thanks for confirmation.
[snip]

>  7.  Security Considerations
>  
> @@ -317,18 +425,31 @@
>     The information related to PCEP security is sensitive and due care
>     needs to be taken by the operator.  This document defines new
>     capability bits that are susceptible to a downgrade attack by
> -   toggling them.  The content of Key ID or Key Chain Name Sub-TLV can
> -   be tweaked to enable a man-in-the-middle attack.  Thus before
> +   setting them to zero.  The content of Key ID or Key Chain Name Sub-TLV can
> +   be altered to enable a man-in-the-middle attack.  Thus before
>     advertising the PCEP security parameters, using the mechanism
>     described in this document, the IGP MUST be known to provide
>     authentication and integrity for the PCED TLV using the mechanisms
>     defined in [RFC5304], [RFC5310] or [RFC5709].
>  
>     Moreover, as stated in [RFC5088] and [RFC5089], if the IGP does not
> -   provide any encryption mechanisms to protect the secrecy of the PCED
> +   provide any encryption mechanisms to protect the confidentiality 
> + of the PCED
>     TLV, then the operator must ensure that no private data is carried in
>     the TLV, e.g. that key-ids or key-chain names do not reveal sensitive
>     information about the network.
> +   
> +jgs: Is this really an "if"? Are there any standardized ways to 
> +protect confidentiality (probably a better word than "secrecy", by 
> +the way, so I've suggested an edit above) of IGP contents? I'm not 
> +aware of any, and indeed RFC 5088 and RFC 5089 clearly say there 
> +aren't. If there are any feasible solutions, please cite them here. 
> +Otherwise, maybe an edit like
> +
> +   Moreover, as stated in the Security Considerations of [RFC5088] and
> +   [RFC5089], there are no mechanisms defined in OSPF or IS-IS to
> +   protect the confidentiality of the PCED TLV. For this reason, the
> +   operator must ensure that no private data is carried in the TLV, e.g.
> +   that key-ids or key-chain names do not reveal sensitive information
> +   about the network.
>  
>  [Qin Wu] Make sense, thanks for proposed change.

You’re welcome. Is there a reason you made the text in the version 10 you attached, conditional? (“If there are no mechanisms… then the operator must”.) That makes it sound as though there might be such mechanisms. My understanding is that currently there _are_ no such mechanisms and therefore we don’t need the “if… then”. Indeed the “if… then” might tempt the reader into confusion. Instead, if you want to hedge against possible future scenarios where such a generic confidentiality mechanism is added, you could append something like “If in the future, a confidentiality mechanism is introduced to OSPF or IS-IS, this consideration might not apply.”

Or, if there is some existing confidentiality mechanism you were thinking of, please say more?
[Qin Wu] Regarding "if and then" issue, I realize I forget to integrate your suggested text in security section, sorry,. I prefer to stick to your proposed text, no more additional text added.
[snip]

> @@ -518,7 +648,34 @@
>  Appendix A.  No MD5 Capability Support
>  
>     To be compliant with Section 10.2 of RFC5440, this document doesn't
> -   consider adding capability for TCP-MD5.  Therefore by default, a PCEP
> +   consider adding capability for TCP-MD5.  
> +   
> +jgs: I don't get how NOT having a TCP-MD5 capability is a requirement 
> +for compliance with RFC 5440 §10.2. Maybe you're saying that TCP-MD5 
> +is a baseline Mandatory-To-Implement algorithm and therefore it 
> +doesn't require signaling? But what about a possible future scenario 
> +where another specification updates RFC 5440 to remove the TCP-MD5 
> +requirement? At that point, it would be necessary to add a flag to 
> +signal MD5 support. So why not define it now, and start rolling it 
> +out now? Seems like it would make cleanly deprecating MD5 in the 
> +future, easier.
> +
> +If the flag were in the base version of this document, if you saw a 
> +PCE advertise PCE-CAP-FLAGS that include either the AO or TLS flags, 
> +and NOT the MD5 flag, you could infer that MD5 was not supported. If 
> +neither the AO nor the TLS flag, nor the MD5 flag, was advertised, it 
> +might be because the advertising PCE doesn't support this 
> +specification at all, so MD5 might be supported, and I can see using 
> +this appendix to report that detail.
> +
> +Forgive me if the question of MD5 has already been discussed by the 
> +WG and this option considered already, I'd be happy to be pointed at 
> +the relevant mail archives if that's the case.
> +
> +In any case, let's please have a conversation so I can, at minimum, 
> +understand what this section is trying to say.
> +
> 
> [Qin Wu] After discussion among our authors, We authors agree with your concern. But is it necessary to define new flag bit for TCP-MD5, we feel not neededsince TCP MD5 has a lot of security concern.
> 
> We could move this into the Security Considerations section and delete the Appendix.
> 
> In the security section, We could add one paragraph to say as follows:
> 
> “
> 
> As described in Section 10.2 of [RFC5440], an PCEP speaker MUST support TCP MD5 [RFC2385], so no capability advertisement is needed to indicate support. However, as noted in [RFC6952], TCP MD5 has been obsoleted by TCP-AO [RFC5925] because of security concerns. However, TCP-AO is not widely implemented and so it is, therefore, RECOMMENDED (per [RFC8253] which updates [RFC5440]) that PCEP is secured using TLS. In any case, an implementation SHOULD offer at least one of the two security capabilities defined in this document.
> 
> “

That seems fine. I might get back to you later with wording suggestions, but if so they would be relatively minor.
[Qin Wu] Okay.
Finally, there were a few nits that seem to have been missed in the -10 you sent, I’ve added the diff at the bottom of this message. (I’m not sending the .txt or iddiff, since it’s just a couple small things.)
[Qin Wu] Fixed, thanks for careful checking.
Thanks,

—John

Residual nits:

--- draft-ietf-lsr-pce-discovery-security-support-10.txt	2022-08-25 05:23:53.000000000 -0400
+++ draft-ietf-lsr-pce-discovery-security-support-10-jgs-comments.txt	2022-08-26 17:17:58.000000000 -0400
@@ -117,7 +117,7 @@
 1.  Introduction
 
    As described in [RFC5440], PCEP communication privacy is one
-   importance issue, as an attacker that intercepts a Path Computation
+   important issue, as an attacker that intercepts a Path Computation
    Element (PCE) message could obtain sensitive information related to
    computed paths and resources.
 
@@ -128,7 +128,7 @@
    for TCP-AO [RFC5926] offer significantly improved security for
    applications using TCP.  As specified in section 4 of [RFC8253], in
    order for a Path Computation Client (PCC) to establish a connection
-   with a PCE server using TLS or TCP-AO, PCC needs to know whether PCE
+   with a PCE server using TLS or TCP-AO, the PCC needs to know whether 
+ the PCE
    server supports TLS or TCP-AO as a secure transport.
 
    [RFC5088] and [RFC5089] define a method to advertise path computation