Re: [Nbd] NBD_CMD_DISC

[Wouter Verhelst <w@...112...>]

On Sun, Apr 10, 2016 at 10:49:00AM +0100, Alex Bligh wrote:
> Wouter,
> 
> (Daniel added to CC list)
> 
> Having done some research, I'm going to cut this argument short and go
> straight to the TLS standard:
> 
> RFC5246 s7.2.1:
> 
>    "The client and the server must share knowledge that the connection is
>    ending in order to avoid a truncation attack.
> 
>    ...
> 
>    Either party may initiate a close by sending a close_notify alert.
>    Any data received after a closure alert is ignored.
> 
>    Unless some other fatal alert has been transmitted, each party is
>    required to send a close_notify alert before closing the write side
>    of the connection.  The other party MUST respond with a close_notify
>    alert of its own and close down the connection immediately,
>    discarding any pending writes.  It is not required for the initiator
>    of the close to wait for the responding close_notify alert before
>    closing the read side of the connection."
> 
> The way OUR standard currently reads is incompatible with that.
> It requires on NBD_CMD_DISC that the server simply drop the connection.

The text does currently say that, mostly because it was written with TCP
in mind as the lower layer. It could instead be written that the
NBD_CMD_DISC command, when processed, means to ask the server to
terminate the lower layer (be that TLS or TCP) of the connection *after*
handling whatever in-flight requests that haven't been handled yet.

> That requires it breach RFC5246, and in my view that isn't acceptable.

Only if you read "disconnect" as "disconnect the TCP layer", rather than
"disconnect the lower layer protocol", which in the case of a TLS
connection is not TCP but TLS.

> We should require the end terminating the connection shuts the
> TLS session down properly as opposed to requiring it to disconnect.

To "shut down the TLS session properly" is, in fact, the same thing as
to disconnect a TLS session.

Given that TLS is a layer-6 protocol[1], it makes sense that one should
have to destroy the connection at the level of the layer-6 protocol
(TLS) before destroying the connection at the layer-5 protocol (TCP).

In other words, if you want to send some application-layer messages over
the TLS layer, you should ensure that the data has reached its
destination (or at the very least, that it has been sent out over the
network) before you drop the socket over which the TLS layer is trying
to write. TLS adds some buffers, yes, but if whatever libraries you're
using don't have any commands to flush those buffers, then the library
is broken and should not be used.

*That* is the "basic programming" that I was referring to. I don't think
it makes much sense to change the NBD protocol to make it easier for
people to do layering violations. We should make it easier to avoid such
layering violations if we can do so, but I don't think NBD_CMD_CLOSE
does that.

I do agree that the lack of a reply message to NBD_CMD_DISC is
unfortunate, and that it would have been cleaner if there was such a
reply message. However, things being what they are, I don't think the
downside to not having a reply message outweighs the downside of adding
yet another protocol option.

[1] strictly speaking, TLS is an application layer protocol in the
    TCP/IP stack, rather than a session layer protocol in the OSI stack.
    However, according to that (not very useful) strict interpretation,
    any TLS-encrypting protocol stack has *two* application-layer
    protocols, whereas obviously one of the two "equal" protocols is
    encapsulating (and transmitting with its own headers etc) the data
    and metadata of the other protocol -- thus, it is a layer between
    the application and transport layers.

> Qemu server side is in breach of RFC5426 precisely because it follows
> our standard.

No. If it is in breach of RFC5426 in the way you state here (and I'm not
claiming you're either right or wrong), it is because it has a layering
violation.

[...]
> Contrary to what you've said, this isn't 'basic programming'.
> It would be basic programming to ensure your output is flushed
> properly if you were in control of the buffering and magic
> bytes and retries didn't appear from elsewhere. It's hard
> to get right.

I'm not saying it's easy to get right. Encryption is hard to get right
in general, not just for TLS. I'm also not saying we definitely should
keep things as they are; maybe it's possible to improve the disconnect
sequence, and if so, we certainly should do so.

However, just telling the client to wait until a message from the server
has arrived, because otherwise it might not send its own final message
to the server is *not* a solution, because it simply *moves* the problem
from the client to the server.

Putting everything together, NBD_CMD_DISC is an application-layer
disconnect message for NBD. Yes, it is possible to do without that, in
which case the disconnect should be handled at a lower layer. However,
there *is* an advantage to a separate disconnect message, in that a
simple client could just send

(negotiation)
NBD_CMD_READ
NBD_CMD_READ
(...)
NBD_CMD_DISC

without waiting for any server message before the DISC message, and then
just read reply messages (on a blocking socket) until the connection
drops.

Is that a critical feature? No. Is it useful? I think so. Not useful
enough to *create* a DISC message, perhaps, but useful enough that we
should retain it.

[...]

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12