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

["Aseem Choudhary (asechoud)" <[e-mail]>]

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,