Hi,
On Mon, Jan 15, 2018 at 7:56 PM, Emmanuel Deloget <log...@free.fr> wrote:
> Hello Selva,
>
> On Tue, Jan 16, 2018 at 12:10 AM, Selva Nair <selva.n...@gmail.com> wrote:
>
>>
>>
>> Can I try one last time to convince you guys to keep EVP_PKEY_id()? If I
>> was targeting only 1.1.0+ that's how most of those checks would have been
>> done, isn't? Makes life easier, code simpler as I have said before..
>>
>> Just throwing in my 0.02c, feel free to ignore.
>>
>> Best,
>>
>> Selva
>>
>
>
> OpenSSL 1.1.0+ and previous versions allows you to create a typed EVP_PKEY
> without any key inside. For example, you can do
>
> EVP_PKEY *pkey = EVP_PKEY_new();
> EVP_PKEY_set_type(pkey, EVP_PKEY_RSA);
>
> This is plainly valid: the key is prepared for later use.
>
> So testing for EVP_PKEY_id() is not sufficient to make sure that current
> and future code won't mess with us.
>
That was not my intent.
Use EVP_KEY_id() to determine the type and choose the code path. Then when
ready, get the key pointer and check it. Of course there may be situations
where its possible to get the pointer and check it in one line as in your
case (3) below.
Anyway, calling EVP_PKEY_get0_RSA() multiple times is lame. Now think of
cases where we may have to delegate the job to separate functions depending
on the key type. To me it looks natural to do that using a
switch(EVP_PKEY_id()) or if-else blocks to decide which function to call
and then get the key and check it inside the function. Anyway, I will leave
it at that.
> To be extra sure, one can do
>
> if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey)) { ... }
>
> Which is exactly what the current code does. Yet the idea (at least mine)
> is to remove that to simplify code a bit.
>
> Now, I agree, this is quite easy to read and to understand.
>
> We have 3 solutions:
>
> 1) keep the code as is
>
> if (EVP_PKEY_id() == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL) {
> ... }
>
In this case the call to EVP_PKEY_id() is redundant, isn't it? Or am I
missing something? If the latter returns non-null then its indeed an RSA
key (I'm assuming the 1.1.0 or the corrected compat layer behaviour here).
>
>
> 2) remove EVP_PKEY_id() but keep a test on EVP_PKEY_getO_RSA()
>
> if (EVP_PKEY_get0_RSA(pkey) != NULL) {
> RSA *rsa = EVP_PKEY_get0_RSA(pkey);
> ...
> }
>
This looks bad to me.
>
>
> 3) rework it as proposed in the patch
>
> RSA *rsa = NULL;
> if ((rsa = EVP_PKEY_get0_RSA(pkey)) != NULL) { ... }
>
In this particular case this may be the best approach but there are
situations where it may not be the natural choice. Hence the "if it were up
to me EVP_KEY_id() will stay" musing...:)
Thanks,
Selva
------------------------------------------------------------------------------
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
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
