Subject: [Gen-art] Gen-ART LC review of draft-ietf-radext-tcp-transport-06.txt

From: Glenn Kowack <[email-masked]>
Date: Tue, 18 May 2010 01:58:50 -0400

I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).

Please resolve these comments along with any other Last Call comments
you may receive.

Document:			<draft-ietf-radext-tcp-transport-06.txt>
Reviewer:			Glenn Kowack
Review Date:			2010-05-18 (late; my apologies)
IETF LC End Date:		2010-05-11
IESG Telechat date:	not known

Summary:			This document should be ready for publication as an Experimental RFC after clarifications (a few are minor; most are nits/editorial).  The ideas appear sound, but the style of the document tends toward conversational, creating opportunities for ambiguity and confusion.  The text should be more direct and conventional.

Major issues:			None.

Minor issues:

Abstract:
“The Remote Authentication Dial In User Server (RADIUS) Protocol has traditionally used the User Datagram Protocol (UDP) as its underlying transport layer.”
Does ‘traditional’ here mean as defined by the specification or is UDP use optional?

“It is not intended to define TCP as a transport protocol for RADIUS in the absence of TLS.”

This might be interpreted by some as disingenuous.  Would it be clearer to say: “The specification only provides for use of TCP with TLS.  Other uses of TCP are out of scope.” ? 

1. Introduction

“Since "bare" TCP does not provide for confidentiality or enable negotiation of credible ciphersuites, its use is not appropriate for inter-server communications where strong security is required. As a result the use of "bare" TCP transport (i.e., without additional confidentiality and security) is NOT RECOMMENDED, as there has been little or no operational experience with it.”

This paragraph is confusing.  It begins with a clear:

    “Since "bare" TCP does not provide for confidentiality or enable negotiation of credible ciphersuites, its use is not appropriate for inter-server communications where strong security is required.”

And another clear statement:

   “As a result the use of "bare" TCP transport (i.e., without additional confidentiality and security) is NOT RECOMMENDED,…”

But adds: “… as there has been little or no operational experience with it.”

That is, the document says there are inherent reasons why TCP is not appropriate, and then says the reason is lack of experience.  This should be unwound and clarified. 

1.1. Applicability of Reliable Transport 

“For a system with a million logins a day running EAP-TLS mutual authentication with 15 round-trips, and having a packet loss probability of P=0.01%, we expect that 0.3% of connections will experience at least one lost packet. That is, 3,000 user sessions each day will experience authentication failure. This is an unacceptable failure rate for a mass-market network service.”

Can the document state an acceptable level of failure, and a supporting citation?

Nits/editorial comments:

1. Introduction

“The RADIUS Protocol has been defined in [RFC2865] as using the User Datagram Protocol (UDP) for the underlying transport layer.”

Present tense would be clearer, unless 2865 is no longer applicable. 

“2.4, there are also some limitations:

* Unreliable transport. As a result, systems using RADIUS have to implement application-layer timers and re-transmissions, as described in [RFC5080] Section 2.2.1.”

Add a blank line between the ‘2.4’ line and the first bullet item, per the rest of the document.

“As RADIUS is widely deployed, and has been widely deployed for well over a decade, these issues have been minor in some use-cases, and problematic in others.”

The leading ‘as’ is awkward and not necessary, per this example: “RADIUS has been widely deployed for well over a decade, and still is.  Experience shows issues that are minor in some use-cases, and problematic in others.”

2.3. Management Information Base (MIB)

“The MIB Module definitions in [RFC4668], [RFC4669], [RFC4670], [RFC4671], [RFC4672], and [RFC4673] ……… SHOULD NOT re-use these MIB Modules to perform statistics for RADIUS over TCP connections.w”

Typo: trailing ‘w’ at end of paragraph. 

2.4. Detecting Live Servers

“As RADIUS is a "hop by hop" protocol, a RADIUS proxy effectively shields the client from any information about downstream servers.”

‘effectively’ adds nothing here. However, this change is stylistic and at the discretion of the author. 

“When UDP was used as a transport protocol…”

