Subject: [OPSAWG] draft-ietf-opsawg-tacacs-06 Contributions, Status and Plans

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Fri, 12 May 2017 18:40:07 +0000

Dear Working Group,

We wanted to respond to recent posts regarding the TACACS+ informational
draft, and update on status and intent:

1) Regarding the use of uncredited text from Alan DeKok:

It is certainly the case that Alan has spent time actively engaged in the
process of critiquing this document to improve it, and provided numerous
proposed textual suggestions,. We would be very happy to acknowledge
Alan¹s contribution to the document by adding wording that is agreeable to
Alan, in the next draft. In fact not having this acknowledgement for
Alan¹s contribution so far was an oversight, for which we apologise to
Alan.

However at this time we do not have plans to change the list of authors.
Alan: if you feel that we have exploited your suggestions too fully, such
that an acknowledgement in the document would be unsatisfactory
recompense, then we are happy to consider removing all text that you
identify, that you feel is derived too closely from your work.

2) Definition of Done

We note that there is still comments along the lines that the document is
not ready, in that the protocol is still not adequately described. We
would like to make sure that the next version does adequately describe the
protocol. 

Rather than to chase a cycle of comment/response, we¹d like to see if we
can determine what the ³Definition of Done² checklist and metrics would
be, by which we can measure that the content is be acceptable for the WG
for such a protocol as TACACS+.

For example, as a start point for this, I think we can define:

1. The packet formats: defining fields and their constraints
2. Identification of fields whose values have meaning for protocol flow.
This will include error and fail fields. The way that these fields
influence the flow must be documented.
3. Identification of the fields which have a common meaning, but are not
intended to direct protocol flow.
4. Identification of fields whose values have meaning in terms of the
deployment, which would simply be listed.

If there are other aspects of the protocol, whose absence would mean that
the protocol is not fully described, we would welcome input to help us.

3) Next Steps:

We have two next steps:

3.1) We will produce a new revision correcting the issues such as the
email address of Lol Grant and the above mentioned acknowledgement of
Alan, and incorporate lessons from 2) above.
3.2) We will provide a summary of the changes between the original draft
spec from 1998 and the new draft.

If this status/plan is not in order, please let us know.

Many thanks!

Best Regards.

The authors.

---

From: Alan DeKok <[email-masked]>
Date: Fri, 12 May 2017 15:47:18 -0400

> Alan¹s contribution so far was an oversight, for which we apologise to

  Thank you.

> However at this time we do not have plans to change the list of authors.

  I will note that document authors serve at the discretion of the WG / chairs / AD.

> identify, that you feel is derived too closely from your work.

  It would generally seem to be better to acknowledge people who have contributed substantially to the draft, instead of removing and re-writing their text.

  The point of the draft is to have a documented protocol, not to artificially limit the set of authors.

> for such a protocol as TACACS+.

  As I've suggested and others have agreed, what people want is a response to reviews.

> For example, as a start point for this, I think we can define:

  Since drafts proceed to RFC via WG consensus, I would suggest that not responding to reviews is a de facto admission that the draft does not have WG consensus.

> deployment, which would simply be listed.

  All of these topics and more are addressed in my reviews.

> the protocol is not fully described, we would welcome input to help us.

  I've given you input, which has largely been ignored.

> spec from 1998 and the new draft.

  i.e. you won't bother to respond to reviews, you want the WG to read the draft again to see if the comments have been addressed.

  Again, drafts get published based on WG consensus.  Ignoring WG consensus is just bad practice, and unproductive.

  At this point, I'm done.  I oppose any and all publication of this draft until such time as the authors can demonstrate that they've addressed concerns raised here.

  I will continue to respond to Q&A about my reviews, but I see no benefit in reviewing new versions of the draft.

  Alan DeKok.

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Sat, 13 May 2017 06:19:12 +0000

Hi Alan,

So our response to your reviews has been to incorporate, where feasible,
and where we can apply then, to the doc.

Would you have a preferred method that we responded?

Thanks.

On 12/05/2017 20:47, "Alan DeKok" <[email-masked]> wrote:

>  It would generally seem to be better to acknowledge people who have
>contributed substantially to the draft, instead of removing and
>re-writing their text.
>
>  Since drafts proceed to RFC via WG consensus, I would suggest that not
>responding to reviews is a de facto admission that the draft does not
>have WG consensus.
>
>  i.e. you won't bother to respond to reviews, you want the WG to read
>the draft again to see if the comments have been addressed.
>
>  Again, drafts get published based on WG consensus.  Ignoring WG
>consensus is just bad practice, and unproductive.
>
>
>  At this point, I'm done.  I oppose any and all publication of this
>draft until such time as the authors can demonstrate that they've
>addressed concerns raised here.
>
>  I will continue to respond to Q&A about my reviews, but I see no
>benefit in reviewing new versions of the draft.
>
>
>  Alan DeKok.
>

---

From: Alan DeKok <[email-masked]>
Date: Sat, 13 May 2017 07:59:11 -0400

> Would you have a preferred method that we responded?

  I told you my preferred method.  Others have agreed that it's the preferred method.

  If you're reading messages on this list, that question has already been answered.

  You've been given detailed reviews of the draft.  Instead of responding to the reviews, you've issued a new revision.  Then, you want the reviewers to verify that the new draft addresses their concerns.

  That's not the right approach..

  The approach in the IETF is to have authors move towards WG consensus.  i.e. to prove to  to the WG that the draft is ready for publication.

  If you're not going to work towards WG consensus, I suggest the chairs replace you with authors who will.

  Alan DeKok.

---

From: Robert Drake <[email-masked]>
Date: Sat, 13 May 2017 13:21:43 -0400

>    If you're not going to work towards WG consensus, I suggest the chairs replace you with authors who will.
That seems unnecessarily rude.

This draft is still about documenting the existing tacacs+ protocol 
right?  Why?

You've been discussing this thing for a year and can't reach a consensus 
about an existing protocol.  I doubt very strongly that a completed 
document will be useful to anyone.  Nobody needs this to implement the 
existing protocol.

My personal belief is that extending the protocol isn't a good idea 
anymore.  Instead just rewrite it to use HTTPS/TLS transport with 
JSON/XML encoding.  That should cut out about half of the documentation 
in the draft.  The new protocol doesn't have to run on the same port.  
It could use 443 or whatever the user declares in the connection URI.  
Vendors could leave their existing tacacs+ client in the code until 
people don't need it anymore.  Servers could be adapted to support both 
protocols.

The most important thing to me is that something gets created.  This 
should be moving from a thought experiment to a reference implementation 
so that people can comment on the details.

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Sat, 13 May 2017 19:03:25 +0000

Hi Alan,

So rather than directly updating the doc, we¹re looking for an individual
response to each item. That is doable, we¹ll start putting that together.

On 13/05/2017 12:59, "Alan DeKok" <[email-masked]> wrote:

>  You've been given detailed reviews of the draft.  Instead of responding
>to the reviews, you've issued a new revision.  Then, you want the
>reviewers to verify that the new draft addresses their concerns.
>
>  The approach in the IETF is to have authors move towards WG consensus.
>i.e. to prove to  to the WG that the draft is ready for publication.
>

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Sun, 14 May 2017 12:11:03 +0000

Dear Alan,

Below is a simple textual concatenation and minor (hopefully lossless)
edit of comments you kindly sent for the October revision (version 5),

We will work through this list, and reply with an item-by item response,
(In place of previous mode of updating the doc to make v6) and then
hopefully move towards a consensus for the content for version 7.

Thanks,

Regards,

Doug.

...
 Some comments.  Mostly little nits and clarifications.

  The major points for me are related to security.  The "Security
Considerations" section is almost empty, and will certainly not pass a
review from the security area.  They will in all likelihood block
publication until substantive comments have been added.

------

3.3.  Single Connect Mode

   The packet header (see below) contains a flag to allow sessions to be
   multiplexed on a connection.

* it would be good to name the flag here.

   If the server sets this flag in the first reply packet in response to
   the first packet from a client, this indicates its willingness to
   support single-connection over the current connection.  The server
   may set this flag even if the client does not set it, but the client
   is under no obligation to honor it.

* what happens if the client does not honour the flag?

   The flag is only relevant for the first two packets on a connection,
   to allow the client and server to establish single connection mode.
   The flag MUST be ignored after these two packets since the single-
   connect status of a connection, once established, must not be
   changed.  The connection must instead be closed and a new connection
   opened, if required.

  Why would it be required to change the mode?  It would be good to
explain the use-cases, and why (or not) they make sense.

   TAC_PLUS_UNENCRYPTED_FLAG := 0x01

   If this flag is set, then body encryption is not used.  If this flag
   is cleared, the packet is encrypted.  Unencrypted packets are
   intended for testing, and are not recommended for normal use.

* it would be good to have a pointer to the security considerations, with
a discussion of the implications.

*  Also, it's odd that a completely insecure mode is just "not recommended
for normal use".  I would suggest more panic-inducing phrasing.  e.g.
."Unencrypted packets offer a complete compromise of all security related
to the protocol and MUST NOT be used outside of a testing environment"

   TAC_PLUS_SINGLE_CONNECT_FLAG := 0x04

   This flag is ussequernt, ed to allow a client and server to agree
whether
   multiple sessions may be multiplexed onto a single connection.

* I find the "agree" phrasing odd.  I've mentioned before that the tone of
the document is often philosophical.  Perhaps saying instead "this flag
signals whether the client and server are capable of performing session
multiplexing"

