Continuing...
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?
... that's it for today. More later.
_______________________________________________
OPSAWG mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/opsawg
