Hi Alvaro, Thank you for your thorough review, really appreciate.
We’ve published a new version to address your comments. Please see inline below for detailed answers. Thanks, Yingzhen > On May 27, 2021, at 12:34 PM, Alvaro Retana <[email protected]> wrote: > > Dear authors: > > Thank you for a well written document! > > I have some comments inline. I would like to at least see you address > the Security Considerations-related issues before starting the IETF > LC. > > Thanks! > > Alvaro. > > > [Line numbers from idnits.] > > ... > 54 Table of Contents > ... > 69 7. Security Considerations . . . . . . . . . . . . . . . . . . . 11 > 70 8. IANA Considerations . . . . . . . . . . . . . . . . . . . . . 12 > 71 9. YANG module . . . . . . . . . . . . . . . . . . . . . . . . . 12 > 72 9.1. Routing policy model . . . . . . . . . . . . . . . . . . 12 > > [minor] Please move the module to *before* the Security Considerations. > [Yingzhen]: moved the module, also added the complete tree. Here is a snapshot: 7. YANG Module and Tree . . . . . . . . . . . . . . . . . . . . 11 7.1. Routing Policy Model Tree . . . . . . . . . . . . . . . . 11 7.2. Routing policy model . . . . . . . . . . . . . . . . . . 12 8. Security Considerations . . . . . . . . . . . . . . . . . . . 33 9. IANA Considerations . . . . . . . . . . . . . . . . . . . . . 35 > > ... > 212 3. Model overview > ... > 220 o A structure that allows routing protocol models to add protocol- > 221 specific policy conditions and actions though YANG augmentations. > 222 There is a complete example of this for BGP [RFC4271] policies in > 223 the proposed vendor-neutral BGP data model > 224 [I-D.ietf-idr-bgp-model]. > > [minor] As far as I can tell, the augmentation in > I-D.ietf-idr-bgp-model is not an example -- it is the real > augmentation. Maybe you meant to write "the augmentation in > I-D.ietf-idr-bgp-model can be used as an example for other protocols". > > See also my comment below about Appendix A. > > [Yingzhen]: The augmentation in I-D.ietf-idr-bgp-model is a real augmentation of the policy model. We don’t want to create a circular dependency on BGP model, so Appendix A is really an example. We added some text for clarification. From section 3: o A structure that allows routing protocol models to add protocol- specific policy conditions and actions though YANG augmentations is also provided. There is a complete example of this for BGP [RFC4271] policies in the proposed vendor-neutral BGP data model [I-D.ietf-idr-bgp-model]. Appendix A provides an example of how an augmentation for BGP policies might be accomplished. Note that this section is not normative as the BGP model is still evolving. > ... > 259 4.1. Defined sets for policy matching > ... > 270 o neighbor sets - Each neighbor set define a set of neighboring > 271 nodes by their IP addresses. A neighbor set is used for selecting > 272 routes based on the neighbors advertising the routes. > > [nit] s/neighbor set define/neighbor set defines > > [Yingzhen]: fixed. > ... > 300 4.2. Policy conditions > > 302 Policy statements consist of a set of conditions and actions (either > 303 of which may be empty). Conditions are used to match route > 304 attributes against a defined set (e.g., a prefix set), or to compare > 305 attributes against a specific value. > > [minor] "conditions and actions (either of which may be empty)" > > This got me thinking, what is the default? If the action set is > empty, what should be the default: accept or deny? I latter found, > buried in §5 that the default is reject-route. It might be a good > idea to mention that here. > > [Yingzhen]: We added a default value to reject-route, also added text to clarify in section 4.2. leaf policy-result { type policy-result-type; default reject-route; description "Select the final disposition for the route, either accept or reject."; } > ... > 356 4.3. Policy actions > ... > 371 +--rw actions > ... > 381 +--rw set-preference? uint16 > > [?] What is the preference? Is this intended to be related to > "administrative distance", or local-pref, or what? Or maybe it is > something completely different. > [Yingzhen]: added description in the model, consistent with RFC 8349. leaf set-preference { type uint16; description "Set the preference for the route. It is also known as 'administrative distance', allows for selecting the preferred route among routes with the same destination prefix. A smaller value is more preferred."; } > > 382 +--rw set-tag? tag-type > 383 +--rw set-application-tag? tag-type > > [?] What is an application tag? What is the difference between that an a tag? > [Yingzhen]: added description. Hope this clarifies. leaf set-application-tag { type tag-type; description "Set the application tag for the route. The application-specific tag is an additional tag that can be used by applications that require semantics and/or policy different from that of the tag. For example, the tag is usually automatically advertised in OSPF AS-External Link State Advertisements (LSAs) while this application-specific tag is not advertised implicitly."; } > > 385 4.4. Policy subroutines > > 387 Policy 'subroutines' (or nested policies) are supported by allowing > 388 policy statement conditions to reference other policy definitions > 389 using the call-policy configuration. Called policies apply their > 390 conditions and actions before returning to the calling policy > 391 statement and resuming evaluation. The outcome of the called policy > 392 affects the evaluation of the calling policy. If the called policy > 393 results in an accept-route, then the subroutine returns an effective > 394 boolean true value to the calling policy. For the calling policy, > 395 this is equivalent to a condition statement evaluating to a true > 396 value and evaluation of the policy continues (see Section 5). Note > 397 that the called policy may also modify attributes of the route in its > 398 action statements. Similarly, a reject-route action returns false > 399 and the calling policy evaluation will be affected accordingly. When > 400 the end of the subroutine policy statements is reached, the default > 401 route disposition action is returned (i.e., boolean false for reject- > 402 route). Consequently, a subroutine cannot explicitly accept or > 403 reject a route. Rather it merely provides an indication that 'call- > 404 policy' condition returns boolean true or false indicating whether or > 405 not the condition matches. Route acceptance or rejection is solely > 406 determined by the top-level policy. > > [nit] That's a long and complex paragraph. Consider breaking it up -- > maybe along true and false results. > > > [] Just to make sure I understand: > > Let's assume I have a prefix-set and a neighbor-set, and I want to > accept the routes in the prefix-set when received from a neighbor-set. > > If I have a single policy definition (match-prefix-set and > match-neighbor-set under a single policy statement) then it should > work as I want it to, right? Assuming that I configure accept-route > as the policy-result. > > OTOH, if I create two policy definitions (one with match-prefix-set > and the other with match-neighbor-set), and no actions...and then use > call-policy to call both of them in order (prefix and neighbors), then > re result will always be a deny, even if I configure acce-route in the > top-level policy. If this correct? > > I hope that made sense. > > It would be great it you provide an example showing the two different > scenarios and their result while using the same prefix/neighbor-sets. > [Yingzhen]: We modified the text, hope it’s clear now. Regarding your example, your understanding is correct. Since you don’t have actions configured in the match-prefix-set and match-neighbor-set, and the default action is to reject-route, both these policies return False, so your top-level policy won’t match anything. > > 408 Note that the called policy may itself call other policies (subject > 409 to implementation limitations). The model does not prescribe a > 410 nesting depth because this varies among implementations. For > 411 example, an implementation may only support a single level of > 412 subroutine recursion. As with any routing policy construction, care > 413 must be taken with nested policies to ensure that the effective > 414 return value results in the intended behavior. Nested policies are a > 415 convenience in many routing policy constructions but creating > 416 policies nested beyond a small number of levels (e.g., 2-3) is > 417 discouraged. Also, implementations should have validation to ensure > 418 that there is no recursion amongst nested routing policies. > > [major] "should have validation" Why is this not a normative > recommendation? s/should/SHOULD > > Recursion doesn't sound like a good idea, ever! Why would the > validation only be recommended and not required? Why SHOULD and not > MUST? > [Yingzhen]: Changed to "implementations MUST validate to ensure that there is no recursion amongst nested routing policies.”. > > 420 5. Policy evaluation > > 422 Evaluation of each policy definition proceeds by evaluating its > 423 corresponding individual policy statements in order. When all the > 424 condition statements in a policy statement are satisfied, the > 425 corresponding action statements are executed. If the actions include > 426 either accept-route or reject-route actions, evaluation of the > 427 current policy definition stops, and no further policy statement is > 428 evaluated. If there are multiple policies in the policy chain, > 429 subsequent policies are not evaluated. Policy chains are sequences > 430 of policy definitions (described in . (Section 4)). > > [nit] s/described in . (Section 4)/as described in Section 4 > [Yingzhen]: fixed. > ... > 472 7. Security Considerations > ... > 487 There are a number of data nodes defined in this YANG module that are > 488 writable/creatable/deletable (i.e., config true, which is the > 489 default). These data nodes may be considered sensitive or vulnerable > 490 in some network environments. Write operations (e.g., edit-config) > 491 to these data nodes without proper protection can have a negative > 492 effect on network operations. These are the subtrees and data nodes > 493 and their sensitivity/vulnerability: > > 495 /routing-policy > > 497 /routing-policy/defined-sets/prefix-sets > > 499 /routing-policy/defined-sets/neighbor-sets > > 501 /routing-policy/defined-sets/tag-sets > > 503 /routing-policy/policy-definitions > > [major] It is expected that each data node include a description of > the sensitivity or vulnerability. > > https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines > [Yingzhen]: added required descriptions. Please see the updated version for details. > 505 Unauthorized access to any data node of these subtrees can disclose > 506 the operational state information of routing policies on this device. > > [major] This paragraph talks about access to the data, but a more > complete coverage is expected: > https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines > > [Yingzhen]: added descriptions. > ... > 531 9. YANG module > > [major] The trees shows before represent parts of the module. But > there's no place where the whole tree is shown -- as it usually is in > most YANG documents (at least it seems to me). Please include a full > tree somewhere. > > [Yingzhen]: added the complete tree. > 533 The routing policy model is described by the YANG modules in the > 534 sections below. [RFC2328], [RFC3101], [RFC5130], and [RFC5302] are > 535 referenced here since they are referenced in the YANG model but not > 536 elsewhere in this document. > > [minor] I think that the common practice is to list all the documents > reference inside the module, and not just the ones not referenced > elsewhere. > [Yingzhen]: fixed. There are still a couple fo references to be published as RFCs (e.g. sub-interface module), will need to modify this again later. > > 538 9.1. Routing policy model > ... > 587 "WG Web: <http://tools.ietf.org/wg/rtgwg/> > > [major] s/<...>/<https://datatracker.ietf.org/wg/rtgwg/> > [Yingzhen]: fixed > > 588 WG List: <Email: [email protected]> > > [minor] s/Email:/mailto:/g > [Yingzhen]: fixed. > ... > 599 description > 600 "This module describes a YANG model for routing policy > 601 configuration. It is a limited subset of all of the policy > 602 configuration parameters available in the variety of vendor > 603 implementations, but supports widely used constructs for > 604 managing how routes are imported, exported, modified and > 605 advertised across different routing protocol instances or > 606 within a single routing protocol instance. This module is > 607 intended to be used in conjunction with routing protocol > 608 configuration modules (e.g., BGP) defined in other models. > > [minor] Does the rest of this description need to be included inside > the module? It seems like overkill to me -- not a showstopper. I > didn't check if the text matches the rest of the document; if keeping > it, please do. > > > ... > 668 This version of this YANG module is part of RFC XXXX > 669 (https://www.rfc-editor.org/info/rfcXXXX); see the RFC itself > 670 for full legal notices. > > [minor] This paragraph is repeated below. Remove this one. > [Yingzhen]: fixed. > > 672 The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL', 'SHALL > 673 NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED', 'NOT > 674 RECOMMENDED', 'MAY', and 'OPTIONAL' in this document are to be > 675 interpreted as described in BCP 14 (RFC 2119) (RFC 8174) when, > 676 and only when, they appear in all capitals, as shown here. > > 678 This version of this YANG module is part of RFC XXXX; > 679 see the RFC itself for full legal notices."; > > > ... > 733 identity route-level { > 734 description > 735 "Base identity for route import or export level."; > 736 } > > [minor] This base identity is described for "import or export", but > the other identities (below) are only described in terms of > importation. Because route-level is only used as an action, then it > seems that the "export" part is not applicable. Am I interpreting > that correctly? If so, the set-route-level container also includes > "export" in the description. > [Yingzhen]: changed to import only. identity route-level { description "Base identity for route import level."; } > ... > 907 typedef default-policy-type { > 908 type enumeration { > 909 enum accept-route { > 910 description > 911 "Default policy to accept the route."; > 912 } > 913 enum reject-route { > 914 description > 915 "Default policy to reject the route."; > 916 } > 917 } > 918 description > 919 "Type used to specify route disposition in > 920 a policy chain. This typedef retained for > 921 name compatibility with default import and > 922 export policy."; > 923 } > > [?] "retained"? From where? > [Yingzhen]: fixed. description "Type used to specify route disposition in a policy chain. This typedef is used in the default import and export policy."; } > > ... > 980 typedef metric-modification-type { > 981 type enumeration { > 982 enum set-metric { > 983 description > 984 "Set the metric to the specified value."; > 985 } > 986 enum add-metric { > 987 description > 988 "Add the specified value to the existing metric. > 989 If the result would overflow the maximum metric > 990 (0xffffffff), set the metric to the maximum."; > > [major] Not all metrics have the same length! How should shorter > metric fields be handled? Is that expected to be solved by an > augmentation, if/when needed? > [Yingzhen]: if a metric is shorter, say only 0xff, and a value is set out of range, the implementation should reject it. It’s set to maximum here for simplicity. > ... > 1009 leaf ip-prefix { > 1010 type inet:ip-prefix; > 1011 mandatory true; > 1012 description > 1013 "The IP prefix represented as an IPv6 or IPv4 network > 1014 number followed by a prefix length with an intervening > 1015 slash character as a delimiter. All members of the prefix > 1016 set MUST be of the same address family as the prefix-set > 1017 mode."; > 1018 } > > [nit] s/prefix set/prefix-set > [Yingzhen]: fixed. > [major] "MUST be of the same address family" This normative text > should be placed in the prefix-sets container because that is where > the prefix-sets are defined. > > Note that the text there is as follows: > > leaf mode { > ... > description > "Indicates the mode of the prefix set, in terms of > which address families (IPv4, IPv6, or both) are > present. The mode provides a hint, but the device > MUST validate that all prefixes are of the indicated > type, and is expected to reject the configuration if > there is a discrepancy."; > > The normative requirements are not the same: "MUST be" vs "MUST > validate". The "expected to reject" action is more akin to "MUST be”. > > [Yingzhen]: I modified description of “leaf mode” to match the description of “ip-prefix”. description "Indicates the mode of the prefix set, in terms of which address families (IPv4, IPv6, or both) are present. The mode provides a hint, all prefixes MUST be of the indicated type. The device MUST validate that all prefixes and reject the configuration if there is a discrepancy."; } > 1020 leaf mask-length-lower { > 1021 type uint8; > > [minor] Shouldn't a range be defined here too? > [Yingzhen]: added a range > > 1022 description > 1023 "Mask length range lower bound. It MUST NOT be less than > 1024 the prefix length defined in ip-prefix."; > 1025 } > 1026 leaf mask-length-upper { > 1027 type uint8 { > 1028 range "1..128"; > 1029 } > 1030 must "../mask-length-upper >= ../mask-length-lower" { > 1031 error-message "The upper bound MUST NOT be less" > 1032 + "than lower bound."; > 1033 } > > [minor] An error message is printed if the mask-length-upper doesn't > meet the expectation. But no error related to mask-length-lower. > Why? > [Yingzhen]: the prefix length defined as part of ip-prefix, so it’s not so easy to define a “must” statement. It can be done in implementation after parsing ip-prefix. > > [major] The text in the error message contains a normative requirement > that is not specified. IOW, "upper bound MUST NOT be less" just > appears in the error message and not as part of the specification: add > it to the description field. [Yingzhen]: added in description. leaf mask-length-upper { type uint8 { range "1..128"; } must "../mask-length-upper >= ../mask-length-lower" { error-message "The upper bound MUST NOT be less" + "than lower bound."; } description "Mask length range upper bound. It MUST NOT be less than lower bound."; } > > > 1034 description > 1035 "Mask length range upper bound. > > 1037 The combination of mask-length-lower and mask-length-upper > 1038 define a range for the mask length, or single 'exact' > 1039 length if mask-length-lower and mask-length-upper are > 1040 equal. > > 1042 Example: 192.0.2.0/24 through 192.0.2.0/26 would be > 1043 expressed as prefix: 192.0.2.0/24, > 1044 mask-length-lower=24, > 1045 mask-length-upper=26 > > 1047 Example: 192.0.2.0/24 (an exact match) would be > 1048 expressed as prefix: 192.0.2.0/24, > 1049 mask-length-lower=24, > 1050 mask-length-upper=24"; > 1051 } > > [minor] Please move the explanation about the combination and the > example to the description field for the prefix grouping. > > > [] Please also add an IPv6 example. > [Yingzhen]: moved the description and added an IPv6 example. > ... > 1067 grouping match-set-options-restricted-group { > 1068 description > 1069 "Grouping for a restricted set of match operation > 1070 modifiers."; > > 1072 leaf match-set-options { > 1073 type match-set-options-type { > 1074 enum any { > 1075 description > 1076 "Match is true if given value matches any > 1077 member of the defined set."; > 1078 } > 1079 enum invert { > 1080 description > 1081 "Match is true if given value does not match > 1082 any member of the defined set."; > 1083 } > 1084 } > 1085 description > 1086 "Optional parameter that governs the behavior of the > 1087 match operation. This leaf only supports matching on > 1088 'any' member of the set or 'invert' the match. > 1089 Matching on 'all' is not supported."; > 1090 } > > [minor] "Matching on 'all' is not supported." > > I was wondering about this and noticed that > match-set-options-restricted-group is used by match-prefix-set and > match-tag-set. I can see how a prefix would probably never match > 'all', but why not offer that option for tags? > > BTW, also realized (from §4.2) that prefix-set and tag-set are the > only places where match-set-options is applied. If neither uses 'all' > then why even define it? > [Yingzhen]: I changed “match-tag-set” to use “match-set-options-group”, so it could use “all” option. grouping tag-set-condition { description "This grouping provides tag-set conditions."; container match-tag-set { leaf tag-set { type leafref { path "../../../../../../../defined-sets/tag-sets" + "/tag-set/name"; require-instance true; } description "References a defined tag set."; } uses match-set-options-group; <--- description "Match a referenced tag set according to the logic defined in the match-options-set leaf."; } } > > ... > 1097 container match-interface { > 1098 leaf interface { > 1099 type leafref { > 1100 path "/if:interfaces/if:interface/if:name"; > 1101 } > 1102 description > 1103 "Reference to a base interface. If a reference to a > 1104 subinterface is required, this leaf MUST be specified > 1105 to indicate the base interface."; > 1106 } > 1107 leaf subinterface { > 1108 type leafref { > 1109 path "/if:interfaces/if:interface/if-ext:encapsulation" > 1110 + "/if-flex:flexible/if-flex:match" > 1111 + "/if-flex:dot1q-vlan-tagged" > 1112 + "/if-flex:outer-tag/if-flex:vlan-id"; > 1113 } > 1114 description > 1115 "Reference to a subinterface -- this requires the base > 1116 interface to be specified using the interface leaf in > 1117 this container. If only a reference to a base interface > 1118 is required, this leaf SHOULD NOT be set."; > > [major] "SHOULD NOT be set" If set, what should it be set to? When > is it ok to set it? Why is this action recommended and not required? > [Yingzhen]: changed to “MUST NOT”. description "Reference to a subinterface -- this requires the base interface to be specified using the interface leaf in this container. If only a reference to a base interface is required, this leaf MUST NOT be set."; > > ... > 1147 container match-prefix-set { > 1148 leaf prefix-set { > 1149 type leafref { > 1150 path "../../../../../../../defined-sets/" + > 1151 "prefix-sets/prefix-set/name"; > 1152 } > 1153 description > 1154 "References a defined prefix set."; > 1155 } > 1156 uses match-set-options-restricted-group; > > 1158 description > 1159 "Match a referenced prefix-set according to the logic > 1160 defined in the match-set-options leaf."; > 1161 } > 1162 } > > 1164 grouping neighbor-set-condition { > 1165 description > 1166 "This grouping provides neighbor-set conditions."; > > 1168 container match-neighbor-set { > 1169 leaf neighbor-set { > 1170 type leafref { > 1171 path "../../../../../../../defined-sets/neighbor-sets/" + > 1172 "neighbor-set/name"; > 1173 require-instance true; > > [nit] Is require-instance needed? The default is true, so... This > caught my attention because it shows up here, but not under > match-prefix-set (above) for example. > [Yingzhen]: This means the neighbor has to be there. But for prefix, it’s possible to define prefix-set without those prefixes really configured. > ... > 1351 container neighbor-sets { > 1352 description > 1353 "Data definition for a list of IPv4 or IPv6 > 1354 neighbors which can be matched in a routing policy."; > > [minor] Can this set contain both IPv4/IPv6 neighbors? > > [Yingzhen]: Type of “address” is “ip-address”, it can be either IPv4 or IPv6. Since it’s a leaf-list, it can contain both IPv4 and IPv6 neighbors. > ... > 1681 Appendix A. Routing protocol-specific policies > > [minor] Please add a reference to this appendix from the main body text. > > > ... > 1689 The example below provides an illustration of how another data model > 1690 can augment parts of this routing policy data model. It uses > 1691 specific examples from draft-ietf-idr-bgp-model-09 to show in a > 1692 concrete manner how the different pieces fit together. This example > 1693 is not normative with respect to [I-D.ietf-idr-bgp-model]. The model > 1694 similarly augments BGP-specific conditions and actions in the > 1695 corresponding sections of the routing policy model. In the example > 1696 below, the XPath prefix "bp:" specifies import from the ietf-bgp- > 1697 policy sub-module and the XPath prefix "bt:" specifies import from > 1698 the ietf-bgp-types sub-module [I-D.ietf-idr-bgp-model]. > > [] If I understand correctly, the tree below was built from the > information in I-D.ietf-idr-bgp-model. To avoid potentially running > out of sync (i.e. the bgp module deciding to do something else), would > it be easier to just point people at that draft, and eliminate this > appendix? Just thinking out loud. > > Even though I-D.ietf-idr-bgp-model is not a Normative reference, we > can also hold publication until both are ready. > [Yingzhen]: clarified above. > ... > 1807 Appendix B. Policy examples > > [minor] Please add a reference to this appendix from the main body text. > > 1809 Below we show an example of XML-encoded configuration data using the > 1810 routing policy and BGP models to illustrate both how policies are > 1811 defined, and also how they can be applied. Note that the XML has > 1812 been simplified for readability. > > [minor] Please describe what the example is about before presenting it: [Yingzhen]: added some descriptions, also added IPv6 example. > > 1814 <config xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> > 1815 <routing-policy > 1816 xmlns="urn:ietf:params:xml:ns:yang:ietf-routing-policy"> > ... > > [EoR -27] _______________________________________________ rtgwg mailing list [email protected] https://www.ietf.org/mailman/listinfo/rtgwg
