Re: MKPasswd idempotency patch

[grub-devel]

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.

Best,
Klaus Frank

On Wed, Jan 16, 2019 at 10:08:05PM +0100, Daniel Kiper wrote:
> 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?
I want to use ansible to set grub passwords. And I also want ansible to 
recognize if a grub password is still valid. The main part of this logic will 
end up within the playbook, but I'll need some functionality to make the 
outcome of grub-mkpasswd-pbkdf2 deterministic, that what providing the salt and 
the salt length provides. For security reasons the caller has to make sure, 
that a given salt is not reused for different passwords.

And this change simultanously allows to use grub-mkpasswd-pbkdf2 within pipes:
`echo "address@hidden" | grub-mkpasswd-pbkdf2 --quiet | tee /path/to/a/file`
and to validate a password by (but by using a longer salt ofcourse):
`diff <(echo 1234 | ./grub-mkpasswd-pbkdf2 --quiet 
--salt-value=D2F60AAE43405216C9C6 -s 10) <(cat /some/file.txt)`

> 
> > 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
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel

0001-Add-quiet-mode.patch
Description: Text Data

0002-Suppress-newline-in-quiet-mode.patch
Description: Text Data

0003-Fix-indention-and-brackets-to-prevent-failures-like-.patch
Description: Text Data

0004-add-verify-password-functionality.patch
Description: Text Data