Re: [patch 1/2] doas(1): Moved some parsing from env.c into parse.y

[bytevolcano]

Hello Philippe,

On Thu, 16 Mar 2017 10:19:12 -0400
Philippe Meunier  wrote:

> Ted Unangst wrote:
> >Did I get it backwards? If you have setenv { HERE= there }, your diff
> >changes behavior.  
> 
> Speaking from the peanut gallery here, but I find this syntax rather
> confusing and error-prone, especially for a security-related file
> such as doas.conf.  How about making the list of variables
> comma-separated instead of space-separated, so that the intent is
> clearer from the syntax?

Whilst I agree with this, I presume this would depend on the popular use
case. If Ted and co found the major use case to be just to pass
variables along like this:

 setenv { FOO BAR }

then a space-delimited list is fine, and the parser is simpler. As I
see it, this is also acceptable:

 setenv { "FOO=" "BAR" }

Adding a comma-separated list will be similar to what I did to move the
'=' processing into the lexer. It's possible, but it does add some code
to the lexer and doesn't allow for much refactoring, unless you also
have command line args like this:

 ... cmd hello args foo, bar

It also means that command-line arguments with commas in them will need
to be enclosed in quote marks.

I have had a lot of time to think about this. Since equal sign
processing is needed anyway in env.c to process the strings in the
"environ" variable (filled in by the OS), might as well keep it all in
one place.

Re: [patch 1/2] doas(1): Moved some parsing from env.c into parse.y

[bytevolcano]

On Wed, 15 Mar 2017 20:15:26 -0400
"Ted Unangst"  wrote:

> Did I get it backwards? If you have setenv { HERE= there }, your diff
> changes behavior.
> 

Now I see where you are coming from. It's meant to empty $HERE, and copy
$there. Thanks, I'll look into this.

Re: [patch 1/2] doas(1): Moved some parsing from env.c into parse.y

[Ted Unangst]

bytevolc...@safe-mail.net wrote:
> > Also, I'm not sure how you tested this, because it doesn't work like
> > you say it does. setenv { HERE=there } copies both $HERE and $there
> > from the current environment. It does not set HERE=there. That's very
> > wrong.
> > 
> 
> How are you getting that result? Here is what I am getting:
> 
>   $ export HERE=hello
>   $ export there=world
>   $ echo $HERE $there
>   hello world
>   $ doas ~/test.sh
>   HERE is there
>   there is

Did I get it backwards? If you have setenv { HERE= there }, your diff
changes behavior.

Re: [patch 1/2] doas(1): Moved some parsing from env.c into parse.y

[bytevolcano]

On Wed, 15 Mar 2017 13:22:53 -0400
"Ted Unangst"  wrote:
...
> > 1. Loops - Use C99 style initialisation?
> > for(int i = 0; i < monsters; i < 666)  
> 
> No.
> 
> > 3. Anything of major concern.  
> 
> Adding a new character to the lexer potentially breaks existing
> config files.

I was considering that, considering that maybe someone will want
"foo=bar" parameters in their command lines.
Which is why I am thinking it is best to strike here while the iron is
hot rather than, say, 5 years later, when people have come up with
complex and tuned-over-the-years configurations. Not something difficult to
fix either, just enclose in quotes or escape, as necessary with curly
braces.

There is a way around this (such as not moving to "eow" on special
characters, when the lexer is in a 'command-line argument-parsing
mode') but that may require code added to the lexer. How much
extra code is needed, I don't know.

This function of parsing is the lexer's domain, and seems neater than
having what is essentially two separate lexers; all I am doing here is
merging the "mini-lexer" into the main lexer.

> 
> Also, I'm not sure how you tested this, because it doesn't work like
> you say it does. setenv { HERE=there } copies both $HERE and $there
> from the current environment. It does not set HERE=there. That's very
> wrong.
> 

How are you getting that result? Here is what I am getting:

$ export HERE=hello
$ export there=world
$ echo $HERE $there
hello world
$ doas ~/test.sh
HERE is there
there is

The shell script:

#!/bin/sh
echo "HERE" is $HERE
echo "there" is $there

And /etc/doas.conf:

permit persist setenv { HERE=there } :wheel
permit nopass keepenv root

This test behaves correctly on my end.

Re: [patch 1/2] doas(1): Moved some parsing from env.c into parse.y

[Ted Unangst]

bytevolc...@safe-mail.net wrote:
> Instead of having to use another 1KB buffer just to figure out where
> the '=' sign is, use the parser infrastructure already present. Also
> allows for variable timeout option. Another (negligible) advantage: if
> one day someone wants to change the maximum line length, they only need
> to do so in one spot.
> 
> I haven't touched the code in env.c that handles the '-' and '$' since
> these are extremely simple anyway, and aren't needed for the next patch.
> 
> This patch doesn't need changes to doas(1) or doas.conf(5) man pages, but 
> there
> is no mention of characters that are required to be escaped when specifying 
> args.
> Tested with all kinds of variable names and values, works fine:
> 
>   ... keepenv { FIRE SWITCH=1 NO_PLACE_LIKE=$HOME WATER}
> 
> Things I would like feedback on, in particular:
> 
> 1. Loops - Use C99 style initialisation?
>   for(int i = 0; i < monsters; i < 666)

No.

> 3. Anything of major concern.

Adding a new character to the lexer potentially breaks existing config files.

Also, I'm not sure how you tested this, because it doesn't work like you say
it does. setenv { HERE=there } copies both $HERE and $there from the current
environment. It does not set HERE=there. That's very wrong.