* i.e. the flag is a signal, not a capability for the systems to agree.

   session_id

   The Id for this TACACS+ session.  The session id is to be selected
   randomly.  This field does not change for the duration of the TACACS+
   session.  (If this value is not a cryptographically strong random
   number, it will compromise the protocol's security, see  RFC 1750

* it would be good to update the paragraph to be more prescriptive.  i.e.

   The Id for this TACACS+ session.  The session id MUST be taken
   fro a cryptographically strong random number generation.  If not,
   the protocol's security will be compromised.  See  RFC 1750
   for further information

...

   length

   The total length of the packet body (not including the header).  This
   value is in network byte order.  Packets are never padded beyond this
   length.

* Why is the text on "padding" there?  The TACACS+ packets are sent over
TCP, not UDP.  So any "padding" would be outside of the scope of the
current packet, and be seen by the other end as a subsequent, but invalid,
TACACS+ packet.

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.

     - There will be no padding in any of the fields or at the end of a
      packet.

* I wonder again what "padding" means here.

3.6.  Encryption

   The body of packets may be encrypted.   ...

* Which is not a standard encryption method.  Security people will
complain here.  I suggest (as before) using the term "obfuscated".  With a
note that for historical reasons, the flags / fields / etc. refer to
TACACS+ "encryption".

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

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

  ...   The session id is used in network byte order.

* nit: if the session ID is a 32-bit opaque token, it's not in any byte
order.  Maybe just "as taken from the packet header" ?

   When a server detects that the secret(s) it has configured for the
   device mismatch, it MUST return ERROR.  The handling of the TCP
   connection by the server is implementation independent.

* I repeat my objection here.  There is just no reason for the TCP
connection to remain open after the connection has been determined to be
fraudulent.  This subject also came up in the RADIUS over TCP drafts.  The
consensus there was the same: if it's not a real / known / good client,
there is every reason to close the connection, and zero reasons to keep it
open.

   After a packet body is decrypted, the lengths of the component values
   in the packet are summed.  If the sum is not identical to the
   cleartext datalength value from the header, the packet MUST be
   discarded, and an error signalled.  The underlying TCP connection MAY
   also be closed, if it is not being used for other sessions in single-
   connect mode.

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

   If an error must be declared but the type of the incoming packet
   cannot be determined, a packet with the identical cleartext header
   but with a sequence number incremented by one and the length set to
   zero MUST be returned to indicate an error.

* this mix of clear-text and encrypted signalling opens up the protocol to
an attacker.  Since this "NAK'" is not protected, an attacker can forge
the NAK at any time, and force the connection to be closed.

* this attack (along with all others) should be noted in the Security
Considerations section.

...
   Packet fields are as follows:

   action

   This describes the authentication action to be performed.  Legal
   values are:

* what happens if a client/server receives values outside of the legal
range?  I suggest sending ERROR, and closing the TCP connection.

* similar comments apply for other fields with "legal values".  While it's
good to define what happens when things go as planned, it's also good to
define what happens when things *don't* go according to plan.

* review of 4.2 and subsequent sections will follow in a later message.

  Summary: the spec is still fairly descriptive / philosophical instead of
prescriptive.  Many optional parts of the protocol are described, "A and B
are allowed", but there is often no description of what to *do* when
either A or B is present.  I've suggested descriptive text, or places
where descriptive text can be added.

The larger worry here is what happens if the descriptive text conflicts
with existing implementations?  This is a topic which should be discussed
in the "Security Considerations" section.

  i.e. the spec documents the existing protocol, with recommendations for
security and inter-operability.  As the protocol has historically been
poorly specified, implementations are likely to follow the broad
requirements of this specification.  They are also likely to not follow
many of the newly-clarified behaviours / requirements.  The hope is that
this specification can guide implementors towards a common understanding
of the protocol.

  Also, vague requirements and open options in a protocol spec are ripe
for implementation bugs and security issues.  That worry drives many of my
comments.

-----

4.2 ...

server_msg, server_msg_len

   c A message to be displayed to the user.  This field is optional.  If
   it exists, it is intended to be presented to the user.  US-ASCII
   charset MUST be used.  The server_msg_len MUST indicate the length of
   the server_msg field, in bytes.

* The last sentence is atypical of RFC 2119 language.  Then language is
typically about system behaviour or inter-field comparisons.  I'm not sure
what it means that a field MUST indicate a length.

* i.e. the "server_msg_len" field is *defined* to encode the length of the
"server_msg" field.  No "MUST" is necessary.

* similar comments apply to many of the other "foo_len" fields, elsewhere
in the document

rem_addr, rem_addr_len

   An US-ASCII string this is a "best effort" description of the remote
   location from which the user has connected to the client.

* perhaps instead of "best effort", just re-use language from the "port"
description.  The contents of this field are the clients description of
the remote location...

4.4 ...

   When the REPLY status equals TAC_PLUS_AUTHEN_STATUS_GETDATA,
   TAC_PLUS_AUTHEN_STATUS_GETUSER or TAC_PLUS_AUTHEN_STATUS_GETPASS,
   then authentication continues and the SHOULD provide server_msg

* editorial: the WHAT should provide...

   ... quest for more information to flexibly support future
   requirements.  If the TAC_PLUS_REPLY_FLAG_NOECHO flag is set in the
   REPLY, then the user response must not be echoed as it is entered.

* the implicit assumption here is that there is a person who is entering
text on a terminal.  It would be good to have a bit more explanation of
this expected use-case.

  NOTE: Only those requests which have changed from their minor_version
   0 implementation (i.e.  CHAP, MS-CHAP and PAP authentications) will
   use the new minor_version number of 1.  All other requests (i.e.  all
   authorisation and accounting and ASCII authentication) MUST continue
   to use the same minor_version number of 0.

* that's an odd phrasing of the requirement.  Perhaps something more
prescriptive would be useful.  e.g.:

  NOTE: Authentication requests which use ASCII login or chpass MUST use
minor version 0.  Authentication requests using PAP, CHAP, or MS-CHAP MUST
  use minor version 1.  All authorization and accounting packets MUST use
minor version 0.

4.4.2...

   This section describes common authentication flows.  If the options
   are implemented, they MUST follow the description.  If the server
   does not implement an option, it will respond with
   TAC_PLUS_AUTHEN_STATUS_ERROR.

* It's not typically required to say that implementations MUST follow the
spec.

* also, perspective terminology is better than descriptive. Not "will do",
but "MUST do".

*  I suggest re-phrasing:

  This section describes common authentication flows.  If the server
   does not implement an option, it MUST respond with
   TAC_PLUS_AUTHEN_STATUS_ERROR.

   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*?

* 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"

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

* RFC 2119 language is recommended here.  "any data (if present) MUST be
ignored", or "the data field MUST NOT be present"

* I have no idea how to make these requirements compatible with existing
implementations.  Sorry...

  CHAP:

   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 16 octets).

* it would be good to recommend a minimum length for challenge.  1 octet
is clearly insufficient, but is (silently) allowed by the spec.  I suggest
at least 8 octets/

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

   To perform the authentication, the server will run MD5 over the id,
   the user's secret and the challenge, as defined in the PPP
   Authentication RFC 1334 and then compare that value
   with the response.

* again "server WILL run".  As opposed to "calculates..."

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

   ... To perform the authentication, the server will use a the algorithm
   specified  RFC2759

* editorial: "specified IN RFC ..."

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

4.4.3.  Aborting an Authentication Session

   The client may prematurely terminate a session by setting the
   TAC_PLUS_CONTINUE_FLAG_ABORT flag in the CONTINUE message.  If this
   flag is set, the data portion of the message may contain an ASCII
   message explaining the reason for the abort.

* again, what happens if the field is / is-not present?

* what does the server do with this message?  Should it be logged
somewhere?

... The session is
   terminated and no REPLY message is sent.

* what happens to the TCP connection?  Does it remain open?  It may be
good to say "The session is terminated, and the session Id is no longer
valid.  However, other sessions on the same TCP connection may continue..."

   The host is specified as either a fully qualified domain name, or an
   ASCII numeric IPV4 address specified as octets separated by dots
   ('.'), or IPV6 address test representation defined in  RFC 4291.

* editorial ".. text .. "representation, not "test"

   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

* what happens if the client gets pushed a key, and then also has a
pre-configured key?  Which one takes precedence?  Why?

  Continuing with 4.4.3

     While the order of hosts in this packet indicates a preference, but
     the client is not obliged to use that ordering.

* the use of "obliged" is unusual here.  Perhaps "the order used by the
client is implementation specific, and does not have to follow the order
sent by the server"

   If the status equals TAC_PLUS_AUTHEN_STATUS_RESTART, then the
   authentication sequence is restarted with a new START packet from the
   client. ...

* nit: it may be good to say that the next session does not have to use
the current TCP connection

* "restart" may also be ambiguous.  Does the session ID change for the
next session?  It may be good to say instead "If the status equals
TAC_PLUS_AUTHEN_STATUS_RESTART, then the client re-authenticates with a
new session, beginning with a new START packet"

...   This REPLY packet indicates that the current authen_type
   value (as specified in the START packet) is not acceptable for this
   session, but that others may be.

* Which others "may" be acceptable?  And why "acceptable for this
session"?   is the client free to re-try the same authentication type in a
different session?  i.e. what does the client *do*?

* perhaps say "the REPLY indicates a negative acknowledgement that the
current authen_type is not accepted by the server.  The client SHOULD
start a new session with a different authen_type"

   If a client chooses not to accept the TAC_PLUS_AUTHEN_STATUS_RESTART
   packet, then it is TREATED as if the status was
   TAC_PLUS_AUTHEN_STATUS_FAIL.

* why uppercase "TREATED" ?

* why "chooses not to accept" ?  again, prescriptive is better than
philosophical

* Perhaps "If the client does not start a new session after receiving a
TAC_PLUS_AUTHEN_STATUS_RESTART, it MUST behave as if it received a
TAC_PLUS_AUTHEN_STATUS_FAIL."

5.  Authorization
...

   The authorization REQUEST message contains a fixed set of fields that
   indicate how the user was authenticated or processed

* nit: remove "or processed"

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

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

* similar comments apply for other uses of "matches"

   It is not legal for an attribute name to contain either of the
   separators.  It is legal for attribute values to contain the
   separators.

* what are the legal characters for an attribute name?  The previous
paragraph says US-ASCII.  So is "($#@!" a legal attribute name?

* a reference to Section 7 would be useful here.

   Optional arguments are ones that may be disregarded by either client
   or server.  Mandatory arguments require that the receiving side
   understands the attribute and will act on it.  If the client receives
   a mandatory argument that it cannot oblige or does not understand, it
   MUST consider the authorization to have failed.

* "oblige" is an unusual word to use here.   Perhaps "If the client
receives a mandatory argument it cannot implement, it MUST consider the
authorization to have failed"

5.2 The Authorization RESPONSE Packet Body
...
   server_msg, server_msg_len

   This is an US-ASCII string that may be presented to the user.  The
   decision to present this message is client specific.

* nit: perhaps "This field is a message from the server to the client.
The client MAY display it to the user"

* saying "decision is client specific" is a long way of saying "the client
MAY do it", but doesn't always have to

...

   The attribute-value pair that describes the authorization to be
   performed.  (see below),

* That's not clear to me.  Perhaps "if present, the list of additional
authorizations permitted the user"

    If the status equals TAC_PLUS_AUTHOR_STATUS_FAIL, then the
   appropriate action is to deny the user action.

* again, a bit more philosophical than prescriptive.  Perhaps "If the
status equals TAC_PLUS_AUTHOR_STATUS_FAIL, then the requested action MUST
be denied"

   If the status equals TAC_PLUS_AUTHOR_STATUS_PASS_ADD, then the
   arguments specified in the request are authorized and the arguments
   in the response are to be used IN ADDITION to those arguments.

* uppercase "IN ADDITION" is unusual.  Perhaps:

   If the status equals TAC_PLUS_AUTHOR_STATUS_PASS_ADD, then the
   arguments specified in the request are authorized and the client MUST
   also enforce the authorization attribute-value pairs (if any) in the
response.

* similar comments apply here:

   If the status equals TAC_PLUS_AUTHOR_STATUS_PASS_REPL then the
   arguments in the request are to be completely replaced by the
   arguments in the response.

* perhaps

   If the status equals TAC_PLUS_AUTHOR_STATUS_PASS_REPL then the
   client MUST use the authorization attribute-value pairs (if any) in the
response,
   instead of any authorization attribute-value pairs from the request.

...

    If the intended action is to approve the authorization with no
   modifications, then the status is set to
   TAC_PLUS_AUTHOR_STATUS_PASS_ADD and the arg_cnt is set to 0.

* should be prescriptive.  Perhaps:

   Servers can signal to client that the authorization is approved with no
    modifications by setting the status to
   TAC_PLUS_AUTHOR_STATUS_PASS_ADD and the arg_cnt is set to 0.

...

   A status of TAC_PLUS_AUTHOR_STATUS_ERROR indicates an error occurred
   on the server.

* what does the client do then?  Perhaps "the client MAY show an error to
the user (how???), and MUST treat the response as
TAC_PLUS_AUTHOR_STATUS_FAIL"

* Also, the last sentence of the subsequent paragraph also talks about
ERROR, and seems misplaced.

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.

  ...   There is a fixed portion and an extensible
   portion.  The extensible portion uses all the same attribute-value
   pairs that authorization uses, and adds several more.

* except that the authorization section didn't list any attribute-value
pairs.  So it's hard to say that those (non-existent) pairs are re-used
here

   All other fields are defined in the authorization and authentication
   sections above and have the same semantics.

* Can an accounting packet indicate an authen_type and authen_service?  If
so, what does that mean?

6.2.  The Accounting REPLY Packet Body

   The response to an accounting message is used to indicate that the
   accounting function on the server has completed.

* what does that mean?  Do servers always implement "functions"?  As
background, RFC 2866 (RADIUS accounting) says:

...
     Upon receipt of an Accounting-Request, the server MUST transmit an
      Accounting-Response reply if it successfully records the
      accounting packet, and MUST NOT transmit any reply if it fails to
      record the accounting packet.
...

* perhaps similar text could be used here.

   ... The server will
   reply with success only when the record has been committed to the
   required level of security,

* what is a "required level of security"?  Is that indicated in the
packet?  Elsewhere?

* my recommendation is just to delete that...

   ... relieving the burden on the client from
   ensuring any better form of accounting is required.

* I find that statement awkward.  Perhaps re-using / re-phrasing text from
RFC 2866 would be good here.

