Re: [Gen-art] Genart telechat review of draft-ietf-opsawg-tacacs-13

[Alissa Cooper <[e-mail]>]

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 <[e-mail]> 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
> [e-mail]
> https://www.ietf.org/mailman/listinfo/gen-art