On Sun, Jan 20, 2019 at 02:04:44AM +0100, grub-de...@agowa338.de 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 <g...@frank.fyi>
> 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 <g...@frank.fyi>
> 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 <g...@frank.fyi>
> 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 <g...@frank.fyi>
> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