6.2.  The Accounting REPLY Packet Body

* there's more duplication definition of fields here.  It may be useful to
just say "server_msg has the same meaning and behaviour as defined in
Section 4.2"

  ...    The server MUST terminate the session after sending a REPLY.

* this is the only reference to a session being terminated in the
document.  What does it mean?  Perhaps that the next REQUEST packet has to
have a different session ID?  If so, it would be good to say that in the
REQUEST section.

   The TAC_PLUS_ACCT_FLAG_START flag indicates that this is a start
   accounting message.  Start messages will only be sent once when a
   task is started.  The TAC_PLUS_ACCT_FLAG_STOP indicates that this is
   a stop record and that the task has terminated.  The
   TAC_PLUS_ACCT_FLAG_WATCHDOG flag means that this is an update record.
   Update records are sent at the client's discretion when the task is
   still running.

* 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

   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.

  ... If the START
   flag is not set, then this indicates a minimal record indicating only
   that task is still running.

* nit: perhaps delete "a minimal record indicating only".

* Some more questions...

* does the client retry an accounting packet if it doesn't get a response
from the server?

* 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)

* 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?

7.  Attribute-Value Pairs

   It is recommended that hosts be specified as a numeric address so as
   to avoid any ambiguities.

* Is this IPv6 safe?  I suspect not... but it should be stated

   Absolute times are specified in seconds since the epoch, 12:00am Jan
   1 1970.  The timezone MUST be UTC unless a timezone attribute is
   specified.

* nit: perhaps "a decimal number representing seconds since..." except
what about time zones?

* how is a time zone specified?  what format is the time?  Examples would
help here.

* nit: the section defines numbers, booleans, hosts, and time.  But no
"text" type.  RADIUS is just in the process of fixing this (20+ years
later...), which is why I noticed

* adding a data type to each attribute definition would be useful

* also, it would be good to say which attributes are mandatory, and which
are optional

   Attributes may be submitted with no value, in which case they consist
   of the name and the mandatory or optional separator.

* this sounds like the separator is optional.  Perhaps "... consist of the
name and separator only"

7.1.  Authorization Attributes

   ...
   a protocol that is a subset of a service.  An example would be any
   PPP NCP.

* nit: expand acronyms on first use.  What's a "PPP NCP" ?

   cmd

   a shell (exec) command.  This indicates the command name for a shell
   command that is to be run.  This attribute MUST be specified if
   service equals "shell".  If no value is present then the shell itself
   is being referred to.

* perhaps a bit more explanation of the use-case here. What is a "shell"
in reference to an administrative management protocol?

   routing

   A boolean. ...

* note that "routing" is defined to be a boolean, but the other attributes
don't have data types defined.  I *think* "cmd" is a textual attribute,
but it's not defined that way

   timeout

   an absolute timer for the connection (in minutes).  A value of zero
   indicates no timeout.

* probably "numeric" data type?

   autocmd

   an auto-command to run.  Used only when service=shell and cmd=NULL

* it's confusing to have an explicit NULL in "cmd".  Perhaps quotes would
clarify it here.  e.g.

  n auto-command to run.  Used only when the packet contains
attribute-values of "service=shell" and "cmd="

   noescape

   Boolean.  ...

* nit: consistent terminology.  "Boolean" ? or "A boolean ..." ??

   callback-dialstring

   Indicates that callback is to be done.  Value is NULL, or a
   dialstring.  A NULL value indicates that the service MAY choose to
   get the dialstring through other means.

* nit: explicit NULL is confusing.  Perhaps "value is a dial string, or
when omitted, indicates that the server may choose to get the value via
other means"

* nit: what's a "dialstring" ?

* what's a callback?  How does that work?

7.2.  Accounting Attributes

...

   The following new attributes are defined for TACACS+ accounting only.
   When these attribute-value pairs are included in the argument list,
   they will precede any attribute-value pairs that are defined in the
   authorization section (Section 5) above.

* "will" precede?  Perhaps "MUST precede"

   task_id

   Start and stop records for the same event MUST have matching task_id
   attribute values.  The client must not reuse a specific task_id in a
   start record until it has sent a stop record for that task_id.

* 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?

   start_time

   The time the action started ().

* nit: what are the brackets for?

   stop_time

   The time the action stopped (in seconds since the epoch.)

* which attributes are allowed in which packets?  I presume it's not
recommended to have a START which contains "stop_time"

   elapsed_time

   The elapsed time in seconds for the action.  Useful when the device
   does not keep real time.

* I'd recommend removing that last sentence.  it also indicates that the
device may not have accurate time, notwithstanding the definition of
"absolute times" at the start of Section 7.

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

* this number might be large.  Should implementations allow 16 bit
numbers? 32-bit?  64-bit?  What are the expected / recommended limits?

* i.e. implementations MUST be able to parse numbers up to 2^32, and
SHOULD be able to parse numbers up to 2^64

   status

   The numeric status value associated with the action.  This is a
   signed four (4) byte word in network byte order. 0 is defined as
   success.  Negative numbers indicate errors.  Positive numbers
   indicate non-error failures.  The exact status values may be defined
   by the client.

* This is the first (and only) time that a field is defined as non-ASCII.
That's very off.

* nit: values "may" be defined by the client?  Perhaps just saying "values
are implementation specific" would be better

   ...  Privilege Levels are ordered values from 0 to 15
   with each level representing a privilege level that is a superset of
   the next lower value.

* nit: perhaps this could be "each level is defined to be a superset of
the next lower value", instead of "representing".

   If a client uses a different privilege level scheme, then it must map
   the privilege level to scheme above.

* editorial :"... to A scheme..."

* and how is mapping done?  It might be better to just omit that sentence.

   Privilege Levels are applied in two ways in the TACACS+ protocol:

* nit: perhaps "used" instead of "applied"

      - As an argument in authorization EXEC phase (when service=shell
      and cmd=NULL), where it is primarily used to set the initial
      privilege level for the EXEC session.

* how does that work?  And what does it mean to set an initial privilege
level?

      - In the packet headers for Authentication, Authorization and
      Accounting.  The privilege level in the header is primarily
      significant in the Authentication phase for enable authentication
      where a different privilege level is required.

* again, how does that work?  Additional text would be useful.  e.g.

In typical use-cases, an administrator can perform a series of commands at
a low privilege level.  When additional privileges are required, the
administrator can authenticate at the desired level, perform a series of
commands, and the drop privilege level to the lower one.  This methodology
minimizes possible errors by using high privilege levels only when
necessary, and using low privilege levels for most commands.

* expanding on the use-case and work flow would be of great benefit.

   The use of Privilege levels to determine session-based access to
   commands and resources is not mandatory for clients, but it is in
   common use so SHOULD be supported by servers.

* and clients, presumably?  Perhaps instead, say:

It is RECOMMENDED that clients and servers use Privilege levels to signal
and control session-based access.  It is RECOMMENDED that clients permit
users to change their Privilege levels, so as to ensure that commands are
executed with the minimum Privilege level required.

9.  TACACS+ Security Considerations

* there is a lot which can be said here.  My proposal here is a 
substantial amount of text.  I'll include it in another message

  My $0.02 on the contents of the Security Considerations section.   I'm 
sure I've missed things.

  Please also comment if the suggestions here are wrong or unworkable.

----
9. Security Considerations

The protocol described in this document has been in widespread use
since specified in "The Draft" (1998).  However it does not meet
modern security standards, and faces vulnerabilities with privacy and
authenticity.

This section describes issues with the TACACS+ specification, then the
protocol, deployments, followed by recommendations for implementations.

Because of the security issues with TACACS+, the authors intend to
follow up this document with an enhanced specification of the
protocol employing modern security mechanisms.

9.1 Security of The Draft Specification

TACACS+ was originally specified in "The Draft" (1998).  However, this
draft is incomplete, and leaves key points unspecified.  As a result,
software authors have had to make implementation choices about what
should, or should not, be done in certain situations.  These
implementation choices are somewhat constrained by ad hoc
interoperability tests.  That is, all TACACS+ clients and servers
interoperate, so there is a rough consensus on how the protocol works.

While the authors have made every effort to maintain compatibility,
there is no guarantee, however, that this specification matches the
behavior of current or historical implementations.  We believe that
any incompatibilites are due to modern security requirements.  We
therefore suggest that implementors should read this document, and
verify that their implementations follow the security practices
recommended here.

9.2 Security of The Protocol

The major security issue with the TACACS+ protocol is the ad hoc
"encryption" defined in Section 3.6.  This "encryption" is more
properly called "obfuscation", and has had no security analysis.  That
is, the security or insecurity of the protocol is entirely unknown.

Attacks on cryptographic hashes are well known and are getting better
with time, as discussed in [RFC4270].  The security of the TACACS+
protocol is dependent on MD5 [RFC1321], which has security issues as
discussed in [RFC6151].  It is not known if the issues described in
[RFC6151] apply to TACACS+.  <<AD: lifted from RFC 6929 Section 11 >>

The choice of encrypting the body but not the packet header is
unusual.  The lack of security on the packet header means that an
attacker can modify the header without detection.

For example, a "session_id" can be replaced by an alternate one, which
could allow an unprivileged administrator to "steal" the authorization
from a session for a privileged administrator.  An attacker could also
update the "flags" field to indicate that one or the other end of a
connection requires TAC_PLUS_UNENCRYPTED_FLAG, which could remove what
little security is available.

What little security that exists is dependent on implementations
limiting access to known clients, and on the strenght of the secret
key.  Attacks who can guess the key or break the obfuscastion method
can gain unrestricted and undetected access to all TACACS+ traffic.
This access permits the attacker to execute any command on any system,
leaving the attacker in complete control of the network.  The negative
side effects of such a successful attack cannot be overstated.

9.2.1 Security of Authentication Sessions

There are a number of security issues with authentication sessions.
As the sessions enable the exchange of clear-text passwords, an
attacker capable of observing the unencrypted traffic or breakign the
encrypted traffic, can gain complete network access.  Similarly,
MSCHAPv1 and MS-CHAPv2 offer little additional security, while ARAP is
insecure and MUST NOT be used.  As of the publication of this
document, there has been no similar attacks on the CHAP protocol.

Section 4.4.3 permits the redirection of a session to another server
via the TAC_PLUS_AUTHEN_STATUS_FOLLOW mechanism.  As part of this
process, the secret key for a new server can be sent to the client.
This public exchange of secret keys means that once one session is
broken, it may be possible to leverage that attack to attacking
connections to other servers.  Section 9.4, below, has recommendations
for securing this portion of the protocol.

9.2.2 Security of Authorization Sessions

The primary issue with authorization sessions is that unauthenticated
authorization is allowed.  Almost as bad, authorization is permitted
where authentication was done by a third party, such as with
TAC_PLUS_AUTHEN_METH_KRB5.  This third party authentication is based
entirely on the client claiming that the user has been authenticated,
and the server trusting that request.  This practice is entirely
insecure.

There is also no way to cryptographically tie an authorization session
to a particular authentication session.  That is, when the client
indicates "authen_method" in the packet header, the authentication and
authorization sessions are tied together implicitly by the contents of
the other fields, such as "use", "port", "rem_addr", etc.

<<AD: it would be good for Section 5.1 to contain recommendations on
how the server can use these fields to tie together authentication /
authorization sessions.  Perhaps recommend that all AAA include a
"task_id"? >>

The specification allows for the exchange of attribute-value pairs.
While a few such attributes are defined here, the protocol is
extensible, and vendors can define their own attributes.  There is no
registry for such attributes, and in the absence of a published
specification, no way for a client or server to know the meaning of a
new attribute.

As a result, vendors SHOULD NOT define new attribute-value pairs,
unless such pairs are used only to communicate between client and
server implementations both written by the same vendor.

The "cmd" and "cmd-arg" attributes define shell commands and
arguments, but does not specify what these are.  The specific
commands, therefore, are undefined.

9.2.3 Security of Accounting Sessions

