Subject: [Netconf] js review of draft-ietf-netconf-access-control-04

From: Juergen Schoenwaelder <[email-masked]>
Date: Sun, 24 Jul 2011 20:55:23 +0200

I have reviewed draft-ietf-netconf-access-control-04. I am fine with
the document in general, although I have a number of issues and
suggestions but nothing I do consider a show stopped on the technical
side. The comments are in document order.

1) I suggested to put the acronym NETCONF into the title so that RFCs
   related to NETCONF are easy to search in the RFC list:

   Network Configuration Protocol (NETCONF) Access Control Model

2) I suggest to get rid of the section titles for section 1.1.1,
   1.1.2, 1.1.3, and 1.1.4. 

3) s/conceptual criteria used/criteria used/

4) I do not understand figure 1. Perhaps the idea is that the
   top-right box is accessing state data but that conflicts with the
   text on the arrow pointing to data node access allowed. The text
   did not help me to understand this either.

   Should the arrow pointing from the lower-right box point to
   "notification" instead of client session? Should there be an arrow
   "client reply"? Or even better, should this be "RPC request" and
   "RPC reply"?

5) In 2.2, what is "NETCONF session output"? Did you mean "state
   data"?

6) In 2.3, s/procedural interface model/remote procedure call model/
   since this is the way 6241 describes this.

7) In 2.3, s/candidate configuration/candidate configuration datastore/
   and s/running configuration/running configuration datastore/

8) My understanding of 2.4.4 is that copy-config fails as soon as
   there is a non-readable object. If so, can you spell out which
   error is being generated?

9) Should 2.6 have been:

   It SHOULD be possible to disable part or all of the access control
   model without deleting any access control rules.

10) Should 2.7 start with "Suitable configuration and monitoring..."
    instead of control?

11) Figure 2 adds details to figure 1 but I am still confused what the
    "<rpc-reply> acc. ctl" box does. If it is protecting state data
    sitting in server instrumentation, why is it not called from the
    "<rpc> processor" much like the "data node acc. ctl"?  Perhaps
    "data node acc. ctl" really is "configuration datastore access
    control" and the other box was meant to be "state data access
    control"? The text again did not help me to figure this out.
    (How does the second item on page 17 map to the figure?)

12) s/be configured in multiple groups/be a member of multiple groups/

13) Section 3.2.3 states that access is granted to sessions. I am not
    sure this is true - at least the examples seem to convince me that
    access is granted to users. Do we need 3.2.3 at all?

14) The first sentence in 3.2.4 does not really tell me much and seems
    to be a bit recursive.

15) In 3.2.5.1, s/access requested/requests/ ?

16) Should there be a notification-default switch as well?

17) If I set exec-default to "deny", then nothing seems to be possible
    anymore except the <hello> exchange. Even notifications require an
    RPC to subscribe to a stream. So how useful is the exec-default
    switch?

18) I assume with "module" you mean "YANG module" or perhaps more
    generic "data model module". (In the NETCONF monitoring data
    model, this is called a schema identifier - boah.)

19) My reading of the rules of procedure is that a user that does not
    map into a group simply bypasses all access control rule checks
    and ends up with access denied if the corresponding default switch
    is set to "permit". Is this desirable behaviour? You mistype a
    name in a group configuration and the result is open access?

20) Can we replace the text in 3.4.1 with pyang -f tree output? I
    happen to find that format more useful to understand the structure
    than lots of text (and it is remarkably simply in this case since
    the data model is remarkably short).

21) I would have used the prefix and toplevel node ncacm instead of
    nacm. For the prefix, this would give us consistency with other
    NETCONF prefixes we already have (nc, ncm, ncwd, [ncn], ...). 

21) s/NETCONF Server Access Control Model/NETCONF Access Control Model/

22) There is of course a question what is secure and very-secure and
    why two levels are needed or enough. But only time will tell..
    (no action expected).

23) improved description bit create (access-operation-type):

    Any operation that creates a new data node.

24) improved description bit read (access-operation-type):

    Any operation or notification that returns the value of
    a data node is a read operation.

25) improved description bit delete (access-operation-type):

    Any operation that removes a data node is a delete operation.

26) improved description group-name-type:

    Name of an administrative group to which users can be assigned.

27) Is denied-data-writes and bumped once per RPC execution or
    multiple times if multiple data nodes are affected? I expect the
    former but it should be clarified (perhaps even spell this out
    in the elements of procedure?).

28) Why is the rule-list name and the rule name restricted to 1..256?
    Seems arbitrary and we do not have restrictions on user names or
    group names.

29) Several comments about the examples: I find the examples very
    helpful. I like to propose some name changes to make them easier
    to read and to better memorize the purpose of rules.

A.2:

OLD				NEW
<name>guest</name>		<name>guest-acl</name>
<name>mod-1</name>		<name>deny-ncm</name>
<name>monitor example</name>	<name>monitor-acl</name>
<name>mod-2</name>		<name>permit-ncm</name>
<name>mod-3</name>		<name>permit-exec</name>
<name>admin example</name>	<name>admin-acl</name>
<name>mod-4</name>		<name>permit-all</name>

A.3:

OLD				NEW
<name>guest</name>		<name>guest-monitor-acl</name>
<name>rpc-1</name>		<name>deny-kill-session</name>
<name>rpc-2</name>		<name>deny-delete-config</name>
<name>monitor</name>		<name>monitor-acl</name>
<name>rpc-3</name>		<name>permit-edit-config</name>

A.4:

OLD				NEW
<name>guest rules</name>	<name>guest-acl</name>
<name>data-1</name>		<name>deny-n:ncacm</name>
<name>monitor rules</name>	<name>monitor-acl</name>
<name>data-acme-config</name>	<name>permit-acme-config</name>
<name>dummy-itf</name>		<name>permit-dummy-interface</name>
<name>admin rules</name>	<name>admin-acl</admin>
<name>admin-itf</name>		<name>permit-interfaces</admin>

I believe it is important to use "speaking" names in the examples
since people will copy from the examples when they write their own
access control rules.

/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: Martin Bjorklund <[email-masked]>
Date: Thu, 28 Jul 2011 15:56:06 -0400 (EDT)

Hi,

We have updated the document taking your comments into account.  See
inline.

> 16) Should there be a notification-default switch as well?

This is covered by read-default.

>     RPC to subscribe to a stream. So how useful is the exec-default

This switch allows a deployment where all acccess rules are explicit.
Some operators might want to configure their boxes in such very secure
mode.

>     name in a group configuration and the result is open access?

This is exactly what the default switches are used for.  This is also
why write-default is deny by default.  This is the other end of the
spectrum - once you're authenticated you get full read/write/exec
access.

/martin & andy