Param locate helper functions. by paulidale · Pull Request #8519 · openssl/openssl

paulidale · 2019-03-19T01:08:37Z

Add a number of functions that combine a get/set with a locate.

documentation is added or updated
tests are added or updated

doc/man3/OSSL_PARAM_TYPE.pod

levitte · 2019-03-19T06:17:11Z

As long as the diverse _get and _set functions in this set think a NULL param is an error, I'm opposed to further "simplification" that will solidify a coding pattern that's already bad. See the change in #8523, that should hopefully explain matters a bit.

paulidale · 2019-03-19T07:39:42Z

Agree that we must avoid having bad patterns in the code (or adding any more...)

I had an idea: these new functions return 1 on success, 0 on failure and -1 for not found. This allows mandatory params via: if (OSSL_PARAM_locate_set_XXX(...) <= 0) error; and non-mandatory via: if (OSSL_PARAM_locate_set_XXX(...)) error;. It isn't the cleanest but it would be workable.

I think we're going to see a lot of both types, so making a short coding pattern for each seems useful.

The other possibilities I can think of are an extra argument or another set of calls to distinguish between the two. Of these, I prefer the latter.

paulidale · 2019-03-19T07:44:26Z

I think it is only the set functions that need to care about about mandatory or not. The get functions will always have to check if the value was retrieved or not.

levitte · 2019-03-19T08:04:38Z

I think it is only the set functions that need to care about about mandatory or not. The get functions will always have to check if the value was retrieved or not.

What set functions and what get functions? If it's this API you're talking about, i.e. the likes of OSSL_PARAM_set_utf8_string, it should be the other way around.

levitte · 2019-03-19T08:07:13Z

Also, I think that it's not up to this API to decide what's mandatory and what's not. For example, for an RSA key that can have numbers assigned (which would be typical for our default provider, yeah?), it would be perfectly plausible to only set the 'n' and 'e' numbers, without making it mandatory to specify the rest of the possible numbers.

crypto/params.c

paulidale · 2019-03-19T08:18:36Z

What set functions and what get functions? If it's this API you're talking about, i.e. the likes of OSSL_PARAM_set_utf8_string, it should be the other way around.

The example that triggered this in #8513 is setting things that were not in the params array. The setters are required. Does your comment mean the getters aren't? I can't think of a use case where param is got but the caller doesn't need to check if it existed.

I've added a number of OSSL_PARAM_optional_set_XXX() calls to distinguish the two usages.

levitte · 2019-03-19T08:34:51Z

What set functions and what get functions? If it's this API you're talking about, i.e. the likes of OSSL_PARAM_set_utf8_string, it should be the other way around.

The example that triggered this in #8513 is setting things that were not in the params array. The setters are required. Does your comment mean the getters aren't? I can't think of a use case where param is got but the caller doesn't need to check if it existed.

And here I see where I think you get confused. The function in question is deflt_get_params, and the typical would be that any get_params functions use the _set_ from the OSSL_PARAM API and vice versa (simple logic; one ends wants to get the params of an object means that the other ends get to set the chunks of memory pointed at by the passed OSSL_PARAM array).

I've added a number of OSSL_PARAM_optional_set_XXX() calls to distinguish the two usages.

You will need a set of OSSL_PARAM_optional_get_XXX() functions as well.

paulidale · 2019-03-19T09:43:51Z

I see the source of the confusion. Thanks for the clarification.

doc/man3/OSSL_PARAM_TYPE.pod

levitte · 2019-03-19T10:10:19Z

I insist, there should be optional_get functions as well

levitte · 2019-03-19T10:39:26Z

doc/man3/OSSL_PARAM_TYPE.pod

I disagree with the error, but good catch on the NULL check. The code should be:

if ((p = OSSL_PARAM_locate(params, "foo")) != NULL) OSSL_PARAM_set_utf8_ptr(p, "foo value"); if ((p = OSSL_PARAM_locate(params, "bar")) != NULL) OSSL_PARAM_set_utf8_ptr(p, "bar value"); if ((p = OSSL_PARAM_locate(params, "cookie")) != NULL) OSSL_PARAM_set_utf8_ptr(p, "cookie value");

