Re: [Nbd] [PATCHv2 6/6] Add TLS testing to nbd-tester-client.c
- To: Alex Bligh <alex@...872...>
- Cc: "nbd-general@lists.sourceforge.net" <nbd-general@lists.sourceforge.net>
- Subject: Re: [Nbd] [PATCHv2 6/6] Add TLS testing to nbd-tester-client.c
- From: Wouter Verhelst <w@...112...>
- Date: Tue, 12 Apr 2016 16:15:26 +0200
- Message-id: <20160412141526.GE15484@...3...>
- In-reply-to: <1460394939-24868-7-git-send-email-alex@...872...>
- References: <1460394939-24868-1-git-send-email-alex@...872...> <1460394939-24868-7-git-send-email-alex@...872...>
On Mon, Apr 11, 2016 at 06:15:39PM +0100, Alex Bligh wrote:
> This commit adds TLS testing to nbd-tester-client and 'make check'.
> If TLS is not compiled in, then the test is skipped.
Alternatively, it could check that nbd-server produces an error in that
case.
> Signed-off-by: Alex Bligh <alex@...872...>
> ---
> nbd.h | 2 +
> tests/run/Makefile.am | 13 +++-
> tests/run/certs/ca-cert.pem | 20 +++++
> tests/run/certs/ca-key.pem | 32 ++++++++
> tests/run/certs/ca.info | 3 +
> tests/run/certs/client-cert.pem | 23 ++++++
> tests/run/certs/client-key.pem | 32 ++++++++
> tests/run/certs/client.info | 8 ++
> tests/run/certs/server-cert.pem | 22 ++++++
> tests/run/certs/server-key.pem | 32 ++++++++
> tests/run/certs/server.info | 5 ++
Might be good to add a little README there to explain what each of the
files does.
> tests/run/nbd-tester-client.c | 169 +++++++++++++++++++++++++++++++++++++++-
> tests/run/simple_test | 45 +++++++++++
> 13 files changed, 402 insertions(+), 4 deletions(-)
> create mode 100644 tests/run/certs/ca-cert.pem
> create mode 100644 tests/run/certs/ca-key.pem
> create mode 100644 tests/run/certs/ca.info
> create mode 100644 tests/run/certs/client-cert.pem
> create mode 100644 tests/run/certs/client-key.pem
> create mode 100644 tests/run/certs/client.info
> create mode 100644 tests/run/certs/server-cert.pem
> create mode 100644 tests/run/certs/server-key.pem
> create mode 100644 tests/run/certs/server.info
>
> diff --git a/nbd.h b/nbd.h
> index 732c605..90c97a6 100644
> --- a/nbd.h
> +++ b/nbd.h
> @@ -59,6 +59,8 @@ enum {
> #define NBD_REPLY_MAGIC 0x67446698
> /* Do *not* use magics: 0x12560953 0x96744668. */
>
> +#define NBD_OPT_REPLY_MAGIC 0x3e889045565a9LL
> +
> /*
> * This is the packet used for communication between client and
> * server. All data are in network byte order.
> diff --git a/tests/run/Makefile.am b/tests/run/Makefile.am
> index d1e28ed..050b51d 100644
> --- a/tests/run/Makefile.am
> +++ b/tests/run/Makefile.am
> @@ -1,11 +1,16 @@
> +if GNUTLS
> +TLSSRC = $(top_srcdir)/crypto-gnutls.c $(top_srcdir)/crypto-gnutls.h $(top_srcdir)/buffer.c $(top_srcdir)/buffer.h
> +else
> +TLSSRC =
> +endif
Same here as in the main Makefile.am, obviously.
[...]
> diff --git a/tests/run/nbd-tester-client.c b/tests/run/nbd-tester-client.c
> index ed4d03b..c80204d 100644
> --- a/tests/run/nbd-tester-client.c
> +++ b/tests/run/nbd-tester-client.c
> @@ -42,6 +42,10 @@
> #define MY_NAME "nbd-tester-client"
> #include "cliserv.h"
>
> +#ifdef WITH_GNUTLS
> +#include "crypto-gnutls.h"
> +#endif
> +
> static gchar errstr[1024];
> const static int errstr_len = 1023;
>
> @@ -50,6 +54,10 @@ static uint64_t size;
> static int looseordering = 0;
>
> static gchar *transactionlog = "nbd-tester-client.tr";
> +static gchar *certfile = NULL;
> +static gchar *keyfile = NULL;
> +static gchar *cacertfile = NULL;
> +static gchar *tlshostname = NULL;
>
> typedef enum {
> CONNECTION_TYPE_NONE,
> @@ -341,6 +349,24 @@ static inline int write_all(int f, void *buf, size_t len)
> return retval;
> }
>
> +/**
> + * Set a socket to blocking or non-blocking
> + *
> + * @param fd The socket's FD
> + * @param nb non-zero to set to non-blocking, else 0 to set to blocking
> + * @return 0 - OK, -1 failed
> + */
> +int set_nonblocking(int fd, int nb) {
> + int sf = fcntl (fd, F_GETFL, 0);
> + if (sf == -1)
> + return -1;
> + return fcntl (fd, F_SETFL, nb ? (sf | O_NONBLOCK) : (sf & ~O_NONBLOCK));
> +}
Maybe this should be moved to cliserv.[ch]?
[...]
> diff --git a/tests/run/simple_test b/tests/run/simple_test
> index 0c05ea1..80b99dc 100755
> --- a/tests/run/simple_test
> +++ b/tests/run/simple_test
> @@ -284,6 +284,51 @@ EOF
> ./nbd-tester-client -N export1 -u ${tmpdir}/unix.sock
> retval=$?
> ;;
> + */tls)
> + # TLS test
> + certdir=`pwd`/certs
I prefer $()-style command substitution over backticks.
[...]
> + cat >${conffile} <<EOF
> +[generic]
> + certfile = $certdir/server-cert.pem
> + keyfile = $certdir/server-key.pem
> + cacertfile = $certdir/ca-cert.pem
More whitespace mishaps (and that happens again for tlshuge, too)
> +[export1]
> + exportname = $tmpnam
> + flush = true
> + fua = true
> + rotational = true
> + filesize = 52428800
> + temporary = true
> +EOF
> + ../../nbd-server -C ${conffile} -p ${pidfile} &
> + PID=$!
> + sleep 1
> + ./nbd-tester-client -N export1 -i -t "${mydir}/integrity-test.tr" -C "${certdir}/client-cert.pem" -K "${certdir}/client-key.pem" -A "${certdir}/ca-cert.pem" -H 127.0.0.1 localhost
> + retval=$?
> + ;;
> + */tlshuge)
> + # TLS test with big operations
> + # takes a while
> + certdir=`pwd`/certs
> + cat >${conffile} <<EOF
> +[generic]
> + certfile = $certdir/server-cert.pem
> + keyfile = $certdir/server-key.pem
> + cacertfile = $certdir/ca-cert.pem
> +[export1]
> + exportname = $tmpnam
> + flush = true
> + fua = true
> + rotational = true
> + filesize = 52428800
> + temporary = true
> +EOF
> + ../../nbd-server -C ${conffile} -p ${pidfile} &
> + PID=$!
> + sleep 1
> + ./nbd-tester-client -N export1 -i -t "${mydir}/integrityhuge-test.tr" -C "${certdir}/client-cert.pem" -K "${certdir}/client-key.pem" -A "${certdir}/ca-cert.pem" -H 127.0.0.1 localhost
> + retval=$?
> + ;;
> *)
> echo "E: unknown test $1"
> exit 1
--
< 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
Reply to: