Re: MKPasswd idempotency patch

[Daniel Kiper]

On Sun, Jan 20, 2019 at 02:04:44AM +0100, address@hidden wrote:
> Hello,
>
> I've separated the changes in single commits.
> The patches include:
>
> 0001: Adds a quiet mode to grub-mkpasswd-pbkdf2. So that it can be used 
> within pipes and other tools.
> 0002: Suppresses the leading newline when using the quiet mode. Please pay 
> attention, I may have messed up the bool definition, I'm not quite sure how 
> to verify if that's the case.
> 0003: Fixes some indention and bracket placement
> 0004: Add a salt-value parameter to allow a caller to perform a string 
> comparison to check if a given password matches a given result string. This 
> is usefull for tools like ansible. Without it one has no possibility to use 
> the "has changed" functionality of ansible and needs to always set it again, 
> as the salt is non deterministic. Now a caller can simply run 
> grub-mkpasswd-pbkdf2 once and copy the salt for a single password into the 
> configuration. Therefore the configuration management tool can recalculate 
> the hash and perform a string compare on the result. After that it can show 
> that the destionation system is within desired state and does not need to be 
> changed.

Next time please use "git send-email" to sent the patches.

[...]

> >From d49d6aaaa59fe0f9853171b5cd9963dbedb37fa7 Mon Sep 17 00:00:00 2001
> From: Klaus Frank <address@hidden>
> Date: Sat, 19 Jan 2019 19:03:40 +0100
> Subject: [PATCH 1/4] Add quiet mode

Please explain in the commit message why it is needed.
And do not forget to add SOB to this and following patches.

> ---
>  util/grub-mkpasswd-pbkdf2.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/util/grub-mkpasswd-pbkdf2.c b/util/grub-mkpasswd-pbkdf2.c
> index 5805f3c10..ceb8570bb 100644
> --- a/util/grub-mkpasswd-pbkdf2.c
> +++ b/util/grub-mkpasswd-pbkdf2.c
> @@ -46,6 +46,7 @@ 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},
> +  {"quiet", 'q', 0, 0, N_("Only output hash, suppress other output, indended 
> for pipes"), 0},
>    { 0, 0, 0, 0, 0, 0 }
>  };
>
> @@ -54,6 +55,7 @@ struct arguments
>    unsigned int count;
>    unsigned int buflen;
>    unsigned int saltlen;
> +  unsigned char quiet;
>  };
>
>  static error_t
> @@ -76,6 +78,11 @@ argp_parser (int key, char *arg, struct argp_state *state)
>      case 's':
>        arguments->saltlen = strtoul (arg, NULL, 0);
>        break;
> +
> +    case 'q':
> +      arguments->quiet = 1;
> +      break;
> +
>      default:
>        return ARGP_ERR_UNKNOWN;
>      }
> @@ -116,7 +123,8 @@ main (int argc, char *argv[])
>    struct arguments arguments = {
>      .count = 10000,
>      .buflen = 64,
> -    .saltlen = 64
> +    .saltlen = 64,
> +    .quiet = 0
>    };
>    char *result, *ptr;
>    gcry_err_code_t gcry_err;
> @@ -135,23 +143,26 @@ main (int argc, char *argv[])
>
>    buf = xmalloc (arguments.buflen);
>    salt = xmalloc (arguments.saltlen);
> -
> -  printf ("%s", _("Enter password: "));
> +
> +  if (!arguments.quiet) {
> +    printf ("%s", _("Enter password: "));
> +  }

You do not need curly braces here.

