Re: [radext] Gen-Art Early Review of draft-ietf-radext-dtls-07

[Alan DeKok <[e-mail]>]

Ben Campbell wrote:
> *** Major issues:
> 
> -- General:
> 
> The draft needs an overhaul of it's use of normative language. There appears to be redundant (and possibly contradictory) language for the same requirements sprinkled throughout. There are also cases of normative language being used for internal implementation guidance, which is not appropriate as described by RFC 2119. (See the minor issues section for details--most of the instances would not qualify as major issues by themselves, but I think they constitute a major issue in the aggregate.)
> 
> -- section 3, 2nd paragraph:  "... long-term use of RADIUS/UDP is NOT RECOMMENDED." 
> 
> While I agree with the sentiment, that's not an appropriate assertion for an experimental RFC. It would either need to go into an standards-track update to the RADIUS spec, or a BCP.

  OK.

> (Also applies to the reiteration in 10.1)

  I'm less sure of that.  10.1 doesn't re-iterate the point above.  It
makes a security recommendation.  i.e. allowing secure and insecure
traffic from the same client is bad.

  This imposes no change on existing RADIUS/UDP clients, as they will
only be sending RADIUS/UDP.  It does impose security requirements on
RADIUS/DTLS clients and servers, which is the intent of the section.

> *** Minor issues:
> 
> -- General:  It might be worth some text on why this is experimental rather than informational. Is there any plan to evaluate the implementation results? Is there an expectation that a future RADIUS/DTLS spec could become standards-track? Is there any plan to evaluate the outcome of this document?

  It's probably easiest to reference Section 1.3 of RFC 6614.  The
issues are the same.

> -- Section 1, 2nd paragraph:
> 
> Isn't this true for almost any use of IPSec? Is there some specific reason this is worse for RADIUS than for other protocols?

  It's true for most protocols.

> -- Section 1, 4th paragraph:
> 
> The second sentence mentions that firewall rules do not need to be changed. The following sentence recommends a change to firewall rules.

  That's left over from earlier drafts.  I'll fix it.

> The firewall rule recommendations in the 3rd sentence seems odd, since it seems like any protocol over DTLS would pass. Also, does this imply a recommendation that firewalls with DPI be used in the first place, since the absence of such a firewall has the same effect as having one that doesn't enforce the protocol? (Is there a reason a protocol spec should recommend firewall rules in the first place, other than to mention places where certain firewall rules might prevent interfere with operation?)

  I'll clarify it, and move it to the Security Considerations section.

> -- section 2.1, 5th paragraph (3rd paragraph after bullet list) :  "Implementations MUST support encapsulated RADIUS packets of 4096 in length..."
> 
> Please be more precise than "MUST support". Specifically, does "MUST support" indicate you must support both sending and receiving of that size? (since 4096 would generally exceed PMTU even for RADIUS/UDP)

  Yes.  RADIUS/UDP has the same 4096 octet limit for sending and
receiving packets.

> -- 2.2 and children:
> 
> I find this section confusing as to where to find the authoritative text. Some, but not all, of the material from 6614 is repeated as normative text in later sections.  The description of this draft as applying 'semantic "patches"' to 6614 seems to imply you mean the 6114 text, with these patches applied, to be normative, which creates some potential conflicts. If, on the other hand, 2.2 (.x) is intended more as a informative description of the differences, please say so.

  What conflicts occur?  The intent is show this specification as a
revision / patch from 6614.

> -- 3, 1st paragraph:
> 
> Can you elaborate on the resulting "timeouts, lost traffic, and network instabilities"?

  RADIUS/UDP has no provisions for protocol negotiation.  So simply
disabling UDP makes it look like the server is gone, rather than
transitioned to DTLS.

> -- 3.1: 
> 
> The section describes UDP/2083 as the "default port". But later sections describe port related behavior in a way that makes it it look like the impementations must always use 2083, which makes it more than just a default. Is the administrator allowed to choose some other port for any reason?

  Yes.  As with most protocols, implementations allow the administrator
to use non0-standard ports.

