On 10/11/24 6:42 PM, Joe Orton wrote:
> 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?
Thanks for explaining. Yes, this makes sense. Just gave my vote on the backport.
Regards
Rüdiger
