Re: [OPSAWG] A review of draft-ma-opsawg-ucl-acl

[Adrian Farrel <[e-mail]>]

Resending this cos somehow by autocomplete got mangled.

Adrian

-----Original Message-----
From: Adrian Farrel <[e-mail]> 
Sent: 22 May 2023 09:59
To: '[e-mail]' <[e-mail]>
Cc: '[e-mail]' <[e-mail]>
Subject: A review of draft-ma-opsawg-ucl-acl

Hi all,

I think that enhancing our ability to configure ACLs could be useful, so I
had a look at this draft. I have some comments...

Best,
Adrian

===

General

You need to decide between "user group" and "user-group"
Similarly, "SDN Controller" or "SDN controller"

---

General

You need to decide whether to use/reference RFC 6991 or
draft-ietf-netmod-rfc6991-bis. I think you can use just the I-D as it
obsoletes the RFC and will be published soon.

---

1.

s/employees/employers/
s/induces/introduces/

---

   This document defines a common schedule YANG module which is designed
   to be applicable for policy activation based on date and time
   conditions.  This model is designed with the intent to be reusable in
   other scheduling contexts.

Like the following paragraph, a forward reference to the section that
defines the module would be nice.

---

   Section 5.2 defines a YANG module for policy-based Network Access
   Control, which extends the IETF Access Control Lists (ACLs) module
   defined in [RFC8519].  This module can be used to ensure consistent
   enforcement of ACL policies based on the group identity.

I think it is 6.2 that defines the module. Section 5.2 describes the module.

---

1.

   As the ACL notion has been generalized, not to be device-specific,
   but also be defined at network/administrative domain levels
   [I-D.dbb-netmod-acl], the YANG module for policy-based network access
   control defined in Section 5.2 does not limit how it can be used.

This paragraph is hard to read! I would also re-order these four paragraphs
to get:

   Section 6.2 defines a YANG module for policy-based Network Access
   Control, which extends the IETF Access Control Lists (ACLs) module
   defined in [RFC8519].  This module can be used to ensure consistent
   enforcement of ACL policies based on the group identity.

   The ACL notion has been generalized to be device-nonspecific, and can
   be defined at network/administrative domain levels
   [I-D.dbb-netmod-acl].  To allow for all applications of ACLs, the
   YANG module for policy-based network access control defined in
   Section 6.2 does not limit how it can be used.

   This document also defines a mechanism to establish a mapping between
   the user-group identifier (ID) and common IP packet header fields and
   other encapsulating packet data (e.g., MAC address) to execute
   policy-based access control.

   Additionally, the document defines a Remote Authentication Dial-in
   User Service (RADIUS) [RFC2865] attribute that is used to communicate
   the user group identifier as part of identification and authorization
   information (Section 7).

---

2.

>> s/In the current version of the document/In this document/
>> d/also/
>> s/connect/connects/
>> s/While host device here/Host device/

---

3.

OLD
   (2) the source IP
   addresses of the same user are in different network segments,
NEW
   (2) a user may have
   different source IP addresses in different network segments,
END

s/heavy/a heavy/

---

3.

Question:

Is it for each IP address, or for each user?

---

4.1

OLD                                                                 
   To provide real-time and consistent enforcement of access control   
   policies, the following functional entities and interfaces are      
   involved:                                                           
NEW                                                                 
   The architecture of a system that provides real-time and consistent 
   enforcement of access control policies is shown in Figure 1 and     
   includes the following functional entities and interfaces.          
END                                                                 

---

4.1

s/a NAS/an NAS/

---

Figure 1

Not sure you need two horizontal lines between Service and Network
Maybe use ======================

---

4.1

s/protocols like/a protocol such as/
s/user-group which/user-group the identity of which/
s/the NAS notify/the NAS notifies/
s/the mapping between/of the mapping between/
s/under the controller's/under the SDN controller's/
s/type of ACL policies/type of ACL policy/

---

4.1

Question

   Whether the PEP enforces the group or IP/MAC address based ACL is
   implementation specific.  Either type of ACL policies may exist on
   the PEP.

 You say "either type", but perhaps you may mean "both types"?