> If so, it might make more sense to refer to the port by role rather than number when discussing port related behavior. (I note that 6614 only mentions the port number in the initial assignment, the IANA considerations, and the appendix.)

  Maybe.  That makes the text a bit more awkward.  "UDP/2083" would get
replaced by "that or any other port you happen to use".

> -- 3.2, last paragraph:
> 
> Can you elaborate on the bid down attack? Given that the use of dtls is configured, rather than negotiated, how would an attacker bid it down?

  When secure and insecure packets are allowed from the same client, a
malicious or broken client can choose the insecure one.  That's bad,
when the intent is to use DTLS.

> -- 4, 2nd paragraph:
> 
> Seems like an (or maybe even the) important point would be that a client should not fall back to cleartext if a server appears to not support it.

  That's reasonable.

> -4, 3rd paragraph:
> 
> Is the recommendation to use a local proxy for all clients, or just those implemented as multiple processes?

  Only ones with multiple processes.

> -- 5.1.1, third paragraph: "In those cases, the implementation MAY delete the underlying session"
> 
> Can you elaborate on why that's a MAY and not a SHOULD or MUST?

  There was a long discussion about this on the RADEXT list.  In short,
the session MUST be closed when there are security issues, or when the
tunneled data isn't RADIUS.

  When security is OK and the tunneled data is RADIUS, it's OK to
silently drop the unexpected RADIUS packets.  Doing so doesn't cause any
problems.

> -- 5.1.1, 4th paragraph:
> 
> This paragraph rephrases 2119 normative language with more normative language. That creates confusion about which text is authoritative. I suggest either keeping the second paragraph and deleting the first, or make the second non-normative.

  I'm not sure what you mean here.  The two paragraphs discuss different
things, so deleting one isn't possible.

> -- 5.1.1, 6th paragraph: "The timestamp SHOULD be updated ..."
> 
> Why not MUST?

  The granularity could be 1 second, with packets being received 1000's
of times per second.

> -- 5.1.1, 7th paragraph:
> 
> Is the "idle time" configurable setting on a per peer or global basis?

  Possibly both.  That depends on the implementation.  I don't see it as
being useful to either restrict the implementations, or make
recommendations here.

  The text here talks about idle timeouts per *session*.  How the
implementation takes that from a configuration setting to a session is
up to the implementation.

> -- 5.1.1, 8th paragraph:
> 
> What does it mean to "track" sessions?  Also, it seems like the "SHOULD stop creating" guidance contradicts later guidance that least recently used sessions might get dropped instead?

  Perhaps "open sessions" is better than "tracked sessions".

  The idea is that it will either drop the new one, or drop an old one.
 Either way, the "maximum sessions" value should not be exceeded.  I'll
update the text.

> -- 5.1.1, 10th paragraph:  
> 
> Should the second sentence contain a normative MUST?

  Yes.

> Also, the 2nd and 3rd sentences taken together seem say "the server must keep the session open until it decides not to" 

  It closes the session when the client asks for it to be closed, or
when the idle timeouts are reached.

  But really, the server is allowed to close sessions any time it wants.
 I don't think we want to restrict that power.

> How would an unauthenticated client close an active DTLS session?

  If a 4-tuple is re-used, AND the server closes an old session to
handle the "new" one.  I'll clarify the text.

> -- 5.2, 1st paragraph:
> 
> The normative requirement for a client to use heartbeats _or_ the application level watchdog algorithm seems to contradict the normative guidance that a server SHOULD use both.

  I'll clarify the text.

> -- 5.2, 3rd paragraph, 1st sentence:
> 
> I would hesitate to phrase this that a client may violate the previous normative SHOULDs. It would be better to phrase this as describing th econsequences should the client ignore the SHOULD.

  I'll clarify the text.

> -- 5.2, 2nd to last paragraph:
> 
> Does this have actual interop or security issues beyond saying, "it's harder to implement and you might screw it up"? If not, it seems counter to the 2119 guidance on when normative language is appropriate. It would be more properly non-normative  implementation guidance.

  OK.

> -- 6, 1st paragraph:
> 
> You mention that these are implementation guidelines, and not part of the protocol. But the section contains 2119 style normative requirements. in general, that's not appropriate unless non-compliance impact interoperability or create some other Bad Thing, such as insecure behavior, excessive bandwidth usage, congestion, etc. Implementation guidance for behavior that is not externally observable should use non-normative.

  OK.

> -- 6, 2nd and 3rd paragraph:
> 
> The RECOMMENDation to allow administrative entry of keys and to derive keys from a PRNG seem contradictory.

  The idea is that admins shouldn't be entering "0xabababab" as a key.
Instead, they get secure keys from a secure location.  People are
notoriously bad at creating randomness.

> -- 6.1, 1st paragraph:
> 
> Does the guidance to use connected sockets remove previous normative requirements about session management and tracking? If not, please indicate the difference.

  No.  The previous text about client session management doesn't talk
about multiple sessions.  It just talks about how to manage *one* session.

  This text is intended to suggest that managing multiple DTLS sessions
on one socket isn't necessary.

> -- 6.1, 3rd paragraph:
> 
> This seems to assume that all radius clients are implementd as multiple processes. Is that the intent?

  No.

> Also,  it's better not to use normative requirements for internal implementation choices. Describe the externally visible behavior normatively. You can give implementation advice in the form of example strategies to fulfill the black-box normative requirements.

  I'll copy the text from earlier in the document which discusses this.

> -- 9
> 
> Why not choose a new port? Is there absolute certainty this won't conflict with the previous usage? I do note that 6414 made the same choice, so I guess consistency with radius/TLS has some value. But that draft talks about compatibility between radsec and radius/tls, which is not mentioned here.

  No one is using UDP/2083 for anything.  Re-using it is OK.

> -- 10, last paragraph:
> 
> You describe the use of null crypto as an implementation or configuration error. Was it intended to be forbidden from intentional use? Is there a need to remove the fixed secret requirement for null crypto, or to disallow null crypto entirely?

  null crypto should be forbidden.

> -- 10.1, 3rd paragraph:
> 
> It would be helpful to have guidance on how to match a certificate to a client or server identity,

  I'll add a note to see RFC 6614 Section 2.5.

> how to configure trust for a self-signed cert, etc.

  I'll avoid that, to be honest.

> -- 10.1, 4th paragraph: 
> 
> -- 10.1, last paragraph:
> 
> Why does the historic use of shared secrets matter, since this document requires all implementations to use a fixed value? Or do you mean the use of poor secrets as PSKs?

  I mean the use of poor secrets as PSKs.

> -- 10.2, 2nd paragraph:
> 
> This is redundant with previous normative requirements. (Also contradictory, since the previous text said "SHOULD", and contradictory guidance on what to do when the limit is exceeded.)

  I've fixed the text to be consistent.

> -- 10.3, 4th paragraph:
> 
> Does this need to be as strong as SHOULD? Is this likely to conflict with dynamic discovery, should it ever exist?

  RADIUS is a critical piece of infrastructure.  Exposing your RADIUS
server to the entire IPv4 range is a bad idea.

  The recommendation is a SHOULD so that it does not conflict with
dynamic discovery.

> -- 10.3, 5th and 6th paragraphs:
> 
> Paragraph 5 says credentials should be statically configured. To me, "credentials" means "the certificate" in this case. That seems to disallow things like statically configuring a name that must match a certificate (perhaps signed by a CA.)

  Credentials for the client should be statically configured.  It could
be a PSK, or a certificate.

  I'm not sure what "name" would match a certificate.  The document
doesn't refer to names, and doesn't use names for anything.

> Paragraph 6 seems to entirely contradict paragraph 5 by recommending private CAs, and accepting any peer that presents a cert signed by that CA. That's pretty much the opposite of statically configured credentials. (Paragraph 8 also seems to contradict the static configuration part.)

  Both client and server still need to be configured with the private
CA.  The alternative is to allow anyone to connect with any credentials,
which seems odd.

> The last sentence refers to "the invalid server". What invalid server is that? None have been mentioned so far.

  The previous sentence talks about a "valid" server.  I'll clarify.

> -- 10.4:
> 
> The guidance in the last paragraph does not make sense. The section seems to say that NATs will break radius/dtls, so you should use dtls when behind a NAT.

  RADIUS/UDP clients should not be behind a NAT.  I'll clarify.

