[PATCH nbdkit] protocol: Implement NBD_OPT_GO.
This is only lightly tested (against just qemu NBD client), and the code might be structured a little better as the _negotiate_handshake_newstyle_options function has now grown to be huge. Anyway works for me. Rich.
--- src/connections.c | 234 +++++++++++++++++++++++++++++++++++++--------- src/protocol.h | 11 +++ 2 files changed, 201 insertions(+), 44 deletions(-) diff --git a/src/connections.c b/src/connections.c index ba6e91d..5f09984 100644 --- a/src/connections.c +++ b/src/connections.c @@ -75,7 +75,9 @@ struct connection { void **handles; size_t nr_handles; + uint32_t cflags; uint64_t exportsize; + uint16_t eflags; int readonly; int can_flush; int is_rotational; @@ -572,6 +574,55 @@ send_newstyle_option_reply_exportname (struct connection *conn, return 0; } +static int +send_newstyle_option_reply_info_export (struct connection *conn, + uint32_t option, uint32_t reply, + uint16_t info) +{ + struct fixed_new_option_reply fixed_new_option_reply; + struct fixed_new_option_reply_info_export export; + + fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC); + fixed_new_option_reply.option = htobe32 (option); + fixed_new_option_reply.reply = htobe32 (reply); + fixed_new_option_reply.replylen = htobe32 (sizeof export); + export.info = htobe16 (info); + export.exportsize = htobe64 (conn->exportsize); + export.eflags = htobe16 (conn->eflags); + + if (conn->send (conn, + &fixed_new_option_reply, + sizeof fixed_new_option_reply) == -1 || + conn->send (conn, &export, sizeof export) == -1) { + nbdkit_error ("write: %m"); + return -1; + } + + return 0; +} + +static int +get_size_and_eflags (struct connection *conn) +{ + int64_t r; + + r = backend->get_size (backend, conn); + if (r == -1) + return -1; + if (r < 0) { + nbdkit_error (".get_size function returned invalid value " + "(%" PRIi64 ")", r); + return -1; + } + conn->exportsize = (uint64_t) r; + + if (compute_eflags (conn, &conn->eflags) < 0) + return -1; + + debug ("newstyle negotiation: flags: export 0x%x", conn->eflags); + return 0; +} + static int _negotiate_handshake_newstyle_options (struct connection *conn) { @@ -581,6 +632,7 @@ _negotiate_handshake_newstyle_options (struct connection *conn) uint32_t option; uint32_t optlen; char data[MAX_OPTION_LENGTH+1]; + struct new_handshake_finish handshake_finish; for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) { if (conn->recv (conn, &new_option, sizeof new_option) == -1) { @@ -625,7 +677,8 @@ _negotiate_handshake_newstyle_options (struct connection *conn) } /* Apart from printing it, ignore the export name. */ data[optlen] = '\0'; - debug ("newstyle negotiation: client requested export '%s' (ignored)", + debug ("newstyle negotiation: NBD_OPT_EXPORT_NAME: " + "client requested export '%s' (ignored)", data); break; @@ -701,6 +754,100 @@ _negotiate_handshake_newstyle_options (struct connection *conn) } break; + case NBD_OPT_GO: + if (conn->recv (conn, data, optlen) == -1) { + nbdkit_error ("read: %m"); + return -1; + } + + if (optlen < 6) { /* 32 bit export length + 16 bit nr info */ + debug ("newstyle negotiation: NBD_OPT_GO option length < 6"); + + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) + == -1) + return -1; + continue; + } + + { + uint32_t exportnamelen; + uint16_t nrinfos; + uint16_t info; + size_t i; + CLEANUP_FREE char *requested_exportname = NULL; + + /* Validate the name length and number of INFO requests. */ + memcpy (&exportnamelen, &data[0], 4); + exportnamelen = be32toh (exportnamelen); + if (exportnamelen+6 > optlen) { + debug ("newstyle negotiation: NBD_OPT_GO: export name too long"); + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) + == -1) + return -1; + continue; + } + memcpy (&nrinfos, &data[exportnamelen+4], 2); + nrinfos = be16toh (nrinfos); + if (optlen != 4 + exportnamelen + 2 + 2*nrinfos) { + debug ("newstyle negotiation: NBD_OPT_GO: number of information requests incorrect"); + if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID) + == -1) + return -1; + continue; + } + + /* As with NBD_OPT_EXPORT_NAME we print the export name and then + * ignore it. + */ + requested_exportname = malloc (exportnamelen+1); + if (requested_exportname == NULL) { + nbdkit_error ("malloc: %m"); + return -1; + } + memcpy (requested_exportname, &data[4], exportnamelen); + requested_exportname[exportnamelen] = '\0'; + debug ("newstyle negotiation: NBD_OPT_GO: " + "client requested export '%s' (ignored)", + requested_exportname); + + /* The spec is confusing, but it is required that we send back + * NBD_INFO_EXPORT, even if the client did not request it! + * qemu client in particular does not request this, but will + * fail if we don't send it. + */ + if (get_size_and_eflags (conn) == -1) + return -1; + if (send_newstyle_option_reply_info_export (conn, option, + NBD_REP_INFO, + NBD_INFO_EXPORT) == -1) + return -1; + + /* For now we ignore all other info requests (but we must + * ignore NBD_INFO_EXPORT if it was requested, because we + * replied already above). Therefore this loop doesn't do + * much at the moment. + */ + for (i = 0; i < nrinfos; ++i) { + memcpy (&info, &data[4+exportnamelen+2+i*2], 2); + info = be16toh (info); + switch (info) { + case NBD_INFO_EXPORT: /* ignore - reply sent above */ break; + default: + debug ("newstyle negotiation: NBD_OPT_GO: " + "ignoring NBD_INFO_* request %u", (unsigned) info); + break; + } + } + } + + /* Unlike NBD_OPT_EXPORT_NAME, NBD_OPT_GO sends back an ACK + * or ERROR packet. + */ + if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1) + return -1; + + break; + default: /* Unknown option. */ if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_UNSUP) == -1) @@ -712,10 +859,10 @@ _negotiate_handshake_newstyle_options (struct connection *conn) } /* Note, since it's not very clear from the protocol doc, that the - * client must send NBD_OPT_EXPORT_NAME last, and that ends option - * negotiation. + * client must send NBD_OPT_EXPORT_NAME or NBD_OPT_GO last, and + * that ends option negotiation. */ - if (option == NBD_OPT_EXPORT_NAME) + if (option == NBD_OPT_EXPORT_NAME || option == NBD_OPT_GO) break; } @@ -733,6 +880,40 @@ _negotiate_handshake_newstyle_options (struct connection *conn) return -1; } + /* If the last option was NBD_OPT_EXPORT_NAME then we have to + * finish the handshake by sending handshake_finish. + */ + if (option == NBD_OPT_EXPORT_NAME) { + finish: + if (get_size_and_eflags (conn) == -1) + return -1; + + memset (&handshake_finish, 0, sizeof handshake_finish); + handshake_finish.exportsize = htobe64 (conn->exportsize); + handshake_finish.eflags = htobe16 (conn->eflags); + + if (conn->send (conn, + &handshake_finish, + (conn->cflags & NBD_FLAG_NO_ZEROES) + ? offsetof (struct new_handshake_finish, zeroes) + : sizeof handshake_finish) == -1) { + nbdkit_error ("write: %m"); + return -1; + } + } + /* If the last option was NBD_OPT_GO then we're all done. */ + else if (option == NBD_OPT_GO) + ; + /* Else the client didn't end the list of options properly. This + * may be a buggy client, but nbdkit has always continued + * regardless. However emit a debug message. + */ + else { + debug ("warning: client options list didn't finish with NBD_OPT_GO " + "or NBD_OPT_EXPORT_NAME"); + goto finish; + } + return 0; } @@ -741,11 +922,6 @@ _negotiate_handshake_newstyle (struct connection *conn) { struct new_handshake handshake; uint16_t gflags; - uint32_t cflags; - struct new_handshake_finish handshake_finish; - int64_t r; - uint64_t exportsize; - uint16_t eflags; gflags = NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES; @@ -761,15 +937,15 @@ _negotiate_handshake_newstyle (struct connection *conn) } /* Client now sends us its 32 bit flags word ... */ - if (conn->recv (conn, &cflags, sizeof cflags) == -1) { + if (conn->recv (conn, &conn->cflags, 4) == -1) { nbdkit_error ("read: %m"); return -1; } - cflags = be32toh (cflags); + conn->cflags = be32toh (conn->cflags); /* ... which we check for accuracy. */ - debug ("newstyle negotiation: client flags: 0x%x", cflags); - if (cflags & ~gflags) { - nbdkit_error ("client requested unknown flags 0x%x", cflags); + debug ("newstyle negotiation: client flags: 0x%x", conn->cflags); + if (conn->cflags & ~gflags) { + nbdkit_error ("client requested unknown flags 0x%x", conn->cflags); return -1; } @@ -777,36 +953,6 @@ _negotiate_handshake_newstyle (struct connection *conn) if (_negotiate_handshake_newstyle_options (conn) == -1) return -1; - /* Finish the newstyle handshake. */ - r = backend->get_size (backend, conn); - if (r == -1) - return -1; - if (r < 0) { - nbdkit_error (".get_size function returned invalid value " - "(%" PRIi64 ")", r); - return -1; - } - exportsize = (uint64_t) r; - conn->exportsize = exportsize; - - if (compute_eflags (conn, &eflags) < 0) - return -1; - - debug ("newstyle negotiation: flags: export 0x%x", eflags); - - memset (&handshake_finish, 0, sizeof handshake_finish); - handshake_finish.exportsize = htobe64 (exportsize); - handshake_finish.eflags = htobe16 (eflags); - - if (conn->send (conn, - &handshake_finish, - (cflags & NBD_FLAG_NO_ZEROES) - ? offsetof (struct new_handshake_finish, zeroes) - : sizeof handshake_finish) == -1) { - nbdkit_error ("write: %m"); - return -1; - } - return 0; } diff --git a/src/protocol.h b/src/protocol.h index aa458a0..c02e164 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -100,15 +100,26 @@ struct new_handshake_finish { #define NBD_OPT_ABORT 2 #define NBD_OPT_LIST 3 #define NBD_OPT_STARTTLS 5 +#define NBD_OPT_GO 7 #define NBD_REP_ACK 1 #define NBD_REP_SERVER 2 +#define NBD_REP_INFO 3 #define NBD_REP_ERR_UNSUP 0x80000001 #define NBD_REP_ERR_POLICY 0x80000002 #define NBD_REP_ERR_INVALID 0x80000003 #define NBD_REP_ERR_PLATFORM 0x80000004 #define NBD_REP_ERR_TLS_REQD 0x80000005 +#define NBD_INFO_EXPORT 0 + +/* NBD_INFO_EXPORT reply (follows fixed_new_option_reply). */ +struct fixed_new_option_reply_info_export { + uint16_t info; /* NBD_INFO_EXPORT */ + uint64_t exportsize; /* size of export */ + uint16_t eflags; /* per-export flags */ +} __attribute__((packed)); + /* Request (client -> server). */ struct request { uint32_t magic; /* NBD_REQUEST_MAGIC. */ -- 2.18.0
On Sat, Aug 4, 2018 at 4:04 PM Richard W.M. Jones <rjones@redhat.com> wrote:
This is only lightly tested (against just qemu NBD client), and the code might be structured a little better as the _negotiate_handshake_newstyle_options function has now grown to be huge. Anyway works for me.
Works for my python nbd client: $ rm -f /tmp/nbd.sock && src/nbdkit -f plugins/file/.libs/nbdkit-file-plugin.so file=test.raw -e export -U /tmp/nbd.sock $ python -c "import logging; logging.basicConfig(level=logging.DEBUG); from ovirt_imageio_common import nbd; c = nbd.Client('/tmp/nbd.sock', 'export'); c.write(1024**2, 'it works'); print c.read(1024**2, 8)" INFO:nbd:Connecting to '/tmp/nbd.sock' 'export' DEBUG:nbd:Received server flags: 3 DEBUG:nbd:Sending client flags: 1: DEBUG:nbd:Sending option: 'IHAVEOPT\x00\x00\x00\x07\x00\x00\x00\x0c' data: bytearray(b'\x00\x00\x00\x06export\x00\x00') DEBUG:nbd:Received reply [magic=3e889045565a9 option=7 type=3 len=12] DEBUG:nbd:Received export info [size=1073741824 flags=109] DEBUG:nbd:Received reply [magic=3e889045565a9 option=7 type=1 len=0] INFO:nbd:Ready for transmission it works Nir
On Sat, Aug 4, 2018 at 10:58 PM Nir Soffer <nsoffer@redhat.com> wrote:
On Sat, Aug 4, 2018 at 4:04 PM Richard W.M. Jones <rjones@redhat.com> wrote:
This is only lightly tested (against just qemu NBD client), and the code might be structured a little better as the _negotiate_handshake_newstyle_options function has now grown to be huge. Anyway works for me.
Works for my python nbd client:
$ rm -f /tmp/nbd.sock && src/nbdkit -f plugins/file/.libs/nbdkit-file-plugin.so file=test.raw -e export -U /tmp/nbd.sock
$ python -c "import logging; logging.basicConfig(level=logging.DEBUG); from ovirt_imageio_common import nbd; c = nbd.Client('/tmp/nbd.sock', 'export'); c.write(1024**2, 'it works'); print c.read(1024**2, 8)" INFO:nbd:Connecting to '/tmp/nbd.sock' 'export' DEBUG:nbd:Received server flags: 3 DEBUG:nbd:Sending client flags: 1: DEBUG:nbd:Sending option: 'IHAVEOPT\x00\x00\x00\x07\x00\x00\x00\x0c' data: bytearray(b'\x00\x00\x00\x06export\x00\x00') DEBUG:nbd:Received reply [magic=3e889045565a9 option=7 type=3 len=12] DEBUG:nbd:Received export info [size=1073741824 flags=109] DEBUG:nbd:Received reply [magic=3e889045565a9 option=7 type=1 len=0] INFO:nbd:Ready for transmission it works
But we have a bug - server configure to allow access to "export", but allow access to the default empty "" export. $ python -c "import logging; logging.basicConfig(level=logging.DEBUG); from ovirt_imageio_common import nbd; c = nbd.Client('/tmp/nbd.sock', '')" INFO:nbd:Connecting to '/tmp/nbd.sock' '' DEBUG:nbd:Received server flags: 3 DEBUG:nbd:Sending client flags: 1: DEBUG:nbd:Sending option: 'IHAVEOPT\x00\x00\x00\x07\x00\x00\x00\x06' data: bytearray(b'\x00\x00\x00\x00\x00\x00') DEBUG:nbd:Received reply [magic=3e889045565a9 option=7 type=3 len=12] DEBUG:nbd:Received export info [size=6442450944 flags=109] DEBUG:nbd:Received reply [magic=3e889045565a9 option=7 type=1 len=0] INFO:nbd:Ready for transmission Same with qemu-nbd: $ qemu-nbd -k /tmp/nbd.sock -t -f raw test.raw --export-name export --cache=none --aio=native --discard=unmap --detect-zeroes=unmap $ python -c "import logging; logging.basicConfig(level=logging.DEBUG); from ovirt_imageio_common import nbd; c = nbd.Client('/tmp/nbd.sock', '')" INFO:nbd:Connecting to '/tmp/nbd.sock' '' DEBUG:nbd:Received server flags: 3 DEBUG:nbd:Sending client flags: 1: DEBUG:nbd:Sending option: 'IHAVEOPT\x00\x00\x00\x07\x00\x00\x00\x06' data: bytearray(b'\x00\x00\x00\x00\x00\x00') DEBUG:nbd:Received reply [magic=3e889045565a9 option=7 type=80000006 len=21] Traceback (most recent call last): File "<string>", line 1, in <module> File "ovirt_imageio_common/nbd.py", line 126, in __init__ self._newstyle_handshake(export_name) File "ovirt_imageio_common/nbd.py", line 181, in _newstyle_handshake self._receive_go_reply() File "ovirt_imageio_common/nbd.py", line 206, in _receive_go_reply .format(ERROR_REPLY[reply], message)) ovirt_imageio_common.nbd.Error: The requested export is not available [message=export '' not present])
On Sun, Aug 05, 2018 at 12:11:04AM +0300, Nir Soffer wrote:
But we have a bug - server configure to allow access to "export", but allow access to the default empty "" export.
nbdkit ignores the export name you pass in and always has done: https://github.com/libguestfs/nbdkit/blob/a2c4e88af503310892ebce6da3ac478beb... The -e option on the command line is only for advertizing an export name to clients. It is otherwise unused & unenforced. One day we'll do something useful with it but we're not sure what. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 08/06/2018 02:54 AM, Richard W.M. Jones wrote:
On Sun, Aug 05, 2018 at 12:11:04AM +0300, Nir Soffer wrote:
But we have a bug - server configure to allow access to "export", but allow access to the default empty "" export.
nbdkit ignores the export name you pass in and always has done:
https://github.com/libguestfs/nbdkit/blob/a2c4e88af503310892ebce6da3ac478beb...
The -e option on the command line is only for advertizing an export name to clients. It is otherwise unused & unenforced.
Agreed - it is not necessarily a bug for the server to permit the client to connect to different name(s) than what the server was told to advertise.
One day we'll do something useful with it but we're not sure what.
We might eventually reject clients that don't request the same export name as what was advertised, but that doesn't make the current behavior buggy (it only means that current clients shouldn't rely on a mismatched name always working). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Mon, Aug 6, 2018 at 5:06 PM Eric Blake <eblake@redhat.com> wrote:
On 08/06/2018 02:54 AM, Richard W.M. Jones wrote:
On Sun, Aug 05, 2018 at 12:11:04AM +0300, Nir Soffer wrote:
But we have a bug - server configure to allow access to "export", but allow access to the default empty "" export.
nbdkit ignores the export name you pass in and always has done:
https://github.com/libguestfs/nbdkit/blob/a2c4e88af503310892ebce6da3ac478beb...
The -e option on the command line is only for advertizing an export name to clients. It is otherwise unused & unenforced.
Agreed - it is not necessarily a bug for the server to permit the client to connect to different name(s) than what the server was told to advertise.
One day we'll do something useful with it but we're not sure what.
We might eventually reject clients that don't request the same export name as what was advertised, but that doesn't make the current behavior buggy (it only means that current clients shouldn't rely on a mismatched name always working).
I does not make sense to allow access if the server was configured with one export, and the user ask for another export. This is likely a bug in user code or the code running nbdkit. If users do not want to specify the export, they run nbdkit with empty export name, and use empty export name in NBD_OPT_GO. If an export was specified it should be handled in a strict way. Here is one use case that will be easily avoided by being strict about export name: 1. application starts nbdkit on a port 2. application reports port to user 3. application time out, terminating nbdkit 4. applcation starts nbdkit again on same port (maybe port range is cycled) 5. application reports port to another user 6. first user access nbdkit, accessing another user disk If we use random export name, and the server is strict, this failure is not possible. Eric, can you point us to the part of the spec allowing ignoring the export name sent by the client? Regardless since this is the current behavior we can improve this later. Nir
On 08/06/2018 09:31 AM, Nir Soffer wrote:
Eric, can you point us to the part of the spec allowing ignoring the export name sent by the client?
Nothing in the NBD spec requires the server to reject unknown export names. So nbdkit never rejects export names (which means it behaves as if all names work, regardless of whether or not it was the name it was configured with, since it exports exactly one volume all the time). An NBD server that exports more than one volume at a time (such as qemu via QMP [but oddly not qemu-nbd], or nbd-server) DOES have to pay attention to export names. And if nbdkit were ever taught to manage more than one export at once, then that would also imply that export names matter. But until then, I don't see nbdkit's behavior as a spec violation. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Mon, Aug 6, 2018 at 5:46 PM Eric Blake <eblake@redhat.com> wrote:
On 08/06/2018 09:31 AM, Nir Soffer wrote:
Eric, can you point us to the part of the spec allowing ignoring the export name sent by the client?
Nothing in the NBD spec requires the server to reject unknown export names. So nbdkit never rejects export names (which means it behaves as if all names work, regardless of whether or not it was the name it was configured with, since it exports exactly one volume all the time).
The spec mention this about NBD_OPT_EXPORT_NAME: If the chosen export does not exist or requirements for the chosen export are not met (e.g., the client did not initiate TLS for an export where the server requires it), the server MUST terminate the session. and for NBD_OPT_GO, we have NBD_REP_ERR_UNKNOWN: The chosen export does not exist on this server. In this case, the server SHOULD NOT send NBD_REP_INFO replies. And the spec also says: If the server is unwilling to allow the export, it MUST terminate the session Why would a server want to allow an export using different name? What is the point of the export name in this case? Again, if export name is not useful when a server export only one export, both server and client can use the default empty export name. Nir
On Mon, Aug 06, 2018 at 08:05:00PM +0300, Nir Soffer wrote:
On Mon, Aug 6, 2018 at 5:46 PM Eric Blake <eblake@redhat.com> wrote:
On 08/06/2018 09:31 AM, Nir Soffer wrote:
Eric, can you point us to the part of the spec allowing ignoring the export name sent by the client?
Nothing in the NBD spec requires the server to reject unknown export names. So nbdkit never rejects export names (which means it behaves as if all names work, regardless of whether or not it was the name it was configured with, since it exports exactly one volume all the time).
The spec mention this about NBD_OPT_EXPORT_NAME:
If the chosen export does not exist or requirements for the chosen export are not met (e.g., the client did not initiate TLS for an export where the server requires it), the server MUST terminate the session.
and for NBD_OPT_GO, we have NBD_REP_ERR_UNKNOWN:
The chosen export does not exist on this server. In this case, the server SHOULD NOT send NBD_REP_INFO replies.
And the spec also says:
If the server is unwilling to allow the export, it MUST terminate the session
Why would a server want to allow an export using different name? What is the point of the export name in this case?
Again, if export name is not useful when a server export only one export, both server and client can use the default empty export name.
I still don't believe that we're contravening the protocol, since we get to define what the "requirements" are (ie. none). Also "SHOULD" not "MUST". The real problem is that export name is a barrier to usage. Beginners have no idea what it is, why it's needed, what it is set to, or how to find out. They will connect to the server, it will fail, they'll give up and think NBD or nbdkit is broken. So I don't want to enforce export names until they have a purpose, which they don't have in nbdkit because we can only ever have a single plugin loaded. In the case you mentioned before of a client connecting to the wrong server, I would suggest using PSK with a fresh key each time. This has the additional benefit of being secure. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On 08/06/2018 12:05 PM, Nir Soffer wrote:
On Mon, Aug 6, 2018 at 5:46 PM Eric Blake <eblake@redhat.com> wrote:
On 08/06/2018 09:31 AM, Nir Soffer wrote:
Eric, can you point us to the part of the spec allowing ignoring the export name sent by the client?
Nothing in the NBD spec requires the server to reject unknown export names. So nbdkit never rejects export names (which means it behaves as if all names work, regardless of whether or not it was the name it was configured with, since it exports exactly one volume all the time).
The spec mention this about NBD_OPT_EXPORT_NAME:
If the chosen export does not exist or requirements for the chosen export are not met (e.g., the client did not initiate TLS for an export where the server requires it), the server MUST terminate the session.
But in the case of nbdkit, ALL export names exist (not just the one you configured with), since the name has no meaning other than what is reported over the wire to NBD_OPT_LIST.
and for NBD_OPT_GO, we have NBD_REP_ERR_UNKNOWN:
The chosen export does not exist on this server. In this case, the server SHOULD NOT send NBD_REP_INFO replies.
That's what happens when the server does not want to accept the name given by the client, but does not prevent a server from accepting ALL names (by ignoring the name).
And the spec also says:
If the server is unwilling to allow the export, it MUST terminate the session
Why would a server want to allow an export using different name? What is the point of the export name in this case?
In the case of nbdkit, the point of not caring about names is a matter of convenience - with exactly one thing being exported, naming that thing doesn't matter, so we can get to the connection faster and with less code without having to validate something that didn't matter. As mentioned elsewhere in the thread, someday we may have a reason to care, at which point name validation will make sense. And a portable client cannot blindly assume that it is going to work to ask for a different name than what is returned in NBD_OPT_LIST (but note also that the spec does not require clients to call NBD_OPT_LIST in the first place). But at the same time, a client cannot assume that only the names returned in NBD_OPT_LIST will work.
Again, if export name is not useful when a server export only one export, both server and client can use the default empty export name.
Yes, the empty name is one (of a large number, but not truly infinite, due to length limitations) of the possible valid names that nbdkit otherwise ignores. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Mon, Aug 06, 2018 at 05:31:54PM +0300, Nir Soffer wrote:
On Mon, Aug 6, 2018 at 5:06 PM Eric Blake <eblake@redhat.com> wrote:
On 08/06/2018 02:54 AM, Richard W.M. Jones wrote:
On Sun, Aug 05, 2018 at 12:11:04AM +0300, Nir Soffer wrote:
But we have a bug - server configure to allow access to "export", but allow access to the default empty "" export.
nbdkit ignores the export name you pass in and always has done:
https://github.com/libguestfs/nbdkit/blob/a2c4e88af503310892ebce6da3ac478beb...
The -e option on the command line is only for advertizing an export name to clients. It is otherwise unused & unenforced.
Agreed - it is not necessarily a bug for the server to permit the client to connect to different name(s) than what the server was told to advertise.
One day we'll do something useful with it but we're not sure what.
We might eventually reject clients that don't request the same export name as what was advertised, but that doesn't make the current behavior buggy (it only means that current clients shouldn't rely on a mismatched name always working).
I does not make sense to allow access if the server was configured with one export, and the user ask for another export. This is likely a bug in user code or the code running nbdkit.
If users do not want to specify the export, they run nbdkit with empty export name, and use empty export name in NBD_OPT_GO. If an export was specified it should be handled in a strict way.
Here is one use case that will be easily avoided by being strict about export name:
1. application starts nbdkit on a port 2. application reports port to user 3. application time out, terminating nbdkit 4. applcation starts nbdkit again on same port (maybe port range is cycled) 5. application reports port to another user 6. first user access nbdkit, accessing another user disk
If we use random export name, and the server is strict, this failure is not possible.
Export name validation should not be considered a security feature, as the export name is not designated or treated as sensitive data. If you want to prevent one user access other users' disks, whether by accidental or maliciously, you need to use TLS and validate clients certificates in the server, or use the new PSK credentials. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Aug 6, 2018 at 5:51 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Mon, Aug 06, 2018 at 05:31:54PM +0300, Nir Soffer wrote:
On Mon, Aug 6, 2018 at 5:06 PM Eric Blake <eblake@redhat.com> wrote:
On 08/06/2018 02:54 AM, Richard W.M. Jones wrote:
On Sun, Aug 05, 2018 at 12:11:04AM +0300, Nir Soffer wrote:
But we have a bug - server configure to allow access to "export", but allow access to the default empty "" export.
nbdkit ignores the export name you pass in and always has done:
https://github.com/libguestfs/nbdkit/blob/a2c4e88af503310892ebce6da3ac478beb...
The -e option on the command line is only for advertizing an export name to clients. It is otherwise unused & unenforced.
Agreed - it is not necessarily a bug for the server to permit the client to connect to different name(s) than what the server was told to advertise.
One day we'll do something useful with it but we're not sure what.
We might eventually reject clients that don't request the same export name as what was advertised, but that doesn't make the current behavior buggy (it only means that current clients shouldn't rely on a mismatched name always working).
I does not make sense to allow access if the server was configured with one export, and the user ask for another export. This is likely a bug in user code or the code running nbdkit.
If users do not want to specify the export, they run nbdkit with empty export name, and use empty export name in NBD_OPT_GO. If an export was specified it should be handled in a strict way.
Here is one use case that will be easily avoided by being strict about export name:
1. application starts nbdkit on a port 2. application reports port to user 3. application time out, terminating nbdkit 4. applcation starts nbdkit again on same port (maybe port range is cycled) 5. application reports port to another user 6. first user access nbdkit, accessing another user disk
If we use random export name, and the server is strict, this failure is not possible.
Export name validation should not be considered a security feature, as the export name is not designated or treated as sensitive data.
I did not say this is a security issue. If you want to prevent one user access other users' disks, whether by
accidental or maliciously, you need to use TLS and validate clients certificates in the server, or use the new PSK credentials.
TLS will work but is not needed if you don't need a secure transport. Nir
participants (4)
-
Daniel P. Berrangé -
Eric Blake -
Nir Soffer -
Richard W.M. Jones