On Tue, Oct 21, 2014 at 10:35:06AM +0100, Daniel P. Berrange wrote: > On Tue, Oct 21, 2014 at 12:10:39AM +0200, Wouter Verhelst wrote: > > On Mon, Oct 20, 2014 at 01:56:43PM +0200, Florian Weimer wrote: > > > I cannot comment on whether the proposed STARTTLS command is at the correct > > > stage of the NBD protocol. If there is a protocol description for NBD, I > > > can have a look. > > > > To make this discussion in that regard a bit easier, I've committed the > > proposed spec to a separate branch: > > > > https://github.com/yoe/nbd/blob/tlsspec/doc/proto.txt > > So, if I'm understanding correctly the new style "fixed" handshake > currently works like this: > > Server starts transmission with version info > > - S: "NBDMAGIC" > - S: 0x49484156454F5054 > - S: 0x1 > > And the client acknowledges with a 32 bits of and first bit set > > - C: 0x1 > > > Now the client has to request one or more options, of which the > NBD_OPT_EXPORT_NAME (0x1) option is mandatory, so it will be the > first thing the client sends next. No, on the contrary. NBD_OPT_EXPORT_NAME _must_ be the very last thing the client sends if the client wants to actually select an export to be used. This is because for historical reasons, NBD_OPT_EXPORT_NAME can't get an NBD_REP_ACK, but must immediately introduce the next phase of the protocol (where data is flowing across the channel). It should maybe be renamed to something like "NBD_OPT_SELECT_AND_GO" or some such, but I'm not sure that makes much sense. > So it would send > > - C: 0x49484156454F5054 > - C: 0x1 > - C: 0xa (length of name) > - C: "volumename" > > > > You're proposing to add a new option NBD_OPT_STARTTLS (0x5). For this to > work we would need to update the language to say that the NBD_OPT_STARTTLS > must be the first option that the client sends to the server, because we > want the option name to transmit in ciphertext. Yes, my proposed protocol allows that. > So the new protocol startup would be > > Server starts transmission with version info > > - S: "NBDMAGIC" > - S: 0x49484156454F5054 > - S: 0x1 > > And the client acknowledges with a 32 bits of zero > > - C: 0x1 > > Client requests TLS > > - C: 0x49484156454F5054 > - C: 0x5 > > Server acknowledges TLS: > > - S: 0x3e889045565a9 > - S: 0x5 > - S: 0x1 (REP_ACK) > - S: 0x0 > > Now the TLS handshake is performed by client + server > > > > The client can now set other options using the secure > channel, of which the NBD_OPT_EXPORT_NAME (0x1) option > is mandatory, so it will be the first thing the client (last thing) > sends next. So they would send > > - C: "0x49484156454F5054" > - C: 0x1 > - C: 0xa (length of name) > - C: "volumename" > > ....etc... > > > This is secure when both client and server want to use TLS. > > This is secure when the client wants TLS and the server does > not want TLS, because the server will reject TLS and the client > will then close the connection. > > My concern is the case where the client does not want TLS but > the server does want TLS. In this case the client will immediately > send the NBD_OPT_EXPORT_NAME in clear text over the wire, not giving > the server any chance to reject the use of a clear text channel. In that case (if the client doesn't want TLS and also doesn't want to negotiate anything else apart from a name), the server should disconnect (anything else will fail to communicate with older clients). > This problem is inherant to the approach of using the NBD protocol > options to negotiate TLS. True. > One way I see to solve that insecurity, would be for the server > to make use of one of the extra reserved bits in the very first > message it sends. ie we need to negotiate TLS immediately after the > version number / magic acknowledgment, before we actually start > the main body of the protocol > > ie, with the new style fixed handshake the server should send > > Server starts transmission with version info > > - S: "NBDMAGIC" > - S: 0x49484156454F5054 > - S: 0x2 That would be 0x4, because 0x2 is already used for something else (yes, yes, details). > And the client acknowledges with a 32bits and first two bits set > > - C: 0x2 > > The questionmark here is what happens when the client sees the > second bit of reserved field set ? > > The protocol docs merely say > > "S: 16 bits of zero (bits 1-15 reserved for future use; bit 0 in use to > signal fixed newstyle (see below))" > > But don't mention what clients should do upon seeing unknown bits > set in the server's message ? If the client doesn't send the bit you proposed, then the server would know it doesn't understand the TLS variant of the protocol and would then either fall back to unencrypted or drop the connection (depending on policy). I'm not sure using a separate bit for this is necessary, though. The server is supposed to send NBD_REP_ERR_UNSUP for messages it doesn't understand, so a client that talks to a server which doesn't support the STARTTLS message should be able to communicate just fine (if client policy allows unencrypted communication, yada yada). > If clients ignore unknown bits, then we have the same problem where a > client can ignore the TLS bit and start sending option name in the > clear before the server rejects the session. > > If clients abort (close connection) on seeing unknown bits, then we > are good. > > > > A 3rd alternative is to actually use a separate magic number, which > should guarantee the client would immediately close upon seeing a > magic number which indicated TLS is required. Yes, but that introduces a backwards incompatible change for no good reason (the other two options are backwards-compatible), so is not an option for me. -- It is easy to love a country that is famous for chocolate and beer -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list