Re: [Lsr] [yang-doctors] Yangdoctors last call review of draft-ietf-isis-yang-is-is-cfg-29

["Acee Lindem (acee)" <[e-mail]>]

Hi Andy, 

On 1/10/19, 7:38 AM, "Andy Bierman" <[e-mail]> wrote:

    On Thu, Jan 10, 2019 at 12:34 AM <[e-mail]> wrote:
    
    > Hi Andy,
    >
    >
    >
    > What I’m still not catching is the difference you make between having a
    > description statement telling :” A server MUST accept a string up to 64
    > characters in length” and a type string with length “0..64” ?
    >
    > There is probably something that I’m missing here.
    >
    >
    >
    
    A server MAY accept a string longer than 64 characters.
    A range 0..64 means a server MUST NOT accept a string longer than 64
    Characters

I don't think we should set the string range unless the range in specified in the protocol RFCs. In some cases, it may be beneficial to provide guidance in the description. I believe this is similar to Juergen's position. 

Thanks,
Acee
    
    
    
    > Brgds,
    >
    >
    >
    
    
    Andy
    
    
    > *From:* Andy Bierman [mailto:[e-mail]]
    > *Sent:* Wednesday, January 09, 2019 18:06
    > *To:* Juergen Schoenwaelder; LITKOWSKI Stephane OBS/OINIS; tom petch;
    > Ebben Aries; [e-mail];
    > [e-mail]; [e-mail]
    > *Subject:* Re: [yang-doctors] [Lsr] Yangdoctors last call review of
    > draft-ietf-isis-yang-is-is-cfg-29
    >
    >
    >
    > Hi,
    >
    >
    >
    > I agree with Juergen.
    >
    > The protocol has a "too-big" error is the server will not accept a big
    > string.
    >
    > There should not be a false choice between an arbitrary maximum and
    > "terabytes".
    >
    >
    >
    > You cannot use a range-stmt to specify the minimum required length that
    > must be supported.
    >
    > length "3..max" does not allow strings of length 0 - 2.
    >
    >
    >
    > The description-stmt can have "A server MUST accept a string up to 64
    > characters in length"
    >
    > which lets the client choose a length from 0 to 64, and this will work on
    > all server implementations.
    >
    >
    >
    > Andy
    >
    >
    >
    >
    >
    > On Wed, Jan 9, 2019 at 5:10 AM Juergen Schoenwaelder <
    > [e-mail]> wrote:
    >
    > Hi,
    >
    > please see my other email about the distinction I make between a hard
    > length restriction and the minimum length expected to be supported.  I
    > wonder how you can sensibly pick a limit for things like non-best-reason:
    >
    >         leaf non-best-reason {
    >           type string;
    >           description
    >             "Information field to describe why the alternate
    >              is not best.";
    >         }
    >
    > You are simply creating an arbitrary restriction. And humble server is
    > not likely to send you an jpg image (and a bogus server will do so
    > anyway). (There are other similar objects.)
    >
    > Since I am searching for 'type string', I wonder whether these are
    > clear enough definitions.
    >
    >         leaf prefix {
    >           type string;
    >           description
    >             "Protected prefix.";
    >         }
    >         leaf alternate {
    >           type string;
    >           description
    >             "Alternate nexthop for the prefix.";
    >         }
    >
    > What is the (canonical) format of the allowed values? (There are more of
    > these.)
    >
    > /js
    >
    > On Wed, Jan 09, 2019 at 12:52:07PM +0000, [e-mail]
    > wrote:
    > > Hi Tom,
    > >
    > > If you agree, I will set a length restriction on each string (ops and
    > cfg). It looks clearer for me rather than setting it in the description.
    > >
    > > For the references, I'm working on it.
    > >
    > > Brgds,
    > >
    > >
    > > -----Original Message-----
    > > From: tom petch [mailto:[e-mail]]
    > > Sent: Wednesday, January 09, 2019 12:38
    > > To: LITKOWSKI Stephane OBS/OINIS; Ebben Aries; [e-mail]
    > > Cc: [e-mail]; [e-mail]
    > > Subject: Re: [Lsr] Yangdoctors last call review of
    > draft-ietf-isis-yang-is-is-cfg-29
    > >
    > > Stephane
    > >
    > > Thanks for persisting.
    > >
    > > On string lengths, I take your point about no user input to Operational
    > > State; there, my concern is more about giving guidance to implementors
    > > as to what they should cater for - as I said, this has been a topic of
    > > lively discussion in other WG.  Even before that, whenever I see a
    > > string, I think is there an implicit length restriction and if not,
    > > should there be an explicit one which, as Juergen suggested, could be in
    > > the description clause.  My experience is that those working with
    > > networks think about size, about length; those coming from applications
    > > tend to think 'What is a few terabytes between friends?' and are unaware
    > > that sizes that may be commonplace in servers and associated storage can
    > > create difficulties over a network.  Whatever, I leave this one up to
    > > you.
    > >
    > > On references, I would like a change; you say this information is in the
    > > base ISO spec.  Well, yes, to me that means that it should be a
    > > Normative Reference.  I could not understand the description of e.g.
    > > 'i/e' and needed to look it up but seemingly cannot do so with the
    > > listed references of the I-D.  Note that RFC such as RFC5305 and RFC6119
    > > do reference International Standard 10589 and I think that this one
    > > should too, perhaps in s.2.7 and s.5.
    > >
    > > Tom Petch
    > >
    > > ----- Original Message -----
    > > From: <[e-mail]>
    > > Sent: Wednesday, January 09, 2019 9:18 AM
    > >
    > > Hi Tom,
    > >
    > > Please find inline answers.
    > >
    > >
    > > -----Original Message-----
    > > From: tom petch [mailto:[e-mail]]
    > > Sent: Tuesday, January 08, 2019 18:45
    > >
    > > Ok.  Top-posting the ones that are not 'ok':
    > >
    > > I said that I thought that LSP did not need expanding on first use and
    > > then checked the RFC Editor list to find that it is not one they regard
    > > as well-known and that LSR protocols use it differently to others, so I
    > > suggest expanding LSP on first use.
    > >
    > > [SLI] Already done for the next version.
    > >
    > > On lengths of text messages, perhaps I am too sensitive to buffer
    > > overrun attacks and the like and so want a maximum on many things (and
    > > then people attach a friendly, 20Mbyte photo to their e-mail at
    > > Christmas and
    > > wonder why their ESP rejects the message so I do not congratulate them
    > > on the latest addition to the family:-).  The IDR WG had a lively
    > > discussion about maximum message lengths in 2017 and that has also
    > > stayed in my mind.  I have seen the comments on this by Juergen and
    > > Lada; perhaps as Juergen intimates, something in the Description would
    > > help; and while the server may not be rogue, it may not have a perfect
    > > implementation.
    > >
    > > [SLI] What I need to understand from your comment on string length
    > > enforcement is if it applies to operational state or just config states
    > > ? I do not see any issue of not enforcing the operational state as there
    > > is no input from the user there and so no attack vector, this is purely
    > > internal to the implementation. For config statements, it makes sense as
    > > there is an input from the user that can be anything.
    > >
    > >
    > > On the length of password, I saw a Security AD wanting clarification on
    > > this not too long ago so you may see this comment again from one such .
    > > Likewise, MD5 tends to be a red flag although I see it appears in bgp
    > > yang.
    > >
    > > I like the sort of detail in ippm-twamp-yang, on algorithms, entropy and
    > > such like (although I have not seen a review by Security AD/directorate
    > > of that).
    > >
    > > But I am left confused as to exactly what the cited object is doing.
    > > Yes, TLV10 provides authentication for any PDU, but what are the fields
    > > in the YANG module doing?  Is
    > >        leaf authentication-type {
    > > the first octet of TLV10?  Is
    > >       leaf authentication-key {
    > > the rest of TLV10?  And where is this 'presented' as the YANG module
    > > says?  Are you thinking of a YANG client presenting the field to a user
    > > at a terminal, one router presenting it to another, or what?
    > >
    > > I am using RFC5310 as my source for TLV10 and wondering why that is not
    > > a Normative Reference for this I-D
    > >
    > > [SLI] TLV 10 is defined in the base ISO spec of IS-IS. RFC5310 just adds
    > > the crypto auth as new types.
    > > The authentication-type is the first byte of the TLV (called
    > > Authentication Type as well).
    > > The authentication-key cannot be mapped directly to the
    > > authentication-value field. This is the case for ClearText password
    > > authtype but not crypto which adds a keyID in front of the
    > > authentication data.
    > >
    > >
    > > On the I/E bit, the question is, which standard?  I have 30 Normative
    > > references to choose from.   I found up/down in RFC5305, but only by
    > > accident, and I have not found i/e yet so a reference would be good.
    > >
    > > [SLI] I/E bit is part of the base ISO spec of IS-IS and relevant for
    > > "legacy advertisements" of prefix and links. By the way, after checking,
    > > the position of the i-e leaf is currently wrong and needs to be within
    > > the metric (default, delay...). I will fix this.
    > >
    > >
    > > Tom Petch
    > >
    > > ----- Original Message -----
    > > From: [e-mail]
    > > Sent: Monday, January 07, 2019 9:22 AM
    > >
    > > Hi Tom,
    > >
    > > Thanks for your comments.
    > > I wish you an happy new year !
    > >
    > > Please find inline comments.
    > >
    > > Brgds,
    > >
    > > -----Original Message-----
    > > From: tom petch [mailto:[e-mail]]
    > > Sent: Wednesday, January 02, 2019 12:17
    > >
    > > Here are the rest of my comments on -29 with a slight tweak to the
    > > subject line.  I would regard most of these (but not the first two) as
    > > non-discussable, ie I won't complain if you disagree:-)
    > >
    > > RFC1195 is in the YANG module but not the references of the I-D
    > > [SLI] Will fix it
    > >
    > > RFC5029 is in the YANG module but not the references of the I-D
    > > [SLI] Will fix it
    > >
    > > PSNPs, CSNPs
    > > [SLI] Will fix it
    > >
    > > expand on first use - LSP I think ok
    > >
    > >         leaf best {           type boolean;
    > > what is true and what false?  I can guess from the English semantics of
    > > the name but would rather not guess.
    > > [SLI] Will fix it
    > > To replace the current description which is : ""Indicates if the
    > > alternate is the preferred."", do you prefer: "Set to true when the
    > > alternate is preferred, set to false otherwise" ?
    > >
    > >
    > >         leaf non-best-reason {          type string;
    > > suggest a maximum length, perhaps 127 or 255 ( unless you expect
    > > screenshots or packet traces to be attached).  As it stands, you could
    > > validly receive
    > > a length of 18446744073709551615.
    > > [SLI] Agree, will fix it
    > >
    > > You have a mixture of
    > > System-id system-id System id System ID System Id system id system ID
    > > suggest consistency; system-id wfm
    > > [SLI] Will fix it
    > >
    > > You have a mixture of
    > > lsp-id LSPID LSP ID
    > > here, perhaps lsp-id for the names and LSP ID in the text
    > > [SLI] Will fix it
    > >
    > >       case password {        leaf key {           type string;
    > > perhaps better with a minimum length
    > > [SLI] I agree that it could make sense but is it really something that
    > > we should impose ?
    > >
    > >
    > >         leaf i-e {          type boolean;
    > > what is true and what false?  here I am reluctant even to guess
    > > [SLI] This is coming from the standard, is it really worth repeating it
    > > ? Same for up/down bit.
    > >
    > >
    > > /"Authentication keyto/  "Authentication key to/
    > >
    > > "     the authentication key MUST NOT be presented in"
    > > RFC2119 language means that RFC2119 boilerplate should be in the YANG
    > > module (but without the [..] ie the reference must be plain text not an
    > > anchor).
    > >
    > > [SLI] You are right, it is missing.
    > >
    > >
    > >  It is recommended to use an MD5
    > >            hash to present the authentication-key.";
    > > Mmm I think that this may be a red flag to security AD or directorate as
    > > being too vague as well as MD5 too weak; and I think this should be
    > > explicitly called out in Security Considerations.
    > >
    > > [SLI] I agree that there is a point to discuss here. The fact is that we
    > > must not retrieve passwords in clear text. Maybe it is something with a
    > > wider scope than IS-IS. How do the other models deal with passwords
    > > retrieved through "get" or "get-config" ?
    > >
    > >
    > >       list level-db {        key level;        leaf level {
    > > A common convention is for a list of leaf thing to be named things i.e.
    > >       list levels {         key level;        leaf level {
    > >
    > > [SLI] ack
    > >
    > >   rpc clear-adjacency {
    > >           "Name of the IS-IS protocol instance whose IS-IS
    > >            information is being queried.
    > > queried or cleared?
    > > [SLI] "cleared"
    > >
    > > Tom Petch
    > >
    > >
    > > ----- Original Message -----
    > > From: "tom petch" <[e-mail]>
    > > Sent: Monday, December 31, 2018 6:21 PM
    > >
    > >
    > > > Stephane
    > > >
    > > > A new and different comment.
    > > >
    > > >   grouping neighbor-gmpls-extensions {
    > > >
    > > > has a text reference to RFC5307 which does not appear in the
    > > references
    > > > for the I-D.  However, before adding it, I would like to know why it
    > > is
    > > > a good reference for switching capabilities (which is part of this
    > > > grouping).  I think that the reference for switching capabilities
    > > should
    > > > be RFC7074 (which this I-D does not currently reference and should
    > > IMO).
    > > >
    > > > And that begs the  question, why is switching-capability an
    > > unrestricted
    > > > uint8 when only 12 values are valid and three are deprecated?
    > > >
    > > > So why not use
    > > >
    > > > draft-ietf-teas-yang-te-types?
    > > >
    > > > I have a number of additional comments on cfg-29 but this is the one
    > > > that may take some discussion.
    > > >
    > > > Tom Petch
    > > >
    > > >
    > > > ----- Original Message -----
    > > > From: <[e-mail]>
    > > >
    > > > Hi Tom,
    > > >
    > > > Thanks for your comments. I will fix them asap.
    > > > Regarding:
    > > > " Line length is within the RFC limit but the effect is to spread many
    > > > of the description clauses over multiple lines with indentation of 56
    > > > characters, not user friendly e.g.
    > > >                                         description
    > > >                                                 "List of max LSP
    > > > bandwidths for different
    > > >                                                  priorities.";
    > > > "
    > > > What's your suggestion on this one ?
    > > >
    > > > Brgds,
    > > >
    > > > -----Original Message-----
    > > > From: tom petch [mailto:[e-mail]]
    > > > Sent: Tuesday, November 27, 2018 12:11
    > > > Subject: Re: [Lsr] Yangdoctors last call review of
    > > > draft-ietf-isis-yang-isis-cfg-24
    > > >
    > > > Some quirks in-25
    > > >
    > > > I see lots of YANG reference statements - good - but no mention of
    > > them
    > > > in the I-D references - not so good.  My list is
    > > >
    > > > 5130
    > > > 5305
    > > > 5306
    > > > 5880
    > > > 5881
    > > > 6119
    > > > 6232
    > > > 7794
    > > > 7810
    > > > 7917
    > > > 8405
    > > >
    > > > Also perhaps
    > > > OLD
    > > >     reference "RFC XXXX - YANG Data Model for Bidirectional
    > > >                Forwarding Detection (BFD).Please replace YYYY with
    > > >                                                          published RFC
    > > > number for draft-ietf-bfd-yang.";
    > > >
    > > > NEW
    > > >     reference "RFC YYYY - YANG Data Model for Bidirectional
    > > >                Forwarding Detection (BFD).
    > > >
    > > > -- Note to RFC Editor Please replace YYYY with published RFC
    > > > number for draft-ietf-bfd-yang.";
    > > >
    > > > OLD
    > > >       reference "draft-ietf-bfd-yang-xx.txt:
    > > >                  YANG Data Model for Bidirectional Forwarding
    > > >                  Detection (BFD)";
    > > > NEW
    > > >     reference "RFC YYYY - YANG Data Model for Bidirectional
    > > >                Forwarding Detection (BFD).
    > > >
    > > > -- Note to RFC Editor Please replace YYYY with published RFC
    > > > number for draft-ietf-bfd-yang.";
    > > >
    > > >
    > > > Line length is within the RFC limit but the effect is to spread many
    > > of
    > > > the description clauses over multiple lines with indentation of 56
    > > > characters, not user friendly
    > > > e.g.
    > > >                                         description
    > > >                                                 "List of max LSP
    > > > bandwidths for different
    > > >                                                  priorities.";
    > > >
    > > >
    > > > Acknowledgements is TBD. I note that the editor list of the YANG
    > > module
    > > > is somewhat longer than the editor list of the I-D.
    > > >
    > > > I note that the augmentation of interfaces seems to have no
    > > conditional
    > > > and so will augment all interfaces. I think that this is a generic
    > > issue
    > > > but do not see it being addressed anywhere.
    > > >
    > > > In a similar vein, you are defining MPLS objects and I am unclear
    > > > whether or not those should be conditional, or part of the MPLS YANG
    > > > modules or both (copying Tarek for this)
    > > >
    > > > Tom Petch
    > > >
    > > > ----- Original Message -----
    > > > From: "Ebben Aries" <[e-mail]>
    > > > Sent: Tuesday, October 23, 2018 12:45 AM
    > > >
    > > > > Reviewer: Ebben Aries
    > > > > Review result: On the Right Track
    > > > >
    > > > > 1 module in this draft:
    > > > > - [e-mail]
    > > > >
    > > > > No YANG compiler errors or warnings (from pyang 1.7.5 and yanglint
    > > > 0.16.54)
    > > > >
    > > > > "ietf-isis@2018-08-09" module is compatible with the NMDA
    > > > architecture.
    > > > >
    > > > > Module [e-mail]:
    > > > > - Both the description and the draft name reference that this module
    > > > is
    > > > >   specific to configuration but contains operational state nodes in
    > > > addition
    > > > >   to RPCs and notifications.  Any wording suggesting this is only
    > > > >   configuration should be changed
    > > > > - Module description must contain most recent copyright notice per
    > > > >
    > > >
    > > https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.1
    > > > > - Module description reads "common across all of the vendor
    > > > implementations".
    > > > >   I don't think this needs to be called out as such as that is the
    > > > overall
    > > > >   intention of *all* IETF models
    > > > > - This module contains '22' features (and the respective OSPF module
    > > > currently
    > > > >   contains '26').  While it is understood the purpose of these
    > > > features in the
    > > > >   module, take precaution as to complexity for clients if they need
    > > to
    > > > >   understand >= quantity of features per module in use on a
    > > > >   network-element.  We are going to end up w/ feature explosion to
    > > > convey
    > > > >   *all* possible features of each network-element leading to
    > > > divergence back
    > > > >   towards native models at the end of the day.  A large amount of
    > > > these
    > > > >   feature names could be defined within a more global namespace
    > > (e.g.
    > > > nsr) but
    > > > >   this gives us a granular yet cumbersome approach (e.g. feature
    > > > isis:nsr,
    > > > >   ospf:nsr, etc..)
    > > > > - RPC 'clear-adjacency' does not have any input leaf that covers
    > > > clearing a
    > > > >   specific neighbor/adjacency (See comments below as well regarding
    > > > RPC
    > > > >   alignment w/ the OSPF model)
    > > > > - RPC 'clear-adjacency' has an input node of 'interface' however
    > > this
    > > > is just
    > > > >   a string type.  Is there any reason this is not a
    > > > leafref/if:interface-ref
    > > > >   (much like in the OSPF model)
    > > > > - Child nodes within a container or list SHOULD NOT replicate the
    > > > parent
    > > > >   identifier per
    > > > >
    > > >
    > > https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-4.3