As always, thanks for the comments! Regards,
Doug Inline... On 17/05/2017 15:54, "Alan DeKok" <al...@deployingradius.com> wrote: >On May 16, 2017, at 4:06 PM, Douglas Gash (dcmgash) <dcmg...@cisco.com> >wrote: >> Many items are marked with just [Agree], if it seems there is a trivial >>way to adjust according to the comment. > > Thanks. I'll comment on things I have comments on, and remove >everything else. > >> 3.5 ... >> >> - Unused fixed length fields SHOULD have values of zero. >> >> * what happens when they don't? I suggest adding text saying that the >> fields are ignored. >> [Agree, changed text to: "To signal that any variable length data >>fields are unused, their >> length value is set to zero." As to what happens when fields are >>unused, this will depend upon the fields] > > I meant what happens when an unused field has a non-zero length? > > If the field is unused, the spec should say the field is ignored, and >treated as if it did not exist. Agreed, though I¹m not sure how an unused field would not be ignored, almost by definition,. The only circumstance I can think of where a field has data but is unused may not be ignored is for logging, are you thinking we should preclude such? > >> ... This document does not discuss the management and storage of >> those keys. ... >> >> * there should be some discussion in the Security Considerations section >> about this. Otherwise, this subject will be brought up in the security >> review. >> [Will be happy to stand corrected, but that strikes me as both outside >>the documentation of the protocol, and likely to lock in practices that >>will become out-of-date.] > > The draft should also document recommended practices for how to use the >protocol, such as how keys should be managed / generated / stored. > > i.e. "As the security of the obfuscation method is unknown, keys >SHOULD be produced via a CSPRNG, keys SHOULD be different for every >client/server pair", etc. > > The original RADIUS RFC used a secret key, but didn't forbid a key of >zero length. I ran into at least one vendor who didn't allow for the >entry of secret keys, and always set it to a zero-length string. The >next rev of the RFC forbade that... So I think the specification of secrets for obfuscated option is, as you say, critical, and will provide some guidance such as non-empty. Also will make sure we include that servers MUST support capability of secret per client-server tuple (see next comment). But I¹m not so sure what we should say about storage. > >> ... It is an implementation detail of the server and client, >> as to whether they will maintain only one key, or a different key for >> each client or server with which they communicate. For security >> reasons, the latter options MUST be available, but it is a site >> dependent decision as to whether the use of separate keys is >> appropriate. >> >> * those sentences seem to disagree with each other. It's an >> "implementation detail", but it MUST be supported. I suggest requiring >> the capability of per-server and per-client keys. >> [Agree, the requirements must be supported by implementations, however >>we leave it for the implementor as to whether this is used] > > The wording is awkward. Perhaps "client implementations MUST support >per-server keys, and server implementations MUST support per-client keys". Sure, that sounds like a good suggestion. > > The sentence on "site dependent decision" can probably be removed. Or >maybe replaced with a suggestion that "using the same key for multiple >hosts is NOT RECOMMENDED" Agreed. > >> * This last sentence is confusing. Are there different secrets for >> different packets in one TCP stream? If so, nothing in the document >>says >> so. If not, then there is every reason to believe that the secret is >> mismatched. And, the *next* packet will decrypt incorrectly. Which >>means >> (again) you might as well just close the TCP connection. >> [though we would not want to restrict to a single secret on server for >>a client, we can restrict the clint that it should us just a single >>secret on a client at a time for a connection to a server. This would >>mean that we could not have single connect, multi session interaction >>from a client with multiple secrets so then we could close the >>connection] > > I'm not sure what that means... > > It should say that the same key is used for the lifetime of a TCP >connection. Though the keys can be changed in between TCP connections. > > And, if there's a key mismatch, all bets are off, and the TCP >connection is closed. Agreed. > >> This is a standard ASCII authentication. The START packet may >> contain the username, but need not do so. >> >> * what happens when the username is / is-not included? i.e. what do >> implementations *do*? >> [Agreed to expand. the server can of course, finish the session at any >>time, as detailed in the "session termination" section. But let's assume >>that we want the server to carry on the process, then it will retrieve >>the username as described in the following proposed text: "The START >>packet MAY >> contain the username. If the user does not include the username then >> the server MAY obtain it from the client with a CONTINUE >> TAC_PLUS_AUTHEN_STATUS_GETUSER. When the server has the username, it >> will obtain the password using a continue with >> TAC_PLUS_AUTHEN_STATUS_GETPASS. ASCII login uses the user_msg field >> for both the username and password. The data fields in both the >> START and CONTINUE packets are not used for ASCII logins, any content >> MUST be ignored. The session is composed of a single START followed >> by zero or more pairs of REPLYs and CONTINUEs, followed by a final >> REPLY indicating PASS, FAIL or ERROR."] > > OK. And if there's still no username? Do you mean, if client replies to the request to TAC_PLUS_AUTHEN_STATUS_GETUSER with an empty username. I would simply repeat the request in case it was a user error. We can document this. The protocol has a built-in limit of the number of interactions due to the seq_no, but implementations can limit interactions. We can add this detail. > >> * if this is a valid protocol flow, then it should be described. e.g. >> "Any username in the START packet is ignored", or "any username in the >> START packet MUST match the username in subsequent packets" >> [Normally, the username is in just one packet] >> >> ... The data fields in both >> the START and CONTINUE packets are not used for ASCII logins. >> >> * what happens if there is data in the fields? Is it ignored? >> [Agreed From perspective of protocol, yes] > > The draft needs to say so. Agreed. > >> * also, the challenge MUST be derived from a cryptographically strong >> pseudo-random number generator, and MUST change on every CHAP request. >> Otherwise, the spec allows (also silently) a fixes challenge. >> [This is interesting because, of course, the challenge is not generated >>for T+ but for a different interaction. the result of that other >>interaction is passed along to T+. These recommendations are good, but >>can we actually police this data?] > > That reply make implementation-specific assumptions. There are TACACS+ >implementations which don't get the challenge from a third-party. Just to clarify my comment: it is not an aspect of the processing of the T+ protocol that chooses the challenge. The protocol will get the challenge and response as pre-packaged unit. So I think we can demand that challenges be generated according to the spec for CHAP. But reaching into that spec to add our own criteria would, I think, be a mixing of concerns. > The draft needs to state security requirements on CHAP and the >challenges. If an implementation chooses to ignore them, then the >implementation is insecure and wrong. > > But the protocol needs to be specified securely in the draft. > >> MSCHAPv1 / MSCHAPv2: >> >> The length of the challenge value can be determined from the length >> of the data field minus the length of the id (always 1 octet) and the >> length of the response field (always 49 octets). >> >> * RFC 2433 says that the challenge is 8 octets. RFC 2759 says that the >> challenge is 16 octets. It would be good to re-iterate that here. >> >> * i.e. smaller challenges are insecure, and should be forbidden >> [again, as T+ interaction is not involved with the generation of the >>challenges, is this something we can actually meaningfully enforce?] > > The same comments as for CHAP apply here. And no, we can't "enforce" >it, but we can demand it. Which is what the protocol specification is >about. Sure, we will update the doc for MS-CHAP in whatever way we find best for CHAP. > >> ... To perform the authentication, the server will use a the >>algorithm >> specified RFC2759 >> >> * editorial: "specified IN RFC ..." >> [agreed] >> >> ... ASCII change password request >> >> * I suspect that the security area review will recommend that this be >> removed. Even RADIUS had "change password" removed at the behest of the >> security people. >> [Noted. I take it this not a request yet to remove though?] > > I predict that the Security Area review will raise questions about it. > > If the functionality isn't really used in current practice, it would be >best to have the draft note that the functionality exists. But that all >specification of the functionality is removed from the document, and >implementations SHOULD NOT support it. > > e.g. "flag FOO was historical, but has been removed for security >reasons. That is all." Ok, I see what you mean. We can add the SHOULD not clause here. Interesting though, as this is a significant use case for T+. Will be insteresting to see what the main objects were for why it was removed from RADIUS. > >> * what does the server do with this message? Should it be logged >> somewhere? >> [I would say, this is out of scope of the protocol, and a matter for >>implementation] > > It's a useful suggestion to implementors. > > "This message is typically logged, which helps administrators determine >reasons for failure" Sounds sensible, will add. >> >> If a key is supplied, the client MAY use the key in order to >> authenticate to that host. The client may use a preconfigured key >> for the host if it has one. >> >> * so the server is pushing keys to clients? The security implications >>of >> this should be discussed >> [SIDE Note: I think this part of the original draft spec is really >>worth culling. I certainly specify it should not be implemented in our >>server. I had intended to propose that is done in the next version when >>we add TLS, however, am considering proposing to do this sooner unless >>anyone objects.] > > I would suggest removing it, but noting it exists, as with the password >change above. Agreed. > >> * from a security point of view, it's unusual for a client to indicate >>to >> a server how a user was authenticated. This allows the client to claim >> authentication when none existed. >> [Areed, and server can accord appropriate confidence in this >>information with that proviso] > > That should be noted here, and in the Security Considerations section. > >> ... >> This field matches the port field in the authentication section >> (Section 4) above >> >> * what is meant by "matches"? Perhaps "The definition of this field is >> the same as in Section 4". Or does the field in the authorization >>session >> have to contain the same data as the similar field from the >>authentication >> session? >> [Agreed, the first meaning] > > The draft should be updated to clarify this. Will do. > >> * what are the legal characters for an attribute name? The previous >> paragraph says US-ASCII. So is "($#@!" a legal attribute name? >> [Agreed. Yes, that is a valid name, because it does not include = or *. >>the text is explicit though, do you think it needs further >>clarification?] > > It wouldn't hurt to re-iterate it. > >> 6.1. The Account REQUEST Packet Body >> >> TACACS+ accounting is very similar to authorization. The packet >> format is also similar. >> >> * nit: that seems redundant. All of the packets use a common format. >> [The header is more consistent than the body rather than the body, is >>that what you mean?] > > I mean it would be clearer to say "both leverage a common header, with >type-specific packets using a standard layout". Sure. The original in the draft is weak. Will update. > >> * Can an accounting packet indicate an authen_type and authen_service? >>If >> so, what does that mean? >> [yes, indicating circumstances (if known) on the device] > > That should be clarified. > >> * what's a "task" ? This is the first reference in the document to a >> "task". Perhaps more explanation here as to the expected use-case >> [Added text: "TACACS+ accounting is intended to record various types of >>events on clients, for example: login sessions, command entry, and >>others as required by the client implementation. These events are >>collectively referred to in `The Draft' [TheDraft] as "tasks"."] > > That's still a little unclear to me, but OK. We can clarify, but the task concept as I see it was always pretty insubstantial, so I don¹t want to elevate it into something it wasn¹t. When it comes down to it, it is really just a way to link accounting records for a specific session. Will move the test to clarify. > >> The START and STOP flags are mutually exclusive. When the WATCHDOG >> flag is set along with the START flag, it indicates that the update >> record is a duplicate of the original START record. >> >> * why would an update duplicate the original START record? This seems >> redundant. >> [Well, here we are constrained by a clear definition form original >>draft.] > > What does it mean, tho? This document should say. i.e. portions of >the protocol should not be left as "we have no idea what this means". I think it is more a case of: it is redundant, but it was specified, so we cannot choose to change it to make it more efficient. > > If implementors don't use / support this functionality, then the >description of it can be removed from the draft. If it is used, >implementors can speak up, and help describe what this functionality does. > >> * Some more questions... >> >> * does the client retry an accounting packet if it doesn't get a >>response >> from the server? >> [client will get PASS or FAIL. do you mean, if the client gets a force >>disconnect for the TCP connection?] > > I mean, what happens if the client sends a packet, the TCP connection >stays open, and there's no response packet? > > Does the client retry? If so, when? If not, what happens? > > I guess similar comments should apply for accounting and >authorization... > > As we're not trying to extend the protocol, it may be good enough to >note that responses may not be received, and it's up to implementations >to decide what to do. Good point. As it is TCP, we can recommend: If not on an active single connect session then close the connection and create a new. Then in either (single/non single) resend the request. > >> * how often do the updates get sent? Saying "implementation defined" is >> OK, but some recommendations would be good. i.e. not more than 100x per >> second, and not less than 1 day apart (as extreme examples) >> [so lets stick with implementation defined, the range of T+ devices we >>see is so diverse and use cases are such that I would not like to >>constrain.] > > It would be good to have recommendations. e.g. "updates more than once >per second are NOT RECOMMEND, updates more than an hour apart are NOT >RECOMMENDED". > > The goal is to guide implementors to choosing values which won't cause >problems for everyone else. Sure, will recommend minimum of 5 seconds, but I¹m not sure we can specify an upper limit as it is not mandatory. > >> * I have similar questions about the TCP connection. What happens if a >> client can't connect to a server? How often should it retry? Should it >> treat "unable to connect" as "authentication / authorization fail"? >> Should the user be forbidden *all* access if the client cannot connect >>to >> the server? Should the user be given minimal access in order to be >>able >> to diagnose server connection problems? >> [I would say that the normal approach is to treat a failure to connect >>as an ERROR condition, so that any support failover or equivalent to >>method-list can be followed. Frequently a device will have a local >>option at the end of a method list, and this local method would contain >>the options for worst case access, but I would say that this behaviour >>is out-of-scope of the protocol description.] > > The draft still needs to suggest what happens when a client can't talk >to a server. Saying "treat it like an ERROR" condition" is OK. Also >"implementations may fail-over to implementation-defined backup methods". > > It's bad to have the draft be silent on the topic. Sure, will add. > >> * nit: what's a "dialstring" ? >> >> * what's a callback? How does that work? >> [These two are rather legacy, and really related to an old form of >>Network Access, so propose we can exclude] > > Sounds good. > >> * how big is the task_id? Since values can be omitted, is it OK to >>have a >> zero-length task_id? or 1 octet? >> >> * are there recommendations for the content of task_id? e.g. >>incrementing >> numbers, strings, etc. >> >> * is task_id mandatory to occur in accounting packets? >> [task_id is best practice, and I think we can promote it to mandatory, >>however this would be a change to spec. It is a text value (that can >>clearly encode a number), but it is widely enough used that I think >>restricting its validity would be difficult.] > > It's fine to say that task_id SHOULD be in all packets. >> >> * which attributes are allowed in which packets? I presume it's not >> recommended to have a START which contains "stop_time" >> [other than task_id, there are no accepted standards for this mapping] > > The draft should say so, then. > >> bytes_in >> >> The number of input bytes transferred by this action >> >> * input to where? This was poorly specified in RADIUS, and some >> implementations got it wrong... >> [Proposed text: The number of input bytes transferred by this action to >>the port] > > from the users system to the port... Well, can we always assume ³users system²?. Often there can be something between the user and the device (robots, scripts etc) (but I guess they may all be part of an overall system). I guess it would be more general to focus on the direction of the device port. > > Pedanticism helps here. Agreed! > :( We weren't pedantic in RADIUS, and multiple implementors screwed it >up. > > Alan DeKok. > _______________________________________________ OPSAWG mailing list OPSAWG@ietf.org https://www.ietf.org/mailman/listinfo/opsawg
