On Fri, Jul 25, 2014 at 11:50:02PM +0200, Ola Lundqvist wrote:
> Hi
>
> Thanks for the suggestion. I like the solution. The question is whether we
> should do this for more commands or not.
All of the other *cmd's get passed through cmdaction(), which essentially
turns them into system("$cmd") -- which means adding arguments to them
does indeed work, it just possibly gets a little tricky if those arguments
themselves would want quoting (or unsafe if they came from an untrusted
source, but that's not an issue in this code).
We could in theory retrofit those like this too, but it would probably be
a considerably more involved patch to do it, since something like this
would be ok:
$foocmd = "ls -al";
@foocmd = ($foocmd);
system(@foocmd);
since that still triggers the 'one argument' version of system(), but
system(@foocmd, "bar") would end badly in this case.
I might be missing something obvious, but in practice I think it would
mean you'd need to rewrite cmdaction(), both to take its parameters
differently and call system() differently, and then have to rewrite
every use of cmdaction too, since some things like $copycmd will take
multiple parameters of its own beyond what the user configures.
Which probably isn't worth the effort (and risk of new bugs) unless
someone shows a real use case that really needs it.
Looking at the list of other commands, the only one that jumps out at
me as this possibly being really useful for too might be $usermailcmd.
But that would also need fairly extensive changes, and might be better
done in the context of the other bug, with a patch to make mail really
be optional.
For $vrfycmd, we win and this is easy and safe, because we know the
scalar version never had more than one argument in it (else it would
have failed), and it's the only one that gets passed directly to
system() without going through cmdaction() or something similar.
So I'd probably suggest:
1) Do $vrfycmd now as a single clean and obvious patch.
2) Look at options for $usermailcmd et al. in the context of
changes needed for the other bug report.
3) Worry about the other commands if anyone ever really hits a
case where the existing usage is troublesome.
Cheers,
Ron
> On Mon, Jul 21, 2014 at 12:27 AM, Ron <r...@debian.org> wrote:
>
> > Package: debarchiver
> > Version: 0.10.1
> > Severity: normal
> >
> > Hi Ola,
> >
> > I ran into another quirk today :) The way that $vrfycmd is used means
> > it's not directly possible to set that to a command that requires some
> > arguments other than the changes file.
> >
> > What I wanted to do was this:
> >
> > $vrfycmd = "dscverify --no-default-keyrings --keyring
> > /var/lib/debarchiver/.gnupg/pubring.gpg";
> >
> > So that I could sign packages uploaded to this repo with a different key
> > to the one that is in the DD keyring, to avoid there ever being any sort
> > of accident with them actually getting pushed into a distro upload queue
> > (either by me, or by someone else later).
> >
> > It looks like there's a few easy options to fix this though.
> > My favourite so far is the patch below, which is backward compatible for
> > any existing user config, but lets me instead do:
> >
> > @vrfycmd = ("dscverify", "--no-default-keyrings", "--keyring",
> > "/var/lib/debarchiver/.gnupg/pubring.gpg");
> >
> > Which ensures there'll never be any quoting issues for weird arguments.
> >
> > Alternatively we could do system("$vrfycmd $cfile"), but that has a
> > different set of pros and cons.
> >
> > It looks like the rest of the *cmd options go through cmdaction() so
> > they don't have this problem.
> >
> > If you like this version, it probably also wants a oneliner in the
> > sample debarchiver.conf too to note @vrfycmd can be an array now.
> >
> > Cheers,
> > Ron
> >
> >
> > --- /usr/bin/debarchiver 2014-07-21 06:05:08.059769856 +0930
> > +++ debarchiver 2014-07-21 07:22:11.316828267 +0930
> > @@ -96,6 +96,7 @@
> > $rmcmd = "rm -f";
> > $movecmd = "mv";
> > $vrfycmd = "dscverify";
> > +@vrfycmd = ($vrfycmd);
> > $cachedir = "/var/cache/debarchiver";
> > $inputdir = "/var/lib/debarchiver/incoming";
> > $destdir = "/var/lib/debarchiver/dists";
> > @@ -1742,9 +1743,9 @@
> > }
> > # Verify signatures.
> > if ($verify) {
> > - if (system($vrfycmd,$cfile)) { # non-zero == verification failure
> > + if (system(@vrfycmd,$cfile)) { # non-zero == verification failure
> > pdebug(4, "Signature verification failed for $cfile");
> > - $CConf{ERROR} = "$CConf{ERROR}$vrfycmd was not able to verify
> > $cfile.\n";
> > + $CConf{ERROR} = "$CConf{ERROR}@vrfycmd was not able to verify
> > $cfile.\n";
> > return "reject";
> > }
> > }
> >
>
>
>
> --
> --- Inguza Technology AB --- MSc in Information Technology ----
> / o...@inguza.com Annebergsslingan 37 \
> | o...@debian.org 654 65 KARLSTAD |
> | http://inguza.com/ Mobile: +46 (0)70-332 1551 |
> \ gpg/f.p.: 7090 A92B 18FE 7994 0C36 4FE4 18A1 B1CF 0FE5 3DD9 /
> ---------------------------------------------------------------
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