The rationale is still the same, if the other end is only interested in the cookie value, why should it be obligated to also get the foo and bar values?

Perhaps, the RSA example would work better. n and e being mandatory, the rest being optional?

If the caller is only interested in getting the values of p and q (if those are accessible)?

levitte · 2019-03-19T10:41:10Z

doc/man3/OSSL_PARAM_TYPE.pod

OSSL_PARAM_optional_set_utf8_ptr(params, "foo", "foo value"); OSSL_PARAM_optional_set_utf8_string(params, "bar", "bar value"); OSSL_PARAM_optional_set_utf8_string(params, "cookie", "cookie value");

If you really want to make an valuable example of mandatory parameters, it would be to use a mixture of locate_get and optional_get functions. Personally, I rather imagine that some parameters might have to be grouped together. For example, that if "foo" is set by the caller, so must "bar".

paulidale · 2019-03-19T11:06:29Z

I've added the optional get functions, although I still don't see why.
I've reverted the example.

These combine an OSSL_PARAM_locate call with the corresponding get/set call.

mattcaswell · 2019-03-19T14:39:53Z

crypto/params.c

+    return OSSL_PARAM_set_octet_ptr(OSSL_PARAM_locate(p, key), val, used_len);
+}
+
+int OSSL_PARAM_optional_get_int(const OSSL_PARAM *p, const char *key, int *val)


The optional "get" forms don't seem very useful. With a "success" return you can't tell whether key and val have been updated or not. Applications would be better off just using OSSL_PARAM_locate_get_int which returns fail if it wasn't found.

Let's say we have a provider that offers RSA, and with which applications (or libcrypto) can set the RSA object parameters n, e and d.

The application (or libcrypto for that matter) wants to create a public key, so tries this:

OSSL_PARAM params[3]; size_t n = 0; unsigned char e_buf[16]; unsigned char n_buf[512]; BN_bn2nativepad(rsa_e, e_buf, sizeof(e_buf)); BN_bn2nativepad(rsa_n, n_buf, sizeof(n_buf)); params[n++] = OSSL_PARAM_construct_BN("e", rsa_e, sizeof(rsa_e), NULL); params[n++] = OSSL_PARAM_construct_BN("n", rsa_n, sizeof(rsa_n), NULL); params[n].key = NULL; if (!EVP_PKEY_set_params(rsa_pkey, params)) fprintf(stderr, "ERROR\n");

Now, on the provider end, set_params is defined like this:

static int rsa_set_params(void *vrsa, const OSSL_PARAM *params) { PROV_RSA *rsa = vrsa; return OSSL_PARAM_locate_get_BN(params, "n", rsa->n) && OSSL_PARAM_locate_get_BN(params, "e", rsa->e) && OSSL_PARAM_locate_get_BN(params, "d", rsa->d); }

With this construction, creating a public key with parameters fails.

Before saying that this won't happen, consider test/testrsapub.pem. How do you imagine that this file will become a default provider PKEY?

Right, so "d" truly is optional in this case. So write it like this:

static int rsa_set_params(void *vrsa, const OSSL_PARAM *params) { PROV_RSA *rsa = vrsa; OSSL_PARAM_locate_get_BN(params, "d", rsa->d); return OSSL_PARAM_locate_get_BN(params, "n", rsa->n) && OSSL_PARAM_locate_get_BN(params, "e", rsa->e); }

And then, another dude just wants to sign with a private key and passes d and n...

And oh, what if the passed d param was invalid?

Maybe it could be left as an exercise to those who write the receiving end to figure out what values have to be grouped together and how to code that properly?

I'm getting worried that this API gets over engineered and tries to solve everything instead of being a set of tools. That comes with a cost.

I'd prefer to lose the optional get calls in favour of the receiver figuring it out.

