Re: [lisp] Last Call SECDIR Review of draft-ietf-lisp-6834bis-11

[Donald Eastlake <[e-mail]>]

Hi Luigi,

I think -12 is pretty good. The change suggested by Alvaro should be
included. And I think the last sentence in Section A.3 in -12 should
probably be a separate paragraph and the start should be "Note that ..."
rather than "To be noted that ...". With those changes, I am OK with the
draft.

Thanks,
Donald
===============================
 Donald E. Eastlake 3rd   [phone] (cell)
 2386 Panoramic Circle, Apopka, FL 32703 USA
 [e-mail]


On Wed, Jun 1, 2022 at 8:16 AM Luigi Iannone <[e-mail]> wrote:

> Hi Donald,
>
> A new revision of the drafts has been submitted.
> Here is the link to the rfcdiff:
> https://www.ietf.org/rfcdiff?url2=draft-ietf-lisp-6834bis-12.txt
>
> Let  me know if this revision does not address your concerns.
>
> Thanks
>
> Ciao
>
> L.
>
> On 1 Jun 2022, at 10:45, Luigi Iannone <[e-mail]> wrote:
>
>
>
> On 1 Jun 2022, at 05:29, Donald Eastlake <[e-mail]> wrote:
>
> See below at <de>
>
> On Mon, May 30, 2022 at 9:32 AM Luigi Iannone <[e-mail]> wrote:
>
>> Hi Donald,
>>
>> Thank you very much for your review.
>> I take this last updated review and provide some answers inline.
>>
>> On 30 May 2022, at 04:35, Donald Eastlake <[e-mail]> wrote:
>>
>> I have updated my review of the -10 version for -11 below.
>> Comments/suggestions that I do not comment on still apply.
>>
>> On Wed, May 25, 2022 at 3:41 PM Donald Eastlake <[e-mail]> wrote:
>>
>>
>> I have reviewed this document as part of the security directorate's
>> ongoing effort to review all IETF documents being processed by the IESG.
>> Document editors and WG chairs should treat these comments just like any
>> other last call comments. Sorry this is a bit late.
>>
>>
>> The summary of the review is Ready with Issues.
>>
>>
>> Well, minor issues...
>>
>> ...
>>
>> SECURITY
>>
>> This draft appears to completely ignore the issue of Map Version Number
>> advancing so far so quickly that an old version number is encountered that
>> appears to be newer than or equal to the current version number. Why can't
>> this happen? Or if it can, why doesn't that hurt?
>>
>> This is more of an operational point. If you update a mapping, the best
>> would be to give sufficient time so that everybody updates and there is no
>> such a risk.
>> What about adding in section 7 “dealing with Map-Version Numbers” the
>> following sentence.
>>
>> It is an operational question to make sure that Map-Version numbers are
>> not updated so frequently as to create the risk that very old version
>> numbers appear newer (because of the circular space).
>>
>> Would that address your issue?
>>
>
> <de> Not really. (1) I think the document needs to say what happens if the
> numbers wrap around and overlap. (2) Assuming the answer to 1 is as bad as
> I think, then it is not "an operational question" to avoid this but rather
> "an operational requirement".  That is, there should be a statement
> something like "Map Version Number incrementing and TTL MUST be managed so
> that an old Version Numbers will not be confused as a new Version Number.
>
>
> This last sentence is great. I will put it in section 7.
>
>
>
> Section 8, last paragraph: Says Map-Versioning can only be used in
>> trusted, closed environments but Section 7.1 and 7.2 seem to talk about
>> what to do about the Map-Version field without any reference to this, but
>> mentioning private deployments for certain error conditions. For example,
>> Section 7.2 point 3 says to discard a packet on an erroneous Map-Version
>> value except perhaps in some private deployments. But if you MUST NOT use
>> Map-Versioning on the open internet, shouldn't it be required to discard
>> all LISP encapsulated packets with Map-Version numbering if received over
>> the public Internet?
>>
>> Actually section 7.1 reads:
>>
>> Operators can configure exceptions to this
>>        recommendation, which are outside the scope of this document.
>>
>>
>> We should have done the same for 7.2, will do in a revision.
>>
> <de> Well, adding that to Section 7.2 would help. But the problem is the
> following sentence in Section 8:
>
>    Map-Versioning MUST NOT be used over the public Internet and SHOULD
>    only be used in trusted and closed deployments.
>
>
> It seems to me that sentence says you can only use Map-Versioning in a
> private network and that private network SHOULD be trusted. So I guess it
> allows use in an untrusted private network... Is that what you want to say?
>
>
> This is similar to the comment made by Paul.
> His suggestion is to change the SHOULD in a MUST. Would this work for you
> as well?
>
>
>
>
>> Otherwise, the Security Considerations seem adequate although I think the
>> 1st and 2nd paragraphs of Section 8 should be swapped.
>>
>> Yes, it may read better. Will be swapped in the next revision.
>>
>> OTHER ISSUES
>>
>> Section 6, right after equation 3: Isn't "(69 + 4096) mod 4096" the same
>> as "69"? And isn't 69 equal to 69, not less than 69? Shouldn't it say
>> "Map-Version numbers in the range [69 + 2049; 68] are smaller than 69"? Or
>> actually "in the ranges [69 + 2049; 4095] and [1;68] are smaller than 69"?
>>
>> Wonderful catch. Should be
>> [69 +  2049; (69 + 4095) mod 4096]
>>
>>
>> Will fix.
>>
>> Section A.3: How is it possible to tell that no more traffic will be
>> received? Should this instead be something like wait the TTL of the
>> mappings to that RLOC plus estimated transit time and some margin for
>> safety?
>>
>> Absolutely right, the sentence should be:
>>
>>    Upon updating the mapping, the RLOC will receive the less and less
>>
>>    traffic because remote LISP sites will get the updated mapping.
>>
>>    At least one TTL after the mapping was updated, it could be
>>
>>    considered safe to shut down the RLOC gracefully, because all
>>
>>    sites actively using the mapping should have updated
>>
>>    it.
>>
>>
>> Sounds better?
>>
>
> <de> Yes, that's better. But I would suggest alternate wording such as the
> following:
>
>      "Upon updating the mapping, the RLOC will receive less and less
>
>    traffic because remote LISP sites will request the updated
>
>    mapping and see that it is disabled. At least one TTL, plus a
>
>    little time for traffic transit, after the mapping is updated,
>
>    it should be safe to shut down the RLOC gracefully, because
>
>    all sites actively using the mapping should have been updated."
>
>
> Thanks a lot, it reads much better.
>
>
> TYPOS/MINOR
>>
>> Should the document say anything about mapping changes possibly causing
>> re-ordering?
>>
>> Not sure what do you mean by “re-ordering”, can you articulate?
>>
>
> <de> I was thinking about adding one sentence somewhere something like the
> following:
> "A change in map version resulting in a change in ETR for a flow can
> result in the re-ordering of the packet in the flow just as any other
> routing change could cause re-ordering."
>
>
> Yes, that is again absolutely correct. I will add the sentence.
>
>
> Section 1: I think the following should end with "ITR": "If this is not
>> the case, the ETR can directly send a Map-Request containing the updated
>> mapping to the ETR,"
>>
>>
>> Above fixed in -11.
>>
>> Section 7.2, first sentence just after point 3: Suggest using "MAY" in
>> "may be more restrictive."
>>
>>
>> Will be changed in the next revision.
>>
>> Section A.2.3: "provider edge" pops up here with no other mention or
>> explanation anywhere in the draft.
>>
>>
>> Either drop the term or provide some sort of definition/explanation.
>>
>>
>> Provider edge should actually be just “domain"
>>
>
> <de> OK.
>
>> Section A.2.3: The last two sentences sound like they contradict each
>> other. I assume the last sentence is refering to change in the Source
>> mapping. Suggest "the mapping" -> "the Source mapping".
>>
>>
>> Yes, is
>>
>> With this setup, the Proxy-ETR, by looking at the Source Map-Version Number,
>>
>> is able to check whether the mapping has changed.
>>
>>
> <de> OK.
>
>> EDITORIAL
>>
>> Section 1: "This operation is two-fold." -> "This information has two
>> uses."
>>
>> New Section 6: "... MUST consist in an increment ..." -> "... MUST
>> consist of an increment ..."
>>
>> New Section A.2.3: "uRPF" is used only once so the acronym should be
>> dropped and only the expansion used.
>>
>> Will update as suggested in the next revision.
>>
>
> <de> OK.
>
>
> Thanks again for the feedback.
>
> Ciao
>
> L.
>
>
>
>
> <de> Thanks,
> Donald
> ===============================
>  Donald E. Eastlake 3rd   [phone] (cell)
>  2386 Panoramic Circle, Apopka, FL 32703 USA
>  [e-mail]
>
> Thanks again for the review.
>> Let me know if the proposed changes address your comments.
>>
>> Ciao
>>
>> L.
>>
>>
>>
>>
>>
>> Thanks,
>> Donald
>> ===============================
>> Donald E. Eastlake 3rd   [phone] (cell)
>> 2386 Panoramic Circle, Apopka, FL 32703 USA
>> [e-mail]
>>
>>
>