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

[tom petch <[e-mail]>]

----- Original Message -----
From: "Andy Bierman" <[e-mail]>
Sent: Thursday, January 10, 2019 5:45 PM

On Thu, Jan 10, 2019 at 4:42 AM Acee Lindem (acee)  wrote:
> Hi Andy,
>
> On 1/10/19, 7:38 AM, "Andy Bierman" <[e-mail]> wrote:
>
>     On Thu, Jan 10, 2019 at 12:34 AM 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.

Agreed -- that is a good guideline for protocol related strings.
But what about admin strings?

NETCONF/YANG does not have a data type like SnmpAdminString
(which has a range of 0..255 octets)

Some of us implement servers using the theory that the NMS knows
what it's doing and servers do what they are told.  So if the client
configured a 1M byte admin string the server would try to accept it
instead
of putting an arbitrary limit (because there is no YANG limit).

I prefer the SnmpAdminString approach because the client knows what
every compliant server will accept.  But YANG models just use plain
string
and only regression test tools ask for 1M byte admin strings. IMO, this
has
not been
a problem in real deployments. We should continue to use plain "string"
for
admin strings.

<tp>

Andy

Having spent decades working with SNMP, I am well familiar with, and
comfortable, with SnmpAdminString and it is doubtless this background
that lies behind some of my comments on strings.  Juergen is right to
differentiate what would be a protocol failure, and so suitable for a
YANG range statement, from a nice-to-know, suitable for a YANG
'description'.  But then I see discussions like the one I referred to on
the IDR WG list about how long to allow for an error message and think
that YANG is missing an adminstring type so I find myself commenting on
the YANG descripiton, wanting it to say more, and I find myself doing
this across several WG..

Tom Petch

Thanks,
> Acee

Andy

>     > Brgds,
>
>     Andy

>     > *From:* Andy Bierman [mailto:[e-mail]]
>     > *Sent:* Wednesday, January 09, 2019 18:06
>     >
>     > 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