I assume at some point we may want to select properties such as "asm=no" or "asm=yes" Does this fit is well with the capability tests??... or should this be handled differently. i.e 2 dispatch entries (one for the generic version + others for asm version(s))

I assume at some point we may want to select properties such as "asm=no" or "asm=yes" Does this fit is well with the capability tests??... or should this be handled differently. i.e 2 dispatch entries (one for the generic version + others for asm version(s))

I'm not sure that is actually desirable. But if we decide to do it then we can split the dispatch table entries at that point.

Now that #8700 has gone in I have rebased this and addressed all comments received so far.

Not sure why AppVeyor is claiming this is non-mergeable with the master branch because this is a fresh rebase, Travis has no such problems, and github reports no merge conflicts.

I look at this, and gotta say that I'm very very doubtful about bringing the library context stuff into the FIPS provider (or really, any provider).

I'm very very doubtful about bringing the library context stuff into the FIPS provider

Note that it's not like I understand why it gets dragged in... I think I need a little bit of time to digest this.

I look at this, and gotta say that I'm very very doubtful about bringing the library context stuff into the FIPS provider (or really, any provider).

This PR brings EVP inside the FIPS boundary. EVP uses core in order to fetch algorithms and all of that code depends on OPENSSL_CTX. So if you want EVP inside the boundary, you must have OPENSSL_CTX.

I pushed a fixup commit addressing the comment above. I also took this out of WIP.

So if you want EVP inside the boundary, you must have OPENSSL_CTX.

Unless you only pass NULL, right?

I'd go further and remove the library context stuff for this release (I forget which PR I made that comment) and only allow NULL and the global context. As I said "over there," I think the context stuff isn't fully thought out yet.

So if you want EVP inside the boundary, you must have OPENSSL_CTX.

Unless you only pass NULL, right?

You misunderstand my statement. I am saying that in order for the EVP code to be inside the FIPS boundary, there is a dependency on the OPENSSL_CTX code. This is nothing to do with passing NULL for the default context or not.

Yes, I misunderstood, thanks. I wonder if we should ask the lab for a viewpoint on this?

