Stewart, thanks for your review. I have entered a DISCUSS ballot based on a couple of your points. Alissa > On May 13, 2019, at 8:54 AM, Stewart Bryant via Datatracker <noreply@xxxxxxxx> wrote: > > Reviewer: Stewart Bryant > Review result: Almost Ready > > I am the assigned Gen-ART reviewer for this draft. The General Area > Review Team (Gen-ART) reviews all IETF documents being processed > by the IESG for the IETF Chair. Please wait for direction from your > document shepherd or AD before posting a new version of the draft. > > For more information, please see the FAQ at > > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. > > Document: draft-ietf-opsawg-tacacs-13 > Reviewer: Stewart Bryant > Review Date: 2019-05-13 > IETF LC End Date: None > IESG Telechat date: 2019-05-16 > > Summary: > > There are a number of issues called out below that need addressing before publication. > > Someone needs to micro-check the text to make sure that all terms are defined and referenced. > I picked up a few, but there were a lot I did not have time to check. > > Major issues: > > SB> The IANA section should ask IANA to point to this RFC as a reference > SB> for port 49 > > ============ > > > The first MD5 hash is generated by concatenating the session_id, the > secret key, the version number and the sequence number and then > running MD5 over that stream. All of those input values are > available in the packet header, except for the secret key which is a > shared secret between the TACACS+ client and server. > > SB> MD5 make a good checksum, but I am surprised to see it used in this > SB> application in a new protocol. > > ============= > > All TACACS+ packets begin with the following 12-byte header. The > header describes the remainder of the packet: > > SB> If ever there was an error in a long term session, how > SB> how would you find in in the following packet structure? > SB> Presumably from an incorrect major version and sequence number? > > SB> Some details on the error cases and the unconditional "safety" > SB> of the protocol would be useful. > > ========== > > TAC_PLUS_AUTHEN_TYPE_ASCII := 0x01 > > TAC_PLUS_AUTHEN_TYPE_PAP := 0x02 > > TAC_PLUS_AUTHEN_TYPE_CHAP := 0x03 > > TAC_PLUS_AUTHEN_TYPE_ARAP := 0x04 (deprecated) > > TAC_PLUS_AUTHEN_TYPE_MSCHAP := 0x05 > > TAC_PLUS_AUTHEN_TYPE_MSCHAPV2 := 0x06 > > > > SB> There are lots of lists similar to the above. > SB> I have not checked them all, but a number of the types > SB> in this and subsequent parts of the design don't seem > SB> to be defined or have a definitive reference > > =========== > > The START packet MUST contain a username and the data > field MUST contain the PAP ASCII password. A PAP authentication only > consists of a username and password RFC 1334 [RFC1334] . The REPLY > from the server MUST be either a PASS, FAIL or ERROR. > > SB> Should there note be a note that RFC1334 is obsolete? > > =========== > > Minor issues: > > The use of the term "packet" as a unit of data is confusing, since the protocol > is carried over TCP which is a streaming protocol. > > They are really TACAS+ PDUs > > ========= > > (For example, Cisco uses "tty10" > to denote the tenth tty line and "Async10" to denote the tenth async > interface). > SB> Is it correct to quote a particular vendor in an RFC of this type? > > ======== > > TAC_PLUS_PRIV_LVL_MAX := 0x0f > > TAC_PLUS_PRIV_LVL_ROOT := 0x0f > > TAC_PLUS_PRIV_LVL_USER := 0x01 > > TAC_PLUS_PRIV_LVL_MIN := 0x00 > > SB> Where are these defined? > > ======== > Nits/editorial comments: > > The normative description of Legacy features such as ARAP and > SB> ARAP not expanded anywhere in document. > > ===== > > SB> telnet and rlogin need references > > ===== > is the user is connected via ISDN or a POTS, > SB> Are ISDN and POTS well known IETF terms? > > ===== > > It is not legal for an attribute name to contain either of the > separators. It is legal for attribute values to contain the > separators. > SB> Is "legal" the correct term here? > > > _______________________________________________ > Gen-art mailing list > Gen-art@xxxxxxxx > https://www.ietf.org/mailman/listinfo/gen-art
