Re: [Monotone-devel] review of nvm.automate_out_of_band

[Thomas Keller]

Am 22.11.09 01:09, schrieb Timothy Brownawell:
> Thomas Keller wrote:
>> Hi there!
>>
>> Was began as Christof's work at the last summit more than a year ago
>> should finally be feature / documentation / test complete: out-of-band
>> messages like warnings / errors / tickers in stdio (and remote_stdio).
>> On top of that I slightly reworked the error codes for stdio commands as
>> described in NEWS and did a couple of smaller fixes on my way.
>>
>> There are one or two FIXMEs still left in the code where I tend to
>> document the missing functionality / behaviour as known problem, f.e.
>> when executing a command remotely which supports tickers (the only one
>> to date with which this should be possible right now is `bandtest' which
>> is hidden anyways, but when nvm.automate-netsync lands there are a
>> couple of commands more).
>>
>> My next task is to bring nvm.automate-netsync to life again, where the
>> ticker stuff was one of the missings pieces which needed to be done
>> beforehand. I'm still open for discussions / brainstorming in this area
>> and also f.e. automate cert, how we can gracefully handle
>> password-encrypted private keys without trying to prompt, which would be
>> particularily bad over stdio.
> 
> Especially remote_stdio, since our network connections aren't encrypted.

Ah, I haven't thought of that, good catch. We don't really want to allow
the decryption of a private key from a remote connection anyways, I guess.

> I guess one possibility for local stdio would be to allow the password
> to be given in an --option, and return a particular error if it's
> invalid or needed and not given.

As an --option on stdio startup or for a particular command? How would
we prevent that this option is in the latter case then given for a
command which is run remotely?


> For remote_stdio... I guess have a special check to blacklist that
> option until we get encrypted transport, possibly unless some
> --i-dont-care-about-security option is given at startup time? I guess
> another possibility would be a command that actually generates the cert
> on the client, and then looks like read_packets over the wire (so, it'd
> still have to be a special case in the remote_stdio loop :-/ ).

I think my task is two-step: Firstly prevent that the password prompt
happens at all over stdio and breaks the whole process and secondly give
stdio some easy way to specify passwords for key decryption. If encoding
the password into stdio is not an option security-wise, we might already
have everything in place for that then - just give the command an
additional --rcfile with a get_password() lua hook.

>> My ideas so far in this area are rather hacky, as the code which prompts
>> for the phrases and decrypts keys should probably get refactored
>> completly anyways, since decrypting a key once is not enough as the key
>> store (and its internal key cache) are not shared between different
>> key_store instances.
>>
>> Please tell me what you think.
> 
> Since the more/last indicator is being overloaded as a stream
> identifier, probably the last packet should always be empty so that
> consumers don't have to have a special case for one stream type that
> uses multiple identifiers. I suppose then 'm' would mean the 'main'
> output stream rather than meaning there's 'more' output.

I didn't touch that because theoretically the new stream format should
be compatible with old clients which simply ignore the new streams, but
it certainly makes sense.

> For remote_stdio, the more/last indicator was being encoded as 1/0. Now
> it's whatever character was chosen. Looks like remote_stdio hasn't been
> in a release yet, so this should be fine.

Yes, thats why I want to have this in trunk as soon as possible, because
the remote stuff is already in there.

> I think it's probably fine to only have an error code set on the 'l'
> packet. That was previously used to distinguish error messages from the
> intended output as well as being the exit status,

Really? I thought it was simply set as soon as an error popped up...

> but now that there are
> stream indicators to distinguish between normal output and messages the
> error code can be for only an exit status and doesn't need to be set
> until the end. But this should probably be consistent across the
> different ways we can get those packets, rather than only for ones sent
> over the network.

I'll work on that.

> ...hm, how much do we care about keeping stdio output compatible? Right
> now the error indicator distinguishes between normal output and error
> messages and doubles as the return code, and the more/last indicator is
> just an EOF marker. So the most compatible thing might be to leave the
> more/last indicator alone, and indicate alternate streams by setting the
> 'error' field to something silly, like 128 and over (and then just set
> the actual exit code for the last (empty) packet). But then if frontends
> actually error out on unknown interface_version's this doesn't really
> matter.

I don't think its actually feasible to have multiple meanings of the
error indicator within the output of a single call (after all, the
program either executes successfully or fails with an specific code). If
we don't care about breaking stdio, we could catch two flies: Make the
'l' stream really the last thing every command issues (without any more
payload) and encode the return code of the command as payload of this
stream. This would then also mean that we could remove the individual
error indicator from all other streams, i.e.

        l5:headse

would lead to

        0:m:41:503c40cda521bbba7dd971110d50b265681979cb
        0:l:1:0

instead of

        0:0:l:41:503c40cda521bbba7dd971110d50b265681979cb

> The comment above CMD_AUTOMATE_HIDDEN(bandtest, ...) identifies it as
> "interface_version".

Woops, sorry :)

Thomas.

-- 
GPG-Key 0x160D1092 | address@hidden | http://thomaskeller.biz
Please note that according to the EU law on data retention, information
on every electronic information exchange might be retained for a period
of six months or longer: http://www.vorratsdatenspeicherung.de/?lang=en

signature.asc
Description: OpenPGP digital signature