The security considerations for accounting sessions are largely the
same as for authorization sessions.  This section describes additional
issues specific to accounting sessions.

There is no way in TACACS+ to signal that accounting is required.
There is no way for a server to signal a client how often accounting
is required.  The accounting packets are received solely at the
clients discretion.  Adding such functionality would assist with
auditing of user actions.

The "task_id" field is defined only for accounting packets, and not
for authentication or authorization packets.  As such, it is difficult
to correlate accounting data with a previous authentication or
authorization request.

<<AD: re-use the recommendations suggested for Section 5.1, above? >>

9.4 Security of Deployments

Due to the above concerns with the protocol, it is critical that it be
deployed in a secure manner.

Systems using TACACS+ MUST be configured so that TACACS+ traffic is
secured.  Any user or service who is not interacting with the TACACS+
protocol MUST NOT be able to observe TACACS+ traffic, even when
TACACS+ "encryption" is used.

When used inside of enterprises, TACACS+ traffic MUST be sent over
management networks.  When used on the wider internet, TACACS+ traffic
MUST be secured via a modern security protocol such as TLS or IPSec.
As this specification describes the historic protocol, recommendations
on TLS and IPSec parameters are outside of the scope of this document.

9.4 Recommendations for Implementations

The following recommendations are for clients and servers which
implement the TACACS+ protocol.  These recommendations describe end
user interaction or user interface design.  While not part of the
protocol itself, good designs can prevent insecure configurations.

Clients and servers MUST default to rejecting all connections which
have the TAC_PLUS_UNENCRYPTED_FLAG set.  Clients and servers MAY
permit such connections when a debugging or test flag is set.  Clients
and servers MUST NOT run with this flag set in normal environments.

Similarly, the default for both clients and servers MUST be to require
a secret key when configuring a new server or client.  The secret key
MUST NOT be empty, and SHOULD be at least eight (8) characters in
length.  Clients and servers SHOULD permit the user of keys unique to
each server or client which they are connecting to.  They SHOULD warn
the administrator if a secret key is re-used.  Documentation for
implementations SHOULD recommend the use of strong secret keys.

Servers should maintain list of known clients, and reject connections
which originate from other systems.  Servers SHOULD NOT permit the
configuration of wildcard clients.  e.g. clients which map to a
network range.

Where a client or server detects that the secret key is incorrect, it
SHOULD terminate the underlying TCP connection, and log a message to
the system administrator.  There is no good reason to keep a
connection open when the systems are unable to communicate in a
meaningful fashion.

Implementions MUST default to using TAC_PLUS_AUTHEN_TYPE_CHAP for
authentication.  We recognize, however, that this authentication
method is incompatible with many password storage systems.  As a
result, other authentication methods are still in the protocol, and
can be used so long as administrators recognize the security issues
with their use.

Clients MUST default to treating TAC_PLUS_AUTHEN_STATUS_FOLLOW as if
TAC_PLUS_AUTHEN_STATUS_FAIL had been received instead.  Clients
SHOULD allow administrators to explicitly enable the
TAC_PLUS_AUTHEN_STATUS_FOLLOW functionality.  When the functionality
is enabled, clients SHOULD permit administrators to specify ranges of
addresses, and/or sets of hosts, along with corresponding secret keys,
for which redirection will be permitted.  Clients SHOULD ignore
redirects to hosts which are outside of the pre-configured range or
list.  Clients SHOULD ignore any key provided via
TAC_PLUS_AUTHEN_STATUS_FOLLOW, and SHOULD instead use a preconfigured
key for that host.

For authorization, servers SHOULD default to requiring
TAC_PLUS_AUTHEN_METH_TACACSPLUS.  Servers SHOULD track authentication
sessions via methods "to be described" in Section 5.1.

Where a server receives an unknown attribute-value pair, it SHOULD
reply with TAC_PLUS_AUTHOR_STATUS_FAIL.  Where a server receives an
unrecognized values in authorization attributes such as "cmd",
"cmd-arg", "acl", etc., the server SHOULD reply with
TAC_PLUS_AUTHOR_STATUS_FAIL.  When a client receives an unknown
authorization attribute, it SHOULD behave as if it had received
TAC_PLUS_AUTHOR_STATUS_FAIL.

---

From: Alan DeKok <[email-masked]>
Date: Sun, 14 May 2017 10:06:54 -0400

> On May 13, 2017, at 3:03 PM, Douglas Gash (dcmgash) <[email-masked]> wrote:
> 
> 

  The point is to explain *why* the review was accepted or rejected.  i.e. to have a discussion around the topic.

>> and where we can apply then, to the doc.

  Which items were incorporated?

  Which items were *not* incorporated?  Why were they not incorporated?

  There is no need to respond to each item individually.  Grouping things together is fine.

  But when there are questions, they should be answered.  When comments are rejected, there should be an explanation.

  My larger issue with the review process so far is that the existing implementors haven't reviewed the document.  So we have no idea whether or not it describes the protocol they've implemented, or the choices they've made.

  Alan DeKok.

---

From: "Blumenthal, Uri - 0553 - MITLL" <[email-masked]>
Date: Sun, 14 May 2017 14:40:17 +0000

The "normal" expected accepted customary way of responding to comments is replying to the list with something like: 

To address the comment <x>, the new document (page#, section, paragraph) says "blah-blah-blah".

Or 

We decided not to make changes requested by comment <x> because of blah-blah-blah. 

Regards,
Uri

Sent from my iPhone

> On May 14, 2017, at 10:08, Alan DeKok <[email-masked]> wrote:
> 
> 
> _______________________________________________
> OPSAWG mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/opsawg

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Sun, 14 May 2017 14:59:44 +0000

Just to clarify: because the last batch of comments were on v5, we will
actually take that as the datum for next version.

Although the current upload (v6) can be folded into this process, what we
can actually do now is is respond to all comments in context of v5 and,
with ensuing discussion of those points, can hopefully get us to a
agreeable version of the document for v7.

On 14/05/2017 15:06, "Alan DeKok" <[email-masked]> wrote:

>  The point is to explain *why* the review was accepted or rejected.
>i.e. to have a discussion around the topic.

Agreed. That should come out of this process.

>  There is no need to respond to each item individually.  Grouping things
Sure, we can group them, we have them in the order you posted them mainly
to try to make sure we don’t miss any,

>
>  But when there are questions, they should be answered.  When comments
>are rejected, there should be an explanation.

So to recap, this process will take v5 and your comments to v5, then
hopefully we should have a transparent process getting to a v7 which we
can agree on. We can leverage v6 where it aligns with this process.

>
>  My larger issue with the review process so far is that the existing
>implementors haven't reviewed the document.  So we have no idea whether
>or not it describes the protocol they've implemented, or the choices
>they've made.

So let’s work through the document and identify what the core should be.
In the end we should have a protocol which clarifies the syntax of
interoperation whilst leaving the flexibility as needed for the varying
deployment implementations. Implementors have made very useful comments
and discussions already in the WG, and hopefully will contribute to the
discussions which will be raised by the response to your comments and the
ensuing move to closure.

Regards,

Doug.

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Sun, 14 May 2017 15:03:23 +0000

Agreed, we’re working on providing that for each of Alan’s comments made
for v5, and after suitable discussion and agreement, can get to next
version.

On 14/05/2017 15:40, "Blumenthal, Uri - 0553 - MITLL" <[email-masked]>
wrote:

>The "normal" expected accepted customary way of responding to comments is
>replying to the list with something like:
>

---

From: Ignas Bagdonas <[email-masked]>
Date: Mon, 15 May 2017 17:00:36 +0100

Hi Alan,

>    If you're not going to work towards WG consensus, I suggest the chairs replace you with authors who will.

WG chairs can appoint or change authors if needed under the process 
described in RFC7221 and its referenced documents. The individual draft 
has been accepted as a WG one a while ago with no changes in author 
list. If current document authors would like to make any changes to 
author/co-author/editor list WG chairs will certainly approve those 
changes. Otherwise unless there is clear evidence that current authors 
cannot make progress with the document, WG chairs do not have intentions 
of changing the author list. This decision may be revisited if evidence 
of author/co-author/editor duties not being performed to the expected 
level surfaces, but at this time there is no such evidence. The process 
of progressing the document is slow, slower than it could have been, but 
it is not stalled.

Thank you.

Ignas

---

From: t.petch <[email-masked]>
Date: Mon, 15 May 2017 18:00:37 +0100

----- Original Message -----
From: "Ignas Bagdonas" <[email-masked]>
To: "Alan DeKok" <[email-masked]>
Sent: Monday, May 15, 2017 5:00 PM

> >    If you're not going to work towards WG consensus, I suggest the
chairs replace you with authors who will.

Ignas

I echo part of what Alan says, that for a WG document, the editors
should reflect the consensus of the WG.  The problem I see is the lack
of consensus, not with people disagreeing, but with an absence of people
agreeing.

Alan made a number of comments in October last year, Alexander made some
in  November but I did not see much follow up from anyone else to either
set of comments.

Trouble is, do the editors incorporate comments that one person has made
and noone else has agreed or disagreed with?  There is no good answer.

In other WGs, I have seen ping-pong, one person comments, comments
incorporated, someone else then disagrees, disagreements incorporated
into a new revision, first person comes back, changes incorporated into
a newer revision and so on, circling around a lack of consensus.
Changing editors, unless it is to someone remote from the subject, is
unlikely to change things..

I did look at Alan's comments, agreed with some, disagreed with others,
ditto Alexander's, but was disinclined to do more with noone else
chipping in, especially as several more did chip in in the initial
stages of should we adopt this, and what status should it be.

How you stir people into life is a challenge for WG chairs.

Tom Petch

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Mon, 15 May 2017 18:10:00 +0000

The lack of interactivity was more our fault than any one else's, we took
Alan¹s comments and incorporated them into the version we uploaded in Feb.

What we should have done was collate Alan¹s comments to promote
discussion. We¹re attempting to rectify than that now:

1) We put Alan¹s comments on v5 put on a single mail list a few days ago
2) We are going to give an initial response ASAP (will take a few days)
3) More discussions will ensure, and other comments on the doc may be
generated
4) We will hopefully steer towards a consensus that will feed into v7.

Regards,

Doug.

---

From: Tianran Zhou <[email-masked]>
Date: Tue, 16 May 2017 01:18:40 +0000

Thanks Tom for pointing this out.
We appreciate any review comment. That really helps to improve the document.
I think it's back on the right track now. Hopefully, the authors can respond to more interactions. 

Regards,
Tianran
> -----Original Message-----
> From: t.petch [mailto:[email-masked]]
> Sent: Tuesday, May 16, 2017 1:01 AM
> To: Alan DeKok; Ignas Bagdonas
> Cc: Douglas Gash (dcmgash); [email-masked];
> [email-masked]; [email-masked];
> [email-masked]
> Subject: Re: [OPSAWG] draft-ietf-opsawg-tacacs-06 Contributions, Status
> and Plans
> 
> I echo part of what Alan says, that for a WG document, the editors should
> reflect the consensus of the WG.  The problem I see is the lack of consensus,
> not with people disagreeing, but with an absence of people agreeing.
> 
> incorporated, someone else then disagrees, disagreements incorporated into
> a new revision, first person comes back, changes incorporated into a newer
> revision and so on, circling around a lack of consensus.
> Changing editors, unless it is to someone remote from the subject, is unlikely
> to change things..
> 
> ditto Alexander's, but was disinclined to do more with noone else chipping
> in, especially as several more did chip in in the initial stages of should
> we adopt this, and what status should it be.
>

---

From: Alan DeKok <[email-masked]>
Date: Tue, 16 May 2017 08:36:30 -0400

On May 15, 2017, at 12:00 PM, Ignas Bagdonas <[email-masked]> wrote:
> WG chairs can appoint or change authors if needed under the process described in RFC7221 and its referenced documents.

  Referencing the rules is nice.  Addressing my comments would be nicer.

> ...  Otherwise unless there is clear evidence that current authors cannot make progress with the document,

  My issue isn't "progress".  My issue is addressing reviews raised on the list.

