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 <kdudka_at_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
Received on 2015-01-13