platform/upstream/nbd.git
8 years agoAdd a "wrong certificate used" test
Wouter Verhelst [Mon, 21 Nov 2016 08:13:33 +0000 (09:13 +0100)]
Add a "wrong certificate used" test

We want to fail authentication when the certificate in use is one not
signed by the correct CA, so ensure that that happens.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoRe-add the SERVER_PRECEDENCE flag
Wouter Verhelst [Sun, 20 Nov 2016 20:21:50 +0000 (21:21 +0100)]
Re-add the SERVER_PRECEDENCE flag

That's still a good idea, even if we require TLS1.2

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoSwap hostname and tlshostname
Wouter Verhelst [Sun, 20 Nov 2016 20:19:03 +0000 (21:19 +0100)]
Swap hostname and tlshostname

The "tlshostname" argument must match the CN attribute on the server's
certificate if we want SNI to work. Since that is set to "localhost", we
should make sure that the -H argument actually matches that, or this
test may fail (depending on GnuTLS version)

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoReorder tls initialization
Wouter Verhelst [Sun, 20 Nov 2016 19:54:17 +0000 (20:54 +0100)]
Reorder tls initialization

If we're going to check whether tlshostname != NULL, then make sure we
don't just always set it to !NULL the line before.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoUpdate priority string
Wouter Verhelst [Sun, 20 Nov 2016 19:46:39 +0000 (20:46 +0100)]
Update priority string

We want to disallow TLS <1.2, as per spec, so update the priority string
to do so.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoMove DH initialization into the start of the program
Wouter Verhelst [Sun, 20 Nov 2016 19:44:42 +0000 (20:44 +0100)]
Move DH initialization into the start of the program

Initializing DH parameters takes a while (~1s on a 2.3GHz Intel Haswell
core i7). Rather than doing it once per connection, do it once per
nbd-server run.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoUpdate HAVE_GNUTLS checks in nbd-tester-client, too
Wouter Verhelst [Wed, 9 Nov 2016 23:09:12 +0000 (00:09 +0100)]
Update HAVE_GNUTLS checks in nbd-tester-client, too

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoImprove TLS-related help output
Wouter Verhelst [Wed, 9 Nov 2016 23:07:53 +0000 (00:07 +0100)]
Improve TLS-related help output

- Make it fit without 80 columns
- Add note about -x parameter, too

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoSigh
Wouter Verhelst [Wed, 9 Nov 2016 23:03:20 +0000 (00:03 +0100)]
Sigh

Add the missing "x"

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoFix check for HAVE_GNUTLS
Wouter Verhelst [Wed, 9 Nov 2016 22:57:47 +0000 (23:57 +0100)]
Fix check for HAVE_GNUTLS

If we always define a variable (but simply set it to 0 sometimes), we
cannot use #ifdef but must use #if instead.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoFix shell syntaxis
Wouter Verhelst [Wed, 9 Nov 2016 22:50:57 +0000 (23:50 +0100)]
Fix shell syntaxis

The "test" command takes one or more arguments, which are then evaluated
in the shell before processing. An assignment results in an exit state,
which as far as test is concerned, is fine enough.

Since we wrote "x$HAVE_GNUTLS=1", that would get expanded to "x0=1",
meaning, we were assigning the value 1 to the variable "x0", which meant
that "test" was seeing one argument rather than the three that we wanted
to give it, and so it was happy.

Fix by adding the required spaces to make test see the equals sign and
perform the comparison.

Shell is a horrible language.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoRemove last reference to tls_dir option
Wouter Verhelst [Wed, 9 Nov 2016 22:10:48 +0000 (23:10 +0100)]
Remove last reference to tls_dir option

This option no longer exists, so we shouldn't reference it anymore.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoSet GnuTLS 2.12.0 as the minimum version
Alex Bligh [Wed, 9 Nov 2016 12:25:17 +0000 (12:25 +0000)]
Set GnuTLS 2.12.0 as the minimum version