Hmmm... assuming that there is just the default library context and none else means that we can ignore issues with multiple initialization (i.e. the RUN_ONCE problem that we're seeing here). That would be @richsalz' point, yeah?

I'm not too keen on ignoring the library context thing, but I have to admit it's a line of thinking worth spending more than just a fleeting thought on.

I've updated this to make the changes suggested above and I've pulled in OPENSSL_atexit() and OPENSSL_cleanup() into the FIPS module. The ex_data stuff isn't really properly handled yet. That will take #8828 to go in first.

I don't understand why atext and cleanup are pulled in. If there is module-specific "cleanup on unload" code, then the core should query for a function pointer and call into the module once it is unloaded. No shared-library module is loaded before the OpenSSL core is initialized. If an application calls init without a flag that the module needs, then loading the module can signal an error; the app is broken for the module it is trying to use.

If there is module-specific "cleanup on unload" code, then the core should query for a function pointer and call into the module once it is unloaded.

This is exactly what does happen. And the module specific function that is called by the core to cleanup on unload is called....OPENSSL_cleanup(). The objective is to share as much code as possible between libcrypto and the FIPS module. We do not want to write two versions of code for everything: once for libcrypto and once for the FIPS module. That has been our approach all the way through the design process. This approach applies to initialisation and cleanup too.

I rebased this now that #8878 has gone in. There are no longer any dependencies on ASN.1 code.

NOTE: the new namemap stuff introduces an issue with aliases. The old maps had both the short name and long names registered with the NID. We would now need a duplicated name for every value in the provider tables.

The new namemap doesn't introduce said issue with aliases, the issue was always there! The only reason we didn't notice was that we piggy-backed on the object database, but as soon as a provider offered algorithm names we don't know beforehand, there are no aliases in sight.

I remember that we talked about aliases at some point, but we never concluded anything that I can recall.

Would an alias API make sense here? The numeric value could map to the first string registered and all string aliases could map to the same numeric value.

The obvious alternatives would be to:

• use NIDs directly (which is being avoided here) or
• let the provider deal with it (which isn't a good API)

On the recursive provider activation:

Finally, this simple (untested) patch should show where I see this going:

I think this will fail to activate the recursive FIPS provider on first use. The recursive FIPS provider is flagged as a fallback, and since we don't explicitly load any other providers the fallback code will kick in.

Well yeah, it simply uses itself (fips_self(libctx), which is currently unimplemented).

I am sure there are some things we could do in the core code to "short circuit" everything given that we know that there will only ever be one provider. But in doing so we add additional "ifdef" complexity to the code. It seems to me that where possible we should be avoiding "special casing" the FIPS module code. Where possible we just do it the same way in FIPS that we do it elsewhere.

Come again? I want to have one #ifdef FIPS_MODE in core_fetch.c rather than a number of them in provider_core.c (I don't want provider_core.c to be involved at all).

A line of thought behind this is that I do not think there is a need to re-run initialization for recursive uses of the FIPS module.

Initialisation is virtually a no-op in the recursive case, so I don't see this as being a big deal? All it does is pass back its dispatch table.

OSSL_provider_init_fn fips_intern_provider_init;
int fips_intern_provider_init(const OSSL_PROVIDER *provider,
                              const OSSL_DISPATCH *in,
                              const OSSL_DISPATCH **out,
                              void **provctx)
{
    *out = intern_dispatch_table;
    return 1;
}

The big deal for me is 1) the recursive activation is added complexity, especially if you want to follow what's going on in something like a debugger, and 2) that's quite a few cycles spent on a no-op.

On aliases:

The provider will have to be involved somehow... who else should decide what's an alias of what?

Come again? I want to have one #ifdef FIPS_MODE in core_fetch.c rather than a number of them in provider_core.c (I don't want provider_core.c to be involved at all).

But surely if we don't go into provider_core.c at all we never end up calling provider_activate(), which means we never setup the provider callback functions...most importantly "query_operation"?

I doubt that most providers will want to know about aliases for existing algorithms -- these should be setup by libcrypto, the default provider or the legacy one. Other providers shouldn't need to care about aliases.

For new algorithms, a provider needs to be able to register such and add aliases but this will be the exception.

I doubt that most providers will want to know about aliases for existing algorithms -- these should be setup by libcrypto, the default provider or the legacy one. Other providers shouldn't need to care about aliases.

The providers already have the knowledge of algorithm names, why should they not have the knowledge of appropriate aliases? Code wise, it's not very hard to implement:

diff --git a/include/openssl/core.h b/include/openssl/core.h
index cf4d3f41de..ea066bd4a4 100644
--- a/include/openssl/core.h
+++ b/include/openssl/core.h
@@ -61,7 +61,7 @@ struct ossl_item_st {
  * An array of these is always terminated by algorithm_name == NULL
  */
 struct ossl_algorithm_st {
-    const char *algorithm_name;      /* key */
+    const char **algorithm_names;    /* key */
     const char *property_definition; /* key */
     const OSSL_DISPATCH *implementation;
 };

Granted, that means that the provider will have to have a separate array of names, but that's not very hard either. I don't believe it's very hard to add alias handling to core_fetch.cevp_fetch.c and core_namemap.c, which would finish it all off...

It also means that every provider needs to know all of the names for each algorithm. I'd prefer the core to add the aliases it knows about and for providers to not need to know about them (although they could if they wanted to).

@paulidale, I completely disagree with you on this... but also, that's not necessarily something that needs to be resolved in this PR, so we should take that discussion elsewhere.

On the recursive provider activation:

But surely if we don't go into provider_core.c at all we never end up calling provider_activate(), which means we never setup the provider callback functions...most importantly "query_operation"?

Why do we need to call provider_activate() recursively? Surely, the OSSL_PROVIDER* that is passed to OSSL_provider_init already has all the information we need? All that's needed is to save that pointer, and since we already create a library context, that's easily solved...

Surely, the OSSL_PROVIDER* that is passed to OSSL_provider_init already has all the information we need? All that's needed is to save that pointer, and since we already create a library context, that's easily solved...

OSSL_PROVIDER is an opaque type. The OSSL_PROVIDER object that is passed to OSSL_provider_init was created libcrypto side. When we make EVP calls recursively from inside the FIPS module we cannot assume that the definition of the OSSL_PROVIDER structure that the FIPS module knows about is the same definition as the one used in libcrypto. The FIPS module maybe being run with a libcrypto from a different OpenSSL version and therefore the OSSL_PROVIDER structure definitions may be different.

When we make EVP calls recursively from inside the FIPS module we cannot assume that the definition of the OSSL_PROVIDER structure that the FIPS module knows about is the same definition as the one used in libcrypto.

Yup, I just realised that as well... so OK, I'm backing off on this one, but would like some prominent commentary on this, 'cause believe you me, the question I raised will surely be asked again unless we provide justification.

would like some prominent commentary on this

I added a new commit with some additional commentary in provider_activate to explain the recursive nature of this. I also fixed an issue where we need to set the provctx value for the recursive initialisation call to be the current library context. This will ensure that EVP calls made from within the FIPS module are always associated with the current library context.

Fixup commit pushed to addressed your latest comments. I reversed the changes to ossl_provider_activate and restricted them to just provider_activate. I also adjusted and added more commentary.

@levitte - please take another look.

Woop!!!! Pushed.....finally!!!

Thanks to everyone involved. We got there in the end. :-)