[AVT] WGLC comments for draft-ietf-avt-dtls-srtp-03

[<Pasi.Eronen at nokia.com>]

Overall, this document looks good, and all my comments 
are quite minor (mostly small clarifications):

The document would benefit from further clarifying the topic of SRTP
contexts. E.g. Section 3 says "Each DTLS-SRTP session contains a
single DTLS association [..], and an SRTP context." But since SRTP 
contexts are unidirectional, it seems that there would usually 
be two SRTP contexts, right?

RFC 3711 also allows different SSRCs (within one (srcIP,dstIP,srcPort,
dstPort)) to use different SRTP contexts. It would be useful to
explicitly say that DTLS-SRTP here uses just a single SRTP context per
direction (if I've understood this right).

Section 8 seems to specify a token DTLS over TCP. However, RFC 4347
doesn't specify how that would work (and it's obvious that it won't
work as is -- e.g. the state machine handling timeouts/retransmissions
needs changes, MTU discovery won't be identical, etc.). If the intent
is to specify DTLS-over-TCP, that really needs a separate section (or
ideally, a separate document) describing the deltas from ordinary
DTLS.

Section 3: Since SRTP does not have concepts of "client" and "server",
it would be useful to say early that DTLS-SRTP assumes that the
parties have agreed out-of-band (e.g. via SDP) who takes which role.

Section 4.3 and 5.2: There are two potential ways how rekeying might
work here: sending a ClientHello the same way as in the beginning, or
TLS re-negotiation (where the handshake messages are protected with
the currently active cipher suite). The latter is probably meant here,
but it would be useful to say so explicitly.

Section 4.1.2 specifies protection profiles that depend on
draft-ietf-avt-srtp-big-aes. However, since the draft is expired long
time ago -- and we'd need a normative reference here -- I'd suggest
deleting these protection profiles (they could be defined in the
big-aes draft if it's ever resurrected).

Section 4.1.2 should also specify what SRTP PRF is used (as SRTP
allows defining new PRFs).

Section 4.1.2 and 9: Requiring Standards Action for new SRTP
Protection Profiles seems really strict -- e.g. for TLS cipher 
suites, Specification Required is enough.

Section 4.2 should say what's the value for per-association context
value that's given to TLS extractor (possibly it's empty).

Section 4.2: the figure showing the key derivation is was
accurate in draft version -00 (which derived keys differently),
but now it's IMHO a bit misleading. Also, it's missing the SRTCP
key derivation. Perhaps something like this?

TLS master 
  secret   label
   |         |
   v         v
+---------------+
| TLS extractor |
+---------------+
       |                                         +------+   SRTP
       +-> client_write_SRTP_master_key ----+--->| SRTP |-> client
       |                                    | +->| KDF  |   write
       |                                    | |  +------+   keys
       |                                    | |
       +-> server_write_SRTP_master_key --  | |  +------+   SRTCP
       |                                  \ \--->|SRTCP |-> client
       |                                   \  +->| KDF  |   write
       |                                    | |  +------+   keys
       +-> client_write_SRTP_master_salt ---|-+
       |                                    |
       |                                    |    +------+   SRTP
       |                                    +--->| SRTP |-> server
       +-> server_write_SRTP_master_salt -+-|--->| KDF  |   write
                                          | |    +------+   keys
                                          | |  
                                          | |    +------+   SRTCP
                                          | +--->|SRTCP |-> server
                                          +----->| KDF  |   write
                                                 +------+   keys




Minor clarifications and editorial nits:

Section 4.1, "Because the DTLS layer does not process the packets, it
does need to distinguish them". This probably should say "..does NOT
need.."?

Section 4.1, "Records of type other than "application_data" MUST use
ordinary DTLS framing." And MUST be sent in separate datagrams?
(DTLS allows placing multiple records in single datagram, and in
theory, mixing DTLS framing and SRTP in single datagram could be
made to work -- but probably that's not intended here).

Section 4.1.1, "SRTPProtectionProfile SRTPProtectionProfiles<2^16-1>;"
should be "...<2..2^16-2>".

Section 4.1.1, "SHOULD not" -> "SHOULD NOT"

Section 5.1.2, Figure 8: Bob and Charlie probably should have
different IP address?

Section 9: IANA considerations for TLS extractor label are missing.

Section 11: RFC 3711 clearly needs to be a normative reference.

Section 11: RFC 2434 has been obsoleted by RFC 5226.

Overall: The document talks about "TLS KDF", but the term "KDF" is
never used in the TLS spec itself (it uses "TLS PRF"). Also, the
document talks about "the TLS KDF" -- but with TLS 1.2 (and DTLS 1.2),
there can be multiple PRFs (which is used is negotiated with the
cipher suite).

Overall: Using symrefs="yes" would make the document easier to read
(and RFC Editor recommends this nowadays, too).
	

Best regards,
Pasi
_______________________________________________
Audio/Video Transport Working Group
avt at ietf.org
https://www.ietf.org/mailman/listinfo/avt