We now compile against GnuTLS 2.12.0 (probably an earlier version would work
too, but I don't have one right here to test against).

Signed-off-by: Alex Bligh <alex@alex.org.uk>
8 years agognutls_transport_set_int is only available for gnutls 3.1.9 or later
Alex Bligh [Wed, 9 Nov 2016 12:17:47 +0000 (12:17 +0000)]
gnutls_transport_set_int is only available for gnutls 3.1.9 or later

gnutls_transport_set_int is only available for gnutls 3.1.9 or later so
on earlier versions use gnutls_transport_set_ptr with a cast, or more
accurately two casts (ugly), as that eliminates a warning about a cast
from an integer to a pointer of a different size.

Signed-off-by: Alex Bligh <alex@alex.org.uk>
8 years agoGNUTLS_SEC_PARAM_MEDIUM was previously called GNUTLS_SEC_PARAM_NORMAL
Alex Bligh [Wed, 9 Nov 2016 12:06:38 +0000 (12:06 +0000)]
GNUTLS_SEC_PARAM_MEDIUM was previously called GNUTLS_SEC_PARAM_NORMAL

Fix one compilation error on early GnuTLS thanks to their non-backwards
compatible API change.

Signed-off-by: Alex Bligh <alex@alex.org.uk>
8 years agoUpdate crypto-gnutls.c to upstream
Alex Bligh [Wed, 9 Nov 2016 11:47:58 +0000 (11:47 +0000)]
Update crypto-gnutls.c to upstream

Update crypto-gnutls.c to upstream to fix bug when GNU_TLS_E_AGAIN
is returned. See:
   https://github.com/abligh/tlsproxy/issues/1

Signed-off-by: Alex Bligh <alex@alex.org.uk>
8 years agoFix defines again
Wouter Verhelst [Tue, 8 Nov 2016 23:36:21 +0000 (00:36 +0100)]
Fix defines again

s/WITH_GNUTLS/HAVE_GNUTLS/g

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoAllow anonymous TLS, too
Wouter Verhelst [Tue, 8 Nov 2016 23:27:25 +0000 (00:27 +0100)]
Allow anonymous TLS, too

Certificate authentication is nice, but we shouldn't require it.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoDocument nbd-client options
Wouter Verhelst [Tue, 8 Nov 2016 23:27:02 +0000 (00:27 +0100)]
Document nbd-client options

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoRemove hostname confusion
Wouter Verhelst [Tue, 8 Nov 2016 23:24:54 +0000 (00:24 +0100)]
Remove hostname confusion

Currently, there is a "hostname" argument (which specifies the server to
connect to, and which is not an option argument) and a '-hostname'
argument (which takes the hostname for SNI connectivity in the TLS
context). The two are only tangentially related, and it could be
confusing to have two "hostname" arguments.

Rename the TLS-related one to tlshostname to clarify.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoAdd TLS support to NBD client
Alex Bligh [Tue, 12 Apr 2016 13:18:27 +0000 (14:18 +0100)]
Add TLS support to NBD client

Signed-off-by: Alex Bligh <alex@alex.org.uk>
(cherry picked from commit ad3ddfe14bce1c7fca8ab7205d210ab2e312aaac)
[wouter: fix conflict with multiple connections changes]
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoAdd options to nbd-client for TLS support
Alex Bligh [Tue, 12 Apr 2016 12:07:26 +0000 (13:07 +0100)]
Add options to nbd-client for TLS support

Signed-off-by: Alex Bligh <alex@alex.org.uk>
(cherry picked from commit 40426ea76ab4c2c37ddda96e56f9145f53c531aa)
[wouter: make --certfile be -F rather than -C, which was already taken by --connections]
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoReorder code
Wouter Verhelst [Tue, 8 Nov 2016 20:01:46 +0000 (21:01 +0100)]
Reorder code

No need to #endif out of the TLS code block if we're going to #ifdef it again
half a screen further down

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agosend_export_info(): use socket_write()
Wouter Verhelst [Tue, 8 Nov 2016 19:34:56 +0000 (20:34 +0100)]
send_export_info(): use socket_write()

This function was still using plain write() calls rather than socket_write()
ones. Fix.

This makes nbd-server survive the TLS test suite.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoFix logic error
Wouter Verhelst [Tue, 8 Nov 2016 18:50:01 +0000 (19:50 +0100)]
Fix logic error

The combined check for tls-only exports with "is this the right export"
was failing. Additionally, it was complicated code. Rather than trying
to fix it, separate the two checks and use "continue" to move on to the
next export if the tls check fails.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoDon't forget to read and check length of STARTTLS
Wouter Verhelst [Tue, 8 Nov 2016 18:39:54 +0000 (19:39 +0100)]
Don't forget to read and check length of STARTTLS

We forgot to read the "length" field of the STARTTLS command.
Additionally, we need to check that the length is in fact zero
(otherwise the STARTTLS command is invalid)

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoBe a bit clearer on failure
Wouter Verhelst [Sat, 5 Nov 2016 13:36:29 +0000 (14:36 +0100)]
Be a bit clearer on failure

Saying "it wasn't NBD_REP_ACK isn't as useful as "I received foo while I
expected NBD_REP_ACK" for debugging.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoShow some more information on why we failed
Wouter Verhelst [Sat, 5 Nov 2016 13:35:33 +0000 (14:35 +0100)]
Show some more information on why we failed

When the handshake fails, it's useful to know why exactly it fails, so
produce an error message in that case.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoDon't dereference pointers that may be NULL
Wouter Verhelst [Sat, 5 Nov 2016 13:34:42 +0000 (14:34 +0100)]
Don't dereference pointers that may be NULL

There's a test for !client, but that is done after we try to dereference
it. That can't be right.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoDereference the correct variable
Wouter Verhelst [Thu, 3 Nov 2016 18:52:23 +0000 (19:52 +0100)]
Dereference the correct variable

tlsdir no longer exists, so stop trying to read from it.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoUpdate comment to sync with reality
Wouter Verhelst [Thu, 3 Nov 2016 18:50:14 +0000 (19:50 +0100)]
Update comment to sync with reality

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoFix AC_DEFINE usage
Wouter Verhelst [Thu, 3 Nov 2016 18:49:31 +0000 (19:49 +0100)]
Fix AC_DEFINE usage

Apparently AC_DEFINE does not expand shell variables, so we need to
place it inside an if structure.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoLog something on address family mismatch
Wouter Verhelst [Sat, 5 Nov 2016 13:41:25 +0000 (14:41 +0100)]
Log something on address family mismatch

Currently, when we refuse to match an authorized_file entry for address
family mismatches, we don't say anything and instead just exit. That
works, but makes debugging harder.

Add a log message, so that users can at least figure out why things
fail.

Should help for github issue #35, although it's not a full fix.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoAdd the set_nonblocking function
Wouter Verhelst [Thu, 3 Nov 2016 18:28:58 +0000 (19:28 +0100)]
Add the set_nonblocking function

Commit be34514f857d60b453bb12a8dca626ace1dc79a5 (which we don't want)
added the set_nonblocking() function to cliserv.c, which
2a5a4f2058827279247641d2db326079b026ca15 added to nbd-tester-client.

Add this now.

Fixes: 2a5a4f2058827279247641d2db326079b026ca15
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoUse correct #ifdefs
Wouter Verhelst [Thu, 3 Nov 2016 18:24:55 +0000 (19:24 +0100)]
Use correct #ifdefs

I forgot to update nbd-tester-client.c to check the correct #ifdefs
(HAVE_GNUTLS rather than WITH_GNUTLS). Fix.

Fixes: 2a5a4f2058827279247641d2db326079b026ca15
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoImprove upon build system even further
Wouter Verhelst [Thu, 3 Nov 2016 18:22:57 +0000 (19:22 +0100)]
Improve upon build system even further

We botched the build system changes a bit, so fix them properly now.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoMake nbd-client -l work again
Wouter Verhelst [Thu, 3 Nov 2016 18:20:07 +0000 (19:20 +0100)]
Make nbd-client -l work again

We don't need to open an nbd device if all we're going to ask for is the
list of exports.

Also, nbd-client -l is a valid thing to run even if you don't have
access to /dev/nbd* (which the test suite does, for instance), so don't
even try to open the device node unless we really need to.

Fixes: 4e08f11ab9996d0c4a8c52d5acecf8b33890825f
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoAdd TLS testing to nbd-tester-client.c
Alex Bligh [Mon, 11 Apr 2016 16:46:35 +0000 (17:46 +0100)]
Add TLS testing to nbd-tester-client.c

This commit adds TLS testing to nbd-tester-client and 'make check'.
If TLS is not compiled in, then the test is skipped.

Signed-off-by: Alex Bligh <alex@alex.org.uk>
(cherry picked from commit 48065afaa8dee3eda6221660200ac473becf5c0e)
[wouter: update to make it apply in the face of handshake test]
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoClarify which group
Wouter Verhelst [Thu, 3 Nov 2016 18:02:41 +0000 (19:02 +0100)]
Clarify which group

We specify the name of the group in an extra parameter, without actually
using it, which causes a compiler warning. While removing that extra
parameter would have fixed this, stating which group we're failing on is
actually useful information, so add that to the output.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoUse correct function
Wouter Verhelst [Thu, 3 Nov 2016 18:02:08 +0000 (19:02 +0100)]
Use correct function

strcmp doesn't take a length, but strncmp does

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoFix tls options
Wouter Verhelst [Thu, 3 Nov 2016 18:00:26 +0000 (19:00 +0100)]
Fix tls options

Rather than specifying a single directory that contains all files,
specify the three options which we need by individual files.

Based on commit e851a027fe889779c091233849797718a7ae3792, originally by
Alex Bligh <alex@alex.org.uk>, but significantly altered to make it also
remove my older stuff.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoAdd GnuTLS infrastructure
Alex Bligh [Sun, 10 Apr 2016 13:33:56 +0000 (14:33 +0100)]
Add GnuTLS infrastructure

Add configure.ac section to detect GnuTLS

Add buffer.[ch] and crypto-gnutls.[ch] from
  https://github.com/abligh/tlsproxy

Add Makefile.am changes to link these new files in

Signed-off-by: Alex Bligh <alex@alex.org.uk>
(cherry picked from commit aac8f6afbaf982431b4b97c978c3d0156badbbe5)
[wouter: updated to cooperate with server-side GnuTLS support that already exists]
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agodoc: Allow NBD_OPT_ABORT in FORCEDTLS
Wouter Verhelst [Tue, 1 Nov 2016 09:28:51 +0000 (10:28 +0100)]
doc: Allow NBD_OPT_ABORT in FORCEDTLS

It does not make sense to disallow a soft disconnect when the client
cannot do TLS, so make that explicitly legal as well in the FORCEDTLS
situation.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoAllow setting the number of connections from the nbdtab file, too
Wouter Verhelst [Mon, 31 Oct 2016 02:03:28 +0000 (03:03 +0100)]
Allow setting the number of connections from the nbdtab file, too

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoDocument new options
Wouter Verhelst [Mon, 31 Oct 2016 01:54:36 +0000 (02:54 +0100)]
Document new options

The last two patches introduced new command-line options for nbd-client
and new config file options for nbd-server, but did not document them.
Fix.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agonbd-client: add support for multiple client connections
Josef Bacik [Thu, 8 Sep 2016 19:30:27 +0000 (12:30 -0700)]
nbd-client: add support for multiple client connections

Now that the kernel provides the ability to have multiple connections per NBD
device, provide an option for the user to specify how many connections to open
up.

Signed-off-by: Josef Bacik <jbacik@fb.com>
(cherry picked from commit 4eec891cd6cd25a420f3fe10b32cd9d561058572)
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoAdd splice support
Josef Bacik [Tue, 5 Jul 2016 18:53:20 +0000 (14:53 -0400)]
Add splice support

Using splice can net a pretty big performance improvement.  Add a per export
option to enable splice.  Using splice will create a pipe for every write to
buffer our write buffer into, and will create a temporary pipe for reads to read
our data into and splice into our socket.

Splice has a few limitations.  We are limited to the in kernel pipe size for our
requests, so if any request exceeds those limits we'll fall back to normal
read/writes.  Copyonwrite would add to the complexity by having multiple pipes
to deal with any diff files, so to avoid that we simply do not allow copyonwrite
coupled with splice.

Signed-off-by: Josef Bacik <jbacik@fb.com>
(cherry picked from commit 3135b8677efa31069e846d659398abf2e8827c67)
[wouter: modified to apply after STARTTLS patch, and to not trigger when TLS is active]
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoProvide a success message for the handshake test
Wouter Verhelst [Sun, 30 Oct 2016 23:38:10 +0000 (00:38 +0100)]
Provide a success message for the handshake test

All other tests produce a message upon successful completion of the
test, so make the handshake test follow suit.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoMove declaration to the right place
Wouter Verhelst [Sun, 30 Oct 2016 23:37:06 +0000 (00:37 +0100)]
Move declaration to the right place

A forward declaration that isn't actually a forward one is less than
helpful.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agotests: Cover recent bug fixes
Eric Blake [Thu, 20 Oct 2016 00:48:15 +0000 (19:48 -0500)]
tests: Cover recent bug fixes

Add a new test 'handshake' that intentionally provokes a server error
during option negotiation, before falling back to NBD_OPT_ABORT to
end negotiation, to prove that the server is correctly allowing a
client to fall back to known options; thus covering two recent bug
fixes for a server sending the wrong length in an error reply, and
for a server not reading enough data when replying to an unknown
command.

Signed-off-by: Eric Blake <eblake@redhat.com>
[reflowed in light of b8852465a]
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoProperly handle maxconnections
Wouter Verhelst [Sun, 30 Oct 2016 23:10:03 +0000 (00:10 +0100)]
Properly handle maxconnections

The "maxconnections" setting considers connections to all exports,
rather than "just" the one where the "maxconnections" setting is set.
This is unexpected and not useful.

Fix by creating a socketpair over which the child asks the parent
process whether a connection to the given export is allowed, rather than
doing the check in the client process.

Closes github #35

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoAdd missing #endif
Wouter Verhelst [Fri, 21 Oct 2016 11:51:55 +0000 (13:51 +0200)]
Add missing #endif

Commit e8c8cdc removed the non-GnuTLS alternate implementation of
handle_starttls, but accidentally removed the #endif at the end, causing
failure to compile.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoEXTRA_DIST support/genver.sh
Wouter Verhelst [Fri, 21 Oct 2016 11:50:17 +0000 (13:50 +0200)]
EXTRA_DIST support/genver.sh

Now that we pass '-I support' through ACLOCAL_AMFLAGS, the "support"
directory needs to actually exist for things to continue working.
Additionally, support/genver.sh needs to exist in non-git directories
for autoconf to do its thing.

Ship genver.sh to kill both flies with one stone.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoDon't forget the NBD_REP_ACK
Wouter Verhelst [Fri, 21 Oct 2016 08:20:05 +0000 (10:20 +0200)]
Don't forget the NBD_REP_ACK

We're not allowed to start TLS negotiation until *after* we've sent an
ACK to the client. Do so.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoSend a proper error if GnuTLS was not compiled in
Wouter Verhelst [Fri, 21 Oct 2016 08:18:05 +0000 (10:18 +0200)]
Send a proper error if GnuTLS was not compiled in

Currently we just hung up, which is not cool (and not what the spec wants).
Instead, we should send NBD_REP_ERR_PLATFORM.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoDon't forget the break
Wouter Verhelst [Fri, 21 Oct 2016 08:16:44 +0000 (10:16 +0200)]
Don't forget the break

After successfully negotiating TLS, we should read the next option, not
send NBD_REP_ERR_UNSUP (in the encrypted channel!) to the client...

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoserver: Read client's TLS length data before next option
Eric Blake [Mon, 17 Oct 2016 20:23:40 +0000 (15:23 -0500)]
server: Read client's TLS length data before next option

Any client attempting to probe support for a new option, such as
NBD_OPT_STARTTLS or NBD_OPT_GO, with plans to do a graceful
fallback to older methods, will fail in its attempt if the server
does not consume the length field and potential payload of the
unrecognized (or rejected) option, because the server will then
be reading out of sync and not see the client's magic for the
next option.  While it is true that sane clients are unlikely to
send more than one NBD_OPT_STARTTLS, and thus never trigger some
of the paths in this patch, it is still better to be robust for
all clients.

Furthermore, even if the server requires TLS, and rejects all but
NBD_OPT_STARTTLS as the first valid option, it should still honor
NBD_OPT_ABORT.

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoserver: Read client's unknown option length before next option
Eric Blake [Mon, 17 Oct 2016 20:23:38 +0000 (15:23 -0500)]
server: Read client's unknown option length before next option

Any client attempting to probe support for a new option, such as
NBD_OPT_STARTTLS or NBD_OPT_GO, with plans to do a graceful
fallback to older methods, will fail in its attempt if the server
does not consume the length field and potential payload of the
unrecognized (or rejected) option, because the server will then
be reading out of sync and not see the client's magic for the
next option.  This bug has been latent in the reference
server since commit 626c2a3 in 2012, even though it is EXACTLY
the bug that NBD_FLAG_FIXED_NEWSTYLE was designed to prevent.

The only reason it has been buggy for so long is that it has
taken us this long to finally want to implement clients that use
a new option.

This patch fixes only the portion of the server that has been
previously released, to make backports easier. The new code for
handling TLS also needs fixing, in the next patch.

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoserver: Swap argument order in consume()
Eric Blake [Mon, 17 Oct 2016 20:23:37 +0000 (15:23 -0500)]
server: Swap argument order in consume()

The signature of consume() threw me off.  Good design says that if
you are going to have paired parameters (buf and bufsize), you
generally want them adjacent, not separated by an unrelated parameter
(len).  Move len to be first, adjusting all callers.

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoserver: Fix botched strlen computation of error message
Eric Blake [Mon, 17 Oct 2016 20:23:36 +0000 (15:23 -0500)]
server: Fix botched strlen computation of error message

Commit 3b80382 tried to make it easy for the server to send an
error message whose length was determined by strlen(), but ended
up sending a length of UINT32_MAX, causing clients to either
hang up (reply too large) or wait for nearly 4G of data that was
never coming.

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoCorrect comment
Wouter Verhelst [Fri, 21 Oct 2016 07:55:23 +0000 (09:55 +0200)]
Correct comment

NBD_OPT_EXPORT_NAME cannot be issued before NBD_OPT_STARTTLS, period,
since it needs to be the final option. However, future versions of
nbd-server will support NBD_OPT_SELECT/NBD_OPT_GO, in which case this
*will* be necessary.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoConditionally compile in GnuTLS
Wouter Verhelst [Fri, 21 Oct 2016 07:54:01 +0000 (09:54 +0200)]
Conditionally compile in GnuTLS

... so that we can still compile on platforms that don't have
GnuTLS >= 3.3

TODO: consider whether allowing an older version of GnuTLS is sensible.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoSet function pointers
Wouter Verhelst [Fri, 21 Oct 2016 07:51:35 +0000 (09:51 +0200)]
Set function pointers

The client->socket_read and client->socket_write function pointers need
to be changed after we negotiated TLS. Forgetting this means we
negotiate TLS successfully, and then try to continue to communicate in
cleartext...

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoFix memory leak
Wouter Verhelst [Fri, 21 Oct 2016 07:50:58 +0000 (09:50 +0200)]
Fix memory leak

If we don't need the session, we need to deallocate after deiniting it.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agobuild: Ignore copied file during in-tree build
Eric Blake [Thu, 20 Oct 2016 19:05:52 +0000 (14:05 -0500)]
build: Ignore copied file during in-tree build

Commit b885246 creates a symlink to work around an automake weakness,
but forgot to ignore the link when doing an in-tree build.

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agobuild: Silence autogen.sh warnings
Eric Blake [Wed, 19 Oct 2016 19:37:15 +0000 (14:37 -0500)]
build: Silence autogen.sh warnings

Starting from a fresh git checkout, running ./autogen.sh gives a
couple of warnings on my Fedora 24 build tools, one from libtool:

libtoolize: Consider adding '-I support' to ACLOCAL_AMFLAGS in Makefile.am.

and one from automake:

tests/run/Makefile.am:4: warning: source file '$(top_srcdir)/cliserv.c' is in a subdirectory,
tests/run/Makefile.am:4: but option 'subdir-objects' is disabled
automake: warning: possible forward-incompatibility.
automake: At least a source file is in a subdirectory, but the 'subdir-objects'
automake: automake option hasn't been enabled.  For now, the corresponding output
automake: object file(s) will be placed in the top-level directory.  However,
automake: this behaviour will change in future Automake versions: they will
automake: unconditionally cause object files to be placed in the same subdirectory
automake: of the corresponding sources.
automake: You are advised to start using 'subdir-objects' option throughout your
automake: project, to avoid future incompatibilities.

Following the advice almost works, except that automake 1.15 still
has a nasty bug (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=13928)
where use of $(foo) in a _SOURCES variable coupled with subdir-objects
creates a directory with a literal name $(foo) rather than the intended
name.  And while open-coding it (using ../../ instead of $(top_srcdir)/)
works around the problem of bad naming in automake 1.15, it fails for
automake 1.11 (hello CentOS 6), due to the order in which .deps files
are erased vs. included in makefiles.  The solution that works across
all automake versions is to only stick files in _SOURCES that do not
live outside of the subtree.  Note that we only need to copy cliserv.c
into the tests/run directory; automakes handling of dependencies will
still rebuild against .h file changes even without listing the .h files
or copying them locally.

I also noticed that the build was already leaving behind an untracked
manpage.log file, in addition to the new .dirstamp witness file created
by our new use of subdir-objects.

This patch has been tested with 'make distcheck' across multiple
automake and libtool versions, ranging from CentOS 6 vintage to current
git toolchains.

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agobuild: Distribute netdb-compat.h without relying on tests
Eric Blake [Wed, 19 Oct 2016 19:37:14 +0000 (14:37 -0500)]
build: Distribute netdb-compat.h without relying on tests

nbd-server depends on netdb-compat.h; however, we were only
including it in the tarball as a side effect of it also being
used by the testsuite.  Make the dependency explicit.

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agomaint: Let emacs know our preferred style
Eric Blake [Thu, 20 Oct 2016 14:22:06 +0000 (09:22 -0500)]
maint: Let emacs know our preferred style

Teach emacs that this entire source tree uses Linux style
indentation (8-space indents, but with hard TAB character
instead of spaces).  Without this hint, the default emacs
C style tries to use 2-space indentation, making it a pain
to edit correctly when automatic formatting is enabled.

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoImprove string
Wouter Verhelst [Tue, 11 Oct 2016 18:47:29 +0000 (20:47 +0200)]
Improve string

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoRename the goto label to clarify that we're dealing with a hard close here
Wouter Verhelst [Tue, 11 Oct 2016 18:47:04 +0000 (20:47 +0200)]
Rename the goto label to clarify that we're dealing with a hard close here

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoDon't send an option reply to OPT_EXPORT_NAME
Wouter Verhelst [Tue, 11 Oct 2016 18:46:47 +0000 (20:46 +0200)]
Don't send an option reply to OPT_EXPORT_NAME

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoSilence compiler warning
Wouter Verhelst [Tue, 11 Oct 2016 18:39:54 +0000 (20:39 +0200)]
Silence compiler warning

size.c: In function ‘main’:
size.c:12:2: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result]
  write(fd, filename, 1);

We don't actually care (if write fails, the size detection will also
fail, so the test will fail), but it doesn't hurt to be somewhat more
explicit.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoWhitespace fix
Wouter Verhelst [Tue, 11 Oct 2016 00:10:56 +0000 (02:10 +0200)]
Whitespace fix

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoDon't change socket options after negotiation
Wouter Verhelst [Tue, 11 Oct 2016 00:08:46 +0000 (02:08 +0200)]
Don't change socket options after negotiation

