Hi Jan, Could you update the state of YANGDOCTORS Last Call Review on I2NSF Consumer-Facing Interface YANG Data Model if the updates are fine to you?
https://datatracker.ietf.org/doc/draft-ietf-i2nsf-consumer-facing-interface-dm/ I think your comments are all addressed in this version. Thanks. Best Regards, Paul On Thu, Mar 12, 2020 at 1:15 AM Mr. Jaehoon Paul Jeong < [email protected]> wrote: > Hi Jan, > We authors have addressed your comments with the revision: > > https://tools.ietf.org/html/draft-ietf-i2nsf-consumer-facing-interface-dm-08 > > I attach a revision letter to explain how to respond to your comments. > > If you have further comments, please let me know. > > Thanks. > > Best Regards, > Paul > > On Tue, Nov 12, 2019 at 1:53 AM Jan Lindblad via Datatracker < > [email protected]> wrote: > >> Reviewer: Jan Lindblad >> Review result: Almost Ready >> >> This is my YD review of draft-ietf-i2nsf-consumer-facing-interface-dm-07. >> I >> have previously reviewed the -05 revision (~end June). I find the new >> revision >> much improved, but still with much to discuss. I will call this "almost >> ready". >> >> Generally speaking, I think the YANG module lacks the precision and >> descriptions needed to foster interoperability. The examples at the end >> are >> very enlightening however, and compensate for much of that, but their >> informal >> nature can never replace proper YANG. The module usage needs to be mostly >> clear >> from the module itself. >> >> The management access control model proposed here is, even with its latest >> adaptation towards NACM, is still quite different from NACM author's >> original >> ideas. I will therefore bring this use case up in the NETMOD WG for >> discussion. >> >> 1. Network access control principles >> >> Network access control is about which users are able to use the network >> being >> managed, for example connect to facebook. The purpose of the NSF module >> is to >> control this access. This version of the YANG module is now based on a >> list of >> policies. >> >> Each policy has a list of rules. Each rule has an event -- condition -- >> action >> triplet. This resembles traditional firewall management, which is a good >> thing, >> because that concept is stable and much tried. This allows operators to >> create >> lists of rules in this style: >> >> if pkt.x == 1: drop // Rule 1 >> elif pkt.y > 2: alert // Rule 2 >> elif pkt.z == 10: pass // Rule 3 >> else: drop // Rule 4 >> >> This pattern relies heavily on the ability to control the order of the >> rules. >> The current model relies on the alphabetical sorting of names rules for >> the >> ordering. The YANG trick I would recommend to give operators the ability >> to >> insert and move rules as they wish is to add ordered-by user on the list: >> >> list rule { >> ordered-by user; // <== Add this line >> leaf rule-name { >> >> Nothing is said about what the system should do in case policies >> conflict. What >> if one policy says pass, the other drop for the same packet? Please >> clarify. >> What should happen to packets that do not match any of the policies? >> >> This module also assumes that all users in the operator's organization are >> listed in one or more NACM groups (e.g. "employees"). That wasn't really >> the >> NACM authors' original intent. Even if this could be made to work maybe, >> there >> is no strong reason to repurpose the NACM group concept for user network >> access >> purposes. It could easily be modeled differently. So in the cases where >> there >> are leafrefs to NACM groups when dealing with network access rather than >> management access, don't use NACM groups. >> >> leaf src-target { >> type leafref { >> path >> /nacm:nacm/nacm:groups/nacm:group/nacm:user-name; // >> <== Point to some other list of network users >> >> 2. Management access control principles >> >> Management access control is about which users are able to configure/run >> actions/the policies and rules. IMO, the most controversial aspect of this >> module has always been its new and creative management access control >> model. In >> this revision, the management principles have been remodeled greatly to >> fit in >> with NACM. I find this redesign very promising, but the result is still >> not >> quite ready for publication. >> >> The point where integration with NACM concepts is important is when it >> comes to >> allow some users to CRUD the NSF policies and rules themselves. There is >> now a >> leaf-list "owners" on each policy and rule which point to a list of NACM >> groups. My understanding is that the idea is that the NSF module should >> be seen >> as a service model that translate high level ownership information to >> specific >> NACM rules. It would be good to mention these ideas somewhere in the NSF >> document. >> >> leaf-list owners { >> type leafref { >> path /nacm:nacm/nacm:groups/nacm:group/nacm:name; >> >> I expect the intent is that any user listed in a NACM group mentioned in >> the >> owners list would get full CRUD privileges for the contents of the rule >> the >> owners leaf sits on. That is never spelled out anywhere, however. >> >> It is a little less clear how leaf-list owners on policy objects should be >> handled. Should owners listed on a policy object get full CRUD powers >> over the >> entire policy, including all the rules inside? Or would they need to be >> listed >> on the rules as well? Not clear. Is the intent that users not listed on >> the >> policy object are unable to create new rules, but to be able to update >> rules >> they are listed as owners of, if any? >> >> Who is allowed to create new policy objects? Should users that are not >> owners >> get read access to all the policies and rules? >> >> Finally, there is an "owner" leaf on each rule with an identityref >> allowing >> operators to configure a role name like dept-head or sec-admin. It is >> marked >> mandatory, but never included in any of the examples at the end of the >> document. This makes me think this may be a remnant from bygone times and >> should be removed from the YANG. If not, an explanation of how to use this >> leaf, and how it interacts with "owners" needs to be added, and the >> examples >> updated. >> >> 3. leafrefs crosspointing between policy instances >> >> There are six leafrefs that point to various objects inside a policy, >> e.g. a >> rule condition pointing to a device group name. None of them restrict >> what can >> be pointed to so that only names within the current policy are valid. It >> is >> therefore possible to configure the name of a device group defined in a >> different policy. I suspect this is not the intention. In order to >> restrict the >> leafrefs to the same policy, the following predicate can be added: >> >> leaf-list src-target { >> type leafref { >> path >> >> "/i2nsf-cfi-policy[policy-name=current()/../../../../../policy-name]/endpoint-group/device-group/name"; >> // <== Add predicate >> >> 4. Mandatory to implement all events, conditions, actions >> >> Each rule is defined with a choice of different events (admin, time), >> conditions (firewall, ddos, custom, threat-feed) and actions (pass, drop, >> alert, mirror, ...). Is the intent that all of these options should be >> mandatory to implement? The current model requires this. Also, would it >> make >> sense to allow additional mechanisms here? If so, it may be good to >> explain to >> readers how the set of choices and identities can be extended by >> implementations. >> >> 5. Optional and mandatory elements >> >> In this revision of the module, 8 leafs have been marked mandatory. A few >> of >> them are list keys, which are conventionally not marked mandatory, since >> list >> keys are always mandatory. A few others are skipped in the XML examples >> at the >> end of the NSF document, which makes me believe they might not really be >> mandatory after all. >> >> Three leafs have a default, but most leafs are left optional without any >> default. In many cases I think I understand what it means to not set a >> leaf, >> but with the ones listed here, I don't think it clear at all. Either add a >> default to make it clear, make them mandatory if they should be, or >> explain in >> the leaf description what happens if not set. >> >> 493: leaf-list name >> 513: leaf-list protocol >> 531: leaf geo-ip-ipv4 >> 541: leaf continent >> 562: leaf feed-server-ipv4 >> 585: leaf payload-description >> 590: leaf-list content >> 600: leaf-list owners >> 870: leaf method >> >> 6. Indentation >> >> The YANG indentation is mostly wrong. Run the YANG text through pyang or >> some >> other tool that can indent the content correctly before you put it into a >> document. >> >> 7. YANG element naming >> >> The YANG convention is to not have lists on the top level in the YANG >> module, >> but to surround lists with a container. The surrounding container often >> has a >> name in the plural and the list in singluar, like this >> >> container interfaces { >> list interface { >> >> To better fit into the world of IETF YANG modules, I'd recommend turning >> the >> top level list i2nsf-cfi-policy into this instead: >> >> container i2nsf-cfi-policies { >> list policy { >> >> Further down, I would change container rule to rules: >> >> container rules { >> list rule { >> >> Finally, it is customary to not repeat the names of parent object in the >> names >> of elements. For example, in the following: >> >> list threat-feed-list >> leaf feed-name >> leaf feed-server-ipv4 >> leaf feed-server-ipv6 >> leaf feed-description >> >> all the leafs should normally not repeat "feed-". Just leaf name, leaf >> server-ipv4, leaf server-ipv6, leaf description. There are many more >> examples >> of this throughout the module. >> >> The condition choice has many containers with a single leaf inside (e.g. >> ddos-source). Their purpose is rather unclear to me. Remove? >> >> container ddos-source { >> description >> "This represents the source."; >> leaf-list src-target { >> >> Also, I find the name "src-target" rather confusing. How about "source"? >> >> 8. No date leaf >> >> The draft text near fig 2 talks about a date leaf. There is no date >> object in >> this revision of te YANG. >> >> "Date: Date when this object was created or last modified" >> >> 9. leaf owner >> >> Near fig.3 leaf Owner is mentioned. Is this leaf still current? >> >> "Owner: This field contains the onwer of the rule. For example, >> the person who created it, and eligible for modifying it." >> >> 10. leaf packet-per-second >> >> This is now modeled as uint16. Is this future proof? Many packet flows on >> the >> internet exceed 64k pps. >> >> 11. container custon-source >> >> Misspelled. Should be custom-source >> >> 12. identity ddos >> >> Is ddos a malware file-type? This is not exactly in line with my >> intuition. >> >> 13. identity protocol-type >> >> There are other modules that already define protocol-types. Would it be >> worth >> reusing one of them? >> >> 14. identity palo-alto >> >> Is it a good IETF practice to list vendor names in modules? Can we >> consider >> this a protocol name? Is there perhaps an RFC/specification name for it >> that we >> could reference instead? >> >> 15. grouping ipsec-based-method >> >> This grouping contains a list which allows listing none of, either of or >> both >> of ipsecike and ikeless. Are all valid configurations? >> >> 16. leaf feed-name >> >> This leaf is the key in a list, which makes it possible to have at most >> one >> feed of each type. If it would make sense to configure more than one feed >> of >> the same type, the YANG needs to be updated here. >> >> 17. leaf-list content >> >> This leaflist is of type string. What is the format of this string? Does >> the >> name refer to something? >> >> 18. Event types >> >> container event has a choice between enforce-admin and time alternatives. >> Each >> of those choices have a leaf that allows the operator to configure an >> identityref to an enforce-type value. What does that mean? What would it >> mean >> if an operator configured admin == 'time' (or enforce-time == 'admin')? >> >> 19. leaf begin-time, end-time >> >> >From the examples, it can be seen that these are meant to be a time of >> day >> values. Currently they are modeled as yang:date-and-time, which means >> they are >> a concrete time a specific day, e.g. 2019-11-11T16:07. This needs to be >> changed >> in order to be what the modeler intended. Perhaps like this: >> >> typedef time-of-day { >> type string { >> pattern '(2[0-3]|[01]?[0-9]):[0-5][0-9]'; >> } >> } >> >> 20. leaf frequency >> >> This leaf is now modeled properly from a YANG perspective. But what does >> it >> mean? If this leaf is set to 'once-only', what exactly will happen only >> once? >> Please write a description that explains this. >> >> 21. Example in Fig.17 >> >> The example contains XML that refers to "endpoint-group/user-group". >> There is >> no such element in the YANG. >> >> <endpoint-group xmlns="urn:ietf:params:xml:ns:yang:ietf-i2nsf-cfi-policy"> >> <user-group> >> >> Furthermore, there is nothing called range-ip-address, start-ip-address, >> end-ip-address. They are called range-ipv4-address, start-ipv4-address, >> end-ipv4-address. >> >> <range-ip-address> >> <start-ip-address>221.159.112.1</start-ip-address> >> <end-ip-address>221.159.112.90</end-ip-address> >> </range-ip-address> >> >> Finally, there must not be any xmlns attribute on an closing XML tag. So >> >> </endpoint-group >> xmlns="urn:ietf:params:xml:ns:yang:ietf-i2nsf-cfi-policy"> >> >> should be >> >> </endpoint-group> >> >> This happens in several of the examples. >> >> 22. Example in Fig.18 >> >> There is no element called policy any more. It's now i2nsf-cfi-policy. >> >> <policy xmlns="urn:ietf:params:xml:ns:yang:ietf-i2nsf-cfi-policy"> >> <policy-name>security_policy_for_blocking_sns</policy-name> >> >> The rules are modeled in a container and list, both by the name rule. So >> there >> needs to be two <rule> tags. >> >> <rule> >> <rule-name>block_access_to_sns_during_office_hours</rule-name> >> >> The security-event element is marked mandatory in the YANG, but missing >> in the >> example. The times given below may be what is intended, but do not match >> the >> date format for the type used (which include a date, etc). >> >> <event> >> <time-information> >> <begin-time>09:00</begin-time> >> <end-time>18:00</end-time> >> </time-information> >> </event> >> >> Since the example is not mentioning leaf frequency, it will have the value >> 'once-only'. Maybe explain what that means in the context of the example? >> >> The condition/firewall-condition says the src-target is mandatory and >> dest-target optional, exactly like below. >> condition/custom-destination/dest-target is mandatory and src-target is >> optional, exactly like below. Is this pure luck, or is there a logical >> explanation why exactly those should be mandatory, and the example use >> precisely those? >> >> <condition> >> <firewall-condition> >> <source-target> >> <src-target>employees</src-target> >> </source-target> >> </firewall-condition> >> <custom-condition> >> <destination-target> >> <dest-target>sns-websites</dest-target> >> </destination-target> >> </custom-condition> >> >> The current YANG model does not allow setting both a firewall-condition >> and >> custom-condition. If that should be allowed, the model needs to change. >> Should >> it be possible to have multiple firewall- or other conditions? That is not >> currently possible. >> >> This example leaves out the mandatory leaf owner. Is that a sign that it >> should >> not be mandatory, or perhaps not exist at all? >> >> 23. Example in Fig.19 >> >> This example lists a firewall-condition with no src-target, which is >> mandatory. >> >> <firewall-condition> >> <destination-target> >> <dest-target>employees</dest-target> >> </destination-target> >> </firewall-condition> >> >> Under condition, there is a container rate-limit with a leaf >> packet-per-second. >> Is this a trigger value for the condition, or is it an actual limit that >> the >> system is expected to enforce? If it's a trigger, it may be good to find a >> clearer name. If it's enforced, it's placement under condition is >> deceiving. >> >> If a rule's action is set to 'rate-limit', to which rate will it be >> limited? >> >> 24. Security Considerations >> >> Section 10 in the NSF document under review is the Security >> Considerations. I >> think it would make sense to mention something about the management access >> control mechanism here, and its relation to NACM. >> >> (End of list) >> >> >> _______________________________________________ >> I2nsf mailing list >> [email protected] >> https://www.ietf.org/mailman/listinfo/i2nsf >> > > > -- > =========================== > Mr. Jaehoon (Paul) Jeong, Ph.D. > Associate Professor > Department of Software > Sungkyunkwan University > Office: +82-31-299-4957 > Email: [email protected], [email protected] > Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php > <http://cpslab.skku.edu/people-jaehoon-jeong.php> > -- =========================== Mr. Jaehoon (Paul) Jeong, Ph.D. Associate Professor Department of Software Sungkyunkwan University Office: +82-31-299-4957 Email: [email protected], [email protected] Personal Homepage: http://iotlab.skku.edu/people-jaehoon-jeong.php <http://cpslab.skku.edu/people-jaehoon-jeong.php>
_______________________________________________ I2nsf mailing list [email protected] https://www.ietf.org/mailman/listinfo/i2nsf