---

4.2

Question

   The user-group ID is an identifier that represents the collective
   identity of a group of users.  It is determined by a set of
   predefined policy criteria (e.g., source IP address, geolocation
   data, time of day, or device certificate).  Users may be moved to
   different user-groups if their composite attributes, environment,
   and/or local enterprise policy change.

You say that "It is determined". That implies the *ID* is determined by the
predefined criteria. If you mean that the user group is determined, you
should s/It/The user group/

On the other hand, if you are describing the ID, then I think "determined"
may be the wrong word. Perhaps...
"The user-group ID is constructed according to a set of..."

I think, looking at the YANG, that the ID is just a number, so you probably
mean the first of these.

---

4.2.2

s/an server/a server/
s/accessing to enterprise/accessing an enterprise/

---

5.1

Surprisingly, this is the first mention of UCL.

What is the UCL model?

I think you need to expand "UCL" and give a forward reference to section
6.1. 

But it seems that the term should have been introduced and discussed earlier
in the document.

---

Figure 2 (and the model itself)

                     +--:(period-explicit)
                     |  +-- explicit-start?   yang:date-and-time
                     |  +-- explicit-end?     yang:date-and-time
                     +--:(period-start)
                        +-- start?            yang:date-and-time
                        +-- duration?         duration

It's not important, but you could reduce this as
                     +-- period-start?        yang:date-and-time
                     +--:(period-explicit)
                     |  +-- explicit-end?     yang:date-and-time
                     +--:(period-duration)
                         +-- duration?         Duration

---

5.1

This section is missing a simple explanation of the tree.            
I suppose that the reader can look this up in RFC 5545, but it       
would be nicer to give some text such as:                            
--                                                                   
The 'period-of-time' allows a time period to be represented using    
either a start and end date and time, or using a start time and      
a duration.                                                          
                                                                     
The 'recurrence' allows an indication of the repeat scheduling of     
an event. The repetition can be scoped by a specified end time or    
by a count of occurrences, and the frequency can be given in time     
units.                                                               
--                                                                   
Of all this, 'direction' 'setpos' and 'wkst' are perhaps the most in 
need of additional explanation.                                      

---

5.1.1

The examples are good, and they are helpful. It is a bit odd to have these
examples of the YANG model before we have seen the YANG model.
I suppose that's OK.

---

5.2

Needs some introduction like at the top of Section 5.1
Move up the paragraph from the end of the section? And add a few more words.

---

5.2

This YANG model is large enough that you need to provide some description of
the key points. At least the main branches and any notable leaf nodes.

---

