[noise] NoiseSocket API feedback

[Trevor Perrin]

Thanks Nemanja

These comments make sense and I agree with most of them.

There's a larger question of whether we should even define an API
here?  I'm not sure how useful it is?


On Wed, Apr 18, 2018 at 8:08 PM, Nemanja Mijailovic
<metalnem at mijailovic.net> wrote:
>
> 1) Initialize
>
> Current definition:
> - INPUT: pattern, dh, cipher, hash
> - OUTPUT: session object
>
> It's strange that not all inputs are specified here. I would either mirror
> the definition of Initialize in HandshakeState from the Noise specification:
> INPUT: handshake_pattern, initiator, prologue, s, e, rs, re
>
> Or maybe just say something more abstract like:
> INPUT: noise_protocol

Second option makes sense:  there's no need to list all the parameters
of a Noise protocol in Initialize and Reinitialize, particularly
because it might change over time (e.g. signatures as well as DH, or
different ways of doing symmetric crypto).


> I think that the OUTPUT is not necessary here, so I would just remove
> it—it's not clear what is the meaning of "session object", and it's not
> mentioned anywhere else in the specification (except in Reinitialize).

Makes sense too.


> 2) ReadHandshakeMessage
>
> ReadHandshakeMessage takes handshake_message as a parameter. This may lead
> the implementer to believe that the parameter is a byte sequence that
> contains the whole message that the user has somehow obtained prior to the
> call. In practice, you will most likely pass the input stream as a
> parameter, and read the handshake_message from it (when I say stream, I mean
> something that you can read a sequence of one or more bytes from, like Go's
> io.Reader, or Java's InputStream). Maybe someting like this would be
> clearer:
>
> ReadHandshakeMessage:
>
> - INPUT: input_stream


Maybe.  Would you also replace WriteMessage and ReadMessage with Write
and Read, that just operate on a stream?  Would that get more
complicated, e.g. would we assume the library might do buffering and
have a flush() function?


> 3) PeekHandshakeMessage
>
> PeekHandshakeMessage will probably completely consume the negotiation data,
> so I would just rename it to ReadNegotiationData.

Makes sense.


> 4) WriteEmptyHandshakeMessage
>
> It does exactly the same thing as the WriteHandshakeMessage when the
> message_body is omitted, so I would just drop this one.

WriteEmptyHandshakeMessage only does the exact same thing as
WriteHandshakeMessage if the underlying Noise protocol doesn't encrypt
the handshake payload.  Otherwise the zero-length message body would
get padded and encrypted (with WriteHandshakeMessage).

So if Bob was able to negotiate a key with Alice but still wants to
reject her initial message, he would use WriteEmptyHandshakeMessage,
so I think we should keep it, or something like it.

Trevor