Now that we may negotiate TLS in the negotiation function, manually
touching the socket after setting up the GnuTLS context is no longer a
good idea.

Move the functions that change socket options so they are called before
negotiate().

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoSend error messages to the client
Wouter Verhelst [Tue, 11 Oct 2016 00:07:55 +0000 (02:07 +0200)]
Send error messages to the client

When an error occurs during negotiation, make the server send a
human-readable error message to the client along with the error reply
message.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoAllow for easy sending of error messages
Wouter Verhelst [Tue, 11 Oct 2016 00:06:53 +0000 (02:06 +0200)]
Allow for easy sending of error messages

While the protocol spec allows it, currently we don't actually send any
data to the client when we send an error message.

Change the send_reply() function to make that easier.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoImplement STARTTLS server side
Wouter Verhelst [Mon, 10 Oct 2016 23:16:46 +0000 (01:16 +0200)]
Implement STARTTLS server side

This adds support for STARTTLS to nbd-server. Still TODO are:
- Testing
- client-side
- Testing
- Verification that everything works the way it should, also against other
  implementations (e.g., qemu)
- Testing
- Add unit tests for STARTTLS implementation, too.
- Testing

Did I mention this hasn't been tested well, yet?

At least it compiles.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoResync comment with reality
Wouter Verhelst [Mon, 10 Oct 2016 22:14:56 +0000 (00:14 +0200)]
Resync comment with reality

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoSpeed up the critical path
Wouter Verhelst [Mon, 10 Oct 2016 22:10:20 +0000 (00:10 +0200)]
Speed up the critical path