> WG chairs do not have intentions of changing the author list. This decision may be revisited if evidence of author/co-author/editor duties not being performed to the expected level surfaces, but at this time there is no such evidence.

   I don't see how reviews (which were ignored) can be construed as "no evidence" that the authors were ignoring reviews.

  Which was my point.  If document authors issue new revs irrespective of what the WG suggests, the chairs should replace the authors with ones who work towards WG consensus.

  The alternative is to accept a draft as a WG document, and then to allow the authors to do pretty much whatever they want, and then to rubber-stamp the final document as an RFC.

> The process of progressing the document is slow, slower than it could have been, but it is not stalled.

  I never claimed it was stalled.  I claimed that the authors had ignored reviews.

  I just don't understand what point you were trying to make here.  The only subjects you addressed were ones I hadn't raised.

  Alan DeKok.

---

From: Alan DeKok <[email-masked]>
Date: Tue, 16 May 2017 08:46:43 -0400

> On May 15, 2017, at 1:00 PM, t.petch <[email-masked]> wrote:

  The only agreement I see was at one of the early WGLCs.  There was wide-spread support for publication of the draft, when it was just not ready.

  i.e. if it had been approved by the WG and gone to Security Area or Transport Area review, it would have been bounced back with comments much like the ones I've made.

> and noone else has agreed or disagreed with?  There is no good answer.

  People argue on the list...

> a newer revision and so on, circling around a lack of consensus.

  Chairs / ADs should make a final decision in such a situation.  Or, the document should be dropped as having insufficient consensus.

> unlikely to change things..

  The problem I see is that most people supporting the document are just too close to it.  There has been wide-spread support for publication when it just wasn't ready.

  Plus, (speaking as an implementor) I had no idea what to do when reading early versions of the draft.

  And other implementors aren't speaking up and saying what they've done, or how to address issues in the document.

  It really does look like the majority of supporters want a TACACS RFC, but they have no opinion about what the content should be.  For me, that raises questions about why we need a TACACS RFC.

> stages of should we adopt this, and what status should it be.

  Part of the problem is that it's hard to have an ongoing conversation around my reviews.  The comments are so numerous that discussing each one publicly would require hundreds of messages.

  Which for me is a sign that the draft is just nowhere near ready for publication.

  Alan DeKok.

---

From: Alan DeKok <[email-masked]>
Date: Tue, 16 May 2017 09:15:14 -0400

Thanks.

  Alan DeKok.

---

From: Ignas Bagdonas <[email-masked]>
Date: Tue, 16 May 2017 16:35:46 +0100

Alan,

>    Which was my point.  If document authors issue new revs irrespective of what the WG suggests, the chairs should replace the authors with ones who work towards WG consensus.

No-one has seen the -07 revision yet, therefore it seems to be too early 
to make judgement whether comments and suggestions were or were not 
addressed. Authors have promised to address the raised comments 
discussing them on the list and responding to previous reviews.

I certainly agree with you that comments were not addressed. That is 
history now and reiterating this topic will do nothing to change what 
has already happened. Progress will happen if comments get addressed. 
Authors do have a token on this now.

>    The alternative is to accept a draft as a WG document, and then to allow the authors to do pretty much whatever they want, and then to rubber-stamp the final document as an RFC.

That is not how the IETF works. I fail to see what else could be 
commented here.

>    I just don't understand what point you were trying to make here.  The only subjects you addressed were ones I hadn't raised.

The point of my mail was on the changes to the list of authors. To 
summarize - changes to author list are not happening at this time. 
Important part to emphasize is "at this time".

Ignas

---

From: Alan DeKok <[email-masked]>
Date: Tue, 16 May 2017 12:22:56 -0400

On May 16, 2017, at 11:35 AM, Ignas Bagdonas <[email-masked]> wrote:
> No-one has seen the -07 revision yet, therefore it seems to be too early to make judgement whether comments and suggestions were or were not addressed. Authors have promised to address the raised comments discussing them on the list and responding to previous reviews.

  My comments (again) were that there was a history of ignoring the reviews.  At the time I made that comment, it was true.  I find it troubling to see a response saying there's "no evidence" of such a problem.

> That is not how the IETF works. I fail to see what else could be commented here.

  I'm saying until I complained, that *was* largely the process being used in this WG.

  Alan DeKok.

---

From: "Blumenthal, Uri - 0553 - MITLL" <[email-masked]>
Date: Tue, 16 May 2017 16:27:22 +0000

On 5/16/17, 11:35 AM, "OPSAWG on behalf of Ignas Bagdonas" <[email-masked] on behalf of [email-masked]> wrote:

    addressed. 

I don’t think you got the point. Which was:

Upon receiving the review (especially if it was posted to the mailing list) the authors are supposed to reply to that mailing list with something like:

“In response to the comment <X> we are changing the text in the document to say <Y>”. 

If it is done at the time (or after the time) when the edits are applied, then adding “page #, section #, paragraph” to the above.

Or “We decided not to make a change requested by the comment <X> because of <Y>”

It should not be a matter of “judgment” – the authors have to explicitly provide that info. Then the WG can discuss and/or decide if the proposed changes adequately address the comments.

What is so difficult about this?

    Authors have promised to address the raised comments 
    discussing them on the list and responding to previous reviews.

I’m waiting for emails like this to stop appearing, and emails addressing the above to start appearing.

---

From: t.petch <[email-masked]>
Date: Tue, 16 May 2017 20:18:44 +0100

Doug

When I look at the I-D -06, I am struck by two 'procedural' flaws, at
the beginning and the end of the I-D, where I am so used to a document
shepherd saying 'yes, that has been done' that I cannot recall when last
I saw an I-D, even at an early stage of the WG process, which had not
got these points right.  Mmm; it ought not to matter, but when I see
them, well it makes me back off a little.

Have a look and see if you see what I see - if not, I will let you know.

And my Reply All to Ignas gets an SMTP bounce
"5.1.0 - Unknown address error 550-'5.1.1 <[email-masked]>... User
unknown'"

Probably part of the expansion of
[email-masked] <[email-masked]>

Mmmm2

Tom Petch

----- Original Message -----
From: "Douglas Gash (dcmgash)" <[email-masked]>
To: "t.petch" <[email-masked]>; "Alan DeKok"
<[email-masked]>; "Ignas Bagdonas" <[email-masked]>
Cc: <[email-masked]>; <[email-masked]>;
<[email-masked]>; <[email-masked]>
Sent: Monday, May 15, 2017 7:10 PM

The lack of interactivity was more our fault than any one else's, we
took
Alan¹s comments and incorporated them into the version we uploaded in
Feb.

Regards,

Doug.

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Tue, 16 May 2017 19:27:49 +0000

Hi Tom,

I’d be grateful if you could let me know ;-)

---

From: "Blumenthal, Uri - 0553 - MITLL" <[email-masked]>
Date: Tue, 16 May 2017 19:45:13 +0000

On 5/16/17, 3:27 PM, "OPSAWG on behalf of Douglas Gash (dcmgash)" <[email-masked] on behalf of [email-masked]> wrote:

    >took Alan¹s comments and incorporated them into the version we uploaded in
    >Feb.
    >

Doug, IMHO it is less critical to put collated Alan’s comments on the list. We’ve all already seen Alan’s comments, so collating them into a single mail would be of a marginal usefulness.

What the public needs is *your answers to each of those comments* being put on the list. As I said before: “foreach (comment): comment -> change in the draft text” or “comment -> reasons not to address”. 

    >2) We are going to give an initial response ASAP (will take a few days)

That would be fine, IMHO. 

My concern (and displeasure) is that in the past few days we’ve seen plenty of emails on this subject, but nothing along the lines of the above – which IMHO is the only thing that would actually matter (everything else is just waste of bandwidth).

    >4) We will hopefully steer towards a consensus that will feed into v7.

Yes and yes.

---

From: Randy Presuhn <[email-masked]>
Date: Tue, 16 May 2017 12:56:24 -0700

Hi -

Uri is said to have said:

...
> My concern (and displeasure) is that in the past few days
 > we’ve seen plenty of emails on this subject, but nothing
 > along the lines of the above – which IMHO is the only thing
 > that would actually matter (everything else is just waste
 > of bandwidth).
...

YES, YES, YES, YES, YES!!!!!!

Randy

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Tue, 16 May 2017 20:06:26 +0000

Hi,

First batch of responses. This is in HTML encoding with intent to make responses easier to see due to their colour coding. If this does not work, please LMK, I will send a different format.

Many items are marked with just [Agree], if it seems there is a trivial way to adjust according to the comment.

The final batch of initial responses will be sent in a few days. All may prefer to wait for final batch.

The security section will be dealt with on a separate thread.

Regards, Doug.

------

3.3.  Single Connect Mode

* it would be good to name the flag here:
[Agreed. New Version:
"Single Connection Mode is intended to improve performance by allowing
   a client to multiplex multiple session on a single TCP connection.

   The packet header contains the TAC_PLUS_SINGLE_CONNECT_FLAG used by
   the client and server to negotiate the use of Single Connect Mode."]

* what happens if the client does not honour the flag?
[It will close the connection after the session completes. Update:
   "The server may set this flag even if the client does not
   set it, but the client may ignore the flag and close the connection
   after the session completes."]

  Why would it be required to change the mode?  It would be good to
explain the use-cases, and why (or not) they make sense.
[We mandate it must not happen. To be honest, I’m not aware of any use case for wanting to change]

[Agreed]

[Partial Agree: Will change to MUST NOT, but do not want to give impression that MD5 is secure.]

* I find the "agree" phrasing odd.  I've mentioned before that the tone of
the document is often philosophical.  Perhaps saying instead "this flag
signals whether the client and server are capable of performing session
multiplexing"
[Agree. Changed to "This flag is used to allow a client and server to negotiate Single
   Connection Mode."]

[Agree]

   session_id

* it would be good to update the paragraph to be more prescriptive.  i.e.
[Agree, changed text to: "The Id for this TACACS+ session.  This field does not change for the
   duration of the TACACS+ session.  This number MUST be generated by a
   cryptographically strong random number generation method.  Failure to
   do so will compromise security of the session.  For more details
   refer to RFC 1750 [RFC1750]"]

...

   length

* Why is the text on "padding" there?  The TACACS+ packets are sent over
TCP, not UDP.  So any "padding" would be outside of the scope of the
current packet, and be seen by the other end as a subsequent, but invalid,
TACACS+ packet.
[Agree: Removed part about padding]

3.5 ...

* 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]

[Agree, removed]

3.6.  Encryption

* Which is not a standard encryption method.  Security people will
complain here.  I suggest (as before) using the term "obfuscated".  With a
note that for historical reasons, the flags / fields / etc. refer to
TACACS+ "encryption".
[Agreed, changed generally to obfuscation, apart from where field names are directly referenced]

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

* 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]

[Agree]

[see next]

[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]

* this mix of clear-text and encrypted signalling opens up the protocol to
an attacker.  Since this "NAK'" is not protected, an attacker can forge
the NAK at any time, and force the connection to be closed.
[Agree, this is probably one of the ideas that is supported by the old spec that we can drop/ The more that we can clean it out, the better. Will add to the list of deprecated features.]

...

   action

* what happens if a client/server receives values outside of the legal
range?  I suggest sending ERROR, and closing the TCP connection.
[Agreed, though we have a section "Treatment of Enumerated Protocol Values", so that we don't need to repeat it for each item]

* similar comments apply for other fields with "legal values".  While it's
good to define what happens when things go as planned, it's also good to
define what happens when things *don't* go according to plan.
[Agreed, the section "Treatment of Enumerated Protocol Values" should mop this all up.]

[Agreed, in general, I believe we can document the positive cases, and to stipulate in one place what occurs if values are not present in case of server, or client]

-----

4.2 ...

* i.e. the "server_msg_len" field is *defined* to encode the length of the
"server_msg" field.  No "MUST" is necessary.
[Agree, will remove MUST].

[Will apply universally]

rem_addr, rem_addr_len

* perhaps instead of "best effort", just re-use language from the "port"
description.  The contents of this field are the clients description of
the remote location...
[Agree: best effort removed. Proposed New Text: "   An US-ASCII string indicating the remote location from which the user
   has connected to the client.  It is intended to hold a network
   address if the user is connected via a network, a caller ID is the
   user is connected via ISDN or a POTS, or any other remote location
   information that is available.  This field is optional (since the
   information may not be available).  The rem_addr_len indicates the
   length of the user field, in bytes."]

4.4 ...

* editorial: the WHAT should provide...
[Agreed. Answer is of course: "server"]

* the implicit assumption here is that there is a person who is entering
text on a terminal.  It would be good to have a bit more explanation of
this expected use-case.
[Agreed. T+ is used in many and varied use case, "echoed" is too restrictive, and should be replaced by something like "disclosed in a user interface"]

  NOTE: Authentication requests which use ASCII login or chpass MUST use
minor version 0.  Authentication requests using PAP, CHAP, or MS-CHAP MUST
  use minor version 1.  All authorization and accounting packets MUST use
minor version 0.
[Agreed: will revamp this section].

4.4.2...

[Agreed]

[Agreed]

*  I suggest re-phrasing:

* 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."]

* 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]

* what happens if there is data in the fields?  Is it ignored?
[Agreed From perspective of protocol, yes]

* I have no idea how to make these requirements compatible with existing
implementations.  Sorry...
[I'm not sure though, that it is relevant to the protocol specification.]

  CHAP:

[See next response]

* 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?]

[Agreed]

MSCHAPv1 / MSCHAPv2:

* 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?]

[agreed]

* 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?]

* again, what happens if the field is / is-not present?
[Agree, We will have a lack of information regarding the reason for the abort. this seems an obvious impact, but we can add]

* 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]

