I am the assigned Gen-ART reviewer for this draft. For background on Gen-ART, please see the FAQ at < http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>. Please resolve these comments along with any other Last Call comments you may receive. Document: draft-ietf-tls-encrypt-then-mac-02.txt Reviewer: Elwyn Davies Review Date: 15 June 2014 IETF LC End Date: 20 June 2014 IESG Telechat date: (if known) - Summary: Almost ready. Major issues: Minor issues: Header/Abstract/Intro: Doesn't this update RFC 6347 and either or both of RFC 6066 and RFC 5246 since it defines a new extension? s3.1: [I am not a TLS expert so I may not have got this right...] If the rehandshake proposes to use a cipher that doesn't need encrypt-then-mac as per GenericStreamCiphers, or GenericAEADCiphers as mentioned in s3, then presumably this is allowed whether encrypt-then-mac had or had not been negotiated previously on the session. It should be clarified whether this is allowed and, if it isn't, what the response should be. If it is allowed and the session switches to a GenericStreamCipher, or a GenericAEADCipher, then it appears that if the original session was using encrypt-then-mac, a downgrade to a cipher that does need encrypt-then-mac but isn't using it could be achieved by using a second rehandshake without encrypt-then-mac since the first rehandshake goes to a situation that doesn't use encrypt-then-mac after the first rehandshake. The solution seems to be to record whether the session ever used encrypt-then-mac rather than whether is is currently using it, and then apply the rules in s3.1 based on the record. Nits/editorial comments: s3: (very nitty nit) I think the MAC calculation piece would be clearer with what it updates noted before the update. Thus: OLD In TLS [2] notation the MAC calculation is: MAC(MAC_write_key, seq_num + TLSCipherText.type + TLSCipherText.version + TLSCipherText.length + ENC(content + padding + padding_length)); for TLS 1.0 without the explicit IV and: MAC(MAC_write_key, seq_num + TLSCipherText.type + TLSCipherText.version + TLSCipherText.length + IV + ENC(content + padding + padding_length)); for TLS 1.1 and greater with explicit IV (for DTLS the sequence number is replaced by the combined epoch and sequence number as per DTLS [4]). NEW: In TLS [2] notation the MAC calculation is: o For TLS 1.0 without the explicit IV MAC(MAC_write_key, seq_num + TLSCipherText.type + TLSCipherText.version + TLSCipherText.length + ENC(content + padding + padding_length)); o For TLS 1.1 and greater with explicit IV MAC(MAC_write_key, seq_num + TLSCipherText.type + TLSCipherText.version + TLSCipherText.length + IV + ENC(content + padding + padding_length)); o For DTLS the sequence number in the TLS 1.1 description is replaced by the combined epoch and sequence number as per DTLS [4]). s3, next to last para: > For DTLS, the record MUST be discarded and a fatal bad_record_mac MAY > be generated [2]. Shouldn't the reference here be [4] for DTLS? s3.1 (last para)/s7.1: Shouldn't the TLS_ext reference [3] be to the updated RFC 6066?