Present tense is more appropriate.

2.6.1. Duplicates and Retransmissions

“In that situation, RADIUS request MAY be retransmitted to another server that known to be part of the same pool.”

s/that known/that is known/

[ Running a grammar checker (e.g., in MSWord) should have found this. ]

2.6.4. Malformed Packets and Unknown Clients

“After applying the above rules, there are still situations where the previous specifications allow a packet to be "silently discarded". In these situations, the TCP connections MAY remain open, or MAY be closed, as an implementation choice. However, the invalid packet MUST be silently discarded.

* Packet with an invalid code field

* Response packets that do not match any outstanding request

These requirements minimize the possibility for a misbehaving client or server to wreak havoc on the network.”

This section would be clearer if the two bullets immediately followed where they are first mentioned, after “…allow a packet to be “silently discarded.”"

Also, does the author mean ‘minimize’ or just ‘reduce’, above?

2.6.5. Limitations of the ID Field

“…there would be no free Id to use…”

s/Id/ID/

This reviewer would like to observe that this is one of the most astute and downright clever Freudian observations about the IETF that he has seen in many years.  He takes his hat off to the author. Sadly, accurately using upper-case-D ‘ID’ regrettably MUST win out over literary invention.

“should reserve ID zero on each”

s/ID zero on each/ID zero (0) on each/

 “Implementors may be tempted to extend RADIUS to permit more than 256 outstanding packets on one connection.  However, doing so will likely require fundamental changes to the RADIUS protocol, and as such, is outside of the scope of this specification.”

Two points are confounded here, as illustrated by:

     Implementors may be tempted to extend RADIUS to permit more than 256 outstanding packets on one connection.  However, doing so is a violation of a fundamental part of the protocol and SHOULD NOT be done.  Making that extension here is outside of the scope of this specification.

_end review comments

---

From: Alan DeKok <[email-masked]>
Date: Mon, 24 May 2010 10:10:20 +0200

Glenn Kowack wrote:
> Major issues: None.
> 
> Minor issues:
> 
> Abstract:
> “The Remote Authentication Dial In User Server (RADIUS) Protocol has
> traditionally used the User Datagram Protocol (UDP) as its underlying
> transport layer.”
> 

  It means "as defined by the spec".  Until now, UDP has been mandatory.

> This might be interpreted by some as disingenuous.  Would it be clearer
> to say: “The specification only provides for use of TCP with TLS.  Other
> uses of TCP are out of scope.” ? 

  Later reviews suggested:

It is not intended to define TCP as a transport protocol for RADIUS in
the absence of a secure transport layer.

> *1. Introduction*
..
> That is, the document says there are inherent reasons why TCP is not
> appropriate, and then says the reason is lack of experience.  This
> should be unwound and clarified.  

  Later reviews suggested:

Since "bare" TCP does not provide for confidentiality or enable
negotiation of credible ciphersuites, its use is not appropriate for
inter-server communications where strong security is required.  As a
result "bare" TCP transport MUST NOT be used without TLS, IPsec, or
other secure upper layer.

"Bare" TCP transport MAY, however, be used when another method such as
IPSec [RFC4301] is used to provide additional confidentiality and
security.  Should experience show that such deployments are useful,
this specification could be moved to standards track.

  I think that text is clearer.

> *1.1. Applicability of Reliable Transport *
> 
> “For a system with a million logins a day running EAP-TLS mutual
> authentication with 15 round-trips, and having a packet loss probability
> of P=0.01%, we expect that 0.3% of connections will experience at least
> one lost packet. That is, 3,000 user sessions each day will experience
> authentication failure. This is an unacceptable failure rate for a
> mass-market network service.”
> 

  Umm... no.  No one is willing to share data.  The above numbers are
based on hallroom conversations with roaming implementors.

> 
> Nits/editorial comments:
> *
> *
> 
> *1. Introduction*
> 
> *“The RADIUS Protocol has been defined in [RFC2865] as using the User
> Datagram Protocol (UDP) for the underlying transport layer.”*
> 

  Agreed.

