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

["maqiufang (A)" <[e-mail]>]

Hi, Adrian

Thank you so much for your comments, we are going to incorporate them to the next version.
See https://raw.githubusercontent.com/boucadair/policy-based-network-acl/gh-pages/draft-ma-opsawg-ucl-acl.txt  for the proposed update, and https://author-tools.ietf.org/diff?doc_1=draft-ma-opsawg-ucl-acl-02&url_2=https://raw.githubusercontent.com/boucadair/policy-based-network-acl/gh-pages/draft-ma-opsawg-ucl-acl.txt  for the diff against your reviewed version(v-02).

Please also see my reply inline.


===

General

You need to decide between "user group" and "user-group"
Similarly, "SDN Controller" or "SDN controller"
[Qiufang] Thanks for pointing them out. We've checked the whole text and replaced "user-group" with "user group" and "SDN Controller" with "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.
[Qiufang] Okay. I used the draft-ietf-netmod-rfc6991-bis and removed the reference to RFC 6991.
---

1.

s/employees/employers/
s/induces/introduces/
[Qiufang] I guess this sentence needs some tweaks, how about:
OLD: " Enabling office flexibility (e.g.,
   mobility across many access locations) for large-scale employees
   induces a set of challenges compared to conventional network access
   management approaches. "
NEW: " Enabling office flexibility (e.g.,
   mobility across many access locations) introduces a set of challenges
   for large-scale enterprises compared to conventional network access
   management approaches."
---

   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.
[Qiufang]Sure, added as: "This document defines a common schedule YANG module (Section 6.1)
   which is ..."
---

   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.
[Qiufang]Yes, good catch. Fixed.
---

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).
[Qiufang] Thanks! This looks better.
---

2.

>> s/In the current version of the document/In this document/ d/also/ 
>> s/connect/connects/ s/While host device here/Host device/
[Qiufang]Fixed.
---

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
[Qiufang] Fixed.
s/heavy/a heavy/
[Qiufang] Fixed.
---

3.

Question:

Is it for each IP address, or for each user?
[Qiufang] The point is that conventional network access control fails to recognize user's identity. So I suppose it is for each IP address or network segment.
---

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                                                                 
[Qiufang] Fixed.
---

4.1

s/a NAS/an NAS/
[Qiufang] I thought it might be right way to use "a NAS", given the same usage in https://datatracker.ietf.org/doc/html/rfc2881, e.g., in section.2 of rfc2881, "A NAS is sometimes referred to as a Remote Access Server (RAS) as it
   typically allows remote access to a network." Agreed?
---

Figure 1

Not sure you need two horizontal lines between Service and Network Maybe use ======================
[Qiufang] A single horizontal line would be enough, I've fixed this figure.
---

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/
[Qiufang]All fixed.
---

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"?
[Qiufang] You're right, this should be fixed. A specific PEP doesn't have to maintain only one type, both types could exist.
---

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.
[Qiufang] Yes, I mean your first assumption, and good catch. but I am thinking that this section talks about user group instead of user group ID, I'd like to reword the first sentence and re-order. See if the following would be better:
OLD:
   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.
NEW:
The user group is determined by a set of predefined policy criteria
   (e.g., source IP address, geolocation data, time of day, or device
   certificate).  It uses an identifier (user group ID) to represent the
   collective identity of a group of users.  Users may be moved to
   different user-groups if their composite attributes, environment,
   and/or local enterprise policy change. 
---

4.2.2

s/an server/a server/
s/accessing to enterprise/accessing an enterprise/
[Qiufang]  Fixed.
---

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.
[Qiufang] Sure, maybe we can define this term in Section 2. Conventions and Definitions as follows:
     User group based ACL (UCL):  A YANG data model for policy-based
         network access control that specifies an extension to the IETF
         ACL model defined in [RFC8519].  It allows policy enforcement
         based on the group identity, which can be used both at the
         network device level and at the network/administrative domain
         level.
Would this be okay?
---

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
[Qiufang] Yes, this looks better, I've fixed it as you proposed but renamed some nodes. Thanks.
           +-- period-start?            yang:date-and-time
                 +-- (period-type)?
                    +--:(period-explicit)
                    |  +-- period-end?        yang:date-and-time
                    +--:(period-duration)
                       +-- period-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.                                      
[Qiufang] Sure. I've added some explanation as suggested, you can review that.
---

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.
[Qiufang] Okay, otherwise I am fine to move the examples to sec.6. But I thought it might be good to directly have more detailed examples after the explanation of this module.
---

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.
[Qiufang]Sure, the following text added at the very beginning of sec.5.2:
      This module specifies an extension to the IETF ACL model [RFC8519].
   This extension adds endpoint groups so that an endpoint group index
   can be matched upon, and also enable access control policy activation
   based on date and time conditions.
---

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.
[Qiufang] Yes, agree. We've added some explanation below the true diagram, and please review that.
---

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.                                                      
[Qiufang]Sure, fixed.
---

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.
[Qiufang] We tried to add both some more explanation, and a more precise reference.
---

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?
[Qiufang]Well, "bymonthday" means days of the month, "byyearday" means days of the year, and we already have "byday" to specify days of the week, and "byyearweek" to specify weeks of the year.
I am thinking what are you referring to regarding "bymonthweekday", maybe a list of days of a list of weeks in the month? But that could already be convered by "byday" and a "weekly" frequency rule.
And "byyearweekday"? maybe you mean a list of days of a list of weeks in the year? That could be covered by configuring both a "byday" node and a "byyearweek" node. Make sense?
---

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.
[Qiufang] Yes, deleted in the new version.
Should probably note that you import from ietf-schedule defined in this document.
[Qiufang] Sure, added.
---

6.2

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

That's this document, right?
[Qiufang] Yes, and note that I've added an editorial note.
---

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)/
[Qiufang]Fixed.
---

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.)
[Qiufang]Fixed.
---

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.
[Qiufang] Added in the Conventions and Definitions section.
---

6.2

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

s/add/Add/
[Qiufang] has been fixed as "Adds".
---

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/ ??
[Qiufang] Fixed as " Specifies how a source and/or destination endpoint group index
          can be referenced as the match criteria in the ACEs. "
---

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
[Qiufang] Fixed.

---

9.1

Obviously, this section needs to be completed with references to the data nodes and sub-trees.
[Qiufang] Yes, fixed.

---

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
[Qiufang] Already removed.
---

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.
[Qiufang] Fixed.
---

Thanks again. 

Best Regards,
Qiufang