Subject: [lmap] AD evaluation: draft-ietf-lmap-yang-10

From: Alissa Cooper <[email-masked]>
Date: Tue, 24 Jan 2017 11:04:47 -0500

I have reviewed this document in preparation for IETF last call. I have a few substantive comments and questions that I’d like to discuss before issuing the LC. I’ve also included some nits and minor comments that should be resolved together with any IETF LC comments.

Substantive comments and questions:

= Section 3 =

"Configuration Information: This is modeled in the /lmap/agent
      subtree, the /lmap/schedules subtree, and the /lmap/tasks subtree
      described below.  Some items have been left out because they are
      expected to be dealt with by the underlying protocol."

I think this needs some further elaboration about the items that have been left out. As far as I can tell the only items from the information model that this data model does not define are ma-config-credentials and ma-config-control-channels. There is already text further below to explain the expectation about how channels will be modeled, which should be referenced here. Is there not something more specific that can be said about how credentials will be handled/modeled, assuming the "underlying protocol" referenced for configuration is NETCONF?

= Section 4.2 =

(1) "It is generally a good idea to always configure
            an end time and to refresh the configuration
            of event object as needed to ensure that agents
            that loose connectivity to their controller
            do not continue their tasks forever."
            
This is more of a comment for the information model, but if this is worth saying here, shouldn't it (also) be mentioned in the corresponding information model definition in draft-ietf-lmap-information-model? This doesn't seem specific to this particular data model.

(2) For all the counters defined here, I think you need to specify the time period over which the counts are to be calculated.

(3) 'list action {
             key name;
             description
               "An action describes a task that is invoked by the
                schedule. Multiple actions are invoked sequentially.";'

I don't get this -- doesn't the execution mode define whether the actions are executed sequentially, in parallel, or pipelined?

(4) "A queue is internally used to pass
                  results to another schedule."

I thought it was up to the implementation to decide how to implement this?

(5) I don't understand why the 'program' elements are included in the task configurations and capabilities. It doesn't seem wise to allow the controller to tell the MA which program it needs to use to complete a task, and I don't understand why the MA would need to communicate that information to the controller either. I thought the recent list discussion indicated that if an MA was not capable of performing an action, that action would simply fail, which seems like all that is needed.

Nits and minor comments:

= Section 4.1 =

Please provide a reference for ISO 8601.

= Section 4.2 =

s/loose connectivity/lose connectivity/

s/schedule determins/schedule determines/

s/A set of month/A set of months/

s/A set of second/A set of seconds/

= Section 5 =

(1) Just a suggestion, but I think this is worth calling out specifically:

OLD
Unauthorized access may leak measurement results.

NEW
Unauthorized access may leak measurement results, including from passive measurements.

(2) "Implementers MUST taken care that
   option names and values are passed literally to programs.  In
   particular, it MUST be avoided that any shell expansions are
   performed that may alter the option names and values."

This text strikes me as a bit odd. Surely there are a whole selection of good programming practices that are necessary to ensure that things don't go haywire when implementing LMAP -- why call out these two with normative recommendations? Why does this guidance only apply to options? I would recommend having this text be non-normative, but if you do keep it I would suggest the following:

Implementers MUST take care that
   option names and values are passed literally to programs.  In
   particular, shell expansions that may alter the option names and values MUST NOT be performed.

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 24 Jan 2017 21:23:05 +0100

Alissa,

thanks for your review comments. Let me see what can be done to
address them...

> I think this needs some further elaboration about the items that have been left out. As far as I can tell the only items from the information model that this data model does not define are ma-confi…

I think this is related to the issue you raised for the information
model. Until we are clear what the MA credentials not associated to
a channel were intended to be used for, I can't really answer this
specific question.

> This is more of a comment for the information model, but if this is worth saying here, shouldn't it (also) be mentioned in the corresponding information model definition in draft-ietf-lmap-informat…

I see your point. I think it makes sense to note this in the second
item 4. of section 3 of the information model. (I will change the
numbering of the second list to use letters to make it easier to refer
to things.)

       [...] For Event objects specifying a
       series of events, it is generally a good idea to configure an end
       time and to refresh the end time as needed to ensure that MAs
       that loose connectivity to their controller do not continue
       executing Schedules forever.

> (2) For all the counters defined here, I think you need to specify the time period over which the counts are to be calculated.

We use counters as defined in RFC 6991 and I think the definition in
RFC 6991 takes care of things.

> I don't get this -- doesn't the execution mode define whether the actions are executed sequentially, in parallel, or pipelined?

