<div dir="ltr">Hi Trevor,<div><br></div><div>This is looking much better. I'm compelled by the noh2 branch now. I especially like moving transport messages back to simple CipherSuite operations, while the HandshakeState remains this fancy state machine which does its thing and then gets out of the way. This is more or less a natural way of organizing an implementation of it too. The HandshakeState object has-a CipherState object. My previous Noise version had things divided exactly like this. With the recent changes, the only difference is that I make `h` a part of the HandshakeState, and not part of the CipherState, since it's not useful for transport messages anymore, and that way I can unambiguously zero out sizeof(struct HandshakeState) at the right point in time. That change might make the spec unnecessarily verbose though, so I'm not sure it's worthwhile, as it's not "substantive" either.</div><div><br></div><div>A few other remarks and nits:</div><div><br></div><div>- "All Noise messages are 65535 bytes in length or less."</div><div>Why? This limitation seems rather pointless. The RFC's ChaPoly gives us a 32bit internal block counter, so we can safely make messages that are rather huge. It seems like that is something best left up to the upper layers to decide, especially since the framing is there too.</div><div><br></div><div>- "KDF(kdf_key, input): Takes a kdf_key of 256 bits and some input data and returns a new value for the cipher key `k`. "</div><div>It's not really useful here to name the return value. Could be shortened to "..and returns a new value for a cipher key."</div><div><br></div><div>- "HMAC-SHA2-256 is an example KDF." "SHA2-256 is an example hash function."</div><div>Are the examples still relevant, since the cipher suite section below specifies them too? The latter is missing ` ` by the way.</div><div><br></div><div>- "Decrypt(ciphertext): Calls DECRYPT(k, n, h, ciphertext) to get a plaintext, then increments n and returns the plaintext. If an authentication failure occurs all variables are set to zeros and the error is signalled to the caller."</div><div>signalled -> signaled</div><div>Which variables are set to zero? The local k, n, h, and ciphertext that are on DECRYPT's stack? The k, n, and h that belongs to the present CipherState object? If the latter, thats a bad DoS. If the former, I don't know if it makes sense to include implementation details like that; hopefully implementors are already zeroing their stacks appropriately.</div><div><br></div><div>- "on the next data_len + 16"</div><div>16 is hard coded here. That probably makes sense, as earlier you also explicitly specify having a 128-bit tag in no matter what ciphersuite. But in case you did want to abstract things with an AUTHLEN, here's a 16.</div><div><br></div><div>- "For "dhxy" calls cipherstate.MixKey(DH(x, ry)) and sets has_key to True."</div><div>Can you italicize the x and y inside the ` ` too?</div><div><br></div><div>- "ReadHandshakeMessage(buffer, descriptor): Takes a byte buffer containing a message, and a descriptor, and returns a payload. If a decryption error occurs all variables are set to zeros and the error is signalled to the caller."</div><div>Same issue with spelling typo and zeroing ambiguity.</div><div><br></div><div>- "EndHandshake(): Returns two new CipherState objects by calling cipherstate.Split()."</div><div>...and zeros everything out.</div><div><br></div><div>- "Big-endian is preferred because..."</div><div>Grumble grumble.</div><div><br></div><div><br></div><div>By the way, after you merge this to master, is there a reason for keeping around noh and noise2 branches? Or are these just left over from old times?</div><div><br></div><div>Jason</div></div>