These socket_read() and socket_write() functions will now be called
everywhere in the critical path (especially socket_write(), which will
be called with a mutex held). Therefore, their performance is critical.

Since a conditional jumps wreak havoc on cache prediction, use function
pointers to avoid them, thereby not negatively impacting performance
too much.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoRefactor negotiation a bit.
Wouter Verhelst [Mon, 10 Oct 2016 22:04:17 +0000 (00:04 +0200)]
Refactor negotiation a bit.

Currently, we did negotiation by way of direct read() and write() calls.
This has two problems:
- The error handling in the implementation is not perfect; it is
  possible to have short reads, which would result in the server
  assuming a buffer has a given size, when in reality it does not.
- Having direct read() and write() calls makes it impossible to abstract
  away whether we're using TLS or not.

The easy fix is to use the new socket_read() and socket_write() calls,
but those require a CLIENT* to be available before they can be used.
Rather than allocating it in handle_export_name(), we now allocate it in
negotiate(), passing the pointer to the export name handler, and
removing its allocation.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoRemove bashism
Wouter Verhelst [Mon, 10 Oct 2016 20:54:24 +0000 (22:54 +0200)]
Remove bashism

Running an EXIT trap on abnormal termination of the shell is a bashism;
dash, for instance, does not run them on SIGINT.

