Thanks Ben for the review! - JOuni
On Nov 19, 2013, at 1:38 AM, Ben Campbell <b...@nostrum.com> wrote: > > I am the assigned Gen-ART reviewer for this draft. For background on > Gen-ART, please see the FAQ at > > <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>. > > This is an early review at WGLC last call. > > Document: draft-ietf-radext-dtls-07 > Reviewer: Ben Campbell > Review Date: 2013-11-18 > > Summary: This draft is on the right track, but there are significant open > issues that should be resolved before it progresses. > > [Note: This review is different from the usual Gen-ART review due to the fact > that it's an early (as in prior to pubreq) review, and that the draft is > intended to become an experimental RFC. The gen-ART review process is tuned > for drafts at IETF last call or later. It's a little hard to figure out what > criteria should be used for drafts at an earlier point in the life-cycle. > That being said, I reviewed this as if it were near-final, and apologize if > that turns out to be too strict for an early review. > > I used similar review criteria as I would for a standards-track draft, since > this draft still specifies protocol with the hope of interoperable > implementations. The only difference comes from a few comments explicitly > about the experimental status of the draft.] > > > *** 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. > > (Also applies to the reiteration in 10.1) > > > *** 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? > > -- 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? > > -- 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. > > 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?) > > -- 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) > > -- 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. > > -- 3, 1st paragraph: > > Can you elaborate on the resulting "timeouts, lost traffic, and network > instabilities"? > > -- 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? 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.) > > > -- 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? > > -- 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. > > -4, 3rd paragraph: > > Is the recommendation to use a local proxy for all clients, or just those > implemented as 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? > > -- 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. > > -- 5.1.1, 6th paragraph: "The timestamp SHOULD be updated ..." > > Why not MUST? > > -- 5.1.1, 7th paragraph: > > Is the "idle time" configurable setting on a per peer or global basis? > > -- 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? > > -- 5.1.1, 10th paragraph: > > Should the second sentence contain a normative MUST? > > Also, the 2nd and 3rd sentences taken together seem say "the server must keep > the session open until it decides not to" > > How would an unauthenticated client close an active DTLS session? > > -- 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. > > -- 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. > > -- 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. > > -- 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. > > -- 6, 2nd and 3rd paragraph: > > The RECOMMENDation to allow administrative entry of keys and to derive keys > from a PRNG seem contradictory. > > > -- 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. > > -- 6.1, 3rd paragraph: > > This seems to assume that all radius clients are implementd as multiple > processes. Is that the intent? > > 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. > > -- 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. > > -- 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? > > -- 10.1, 3rd paragraph: > > It would be helpful to have guidance on how to match a certificate to a > client or server identity, how to configure trust for a self-signed cert, etc. > > -- 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? > > -- 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.) > > -- 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? > > -- 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.) > > 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.) > > The last sentence refers to "the invalid server". What invalid server is > that? None have been mentioned so far. > > -- 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. > > -- 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? > > -- 10.7 > > Redundant normative requirements (This is at least the third time the > separate process issue and local proxy guidance has been described.) > > > > *** Nits/editorial comments: > > -- IDNits reports some issues; please check. > > -- Abstract: > Please avoid citations in the abstract. Please consider removing the "scare > quotes". > > -- 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. > > -- 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. > > -- 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). > > -- 2.2.1, paragraph 5: > > should TLS parameters be DTLS parameters? > > -- 2.2.2: > > Why a separate section to say sections 4 and 6 also apply? (The re-iteration > seems unneeded.) > > -- 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. > > -- 4, 3rd paragraph: > > Do you really mean multiple independent _implementations_? Or multiple > independent instances or processes, perhaps running the same implementation? > > -- section 5 and children: > > In consistent use of "connection" vs "session". > > -- 5.1.1, 2nd paragraph: > > What is an "entry" session? > > -- 6.1, 2nd paragraph: > > Source port? Source address? Both? > > -- 7: > > Only the first paragraph seems to be about implementation experience. > > -- 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. > > -- 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. > > -- 10.3, 7th paragraph: "... pre-configured with a list of known public CAs." > > By pre-configured, you mean by the manufacturer or vendor, right? > > -- 10.5, 2nd paragraph: > > Does this contradict guidance about using a private CA to identify multiple > peers? 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. > > > > > > > > _______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
