Subject: AD Review of draft-ietf-rtgwg-policy-model-27

From: Alvaro Retana <[email-masked]>
Date: Thu, 27 May 2021 12:34:16 -0700

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.

...
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.

...
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

...
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.

...
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.

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?

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.

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?

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

...
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

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

...
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.

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.

538	9.1.  Routing policy model
...
587	       "WG Web:   <http://tools.ietf.org/wg/rtgwg/>

[major] s/<...>/<https://datatracker.ietf.org/wg/rtgwg/>

588	        WG List:  <Email: [email-masked]>

[minor] s/Email:/mailto:/g

...
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.

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.

...
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?

...
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?

...
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

[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".

1020	       leaf mask-length-lower {
1021	         type uint8;

[minor] Shouldn't a range be defined here too?

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?

[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.

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.

...
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?

...
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?

...
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.

...
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?

...
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.

...
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:

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]