Fix by explicitly adding a trap for SIGINT.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoRe-enable integrityhuge test
Wouter Verhelst [Fri, 7 Oct 2016 15:16:07 +0000 (17:16 +0200)]
Re-enable integrityhuge test

This used to cause deadlocks due to the fact that both server and tester
client did single-threaded and blocking I/O. However, this has now been
fixed, so we really should re-enable the check.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoPrepare for allowing STARTTLS
Wouter Verhelst [Fri, 7 Oct 2016 15:10:35 +0000 (17:10 +0200)]
Prepare for allowing STARTTLS

Replace every use of readit() or writeit() on a socket by an abstracting
socket_read()/socket_write() function instead.

This socket_read()/socket_write() function will call
gnutls_record_recv()/gnutls_record_send() if the client is doing TLS,
which we signal by the non-NULL-ness of the new .tls_session member of
the CLIENT struct.

After that, once we add STARTTLS negotiation, we should be able to do
TLS properly.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoDon't claim we're exiting when we're not.
Wouter Verhelst [Fri, 7 Oct 2016 15:08:22 +0000 (17:08 +0200)]
Don't claim we're exiting when we're not.

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoSearch for and compile in GnuTLS
Wouter Verhelst [Fri, 7 Oct 2016 15:07:46 +0000 (17:07 +0200)]
Search for and compile in GnuTLS

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoglibc wants _DEFAULT_SOURCE rather than _BSD_SOURCE nowadays
Wouter Verhelst [Fri, 7 Oct 2016 10:48:12 +0000 (12:48 +0200)]
glibc wants _DEFAULT_SOURCE rather than _BSD_SOURCE nowadays

Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoWork with old autotools
Josef Bacik [Wed, 6 Jul 2016 16:30:15 +0000 (12:30 -0400)]
Work with old autotools