The way I see it now, that speaks for this PR as a whole.

Okay, I'll close unless somebody does think this is beneficial.

I tend to agree.

richsalz · 2019-03-19T14:59:51Z

Adding several dozen functions to avoid one line of code seems like the wrong trade-off to me.
OpenSSL already gets dinged for the size of its API, and adding all these things that will have to live forever seems like a really bad mistake.

paulidale · 2019-03-20T00:28:15Z

It's one line of code that will be repeated a large number of times.

richsalz · 2019-03-20T01:03:46Z

it's one line of code that will be repeated a large number of times.

So, let's write the code without these dozens of functions and then refactor out the common case.

It will be a lot more clear to the programmer what is going on, rather than recalling the semantics of dozens of new functions.

levitte · 2019-03-20T06:52:05Z

doc/man3/OSSL_PARAM_TYPE.pod

+
+    params[n++] = OSSL_PARAM_construct_BN("n", n_buf, sizeof(n_buf), NULL);
+    params[n++] = OSSL_PARAM_construct_uint32("e", rsa_e, NULL);
+    params[n] = OSSL_PARAM_END;


Heh, have you tested that this line goes through a compiler?

I wondered if anyone would notice :)

paulidale mentioned this pull request Mar 19, 2019

Add a default provider and implement SHA256 in it #8513

Closed

paulidale force-pushed the param-locate branch from d1c55bd to 332b50e Compare March 19, 2019 01:33

slontis reviewed Mar 19, 2019

View reviewed changes

doc/man3/OSSL_PARAM_TYPE.pod Outdated Show resolved Hide resolved

levitte reviewed Mar 19, 2019

View reviewed changes

crypto/params.c Outdated Show resolved Hide resolved

levitte reviewed Mar 19, 2019

View reviewed changes

doc/man3/OSSL_PARAM_TYPE.pod Outdated Show resolved Hide resolved

levitte reviewed Mar 19, 2019

View reviewed changes

doc/man3/OSSL_PARAM_TYPE.pod Outdated Show resolved Hide resolved

paulidale force-pushed the param-locate branch from 5e3b6c5 to 8424488 Compare March 19, 2019 10:19

levitte requested changes Mar 19, 2019

View reviewed changes

paulidale added 4 commits March 19, 2019 21:09

Add OSSL_PARAM_locate_ functions.

d2b4d30

These combine an OSSL_PARAM_locate call with the corresponding get/set call.

Add setter functions that don't fail if the parameter isn't found.

54758bd

add optional getters

ddfb541

generate file

bc970d3

paulidale force-pushed the param-locate branch from 6e4ce8d to bc970d3 Compare March 19, 2019 11:10

mattcaswell reviewed Mar 19, 2019

View reviewed changes

additional example of optional get

26fb042

levitte reviewed Mar 20, 2019

View reviewed changes

paulidale closed this Mar 20, 2019

paulidale deleted the param-locate branch September 19, 2019 02:11

Uh oh!

Conversation

paulidale commented Mar 19, 2019

Uh oh!

Uh oh!

levitte commented Mar 19, 2019

Uh oh!

paulidale commented Mar 19, 2019

Uh oh!

paulidale commented Mar 19, 2019

Uh oh!

levitte commented Mar 19, 2019

Uh oh!

levitte commented Mar 19, 2019

Uh oh!

Uh oh!

paulidale commented Mar 19, 2019

Uh oh!

levitte commented Mar 19, 2019

Uh oh!

paulidale commented Mar 19, 2019

Uh oh!

Uh oh!

Uh oh!

levitte commented Mar 19, 2019

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 19, 2019 • 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!

paulidale commented Mar 19, 2019

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 20, 2019 • 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!

richsalz commented Mar 19, 2019

Uh oh!

paulidale commented Mar 20, 2019

Uh oh!

richsalz commented Mar 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

levitte Mar 19, 2019 •

edited

Loading

levitte Mar 20, 2019 •

edited

Loading