[noise] small questions on the spec

[Trevor Perrin]

On Sat, Jun 17, 2017 at 10:02 AM, David Wong <davidwong.crypto at gmail.com> wrote:
>
>> All Noise messages are less than or equal to 65535 bytes in length.
>
> How do you propose we check for that? My idea is that we have to check
> that in multiple places:
>
> * in the WriteMessage() function for the final payload being sent
> * in the ReadMessage() function for reading the other peer's message
> * in the encryptWithAd()/decryptWithAd() for calls after the handshake is done


I would say that's more of an implementation detail than a spec
detail.  I.e. the spec is clear on the rules, how you apply them is up
to you?


>> For "s": Sets temp to the next DHLEN + 16 bytes of the message if HasKey() == True, or to the next DHLEN bytes otherwise. Sets rs to DecryptAndHash(temp).
>
> Shouldn't there be some indication as well for key verification
> (pinning, pki) ? As I understand, Noise_XX has each peer send its
> static key during the handshake so there must be some verification
> here.

Sure, we could add a security consideration stating that how you
authenticate public keys is out of scope of this document, but you
could use certificates, lookups to some trusted source, pinning,
pre-configuration, etc.  I'll add something like that for next
revision.


>  How does `EncryptAndHash()` handles an error returned by
> `EncryptWithAd()` ? Trevp mentionned Section 5:
>
>> The application may choose to delete the CipherState and terminate the session on such an error, or may continue to attempt communications. If EncryptWithAd() or DecryptWithAd() signal an error due to nonce exhaustion, then the application must delete the CipherState and terminate the session.

Sure, maybe we should mention that in the pseudocode as well, feel
free to submit a PR or propose specific text for clarifications like
that.


> There is no real distinctions between public functions and internal
> functions. Public functions seem to be:
>
> Initialize(), ReadMessage(), WriteMessage(), EncryptWithAD(), DecryptWithAd()

> The rest seems like it should be private. My suggestion: add a "_" in
> front of function names that are supposed to be private (this is what
> the Strobe specification does) and an explanation of what is this "_".

I'm not sure that would improve readability, seems like it just makes
things more complicated and the names uglier?


> suggestion: Initialize should return a HandshakeState, not be based on
> a HandshakeState.

Since this is just pseudocode, does that make a difference?  A
concrete proposal or PR would help evaluate little clarifications like
this.  Often what seems like a tiny change affects other parts of the
document, so we'd need to weigh all that.

Trevor