... The session is

[Agreed]

[Agreed]

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

* what happens if the client gets pushed a key, and then also has a
pre-configured key?  Which one takes precedence?  Why?
[... this is the reason why i want to cull this section

Lets drill into these only if we decide this section is worth keeping.]

  Continuing with 4.4.3

* Perhaps "If the client does not start a new session after receiving a
TAC_PLUS_AUTHEN_STATUS_RESTART, it MUST behave as if it received a
TAC_PLUS_AUTHEN_STATUS_FAIL."
[.... end of the section that should probably be culled]

5.  Authorization
...

[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]

...

* 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]

* 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?]

[Agreed]

* "oblige" is an unusual word to use here.   Perhaps "If the client
receives a mandatory argument it cannot implement, it MUST consider the
authorization to have failed"
[Agreed, we will replace "obliged" and clean up]

* saying "decision is client specific" is a long way of saying "the client
MAY do it", but doesn't always have to
[it was shorter :) but less consistent. Agreed]

...

[will clarify]

[Agreed]

* perhaps

...

...

* 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?]

* except that the authorization section didn't list any attribute-value
pairs.  So it's hard to say that those (non-existent) pairs are re-used
here
[All the attributes are in section 7. 1 for author, 7.2 for accounting]

* 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]

...

* perhaps similar text could be used here.
[agreed. Text is: "  The purpose of accounting is to record the action that has occurred
   on the client.  The server MUST reply with success only when the
   accounting request has been recorded.  If the server did not record
   the accounting request then it MUST reply with ERROR."]

   ... The server will

* I find that statement awkward.  Perhaps re-using / re-phrasing text from
RFC 2866 would be good here.
[Removed this text, relying on the text in the previous response]

* this is the only reference to a session being terminated in the
document.  What does it mean?  Perhaps that the next REQUEST packet has to
have a different session ID?  If so, it would be good to say that in the
REQUEST section.
[Propose add a whole section, earlier in the doc, on session completion: "3.4 Session Completion

   The REPLY packets defined for the packets types in the sections below
   (Authentication, Authorization and Accounting) contain a status
   field.  The complete set of options for this field depend upon the

   packet type, but all three REPLY packet types define values
   representing PASS, ERROR and FAIL, which indicate the last packet of
   a regular session (one which is not aborted).

   The server responds with a PASS or a FAIL to indicate that the
   processing of the request completed and the client can apply the
   result (PASS or FAIL) to control the execution of the action which
   prompted the request to be sent to the server.

   The server responds with an ERROR to indicate that the processing of
   the request did not complete.  The client can not apply the result
   and it MUST behave as if the server could not be connected to.  For
   example, the client try alternative methods, if they are available,
   such as sending the request to a backup server, or using local
   configuration to determine whether the action which prompted the
   request should be executed.

   Refer to the section (Section 4.4.3) on Aborting Authentication
   Sessions for details on handling additional status options .

   When the session is complete, then the TCP connection should be
   handled as follows, according to whether Single Connect Mode was
   negotiated:

   If Single Connection Mode was not negotiated, then the connection
   should be closed

   If Single Connection Mode was enabled, then the connection SHOULD be
   left open (see section (Section 3.3) ), but may still be closed after
   a timeout period to preserve deployment resources

   If Single Connection Mode was enabled, but an ERROR occurred due to
   connection issues (such as an incorrect secret, see section
   (Section 3.7) ), then any further new sessions MUST NOT be accepted
   on the connection.  If there are any sessions that have already been
   established then they MAY be completed.  Once all active sessions are
   completed then the connection MUST be closed."]

* 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"."]

* why would an update duplicate the original START record?  This seems
redundant.
[Well, here we are constrained by a clear definition form original draft.]

  ... If the START

[Agreed]

* 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?]

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

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

7.  Attribute-Value Pairs

* Is this IPv6 safe?  I suspect not... but it should be stated
[Agreed. Proposed new Text: “It is recommended that hosts be specified as a IP address so as to avoid any ambiguities.  ASCII numeric IPV4 address are specified as octets separated by dots ('.'), IPV6 address text representation in RFC 4291.”]

* nit: perhaps "a decimal number representing seconds since..." except
what about time zones?
[Agreed. Proposed new Text: “Absolute times are specified in seconds since the epoch, 12:00am Jan 1 1970.  The timezone MUST be UTC unless a timezone attribute is specified.”]

[Agreed. Will ]

* nit: the section defines numbers, booleans, hosts, and time.  But no
"text" type.  RADIUS is just in the process of fixing this (20+ years
later...), which is why I noticed
[There is some description of text encoding as a whole (see below), but can add something specific for attributes “3.6.  Text Encoding All text fields in TACACS+ MUST be US-ASCII, excepting special consideration given to user field and data fields used for passwords.]

[Agreed]

* also, it would be good to say which attributes are mandatory, and which
are optional
[Agreed. The list of mandatory attributes is very small, and there are some dependencies]

* this sounds like the separator is optional.  Perhaps "... consist of the
name and separator only"
[Agreed, will clean to remove this dependency]

   ...

[Agreed, will expand]

   cmd

* perhaps a bit more explanation of the use-case here. What is a "shell"
in reference to an administrative management protocol?
[Agreed, will add. In fact, now that T+ is not used for Network access and only for Device Management, this could do with rather more description. Proposed text:
“  a shell (exec) command.  This indicates the command name of the
   command that is to be run.  The "cmd" attribute MUST be specified if
   service equals "shell".

   Authorization of shell commands is a common use-case for the TACACS+
   protocol.  Command Authorization generally takes one of two forms:
   session-based and command-based.

   For session-based shell authorization, the "cmd" argument will have
   an empty value.  The client determines which commands are allowed in
   a session according to the arguments present in the authorization.

   In command-based authorization, the client requests that the server
   determine whether a command is allowed by making an authorization
   request for each command.  The "cmd" argument will have the command
   name as its value.

   cmd-arg

   an argument to a shell (exec) command.  This indicates an argument
   for the shell command that is to be run.  Multiple cmd-arg attributes
   may be specified, and they are order dependent.”

   routing

   A boolean. ...

* note that "routing" is defined to be a boolean, but the other attributes
don't have data types defined.  I *think* "cmd" is a textual attribute,
but it's not defined that way
[Agreed, along with the similar comment above]

   timeout

[Definately]

   autocmd

  n auto-command to run.  Used only when the packet contains
attribute-values of "service=shell" and "cmd="
[Agreed. The text in comment for command should help. Proposed new Text:
“   an auto-command to run.  Applicable only to session-based shell
   authorization.”]

   noescape

   Boolean.  ...

* nit: consistent terminology.  "Boolean" ? or "A boolean ..." ??
[Agreed. Or even: consistent indefinite article employment.]

   callback-dialstring

* nit: explicit NULL is confusing.  Perhaps "value is a dial string, or
when omitted, indicates that the server may choose to get the value via
other means"
[Agreed. Proposed Text: “Indicates that callback is to be done.  Value is a dialstring, or empty.  Empty value indicates that the service MAY choose to get the dialstring through other means.”]

* 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]

...

[Agreed]

   task_id

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

   start_time

* nit: what are the brackets for?
[Will update as for stop_time]

   stop_time

* 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]

   elapsed_time

[Agreed]

   bytes_in

* 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]

   status

* This is the first (and only) time that a field is defined as non-ASCII.
That's very off.
[I agree, however, we can change this in the follow up doc]

[Will update]

* nit: perhaps this could be "each level is defined to be a superset of
the next lower value", instead of "representing".
[or even more simply: each level is a superset of the next lower value]

* expanding on the use-case and work flow would be of great benefit.
[Agreed, please see text below]

It is RECOMMENDED that clients and servers use Privilege levels to signal
and control session-based access.  It is RECOMMENDED that clients permit
users to change their Privilege levels, so as to ensure that commands are
executed with the minimum Privilege level required.
[Proposed new section: “  The TACACS+ Protocol supports flexible authorization schemes through
   the extensible attributes.

   One scheme is built in to the protocol and has been extensively used
   for Session-based shell authorization: Privilege Levels.  Privilege
   Levels are ordered values from 0 to 15 with each level is a
   superset of the next lower value.  Configuration and implementation
   of the client will map actions (such as the permission to execute of
   specific commands) to different privilege levels.  Pre-defined values are:

      TAC_PLUS_PRIV_LVL_MAX := 0x0f

      TAC_PLUS_PRIV_LVL_ROOT := 0x0f

      TAC_PLUS_PRIV_LVL_USER := 0x01

      TAC_PLUS_PRIV_LVL_MIN := 0x00

   A Privilege level can be assigned to a shell (EXEC) session when it
   starts starts (for example, TAC_PLUS_PRIV_LVL_USER).  The client will
   permit the actions associated with this level to be executed.  This
   privilege level is returned by the server in a session-based shell
   authorization (when "service" equals "shell" and "cmd" is empty).
   When a user required to perform actions that are mapped to a higher
   privilege level, then an ENABLE type reuthentication can be initiated
   by the client, in a way similar to su in unix.  The client will
   insert the required privilege level into the authentication header
   for enable authentication request.

   The use of Privilege levels to determine session-based access to
   commands and resources is not mandatory for clients.  Although the
   privilege level scheme is widely supported, its lack of flexibility
   in requiring a single monotonic hierarchy of permissions means that
   other session-based command authorization schemes have evolved, and
   so it is no longer mandatory for clients to use it.  However, it is
   still common enough that it SHOULD be supported by servers.”]

---

From: Tianran Zhou <[email-masked]>
Date: Wed, 17 May 2017 02:24:06 +0000

> From: OPSAWG [mailto:[email-masked]] On Behalf Of Alan DeKok
> Sent: Wednesday, May 17, 2017 12:23 AM
> To: Ignas Bagdonas
> Cc: Douglas Gash (dcmgash); [email-masked]; [email-masked]
> >>    I don't see how reviews (which were ignored) can be construed as "no
> evidence" that the authors were ignoring reviews.
> >>
> >>   Which was my point.  If document authors issue new revs irrespective
> of what the WG suggests, the chairs should replace the authors with ones
> who work towards WG consensus.
> >
> to make judgement whether comments and suggestions were or were not addressed.
> Authors have promised to address the raised comments discussing them on
> the list and responding to previous reviews.
> 
>   My comments (again) were that there was a history of ignoring the reviews.
> At the time I made that comment, it was true.  I find it troubling to see
> a response saying there's "no evidence" of such a problem.
 