>    if (!grub_password_get (pass1, GRUB_AUTH_MAX_PASSLEN))
>      {
>        free (buf);
>        free (salt);
>        grub_util_error ("%s", _("failure to read password"));
>      }
> -  printf ("%s", _("Reenter password: "));
> -  if (!grub_password_get (pass2, GRUB_AUTH_MAX_PASSLEN))
> +  if (!arguments.quiet) {
> +    printf ("%s", _("Reenter password: "));
> +    if (!grub_password_get (pass2, GRUB_AUTH_MAX_PASSLEN))
>      {
>        free (buf);
>        free (salt);
>        grub_util_error ("%s", _("failure to read password"));
>      }

This piece of code should be moved two spaces right.
You can replace 8 spaces with one tab.

> -  if (strcmp (pass1, pass2) != 0)
> +    if (strcmp (pass1, pass2) != 0)
>      {
>        memset (pass1, 0, sizeof (pass1));
>        memset (pass2, 0, sizeof (pass2));

Ditto and below...

> @@ -159,7 +170,8 @@ main (int argc, char *argv[])
>        free (salt);
>        grub_util_error ("%s", _("passwords don't match"));
>      }
> -  memset (pass2, 0, sizeof (pass2));
> +    memset (pass2, 0, sizeof (pass2));
> +  }
>
>    if (grub_get_random (salt, arguments.saltlen))
>      {
> @@ -200,7 +212,11 @@ main (int argc, char *argv[])
>    ptr += arguments.buflen * 2;
>    *ptr = '\0';
>
> -  printf (_("PBKDF2 hash of your password is %s\n"), result);
> +  if (arguments.quiet) {
> +    printf ("%s\n", result);
> +  } else {
> +    printf (_("PBKDF2 hash of your password is %s\n"), result);
> +  }

You do not need curly braces here.

>    memset (buf, 0, arguments.buflen);
>    free (buf);
>    memset (salt, 0, arguments.saltlen);
> --
> 2.20.1
>

> >From 823a482178577c8c89618e1e537bc5401fbb8e30 Mon Sep 17 00:00:00 2001
> From: Klaus Frank <address@hidden>
> Date: Sun, 20 Jan 2019 01:13:34 +0100
> Subject: [PATCH 2/4] Suppress newline in quiet mode

Why this patch is needed?
And missing SOB...

> ---
>  grub-core/lib/crypto.c             | 12 ++++++++++--
>  grub-core/osdep/unix/password.c    | 12 ++++++++++--
>  grub-core/osdep/windows/password.c | 11 ++++++++++-
>  include/grub/crypto.h              |  8 ++++++++
>  util/grub-mkpasswd-pbkdf2.c        |  4 ++--
>  5 files changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/lib/crypto.c b/grub-core/lib/crypto.c
> index ca334d5a4..50c800af5 100644
> --- a/grub-core/lib/crypto.c
> +++ b/grub-core/lib/crypto.c
> @@ -451,7 +451,7 @@ grub_crypto_memcmp (const void *a, const void *b, 
> grub_size_t n)
>  #ifndef GRUB_UTIL
>
>  int
> -grub_password_get (char buf[], unsigned buf_size)
> +_grub_password_get (char buf[], unsigned buf_size, bool newline)
>  {
>    unsigned cur_len = 0;
>    int key;
> @@ -484,10 +484,18 @@ grub_password_get (char buf[], unsigned buf_size)
>
>    grub_memset (buf + cur_len, 0, buf_size - cur_len);
>
> -  grub_xputs ("\n");
> +  if (newline) {
> +    grub_xputs ("\n");
> +  }
>    grub_refresh ();
>
>    return (key != GRUB_TERM_ESC);
>  }
> +
> +int
> +grub_password_get (char buf[], unsigned buf_size)
> +{
> +  return _grub_password_get(buf, buf_size, true);
> +}
>  #endif
>
> diff --git a/grub-core/osdep/unix/password.c b/grub-core/osdep/unix/password.c
> index 9996b244b..5c3a2b021 100644
> --- a/grub-core/osdep/unix/password.c
> +++ b/grub-core/osdep/unix/password.c
> @@ -27,7 +27,7 @@
>  #include <stdio.h>
>
>  int
> -grub_password_get (char buf[], unsigned buf_size)
> +_grub_password_get (char buf[], unsigned buf_size, bool newline)
>  {
>    FILE *in;
>    struct termios s, t;
> @@ -65,7 +65,9 @@ grub_password_get (char buf[], unsigned buf_size)
>    if (tty_changed)
>      (void) tcsetattr (fileno (in), TCSAFLUSH, &s);
>
> -  grub_xputs ("\n");
> +  if (newline) {
> +    grub_xputs ("\n");
> +  }
>    grub_refresh ();
>
>    if (in != stdin)
> @@ -73,3 +75,9 @@ grub_password_get (char buf[], unsigned buf_size)
>
>    return 1;
>  }
> +
> +int
> +grub_password_get (char buf[], unsigned buf_size)
> +{
> +  return _grub_password_get(buf, buf_size, true);
> +}
> diff --git a/grub-core/osdep/windows/password.c 
> b/grub-core/osdep/windows/password.c
> index 1d3af0c2c..79eafb4fa 100644
> --- a/grub-core/osdep/windows/password.c
> +++ b/grub-core/osdep/windows/password.c
> @@ -27,7 +27,7 @@
>  #include <stdio.h>
>
>  int
> -grub_password_get (char buf[], unsigned buf_size)
> +_grub_password_get (char buf[], unsigned buf_size, bool newline)

Two underscores at the beginning of the function name please.

>  {
>    HANDLE hStdin = GetStdHandle (STD_INPUT_HANDLE);
>    DWORD mode = 0;
> @@ -45,7 +45,16 @@ grub_password_get (char buf[], unsigned buf_size)
>
>    SetConsoleMode (hStdin, mode);
>
> +  if (newline) {
> +    grub_xputs ("\n");
> +  }

Redundant curly braces. And why you add "\n" here?

>    grub_refresh ();
>
>    return 1;
>  }
> +
> +int
> +grub_password_get (char buf[], unsigned buf_size)
> +{
> +  return _grub_password_get(buf, buf_size, false);
> +}
> diff --git a/include/grub/crypto.h b/include/grub/crypto.h
> index a24e89dd9..9cdf0693f 100644
> --- a/include/grub/crypto.h
> +++ b/include/grub/crypto.h
> @@ -28,6 +28,11 @@
>  #include <grub/err.h>
>  #include <grub/mm.h>
>
> +#ifndef GRUB_POSIX_BOOL_DEFINED
> +#define GRUB_POSIX_BOOL_DEFINED 1
> +#include <stdbool.h>
> +#endif
> +
>  typedef enum
>    {
>      GPG_ERR_NO_ERROR,
> @@ -396,6 +401,9 @@ grub_crypto_pbkdf2 (const struct gcry_md_spec *md,
>  int
>  grub_crypto_memcmp (const void *a, const void *b, grub_size_t n);
>
> +int
> +_grub_password_get (char buf[], unsigned buf_size, bool newline);
> +
>  int
>  grub_password_get (char buf[], unsigned buf_size);
>
> diff --git a/util/grub-mkpasswd-pbkdf2.c b/util/grub-mkpasswd-pbkdf2.c
> index ceb8570bb..66f3ab2ba 100644
> --- a/util/grub-mkpasswd-pbkdf2.c
> +++ b/util/grub-mkpasswd-pbkdf2.c
> @@ -147,14 +147,14 @@ main (int argc, char *argv[])
>    if (!arguments.quiet) {
>      printf ("%s", _("Enter password: "));
>    }
> -  if (!grub_password_get (pass1, GRUB_AUTH_MAX_PASSLEN))
> +  if (!_grub_password_get (pass1, GRUB_AUTH_MAX_PASSLEN, false))
>      {
>        free (buf);
>        free (salt);
>        grub_util_error ("%s", _("failure to read password"));
>      }
>    if (!arguments.quiet) {
> -    printf ("%s", _("Reenter password: "));
> +    printf ("%s", _("\nReenter password: "));
>      if (!grub_password_get (pass2, GRUB_AUTH_MAX_PASSLEN))
>      {
>        free (buf);
> --
> 2.20.1
>

> >From 8d1bda7fbc3d19fedec46a043adb81720a177103 Mon Sep 17 00:00:00 2001
> From: Klaus Frank <address@hidden>
> Date: Sat, 19 Jan 2019 19:23:25 +0100
> Subject: [PATCH 3/4] Fix indention and brackets, to prevent failures like the
>  double goto fail from apple.

Everything after comma should go into the commit message. And I think
that this should be extended a bit here too.

> ---
>  util/grub-mkpasswd-pbkdf2.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/util/grub-mkpasswd-pbkdf2.c b/util/grub-mkpasswd-pbkdf2.c
> index 66f3ab2ba..92e18fd37 100644
> --- a/util/grub-mkpasswd-pbkdf2.c
> +++ b/util/grub-mkpasswd-pbkdf2.c
> @@ -101,16 +101,18 @@ 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 {
> +        *hex = ((*bin & 0xf0) >> 4) + 'A' - 10;
> +      }

Redundant curly braces.

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

Ditto.

>        hex++;
>        bin++;
>      }
> --
> 2.20.1
>

> >From f06f4c0388797a50b55f479f7b10a24a52be4701 Mon Sep 17 00:00:00 2001
> From: Klaus Frank <address@hidden>
> Date: Sun, 20 Jan 2019 00:03:13 +0100
> Subject: [PATCH 4/4] add verify password functionality

Why it is needed?

> ---
>  util/grub-mkpasswd-pbkdf2.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/util/grub-mkpasswd-pbkdf2.c b/util/grub-mkpasswd-pbkdf2.c
> index 92e18fd37..4ed62d04f 100644
> --- a/util/grub-mkpasswd-pbkdf2.c
> +++ b/util/grub-mkpasswd-pbkdf2.c
> @@ -42,10 +42,13 @@
>
>  #include "progname.h"
>
> +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},
>    {"quiet", 'q', 0, 0, N_("Only output hash, suppress other output, indended 
> for pipes"), 0},
>    { 0, 0, 0, 0, 0, 0 }
>  };
> @@ -55,6 +58,7 @@ struct arguments
>    unsigned int count;
>    unsigned int buflen;
>    unsigned int saltlen;
> +  char *value;
>    unsigned char quiet;
>  };
>
> @@ -79,6 +83,10 @@ argp_parser (int key, char *arg, struct argp_state *state)
>        arguments->saltlen = strtoul (arg, NULL, 0);
>        break;
>
> +    case 'v':
> +      arguments->value = arg;
> +      break;
> +
>      case 'q':
>        arguments->quiet = 1;
>        break;
> @@ -119,6 +127,19 @@ hexify (char *hex, grub_uint8_t *bin, grub_size_t n)
>    *hex = 0;
>  }
>
> +int
> +unhexify(grub_uint8_t* output, const char* hexstring) {
> +  if (sizeof(output) < (sizeof(hexstring) - 1) / 2 + 1) {

I am afraid that this does not what you expect.

> +    return -1;
> +  }
> +  char *output_ptr = (char*)output;
> +  char *hexstring_ptr = (char*)hexstring;

Missing empty line. And declarations should go first in the function.
Then the code.

> +  for (int c; hexstring[0] && hexstring[1] && sscanf(hexstring_ptr, "%2x", 
> &c); hexstring_ptr += 2) {

Please define c variable at the beginning of the function.

> +    sprintf(output_ptr++, "%s",(char*)&c);
> +  }

Redundant curly braces and missing empty line here.

> +  return 0;
> +}
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -126,6 +147,7 @@ main (int argc, char *argv[])
>      .count = 10000,
>      .buflen = 64,
>      .saltlen = 64,
> +    .value = NULL,
>      .quiet = 0
>    };
>    char *result, *ptr;
> @@ -175,13 +197,22 @@ main (int argc, char *argv[])
>      memset (pass2, 0, sizeof (pass2));
>    }
>
> -  if (grub_get_random (salt, arguments.saltlen))
> +  if (arguments.value) {
> +    if (unhexify(salt, arguments.value)) {
> +      memset(pass1, 0, sizeof(pass1));
> +      free(buf);
> +      free(salt);
> +      grub_util_error("%s", _("couldn't convert hexstring into salt"));
> +    }
> +  } else {
> +    if (grub_get_random (salt, arguments.saltlen))
>      {
>        memset (pass1, 0, sizeof (pass1));
>        free (buf);
>        free (salt);
>        grub_util_error ("%s", _("couldn't retrieve random data for salt"));

Incorrect indention.

>      }

Anyway, patches looks better right now.
So, please keep working on them.

Daniel