Hello,
and sorry for the delay (things like 'real life', you know).
On Sat, Jan 20, 2018 at 3:22 PM, Selva Nair <[email protected]> wrote:
> Hi,
>
> On Sat, Jan 20, 2018 at 6:30 AM, Steffan Karger <[email protected]> wrote:
> > Hi,
> >
> > On 17-01-18 14:10, Emmanuel Deloget wrote:
> >> Calling EVP_KEY_id() before EVP_PKEY_get0_*() is unnecessary as
> >> the same check is also performed in the later.
> >>
> >> We also make the code a bit better by not calling the various
> >> EVP_PKEY_get0_*() functions twice (this needs a bit or reordering to
> >> avoid introducing yet another #ifndef OPENSSL_NO_EC in the code).
> >>
> >> Signed-off-by: Emmanuel Deloget <[email protected]>
> >> ---
>
> ..
>
> > Unfortunately, OpenSSL 1.1.0 treats calling EVP_PKEY_get0_foo on a
> > non-foo key as an error. Running 'make check' with this patch and
> > openssl 1.1.0 fails:
> >
> > Sat Jan 20 12:23:41 2018 Control Channel: TLSv1.2, cipher TLSv1.2
> > ECDHE-RSA-AES256-GCM-SHA384, 2048 bit RSA
> > Sat Jan 20 12:23:41 2018 OpenSSL: error:06078081:digital envelope
> > routines:EVP_PKEY_get0_DSA:expecting a dsa key
> > Sat Jan 20 12:23:41 2018 OpenSSL: error:0608308E:digital envelope
> > routines:EVP_PKEY_get0_EC_KEY:expecting a ec key
> > Sat Jan 20 12:23:41 2018 TLS_ERROR: BIO read tls_read_plaintext error
> >
> > So, NAK. (And Selva gets to keep EPV_PKEY_id() ;-) )
> >
> > Sorry for not spotting this earlier, could have saved us quite a bit of
> > work...
>
I should have spotted that before I sent the change, but I only tested it
with 1.0.2 and compile-tested it against 1.1. I'm going to refactor my test
environment in order to avoid any further error like this one.
>
> I'm surprised that my argument about
>
> if (EVP_PKEY_id(foo) == ...EC..) { do EC stuff }
> else if (EVP_PKEY_id(foo) == ..RSA..) { do RSA stuff }
>
> or switch(EVP_PKEY_id(foo))
>
> being stylistically better[tm] did not work!
>
> Still pleased by the end result. Now I get to spit out some of the
> "if (EVP_PKEY_get0_RSA(..))" that was pushed down my throat :).
> For patches already on the ML will do so after review.
>
>
I like the switch - I find it elegant - but it does not avoid an internal
'if', so the code would look like:
switch (EVP_PKEY_id(foo)) {
case ..RSA..: {
RSA *rsa = EVP_PKEY_get0_RSA(foo);
if (rsa) {
openvpn_printf(...);
}
}
break;
case ...
}
If this is acceptable, I can respin patch 2. Otherwise, feel free to
discard both patch 2 and patch 3 of the series as any change in patch 2
would not be a largen enhancement of the already existing code.
> Selva
>
>
Best regards,
-- Emmanuel Deloget
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
