Hi Peter,
thanks for your feedback. Attached you will find a refined and rebased
patch that includes your feedback/comments. My answers can be found
inline, too.
Am 13.11.2013 03:06, schrieb Peter Stuge:
> 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.
Thanks, I adopted my patch to the new style.
>
>> I'll also send you some more comments on this patch, there are a
>> few small things to take care of still.
> See below.
Thanks again.
>
>> 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.
For now I didn't touch my previous changes to pem.c. If you have any
ideas or made a strategic decision yet, we can of course refactor again.
>
>> 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.
If I would do that, the line wouldn't fit onto one terminal row. I think
it looks cleaner with those intentional newlines after each option.
>
>> @@ -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.
For now I have removed this feature/functionality from my patch since it
is not required in order to have a WinCNG backend.
> 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.)
Once WinCNG has been accepted and merged, I could refactor this part in
order to include all backends and be more generic, yes.
>
>> @@ -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.
As you suggested I have now removed the conditional check for math.h
since it is currently required in order to build WinCNG.
>
>> +++ 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?
I will leave this decision up to you. As long as they are available for
WinCNG to use, everything should be fine.
>
>> 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!
I have tested the WinCNG backend against OpenSSHd and Bitvise WinSSHd
servers using the previously mentioned supported algorithms and features.
The most intensive testing was done using the libssh2 examples against a
WinSSHd instance on my local machine. I verified that the following
features work:
- Cryptophic functionality:
- RSA authentication
- DSA authentication
- MD5 hash algorithm with HMAC support
- SHA1 hash algorithm with HMAC support
- AES CBC 128, 192 and 256 bit block cipher
- RC4 128 bit stream cipher
- 3DES 192 bit block cipher
- Generic functionality:
- Loading private key from file (currently plain-text only)
- Extracting public key from private key file (currently plain-text only)
I am open for and appreciate any further feedback and comments.
Best regards,
Marc
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel