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

From: Alan DeKok <[email-masked]>
Date: Thu, 23 Jan 2014 14:05:44 -0500

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.

---

From: Jouni Korhonen <[email-masked]>
Date: Fri, 24 Jan 2014 17:47:52 +0200

Ben,

Alan just posted -08 revision. Could you check it whether all your concerns
have been addressed and you are OK with the revision.

- JOuni (doc shepherd)

On Jan 23, 2014, at 9:05 PM, Alan DeKok <[email-masked]> wrote:

> _______________________________________________
> radext mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/radext

---

From: Ben Campbell <[email-masked]>
Date: Thu, 30 Jan 2014 16:50:58 -0600

Jouni poked me to review 08. In the process of doing that, I found your email that I had somehow missed. Apologies for the late response. I've removed sections that do not seem to require further discussion or comment.

Thanks!

Ben.

>> *** Major issues:

[...]

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

Version 08 partially addresses this by changing "NOT RECOMMENDED" to lower case. I don't think that's sufficient; it's not a good idea to rely on case alone to distinguish normative from non-normative text. I propose changing the sense of the statement to something along the lines of "The author recommends...". 

(Unlike some reviewers, I will not go so far as to say the word "should" should never be used non-normatively, since the resulting text tends to be awkward. But I think the NOT RECOMMENDED/not recommended difference can be handled with more clearly non-normative text without introducing the same awkwardness.)

> RADIUS/DTLS clients and servers, which is the intent of the section.

The text to which I refer says "It is RECOMMENDED that all RADIUS clients and servers implement this specification, or [RFC6614]." That seems to to apply a new normative statement to all RADIUS clients and servers, and does not in any way seem scoped to "RADIUS/DTLS clients and servers".

>  It's probably easiest to reference Section 1.3 of RFC 6614.  The

I don't think that's the best approach. That discussion is specific to 6614. Referencing it would leave the reader to infer how that discussion would apply to this draft. I think it would be better to include a similar discussion here.

>  It's true for most protocols.

It's worth mentioning that.

[...]

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

The clarification added at the end of  section 10 in version 08 mostly addresses this. But I assume that the point is to make sure no one sends non-protected RADIUS over that port, but it doesn't prevent someone from sending some other protocol over DTLS over the same port, correct? If so, I think that's worth a mention.

[...]

> revision / patch from 6614.

On a quick re-review, I have not found a specific example of conflicting or redundant normative text in version 08. I thought I had run into some around peer authentication before, but either I was mistaken or the 08 updates corrected it.

[...]

> replaced by "that or any other port you happen to use".

It doesn't need to be that hard. One can start by saying something like the RADIUS/DTLS port is UDP/2083 by default, but administrators can assign other, non-standard values. Then all future mentions refer to "the RADIUS/DTLS port" rather than UDP/2023. But in any case, this is only a mild suggestion; with the addition of the disclaimer to section 3.1 of version 08, the future UDP/2083 references are less troublesome.

> when the intent is to use DTLS.

One assumes a "malicious or broken" client already has access to the cleartext messages. Are we talking about a MiTM "client"? Are there attacks where a third party can induce the client or server to send unprotected packets, even with no signaled negotiation? I can imagine one for the specific case where a node falls back to clear text if DTLS packets fail; an attacker could induce failures for DTLS packets. (Thus the next comment.)

>  Only ones with multiple processes.

The text seems to say something to the effect of " Some RADIUS clients have historically been implemented as multiple independent processes, therefore [all] RADIUS clients should use a local proxy".  I think you mean to say something more like "Some RADIUS clients have historically been implemented as multiple independent processes; clients that are implemented that way should use a local proxy." 

> silently drop the unexpected RADIUS packets.  Doing so doesn't cause any

It would be helpful to add a couple of sentences to that effect. 

> things, so deleting one isn't possible.