m4_esyscmd_s doesn't exist with old autotools, nor does serial_tests.  Fix up
these to use the old variants and not include serial_tests if our autotools
don't have support for it.  Also add gthread to the glib check since older glibs
don't get the proper includes/libs by default without it.

Signed-off-by: Josef Bacik <jbacik@fb.com>
(cherry picked from commit dc6b6e208f597e7e287a3181f1fcbd923d2bf223)
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoFix compilation errors on old glib versions
Josef Bacik [Wed, 6 Jul 2016 16:25:08 +0000 (12:25 -0400)]
Fix compilation errors on old glib versions

On centos6 the older version of glib needs to have g_thread_init() run before we
create the threadpool, however this is a deprecated function and we don't need
it on newer glibs.  So add a configure check to see if our glib is the old one
and add the appropriate includes and g_thread_init() call.  Tested on centos6
and Fedora 23 to make sure it did the right thing in both cases.

Signed-off-by: Josef Bacik <jbacik@fb.com>
(cherry picked from commit 92fc4f11aa6c27aeb7ba28a510b550b550d13b60)
Signed-off-by: Wouter Verhelst <w@uter.be>
8 years agoClarify that clients cannot expect ordering in the face of multiple connections.
Wouter Verhelst [Thu, 6 Oct 2016 12:46:16 +0000 (14:46 +0200)]
Clarify that clients cannot expect ordering in the face of multiple connections.

