Re: [Nbd] Thoughts about the handshake protocol

[Goswin von Brederlow <goswin-v-b@...186...>]

Wouter Verhelst <w@...112...> writes:

> *sigh*
>
> On Mon, May 30, 2011 at 01:45:27PM +0200, Goswin von Brederlow wrote:
>> Hi,
>> 
>> revently it was mentioned that the new-style handshake has some problems
>> and needs some fixing up for the future. So I wanted to collect some
>> thoughts about this to maybe spark a redesign. Turned out to describe
>> pretty much the complete handshake. Let me know what you think:
>> 
>> ----------------------------------------------------------------------
>> Currently the handshake starts with:
>> 
>> S: "NBDMAGIC" (as in the old style handshake)
>> S: 0x49484156454F5054 (note different magic number)
>> S: 16 bits of zero (reserved for future use)
>> C: 32 bits of zero (reserved for future use)
>> 
>> So the server identifies itself as NBD server. The client though does
>> not identify itself but just starts sending payload.
>> 
>> Now what are those 16/32 bit of zero for? Some sort of version number or
>> bitfield of options? The problem I have with them is that they allow no
>> room for compromise. If the client is not at least as new as the server
>> it will not understand all the bits the server sends. The only option
>> the client then has it to give up.
>
> No, that's not true. Why would the client have to give up? If it doesn't
> understand what a particular bit in the reserved field means, it must
> ignore it. If a negotiation happens between a client and a server
> whereby the client does not acknowledge understanding of a particular
> option that is announced in the server's reserved field, then it's up to
> the server to either end the communication, or fall back to old
> behaviour. That's the whole point of these reserved bits.

Ah, ok. That isn't clear from the proto.txt.

> The only kind of 'required' future bits that I could imagine adding are
> those that would pertain to authentication. Everything else we can
> disable if the client would not support it. And since we're not
> encrypting data between the client and the server (and I have no
> intention of doing that, either), any authenticated TCP session could
> theoretically be hijacked, which makes authentication useless; so I'm
> not even sure authentication is ever going to happen.

+1.

>> Next the option haggling is a good idea. But it lacks a way for the
>> server to say when it doesn't understand an option and for the client to
>> say there are no more option comming.
>
> We've talked about that, earlier, and there is a solution already that
> does not require breaking backwards compatibility.
>
> Now while I don't oppose improving the handshake per se if it can be
> done and there is a benefit in doing so, I do oppose breaking backwards
> compatibility if it can be avoided.

Didn't you say you didn't like that either?

>> ----------------------------------------------------------------------
>> So I would like to get a bit more flexibility into this:
>> 
>> S: "NBDMAGIC" (as in the old style handshake)
>> S: 8 byte magic number
>> S: 16 bits required features
>> S: 48 bits offer optional features
>
> Doing it that way *will* break backwards compatibility, because you're
> changing the initial greeting you get from the server which is sent to
> the client before the client can even respond. That's *bad*, period.
>
> I've changed the protocol incompatibly once, and I *do* *not* want to do
> that again. Yes, I made some mistakes, and it would've been better with
> some more review, but unfortunately I didn't get any at the time, so now
> we're stuck with it. And that's the end of it.
>
> Also, there's no need to differ between 'optional' and 'required'
> features. If there is a required feature, there are several ways in
> which the client can inform the server that it understands this new
> option (through setting a bit in its reserved field, or through sending
> an option message). If that doesn't happen, the server can simply close
> the connection.
>
> It might be nice to add a message that the server can send which would
> say something like "you haven't acknowledged an option that I require",
> or to have a way for the server to say "I don't understand that option".
> But beyond that, I don't see the benefit of changing the negotation any
> further.
>
> [...]
>> A client MUST understand all required features set by the server. For
>> now all bits would be reserved and 0 and any incompatible change would
>> allocate one bit and set it. Hopefully there won't be any. Bit 15 of
>> required features can be reserved to indicate another 8 byte of feature
>> bits follow. The client acknowledges that it understands all the
>> required features by echoing them back. Again bit 15 of the required
>> features would indicate another 8 byte of feature bits follow the reply
>> just like in the servers hello.
>> 
>> Optional features will be set by the server to indicate it supports a
>> certain feature (e.g. FUA or FLUSH). The client responds with a bitfield
>> indicating those features it wishes to use. Any unknown feature must be
>> disabled (set to 0) and this must be safe to do so. Only bits set by the
>> server might be returned set by the client. Only those features set by
>> both server and client MAY be used.
>
> There's no need for any of that, IMO.
>
> Things I will change include:
> - Supporting more options by making NBD_OPT_EXPORT_NAME 'special', as
>   discussed previously.
> - Setting a flag, which (if the client acknowledges it through its
>   reserved field) will allow the server to drop the 124 bytes of zeroes
>   (they're useless cruft that I forgot to remove)
>
> Beyond that, I don't see why it would be necessary to change more
> things. As long as the client and the server can communicate and
> negotiate with one another properly (and they can), there's no need to
> change.

Ok. So you will use up the first reserved bit now. My suggestion then
would be to do this all through setting a flag instead of making
NBD_OPT_EXPORT_NAME 'special':

Bit 0 (server+client): support new-style handshake V2

Required changes:

- NBD_OPT_EXPORT_NAME now recieves a reply containing size+flags of the
  export. No trailing zeroes. This option is required before NBD_OPT_END.

- All options are acknowledged by the server
  S: 0x49484156454F5054
  S: 32 bits denoting the requested option
  S: 32 bit error code (0 == SUCCESS)
  S: 32 bit length of option data
  S: length bytes of data

  On success the payload is option specific, on error the payload is
  plain text describing the error.

- If any further reserved bits (1-15) are not ACKed by the client or any
  of the client options (bits 16-31) are not understood the server may
  repsond to the first option with an error explaining what required
  feature is not met and terminate the connection even though that reply
  has nothing to do with the requested option.

- New option NBD_OPT_END
  This must be the last option send. On success the NBD protocol
  starts. On error the connection is terminated.


Optional features (i.e. just reserve the numbers for the options,
clients don't have to implement them):

- New option NBD_OPT_LIST_EXPORTS
  Return a list of available exports (array of 0 terminated strings?)

- New option NBD_OPT_SYNC
  Open the export with O_SYNC

- New option NBD_OPT_READ_ONLY
  Open the export read-only

- New option NBD_OPT_AUTO_SYNC
  Run in sync mode until the first FUA/FLUSH command is used.

- New option NBD_OPT_ENABLE_PRIVATE
  Enable private options between client and server. Used for
  experimental features between client and server of the same make.
  Client payload: client identifier
  Server payload: defined by client

- New options NBD_OPT_PRIVATE_1-255
  Client defined options, only valid after NBD_OPT_ENABLE_PRIVATE
  returned success. Used for experimental features between client and
  server of the same make.

Does that sound better?

MfG
        Goswin