> * Unreliable transport. As a result, systems using RADIUS have to
> implement application-layer timers and re-transmissions, as described in
> [RFC5080] Section 2.2.1.”
> 

  Done.

> “As RADIUS is widely deployed, and has been widely deployed for well
> over a decade, these issues have been minor in some use-cases, and
> problematic in others.”
> 
> The leading ‘as’ is awkward and not necessary, per this example: “RADIUS
> has been widely deployed for well over a decade, and still
> is.  Experience shows issues that are minor in some use-cases, and
> problematic in others.”

  Agreed.

> *2.3. Management Information Base (MIB)*
> 
> “The MIB Module definitions in [RFC4668], [RFC4669], [RFC4670],
> [RFC4671], [RFC4672], and [RFC4673] ……… SHOULD NOT re-use these MIB
> Modules to perform statistics for RADIUS over TCP connections.w”
> 

  Fixed.

> *2.4. Detecting Live Servers*
> 
> “As RADIUS is a "hop by hop" protocol, a RADIUS proxy effectively
> shields the client from any information about downstream servers.”
> 

  I'll delete it.

> Present tense is more appropriate.

  Agreed.

> *2.6.1. Duplicates and Retransmissions*
> 
> “In that situation, RADIUS request MAY be retransmitted to another
> server that known to be part of the same pool.”
> 

  Fixed.

> [ Running a grammar checker (e.g., in MSWord) should have found this. ]

  vi + nroff? :(

> *2.6.4. Malformed Packets and Unknown Clients*
> 
> “After applying the above rules, there are still situations where the
> previous specifications allow a packet to be "silently discarded". In
> these situations, the TCP connections MAY remain open, or MAY be closed,
> as an implementation choice. However, the invalid packet MUST be
> silently discarded.
> 
> This section would be clearer if the two bullets immediately followed
> where they are first mentioned, after “…allow a packet to be “silently
> discarded.”"

  Agreed.

> Also, does the author mean ‘minimize’ or just ‘reduce’, above?

  "reduce".  I'll fix it for the next revision.

> *2.6.5. Limitations of the ID Field*
> 
> This reviewer would like to observe that this is one of the most astute
> and downright clever Freudian observations about the IETF that he has
> seen in many years.  He takes his hat off to the author. Sadly,
> accurately using upper-case-D ‘ID’ regrettably MUST win out over
> literary invention.

  Yes... fixed for the next revision.

> s/ID zero on each/ID zero (0) on each/

  Fixed.

>  “Implementors may be tempted to extend RADIUS to permit more than 256
> outstanding packets on one connection.  However, doing so will
> likely require fundamental changes to the RADIUS protocol, and as such,
> is outside of the scope of this specification.”
> 
>      Implementors may be tempted to extend RADIUS to permit more than
> 256 outstanding packets on one connection.  However, doing so is a
> violation of a fundamental part of the protocol and SHOULD NOT be
> done.  Making that extension here is outside of the scope of this
> specification.

  Agreed.

  Alan DeKok.

---

From: Glenn Kowack <[email-masked]>
Date: Mon, 24 May 2010 13:05:37 -0400

Alan,
    clarifications of a few of my original review comments:

>  It means "as defined by the spec".  Until now, UDP has been mandatory.

I recommend you say that explicitly: as defined by the specification.  It's not
clear to me what 'traditional' means to random readers; they may view the
cited use of UDP as strictly historical.  In any event, they probably won't
understand that 'traditional' means 'normative'.

> the absence of a secure transport layer.

Although an improvement, your change does not provide the clarification
I believe is necessary.  Stating a negative, and also citing 'intent' in "not
intended to define", is awkward and leaves room for speculation about the
positive case.  Please see my prior example.

> based on hallroom conversations with roaming implementors.

I think I should have been clearer that my question is about generally-
acceptable levels of failure for a mass-market network service.  This sort
of data covers a broad area and should be publicly available.

Finally, I didn't think about the delights of Nroff when it comes to running
grammar and other checks - sigh.  Why can't these tools all just learn to
get along?

regards,
Glenn
___