The opening sentence of the second paragraph says, "The above paragraph can be rephrased more generically", then appears to go on to do so. That makes it sound like the second paragraph asserting effectively the same normative requirements in more general terms. If you intend each paragraph to stand alone, then I suggest deleting that sentence.

>  The granularity could be 1 second, with packets being received 1000's

It would be helpful to elaborate on that in the text. Your comment makes me thing it would be also worth having some guidance on a reasonable resolution for the time stamp.

If you really expect 1 second resolutions, then it might be better to say something like "The timestamp MUST be updated if a valid message or heartbeat is received during the last time interval."

[...]

> notoriously bad at creating randomness.

So paragraph 3 is a requirement on the administrator, rather than on the implementor? Or do you mean to say the implementation should provide tools for random key generation?

[...]

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

Does that mean that RadSec is not used at all by anyone? or that this draft is compatible with it?

[...]

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

I see you added that to section 10.3, 3rd paragraph. But does that contradict the statement in 2.2.1 that 6614 section 2.5 does not apply to RADIUS/DTLS?

[...]

>  I've fixed the text to be consistent.

The paragraph is still redundant with the new text in 5.1.1 (which says you MUST either drop old sessions or limit creation of new ones.)

> server to the entire IPv4 range is a bad idea.

That's true of many protocols that don't have normative requirements about IP filtering. This is more an operational issue than an implementation one, and it may not be the best choice for all (or even most) deployments. There may be some advantage in filtering packets before they get to the DTLS layer, but IP filtering is trivial to defeat, and it has real administrative costs. For example, any peer network renumbering has to be coordinated across all of your RADIUS nodes.

I agree it makes sense to point out that IP filtering might be helpful. But I think a normative SHOULD is excessive.

>  The recommendation is a SHOULD so that it does not conflict with

If it stays normative, It would be worth mentioning that as an example of why it's a SHOULD rather than a MUST.

> doesn't refer to names, and doesn't use names for anything.

The whole point of a certificate is to bind a key pair to an identity (often, a name.)

Example: I enter the name "trusted-peer.example.com" in a table. If a (perhaps dynamically discovered) peer presents certificate with a CN or SAN that matches "trusted-peer.example.com", and that certificate is issued by a CA that I trust, then I trust that peer. 

> CA.  The alternative is to allow anyone to connect with any credentials,

That's static configuration of your trusted CA(s). That's not the same thing as static configuration of peer credentials, and it doesn't imply the static relationship between clients and servers mentioned in paragraph 5.

[...]

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

Do you mean RADIUS/UDP or RADIUS/DTLS? If the former, then that brings back the problem of making normative statements about the base protocol. Also, I'm still confused by the fact the the 2nd sentence of the last paragraph says "If clients are located behind a NAT gateway, then a secure transport such as DTLS MUST be used." Given that the previous paragraph seems to say that RADIUS/DTLS has issues over NAT, I'm surprised to see it listed as mitigating NATs.

> ignore them, as it does today with RADIUS/UDP.  Closing the connection

I think your previous answer about sending packets with the same 4-tuple answers this as well.

> benefits of the approach.

I agree it should be reiterated, but not _normatively_. In generally, you don't want to have the same normative requirement stated more than once. It makes sense for the security consideration section to say something like "Section XX recommends that clients use a local proxy"  But it's best to avoid any language that obscures which bit of text is authoritative for a given normative requirement. (There can be only one...)

I also notice the language here also fails to account for the fact that the requirement only applies to clients implemented as multiple independent instances. This could be fixed by something really simple like saying "... such clients ..." instead of "... clients... " in the second paragraph.

> DTLS is the UDP layer.  Most everything else can be re-used directly

[...]

>  I'll try to clarify the text.

I don't see a change in version 08. In particular, I'm confused by "Sessions (both 4-tuple and entry) ..." That sounds like you have two kinds of sessions, 4-tuple sessions and entry sessions. I think you mean something more like "Both the 4-tuple and the session table entry for a session ...." right?

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

No change in version 08.

[...]