Re: [radext] Gen-ART review of draft-ietf-radext-dynamic-discovery-13

[Stefan Winter <[e-mail]>]

Hello,

thanks for those kind words :-)

> Minor issues:
> 
> S-NAPTR tags for RADIUS are defined, but none for Diameter.  I'm sure
> this was considered, but it seems odd on first reading.

Diameter defines its own S-NAPTR tags in its own RFCs - see the IANA
registry at
https://www.iana.org/assignments/s-naptr-parameters/s-naptr-parameters.xhtml#s-naptr-parameters-2
for a list.

The authors if the current RADIUS draft were aware of that and have
defined the radius tags exactly analogous to the Diameter ones (e.g.
diameter.tls.tcp matches radius.tls.tcp).

> S2.1.1.2 uses SHOULD to recommend that clients retry with their other
> certificates.  I'd recommend use of MAY here, unless you would like to
> expand on why this a SHOULD is valuable.

Well... realm discovery is triggered typically by an end user trying to
authenticate. If a client has a certificate but doesn't want to show it,
this becomes an avoidable DoS for the user. Also, if the client doesn't
always try the certificates in the same order, the connection setup also
becomes indeterministic - sometimes it works, sometimes not.

The clause makes a case for "don't give up prematurely" and makes the
behaviour deterministic.

So, I believe a SHOULD is the right thing to ask for here - would you
prefer an updated draft with this reasoning included, or are the
explanations in this mail sufficient?

> S2.1.1.3.3. I know that this is experimental and all, but this seems a
> little too hand-wavy even for that.  I think that this is the right
> design, but the section could be a little more confident (and precise)
> in the description of how this works.  It took me a couple of goes to
> understand how this was intended to work.  One important realization
> was that a common root for the realm needs to be configured.  I'd like
> to see this section expanded slightly (my initial inclination would
> have been toward removal, but the content is in line with the
> experiment, so it's probably valuable).

Oh, it's totally handwavy, agreed :-) The mechanism described here is
not implemented anywhere, and DANE in general only deals with the
server-side auth, leaving a gaping hole on the question how to validate
client certs. So, it really is just a sketch - that's why I used that
word in the text.

Actually I find that approach rather elegant, but it would take people
with a lot of energy to pick it up, complete the DANE specs for
client-side auth, implement TLS libraries that can do this, and to
actually deploy it.

I'm not sure I know how to handle your comment text-wise... when it
comes to the need of a common root, I would have hoped that there is
already text making it clear, i.e.:

"if
   DANE/TLSA records of all authorized servers are put into a DNSSEC
   zone with a common, consortium-specific branch of the DNS tree, then
   the entity executing the algorithm can retrieve TLSA Resource Records
   (TLSA RR) for the label "realm.commonroot" [...]"

If you can suggest specific text that helps readers understand this
easier, I'm all ears...

> S2.2 says:
>    Since the Domain Name System is not
>    necessarily trustworthy (e.g. if DNSSEC is not deployed for the
>    queried domain name), it is important to verify that the server which
>    was contacted is authorized to service requests for the user which
>    triggered the discovery process.
> 
> This implies that DNS integrity is the only reason for authentication.
> To paraphrase Steve Bellovin: you hand your packets to the attacker to
> deliver.

Hmm... that text indeed is wrong / comes across badly.

Actually, even if DNS is trustworthy, the authorisation needs to be
checked. It's more like:

(new)
"Regardless whether the results from DNS discovery are trustworthy or
not (e.g. DNSSEC in use), it is always important to verify that the
server which was contacted is authorized to service requests for the
user which triggered the discovery process."

I've updated my working copy with that change; waiting for AD go-ahead
to publish a -14 eventually.

(What I planned to say there was that if DNS without SEC is used, not
only the authorisation is in question and needs to be checked, but also
the discovered IP/port; but that's not totally important in itself
because the authorisation check comes unconditionally and covers both
anyway)

> Also:
>    It MAY substitute labels on the leftmost dot-
>    separated part of the NAI with the single character "*" to indicate a
>    wildcard match for "all labels in this part".
> Use the singular form here to avoid confusion:
>   It MAY substitute the leftmost dot-
>    separated label of the NAI with the single character "*" to indicate a
>    wildcard match for "all labels in this part".

Thanks, changed in my working copy.

> S3.4.1 is the first mention of UTF-8 realms, which are not permitted
> by RFC 4282.  This was a point of confusion for me.  I note that the
> new nai draft permits UTF-8 realms.  Please just cite that and remove
> the reference to 4282.

Right, currently the draft mentions both (see Terminology), but
meanwhile the publication of the new NAI document is imminent, so 4282
can go away. I've purged it from my working copy.

> S 3.4.3 doesn't cover how to handle NAPTR records with flags set to
> "a", though other parts of the document describe that.  I think there
> is some refactoring of the dependency on the S-NAPTR algorithm, and an
> allowance for a default port.

That's already done in step 9, which basically hands off further
resolution to S-NAPTR by stating: "For the extracted NAPTRs, perform
successive resolution as defined in [RFC3958], section 2.2."

If anything, poiting to section 2.2 is maybe too specific, because many
parts of RFC3958 are about "s" and "a" NAPTR entries; do you think it
makes the document more readable if I remove the "section 2.2" ?

> Nits/editorial comments:
> 
> S1.3 Capital 'P' on first bullet

Changed to lower case.

> S2.1.1.1 Missing period on first note.

Fixed, thanks.

> S2.1.1.2 Please provide a reference for "Effective TTL".

Effective TTL is defined in section 3.3 ("Terms"); I've added a
forward-reference.

> S2.1.1.3 s/PKS/PSK/ and "used for in the subsequent"

Fixed and changed to just "in" (deleted "for")

> (S2.1.3 Please be consistent with the trailing period in DNS record entries)

Added plenty of dots.

> 2.1.3.c. "x-" ugh...RFC 6648

I believe eduroam's decision to make use of x-eduroam significantly
predates RFC6648. In any case, that's how they use it; the draft just
documents reality, however frowned-upon that reality may be :-)

> S.3.4.4 "If these
>    TTLs are very low, thrashing of connections becomes possible; the
>    Effective TTL mitigates that risk." - isn't this the MIN_EFF_TTL instead?

Well, both - Effective TTL has MIN_EFF_TTL as one of its parameters. In
a way, having MIN_EFF_TTL somewhere in the calculation prevents the
thrashing; if looked at from a different angle, only Effective TTLs wise
inclusion of that MIN_EFF_TTL in its calculations is the neuralgic spot
that actually prevents it.

So, I don't care. Do you insist on a text change?

> S3.4.4 the last MAY here can be a SHOULD, I think

Sounds reasonable. Changed.

> S5 "the result O can not be trusted" -> "the result of the discovery
> process can not be trusted"

Changed.

Thanks much for the review!

Greetings,

Stefan Winter


-- 
Stefan WINTER
Ingenieur de Recherche
Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et
de la Recherche
6, rue Richard Coudenhove-Kalergi
L-1359 Luxembourg

Tel: [phone]
Fax: [phone]

PGP key updated to 4096 Bit RSA - I will encrypt all mails if the
recipient's key is known to me

http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xC0DE6A358A39DC66