On Wed, Oct 02, 2024 at 08:57:53AM +0200, Ruediger Pluem wrote: > On 9/12/24 6:04 PM, jor...@apache.org wrote: > > Author: jorton > > Date: Thu Sep 12 16:04:39 2024 > > New Revision: 1920597 > > > > URL: http://svn.apache.org/viewvc?rev=1920597&view=rev > > Log: ... > > + /* If STORE support is not present, all errors are fatal here; if > > + * STORE is present and the ENGINE could not be loaded, ignore the > > + * error and fall through to try loading via the STORE API. */ > > + else if (!MODSSL_HAVE_OPENSSL_STORE || rv != APR_ENOTIMPL) { > > + return ssl_die(s); > > + } > > Sorry for my late comments in chunks, but I stumbled across this when doing > the backport review. > Why are all errors fatal in case we have no STORE support? > If we neither have MODSSL_HAVE_ENGINE_API nor MODSSL_HAVE_OPENSSL_STORE we > just return > APR_ENOTIMPL
Sorry I missed this, but thanks for your persistence in review! This function is only used if modssl_is_engine_id() returns true for some filename, which is only possible if either STORE or ENGINE are supported. If neither are supported the "return APR_ENOTIMPL" path will be the only one compiled, but it should be unreachable. The function must fail if *only* STORE or *only* ENGINE is present and the lookup of the key/cert fails; the point of that conditional was to allow the fallback from ENGINE to STORE in the case that both are compiled. We could make it so that mod_ssl works only with ENGINE or STORE but not both if that's preferred, but it was not technically hard to make this fallback route work so it seemed worth doing IMO. It's a niche feature and in RHEL we are likely to only support ENGINE (RHEL < 10) or STORE (RHEL 10+) but not both, so it's not important for use cases I care about. Does that all make sense? Regards, Joe