Good catch. Changed this to:

           list action {
             key name;
             description
               "An action describes a task that is invoked by the
		schedule. Multiple actions are invoked according to
                the execution-mode of the schedule.";

> I thought it was up to the implementation to decide how to implement this?

OK. I meanwhile understand that the word 'queue' has too many
connotations. Is 'buffer' a less problematic term? The key here is
that the data producer and the data consumer are in general not
running at the time and hence data needs to be stored temporarily
somewhere. I will replace queue with buffer for now.

> (5) I don't understand why the 'program' elements are included in the task configurations and capabilities. It doesn't seem wise to allow the controller to tell the MA which program it needs to use…

There are multiple things:

(a) How do I tell which task I want to have executed? The information
    model assumes that this can be done with the help of a registry.
    The YANG data model, in addition, allows to use a simple 'program
    name'. Note that this is a choice, i.e., you either use a registry
    or a program name, but not both.

    The registry at the end is just some level of indirection - but
    this indirection also requires to have tasks registered in a
    registry. Right now, the implementation I know of only supports
    program names.

(b) The task list in the capabilities branch serves as an inventory,
    i.e., it tells the controller which tasks are supported by a given
    implementation. The other task list defined options that are used
    when a certain task is invoked (the Task Configuration in the
    information model). If a controller configures a task that the
    agent does support (i.e., it is not listed in the capability
    tasks), it will not be executed.

Note that a capability task name 'traceroute' exposed by the LMAP
agent does not necessarily mean that there is a program called
traceroute at the operating system level. In fact, an implementation
could choose to run traceroute internally without an explicit system
level process (like RIPE Atlas did everything in a big event loop, not
sure whether this is still the case).
 
> Please provide a reference for ISO 8601.

added

> s/A set of second/A set of seconds/

fixed
 
> Unauthorized access may leak measurement results, including from passive measurements.

added

>    particular, shell expansions that may alter the option names and values MUST NOT be performed.

I have no strong opinion on MUST vs must here and I usually happily
follow the advice of IESG members on RFC 2119 keywords. ;-)

Yes, there are many ways to write broken code but this is such a
common problem that I think can't hurt to remind implementors.

A diff of -10 to my working copy (of -11) can be found here:

  http://beadg.de/lmap/

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>

---

From: Alissa Cooper <[email-masked]>
Date: Wed, 25 Jan 2017 13:25:26 -0500

Hi Juergen,

Responses below. I trimmed out areas that don’t need a response.

> On Jan 24, 2017, at 3:23 PM, Juergen Schoenwaelder <[email-masked]> wrote:
> 
>> = Section 4.2 =
>> 

Sounds good.

> RFC 6991 takes care of things.

Ok.

>                the execution-mode of the schedule.”;

Sounds good.

> somewhere.

Isn’t this implied though? Even with “buffer” it still seems like specifying an implementation detail. I’d prefer to see something like your last sentence above instead.

> I will replace queue with buffer for now.
> 

Ok, I can understand that under circumstances where there is no registry, there needs to be some identifier to indicate which task to run, and which tasks the MA is capable of running. But the examples seem to imply that ‘program’ is the path and file name of the actual executable on the MA, which is what seems dangerous and unnecessary to me. Why would the controller necessarily even know where such executables reside on the file system? And I know there are a lot of things that could go wrong if a Controller gets compromised, but it just seems like making it so trivial for an MA implementation to literally just run the executable name specified by the Controller creates unnecessary risk.

If the task name on its own is not sufficient for the MA to be able to figure out which program is suitable to run, why not have the additional field defined as ‘program-name’ with some guidance about how to populate it, so that, e.g., what you end up with in that field is “mtr” or “fping” instead of "/usr/bin/mtr” or "/usr/bin/fping”?

> follow the advice of IESG members on RFC 2119 keywords. ;-)

Ok, my suggestion is to use non-normative “ought to” rather than 2119 MUST.

Thanks,
Alissa

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Thu, 26 Jan 2017 11:17:06 +0100

Trimming things down...

> Isn’t this implied though? Even with “buffer” it still seems like specifying an implementation detail. I’d prefer to see something like your last sentence above instead.

So I will replace 'buffer' with 'somewhere'. Oh boy. Here is the
new text:

               A set of schedules receiving the output produced
               by this action. The output is stored temporarily
               somewhere since the destination schedules will in
               general not be running when output is passed to
               them. The behaviour of an action passing data to
               its own schedule is implementation specific.

> Ok, I can understand that under circumstances where there is no registry, there needs to be some identifier to indicate which task to run, and which tasks the MA is capable of running. But the examples seem to imply that ‘program’ is the path and file name of the actual executable on the MA, which is what seems dangerous and unnecessary to me.

I can take out the path if that helps.

Why would the controller necessarily even know where such executables reside on the file system? And I know there are a lot of things that could go wrong if a Controller gets compromised, but it just seems like making it so trivial for an MA implementation to literally just run the executable name specified by the Controller creates unnecessary risk.

Perhaps we need to add more explicit text to /tasks/task saying that a
configured LMAP task MUST resolve to a task listed in the capabilities.
This is in my view what matters most.

> If the task name on its own is not sufficient for the MA to be able to figure out which program is suitable to run, why not have the additional field defined as ‘program-name’ with some guidance ab…

I do not really see the logic here. An implementation that blindly
executes a program called 'mtr' by search a search PATH may actually
be worse than an implementation that executes /some/path/mtr.

> Ok, my suggestion is to use non-normative “ought to” rather than 2119 MUST.

Seriously? I have left the 'must' in (there are other occurances of
must) and I leave it to the discretion of the IESG to discuss this if
needed - and then I do whatever comes out of it.

/js