Subject: Re: [PATCH] Added Windows Cryptography API: Next Generation backend

Re: [PATCH] Added Windows Cryptography API: Next Generation backend

From: Peter Stuge <peter_at_stuge.se>
Date: Wed, 13 Nov 2013 03:06:32 +0100

Peter Stuge wrote:
> > Please take a look and tell me if it fits your approach.
>
> It does, but current master still needs to be fixed. I'll push the
> suggested update I had for it in a bit.

I've pushed that fix now, it moves the conditional out of the shared
Makefile.inc into Makefile.am.

> I'll also send you some more comments on this patch, there are a
> few small things to take care of still.

See below.

> It also seems that we need to do a bit of preparatory work in pem.c.

This one may be more tricky, or not.

It's not reasonable to have crypto ifdefs anywhere outside crypto.c
and crypto.h. A suitable abstraction for pem.c would be good. We can
possibly take advantage of the fact that all non-autotools build
systems only support OpenSSL anyway.

Daniel Stenberg wrote:
> Marc, can you please update us the state of your work and what it
> would take or require to not wait for Peter?

Why don't you too review Marc's patch, and see for yourself?

Marc Hörsken wrote:
> From e4b794c78c4566c2f6811bcaa1dadda7b5e93a86 Mon Sep 17 00:00:00 2001
> From: Marc Hoersken <info_at_marc-hoersken.de>
> Date: Sun, 22 Sep 2013 10:04:15 +0200
> Subject: [PATCH] Added Windows Cryptography API: Next Generation based backend
..
> +++ b/configure.ac
..
> if test "$found_crypto" = "none"; then
> AC_MSG_ERROR([No crypto library found!
> -Try --with-libssl-prefix=PATH\
> - or --with-libgcrypt-prefix=PATH\
> +Try --with-libssl-prefix=PATH
> + or --with-libgcrypt-prefix=PATH
> + or --with-wincng on Windows\
> ])

Marc, please only add new lines into the above block. Remember to
check that they end with a backslash.

> @@ -167,6 +197,13 @@ if test "$GEX_NEW" != "no"; then
> AC_DEFINE(LIBSSH2_DH_GEX_NEW, 1, [Enable newer diffie-hellman-group-exchange-sha1 syntax])
> fi
>
> +AC_ARG_ENABLE(memory-overwrite,
> + AC_HELP_STRING([--disable-memory-overwrite],[Disable memory overwrite before being freed]),
> + [MEMORY_OVERWRITE=$enableval])
> +if test "$MEMORY_OVERWRITE" != "no"; then
> + AC_DEFINE(LIBSSH2_MEMORY_OVERWRITE, 1, [Enable memory overwrite before being freed])
> +fi
> +

This isn't specific for WinCNG crypto so I think it needs to be in a
separate commit, and that commit should of course add the feature to
all crypto code.

Or alternatively, make it clear that this a WinCNG-specific option.
Maybe call it --disable-wincng-memory-clear (I like 'clear' because
it's shorter than 'overwrite' but if you prefer the latter then go
for that.)

> @@ -252,7 +289,7 @@ AM_CONDITIONAL([BUILD_EXAMPLES], [test "x$build_examples" != "xno"])
> # AC_HEADER_STDC
> AC_CHECK_HEADERS([errno.h fcntl.h stdio.h stdlib.h unistd.h sys/uio.h])
> AC_CHECK_HEADERS([sys/select.h sys/socket.h sys/ioctl.h sys/time.h])
> -AC_CHECK_HEADERS([arpa/inet.h netinet/in.h])
> +AC_CHECK_HEADERS([arpa/inet.h netinet/in.h math.h])
> AC_CHECK_HEADERS([sys/un.h], [have_sys_un_h=yes], [have_sys_un_h=no])
> AM_CONDITIONAL([HAVE_SYS_UN_H], test "x$have_sys_un_h" = xyes)
..
> +#ifdef HAVE_MATH_H
> +#include <math.h>
> +#endif

Is the header required to build with WinCNG crypto? I expect WinCNG
to stay Windows-specific, so maybe windows.h is the only include file
really needed? Do you think other crypto wrappers may need math.h too?
If not, no need to check for it in configure.ac, and then it can be
included unconditionally or not included at all in the WinCNG code.

> +++ b/src/pem.c
> @@ -38,7 +38,8 @@
>
> #include "libssh2_priv.h"
>
> -#ifdef LIBSSH2_LIBGCRYPT /* compile only if we build with libgcrypt */
> +/* compile only if we build with libgcrypt or wincng */
> +#if defined(LIBSSH2_LIBGCRYPT) || defined(LIBSSH2_WINCNG)
>
> static int
> readline(char *line, int line_size, FILE * fp)
> @@ -113,6 +114,11 @@ _libssh2_pem_parse(LIBSSH2_SESSION * session,
> return ret;
> }
>
> +#endif /* LIBSSH2_LIBGCRYPT or LIBSSH2_WINCNG */
> +
> +/* compile only if we build with libgcrypt */
> +#ifdef LIBSSH2_LIBGCRYPT
> +
> static int
> read_asn1_length(const unsigned char *data,
> unsigned int datalen, unsigned int *len)

I think we need to figure something better out here. Is there a reason
not to simply always compile those first few functions in pem.c?

> diff --git a/src/wincng.c b/src/wincng.c
> new file mode 100644
..
> diff --git a/src/wincng.h b/src/wincng.h
> new file mode 100644

I haven't looked at these too closely - if they work then all is good!

//Peter
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2013-11-13