Re: [Idr] Bug in draft-ietf-idr-rfc5575bis, worth fixing?

[[e-mail]]

Hi John,

Please see inline [Bruno2]

From: John Scudder [mailto:[e-mail]]
Sent: Monday, September 28, 2020 8:16 PM
To: DECRAENE Bruno TGI/OLN <[e-mail]>
Cc: idr@ietf. org <[e-mail]>; [e-mail]; Hares Susan <[e-mail]>
Subject: Re: Bug in draft-ietf-idr-rfc5575bis, worth fixing?

Hi Bruno,

Thanks for your further comments. My responses in line.


On Sep 28, 2020, at 9:45 AM, [e-mail]<mailto:[e-mail]> wrote:



Hi John,

Thanks for your answer.
Please see two replies inline [Bruno]

From: John Scudder [mailto:[e-mail]]
Sent: Friday, September 25, 2020 9:04 PM
To: DECRAENE Bruno TGI/OLN <[e-mail]<mailto:[e-mail]>>
Cc: idr@ietf. org <[e-mail]<mailto:[e-mail]>>; [e-mail]<mailto:[e-mail]>; Hares Susan <[e-mail]<mailto:[e-mail]>>
Subject: Re: Bug in draft-ietf-idr-rfc5575bis, worth fixing?

Hi Bruno,

Thanks for your comments. Some replies in line.



On Sep 25, 2020, at 12:56 PM, [e-mail]<mailto:[e-mail]> wrote:

...


Just as a side note, “error handling according to Section 10” points us to RFCs 7606 and 4760, which end up telling us to reset the session if the NLRI is malformed.

[Bruno] Thanks for providing the full picture and hence the importance of the point.
_IMHO_, RFC 7606 calls for session reset as a ‘last’  resort error handling behaviour.
8.  Guidance for Authors of BGP Specifications
[…]
The "treat-as-withdraw" approach is generally preferred and the "session reset" approach is discouraged.
https://tools.ietf.org/html/rfc7606#section-8<https://urldefense.com/v3/__https:/tools.ietf.org/html/rfc7606*section-8__;Iw!!NEt6yMaO-gk!VbvkPXEkoG_mG12V_w5W4g3uJjg0nhKD6gBw8TalRXnfznNSdAmgpOnjLHVOBw$>

Treat-as-withdraw cannot be performed when the NLRI can’t be found in the update, or the NLRI can’t be effectively withdrawn typically because the peer won’t accept it in the withdraw (e.g. and IPv4 address of 5 octets). Or (my reading) we feel that the received NLRI seem so wrong that what we read may not be what was intended to be send. IOW, there is an error in the syntax.

Exactly right.



IHMO, RFC 7606 specifies “session reset” when the MP_REACH_NLRI attribute is malformed or the NLRI is syntactically incorrect.

I’m not familiar with FS, but so far, I don’t feel that an unknown component type in FS:
- prevents the “treat-as-withdraw” approach.
- matches the RFC 7606 description of a syntactically incorrect NLRI field https://tools.ietf.org/html/rfc7606#section-5.3<https://urldefense.com/v3/__https:/tools.ietf.org/html/rfc7606*section-5.3__;Iw!!NEt6yMaO-gk!VbvkPXEkoG_mG12V_w5W4g3uJjg0nhKD6gBw8TalRXnfznNSdAmgpOmjo43yJQ$>

My reading was that since the FS document carefully says “is considered malformed” and then references Section 10, which in turn references 7606 and 4760, and since 7606 and 4760 both say “reset the session if it’s malformed” that a session reset was called for.
[Bruno]
I agree that this is a valid reading.
However my reading was that draft-ietf-idr-rfc5575bis was saying that the NLRI was semantically malformed. Then that 7606 was calling for session reset only for the malformed MP_REACH attribute or syntactically incorrect NRLI, while calling for treat as withdraw when the NLRI could be recovered and hence withdrawn.

I’m fine to be declared in the rough, but so far (assuming I’m not wrong on the below second point), I do not support the proposed change.

I’m unclear on this point: the only change proposed so far (other than the one you’ll come to later in this message) is to add the clause "including an NLRI that contains an unknown component type” to Section 4.2. I don’t think that has any effect on the discussion we are now having about choice of error handling strategy, treat-as-withdraw vs. session reset, so your comment that you do not support it seems like a bit of a non sequitur.
[Bruno2] You have a point.
On my side, I’m seeing another change of perspective, which is indeed not a change in the text. I had assumed that the error handling were treat-as-withdraw. You are stating that the error handling is session reset. This is a change in perspective which, from my view, is not in favor of adding the statement that un unknown type is to be translated as a syntax error in the NLRI.
We seem to have two points which are not crystal clear from the text:

-          Whether an unknown component type is to be considered as a syntax error or semantic error.

-          Whether the above error handling is to be as per treat as withdraw or session reset.

If we open the discussion on one point, I think that we should open the discussion on the second (related) one.
Another path, is that what is done, is done. The document followed the whole process and in theory could already be published as a final RFC.
As a third path, I can also live with your proposed change.

Coming back to your point, i.e. the proposed text change, I could propose
OLD (draft):
A NLRI value not encoded as specified specified here is considered
   malformed and error handling according to Section 10<https://tools.ietf.org/html/draft-ietf-idr-rfc5575bis-26#section-10> is performed.

NEW:
A NLRI value not encoded as specified here is considered syntactically
   malformed and error handling according to Section 10<https://tools.ietf.org/html/draft-ietf-idr-rfc5575bis-26#section-10> is performed.

An NLRI that contains an unknown component type, is considered semantically

   malformed and error handling according to Section 10<https://tools.ietf.org/html/draft-ietf-idr-rfc5575bis-26#section-10> is performed.


I also note that this action may be seen as a local decision not impacting interop. Hence if we chose to modify the text, we could allow for both, allowing both types of implementation to be compliant with the RFC.

Also reviewing https://github.com/stoffi92/rfc5575bis/issues/20 I think that the outcome of the discussion was that the NRLI should not be propagated but “throw[n] away”.
I also see a specific concluding comment from stoffi92 (Christoph Loibl, the editor of the document) specifically referring to treat-as-withdraw  “unknown flow components / NLRI that are not understood completely lead to treat-as-withdraw (and thus are not propagated) -> error handling.”



I think that is the right thing, too: FS uses T,V and not T,L,V for its component types, the lengths are implicit. So, if my parser encounters an unknown type code it is impossible for me to know how to skip over that type code. At that point, parsing breaks down.
[Bruno] I had in mind the higher level of hierarchy:

   The NLRI field of the MP_REACH_NLRI and MP_UNREACH_NLRI is encoded as
   one or more 2-tuples of the form <length, NLRI value>.  It consists
   of a 1- or 2-octet length field followed by a variable-length NLRI
   value.  The length is expressed in octets.

                     +-------------------------------+
                     |    length (0xnn or 0xfnnn)    |
                     +-------------------------------+
                     |    NLRI value   (variable)    |
                     +-------------------------------+

                Figure 1: Flow Specification NLRI for IPv4

At this level, assuming that the NLRI value is found semantically incorrect, it seems to me that one could:
-          Skip this NLRI (thanks to the ‘length’ field) and continue with the next NLRI
-          Read the ‘NLRI value’ as an opaque data, and treat this NLRI as withdraw. (In the context of the discussion, this NLRI would never had been accepted, so ‘treat-as-withdraw’  could even be replaced with ‘ignore’. But I’m _not_ calling for this).
Hence it seems to me that treat-as-withdraw is technically possible.

Fair enough. It’s a little unfortunate that the draft contains this ambiguity; in retrospect it would have been better to be explicit about the error-handling strategy chosen rather than simply referencing RFC 7606.
[Bruno2] I agree.
 Whether or not we want to respin the draft in order to clarify it, is a question for the WG. If we were to make a change, it could potentially be the addition of this sentence, in Section 10:


   Error handling according to [RFC7606<https://tools.ietf.org/html/rfc7606>] and [RFC4760<https://tools.ietf.org/html/rfc4760>] applies to this

   specification. Notably, an NLRI that is found to be semantically

   incorrect (for example due to an unknown component type) MUST be

   handled using the “treat-as-withdraw” strategy (which in this case,

   it must be noted, works out to be the same as skipping over the NLRI).

[Bruno2] Would work for me, obviously. Thanks for the suggestion. I can also live with a SHOULD to avoid making some implementation non-compliant, which seem a bit unfair as one could have understood “session reset” in good faith. At this point in the process, I could also live with leaving both options equally open to implementations. But indeed to be frank, making “session reset” as a MUST is a bridge too far for me, although I could also live with this (as with all lived before RFC 7606)
But from other emails, it seems clear that some people read the draft with “session reset” in mind. So I’m merely stating my own point of view. I’m not stating that this is the truth to be followed. So up to the WG/chairs. Good luck.

--Bruno

With either ‘session reset’ or ‘treat as withdraw”, this NLRI is equally dead/withdrawn. The question is about punishing the other NLRIs from this AFI/SAFI and even all other AFI/SAFI sharing this BGP session.

I also note that RFC 7606 is opening another error handling strategy in between (called AFI/SAFI disable)

Just as a footnote, that strategy actually comes from RFC 4760, RFC 7606 merely cites it (and gives it a name).


—John



but this seem out of scope of the discussion as “session reset” does not appear in draft-ietf-idr-rfc5575bis. Hence this would be about RFC 7606 interpretation. (my interpretation is that 7606 allows for both, with no preference given)



Thank you.

--Bruno


Also, I don’t see capability negotiation so how is the sender supposed to know the list of component types which is supported by the receiver?

It’s not, this is a deficiency in FS that I think the WG needs to address in the proposed FSv2 work.

Regards,

—John

_________________________________________________________________________________________________________________________



Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc

pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler

a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,

Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.



This message and its attachments may contain confidential or privileged information that may be protected by law;

they should not be distributed, used or copied without authorisation.

If you have received this email in error, please notify the sender and delete this message and its attachments.

As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.

Thank you.


_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.