| Message ID | 20211019183127.614175-13-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OpenSSL 3.0 improvements for OpenVPN | expand |
Hi, Not a code review but a general comment as this is a new option that warrants some discussion. On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <arne@rfc2549.org> wrote: > This allows OpenVPN to load non-default providers. This is mainly > useful for loading the legacy provider with --provider legacy:default > While this format looks okay, we need to consider a few things to get this option right and possibly extensible in future without causing a regression. (i) Provider name may not be a simple word like legacy -- could also be the path to a shared object with possible embedded space in the path name. A single option with optional quotes, picked apart using ":" as separator can still work. Ideally I would have preferred space separated provider names with each quoted if needed, but the format proposed here may be easier to parse. Need clear documentation, though. (ii) We may want to attach some meaning to the order in which providers are specified. Here is why: When multiple providers implement the same method (say, KEYEXCH), there is no guarantee in OpenSSL as to which will get used. Unless a property query like "?provider=my-fast-aes" is also used. Here the prefix "?" to mean preferred but not exclusive. In cases like an pkcs11 token provider, one would need to say the opposite like "?provider=default" to ensure default is preferred and the token provider is to be used only when required -- say when the key is in a token. Same would be the case with our proposed built-in "external-key provider" which should be used only for certain operations that handle the external key. How this relates to the --provider option is like this: in some cases we want to know which provider the user prefers. Instead of hard-coding the preferred provider to "default" which the user may not even want to load. We could state that the first provider specified will become the preferred one when there are multiple choices for a method or when a preferred fallback is required. In that case the option for legacy would be --provider default:legacy And, if this option is absent, we take it to mean that the "default" provider should be loaded. This explicit loading of providers takes away some ambiguity regarding which one's are made available. However, this could conflict with what is in the OpenSSL config file. Which is not a bad thing as we probably should not use the config file to load providers (which we currently do) -- it's not supported on some platforms (Windows as of now) and can lead to broken setups due to user error. At the same time, provider paths and their names could be set up in OpenSSL config files, so loading them through config has some advantages. So, an alternate option is to not add this option at all and require the use of a config file to load additional providers like "legacy + default". Loading a custom config file is possible with OpenSSL 3.0 and could be enabled on Windows in a safe manner. Or always load providers from OPENSSL config and use the "--provider foo" on top of that. We could also consider a separate option to specify a preferred property query ("?provider=myprov") that could be added later and not rely on any ordering here. The above is just meant for further discussion -- I'm not entirely sure what the best approach is. Regards, Selva <div dir="ltr"><div>Hi,</div><div><br></div><div>Not a code review but a general comment as this is a new option that warrants some discussion.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This allows OpenVPN to load non-default providers. This is mainly<br> useful for loading the legacy provider with --provider legacy:default<br></blockquote><div><br></div><div>While this format looks okay, we need to consider a few things to get this option right and possibly extensible in future without causing a regression.</div><div><br></div><div>(i) Provider name may not be a simple word like legacy -- could also be the path to a shared object with possible embedded space in the path name. A single option with optional quotes, picked apart using ":" as separator can still work.</div><div><br></div><div>Ideally I would have preferred space separated provider names with each quoted if needed, but the format proposed here may be easier to parse. Need clear documentation, though.</div><div><br></div><div>(ii) We may want to attach some meaning to the order in which providers are specified. Here is why:</div><div><br></div><div>When multiple providers implement the same method (say, KEYEXCH), there is no guarantee in OpenSSL as to which will get used. Unless a property query like "?provider=my-fast-aes" is also used. Here the prefix "?" to mean preferred but not exclusive. In cases like an pkcs11 token provider, one would need to say the opposite like "?provider=default" to ensure default is preferred and the token provider is to be used only when required -- say when the key is in a token. Same would be the case with our proposed built-in "external-key provider" which should be used only for certain operations that handle the external key.</div><div><br></div><div>How this relates to the --provider option is like this: in some cases we want to know which provider the user prefers. Instead of hard-coding the preferred provider to "default" which the user may not even want to load. We could state that the first provider specified will become the preferred one when there are multiple choices for a method or when a preferred fallback is required. In that case the option for legacy would be</div><div><br></div><div>--provider default:legacy</div><div><br></div><div>And, if this option is absent, we take it to mean that the "default" provider should be loaded. This explicit loading of providers takes away some ambiguity regarding which one's are made available. However, this could conflict with what is in the OpenSSL config file. Which is not a bad thing as we probably should not use the config file to load providers (which we currently do) -- it's not supported on some platforms (Windows as of now) and can lead to broken setups due to user error.</div><div><br></div><div>At the same time, provider paths and their names could be set up in OpenSSL config files, so loading them through config has some advantages. So, an alternate option is to not add this option at all and require the use of a config file to load additional providers like "legacy + default". Loading a custom config file is possible with OpenSSL 3.0 and could be enabled on Windows in a safe manner. </div><div><br></div><div>Or always load providers from OPENSSL config and use the "--provider foo" on top of that.</div><div><br></div><div>We could also consider a separate option to specify a preferred property query ("?provider=myprov") that could be added later and not rely on any ordering here.</div><div><br></div><div>The above is just meant for further discussion -- I'm not entirely sure what the best approach is.</div><div><br></div><div>Regards,</div><div><br></div><div>Selva</div></div></div>
Am 20.10.21 um 20:19 schrieb Selva Nair: > Hi, > > Not a code review but a general comment as this is a new option that > warrants some discussion. > > On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <arne@rfc2549.org > <mailto:arne@rfc2549.org>> wrote: > > This allows OpenVPN to load non-default providers. This is mainly > useful for loading the legacy provider with --provider legacy:default > > > While this format looks okay, we need to consider a few things to get > this option right and possibly extensible in future without causing a > regression. > > (i) Provider name may not be a simple word like legacy -- could also be > the path to a shared object with possible embedded space in the path > name. A single option with optional quotes, picked apart using ":" as > separator can still work. > > Ideally I would have preferred space separated provider names with > each quoted if needed, but the format proposed here may be easier to > parse. Need clear documentation, though. Yeah using : was easier to parse since I need otherwise to setup an array or a list to store the parameter but it is doable. I was just hoping that nobody would use a : in the provider name. > (ii) We may want to attach some meaning to the order in which providers > are specified. That would be mostly a documentation thing to explain that the providers are loaded the in order specified with that option. Here is why: > > When multiple providers implement the same method (say, KEYEXCH), there > is no guarantee in OpenSSL as to which will get used. Unless a property > query like "?provider=my-fast-aes" is also used. Here the prefix "?" to > mean preferred but not exclusive. In cases like an pkcs11 token > provider, one would need to say the opposite like "?provider=default" to > ensure default is preferred and the token provider is to be used only > when required -- say when the key is in a token. Same would be the case > with our proposed built-in "external-key provider" which should be used > only for certain operations that handle the external key. > > How this relates to the --provider option is like this: in some cases we > want to know which provider the user prefers. Instead of hard-coding the > preferred provider to "default" which the user may not even want to > load. We could state that the first provider specified will become the > preferred one when there are multiple choices for a method or when a > preferred fallback is required. In that case the option for legacy would be > > --provider default:legacy > > And, if this option is absent, we take it to mean that the "default" > provider should be loaded. I would not explicitly load the default provider then but rather inherit what the system gives us. If the system wide config loads fips we should not load default instead. > This explicit loading of providers takes away > some ambiguity regarding which one's are made available. However, this > could conflict with what is in the OpenSSL config file. Which is not a > bad thing as we probably should not use the config file to load > providers (which we currently do) -- it's not supported on some > platforms (Windows as of now) and can lead to broken setups due to user > error. Yeah. I agree on that. I needed this for ANdroid where I also don't have a OpenSSL and adding one is not something I really want to do. > At the same time, provider paths and their names could be set up in > OpenSSL config files, so loading them through config has some > advantages. So, an alternate option is to not add this option at all and > require the use of a config file to load additional providers like > "legacy + default". Loading a custom config file is possible with > OpenSSL 3.0 and could be enabled on Windows in a safe manner. > > Or always load providers from OPENSSL config and use the "--provider > foo" on top of that. > > We could also consider a separate option to specify a preferred property > query ("?provider=myprov") that could be added later and not rely on any > ordering here. > > The above is just meant for further discussion -- I'm not entirely sure > what the best approach is. No, me neither. Arne
On Thu, Oct 21, 2021 at 8:52 AM Arne Schwabe <arne@rfc2549.org> wrote: > Am 20.10.21 um 20:19 schrieb Selva Nair: > > Hi, > > > > Not a code review but a general comment as this is a new option that > > warrants some discussion. > > > > On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <arne@rfc2549.org > > <mailto:arne@rfc2549.org>> wrote: > > > > This allows OpenVPN to load non-default providers. This is mainly > > useful for loading the legacy provider with --provider legacy:default > > > > > > While this format looks okay, we need to consider a few things to get > > this option right and possibly extensible in future without causing a > > regression. > > > > (i) Provider name may not be a simple word like legacy -- could also be > > the path to a shared object with possible embedded space in the path > > name. A single option with optional quotes, picked apart using ":" as > > separator can still work. > > > > Ideally I would have preferred space separated provider names with > > each quoted if needed, but the format proposed here may be easier to > > parse. Need clear documentation, though. > > > Yeah using : was easier to parse since I need otherwise to setup an > array or a list to store the parameter but it is doable. I was just > hoping that nobody would use a : in the provider name. > Use of '.' in the name is the recommended method to have a namespace in the provider name and reduce chances of name conflict. No one may use ':' and filenames are unlikely to contain ':' as its not legal in some platforms. That said, adding an array or list is easier than it looks. We have several options that use an array or list and the code overhead is small. > > > (ii) We may want to attach some meaning to the order in which providers > > are specified. > > That would be mostly a documentation thing to explain that the providers > are loaded the in order specified with that option. > Here is why: > > > > When multiple providers implement the same method (say, KEYEXCH), there > > is no guarantee in OpenSSL as to which will get used. Unless a property > > query like "?provider=my-fast-aes" is also used. Here the prefix "?" to > > mean preferred but not exclusive. In cases like an pkcs11 token > > provider, one would need to say the opposite like "?provider=default" to > > ensure default is preferred and the token provider is to be used only > > when required -- say when the key is in a token. Same would be the case > > with our proposed built-in "external-key provider" which should be used > > only for certain operations that handle the external key. > > > > How this relates to the --provider option is like this: in some cases we > > want to know which provider the user prefers. Instead of hard-coding the > > preferred provider to "default" which the user may not even want to > > load. We could state that the first provider specified will become the > > preferred one when there are multiple choices for a method or when a > > preferred fallback is required. In that case the option for legacy would > be > > > > --provider default:legacy > > > > And, if this option is absent, we take it to mean that the "default" > > provider should be loaded. > > I would not explicitly load the default provider then but rather inherit > what the system gives us. If the system wide config loads fips we should > not load default instead. > IMO, this idea that OpenSSL folks have that just adding a "fips=yes" plus a few lines in the config can make the application FIPS enabled is far-fetched. In reality OpenVPN will have to be recompiled with some changes to make it FIPS compliant. At that point one can also change what providers are loaded by default. I was thinking of having a load_providers() function called early -- say in options-post-process stage where we load (i) default provider (ii) providers specified in --providers option This adds much certainty to what is available. A FIPS-enabled OpenVPN can change this or we could have a --fips option that changes this. No dependency on system-wide config file and those who need legacy only need to just specify "--providers legacy" In addition, one could automatically load "legacy" if any of the options indicate its need -- like NTLM auth for proxy or BF-CBC in --data-ciphers etc. In situations where it cannot be inferred from options, user will have to use --providers legacy, though. It may be necessary to call this load_providers() also to process options like --show-ciphers and similar. Selva <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 21, 2021 at 8:52 AM Arne Schwabe <<a href="mailto:arne@rfc2549.org">arne@rfc2549.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Am 20.10.21 um 20:19 schrieb Selva Nair:<br> > Hi,<br> > <br> > Not a code review but a general comment as this is a new option that<br> > warrants some discussion.<br> > <br> > On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a><br> > <mailto:<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>>> wrote:<br> > <br> > This allows OpenVPN to load non-default providers. This is mainly<br> > useful for loading the legacy provider with --provider legacy:default<br> > <br> > <br> > While this format looks okay, we need to consider a few things to get<br> > this option right and possibly extensible in future without causing a<br> > regression.<br> > <br> > (i) Provider name may not be a simple word like legacy -- could also be<br> > the path to a shared object with possible embedded space in the path<br> > name. A single option with optional quotes, picked apart using ":" as<br> > separator can still work.<br> > <br> > Ideally I would have preferred space separated provider names with<br> > each quoted if needed, but the format proposed here may be easier to<br> > parse. Need clear documentation, though.<br> <br> <br> Yeah using : was easier to parse since I need otherwise to setup an<br> array or a list to store the parameter but it is doable. I was just<br> hoping that nobody would use a : in the provider name.<br></blockquote><div><br></div><div>Use of '.' in the name is the recommended method to have a namespace in the provider name and reduce chances of name conflict. No one may use ':' and filenames are unlikely to contain ':' as its not legal in some platforms.</div><div><br></div><div>That said, adding an array or list is easier than it looks. We have several options that use an array or list and the code overhead is small.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > (ii) We may want to attach some meaning to the order in which providers<br> > are specified. <br> <br> That would be mostly a documentation thing to explain that the providers<br> are loaded the in order specified with that option. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> Here is why:<br> > <br> > When multiple providers implement the same method (say, KEYEXCH), there<br> > is no guarantee in OpenSSL as to which will get used. Unless a property<br> > query like "?provider=my-fast-aes" is also used. Here the prefix "?" to<br> > mean preferred but not exclusive. In cases like an pkcs11 token<br> > provider, one would need to say the opposite like "?provider=default" to<br> > ensure default is preferred and the token provider is to be used only<br> > when required -- say when the key is in a token. Same would be the case<br> > with our proposed built-in "external-key provider" which should be used<br> > only for certain operations that handle the external key.<br> > <br> > How this relates to the --provider option is like this: in some cases we<br> > want to know which provider the user prefers. Instead of hard-coding the<br> > preferred provider to "default" which the user may not even want to<br> > load. We could state that the first provider specified will become the<br> > preferred one when there are multiple choices for a method or when a<br> > preferred fallback is required. In that case the option for legacy would be<br> > <br> > --provider default:legacy<br> > <br> > And, if this option is absent, we take it to mean that the "default"<br> > provider should be loaded.<br> <br> I would not explicitly load the default provider then but rather inherit<br> what the system gives us. If the system wide config loads fips we should<br> not load default instead.<br></blockquote><div><br></div><div>IMO, this idea that OpenSSL folks have that just adding a "fips=yes" plus a few lines in the config can make the application FIPS enabled is far-fetched. In reality OpenVPN will have to be recompiled with some changes to make it FIPS compliant. At that point one can also change what providers are loaded by default.</div><div><br></div><div>I was thinking of having a load_providers() function called early -- say in options-post-process stage where we load</div><div><br></div><div>(i) default provider</div><div>(ii) providers specified in --providers option</div><div><br></div><div>This adds much certainty to what is available. A FIPS-enabled OpenVPN can change this or we could have a --fips option that changes this. No dependency on system-wide config file and those who need legacy only need to just specify "--providers legacy"</div><div><br></div><div>In addition, one could automatically load "legacy" if any of the options indicate its need -- like NTLM auth for proxy or BF-CBC in --data-ciphers etc. In situations where it cannot be inferred from options, user will have to use --providers legacy, though.</div><div><br></div><div>It may be necessary to call this load_providers() also to process options like --show-ciphers and similar.</div><div><br></div><div>Selva</div></div></div>
> > IMO, this idea that OpenSSL folks have that just adding a "fips=yes" > plus a few lines in the config can make the application FIPS enabled is > far-fetched. In reality OpenVPN will have to be recompiled with some > changes to make it FIPS compliant. At that point one can also change > what providers are loaded by default. There is a wide range between completely fully FIPS compliant and non-FIPS compliant. Especially on Red Hat Enterprise, you can set the system to a FIPS mode where OpenSSL stops offering non FIPS algorithms and also other crypto libraries do not do non-FIPS algorithms anymore. And I put some effort into making OpenVPN still behave nicely in those situations. It is not really compliant/certified per se but for a lot of customers that is "good enough". The "fips=yes" is similiar I think. It is "good enough" for many people. > I was thinking of having a load_providers() function called early -- say > in options-post-process stage where we load This commit already loads them as early as possible after parsing options if the option is enabled. > > (i) default provider > (ii) providers specified in --providers option > > This adds much certainty to what is available. A FIPS-enabled OpenVPN > can change this or we could have a --fips option that changes this. No > dependency on system-wide config file and those who need legacy only > need to just specify "--providers legacy" > > In addition, one could automatically load "legacy" if any of the options > indicate its need -- like NTLM auth for proxy or BF-CBC in > --data-ciphers etc. In situations where it cannot be inferred from > options, user will have to use --providers legacy, though. > > It may be necessary to call this load_providers() also to process > options like --show-ciphers and similar. No. I am actually against loading legacy on demand or loading the default provider if --provider is not specified. Often there are system wide security defaults in place and I don't think OpenVPN should override them unless explicitly instructed to do so. Especially in the Red Hat Enterprise 9 or whatever will have OpenSSL 3.0 I would expect in fips mode to switch to a global config of OpenSSL that uses the FIPS provider instead of default. And I don't want to break that by loading default. Arne
Hi No. I am actually against loading legacy on demand or loading the > default provider if --provider is not specified. Often there are system > wide security defaults in place and I don't think OpenVPN should > override them unless explicitly instructed to do so. > Okay, that adds some clarity :) In that case let the system default + explicit options take control. If that breaks anything or causes an error, we can try our best to print a helpful error message. Selva <div dir="ltr"><div>Hi</div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> No. I am actually against loading legacy on demand or loading the<br> default provider if --provider is not specified. Often there are system<br> wide security defaults in place and I don't think OpenVPN should<br> override them unless explicitly instructed to do so.<br></blockquote><div><br></div><div>Okay, that adds some clarity :) </div><div><br></div><div>In that case let the system default + explicit options take control. If that breaks anything or causes an error, we can try our best to print a helpful error message.</div><div><br></div><div>Selva</div></div></div>
Hi, Some very minor nits and a couple of points that would require a v4: On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <arne@rfc2549.org> wrote: > This allows OpenVPN to load non-default providers. This is mainly > useful for loading the legacy provider with --provider legacy:default > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > doc/man-sections/generic-options.rst | 10 ++++++++++ > src/openvpn/crypto_backend.h | 7 +++++++ > src/openvpn/crypto_mbedtls.c | 8 ++++++++ > src/openvpn/crypto_openssl.c | 29 ++++++++++++++++++++++++++++ > src/openvpn/openvpn.c | 4 ++++ > src/openvpn/options.c | 4 ++++ > src/openvpn/options.h | 1 + > 7 files changed, 63 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 + 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" Please consider renaming the option as "--providers" in plural. > + + > --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 cc897acf4..fa265e6c2 100644 > --- a/src/openvpn/crypto_backend.h > +++ b/src/openvpn/crypto_backend.h > @@ -78,6 +78,13 @@ void crypto_clear_error(void); > */ > void crypto_init_lib_engine(const char *engine_name); > > + > +/** > + * Load the given (OpenSSL) providers > + * @param providers list of providers to load, seperated by : > + */ > +void crypto_init_lib_provider(const char *providers); > + > #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 2f7f00d19..e6ed1ae99 100644 > --- a/src/openvpn/crypto_mbedtls.c > +++ b/src/openvpn/crypto_mbedtls.c > @@ -70,6 +70,14 @@ crypto_init_lib_engine(const char *engine_name) > "available"); > } > > +void crypto_init_lib_provider(const char *providers) > +{ > + if (providers) > + { > + msg(M_WARN, "Note: mbed TLS provider functionality is not > available"); > + } > +} > + > /* > * > * Functions related to the core crypto library > diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c > index 407ea4a7c..1900ccc1b 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 > > /* > * Check for key size creepage. > @@ -145,6 +148,32 @@ crypto_init_lib_engine(const char *engine_name) > #endif > } > > +void > +crypto_init_lib_provider(const char *providers) > +{ > + if (!providers) > + { > + return; > + } > +#if OPENSSL_VERSION_NUMBER >= 0x30000000L > + struct gc_arena gc = gc_new(); > + char *tmp_providers = string_alloc(providers, &gc); + > + const char *provname; > + while ((provname = strsep(&tmp_providers, ":"))) > + { > + /* Load providers into the default (NULL) library context */ > + OSSL_PROVIDER* provider = OSSL_PROVIDER_load(NULL, provname); > + if (!provider) > + { > + crypto_msg(M_FATAL, "failed to load provider '%s'", provname); > + } > The gc has to be freed here. > + } > +#else /* OPENSSL_VERSION_NUMBER >= 0x30000000L */ > + msg(M_WARN, "Note: OpenSSL hardware crypto engine functionality is > not available"); > "hardware crypto engine" --> "provider" +#endif > +} > + > /* > * > * Functions related to the core crypto library > diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c > index f8e94509f..3c9bcf885 100644 > --- a/src/openvpn/openvpn.c > +++ b/src/openvpn/openvpn.c > @@ -112,6 +112,10 @@ 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 intialised this > + * early since option post processing and also openssl info > + * printing depends on it */ > + crypto_init_lib_provider((*c).options.providers); c->options.providers > } > > static void uninit_early(struct context *c) > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index ed2dcd53d..ab7b00783 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -8178,6 +8178,10 @@ add_option(struct options *options, > options->engine = "auto"; > } > } > + else if (streq(p[0], "provider") && p[1] && !p[2]) > + { > + options->providers = p[1]; > + } > #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 98c21a2a8..6759f1950 100644 > --- a/src/openvpn/options.h > +++ b/src/openvpn/options.h > @@ -521,6 +521,7 @@ struct options > const char *prng_hash; > int prng_nonce_secret_len; > const char *engine; > + const char *providers; > bool replay; > bool mute_replay_warnings; > int replay_window; -- We should not repeatedly load providers in each SIGHUP loop without unloading them in some uninit() call. That would require saving pointers to these explicitly loaded providers, unfortunately... Selva <div dir="ltr"><div dir="ltr">Hi,</div><div><br></div><div>Some very minor nits and a couple of points that would require a v4:</div><div><br></div><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This allows OpenVPN to load non-default providers. This is mainly<br> useful for loading the legacy provider with --provider legacy:default<br> <br> Signed-off-by: Arne Schwabe <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>><br> ---<br> doc/man-sections/generic-options.rst | 10 ++++++++++<br> src/openvpn/crypto_backend.h | 7 +++++++<br> src/openvpn/crypto_mbedtls.c | 8 ++++++++<br> src/openvpn/crypto_openssl.c | 29 ++++++++++++++++++++++++++++<br> src/openvpn/openvpn.c | 4 ++++<br> src/openvpn/options.c | 4 ++++<br> src/openvpn/options.h | 1 +<br> 7 files changed, 63 insertions(+)<br> <br> diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst<br> index e6c1fe455..f5b8a9135 100644<br> --- a/doc/man-sections/generic-options.rst<br> +++ b/doc/man-sections/generic-options.rst<br> @@ -280,6 +280,16 @@ which mode OpenVPN is configured as.<br> This option solves the problem by persisting keys across :code:`SIGUSR1`<br> resets, so they don't need to be re-read.<br> <br> +--provider providers </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + Load the : separated list of (OpenSSL) providers. This is mainly useful for<br> + using an external provider for key management like tpm2-openssl or to load<br> + the legacy provider with<br> +<br> + ::<br> +<br> + --provider "legacy:default" </blockquote><div><br></div><div>Please consider renaming the option as "--providers" in plural.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +<br> --remap-usr1 signal<br> Control whether internally or externally generated :code:`SIGUSR1` signals<br> are remapped to :code:`SIGHUP` (restart without persisting state) or<br> diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h<br> index cc897acf4..fa265e6c2 100644<br> --- a/src/openvpn/crypto_backend.h<br> +++ b/src/openvpn/crypto_backend.h<br> @@ -78,6 +78,13 @@ void crypto_clear_error(void);<br> */<br> void crypto_init_lib_engine(const char *engine_name);<br> <br> +<br> +/**<br> + * Load the given (OpenSSL) providers<br> + * @param providers list of providers to load, seperated by :<br> + */<br> +void crypto_init_lib_provider(const char *providers);<br> +<br> #ifdef DMALLOC<br> /*<br> * OpenSSL memory debugging. If dmalloc debugging is enabled, tell<br> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c<br> index 2f7f00d19..e6ed1ae99 100644<br> --- a/src/openvpn/crypto_mbedtls.c<br> +++ b/src/openvpn/crypto_mbedtls.c<br> @@ -70,6 +70,14 @@ crypto_init_lib_engine(const char *engine_name)<br> "available");<br> }<br> <br> +void crypto_init_lib_provider(const char *providers)<br> +{<br> + if (providers)<br> + {<br> + msg(M_WARN, "Note: mbed TLS provider functionality is not available");<br> + }<br> +}<br> +<br> /*<br> *<br> * Functions related to the core crypto library<br> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c<br> index 407ea4a7c..1900ccc1b 100644<br> --- a/src/openvpn/crypto_openssl.c<br> +++ b/src/openvpn/crypto_openssl.c<br> @@ -54,6 +54,9 @@<br> #if (OPENSSL_VERSION_NUMBER >= 0x10100000L) && !defined(LIBRESSL_VERSION_NUMBER)<br> #include <openssl/kdf.h><br> #endif<br> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L<br> +#include <openssl/provider.h><br> +#endif<br> <br> /*<br> * Check for key size creepage.<br> @@ -145,6 +148,32 @@ crypto_init_lib_engine(const char *engine_name)<br> #endif<br> }<br> <br> +void<br> +crypto_init_lib_provider(const char *providers)<br> +{<br> + if (!providers)<br> + {<br> + return;<br> + }<br> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L<br> + struct gc_arena gc = gc_new();<br> + char *tmp_providers = string_alloc(providers, &gc); </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +<br> + const char *provname;<br> + while ((provname = strsep(&tmp_providers, ":")))<br> + {<br> + /* Load providers into the default (NULL) library context */<br> + OSSL_PROVIDER* provider = OSSL_PROVIDER_load(NULL, provname);<br> + if (!provider)<br> + {<br> + crypto_msg(M_FATAL, "failed to load provider '%s'", provname);<br> + }<br></blockquote><div><br></div><div>The gc has to be freed here.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + }<br> +#else /* OPENSSL_VERSION_NUMBER >= 0x30000000L */<br> + msg(M_WARN, "Note: OpenSSL hardware crypto engine functionality is not available");<br></blockquote><div><br></div><div>"hardware crypto engine" --> "provider"</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +#endif<br> +}<br> +<br> /*<br> *<br> * Functions related to the core crypto library<br> diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c<br> index f8e94509f..3c9bcf885 100644<br> --- a/src/openvpn/openvpn.c<br> +++ b/src/openvpn/openvpn.c<br> @@ -112,6 +112,10 @@ void init_early(struct context *c)<br> /* init verbosity and mute levels */<br> init_verb_mute(c, IVM_LEVEL_1);<br> <br> + /* Initialise OpenVPN provider, this needs to be intialised this<br> + * early since option post processing and also openssl info<br> + * printing depends on it */<br> + crypto_init_lib_provider((*c).options.providers);</blockquote><div><br></div><div>c->options.providers<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> }<br> <br> static void uninit_early(struct context *c)<br> diff --git a/src/openvpn/options.c b/src/openvpn/options.c<br> index ed2dcd53d..ab7b00783 100644<br> --- a/src/openvpn/options.c<br> +++ b/src/openvpn/options.c<br> @@ -8178,6 +8178,10 @@ add_option(struct options *options,<br> options->engine = "auto";<br> }<br> }<br> + else if (streq(p[0], "provider") && p[1] && !p[2])<br> + {<br> + options->providers = p[1];<br> + }<br> #endif /* ENABLE_CRYPTO_MBEDTLS */<br> #ifdef ENABLE_PREDICTION_RESISTANCE<br> else if (streq(p[0], "use-prediction-resistance") && !p[1])<br> diff --git a/src/openvpn/options.h b/src/openvpn/options.h<br> index 98c21a2a8..6759f1950 100644<br> --- a/src/openvpn/options.h<br> +++ b/src/openvpn/options.h<br> @@ -521,6 +521,7 @@ struct options<br> const char *prng_hash;<br> int prng_nonce_secret_len;<br> const char *engine;<br> + const char *providers;<br> bool replay;<br> bool mute_replay_warnings;<br> int replay_window; </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> --</blockquote><div><br></div><div>We should not repeatedly load providers in each SIGHUP loop without unloading them in some uninit() call. That would require saving pointers to these explicitly loaded providers, unfortunately...</div><div><br></div><div>Selva </div></div></div></div>
Hi, I have caused more than enough chatter about this patch, hopefully this is the last comment I previously wrote: > > We should not repeatedly load providers in each SIGHUP loop without > unloading them in some uninit() call. That would require saving pointers to > these explicitly loaded providers, unfortunately... > I want to suggest an alternative: act on this option only the first time, not in every SIGHUP loop. The reason being it's not easy to support changing of loaded providers in the default libctx during every SIGHUP in a good way: OpenSSL will parse its config file only once and cannot be convinced to do it again at SIGHUP. Also it will attempt to do the automatic provider loading only once (the so-called fallback to default). That means, running with "--providers foo" and doing a SIGHUP with that option removed from the config file will result in no providers if we unload "foo". If we do not unload it on SIGHUP, it will always be loaded contrary to user intent, as well as leak memory due to multiple loads. Just document that "--providers" will be parsed once and the program has to be restarted to change it. There are other approaches like swapping the default libctx, but not worth the trouble, IMO. Selva > <div dir="ltr"><div dir="ltr">Hi,<div><br></div><div>I have caused more than enough chatter about this patch, hopefully this is the last comment</div><div><br></div><div>I previously wrote:</div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>We should not repeatedly load providers in each SIGHUP loop without unloading them in some uninit() call. That would require saving pointers to these explicitly loaded providers, unfortunately...</div></div></div></blockquote><div><br></div><div>I want to suggest an alternative: act on this option only the first time, not in every SIGHUP loop. The reason being it's not easy to support changing of loaded providers in the default libctx during every SIGHUP in a good way:</div><div><br></div><div>OpenSSL will parse its config file only once and cannot be convinced to do it again at SIGHUP. Also it will attempt to do the automatic provider loading only once (the so-called fallback to default). That means, running with "--providers foo" and doing a SIGHUP with that option removed from the config file will result in no providers if we unload "foo". If we do not unload it on SIGHUP, it will always be loaded contrary to user intent, as well as leak memory due to multiple loads.</div><div><br></div><div>Just document that "--providers" will be parsed once and the program has to be restarted to change it.</div><div><br></div><div>There are other approaches like swapping the default libctx, but not worth the trouble, IMO.</div><div><br></div><div>Selva</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote></div></div>
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 + 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" + + --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 cc897acf4..fa265e6c2 100644 --- a/src/openvpn/crypto_backend.h +++ b/src/openvpn/crypto_backend.h @@ -78,6 +78,13 @@ void crypto_clear_error(void); */ void crypto_init_lib_engine(const char *engine_name); + +/** + * Load the given (OpenSSL) providers + * @param providers list of providers to load, seperated by : + */ +void crypto_init_lib_provider(const char *providers); + #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 2f7f00d19..e6ed1ae99 100644 --- a/src/openvpn/crypto_mbedtls.c +++ b/src/openvpn/crypto_mbedtls.c @@ -70,6 +70,14 @@ crypto_init_lib_engine(const char *engine_name) "available"); } +void crypto_init_lib_provider(const char *providers) +{ + if (providers) + { + msg(M_WARN, "Note: mbed TLS provider functionality is not available"); + } +} + /* * * Functions related to the core crypto library diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c index 407ea4a7c..1900ccc1b 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 /* * Check for key size creepage. @@ -145,6 +148,32 @@ crypto_init_lib_engine(const char *engine_name) #endif } +void +crypto_init_lib_provider(const char *providers) +{ + if (!providers) + { + return; + } +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + struct gc_arena gc = gc_new(); + char *tmp_providers = string_alloc(providers, &gc); + + const char *provname; + while ((provname = strsep(&tmp_providers, ":"))) + { + /* Load providers into the default (NULL) library context */ + OSSL_PROVIDER* provider = OSSL_PROVIDER_load(NULL, provname); + if (!provider) + { + crypto_msg(M_FATAL, "failed to load provider '%s'", provname); + } + } +#else /* OPENSSL_VERSION_NUMBER >= 0x30000000L */ + msg(M_WARN, "Note: OpenSSL hardware crypto engine functionality is not available"); +#endif +} + /* * * Functions related to the core crypto library diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c index f8e94509f..3c9bcf885 100644 --- a/src/openvpn/openvpn.c +++ b/src/openvpn/openvpn.c @@ -112,6 +112,10 @@ 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 intialised this + * early since option post processing and also openssl info + * printing depends on it */ + crypto_init_lib_provider((*c).options.providers); } static void uninit_early(struct context *c) diff --git a/src/openvpn/options.c b/src/openvpn/options.c index ed2dcd53d..ab7b00783 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -8178,6 +8178,10 @@ add_option(struct options *options, options->engine = "auto"; } } + else if (streq(p[0], "provider") && p[1] && !p[2]) + { + options->providers = p[1]; + } #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 98c21a2a8..6759f1950 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -521,6 +521,7 @@ struct options const char *prng_hash; int prng_nonce_secret_len; const char *engine; + const char *providers; bool replay; bool mute_replay_warnings; int replay_window;
This allows OpenVPN to load non-default providers. This is mainly useful for loading the legacy provider with --provider legacy:default Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- doc/man-sections/generic-options.rst | 10 ++++++++++ src/openvpn/crypto_backend.h | 7 +++++++ src/openvpn/crypto_mbedtls.c | 8 ++++++++ src/openvpn/crypto_openssl.c | 29 ++++++++++++++++++++++++++++ src/openvpn/openvpn.c | 4 ++++ src/openvpn/options.c | 4 ++++ src/openvpn/options.h | 1 + 7 files changed, 63 insertions(+)