Hi Kamil, Thanks for the reply. I’m not really proposing anything at the moment, I’m looking for suggestions on how to resolve the issue of replicating 3 structures just so we don’t have to change kmdhgGPsha1kex_state_t. Right now, kmdhgGPsha1kex_state_t has two members that need to change; h_sig_comp and exchange_hash.
First, h_sig_comp needs to be big enough to hold a SHA256 digest, that’s an easy fix; I’d propose something like: #define MAX_SHA_DIGEST_LEN SHA256_DIGEST_LENGTH And then change the member variable to: unsigned char h_sig_comp[MAX_SHA_DIGEST_LEN] Second, exhange_hash needs to change from libssh2_sha1_ctx to something more generic. This is the real hang-up as it needs to be either libssh2_sha1_ctx or libssh2_sha256_ctx. This is what I’m soliciting ideas on how to resolve this issue. Any recommendations are welcome. Cheers, Will > On Jan 13, 2015, at 4:17 AM, Kamil Dudka <kdu...@redhat.com> wrote: > > On Monday 12 January 2015 16:29:12 Will Cosgrove wrote: >> Hi All, >> I’m adding diffie-hellman-group-exchange-sha256 support and have it working. >> However, if I am to submit this patch back to the project I have a couple >> code style questions. >> >> First, kmdhgGPsha1kex_state_t is coded to be specific to sha1. No big deal >> I thought, I could add a sha256 version. However that leads to >> key_exchange_state_low_t which is included in key_exchange_state_t. So now >> we’re duplicating three structs and causing a lot of branching, not so >> great. >> >> At that point, I decided to change kmdhgGPsha1kex_state_t to support sha256. >> The following changes were made: >> >> unsigned char h_sig_comp[SHA256_DIGEST_LENGTH]; //SHA1_DIGEST_LENGTH >> >> //libssh2_sha1_ctx exchange_hash; >> EVP_MD_CTX exchange_hash; >> >> This isn’t so hot as it hard-codes openssl support instead of using the >> libssh2_sha1_ctx macro. On the flip side, creating three new structures >> for a couple calls seems excessive. >> >> Anyone out there have opinions on how to proceed? > > For me it is difficult to infer what exactly you are proposing to change > from the above description. Could you please attach the current version > of your patch to clarify it? > > In general, avoiding duplicated code is good. On the other hand, I believe > that the only module where OpenSSL should be hard-coded is src/openssl.c > whereas the structures defined in libssh2_priv.h should be independent on > a particular crypto library. > > Kamil > >> Cheers, >> Will >> _______________________________________________ >> libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel _______________________________________________ libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
