Reordering new API's that have a libctx, propq

[Tomas Mraz]

On Wed, 2020-09-09 at 11:41 +0200, Richard Levitte wrote:
> 
> Regarding the library context, when viewed as a global state, it
> makes
> sense to have it as a first argument where it's being passed, if at
> all.  The question is, where should we actually pass it?  We have a
> few different suggestions, in wording or in code, on the table:
> 
> 1.  Pass it as first argument everywhere.  This is problematic, as
>     "everywhere" is a *lot*, and if we do this, we might as well
>     re-design the whole libcrypto API [*], and that's definitely not
>     what OpenSSL 3.0 is about, quite the contrary.
> 
>     This has been partially done, with all the _with_libctx
>     functions.  As far as I've understood, these have been added on
>     need basis, i.e. somewhere down the code path, a library context
>     or a property query string is needed.  I can't say if this gives
>     any sense of completeness or consistency, viewed as an cohesive
>     API, or if we stand any chance of getting there without a
> complete
>     API re-design.
> 
>     Something to be noted is that it doesn't make sense to pass the
>     library context everywhere, because it's already part of other
>     structures that are passed.  For example, any method structure,
>     such as EVP_CIPHER, EVP_MD, etc are tied to a provider, which is
>     in turn tied to a library context.  Passing another library
>     context to anything that includes one of those method structures
>     would only be confusing.
> 
> 2.  Pass it when constructing different structures, mostly other
>     context structures.  As an example, EVP_PKEY_CTX_new_from_pkey()
>     that I displayed above.  There may be cases where we need to pass
>     it directly to functions that aren't constructors, but I expect
>     those cases to be relatively few.
> 
>     This has been done for some other structures as well, on an as
>     needed basis:
> 
>         X509 *X509_new_with_libctx(OPENSSL_CTX *libctx, const char
> *propq);
> 
>         X509_STORE_CTX *X509_STORE_CTX_new_with_libctx(OPENSSL_CTX
> *libctx,
>                                                        const char
> *propq);
> 
>     (the following are internal only)
> 
>         int x509_set0_libctx(X509 *x, OPENSSL_CTX *libctx, const char
> *propq);
>         int x509_crl_set0_libctx(X509_CRL *x, OPENSSL_CTX *libctx,
> const char *propq);
> 
> 3.  Set a current library context, a pointer in a thread local
>     variable.  We already have support for that, which this function:
> 
>         OPENSSL_CTX *OPENSSL_CTX_set0_default(OPENSSL_CTX *libctx);
> 
>     The usage model is this:
> 
>         OPENSSL_CTX *prevctx = OPENSSL_CTX_set0_default(libctx);
> 
>         /* do stuff with |libctx| as implicit current global state */
> 
>         OPENSSL_CTX_set0_default(prevctx)
> 
> Looking at that list now, I realise that it goes from most intrusive
> to least intrusive, viewed as a public API.
> 
> The third choice in particular would let any application or library
> just set their library context for the duration of the code that
> should be execute with that as a "global state", and restore it when
> done, leaving the rest of the OpenSSL calls untouched.

We could even provide a convenience thread local stack of lib contexts
so the caller would not have to keep the old value but would just push
the new libctx when entering and pop the old one when leaving. With
that, I think the changes needed in the application code would be
fairly simple and minimal.

> Something to be noted is that model 2 and 3 is possible to combine,
> which could give us a smoother transition between the current API and
> whatever we design going forward.

Although I have big sympathy with people that worked hard to add the
various _with_libctx() calls I would say that keeping the new
with_libctx variants would be too confusing. Especially for the reason
we would not have a complete set of them anyway. The exception might be
the low level EVP calls where we deal with the libctx directly and
where the algorithm fetched is actually associated with the context.

>                         --------
> 
> Regarding the property query string, looking at it separate from the
> library context, the question remains where to put it, and a few
> proposals are on the table:
> 
> 1.  Put it last.
> 
> 2.  Put it next to the algorithm name or the object that an algorithm
>     name is inferred from.
> 
> 3.  Set it "globally" with a thread local variable, a bit like what
>     OPENSSL_CTX_set0_default() does for library contexts.
> 
>     For this model, it's been argued if it should simply be stored in
>     the current library context, thereby avoiding to add another
>     thread local variable (which, for all intents and purposes, is
>     another actually global thing to deal with, even though on
>     per-thread level).
> 
> In my mind, model 2 would be more sensible than model 1, because of
> the implied tie between algorithm name and property query string.
> Also, even here, model 3 is possible to combine with model 2, and
> even
> with model 1.

I agree that the model 2 makes more sense than model 1. However it is
also indicative of one thing - that the property query really does not
make sense to be a parameter of things like X509_new(), because from
the top level point of view of a caller you do not actually know for
what purpose it will be later used. You might want a different property
query to be used for the internal SHA1 digest of the certificate and a
different property query when you want to verify a certificate
signature.

So again instead of trying to put property query as an additional
member of higher level objects I'd say that using model 3 would make
more sense to me here as well. Of course for the low level algorithm
fetches the property query as parameter should stay.

> Regarding model 3, it must be said that there is potential for
> confusion
> on what it's supposed to do, replace the default property query
> string
> (settable with EVP_set_default_properties()), or merge with it.
> Remember that a property query string given through any XXX_fetch()
> function is temporarly merged with the default properties when
> looking
> for fitting algorithms.

Here I would actually argue, that there should be another property
query in the libctx in addition to the default, that would have the
exactly same semantics as the propq parameter has now and for the
functions with the propq parameter it even would not be applied. How to
name it so it won't be confused with the default property query, that's
hard to say though.

-- 
Tomáš Mráz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
[You'll know whether the road is wrong if you carefully listen to your
conscience.]