Document: draft-ietf-syslog-tls-13.txt
Reviewer: Scott Brim
Review Date: 30 July 2008
IETF LC Date: 5 Aug 2008

Summary:

This draft is basically ready for publication, but has nits that should be fixed before publication.

Comments:

There is only one item that is significant, which is the last of the SHOULD comments (just below).  As for the rest, they are not showstoppers but imho if you take them into consideration you'll have a better draft.

First, MUST/SHOULD
------------------

 >    TLS typically uses certificates [RFC5280] to authenticate peers.
 >    Implementations MUST support TLS 1.2 [I-D.ietf-tls-rfc4346-bis] and
 >    are REQUIRED to support the mandatory to implement cipher suite,

Small: Change "are REQUIRED to" to "MUST".

 >    The transport receiver and transport sender SHOULD provide
 >    mechanisms to record the end-entity certificate for the purpose of
 >    correlating it with the sent or received data.

Medium: In general it's good to explain the conditions under which a SHOULD is not expected.  Otherwise you get some implementations that treat it as a MUST and many others that just ignore it.  What happens if the sender or receiver does not record the end-entity certificate? 
Under what conditions is that a problem?

 >    If the peer does not meet the requirements of the security policy,
 >    the TLS handshake SHOULD be aborted with an appropriate TLS alert.

Medium-Large: This SHOULD surprises me.  You allow the peer not to abort the handshake under some conditions?  What could those conditions be?  I think you should explain this SHOULD carefully so as to avoid a security hole through lack of documentation.


Second, terminology
-------------------

You have

   Transport sender	    Transport receiver
   Server    		    Client
   Peer			    Peer

These terms overlap.  I can't tell if the first two pairs are synonymous or not.  I can see reasons to use all of these, based on different relationships and functions, but you seem to mix and match sometimes.  Sometimes limiting your terminology can make your spec tighter and clearer, so consider getting rid of one pair.  Here are some
examples:

 > 1.1 Terminology
 >
 >    o  A "TLS client" is an application that can initiate a TLS
 >       connection by sending a Client Hello to a peer.
 >
 >    o  A "TLS server" is an application that can receive a Client
 >       Hello from a peer and reply with a Server Hello.

Small: If you're going to use client, then use server, and vice versa.
A TLS client initiates a Client Hello to a *server*, and a server receives a Client Hello from a *client*.

 > 4.1 Port Assignment
 >
 >    A syslog transport sender is always a TLS client and a transport
 >    receiver is always a TLS server.

I would have thought that since a server occasionally sends a message to the client (for example a close_notify), that the server could be a "transport sender" too ... and similarly the client receiving packets could be a "transport receiver".

 > 4.2 Initiation
 >
 >    The transport sender should initiate a connection to the
 >    transport receiver and then send the TLS Client Hello to begin
 >    the TLS handshake.  When the TLS handshake has finished the
 >    transport sender MAY then send the first syslog message.

There are two actions here, "initiate a connection" and then have a TLS handshake.  It would make more sense to me to use the terms client/server than sender/receiver.

 >    A TLS client MUST close the associated TLS connection if the
 >    connection is not expected to deliver any syslog messages later.

Just above, the "transport sender" was told to set the message length, but here the "TLS client" is told to close the TLS connection.  That feels inconsistent.