8 years agoClarify that it's not really *all* options
Wouter Verhelst [Sat, 1 Oct 2016 11:34:51 +0000 (13:34 +0200)]
Clarify that it's not really *all* options

Closes github issue#40

8 years agoEnsure length of flush command is set to zero
Alex Bligh [Mon, 26 Sep 2016 18:36:10 +0000 (19:36 +0100)]
Ensure length of flush command is set to zero

Signed-off-by: Alex Bligh <alex@alex.org.uk>
8 years agonbd-tester-client: fix flags usage.
Carl-Daniel Hailfinger [Mon, 26 Sep 2016 01:22:52 +0000 (03:22 +0200)]
nbd-tester-client: fix flags usage.

Running nbd-tester-client against nbdkit with oldstyle negotiation was fun.
I managed to segfault nbdkit and noticed that nbd-tester-client speaks
the oldstyle protocol incorrectly, ignoring flags sent by the server.

Fix.

8 years agoDocs: remove reference to 'write barrier' in NBD_CMD_FLUSH
Alex Bligh [Thu, 15 Sep 2016 12:25:47 +0000 (13:25 +0100)]
Docs: remove reference to 'write barrier' in NBD_CMD_FLUSH

Reference to a 'write barrier' in NBD_CMD_FLUSH is confusing as it might
appear to be a reference to an old-style linux block layer write barrier
which actually waits for all writes to complete, rather than just requiring
writes that have completed (and for which replies have been sent) have
been persisted.

