Add EVP_PKEY API to retrieve raw OSSL_PARAM values from a provided key by slontis · Pull Request #11340 · openssl/openssl

slontis · 2020-03-17T04:34:50Z

Checklist

documentation is added or updated
tests are added or updated

paulidale

I've some doubts about the whole approach to exporting...

paulidale · 2020-03-17T04:49:32Z

include/openssl/evp.h

 # define EVP_PKEY_OP_DECRYPT             (1<<11)
 # define EVP_PKEY_OP_DERIVE              (1<<12)

+


I added a op and then deleted it - but not the newline

paulidale · 2020-03-17T04:51:44Z

test/evp_pkey_provided_test.c

+        goto err;
+
+    /*
+     * TODO(3.0): How do we compare bignums here?


Convert to BN and use the test macro?

The data coming back is in reverse order - how does converting to BN help with this?

BN_native2bn does the right thing.

And create a BIGNUM with BN_bin2bn for ec_priv_keydata, 'cause I suppose that one is in BE order, right?

paulidale · 2020-03-17T04:55:52Z

crypto/evp/keymgmt_lib.c

+    OSSL_PARAM *p = (OSSL_PARAM *)params;
+
+    for (i = 0; p->key != NULL && i < used_len; p++, i++) {
+        if (!used[i] && strcmp(key, p->key) == 0) {


RSA is a good example of something with multiple fields that are the same, which is particularly ugly (i.e- how do we know how many it has before we do the call).

(i.e- how do we know how many it has before we do the call).

find out by getting the value of the "primes" (OSSL_PKEY_PARAM_RSA_PRIMES)

No!

When we get a new algorithm that requires n happy numbers to work, asking for primes isn't useful.

But supposedly, the application knows what to ask for.

"Know your provider" -- me

paulidale · 2020-03-17T04:56:48Z

crypto/evp/keymgmt_lib.c

+
+#define OSSL_PARAM_TODATA_MAX 25
+
+static int todata(const OSSL_PARAM src[], void *cbarg)


I can't say I like this. Passing the caller's OSSL_PARAM array to the export and filling the fields they want seems more natural than copying everything here. Still, if the fields aren't known.....

Could an OSSL_PARAM array and its size be passed to the export and it gets filled but not overflowed without this callback being involved?

The export is there to do export 'everything'.. I have just hijacked it..
The callback is normally used for the import process (using the params that are built)

Maybe a "query all possible params to export", a "clone a param list" and an "export" would simplify things.

The normal export is also a import

The normal export is also a import

... depending on whose point of view. What libcrypto exports, the provider imports, and vice versa

I still believe that a query of all parameters makes sense. I'd work with a query of all parameters and their values, plus a separate free function.

levitte · 2020-03-17T06:49:26Z

Since these are values that the caller knows (or should know) the name of, I don't see why OP_keymgmt_get_params() isn't used for this? And yeah, it means that we need to add cases for the get_params functions.

In a way, it means that OP_keymgmt_export() and OP_keymgmt_get_params() do more or less the same thing, but with a twist; OP_keymgmt_export() is meant for those who do not know and do not need to know the exact contents of the key beforehand, all the caller wants is a bunch of data, while OP_keymgmt_get_params() is meant for those who do know, and that may want to extract very specific data.

I've probably made the mistake of saying (even documenting) that op_keymgmt_get_params() should only be concerned with key meta-data -- I lost track of the need for what this PR is trying to achieve. That was a mistake, there's really nothing that says that an asymmetric key implementation couldn't let OP_keymgmt_get_params() return any data they want, including the key data.

levitte · 2020-03-17T06:55:07Z

test/evp_pkey_provided_test.c

+
+    /*
+     * TODO(3.0): How do we compare bignums here?
+     * The value returned in priv is in BE order.


Er, what? The value of priv should come in whatever byte order a native integer uses.

Could we have a seperate type for OSSL_PARAM_TYPE_BIGNUM that doesnt need to do native order.... Then we dont need to do this reversing bytes stuff that is confusing,
(I keep on saying its confusing because IT IS CONFUSING :)).
i.e- fromdata takes a non native unsigned char[], todata currently returns a unsigned char[] with reversed byte order (THIS IS NOT GOOD).

I don't think that OSSL_PARAM_TYPE_BIGNUM is good at all. However, since the data type is supposed to describe memory organization, I'd suggest something like OSSL_PARAM_TYPE_INTEGER_LE, OSSL_PARAM_TYPE_UNSIGNED_INTEGER_LE, OSSL_PARAM_TYPE_INTEGER_BE OSSL_PARAM_TYPE_UNSIGNED_INTEGER_BE, and either end that cares must deal with the difference.

The reason I don't want to treat BIGNUM as an essentially different type than any other number can be illustrated with RSA's e. That's usually a small number, and it should be perfectly legitimate for one end to treat it as an unsigned int and the other to treat it as a BIGNUM without having to jump through extra hoops.

I dont agree with this logic..
I think this is dealing with an edge case that is not needed.
If we want to put a constant into a bignum anywhere else in the code we convert it to a bignum.
For the todata operation - creating a unsigned char array shouldnt be a big deal.

It essentially boils down to this for me...
If I feed an array of bytes into a system, I expect it to be able to be extracted as an array of bytes in exactly the same order (not reversed due to some internal implementation details).

There is no way of knowing that a number is an actual bignum currently (only the end point API's convert to and from a bignum). While this isnt a problem at the moment we might find a case where it becomes a issue and then we cant fix it because we have shared the UNSIGNED_INT type. I have no problem if it asserts if a int is assigned to a bignum type.

I'd suggest something like OSSL_PARAM_TYPE_INTEGER_LE, OSSL_PARAM_TYPE_UNSIGNED_INTEGER_LE,

You are talking about an internal representation which seems irrelevant.
What about OSSL_PARAM_TYPE_BIG_UNSIGNED_INTEGER?

Its a array that gets reversed when it comes back out.

The point here is that it seems you'd rather have OCTET_STRING to transfer these bits. And that's fair, I don't always see a point of insisting that the key data be seen as numbers, even though they are internally treated as such. The EC case is a bit absurd in that regard, with one half of the key being transferred as an octet string and the other as a number..

exactly - but then it still needs the type so it knows its a bignum right?

[sigh]
@slontis, you want something that's not represented in native byte order, but rather specifically in big endian order. I proposed data types for that. How about we go from there?

It being used as a BIGNUM on either end is internal information.

slontis · 2020-03-17T07:30:06Z

And yeah, it means that we need to add cases for the get_params functions.

I guess I was trying to prevent having to duplicate nearly identical code in multiple places.
(It may lead to some out of synch bugs as we move forward over time).

levitte · 2020-03-17T13:32:08Z

I guess I was trying to prevent having to duplicate nearly identical code in multiple places.

I haven't looked too closely, but I wouldn't be surprised if one can be made in terms of the other.
The other option could be to expose the export function to the public, including the callback. I'm not too keen on that, but

richsalz · 2020-03-17T13:54:21Z

Does requiring something like PEM serialization, which has a standard format for all keys, lessen the need for this?

levitte · 2020-03-17T14:00:21Z

Does requiring something like PEM serialization, which has a standard format for all keys, lessen the need for this?

DER serialization should suffice... but no, not in the general case. We want something that helps applications to extract data from a key, such as n from an RSA key, or xyzzy from a BLARGH key (the latter being fictional, of course, but serves the purpose of reminding us all that we need to be able to deal with what's unknown to us)

richsalz · 2020-03-17T14:05:00Z

So perhaps for 3.0 we only support DER serialization, and once we get more data-points we try to solve the general case.

levitte · 2020-03-17T14:27:12Z

That's easy, just tell the application authors serialize the key to DER using OSSL_SERIALIZER, then de serialize the result with the appropriate d2i function to convert it to whatever the type may be (DH is the popular example du jour). That can be done, here and now, yeah.

richsalz · 2020-03-17T14:31:27Z

So you're saying that the problem we know about is solved? And we can put this aside for 3.0?

levitte · 2020-03-17T14:44:26Z

For once, yeah it's possible to do, limited to the key types we know internally.

slontis · 2020-03-17T22:10:40Z

So perhaps for 3.0 we only support DER serialization, and once we get more data-points we try to solve the general case.

Wouldnt recommend it.
The PEM format allows extra data to be added onto the end, which may be real useful (for key validation for example).

levitte · 2020-03-18T03:23:39Z

@slontis,what you quote says DER, why did you go back to PEM?

slontis · 2020-03-19T23:52:59Z

Replaced by #11365

Add EVP_PKEY API to retrieve raw OSSL_PARAM values from a provided key

952d830

slontis added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Mar 17, 2020

paulidale reviewed Mar 17, 2020

View reviewed changes

levitte reviewed Mar 17, 2020

View reviewed changes

slontis mentioned this pull request Mar 18, 2020

Add PKEY_get_params support for accessing RSA key fields #11352

Closed

2 tasks

levitte mentioned this pull request Mar 18, 2020

EVP & TLS: Add necessary EC_KEY data extraction functions, and use them #11358

Closed

slontis closed this Mar 19, 2020

		# define EVP_PKEY_OP_DECRYPT (1<<11)
		# define EVP_PKEY_OP_DERIVE (1<<12)


		#define OSSL_PARAM_TODATA_MAX 25

		static int todata(const OSSL_PARAM src[], void *cbarg)

Uh oh!

Comments

Conversation

slontis commented Mar 17, 2020

Checklist

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented Mar 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slontis commented Mar 17, 2020

Uh oh!

levitte commented Mar 17, 2020

Uh oh!

richsalz commented Mar 17, 2020

Uh oh!

levitte commented Mar 17, 2020

Uh oh!

richsalz commented Mar 17, 2020

levitte Mar 17, 2020 •

edited

Loading

levitte Mar 17, 2020 •

edited

Loading

levitte Mar 18, 2020 •

edited

Loading