> -- 10.6, last paragraph:
> 
> Can you elaborate on this? Why would an unauthorized 3rd party be able to get packets past the DTLS layer? OTOH, If an authorized client is sending invalid radius packets, wouldn't  you want to terminate the session?

  That was a *long* discussion on the RADEXT list.

  A server may be at protocol rev X, and a client may be at protocol rev
X+1.  If the client sends packets allowed by X+1, the server should
ignore them, as it does today with RADIUS/UDP.  Closing the connection
is not necessary.

> -- 10.7
> 
> Redundant normative requirements (This is at least the third time the separate process issue and local proxy guidance has been described.)

  It's a security considerations section, and needs to mention this.  It
refers to Section 6.1, and adds new text explaining the security
benefits of the approach.

> 
> 
> *** Nits/editorial comments:
> 
> -- IDNits reports some issues; please check.

  They're fine.

> -- Abstract:
> Please avoid citations in the abstract. Please consider removing the "scare quotes".

  OK.

> -- Section 1, 5th paragraph:
> 
> it seems like the RADIUS problems that this does not solve are in generally solved by RADIUS/TLS. If so, it would probably be worth a mention.

  OK.

> -- section 2.1, 4th paragraph from end: 
> 
> Seems like there are other changes as well. For example, you don't include "Use DTLS" as one of the changes in RADIUS/DTLS from RADIUS/USP.

  OK.

> -- 2.2 and children:
> 
> In my strictly personal opinion, the approach of patching 6614 transfers a lot of effort from the author to the reader. A reader will basically need to keep both docs open side by side to understand this.  I understand the desire to avoid duplicating normative text, but I think that the balance here has swung too far away from readability.  (OTOH, see my related comment under "minor issues).

  I think re-doing the document is a non-starter at this point.

  Having done this myself, the bulk of the implementation work in doing
DTLS is the UDP layer.  Most everything else can be re-used directly
from RADIUS/TLS.

> -- 2.2.1, paragraph 5:
> 
> should TLS parameters be DTLS parameters?

  Yes.

> -- 2.2.2:
> 
> Why a separate section to say sections 4 and 6 also apply? (The re-iteration seems unneeded.)

  OK.

> -- 3.1, last sentence:
> 
> Really, 2.2.1 doesn't describe that. 6114 describes that. Better to cite the source than to cite a citation to the source, especially when that citation doesn't mention the issues at all.

  OK.

> -- 4, 3rd paragraph:
> 
> Do you really mean multiple independent _implementations_? Or multiple independent instances or processes, perhaps running the same implementation?

  All of that.

> -- section 5 and children:
> 
> In consistent use of "connection" vs "session".

  I'll fix that.

> -- 5.1.1, 2nd paragraph:
> 
> What is an "entry" session?

  The 4-tuple is logically a table tracking (4-tuple) -> (session data).
 Each one is an entry.

  I'll try to clarify the text.

> -- 6.1, 2nd paragraph:
> 
> Source port? Source address? Both?

  Both.

> -- 7:
> 
> Only the first paragraph seems to be about implementation experience.

  OK.

> -- 10, 2nd paragraph: "All of the security considerations for RADIUS apply to the RADIUS portion of the specification."
> 
> The next paragraph seems to contradict this.

  OK.

> -- 10.3, 2nd paragraph: "... a client has a fixed IP address for a server ..."
> 
> This is ambiguous. Do you mean the server knows the client address, or the client knows the server address? It sounds like the latter, but later text makes me wonder if you meant the former.

  The following sentence addresses this.  The client has a fixed IP for
the server, and the server has a fixed IP for the client.

> -- 10.3, 7th paragraph: "... pre-configured with a list of known public CAs."
> 
> By pre-configured, you mean by the manufacturer or vendor, right?

  Yes.

> -- 10.5, 2nd paragraph:
> 
> Does this contradict guidance about using a private CA to identify multiple peers?

  No.

> I don't think you intend it that way; I assume you mean the cert presented by a client must be unique, but as written it's easy to assume that you mean the server has to have unique certs configured for each client, which it would not if it were to allow any arbitrary client that has a cert signed by a private CA.

  That's what it says, so far as I can tell.