Re: MKPasswd idempotency patch

[Daniel Kiper]

On Tue, Dec 25, 2018 at 12:01:12PM +0100, address@hidden wrote:
> Hello,
>
> I want to use grub-mkpasswd inside of a ansible playbook, therefore a
> idempotency or check feature is required. Without it, there is no
> posibility to verify, that the configuration is already inplace.
> Therefore I want to suggest implementing the attached changes. The
> simplest one I was thinking of, is grub-mkpasswd allowing to specify a
> salt and comparing the results.

I am not sure I understand. Could you elaborate on this?

> My patch also introduces a quiet mode, that basically suppresses all
> prompts and only expects the password to be entered/piped in once. It
> also changes the output slightly to not inlcude the `PBKDF2 hash of
> your password is `-part.

It seems to me that it should be split into 3 parts...

> The patch is attached to this mail.
>
> Best,
> Klaus Frank

> >From d289bbb5f5dffbbed2c871989f55e48b756e9ae2 Mon Sep 17 00:00:00 2001
> From: Klaus Frank <address@hidden>
> Date: Tue, 25 Dec 2018 06:17:38 +0100
> Subject: [PATCH] Implement salt-value and quiet parameter for
>  grub-mkpasswd-pbkdf2
>
> ---
>  util/grub-mkpasswd-pbkdf2.c | 103 ++++++++++++++++++++++++++++++------
>  1 file changed, 88 insertions(+), 15 deletions(-)
>
> diff --git a/util/grub-mkpasswd-pbkdf2.c b/util/grub-mkpasswd-pbkdf2.c
> index 5805f3c10..018715bf3 100644
> --- a/util/grub-mkpasswd-pbkdf2.c
> +++ b/util/grub-mkpasswd-pbkdf2.c
> @@ -42,10 +42,15 @@
>
>  #include "progname.h"
>
> +static int dec_from_hex(const int); // Converts the charcode into an integer 
> representation of the representing hex value.
> +int unhexify(grub_uint8_t*, const char*);
> +
>  static struct argp_option options[] = {
>    {"iteration-count",  'c', N_("NUM"), 0, N_("Number of PBKDF2 iterations"), 
> 0},
>    {"buflen",  'l', N_("NUM"), 0, N_("Length of generated hash"), 0},
>    {"salt",  's', N_("NUM"), 0, N_("Length of salt"), 0},
> +  {"salt-value", 'v', N_("STR"), 0, N_("Salt"), 0},

For this patch number one...

> +  {"quiet", 'q', 0, 0, N_("Only output hash, suppress other output, indended 
> for pipes"), 0},

...and for this patch number two.

The rest goes into patch three. The basic idea is that one logical
change is one patch.

>    { 0, 0, 0, 0, 0, 0 }
>  };
>
> @@ -54,6 +59,8 @@ struct arguments
>    unsigned int count;
>    unsigned int buflen;
>    unsigned int saltlen;
> +  char *value;
> +  unsigned char quiet;
>  };
>
>  static error_t
> @@ -76,6 +83,15 @@ argp_parser (int key, char *arg, struct argp_state *state)
>      case 's':
>        arguments->saltlen = strtoul (arg, NULL, 0);
>        break;
> +
> +    case 'v':
> +      arguments->value = arg;
> +      break;
> +
> +    case 'q':
> +      arguments->quiet = 1;
> +      break;
> +
>      default:
>        return ARGP_ERR_UNKNOWN;
>      }
> @@ -94,29 +110,70 @@ hexify (char *hex, grub_uint8_t *bin, grub_size_t n)
>  {
>    while (n--)
>      {
> -      if (((*bin & 0xf0) >> 4) < 10)
> -     *hex = ((*bin & 0xf0) >> 4) + '0';
> -      else
> -     *hex = ((*bin & 0xf0) >> 4) + 'A' - 10;
> +      if (((*bin & 0xf0) >> 4) < 10) {
> +        *hex = ((*bin & 0xf0) >> 4) + '0';
> +      } else {

Curly braces are not needed here and below.

> +        *hex = ((*bin & 0xf0) >> 4) + 'A' - 10;
> +      }

Please do not introduce clutter like this. If you wish to do some
cleanups do it in separate patch.

>        hex++;
>
>        if ((*bin & 0xf) < 10)
> -     *hex = (*bin & 0xf) + '0';
> +        *hex = (*bin & 0xf) + '0';

Ditto.

>        else
> -     *hex = (*bin & 0xf) + 'A' - 10;
> +        *hex = (*bin & 0xf) + 'A' - 10;

Ditto.

>        hex++;
>        bin++;
>      }
>    *hex = 0;
>  }
>
> +static int
> +dec_from_hex(const int hex) {
> +  if (48 <= hex && hex <= 57) {
> +    return hex - 48;
> +  } else if (65 <= hex && hex <= 90) {
> +    return hex - 65 + 10;
> +  } else if (97 <= hex && hex <= 122) {
> +    return hex - 97 + 10;
> +  } else {
> +    exit(1);
> +  }
> +}

grub_strtoul() please.

> +int
> +unhexify(grub_uint8_t* output, const char* hexstring) {
> +  // sizeof(output) == (sizeof(hexstring) - 1) / 2 + 1
> +  if (sizeof(output) < (sizeof(hexstring) - 1) / 2 + 1) {
> +    return -1;
> +  }
> +  char *output_ptr = NULL;
> +  output_ptr = (char*)output;
> +  int CharCodeInDecimal = 0;
> +  void *HexDigits = xmalloc(sizeof("FF"));
> +  char *HexDigitOne = HexDigits;
> +  char *HexDigitTwo = (char*)HexDigits + 1;
> +  char *HexString_ptr = NULL;
> +  HexString_ptr = (char*)hexstring;
> +  while(*HexString_ptr) {
> +    memcpy (HexDigitOne, HexString_ptr++, sizeof (char));
> +    memcpy (HexDigitTwo, HexString_ptr++, sizeof (char));
> +
> +    CharCodeInDecimal = dec_from_hex((int)*HexDigitOne) * 16 + 
> dec_from_hex((int)*HexDigitTwo);
> +    *output_ptr++ = (char)CharCodeInDecimal;
> +  }
> +  output_ptr = '\0';
> +  return 0;
> +}

This is a mess. There is a pretty good chance that similar function
exists in the wild. So, please do not reinvent the wheel.

Daniel