[PATCH v2 3/7] common: integrate crypt-based passwords

[Simon Glass]

Hi Steffen,

On Mon, 10 May 2021 at 13:37, Steffen Jaeckel
<jaeckel-floss at eyet-services.de> wrote:
>
> Hi Simon
>
> On 5/10/21 10:24 PM, Simon Glass wrote:
> > On Mon, 10 May 2021 at 14:06, Steffen Jaeckel
> > <jaeckel-floss at eyet-services.de> wrote:
> >> On 5/10/21 9:19 PM, Simon Glass wrote:
> >>> On Mon, 10 May 2021 at 11:05, Steffen Jaeckel
> >>> <jaeckel-floss at eyet-services.de> wrote:
> >>>> On 5/10/21 6:27 PM, Simon Glass wrote:
> >>>>> On Mon, 10 May 2021 at 00:19, Steffen Jaeckel
> >>>>> <jaeckel-floss at eyet-services.de> wrote:
> >>>>>>
> >>>>>> Hook into the autoboot flow as an alternative to the existing
> >>>>>> mechanisms.
> >>>>>>
> >>>>>> Signed-off-by: Steffen Jaeckel <jaeckel-floss at eyet-services.de>
> >>>>>> ---
> >>>>>>
> >>>>>> (no changes since v1)
> >>>>>>
> >>>>>>  common/Kconfig.boot | 37 ++++++++++++++++++---
> >>>>>>  common/autoboot.c   | 80 ++++++++++++++++++++++++++++++++++++++++-----
> >>>>>>  2 files changed, 103 insertions(+), 14 deletions(-)
> >>>>>
> >>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
> >>>>>
> >>>>> But I think you'll need to allow both to be enabled.
> >>>>
> >>>> Sorry, but what exactly do you mean?
> >>>
> >>> I mean the ability to enable both crypt and the sha options rather
> >>> than just one at a time.
> >>
> >> You have a point there. Even though this approach should IMO become the
> >> recommended way to store passwords, one could imagine that support for
> >> both approaches in parallel could be needed, e.g. in a transition period.
> >>
> >> The biggest problem I see is that the passwd_abort_{crypt,sha256}()
> >> functions consume the serial input. I fear that an implementation that
> >> supports both would need to have a painful amount of special case
> >> handling in order to not break the expected behavior of the existing
> >> sha256 implementation.
> >>
> >> Supporting both in a backwards compatible way would make the
> >> implementation a lot more complex and therefor I'd prefer to leave it as is.
> >
> > I don't quite mean that. Only one should be used at once. But would it
> > be possible to support both at runtime, so that (at runtime) you check
> > an env var to decide which is active?
>
> oh okay, that's easier :)
>
> maybe something like this?
>
> diff --git a/common/autoboot.c b/common/autoboot.c
> index 50ab9281e7..6f55abe388 100644
> --- a/common/autoboot.c
> +++ b/common/autoboot.c
> @@ -316,3 +316,4 @@ static int abortboot_key_sequence(int bootdelay)
>         if (IS_ENABLED(CONFIG_AUTOBOOT_ENCRYPTION)) {
> -               if (IS_ENABLED(CONFIG_CRYPT_PW))
> +               if (IS_ENABLED(CONFIG_CRYPT_PW) &&
> +                   env_get_yesno("bootstopusesha256") != 1)
>                         abort = passwd_abort_crypt(etime);

Yes, and then you can enable both in sandbox and potentially have a
test for your code within the standard sandbox build.

Regards,
Simon