Re: [Monotone-devel] [Patch] mtn automate lua

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> Stephen Leake schrieb:
>>> I'm still a bit undecided when it comes to security constraints, like
>>> should a couple of hooks be disabled (i.e. should I maintain a list of
>>> "supported" hooks) or should I just leave this out into the wild as is?
>> 
>> As I understand the current security model, there are two layers. If
>> there is a network connection, that connection has its own
>> authentication/validation method, which we can assume works. Then
>> there is a key required for write access to the database.
>
> The problem is that if you do networking with automate stdio (like f.e.
> Thomas Moschny's json server) there is no such security model at all,
> i.e. netsync permissions are simply skipped / not used. You can still
> put arbitrary contents over this interface into the database, via the
> put_file, put_revision and cert commands.

Hmm. I need a key to do commit to a local database; that has nothing
to do with netsync:

address@hidden mtn --db test.db db init 
address@hidden cd test_paths/
address@hidden ls
fs.cc  fs.hh  main.cc  Makefile
address@hidden mtn --db ../test.db setup --branch foo
address@hidden mtn add fs.cc
mtn: adding fs.cc to workspace manifest
address@hidden mtn --key foo commit --message "key foo"
mtn: beginning commit on branch 'foo'
mtn: misuse: no key pair 'foo' found in key store '/home/stephe/.monotone/keys'

I guess you are saying it is the 'commit' command
itself that checks the key, not the low level 'put_*'. That's
surprising, but true:

address@hidden mtn automate put_file fs.cc
541589ed4bcfc1bc300597d937b2e9b32947099d

Why is the key not checked for every database write operation?


Getting back to the 'automate stdio' issue. It is similar to 'mtn sync
ssh:' now; mtn assumes the ssh authentication is adequate, and does
not require keys. This saves both runtime and administrative overhead.

So 'mtn automate stdio' assumes the network connection does adequate
authentication. It is up to the user to ensure that is true.

>>> However, this might be a completly different cup of tea if we open / use
>>> the automation interface for networking - of course we do not want to
>>> allow anybody even with write access to a database being able to
>>> retrieve any kind of the local password. 
>> 
>> That's a good point. In terms of the two layers above, this adds a
>> third layer, requiring protection of "local data". Not really another
>> layer, just covers different data.
>
> Yes, one could speak of "local data" here, but it might not always be
> easy to draw the line here. Are uncommitted changes in a local workspace
> ("local data") already something which we want to protect from being
> read by others? 

No, since they can read the committed versions. 

At the same time, does automate provide such a function? Should it?

The help string for 'get_file' says:

  Prints the contents of a file (given an identifier).

I _assume_ that means a committed file, read from the database, but
it's not clear.

> (Sure, it might not make sense in first place to manage a workspace
> via stdio over a network, but what do I know about weird use cases
> of our users?!)

yes, we cannot assume what use cases won't be used. But we _can_
document the authentication behavior of 'automate stdio', and let the
users assume responsibility for using it appropriately.

>>> +function hook_wrapper(func_name, ...)
>>> +    -- evaluate each single string argument to resolve types
>>> +    -- like nil's, table's and others - the select('#', ...) syntax is
>>> +    -- borrowed from http://lua-users.org/wiki/StoringNilsInTables to
>>> +    -- let this code properly work for nil arguments as well
>>> +    local args = {n=select('#',...), ...}
>>> +    for i=2,args.n do
>>> +        args[i] = assert(loadstring("return " .. args[i]))()
>> 
>> It might be good to specify a more informative message in `assert' here.
>> I think this is where syntax and "not defined" problems in the
>> arguments will be detected, and a generic "assertion failed" will not
>> be much help. Printing out the actual argument that caused the failure
>> might be enough.
>> 
>> In the tests, include some that have erroneous arguments, to show what
>> kind of error messages you get.
>
> Thanks for the pointer, I found a problem with the argument evaluation
> code (apparently this whole part was skipped due to a stupid bug of
> mine), so this has been fixed and wrong arguments are now messaged to
> the user:
>
> $ ./mtn au lua echo 3 "{[1]=;}"
> mtn: warning: [string "<std hooks>"]:1195: argument {[1]=;} could not be
> evaluated
> mtn: misuse: lua call 'echo' failed

This is good.

> There is another special failure: String arguments need to be given with
> extra quotes, so
>
> $ ./mtn au lua echo "'foo'" or
> $ ./mtn au lua echo '"foo"'
>
> works, while a simple
>
> $ ./mtn au lua echo "foo" or
> $ ./mtn au lua echo 'foo'
>
> does not, since "foo" is evaluated as unknown global variable in these
> cases and thus gets "silently" set to nil.

Ah. That's a pain, but at least is has a better work-around than
"double all quotes"!

> This is handled differently:
>
> mtn: warning: [string "<std hooks>"]:1197: argument foo was evaluated to nil
> mtn: misuse: lua call 'echo' failed

This is good.

-- 
-- Stephe