[secdir] Re: draft-ietf-rtgwg-qos-model-15 ietf last call Secdir review

[Prachi Jain <[e-mail]>]

Thank you. That looks good.

On Mon, Aug 10, 2026 at 2:33 AM Aseem Choudhary (asechoud) <
[e-mail]> wrote:

> Hi Prachi,
>
> Thanks for the detailed review comments. Please see my response inline
> starting with [AC]
> We plan to address all Must-fix items and smaller points 7–11 in draft -16
> .
>
> Regards,
> Aseem
>
> *From: *Prachi Jain via Datatracker <[e-mail]>
> *Date: *Sunday, August 2, 2026 at 4:21 PM
> *To: *[e-mail] <[e-mail]>
> *Cc: *[e-mail] <
> [e-mail]>; [e-mail] <
> [e-mail]>; [e-mail] <[e-mail]>
> *Subject: *draft-ietf-rtgwg-qos-model-15 ietf last call Secdir review
>
> Document: draft-ietf-rtgwg-qos-model
> Title: YANG Models for Quality of Service (QoS) in IP networks
> Reviewer: Prachi Jain
> Review result: Not Ready
>
> Hello,
>
> I've finished my review. I'm not a routing person, so I've looked at this
> purely from the security side, apologies in advance if I've misread
> anything
> domain-specific.
>
> Few things I think need fixing, then a handful of smaller notes.
>
> Must fix
>
> 1.  Section 6 names nodes that don't exist.
>
>       - Three of them. voilate-pkts and voilate-bytes are just typos - the
>       modules spell them violate-pkts and violate-bytes.
> [AC]:  Section 6 will be corrected to violate-pkts and violate-bytes,
> matching ietf-qos-oper.
>
> - red-drop-bytes
>       doesn't appear anywhere in the document at all; I'm assuming it means
>       red-statistics/drop-bytes, though it could equally be
>       red-statistics/wred-stats/drop-bytes. Could the authors confirm
> which?
> [AC]: I will change red-drop-bytes => red-statistics, since it describes
> both
>        drop or marked for ECN
>
>
>    - The third one isn't a typo. Section 6 refers to "the clear RPC
>
>       operation", but clear is a YANG action, and there's no rpc statement
>       anywhere in the seven modules. This matters for more than tidiness:
> under
>       RFC 8341 an action is access-controlled through the data node it
> hangs
>       off, while an RPC is controlled by an rpc-name rule. Someone
> following
>       Section 6 would write the wrong kind of NACM rule.
> [AC]: Agree, ‘clear’ is a YANG action. I will replace ‘clear RPC
> operation’ with ‘clear action’
>
> - While I'm here, the
>       node names are given without paths. drop-pkts and drop-bytes each
> turn up
>       in four different places in ietf-qos-oper, so there's no way to tell
>       which is meant.
> [AC]: I would add the context to avoid any ambiguity.
>
> 2.  RFC 8341 (NACM) should be a normative reference.
>       - It's currently in Section 9.2 with the informative ones. The
> template
>       is explicit that it has to be normative, and given NACM is the only
>       access control mechanism Section 6 leans on, that seems right. -
> Worth
>       adding RFC 9907 too -  it's the current YANG guidelines RFC and it
> isn't
>       cited anywhere.
> [AC]:   Agree, I will move to Section 9.1 (Normative References), and
> Section 6
> will state that access control for configuration, operational data, and
> actions is
> expected to be enforced using NACM per RFC 8341.
>
>  [RFC9907] will be added as a normative reference and cited in Section 6
>
>  as the basis for restructuring the security considerations text.
>
> 3.  The writable-node list leaves out the disruptive ones.
>       - The template requires writable nodes that could be especially
>       disruptive to be listed by name. Section 6 lists three fairly generic
>       ones - filter-operation, filter and action - and skips the ones I'd
>       actually worry about: - qos-target-policy is what binds a policy to
> an
>       interface, so removing an entry removes the traffic treatment from
> that
>       interface and direction. That's the biggest one and it isn't there at
>       all. - discard, which is an action-type identity that does what it
> says.
>       - dscp-mark and dscp-marking - RFC 2475 Section 6 already describes
> DSCP
>       remarking as a theft-of-service vector. - The meter rates
>       (committed-information-rate, peak-information-rate, and the three
> burst
>       sizes), which decide what traffic gets treated as conforming versus
>       violating.
> [AC]:I will expand the writable-node analysis beyond the generic
> classifier/action entries,
> and add 'qos-target-policy', ‘discard’, ‘dscp-mark’, ‘meter-rates' as high
> impact writable.
>
>
> 4.  The readable-node list is incomplete too.
>       - Section 6 covers the metering counters and the drop/ECN ones, but
> not
>       classified-pkts, classified-bytes or classified-rate. Those strike
> me as
>       more sensitive than the ones that are listed, because they give away
>       policy structure rather than volume - someone who can read them and
> also
>       inject traffic can work out which prefixes, ports and DSCP values the
>       operator matches on, without ever seeing the config.
>
> [AC]: We will add 'classified-pkts', 'classified-bytes', and '
> classified-rate' under the
>
>      classifier statistics paths, and note that they can reveal which
> classifiers/policies are
>
>      matching traffic and thus infer policy structure when combined with
> injected probe traffic.
>
>  - Also missing: the
>       queue occupancy leaves (queue-current-size-bytes and friends), which
> are
>       a live readout of link congestion, and policy-name, which is an
>       unconstrained string, so the model can't really assume it's free of
>       customer-identifying information.
>
> [AC]: We will add:
>
>    - queue-current-size-bytes
>    - queue-average-size-bytes
>    - queue-peak-size-bytes
>
> under .../stats-per-direction/queueing/, noting they expose live
> congestion state
>
> As well as, we will add 'policy-name' under stats-per-direction and note
> that unconstrained
>
> string values may contain operator- or customer-identifying information
> and should be
>
> read-protected accordingly.
>
> 5.  Section 6 needs rebuilding from the current template.
>       - Using the current template is mandatory under RFC 9907 Section
> 3.7, and
>       this one predates the 2025 rewrite.
>
>  [AC]. Section 6 will be regenerated from the current RFC 9907 YANG
> security
>
>  considerations template rather than incrementally patched.
>
>  - Four gaps noticed:
>           - The mutual authentication requirement is missing - the template
>           asks for secure transport and mutual authentication, and Section
> 6
>           only has the first. - ietf-qos-types needs its own paragraph,
> since
>           it defines only identities, typedefs and features and the
> template
>           has specific text for that case. - The reused-groupings
> paragraph is
>           absent, which matters because ietf-traffic-policy and
> ietf-qos-oper
>           both augment /if:interfaces/if:interface, so RFC 8343's security
>           considerations should be pointed at. - Section 6 says "The YANG
>           module specified in this document" when Section 5 registers
> seven.
>       - Easiest fix is probably to regenerate the whole section from the
>       template on the IETF wiki and then re-add the module-specific parts.
>
> [AC]:  We will add explicit text that management access SHOULD use
>
>        secure transport with mutual authentication (e.g., TLS client
>
>        authentication or SSH key-based client authentication) before
>
>        configuration or operational data is exposed.
>
>        We will add a dedicated ietf-qos-types paragraph using
>
>        the template language for types-only modules (no config/state
>
>       data nodes; identities/features affect which nodes appear in
>
>       other modules).
>
>     Opening text will read “The YANG modules specified in this document…”
>
>       and Section 6 will include per-module subsections or paragraphs
>
>      for all seven modules.
>
> 6.  Nothing stops a policy referencing itself.
>       - This is the one thing in the YANG itself I'd hold the document for.
>
> [AC]: Agree. We agree this is a modeling defect and will fix it in -16.
>
>       - The child-policy grouping has a single leaf name of type string -
> not a
>       leafref, and with no must or when on it. So it can name a policy that
>       doesn't exist, and the document doesn't say what a server should do
> about
>       that
>
> [AC]: Agree. We will:
>
>    1. Change child-policy/name from string to leafref pointing to
>    /traffic-policy:policies/policy/name
>    2. Add prose that a reference to a non-existent policy MUST be
>    rejected at validation/commit time (consistent with other leafref
>    usage in the model)
>
> - More concerning, it can form a cycle: A's child names B, B's
>       child names A. That satisfies every constraint in the document, and
> since
>       nothing prohibits it or says how to handle it, an implementation
>       resolving the chain without a depth limit wouldn't terminate.
>
> [AC]: Agree. We will add normative text that implementations MUST reject:
>
>    - direct self-reference (child names containing policy), and
>    - cyclic child-policy chains
>
>  - I suspect this is just an oversight, since the same document uses
> leafref for
>       meter-reference and queue-policy-name. Suggest making it a leafref,
>       adding a constraint against self-reference, and saying explicitly
> that
>       servers must reject cyclic hierarchies.
> [AC]: Agree exactly as suggested. leafref + anti-self-reference +
> anti-cycle rejection
>  will be added; behavior will be documented in the module description
> and/or
> Section 3/4 prose and reflected in Security Considerations.
>
> Smaller points
> 7.  The clear action has no NACM protection. RFC 8341's exec-default is
> permit,
> and nothing here overrides it - ietf-netconf-acm isn't imported by any of
> the
> seven modules. So with NACM on but no rule written for this action, any
> authenticated user can invoke it, including one with no write access.
> Adding
> nacm:default-deny-all is a recommendation rather than a hard requirement,
> but
> describing the sensitivity isn't.
> [AC]: Agree, we will modify:
>
>    - Section 6 will describe clear as sensitive (resets operational
>    counters;
>    can hide evidence of misbehavior or disrupt monitoring).
>    - Section 6 will note that with default NACM exec-default = permit,
>    operators
>    MUST configure explicit invoke rules unless they intend all
>    authenticated
>    users to clear counters.
>    - In ietf-qos-oper, we will 'import ietf-netconf-acm' and add
>    'nacm:default-deny-all' on the clear action (recommended
>    hardening per RFC 8341).
>
>
> 8.  Rate values aren't constrained against their unit. value is an
> unbounded
> uint64 and unit can be percent, with nothing tying them together, so value:
> 4000, unit: percent validates fine. Same story for burst-value, and neither
> leaf is mandatory. A must capping it at 0-100 when the unit is percent
> would
> cover it.
>
> [AC]: Agree. In rate-value-unit we will add:
>
> must "not(../unit = 'qos-types:percent') or (. <= 100)”;
>
> We will review burst-value / burst-unit for the same pattern where
> percentage
>
> units apply, and clarify in descriptions when leaves are required for a
> valid
>
>  configuration.
> 9.  The scheduled clear is underspecified. started-at takes a future
> timestamp,
> but there's no bound on how far ahead, no way to list or cancel something
> pending, no notification when it fires, and no stated behaviour for a
> timestamp
> in the past.
>
> [AC]: Agree. We will add implementation guidance in the clear action
> description and Section 4.4:
>
> Section 6 will note that scheduled clear can affect monitoring/forensics
> timing and
>
> should be invoke-restricted like immediate clear.
> 10. RFC 2475 Section 6 isn't referenced. Its security section already
> covers
> theft of service via DSCP remarking, denial of service through marking,
> and the
> domain trust boundary - all of which this document configures. Right now
> Section 6's only line on the subject is that the action "decides what
> action
> will be taken on the packet."
> [AC]: Agree. We will:
>
>    - Add [RFC2475] to references (informative, if not already cited
>    elsewhere)
>    - Expand Section 6 to reference RFC 2475 Section 6 for
>    theft-of-service via
>    remarking, DoS via marking, and DiffServ domain trust-boundary
>    considerations
>    - Replace the generic “action decides what will be taken on the
>    packet” text with
>    specific treatment of marking, metering, discard, and policy binding
>    risks
>
>
> 11. Configuration is readable as well as writable. The ietf-diffserv
> filters
> hold source and destination prefixes, port ranges, protocol numbers and
> DSCP
> values. The template is explicit that the readable-nodes analysis covers
> config
> nodes too, since they can be read via get-config, but Section 6 only looks
> at
> counters.
>
> [AC]: Agree. Section 6 readable-node analysis will explicitly include
> configuration
>
>  data readable via <get-config> / RESTCONF, including at minimum:
>
>    - /classifiers/classifier/filter/* (all diffserv filter parameters)
>    - /policies/policy/classifier/inline/filter/*
>    - /policies/policy/classifier/action/*
>    - /interfaces/interface/qos-target-policy
>    - /meters/meter/*, /queues/queue/*
>
>
>
> Happy to discuss further.
> Thanks,
>
>
>