Hi Alan,

Your reviews are appreciated. Via diff, I saw many of your comments are reflected in the 06 version. 
But the authors should have discussed your comments in the mailing list and gotten consensus on the revision.
Now, they are back on the right track. See Doug's latest post.
Let's back to the I-D discussion. ;-)

Regards,
Tianran

> >>   The alternative is to accept a draft as a WG document, and then to allow
> the authors to do pretty much whatever they want, and then to rubber-stamp
> the final document as an RFC.
> >

---

From: Alan DeKok <[email-masked]>
Date: Wed, 17 May 2017 10:54:16 -0400

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

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

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

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

  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"

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

  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.

>    REPLY indicating PASS, FAIL or ERROR."]

  OK. And if there's still no username?

> [Agreed From perspective of protocol, yes]

  The draft needs to say so.

> [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…

  That reply make implementation-specific assumptions.  There are TACACS+ implementations which don't get the challenge from a third-party.

  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.

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

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

> [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"
> [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…

  I would suggest removing it, but noting it exists, as with the password change above.

> [Areed, and server can accord appropriate confidence in this information with that proviso]

  That should be noted here, and in the Security Considerations section.

> [Agreed, the first meaning]

  The draft should be updated to clarify this.

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

> [yes, indicating circumstances (if known) on the device]

  That should be clarified.
 
> [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 ev…

  That's still a little unclear to me, but OK.

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

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

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

> [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 hav…

  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.

> [These two are rather legacy, and really related to an old form of Network Access, so propose we can exclude]

  Sounds good.

> [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 th…

  It's fine to say that task_id SHOULD be in all packets.
> [other than task_id, there are no accepted standards for this mapping]

  The draft should say so, then.
 
> [Proposed text: The number of input bytes transferred by this action to the port]

  from the users system to the port...

  Pedanticism helps here. :(  We weren't pedantic in RADIUS, and multiple implementors screwed it up.

  Alan DeKok.

---

From: t.petch <[email-masked]>
Date: Thu, 18 May 2017 17:57:01 +0100

WG Chairs

A slight change of thought.

This I-D, as Alan has commented and Doug acknowledges, has several
places where the description of security is more 1997 than 2017.  If we
turn such parts into a clear, concise specification, we may then find
that we have wasted our time since the Security Directorate then says
that no
way can that appear in an RFC, even an Informational one.

Would it be worth seeking guidance now on what is or is not likely to be
acceptable to a Security Directorate review?  Not a line by line
analysis but rather
higher level guidance as to whether such things as MD4, ASCII login,
RFC2433 as Best Practice and so on can appear.

Tom Petch

----- Original Message -----
From: "Tianran Zhou" <[email-masked]>
To: "t.petch" <[email-masked]>; "Alan DeKok"
<[email-masked]>; "Ignas Bagdonas" <[email-masked]>
Cc: "Douglas Gash (dcmgash)" <[email-masked]>; <[email-masked]>;
<[email-masked]>; <[email-masked]>;
<[email-masked]>
Sent: Tuesday, May 16, 2017 2:18 AM

Thanks Tom for pointing this out.
We appreciate any review comment. That really helps to improve the
document.
I think it's back on the right track now. Hopefully, the authors can
respond to more interactions.

Regards,
Tianran
> I echo part of what Alan says, that for a WG document, the editors
should
> incorporated, someone else then disagrees, disagreements incorporated
into
> Changing editors, unless it is to someone remote from the subject, is
unlikely
> ditto Alexander's, but was disinclined to do more with noone else
chipping

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Thu, 18 May 2017 17:39:36 +0000

In such a query, it might be worth mentioning that the doc was originally
intended to include TLS support added in order to address significant
elements of the security issues.

The current plan is a two-phased approach whereby the original protocol
would be documented for information first, and then it would be followed
up by a document describing how to increase the security using TLS.

---

From: Alan DeKok <[email-masked]>
Date: Thu, 18 May 2017 13:40:26 -0400

On May 18, 2017, at 12:57 PM, t.petch <[email-masked]> wrote: of thought.
> way can that appear in an RFC, even an Informational one.

  They've approved RADIUS RFCs... by holding their nose.

> RFC2433 as Best Practice and so on can appear.

  I've been on the Security Directorate for a while now.  While I don't claim to speak for everyone, I think the current approach in the draft will be fine.

  They may ask for some sections to be removed (i.e. servers pushing keys to clients). But everything else is pretty much fine.

  The idea is that having a documented protocol, with warnings and caveats, is much better than an undocumented one.

  Alan DeKok.

---

From: Tianran Zhou <[email-masked]>
Date: Fri, 19 May 2017 04:06:12 +0000

Hi Tom,

There was a conclusion based on the WG consensus:
https://mailarchive.ietf.org/arch/msg/opsawg/lR3TFRfaR8OldmPFjuVhRTQZHEE

As the first step, we
"Produce an informational document which documents the TACACS+ protocol as it stands today (as best as we can)."

My suggestion is that we do not cost too much time on the basic TACACS+.

On one hand, Alan as from the SECDIR gave this I-D many useful suggestions. On the other hand, we are going to ask for SECDIR early review once the existing comments are solved. 

Regards,
Tianran

> Sent: Friday, May 19, 2017 12:57 AM
> To: Tianran Zhou; Ignas Bagdonas
> Cc: [email-masked]; [email-masked];
> [email-masked]
> This I-D, as Alan has commented and Doug acknowledges, has several places
> where the description of security is more 1997 than 2017.  If we turn such
> parts into a clear, concise specification, we may then find that we have
> wasted our time since the Security Directorate then says that no way can
> that appear in an RFC, even an Informational one.
> 
> acceptable to a Security Directorate review?  Not a line by line analysis
> but rather higher level guidance as to whether such things as MD4, ASCII
> login,

---

From: Tianran Zhou <[email-masked]>
Date: Fri, 19 May 2017 04:12:05 +0000

> From: Alan DeKok [mailto:[email-masked]]
> Sent: Friday, May 19, 2017 1:40 AM
> To: t.petch
> Cc: Tianran Zhou; Ignas Bagdonas; IETF OOPSAWG;
> [email-masked]; [email-masked]
> > places where the description of security is more 1997 than 2017.  If
> > we turn such parts into a clear, concise specification, we may then
> > find that we have wasted our time since the Security Directorate then
> > says that no way can that appear in an RFC, even an Informational one.
> 
> > Would it be worth seeking guidance now on what is or is not likely to
> > be acceptable to a Security Directorate review?  Not a line by line
> > analysis but rather higher level guidance as to whether such things as
> > MD4, ASCII login,
>   I've been on the Security Directorate for a while now.  While I don't claim
> to speak for everyone, I think the current approach in the draft will be
> fine.
> 
>   They may ask for some sections to be removed (i.e. servers pushing keys
> to clients). But everything else is pretty much fine.
> 

Good to know this information. Thanks.

> is much better than an undocumented one.

Yes. It's what we got from the previous WG consensus.

Tianran

---

From: t.petch <[email-masked]>
Date: Fri, 19 May 2017 10:51:55 +0100

----- Original Message -----
From: "Alan DeKok" <[email-masked]>
Sent: Thursday, May 18, 2017 6:40 PM

On May 18, 2017, at 12:57 PM, t.petch <[email-masked]> wrote: of
thought.
> places where the description of security is more 1997 than 2017.  If
we

> Would it be worth seeking guidance now on what is or is not likely to
be
> RFC2433 as Best Practice and so on can appear.

  I've been on the Security Directorate for a while now.  While I don't
claim to speak for everyone, I think the current approach in the draft
will be fine.

<tp>
Ah, I did not know that.

Scrub my suggestion (while I hold my nose:-)

Tom Petch

p.s.  I notice that one of the addressees is now OOPS A WG; mmm, yes.

  They may ask for some sections to be removed (i.e. servers pushing
keys to clients). But everything else is pretty much fine.

  The idea is that having a documented protocol, with warnings and
caveats, is much better than an undocumented one.

  Alan DeKok.

---

From: t.petch <[email-masked]>
Date: Fri, 19 May 2017 11:38:01 +0100

Another fresh topic, so a slight change in the Subject:

I think that the use of the term ASCII needs more thought.

a) There is  mix of US-ASCII and ASCII with seemingly no difference in
semantic.  I would like this to be US-ASCII ..

b) except that there is usage of 'ASCII login' which looks like a Term
of Art, at least within TACACS so I suggest keeping ASCII solely for the
login.

c) there is no reference for US-ASCII.  There is no good reference for
US-ASCII but I usually see RFC20 being usedalthough some prefer ANSI
X3.4, 1986.

d) in some places, I think that the term ASCII is being used too
loosely.  ASCII is a character set and an encoding.  If you simply say
US-ASCII, then you are including DC3 and FF, for example, which are
unlikely to be valid.  Some use US-ASCII to mean printable ASCII, some
to mean alphameric plus a few others such as hyphen-minus and period.
This needs defining.  I don't know how many different character sets you
have - I was surprised that '&£#' (or some such) is a valid identifier
in places where an equal sign is not - so this needs more work.

e) this leads into data types, which Alan raised.  Boolean is used as a
data type. (I have seen it as a character string of 'true'/'false' of
'0'/'1' with zero meaning either true or false or vice versa or as a
binary integer of some number of bits or ....)  As Alan implied, section
7 and others are full of data types but on the one hand, what type it is
is usually omitted and on the other hand, the data types are not
defined.

You need to define datatypes; from the current I-D, I do not know how
many there would be; probably not many.  Look for example at TLS
(RFC5246) or YANG (RFC6020) which nail down the datatypes, semantic and
encoding, before they define data structures.  This is what I think that
you should do, on a smaller scale.  Since YANG is being so widely
deployed, you would get brownie points IMO for being in line with YANG
whereever possible

I see a need for boolean, character string/text, IPv4 address, IPv6
address, time interval, integer (positive ?negative).  I would like a
section on datatypes at the front, section 1 or 2.

f) in a similar vein, you use what I take to be hexadecimal
representation but are inconsistent with it. I see
OX0D 0x0D 0x1 0x01
This should be consistent and is also worth defining, as a
representation.

g) and then there is i18n, which gets an implicit mention with UTF8 but
harks back to d).  How much of UTF8 is allowed and where; it encompasses
over 65k characters these day:-(

This amounts to a lot of potential detailed change, but I would like to
see consensus on the approach first before edits are proposed or made.

Tom Petch

----- Original Message -----
From: "Alan DeKok" <[email-masked]>
To: "Douglas Gash (dcmgash)" <[email-masked]>
Cc: <[email-masked]>; <[email-masked]>
Sent: Wednesday, May 17, 2017 3:54 PM
Subject: Re: [OPSAWG] draft-ietf-opsawg-tacacs-06 Contributions, Status
and Plans

<snip>

---

From: Alan DeKok <[email-masked]>
Date: Fri, 19 May 2017 13:11:45 -0400

> I think that the use of the term ASCII needs more thought.

  Speaking only as an opinionated WG member... yes.

> in places where an equal sign is not - so this needs more work.

  I agree.

> is usually omitted and on the other hand, the data types are not

  The main issue with data types is that TACACS+ is a protocol based on printable strings.  So "data types" really means "printed versions of data types", which is a lot more problematic than "32-bit integer".

> wherever possible

  That's good, tho there are inconsistencies.  e.g. Yang "string" doesn't exactly map to TACACS+ "US-ASCII thing".  Yang "boolean" is "true/false", while TACACS+ has used "yes/no".

  We added data types to RADIUS, because it had (roughly) data types from day one, 32-bit integers, and all of the implementors had already agreed on meanings / encodings for them.  So RFC8044 was just a codification of existing practices, and not any change to implementations.

  Then there's the additional issue of trying to define data types for a protocol that's entirely string based, and has 18 years of implementation practice.

  i.e. before defining data types, it would be good to know what implementors have done, and then define types that match that.

  However, implementors are largely silent about all possible TACACS+ issues.  Which makes me think that the draft should name the data types, but be a bit vague about what they contain.

> section on datatypes at the front, section 1 or 2.

  I'd agree, subject to the caveats raised above.  i.e. "boolean is boolean, typically yes/no, but maybe also true/false, we really don't know..."

> This should be consistent and is also worth defining, as a

  I agree, subject to the same caveats.  It would also be nice to know what implementations do...

> over 65k characters these day:-(

  IMHO, the draft should just mention 18n, and run away screaming. :(

  As in, "we know about it, we don't know how to fix it, we don't know what implementations do, the fields are defined to be US-ASCII, if they contain anything else, that's bad."

> see consensus on the approach first before edits are proposed or made.

  I think this is the right approach.

  Alan DeKok.

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Fri, 19 May 2017 17:51:49 +0000

>> in places where an equal sign is not - so this needs more work.
Will align the ASCII references for sure, and tie down. I propose to
restrict to printable. Though, to add to complexity: it is common
practice, for example, to include newline in banners.

>  The main issue with data types is that TACACS+ is a protocol based on
>printable strings.  So "data types" really means "printed versions of
>data types", which is a lot more problematic than "32-bit integer".

I agree. Will put a section in regarding at a types, specifically for
attributes (As Alan pointed out).

>  That's good, tho there are inconsistencies.  e.g. Yang "string" doesn't
>exactly map to TACACS+ "US-ASCII thing".  Yang "boolean" is "true/false",
>while TACACS+ has used "yes/no".
>
>  We added data types to RADIUS, because it had (roughly) data types from
>day one, 32-bit integers, and all of the implementors had already agreed
>on meanings / encodings for them.  So RFC8044 was just a codification of
>existing practices, and not any change to implementations.
>
>  Then there's the additional issue of trying to define data types for a
>protocol that's entirely string based, and has 18 years of implementation
>practice.
>
>  i.e. before defining data types, it would be good to know what
>implementors have done, and then define types that match that.
>
>  However, implementors are largely silent about all possible TACACS+
>issues.  Which makes me think that the draft should name the data types,
>but be a bit vague about what they contain.

Agreed, but try to be explicit about the vagueness.

>  I'd agree, subject to the caveats raised above.  i.e. "boolean is
>boolean, typically yes/no, but maybe also true/false, we really don't
>know..."
>

Well, the approach we had in mind is printable US-ASCII for all fields,
apart form username and passwords, which are the hard points in
interfacing to identity provides which support. For these fields, to allow
UTF-8 on top of the byte streams.

>  As in, "we know about it, we don't know how to fix it, we don't know
>what implementations do, the fields are defined to be US-ASCII, if they
>contain anything else, that's bad."
>

---

From: Eliot Lear <[email-masked]>
Date: Fri, 19 May 2017 20:06:58 +0200

Isn't this handled by RFC 20?

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Sat, 20 May 2017 12:24:47 +0000

As always, thanks for the comments!

Regards,

Doug

Inline...

On 17/05/2017 15:54, "Alan DeKok" <[email-masked]> wrote:

>>       length value is set to zero." As to what happens when fields are
>>unused, this will depend upon the fields]
>

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?

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

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

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

Agreed. 

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

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.

>  The draft needs to say so.
Agreed.

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

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

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.

>  "This message is typically logged, which helps administrators determine

Sounds sensible, will add.

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

Agreed.

>  The draft should be updated to clarify this.

Will do.

>> [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?]
>
>  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.

>> [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"."]
>

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.

>  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...
>>  
>  I mean, what happens if the client sends a packet, the TCP connection
>stays open, and there's no response packet?
>
>  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.

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

Sure, will recommend minimum of 5 seconds, but I¹m not sure we can specify
an upper limit as it is not mandatory.

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

Sure, will add.

>> [These two are rather legacy, and really related to an old form of
>>Network Access, so propose we can exclude]
>
>  Sounds good.
>
>> [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.]
>

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

---

From: t.petch <[email-masked]>
Date: Sun, 21 May 2017 21:34:09 +0100

----- Original Message -----
Sent: Friday, May 19, 2017 7:06 PM

<tp>
Eliot

RFC20, AFACT, does not define Boolean, or Numeric or Printable (albeit
something which the current I-D does not currently use) while its
definition of 'text' (which the I-D does use) is based on starting with
STX so IMHO, no.

RFC20 is just a list of 128 characters with a numeric code.  Every other
protocol specification I can think of places restrictions thereon; DC3
in a username, anyone?

RFC4234 is to me a good example of an RFC that starts with RFC20 (or the
equivalent thereof) and produces something more usable.

Tom Petch

---

From: Alan DeKok <[email-masked]>
Date: Tue, 23 May 2017 10:03:30 -0400

> has data but is unused may not be ignored is for logging, are you thinking

  No.  Nut the draft needs to say what to do.

  If the protocol has provisions for something to happen, then the draft needs to say what implementations should do when that thing happens.

  In this case, it should state that unused fields are ignored, and treated as if they were not present.

> client-server tuple (see next comment).

  And it should suggest a minimum secret length.

> But I¹m not so sure what we should say about storage.

  "secret keys need to be kept secret". 

> seq_no, but implementations can limit interactions.

  Thanks.  My $0.02 is that the username SHOULD be in the first packet.  If not, it MUST be in the second packet.  If not, the session is broken, and the server returns ERROR.

  i.e. relying on the 256 packet limit for authentication is probably too lax.

...  RE: CHAP challenges and length
> challenge and response as pre-packaged unit.

   If the protocol transports data without looking at it, it's fine to treat that data as an opaque blob.  If the protocol looks at the data, the draft should have requirements on the data format.

   For an example, see:

https://tools.ietf.org/html/rfc2865#section-5.40

  The CHAP-Challenge attribute in RADIUS is defined to have at least 5 octets of challenge data.

> I think, be a mixing of concerns.

  Nope.  The TACACS+ server which authenticates the user should be secure by design.  This means rejecting zero-length challenges, and challenges which "too short".

  I would suggest that the challenges are a minimum of 8 octets in length.

... RE: password changes.
> insteresting to see what the main objects were for why it was removed from

  It was insecure, and no one liked it.  The other reasons have been largely lost to time.

> cannot choose to change it to make it more efficient.

  It's not about being "more efficient".  It's about specifying what this means.

  I have no idea what it means to duplicate a START record.  Is the data in the second START the same?  Is it different?  What is done in either case?

  Again, you need to document the protocol.  I'm not sure this point is getting across.  If you don't know what a portion of the protocol means, then either find out and document it, or document that "this exists, but we have no idea what it is, what it does, and we don't recommend that people use it".

  Leaving portions of the protocol entirely undefined and implementation-defined is bad.

  In RADIUS, I've authored or co-authored about 4 RFCs fixing things which were missed or undefined, and which cause interoperability issues.  And I'm still finding things which are broken in the specs.  Two in the last month, in fact...

  If TACACS is to be a standard, it should be clear what portions are well defined, and what portions are "I dunno...".  That gives guidance for future drafts as to what works, and what needs fixing.

> an upper limit as it is not mandatory.

   It's always good to recommend best practices, even if the behaviour isn't mandatory.

  Alan DeKok.

---

From: t.petch <[email-masked]>
Date: Fri, 26 May 2017 16:45:35 +0100

Doug

I mentioned my wish for the I-D to have a definition of the types used
and promptly noticed something along those lines in another I-D, which I
append below.  It comes from an Informational Model and so does not
specify encoding, which this I-D would need, but I like the terms used
and the descriptions as far as they go.  This I-D would only need a
subset.

Tom Petch

=========================================

   int         A type representing signed or unsigned integer numbers.
               This information model does not define a precision nor
               does it make a distinction between signed and unsigned
               number ranges.  This type is also used to represent
               enumerations.

   boolean     A type representing a boolean value.

   string      A type representing a human-readable string consisting of
               a (possibly restricted) subset of Unicode and ISO/IEC
               10646 [ISO.10646] characters.

   datetime    A type representing a date and time using the Gregorian
               calendar.  The datetime format MUST conform to RFC 3339
               [RFC3339].

   uuid        A type representing Universally Unique IDentifier (UUID(
               as defined in RFC 4122 [RFC4122].  The UUID values are
               expected to be unique within an installation of a large-
               scale measurement system.

   uri         A type representing a Uniform Resource Identifier as
               defined in STD 66 [RFC3986].

   ip-address  A type representing an IP address.  This type supports
               both IPv4 and IPv6 addresses.

   counter     A non-negative integer that monotonically increases.
               Counters may have discontinuities and they are not
               expected to persist across restarts.

   credentials An opaque type representing credentials needed by a
               cryptographic mechanism to secure communication.  Data

               models must expand this opaque type as needed and
               required by the security protocols utilized.

   data        An opaque type representing data obtained from
               measurements.

   Names of objects are generally assumed to be unique within an
   implementation.

----- Original Message -----
From: "Alan DeKok" <[email-masked]>
To: "Douglas Gash (dcmgash)" <[email-masked]>
Cc: <[email-masked]>; <[email-masked]>
Sent: Wednesday, May 17, 2017 3:54 PM
Subject: Re: [OPSAWG] draft-ietf-opsawg-tacacs-06 Contributions, Status
and Plans

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Fri, 26 May 2017 16:49:39 +0000

Looks like a nice model for emulating, will add along these lines. This is
essentially for embedding at the start of the attributes section.

Cheers!

Regards,

Doug.

---

From: "Douglas Gash (dcmgash)" <[email-masked]>
Date: Fri, 26 May 2017 17:05:00 +0000

Hi Alan,

Thanks again for responses, please see inline:

Just for accounting: I do owe you an initial response to one batch of your
comments that I missed first item, and we have the security section as a
whole. Below though, are the responses to the comments form Tuesday,
thanks!

On 23/05/2017 15:03, "Alan DeKok" <[email-masked]> wrote:

>  If the protocol has provisions for something to happen, then the draft
>needs to say what implementations should do when that thing happens.
>

Agreed, will clarify that, within the context of the protocol flow. These
may still be logged.

>  "secret keys need to be kept secret".

Sure, I’d hope this would be an axiom ;-) but not harm in emphasising.

>  Thanks.  My $0.02 is that the username SHOULD be in the first packet.
>If not, it MUST be in the second packet.  If not, the session is broken,
>and the server returns ERROR.
>

Sure, makes sense.

>   If the protocol transports data without looking at it, it's fine to
>treat that data as an opaque blob.  If the protocol looks at the data,
>the draft should have requirements on the data format.
>
>   For an example, see:
>

We can follow a similar path with T+. I Will add. (covers below comment
too).

>  Nope.  The TACACS+ server which authenticates the user should be secure
>by design.  This means rejecting zero-length challenges, and challenges
>which "too short".
>
>  I have no idea what it means to duplicate a START record.  Is the data
>in the second START the same?  Is it different?  What is done in either
>case?
>
>  Again, you need to document the protocol.  I'm not sure this point is
>getting across.  If you don't know what a portion of the protocol means,
>then either find out and document it, or document that "this exists, but
>we have no idea what it is, what it does, and we don't recommend that
>people use it".
>
>  In RADIUS, I've authored or co-authored about 4 RFCs fixing things
>which were missed or undefined, and which cause interoperability issues.
>And I'm still finding things which are broken in the specs.  Two in the
>last month, in fact...
>
>  If TACACS is to be a standard, it should be clear what portions are
>well defined, and what portions are "I dunno...".  That gives guidance
>for future drafts as to what works, and what needs fixing.

I think then I committed a misunderstanding here. I thought you were
suggesting to drop the redundant accounting request, now I realist your
comment is to explain why it is used. Sure, can clarify this.

>   It's always good to recommend best practices, even if the behaviour

I was thinking rather about the scenario that it may not recur at all...
However, we can say: for recurring accounting, this is the range (5
seconds to 1 hour), or updates are not sent at all.