[openssl-dev] [openssl.org #3894] AutoReply: PATCH: EVP_PKEY_get_type (new function)

[noloader@gmail.com via RT]

Thanks Kurt. I think I'll need to think about this some more because I
don't recall EVP_PKEY_id.

I think I never considered it because I could not find it when
searching for something to return the inner type ('id' does not make a
lot of sense to me, even now).

Maybe I should back up a bit. What is 'id' supposed to do above and
beyond providing the 'type'?

*****

> I don't know if there are plans to run the EVP_PKEY into a opaque
> struct soon, but it probably should.

That's fine as long as we have an accessor to ensure we are working
with what we expect. Otherwise, we can't validate which means we can't
use the value.

*****

> This doesn't make sense.  You're discouraging the function you
> add?  Maybe you mean EVP_PKEY_type(pkey->type)?

Yeah, you're kind of right. On one hand, its lower level and its use
is discouraged (see the NOTES in evp,h). On the other hand, we need it
for use.

*****

According to the man pages for EVP_PKEY_type:

       EVP_PKEY_type() returns the type of key corresponding to the value
       type. The type of a key can be obtained with EVP_PKEY_type(pkey->type).

Reading the man pages, it appears there's no accessor for
`pkey->type`. Otherwise, we would have been told to use `EVP_PKEY_id`.

*****

> This seems to do almost exactly the same as EVP_PKEY_base_id().

Actually, I think its closer to EVP_PKEY_id().

Also, we have a NULL pointer dereference in the existing function:

    int EVP_PKEY_id(const EVP_PKEY *pkey)
    {
        return pkey->type;
    }

(Sorry, I did not recall seeing that function).

*****

> EVP_PKEY_type() currently documents:
>
> EVP_PKEY_type() returns the type of key corresponding to
> the value type. The type of a key can be obtained with
> EVP_PKEY_type(pkey->type). The return value will be EVP_PKEY_RSA,
> EVP_PKEY_DSA, EVP_PKEY_DH or EVP_PKEY_EC for the corresponding key
> types or NID_undef if the key type is unassigned.
>
> I'm guessing one of them is wrong.

key->type appears to be able to return both EVP_PKEY_RSA and
EVP_PKEY_RSA2. So what should be done here?

Perhaps something like:

    int EVP_PKEY_id(const EVP_PKEY *pkey)
    {
        if(!pkey) return EVP_PKEY_NONE;

        const int type= pkey->type;

        if(type == EVP_PKEY_RSA || type == EVP_PKEY_RSA2)
            return EVP_PKEY_RSA;

        ...
    }

Jeff

On Thu, Jun 4, 2015 at 3:09 PM, Kurt Roeckx via RT <rt at openssl.org> wrote:
> On Wed, Jun 03, 2015 at 08:50:25PM +0000, noloader at gmail.com via RT wrote:
>> Here's an updated patch that includes the documentation changes. `git
>> diff master` is needed after `git add` because adding doesn't seem to
>> really add things for git :)
>>
>> riemann::openssl-git$ cat evp_pkey_get_type.diff
>> diff --git a/crypto/evp/evp_lib.c b/crypto/evp/evp_lib.c
>> index 1fdde9a..0cd8a42 100644
>> --- a/crypto/evp/evp_lib.c
>> +++ b/crypto/evp/evp_lib.c
>> @@ -61,6 +61,15 @@
>>  #include <openssl/evp.h>
>>  #include <openssl/objects.h>
>>
>> +/* Returns the key type or EVP_PKEY_NONE if pkey is NULL */
>> +int EVP_PKEY_get_type(EVP_PKEY *pkey)
>> +{
>> +    if (!pkey)
>> +        return EVP_PKEY_NONE;
>> +
>> +    return EVP_PKEY_type(pkey->type);
>> +}
>> +
>
> This seems to do almost exactly the same as EVP_PKEY_base_id().
>
>>  int EVP_CIPHER_param_to_asn1(EVP_CIPHER_CTX *c, ASN1_TYPE *type)
>>  {
>>      int ret;
>> diff --git a/doc/crypto/EVP_PKEY_get_type.pod b/doc/crypto/EVP_PKEY_get_type.pod
>> new file mode 100644
>> index 0000000..1faaae5
>> --- /dev/null
>> +++ b/doc/crypto/EVP_PKEY_get_type.pod
>> @@ -0,0 +1,65 @@
>> +=pod
>> +
>> +=head1 NAME
>> +
>> +EVP_PKEY_get_type - queries the EVP_PKEY for the type of key
>> +
>> +=head1 SYNOPSIS
>> +
>> + #include <openssl/evp.h>
>> +
>> + int EVP_PKEY_get_type(EVP_PKEY *pkey);
>> +
>> +=head1 DESCRIPTION
>> +
>> +B<EVP_PKEY_get_type> queries the EVP_PKEY for the type of key.
>
> I think I prefer that to be written as EVP_PKEY_get_type() instead
> of B<EVP_PKEY_get_type>.
>
>> +Though B<EVP_PKEY_get_type> is available, its use is discouraged
>> because it peeks into implementation specific details.
>
> This doesn't make sense.  You're discouraging the function you
> add?  Maybe you mean EVP_PKEY_type(pkey->type)?
>
> I don't know if there are plans to run the EVP_PKEY into a opaque
> struct soon, but it probably should.  I would avoid talking about
> implementation details unless it's important for the user to know,
> and that doesn't seem to be the case.
>
>> +==head1 RETURN VALUES
>> +
>> +If B<pkey> is B<NULL>, then the function returns B<EVP_PKEY_NONE>.
>> +
>> +If B<pkey> is not B<NULL>, then the return value will be one of the
>> following values from B<evp.h>:
>> +
>> + * EVP_PKEY_NONE
>> + * EVP_PKEY_RSA
>> + * EVP_PKEY_RSA2
>> + * EVP_PKEY_DSA
>> + * EVP_PKEY_DSA1
>> + * EVP_PKEY_DSA2
>> + * EVP_PKEY_DSA3
>> + * EVP_PKEY_DSA4
>> + * EVP_PKEY_DH
>> + * EVP_PKEY_DHX
>> + * EVP_PKEY_EC
>> + * EVP_PKEY_HMAC
>> + * EVP_PKEY_CMAC
>
> EVP_PKEY_type() currently documents:
>
> EVP_PKEY_type() returns the type of key corresponding to
> the value type. The type of a key can be obtained with
> EVP_PKEY_type(pkey->type). The return value will be EVP_PKEY_RSA,
> EVP_PKEY_DSA, EVP_PKEY_DH or EVP_PKEY_EC for the corresponding key
> types or NID_undef if the key type is unassigned.
>
> I'm guessing one of them is wrong.