Re: bioctl: do not confirm new passphrases on stdin

2023-08-18 Thread Klemens Nanni
On Fri, Aug 18, 2023 at 10:35:37AM +0200, Omar Polo wrote:
> sorry for the noise, noticed just now re-reading the diff.
> 
> On 2023/08/17 09:32:43 +, Klemens Nanni  wrote:
> > --- bioctl.86 Jul 2023 21:08:50 -   1.111
> > +++ bioctl.817 Aug 2023 09:24:28 -
> > @@ -288,7 +288,7 @@ is specified as "auto", the number of ro
> >  based on system performance.
> >  Otherwise the minimum is 4 rounds and the default is 16.
> >  .It Fl s
> > -Read the passphrase for the selected crypto volume from
> > +Omit prompts and read passphrases without confirmation from
> >  .Pa /dev/stdin
> >  rather than
> >  .Pa /dev/tty .
> 
> "read passphrases" hints that it reads more than one, which is not
> what it would do now.

Since -s applies to -P, it does effect multiple passphrases in one usage,
i.e. you enter an old and a new one.

The plural also reads more generic, less specific to me.

> 
> what about something like
> 
> +Read the passphrase from standard input without prompting.

So which one does "the" refer to, old or new one?

I'll commit the manual text as-is now, since that it is already an
improvement;  we can polish in-tree.