6.1

   import ietf-yang-types {                                            
     prefix yang;                                                      
     revision-date 2023-01-23;                                         
     reference                                                         
       "RFC XXXX: Common YANG Data Types";                             
                                                                       
 You don't mean XXXX because you use that for the RFC that this draft will
become.                                                          

 You need to use YYYY and add an Editor's Note that this number is the RFC
number for the RFC that draft-ietf-netmod-rfc6991-bis becomes 
 Alternatively, simply make the reference here to the draft - that might be
better.                                                      

---

6.1

     typedef duration {
       type string {
         pattern '((\+)?|\-)P((([0-9]+)D)?(T(0[0-9]|1[0-9]|2[0-3])'
               + ':[0-5][0-9]:[0-5][0-9]))|P([0-9]+)W';
       }
       description
         "Duration of the time. The format can represent nominal
          durations (weeks and days) and accurate durations (hours,
          minutes, and seconds). Note that this value type doesn't
          support the 'Y' and 'M' designators to specify durations in
          terms of years and months. Negative durations are typically
          used to schedule an alarm to trigger before an associated time.";
       reference
         "RFC 5545: Internet Calendaring and Scheduling Core Object
          Specification (iCalendar)";
     }

Contains a line that is too long.

While the Reference is helpful, it is pretty hard to know from this and the
Description clause how to encode/decode the string.
I think you need to add some explanation or a more precise reference.

... Similar issue with 'period-start' although there is no Reference there.

---

6.1

         list byday {
           key "weekday";
           description
             "Specify a list of days of the week.";
           leaf-list direction {
             when '(enum-value(../../freq) = 6) or ' +
               '(enum-value(../../freq) = 7) and not(../../byyearweek)';
             type int32 {
               range "-53..-1|1..53";
             }
             description
               "When specified, it indicates the nth occurrence of a
                specific day within the MONTHLY or YEARLY 'RRULE'. For
                example, within a MONTHLY rule, +1 monday represents the
                first monday within the month, whereas -1 monday
                represents the last monday of the month.";
           }
           leaf weekday {
             type schedule:weekday;
             description
               "Corresponding to seven days of the week.";
           }
         }

         leaf-list bymonthday {
           type int32 {
             range "-31..-1|1..31";
           }
           description
             "Specifies a list of days of the month.";
         }
         leaf-list byyearday {
           type int32 {
             range "-366..-1|1..366";
           }
           description
             "Specifies a list of days of the year.";
         }


Unclear why there is a lack of symmetry. 
You have 'bymonthday' and 'byyearday', but you don't have 'bymonthweekday'
and 'byyearweekday'.  
Why the difference?

---

6.2

   This module imports types defined in [RFC6991], [RFC8194], and
   [RFC8519].

You should import from draft-ietf-netmod-rfc6991-bis not RFC6991.

But, having said that, I don't see any imports from 6991 or 8194.

Should probably note that you import from ietf-schedule defined in this
document.

---

6.2

     import ietf-schedule {
       prefix schedule;
       reference
         "RFC XXXX: A Policy-based Network Access Control";
     }

That's this document, right?

---

6.2

     organization
       "IETF OPSWG Working Group";
     contact
       "WG Web: <https://datatracker.ietf.org/wg/opsawg/>
        WG List: <mailto:[e-mail]>";
     description
       "This YANG module augments the IETF access control lists(ACLs)

s/lists(ACLs)/lists (ACLs)/

---

6.2

     identity group-acl-type {
       if-feature "group";
       base acl:acl-base;
       description
         "An ACL that matches based on an endpoint group identity,
          which can represent the collective identity of a group of
          authenticated users or enterprise end devices. An endpoint
          group identity may be carried in the outer/inner packet
          header(e.g., via NVO3 encapsulation), but may not correspond
          to any field in the packet header.";
     }

s/header(e.g./header (e.g.)

---

6.2

     identity mixed-ipv4-group-type {
       if-feature "mixed-ipv4-group";
       base acl:ipv4-acl-type;
       base uacl:group-acl-type;
       description
         "An ACL that contains a mix of entries that match on fields
          in IPv4 headers and endpoint group identities, which can
          represent the collective identity of a group of authenticated
          users or enterprise end devices. Matching on Layer 4 header
          fields may also exist in the ACEs.";
     }

Need to explain (somewhere) what is "ACE". Ah, you do it a couple of pages
later, but probably should be in the text sections of the doc.

---

6.2

     augment "/acl:acls/acl:acl" {
       description
         "add a new container to store endpoint group information.";

s/add/Add/

---

6.2

     augment "/acl:acls/acl:acl/acl:aces/acl:ace/acl:matches" {
       description
         "Add another choice to allow ace match based on endpoint group
          id.";

s/ace/ACE/ ??

---

8.

Access- Access- Access-  Challenge Acct.    #        Attribute
Request Accept  Reject             Request
 0+      0+      0        0         0+      241.TBA1 User-Access-Group-ID

Line too long

---

9.1

Obviously, this section needs to be completed with references to the data
nodes and sub-trees.

---

11.2

   [NIST-ABAC]
              Hu, V. C., "Guide to Attribute Based Access Control (ABAC)
              Definition and Considerations", January 2014,
              <https://www.iana.org/assignments/media-types>.

Reference not used

---

A.3

Your examples should use addresses from the  documentation address ranges
192.0.2.x, 198.51.100.x, or 203.0.113.x.

---