[PATCH 3/3] openssl: configure engines with uci

[Eneas U de Queiroz]

Hi Florian

On Thu, Apr 29, 2021 at 3:44 AM Florian Eckert <fe at dev.tdt.de> wrote:
> >  $(if
> > CONFIG_OPENSSL_ENGINE_BUILTIN_DEVCRYPTO,/etc/ssl/engines.cnf.d/devcrypto.cnf)
> >  $(if
> > CONFIG_OPENSSL_ENGINE_BUILTIN_PADLOCK,/etc/ssl/engines.cnf.d/padlock.cnf)
>
> I think AFALG is missing there?
>
As I mentioned in the earlier thread, builtin AFALG is weird.  If I
enable it in openssl.cnf, it will always look for afalg.so, and will
fail.  I think it was on oversight, but AFALG is not part of
OPENSSL_INIT_ENGINE_ALL_BUILTIN [1], so it will not be enabled by
default, unless you call
OPENSSL_init_crypto(OPENSSL_INIT_ENGINE_AFALG, NULL).  The AFALG
engine does not have any control commands, so configuration is a noop
anyway.

[1] https://github.com/openssl/openssl/blob/0f077b5fd86e2df0b41608fbd5684fa1a2b58f59/include/openssl/crypto.h.in#L452
> >  endef
> > @@ -378,15 +377,17 @@ define Package/libopenssl/install
> >  endef
> >
> >  define Package/libopenssl-conf/install
> > -     $(INSTALL_DIR) $(1)/etc/ssl/engines.cnf.d
> > +     $(INSTALL_DIR) $(1)/etc/ssl/engines.cnf.d $(1)/etc/config
> > $(1)/etc/init.d
> >       $(CP) $(PKG_INSTALL_DIR)/etc/ssl/openssl.cnf $(1)/etc/ssl/
> > -     $(CP) ./files/engines.cnf $(1)/etc/ssl/engines.cnf.d/
> > +     $(INSTALL_BIN) ./files/openssl.init $(1)/etc/init.d/openssl
> > +     $(SED) 's!%ENGINES_DIR%!/usr/lib/$(ENGINES_DIR)!'
> > $(1)/etc/init.d/openssl
>
> I do not understand that waht you are doing there.
ENGINES_DIR is where the engine so files are stored.  It is versioned,
so it is stored in a variable in engine.mk.  I'm just setting it in
/etc/init.d/openssl,  from ./files/openssl.init#3:
ENGINES_DIR="%ENGINES_DIR%"
The final result, installed in /etc/init.d/openssl#3 is:
ENGINES_DIR="/usr/lib/engines-1.1"

> >       $(if $(CONFIG_OPENSSL_ENGINE_BUILTIN_PADLOCK),
> >               $(CP) ./files/padlock.cnf $(1)/etc/ssl/engines.cnf.d/
> > -             echo padlock=padlock >> $(1)/etc/ssl/engines.cnf.d/engines.cnf)
> > +             echo -e "\nconfig engine 'padlock'\n\toption enabled '1'" >>
> > $(1)/etc/config/openssl)
>
> What about AFALG?
The same explanation above fits here.

> >  #!/bin/sh
> > +OPENSSL_UCI="$$$${IPKG_INSTROOT}/etc/config/openssl"
> > +if [ -n "$$$${IPKG_INSTROOT}" ] || ! uci -q get openssl.$(1) >/dev/null; then
> > +    cat << EOF >> "$$$${OPENSSL_UCI}"
> > +config engine '$(1)'
> > +     option enabled '1'
> > +EOF
>
>  From my point of view, I think it would be better if we used the uci cli
> command directly here.
> to add the config engine section and enable this engine.

However, uci is not available when the package is installed by the
buildsystem, such as when building the firmware image.  That's why I
always check for $IPKG_INSTROOT before calling any commands available
in the target only, as seen above.

>
> >  fi
> > +[ -z "$$$${IPKG_INSTROOT}" ] && /etc/init.d/openssl reload
> >    endef
> >
> > -  define Package/$$(OSSL_ENG_PKG)/prerm :=
> > +  define Package/$$(OSSL_ENG_PKG)/postrm :=
> >  #!/bin/sh
> > -ENGINES_CNF="$$$${IPKG_INSTROOT}/etc/ssl/engines.cnf.d/engines.cnf"
> > -[ -f "$$$${ENGINES_CNF}" ] || exit 0
> > -sed -e '/$(1)=$(1)/d' -i "$$$${ENGINES_CNF}"
> > +[ -z "$$$${IPKG_INSTROOT}" ] && /etc/init.d/openssl reload
>
> Should we not also remove the uci option on an uninstall wit the uci
> command?
>
I'll change this.  My idea was to save the configuration, if user
later reinstall the package.  However, since the %ENGINE%.cnf file is
not removed, then openssl will try to enable the removed engine and
fail.

> > +++ b/package/libs/openssl/files/openssl-engines.init
> > @@ -0,0 +1,19 @@
> > +#!/bin/sh /etc/rc.common
>
> Is the init script also switched on at the first boot?
> So that the service runs immediately?
> Not that the service has to be switched on in /etc/rc.d/ first - that
> would be unpleasant.

Yes, it is: file
build_dir/target-arm_cortex-a9+vfpv3-d16_musl_eabi/root-mvebu/etc/rc.d/S13openssl
build_dir/target-arm_cortex-a9+vfpv3-d16_musl_eabi/root-mvebu/etc/rc.d/S13openssl:
symbolic link to ../init.d/openssl
>
> > +
> > +START=05
> > +OSSL_ENGINES_CNF="/etc/ssl/engines.cnf.d/engines.cnf"
> > +
> > +enable_engine() {
> > +     echo "$1=$1" >> "${OSSL_ENGINES_CNF}"
>
> The writing happens here on the persistent storage at every boot!
> This is not so good for embedded target with FLASH.
> It would be better to write this to the tmp.
>

This file, along with engines.cnf were left over from a previous idea,
and not are not used.  I will take care of them in the v2.  The list
is actually saved in /var/etc/ssl/engines.cnf.

> > +     config_list_foreach openssl.openssl[0] engines enable_engine
>
> How about the named uci section globals
> config openssl globals
>
This is also part of the leftover file.
I've spotted a missing fix for the postinst/postrm scripts that were
failing when building the final image.  I'll send a v2 in a bit.

Thanks for the review!

Eneas