> 
> I'm not sure how to fit "without confirmation" nicely and whether the
> singular "passphrase" is enough to convey what it does.
> 
> 
> also, to correct my previous mail
> 
> On 2023/08/17 16:28:52 +0200, Omar Polo  wrote:
> > On 2023/08/17 09:32:43 +, Klemens Nanni  wrote:
> > > [...]
> > > @@ -1316,6 +1316,7 @@ derive_key(u_int32_t type, int rounds, u
> > >   size_t  pl;
> > >   struct stat sb;
> > >   charpassphrase[1024], verifybuf[1024];
> > > + int rpp_flag = RPP_ECHO_OFF;
> > 
> > since this is the default...
> > 
> > >   if (!key)
> > >   errx(1, "Invalid key");
> > > @@ -1351,6 +1352,8 @@ derive_key(u_int32_t type, int rounds, u
> > >  
> > >   fclose(f);
> > >   } else {
> > > + rpp_flag |= interactive ? RPP_REQUIRE_TTY : RPP_STDIN;
> > > +
> > 
> > I'd find slightly easier to read
> > 
> > +   rpp_flag = interactive ? RPP_REQUIRE_TTY : RPP_STDIN
> > 
> > but no strong opinion.
> 
> obviously i meant for this line to be moved earlier in my suggestion.

Also works, but I think jsing has a point here, where XOR'ing the value
makes it clearer how rpp_flag works.

This also matches signify(1)'s code around readpassphrase(3).

> 
> > >   if (readpassphrase(prompt, passphrase, sizeof(passphrase),
> > >   rpp_flag) == NULL)
> > >   err(1, "unable to read passphrase");
> 



Re: bioctl: do not confirm new passphrases on stdin

2023-08-18 Thread Omar Polo
sorry for the noise, noticed just now re-reading the diff.

On 2023/08/17 09:32:43 +, Klemens Nanni  wrote:
> --- bioctl.8  6 Jul 2023 21:08:50 -   1.111
> +++ bioctl.8  17 Aug 2023 09:24:28 -
> @@ -288,7 +288,7 @@ is specified as "auto", the number of ro
>  based on system performance.
>  Otherwise the minimum is 4 rounds and the default is 16.
>  .It Fl s
> -Read the passphrase for the selected crypto volume from
> +Omit prompts and read passphrases without confirmation from
>  .Pa /dev/stdin
>  rather than
>  .Pa /dev/tty .

"read passphrases" hints that it reads more than one, which is not
what it would do now.

what about something like

+Read the passphrase from standard input without prompting.

I'm not sure how to fit "without confirmation" nicely and whether the
singular "passphrase" is enough to convey what it does.


also, to correct my previous mail

On 2023/08/17 16:28:52 +0200, Omar Polo  wrote:
> On 2023/08/17 09:32:43 +, Klemens Nanni  wrote:
> > [...]
> > @@ -1316,6 +1316,7 @@ derive_key(u_int32_t type, int rounds, u
> > size_t  pl;
> > struct stat sb;
> > charpassphrase[1024], verifybuf[1024];
> > +   int rpp_flag = RPP_ECHO_OFF;
> 
> since this is the default...
> 
> > if (!key)
> > errx(1, "Invalid key");
> > @@ -1351,6 +1352,8 @@ derive_key(u_int32_t type, int rounds, u
> >  
> > fclose(f);
> > } else {
> > +   rpp_flag |= interactive ? RPP_REQUIRE_TTY : RPP_STDIN;
> > +
> 
> I'd find slightly easier to read
> 
> + rpp_flag = interactive ? RPP_REQUIRE_TTY : RPP_STDIN
> 
> but no strong opinion.

obviously i meant for this line to be moved earlier in my suggestion.

> > if (readpassphrase(prompt, passphrase, sizeof(passphrase),
> > rpp_flag) == NULL)
> > err(1, "unable to read passphrase");



Re: bioctl: do not confirm new passphrases on stdin

2023-08-17 Thread Omar Polo
On 2023/08/17 09:32:43 +, Klemens Nanni  wrote:
> > I think it would be less ugly to have an iteractive global (or similar)
> > and clear that when -s is given (the correct way to write the above would
> > require masking rpp_flag). 
> 
> Done.  This makes all of the following behave as expected
>   bioctl -cC -lvnd0a softraid0
>   bioctl -d sd2
>   bioctl -s -cC -lvnd0a softraid0
>   bioctl -P sd2
>   bioctl -s -P sd2
> 
> Feedback? OK?

I like this more since using the flags in the global was meh.

> [...]
> @@ -1316,6 +1316,7 @@ derive_key(u_int32_t type, int rounds, u
>   size_t  pl;
>   struct stat sb;
>   charpassphrase[1024], verifybuf[1024];
> + int rpp_flag = RPP_ECHO_OFF;

since this is the default...

>   if (!key)
>   errx(1, "Invalid key");
> @@ -1351,6 +1352,8 @@ derive_key(u_int32_t type, int rounds, u
>  
>   fclose(f);
>   } else {
> + rpp_flag |= interactive ? RPP_REQUIRE_TTY : RPP_STDIN;
> +

I'd find slightly easier to read

+   rpp_flag = interactive ? RPP_REQUIRE_TTY : RPP_STDIN

but no strong opinion.

>   if (readpassphrase(prompt, passphrase, sizeof(passphrase),
>   rpp_flag) == NULL)
>   err(1, "unable to read passphrase");

still ok for me whichever option you prefer.


Thanks,

Omar Polo



Re: bioctl: do not confirm new passphrases on stdin

2023-08-17 Thread Klemens Nanni
On Thu, Aug 17, 2023 at 06:43:36PM +1000, Joel Sing wrote:
> I agree with the intent, however the man page should probably reflect this
> change (i.e. -s makes it non-interactive and you will not get confirmation).

Done.

> 
> > Index: bioctl.c
> > ===
> > RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
> > retrieving revision 1.151
> > diff -u -p -r1.151 bioctl.c
> > --- bioctl.c18 Oct 2022 07:04:20 -  1.151
> > +++ bioctl.c17 Aug 2023 02:16:37 -
> > @@ -989,7 +989,7 @@ bio_kdf_generate(struct sr_crypto_kdfinf
> > derive_key(kdfinfo->pbkdf.generic.type, kdfinfo->pbkdf.rounds,
> > kdfinfo->maskkey, sizeof(kdfinfo->maskkey),
> > kdfinfo->pbkdf.salt, sizeof(kdfinfo->pbkdf.salt),
> > -   "New passphrase: ", 1);
> > +   "New passphrase: ", rpp_flag == RPP_REQUIRE_TTY ? 1 : 0);
> 
> I think it would be less ugly to have an iteractive global (or similar)
> and clear that when -s is given (the correct way to write the above would
> require masking rpp_flag). 

Done.  This makes all of the following behave as expected
bioctl -cC -lvnd0a softraid0
bioctl -d sd2
bioctl -s -cC -lvnd0a softraid0
bioctl -P sd2
bioctl -s -P sd2

Feedback? OK?


Index: bioctl.8
===
RCS file: /cvs/src/sbin/bioctl/bioctl.8,v
retrieving revision 1.111
diff -u -p -r1.111 bioctl.8
--- bioctl.86 Jul 2023 21:08:50 -   1.111
+++ bioctl.817 Aug 2023 09:24:28 -
@@ -288,7 +288,7 @@ is specified as "auto", the number of ro
 based on system performance.
 Otherwise the minimum is 4 rounds and the default is 16.
 .It Fl s
-Read the passphrase for the selected crypto volume from
+Omit prompts and read passphrases without confirmation from
 .Pa /dev/stdin
 rather than
 .Pa /dev/tty .
Index: bioctl.c
===
RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
retrieving revision 1.151
diff -u -p -r1.151 bioctl.c
--- bioctl.c18 Oct 2022 07:04:20 -  1.151
+++ bioctl.c17 Aug 2023 09:23:13 -
@@ -94,7 +94,7 @@ char  *password;
 
 void   *bio_cookie;
 
-int rpp_flag = RPP_REQUIRE_TTY;
+int interactive = 1;
 
 int
 main(int argc, char *argv[])
@@ -200,7 +200,7 @@ main(int argc, char *argv[])
al_arg = optarg;
break;
case 's':
-   rpp_flag = RPP_STDIN;
+   interactive = 0;
break;
case 't': /* patrol */
func |= BIOC_PATROL;
@@ -989,7 +989,7 @@ bio_kdf_generate(struct sr_crypto_kdfinf
derive_key(kdfinfo->pbkdf.generic.type, kdfinfo->pbkdf.rounds,
kdfinfo->maskkey, sizeof(kdfinfo->maskkey),
kdfinfo->pbkdf.salt, sizeof(kdfinfo->pbkdf.salt),
-   "New passphrase: ", 1);
+   "New passphrase: ", interactive);
 }
 
 int
@@ -1316,6 +1316,7 @@ derive_key(u_int32_t type, int rounds, u
size_t  pl;
struct stat sb;
charpassphrase[1024], verifybuf[1024];
+   int rpp_flag = RPP_ECHO_OFF;
 
if (!key)
errx(1, "Invalid key");
@@ -1351,6 +1352,8 @@ derive_key(u_int32_t type, int rounds, u
 
fclose(f);
} else {
+   rpp_flag |= interactive ? RPP_REQUIRE_TTY : RPP_STDIN;
+
if (readpassphrase(prompt, passphrase, sizeof(passphrase),
rpp_flag) == NULL)
err(1, "unable to read passphrase");



Re: bioctl: do not confirm new passphrases on stdin

2023-08-17 Thread Joel Sing
On 23-08-17 02:21:18, Klemens Nanni wrote:
> On Fri, Aug 11, 2023 at 03:44:46PM +, Klemens Nanni wrote:
> > On Wed, Aug 02, 2023 at 10:37:36AM +, Klemens Nanni wrote:
> > > Creating new volumes prompts
> > >   Passphrase:
> > >   Re-type passphrase:
> > > which is sane for interative usage, but -s (which omits prompts) to read
> > > from stdin also prompts twice.
> > > 
> > > I think that's neither intuitive nor ergonomical and as intended for
> > > non-interactive scripts, -s should take a new passphase just once.
> > > 
> > > Until a month ago, the manual errorneously said -s could not be used 
> > > during
> > > initial creation anyway, so I worry little about existing scripts like
> > > 
> > >   printf '%s\n%s\n' "$pw" "$pw" | bioctl -s -cC -lsd0a softraid0
> > > 
> > > Diff below makes -s create new volumes with a single passphase:
> > > 
> > >   # echo secret |   bioctl -s -Cforce -cC -lvnd0a softraid0
> > >   bioctl: Passphrases did not match
> > >   # echo secret | ./obj/bioctl -s -Cforce -cC -lvnd0a softraid0
> > >   softraid0: CRYPTO volume attached as sd3
> > > 
> > > Feedback? Objection? OK?
> > 
> > Ping.
> 
> I'll commit this in a few days unless I hear objections.
> 
> The current -s behaviour is stupid;  nothing else I know *silently* prompts
> for passphrase *and* confirmation without anything in between.
> 
> This stuff must be either interactive or quiet and one-shot, not in between.

I agree with the intent, however the man page should probably reflect this
change (i.e. -s makes it non-interactive and you will not get confirmation).

> Index: bioctl.c
> ===
> RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
> retrieving revision 1.151
> diff -u -p -r1.151 bioctl.c
> --- bioctl.c  18 Oct 2022 07:04:20 -  1.151
> +++ bioctl.c  17 Aug 2023 02:16:37 -
> @@ -989,7 +989,7 @@ bio_kdf_generate(struct sr_crypto_kdfinf
>   derive_key(kdfinfo->pbkdf.generic.type, kdfinfo->pbkdf.rounds,
>   kdfinfo->maskkey, sizeof(kdfinfo->maskkey),
>   kdfinfo->pbkdf.salt, sizeof(kdfinfo->pbkdf.salt),
> - "New passphrase: ", 1);
> + "New passphrase: ", rpp_flag == RPP_REQUIRE_TTY ? 1 : 0);

I think it would be less ugly to have an iteractive global (or similar)
and clear that when -s is given (the correct way to write the above would
require masking rpp_flag). 

>  }
>  
>  int



Re: bioctl: do not confirm new passphrases on stdin

2023-08-17 Thread Omar Polo
On 2023/08/17 02:21:18 +, Klemens Nanni  wrote:
> On Fri, Aug 11, 2023 at 03:44:46PM +, Klemens Nanni wrote:
> > On Wed, Aug 02, 2023 at 10:37:36AM +, Klemens Nanni wrote:
> > > Creating new volumes prompts
> > >   Passphrase:
> > >   Re-type passphrase:
> > > which is sane for interative usage, but -s (which omits prompts) to read
> > > from stdin also prompts twice.
> > > 
> > > I think that's neither intuitive nor ergonomical and as intended for
> > > non-interactive scripts, -s should take a new passphase just once.
> > > 
> > > Until a month ago, the manual errorneously said -s could not be used 
> > > during
> > > initial creation anyway, so I worry little about existing scripts like
> > > 
> > >   printf '%s\n%s\n' "$pw" "$pw" | bioctl -s -cC -lsd0a softraid0
> > > 
> > > Diff below makes -s create new volumes with a single passphase:
> > > 
> > >   # echo secret |   bioctl -s -Cforce -cC -lvnd0a softraid0
> > >   bioctl: Passphrases did not match
> > >   # echo secret | ./obj/bioctl -s -Cforce -cC -lvnd0a softraid0
> > >   softraid0: CRYPTO volume attached as sd3
> > > 
> > > Feedback? Objection? OK?
> > 
> > Ping.
> 
> I'll commit this in a few days unless I hear objections.
> 
> The current -s behaviour is stupid;  nothing else I know *silently* prompts
> for passphrase *and* confirmation without anything in between.
> 
> This stuff must be either interactive or quiet and one-shot, not in between.

fwiw I agree, i find it dumb too to read the password twice when
reading from, for e.g., a pipe.  Outside of my comfort zone but it
reads fine so ok op@.

> Index: bioctl.c
> ===
> RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
> retrieving revision 1.151
> diff -u -p -r1.151 bioctl.c
> --- bioctl.c  18 Oct 2022 07:04:20 -  1.151
> +++ bioctl.c  17 Aug 2023 02:16:37 -
> @@ -989,7 +989,7 @@ bio_kdf_generate(struct sr_crypto_kdfinf
>   derive_key(kdfinfo->pbkdf.generic.type, kdfinfo->pbkdf.rounds,
>   kdfinfo->maskkey, sizeof(kdfinfo->maskkey),
>   kdfinfo->pbkdf.salt, sizeof(kdfinfo->pbkdf.salt),
> - "New passphrase: ", 1);
> + "New passphrase: ", rpp_flag == RPP_REQUIRE_TTY ? 1 : 0);
>  }
>  
>  int




Re: bioctl: do not confirm new passphrases on stdin

2023-08-16 Thread Klemens Nanni
On Fri, Aug 11, 2023 at 03:44:46PM +, Klemens Nanni wrote:
> On Wed, Aug 02, 2023 at 10:37:36AM +, Klemens Nanni wrote:
> > Creating new volumes prompts
> > Passphrase:
> > Re-type passphrase:
> > which is sane for interative usage, but -s (which omits prompts) to read
> > from stdin also prompts twice.
> > 
> > I think that's neither intuitive nor ergonomical and as intended for
> > non-interactive scripts, -s should take a new passphase just once.
> > 
> > Until a month ago, the manual errorneously said -s could not be used during
> > initial creation anyway, so I worry little about existing scripts like
> > 
> > printf '%s\n%s\n' "$pw" "$pw" | bioctl -s -cC -lsd0a softraid0
> > 
> > Diff below makes -s create new volumes with a single passphase:
> > 
> > # echo secret |   bioctl -s -Cforce -cC -lvnd0a softraid0
> > bioctl: Passphrases did not match
> > # echo secret | ./obj/bioctl -s -Cforce -cC -lvnd0a softraid0
> > softraid0: CRYPTO volume attached as sd3
> > 
> > Feedback? Objection? OK?
> 
> Ping.

I'll commit this in a few days unless I hear objections.

The current -s behaviour is stupid;  nothing else I know *silently* prompts
for passphrase *and* confirmation without anything in between.

This stuff must be either interactive or quiet and one-shot, not in between.

Index: bioctl.c
===
RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
retrieving revision 1.151
diff -u -p -r1.151 bioctl.c
--- bioctl.c18 Oct 2022 07:04:20 -  1.151
+++ bioctl.c17 Aug 2023 02:16:37 -
@@ -989,7 +989,7 @@ bio_kdf_generate(struct sr_crypto_kdfinf
derive_key(kdfinfo->pbkdf.generic.type, kdfinfo->pbkdf.rounds,
kdfinfo->maskkey, sizeof(kdfinfo->maskkey),
kdfinfo->pbkdf.salt, sizeof(kdfinfo->pbkdf.salt),
-   "New passphrase: ", 1);
+   "New passphrase: ", rpp_flag == RPP_REQUIRE_TTY ? 1 : 0);
 }
 
 int



Re: bioctl: do not confirm new passphrases on stdin

2023-08-11 Thread Klemens Nanni
On Wed, Aug 02, 2023 at 10:37:36AM +, Klemens Nanni wrote:
> Creating new volumes prompts
>   Passphrase:
>   Re-type passphrase:
> which is sane for interative usage, but -s (which omits prompts) to read
> from stdin also prompts twice.
> 
> I think that's neither intuitive nor ergonomical and as intended for
> non-interactive scripts, -s should take a new passphase just once.
> 
> Until a month ago, the manual errorneously said -s could not be used during
> initial creation anyway, so I worry little about existing scripts like
> 
>   printf '%s\n%s\n' "$pw" "$pw" | bioctl -s -cC -lsd0a softraid0
> 
> Diff below makes -s create new volumes with a single passphase:
> 
>   # echo secret |   bioctl -s -Cforce -cC -lvnd0a softraid0
>   bioctl: Passphrases did not match
>   # echo secret | ./obj/bioctl -s -Cforce -cC -lvnd0a softraid0
>   softraid0: CRYPTO volume attached as sd3
> 
> Feedback? Objection? OK?

Ping.

Index: bioctl.c
===
RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
retrieving revision 1.151
diff -u -p -r1.151 bioctl.c
--- bioctl.c18 Oct 2022 07:04:20 -  1.151
+++ bioctl.c11 Aug 2023 15:42:41 -
@@ -989,7 +989,7 @@ bio_kdf_generate(struct sr_crypto_kdfinf
derive_key(kdfinfo->pbkdf.generic.type, kdfinfo->pbkdf.rounds,
kdfinfo->maskkey, sizeof(kdfinfo->maskkey),
kdfinfo->pbkdf.salt, sizeof(kdfinfo->pbkdf.salt),
-   "New passphrase: ", 1);
+   "New passphrase: ", rpp_flag == RPP_REQUIRE_TTY ? 1 : 0);
 }
 
 int