Re: [Nbd] [PATCHv2 6/6] Add TLS testing to nbd-tester-client.c

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

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