[radext] (belated) AD review of draft-ietf-radext-coa-proxy-04

[Benjamin Kaduk <[e-mail]>]

Apologies for the long delay on this one; I had written up an AD review a
month or two ago but it got lost in the ether.  I've tried to reconstruct
my comments based on notes I made on a paper printout, so there will be a
few places where I imply that I changed my mind since the initial review.

I have a couple of concerns about this document that I think should be
resolved before I move it into IETF Last Call:

This document lists RFC 5176 and RFC 2866 as informative references,
possibly so as to avoid the awkwardness of a standards-track document
making a normative downreference to informational documents.  I think that
at least RFC 5176 should be a normative reference, since it really must be
understood in order to implement this document -- after all, we even Update
it!  Looking back again (for the third time, actually), it's less clear
that the RFC 2866 reference is actually as problematic as it seemed on
first read; as there is only a single reference and it is just explaining
why this document has a MUST-level requirement.  I don't think there's an
actual issue with making 5176 a normative reference in terms of process; we
just have to call out the "downref" during IETF Last Call and then it goes
on the list of approved downrefs.

I also think there needs to be some more clarity in the Requirements on
Visited Networks (Section 4.2), in several ways:

* First off, it refers to "all the checks discussed above for proxies",
  which presumatly is an artifact of subsection reordering.  Even if it is
  changed to "below", though, I'm still unclear on exactly which checks are
  implied -- is this just the filtering from Section 4.3.2?

* Furthermore, it's unclear what it actually means to have a MUST-level
  requirement to do some checks that are only SHOULD-level in Section
  4.3.2.

* I'm also a little fuzzy on where thse requirements apply, as a "Visited
  Network" is kind of a fuzzy concept, and is going to potentially involve
  things like a network perimeter/firewall, some form of proxies, RADIUS
  servers, etc. -- should we say something more specific about where the
  actions to meet these requirements will be taken?

* I don't think I followed how assuming there's a proxy between the NAS and
  any external proxy implied a requirement for the Visited Network to
  perform "all of the checks".  (Maybe if what the checks were was
  clarified this concern would go away, though.)

* The second paragraph leaves me generally confused, mostly in that it is
  talking about somethinf "phrased more generically in Section 4.3.2",
  which is my best guess for the thing that the first paragraph is trying
  to require.  So is this whole paragraph redundant?

* Why is the cryptographically-strong requirement for Operator-NAS-Identity
  only SHOULD and not MUST?

* Is there a conflict between the "without requiring to track ... in a
  database" in the third paragraph, and the text for the Value field in the
  Operator-NAS-Identifier definition, that says "either track these tokens
  in a database, or [...] create tokens which can be decoded"?


I also think this document should probably switch over to the RFC 8174
boilerplate instead of the RFC 2119 boilerplate, if only to save all of the
IESG and the genart reviewer from having to comment about it.

Somewhat editorial, but with great potential for confusion: in Section 3.3,
please insert text like "The new Operator-NAS-Identifier attribute is
defined as follows" before the Description/Type/etc.

Also in Section 3.3, I wondered if the "in practice" clarification for
using a database or self-encrypted tokens was intentionally excluding the
possibility of always using the same value for the token (e.g., if there is
only one NAS in the Visited Network, which is admittedly unlikely in
nontrivial deployments).


Some editorial comments follow.

As a general comment, the RFC Editor generally likes to distinguish between
"which" (inate attributes that apply to all elements of a class) and "that"
(attributes that only apply to a subset of the class, and thereby
specifying a new restriction on what is being discussed).  I don't really
have a horse in the race, but if you want to avoid getting asked about it
later, you could make a pass over the document looking for them.  There's a
couple spots in Section 4.3.1 that would probably change from "which" to
"that", if you do.

In Section 1, I think the second paragraph should start with "We partially
correct" given the limitation made in the third paragraph.  I might also
reword the third paragraph to something like "This correction is limited to
the use case of CoA proxying Realm-basec proxying [...]", as it's not
exactly the use case that's being limited as much as the applicability of
this document.  (It looks like I had originally suggested reordering the
second and third paragraphs, but that doesn't make sense to me now.)

In Section 2.1, fourth paragraph, it may be useful to clarify whether "The
RADIUS client" applies only to the endpoint (non-proxy) client or to the
client portion of all entities participating in a proxy chain along the
path.

In Section 2.3, "there may be a different CoA server which could accept the
packet", I assume that "accept the packet" is supposed to imply "and do
something useful with it"; perhaps this could be reworded to something like
"successfully process the packet".  Additionally, it doesn't seem accurate
to say that 5176 "is silent on the topic of 'session identification
attributes'", since that phrase appears ten times; it seems like we are
saying that it doesn't actually tell you how to use them or what they are,
but just says that they can exist (kind of like the situation for CA
proxying itself).

The pedant in me takes issue with "impossible" in "impossible to send CoA
packets to the Visited Network", in Section 3.1 -- I assume that there are
other contrived and inefficient potential solutions.

s/one ore more/one or more/ in Section 3.2

In Section 3.3, third paragraph, the references to 2865 and 2866 are both
Section 4.1; we should probably say both "Section 4.1 of [2865], and of
Section 4.1 of [2866].  It also seems that a reference to Section 4.1 (what
a coincidence!) of RFC 5580 might be in order, to note the parallelism
between Operator-NAS-Identifier and Operator-Name.

In Section 4.2, third paragraph, presumably the non-requirement for
tracking in a database "every individual value" applies only to those
values that are actually issued.  (That is, I'm asking to add "issued," in
"every value of Operator-NAS-Identifier issued, in a database".)

In Section 4.3.2, do we want to "specifically note" or "call out" instead
of just "note" that the above requirements apply to future attributes added
by a proxy (as well as the ones defined here)?  (This is something of a
style question, so it's up to you.)

Section 5.1, s/xwill/will/

In the last paragraph of Section 5.1, do we need to say that the recording
is done "if present"?

Section 5.2, s/disconnecte/disconnected/; s/recieve/receive/;
s/fromt he/from the/

Also in Section 5.2, do we want to add "next-hop" when talking about
finding "the CoA server for that realm"?  And, in the last paragraph, we
say "Any response from the CoA server [...]", but isn't the CoA server
required to send such a response?

-Ben