Signed-off-by: Alex Bligh <alex@alex.org.uk>
8 years agoAdd CII Best Practices badge. No, we're not ready yet.
Wouter Verhelst [Thu, 11 Aug 2016 17:25:09 +0000 (19:25 +0200)]
Add CII Best Practices badge. No, we're not ready yet.

8 years agoDon't forget to ship the nbd@.service.tmpl file
Wouter Verhelst [Wed, 13 Jul 2016 15:17:30 +0000 (17:17 +0200)]
Don't forget to ship the nbd@.service.tmpl file

8 years agoMove statements to their correct section
Wouter Verhelst [Wed, 13 Jul 2016 08:42:57 +0000 (10:42 +0200)]
Move statements to their correct section

Before belongs in the [Unit] section, not the [Service] one. Also,
remove duplicate RequiredBy statement in Service section.

8 years agoAdd missing closing tag for para element nbd-3.14
Wouter Verhelst [Thu, 30 Jun 2016 18:06:24 +0000 (20:06 +0200)]
Add missing closing tag for para element

8 years agoForce generation of the nbd@.service file
Wouter Verhelst [Thu, 23 Jun 2016 14:37:31 +0000 (16:37 +0200)]
Force generation of the nbd@.service file

8 years agoDon't do the symlink things
Wouter Verhelst [Thu, 23 Jun 2016 14:27:17 +0000 (16:27 +0200)]
Don't do the symlink things

This confuses automake a bit too much. Instead, use a relative reference

8 years agoAdd systemd directory to autogen, too
Wouter Verhelst [Thu, 23 Jun 2016 14:23:36 +0000 (16:23 +0200)]
Add systemd directory to autogen, too