Hi,
Looks good except for some documentation errors and some nits
On Thu, Nov 11, 2021 at 8:01 AM Arne Schwabe <a...@rfc2549.org> wrote:
> This allows OpenVPN to load non-default providers. This is mainly
> useful for loading the legacy provider with --provider legacy default
>
> Patch v4: use spaces to seperate providers, unload providers.
>
> Signed-off-by: Arne Schwabe <a...@rfc2549.org>
> ---
> doc/man-sections/generic-options.rst | 10 ++++++++++
> src/openvpn/crypto_backend.h | 14 +++++++++++++
> src/openvpn/crypto_mbedtls.c | 13 ++++++++++++
> src/openvpn/crypto_mbedtls.h | 3 +++
> src/openvpn/crypto_openssl.c | 30 ++++++++++++++++++++++++++++
> src/openvpn/crypto_openssl.h | 9 +++++++++
> src/openvpn/openvpn.c | 13 ++++++++++++
> src/openvpn/options.c | 7 +++++++
> src/openvpn/options.h | 9 +++++++++
> 9 files changed, 108 insertions(+)
>
> diff --git a/doc/man-sections/generic-options.rst
> b/doc/man-sections/generic-options.rst
> index e6c1fe455..f5b8a9135 100644
> --- a/doc/man-sections/generic-options.rst
> +++ b/doc/man-sections/generic-options.rst
> @@ -280,6 +280,16 @@ which mode OpenVPN is configured as.
> This option solves the problem by persisting keys across :code:`SIGUSR1`
> resets, so they don't need to be re-read.
>
> +--provider providers
>
For the option name please use "--providers" in plural. Here and in
options.c. For the missing one-line description in usage_message,
"--providers provider-list" would be apt.
+ Load the : separated list of (OpenSSL) providers. This is mainly useful
> for
> + using an external provider for key management like tpm2-openssl or to
> load
> + the legacy provider with
> +
> + ::
> +
> + --provider "legacy:default"
>
This is now changed to space-separated, right? So:
--providers legacy default
It may be worth pointing out that it's best to not change this
option between SIGHUPs.
Unlike most other options, this would behave somewhat unintuitively on
SIGHUP. If started with "--providers foo" and then if this option is
removed and a SIGHUP done, no providers will be loaded and connection will
fail. On the other hand a fresh start with no "--providers" option, the
fallback provider (default) will get auto-loaded.
> +
> +
> --remap-usr1 signal
> Control whether internally or externally generated :code:`SIGUSR1`
> signals
> are remapped to :code:`SIGHUP` (restart without persisting state) or
> diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
> index 5aab3e1b7..40984c559 100644
> --- a/src/openvpn/crypto_backend.h
> +++ b/src/openvpn/crypto_backend.h
> @@ -78,6 +78,20 @@ void crypto_clear_error(void);
> */
> void crypto_init_lib_engine(const char *engine_name);
>
> +
> +/**
> + * Load the given (OpenSSL) providers
> + * @param provider name of providers to load
> + * @return reference to the loaded provider
> + */
> +provider_t *crypto_load_provider(const char *provider);
> +
> +/**
> + * Unloads the given (OpneSSL) provider
To nit-pick: @param provname
> + * @param provider pointer to the provider to unload
> + */
> +void crypto_unload_provider(const char* provname, provider_t *provider);
> +
> #ifdef DMALLOC
> /*
> * OpenSSL memory debugging. If dmalloc debugging is enabled, tell
> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
> index 08b9e004f..39dbf38a5 100644
> --- a/src/openvpn/crypto_mbedtls.c
> +++ b/src/openvpn/crypto_mbedtls.c
> @@ -69,6 +69,19 @@ crypto_init_lib_engine(const char *engine_name)
> "available");
> }
>
> +provider_t *crypto_load_provider(const char *provider)
> +{
> + if (provider)
> + {
> + msg(M_WARN, "Note: mbed TLS provider functionality is not
> available");
> + }
> + return NULL;
> +}
> +
> +void crypto_unload_provider(const char* provname, provider_t *provider)
> +{
Personally I do not care, but in case we have a policy on no-unused vars,
something like "(void) provname;" etc would be needed.
> +}
> +
> /*
> *
> * Functions related to the core crypto library
> diff --git a/src/openvpn/crypto_mbedtls.h b/src/openvpn/crypto_mbedtls.h
> index 019de01d1..758ab1b40 100644
> --- a/src/openvpn/crypto_mbedtls.h
> +++ b/src/openvpn/crypto_mbedtls.h
> @@ -48,6 +48,9 @@ typedef mbedtls_md_context_t md_ctx_t;
> /** Generic HMAC %context. */
> typedef mbedtls_md_context_t hmac_ctx_t;
>
> +/* Use a dummy type for the provider */
> +typedef void provider_t;
> +
> /** Maximum length of an IV */
> #define OPENVPN_MAX_IV_LENGTH MBEDTLS_MAX_IV_LENGTH
>
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index cc1d62210..ab38d6e5c 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -54,6 +54,9 @@
> #if (OPENSSL_VERSION_NUMBER >= 0x10100000L) &&
> !defined(LIBRESSL_VERSION_NUMBER)
> #include <openssl/kdf.h>
> #endif
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L
> +#include <openssl/provider.h>
> +#endif
>
> #if defined(_WIN32) && defined(OPENSSL_NO_EC)
> #error Windows build with OPENSSL_NO_EC: disabling EC key is not
> supported.
> @@ -149,6 +152,33 @@ crypto_init_lib_engine(const char *engine_name)
> #endif
> }
>
> +provider_t *
> +crypto_load_provider(const char *provider)
> +{
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L
> + /* Load providers into the default (NULL) library context */
> + OSSL_PROVIDER* prov = OSSL_PROVIDER_load(NULL, provider);
> + if (!prov)
> + {
> + crypto_msg(M_FATAL, "failed to load provider '%s'", provider);
> + }
> + return prov;
> +#else /* OPENSSL_VERSION_NUMBER >= 0x30000000L */
> + msg(M_WARN, "Note: OpenSSL hardware crypto engine functionality is
> not available");
>
"hardware crypto engine" --> "provider"
> + return NULL;
> +#endif
> +}
> +
> +void crypto_unload_provider(const char* provname, provider_t *provider)
> +{
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L
> + if (!OSSL_PROVIDER_unload(provider))
> + {
> + crypto_msg(M_FATAL, "failed to undload provider '%s'", provname);
>
"undload" --> "unload"
> + }
> +#endif
> +}
> +
> /*
> *
> * Functions related to the core crypto library
> diff --git a/src/openvpn/crypto_openssl.h b/src/openvpn/crypto_openssl.h
> index e540a76b9..446f08508 100644
> --- a/src/openvpn/crypto_openssl.h
> +++ b/src/openvpn/crypto_openssl.h
> @@ -33,6 +33,10 @@
> #include <openssl/hmac.h>
> #include <openssl/md5.h>
> #include <openssl/sha.h>
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L
> +#include <openssl/provider.h>
> +#endif
> +
>
> /** Generic cipher key type %context. */
> typedef EVP_CIPHER cipher_kt_t;
> @@ -49,12 +53,17 @@ typedef EVP_MD_CTX md_ctx_t;
> /** Generic HMAC %context. */
> #if OPENSSL_VERSION_NUMBER < 0x30000000L
> typedef HMAC_CTX hmac_ctx_t;
> +
> +/* Use a dummy type for the provider */
> +typedef void provider_t;
> #else
> typedef struct {
> OSSL_PARAM params[3];
> uint8_t key[EVP_MAX_KEY_LENGTH];
> EVP_MAC_CTX *ctx;
> } hmac_ctx_t;
> +
> +typedef OSSL_PROVIDER provider_t;
> #endif
>
> /** Maximum length of an IV */
> diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c
> index da06f59c2..095d448b0 100644
> --- a/src/openvpn/openvpn.c
> +++ b/src/openvpn/openvpn.c
> @@ -112,10 +112,23 @@ void init_early(struct context *c)
> /* init verbosity and mute levels */
> init_verb_mute(c, IVM_LEVEL_1);
>
> + /* Initialise OpenVPN provider, this needs to be initialised this
>
"OpenVPN provider" --> "OpenSSL provider"
> + * early since option post-processing and also openssl info
> + * printing depends on it */
> + for (int j=1; j < MAX_PARMS && c->options.providers.names[j]; j++)
> + {
> + c->options.providers.providers[j] =
> + crypto_load_provider(c->options.providers.names[j]);
> + }
> }
>
> static void uninit_early(struct context *c)
> {
> + for (int j=1; j < MAX_PARMS && c->options.providers.providers[j]; j++)
> + {
> + crypto_unload_provider(c->options.providers.names[j],
> + c->options.providers.providers[j]);
> + }
> net_ctx_free(&c->net_ctx);
> }
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index b5d65d293..87062d58d 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -8157,6 +8157,13 @@ add_option(struct options *options,
> options->engine = "auto";
> }
> }
> + else if (streq(p[0], "provider") && p[1])
>
"providers" please
> + {
> + for (size_t j = 1; j < MAX_PARMS && p[j] != NULL;j++)
> + {
> + options->providers.names[j] = p[j];
> + }
> + }
> #endif /* ENABLE_CRYPTO_MBEDTLS */
> #ifdef ENABLE_PREDICTION_RESISTANCE
> else if (streq(p[0], "use-prediction-resistance") && !p[1])
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index 20b34ed4e..d4f41cd71 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -179,6 +179,14 @@ struct remote_list
> struct remote_entry *array[CONNECTION_LIST_SIZE];
> };
>
> +struct provider_list
> +{
> + /* Names of the providers */
> + const char *names[MAX_PARMS];
> + /* Pointers to the loaded providers to unload them */
> + provider_t *providers[MAX_PARMS];
> +};
> +
> enum vlan_acceptable_frames
> {
> VLAN_ONLY_TAGGED,
> @@ -519,6 +527,7 @@ struct options
> const char *ncp_ciphers;
> const char *authname;
> const char *engine;
> + struct provider_list providers;
> bool replay;
> bool mute_replay_warnings;
> int replay_window;
>
The new option is not added to usage_message for --help and usage().
Selva
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
