MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
From: Prachi Jain via Datatracker <noreply@ietf.org>
To: <secdir@ietf.org>
Auto-Submitted: auto-generated
Precedence: bulk
Message-ID: 
 <178571286344.1803828.2635234331310539309@dt-datatracker-d4d6ff9d9-fsx7d>
Date: Sun, 02 Aug 2026 16:21:03 -0700
Message-ID-Hash: I5P6CLDL3XO34IS7GP243WUC54TTLU3K
CC: draft-ietf-rtgwg-qos-model.all@ietf.org, last-call@ietf.org,
 rtgwg@ietf.org
Reply-To: Prachi Jain <prachi.jain1288@gmail.com>
Subject: =?utf-8?q?=5Bsecdir=5D_draft-ietf-rtgwg-qos-model-15_ietf_last_call_Secdir_r?=
	=?utf-8?q?eview?=
Archived-At: 
 <https://mailarchive.ietf.org/arch/msg/secdir/Z6mCcxaA6H8l7q4493DwSWfg-Zk>

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

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.

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.

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

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

6.  Nothing stops a policy referencing itself.

      - This is the one thing in the YANG itself I'd hold the document for.
      - 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. - 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. - 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.

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.

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.

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.

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

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.

Happy to discuss further.

Thanks,