[PATCH] DPP: Allow loading bootstrap keys using an OpenSSL engine

[Jouni Malinen]

On Tue, Apr 06, 2021 at 04:03:48PM +0000, Andrew Beltrano wrote:
> Add ability to load a DPP bootstrap key-pair using an arbitrary OpenSSL
> engine instead of requiring the private key to be specified explicitly.
> The engine name, so path, and key identifier must be specified to
> enable loading a key using an OpenSSL engine. The key identifier is
> an engine-specific field used to identify the key to load.
> 
> Explicit private keys, if specified, take precedence over OpenSSL
> engine-based keys.

> diff --git a/src/common/dpp.c b/src/common/dpp.c
> @@ -180,6 +180,13 @@ void dpp_bootstrap_info_free(struct dpp_bootstrap_info *info)
> +#ifndef OPENSSL_NO_ENGINE
> +	os_free(info->key_id);
> +	os_free(info->engine_id);
> +	os_free(info->engine_path);
> +	if (info->engine)
> +		ENGINE_finish(info->engine);
> +#endif /* OPENSSL_NO_ENGINE */
>  	EVP_PKEY_free(info->pubkey);
>  	str_clear_free(info->configurator_params);
>  	os_free(info);

I'd prefer to move most of the OpenSSL specific items away from dpp.c
(as was actually done with another patch for the items shown here in the
earlier code snapshot). Maybe that info->engine could be a new context
struct from src/crypto/crypto.h with the key_id/engine_id/engine_path
being "hidden" within that struct just like the OpenSSL specific
reference within crypto_openssl.c.

> @@ -3893,6 +3900,11 @@ int dpp_bootstrap_gen(struct dpp_global *dpp, const char *cmd)
>  	info = get_param(cmd, " info=");
>  	curve = get_param(cmd, " curve=");
>  	key = get_param(cmd, " key=");
> +#ifndef OPENSSL_NO_ENGINE
> +	bi->key_id = get_param(cmd, " key_id=");
> +	bi->engine_id = get_param(cmd, " engine=");
> +	bi->engine_path = get_param(cmd, " engine_path=");
> +#endif /* OPENSSL_NO_ENGINE */

This could be done without that OPENSSL_NO_ENGINE.

> +#ifndef OPENSSL_NO_ENGINE
> +	else if (bi->key_id) {
> +		bi->engine = dpp_load_engine(bi->engine_id, bi->engine_path);
> +		if (!bi->engine)
> +			goto fail;
> +	}
> +#endif /* OPENSSL_NO_ENGINE */

And I guess this could also get rid of the OPENSSL_NO_ENGINE use and
replace dpp_load_engine() with something like crypto_load_engine(). It
might also be more convenient to provide key_id to that function so that
all such crypto implementation specific parameters would be within the
same context and dpp_load_keypair() would not need to pass the key_id
separately.

> diff --git a/src/common/dpp.h b/src/common/dpp.h
> +#ifndef OPENSSL_NO_ENGINE
> +#include <openssl/engine.h>
> +#endif /* OPENSSL_NO_ENGINE */ 

> @@ -166,6 +169,12 @@ struct dpp_bootstrap_info {
> +#ifndef OPENSSL_NO_ENGINE
> +	char *key_id;
> +	char *engine_id;
> +	char *engine_path;
> +	ENGINE *engine;
> +#endif /* OPENSSL_NO_ENGINE */
>  };

So that could be replaced with struct crypto_engine (etc.) so that all
the OpenSSL specific stuff could be in openssl_engine.c (or
crypto_openssl.c if the Makefile conditions do not work easily for
adding that file based on OPENSSL_NO_ENGINE).

> diff --git a/src/common/dpp_crypto.c b/src/common/dpp_crypto.c
> +#ifndef OPENSSL_NO_ENGINE
> +static EVP_PKEY * dpp_load_keypair(const struct dpp_curve_params **curve,
> +				  ENGINE *engine, const char *key_id)
> +{

EVP_PKEY/ENGINE could be replace with struct crypto_ec_key/crypto_engine
now.

> +	EVP_PKEY *pkey;
> +	EC_KEY *eckey;
> +	const EC_GROUP *group;
> +	int nid;
> +
> +	pkey = ENGINE_load_private_key(engine, key_id, NULL, NULL);

And most of the fucntion contents moved into the OpenSSL specific file.
Or well, I guess the full function could go there since there would not
really be much need for an extra wrapper on top of that.

> +static int dpp_openssl_engine_load_dynamic(const char *engine_id,
> +			const char *engine_path)
> +{
> +	const char *pre_cmd[] = {
> +		"SO_PATH", engine_path,
> +		"ID", engine_id,
> +		"LIST_ADD", "1",
> +		"LOAD", NULL,
> +		NULL, NULL
> +	};
> +	const char *post_cmd[] = {
> +		NULL, NULL
> +	};
> +
> +	if (!engine_id || !engine_path)
> +		return 0;
> +
> +	wpa_printf(MSG_DEBUG, "ENGINE: Loading %s Engine from %s",
> +		   engine_id, engine_path);
> +
> +	return openssl_engine_load_dynamic_generic(pre_cmd, post_cmd, engine_id);
> +}

I'm not sure what would be the cleanest approach for this function.
Maybe this should all go into the OpenSSL specific files even though
this does not strictly speaking call any OpenSSL library specific
function, but that last function there is still a crypto library
specific one.

> +ENGINE * dpp_load_engine(const char *engine_id, const char *engine_path)
> +{
> +	if (dpp_openssl_engine_load_dynamic(engine_id, engine_path) < 0)
> +		return NULL;
> +
> +	ENGINE *engine = ENGINE_by_id(engine_id);
> +	if (!engine) {
> +		wpa_printf(MSG_ERROR, "ENGINE: engine %s not available [%s]",
> +			engine_id, ERR_error_string(ERR_get_error(), NULL));
> +		return NULL;
> +	}
> +
> +	if (ENGINE_init(engine) != 1) {
> +		wpa_printf(MSG_ERROR, "ENGINE: engine init failed "
> +			"(engine: %s) [%s]", engine_id,
> +			ERR_error_string(ERR_get_error(), NULL));
> +		ENGINE_free(engine);
> +		return NULL;
> +	}
> +
> +	return engine;
> +}

And this would move into openssl_engine/crypto_openssl.c as well.

-- 
Jouni Malinen                                            PGP id EFC895FA