Re: [Monotone-devel] key management

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> Am 10.08.2010 01:59, schrieb Stephen Leake:
>> Thomas Keller <address@hidden> writes:
>> 
>>> Am 07.08.2010 20:40, schrieb Stephen Leake:
>>>> Stephen Leake <address@hidden> writes:
>>>>
>>>> I used the command names 'automate pubkey', 'automate dropkey', to match
>>>> the corresponding non-automate commands.
>>>>
>>>> 'automate dropkey' drops the private key if present, as non-automate
>>>> does. I didn't see any reason to change the behavior.
>>>
>>> Ouch - that might not be a good idea. This would e.g. enable the
>>> deletion of the key which is used to authenticate the server, rendering
>>> a running monotone instance completely useless. We cannot even restrict
>>> the execution of this command by argument easily, ie. I don't want to
>>> tell server admins to expand their get_remote_automate_permitted() hook
>>> to specifically exclude the key id for this new command, this is way too
>>> harmful if forgotten.
>>>
>>> So please, either split the functionality in two commands
>>> (drop_public_key / drop_private_key) or disable key deletion over
>>> automate. In the former case we could at least give sensible hints for a
>>> server admin to disallow the drop_private_key command completely.
>> 
>> I changed 'automate dropkey' to 'automate drop_public_key'.
>
> Ok, cool, I had the time to quickly look over the changes, some minor
> things:
>
> 1) Please rename pubkey to get_public_key - we should try to keep the
> command names consistent within automate. And while we're at it, rename
> genkey to generate_key. We'll break BC in automate for 0.99 anyways when
> nvm.options lands, so we can clean up the command naming mess on the
> same run. We still have a slight inconsistency wrt getter commands (most
> of them have a "get_" prefix, most of the tree commands have not
> though), but thats out of scope for this branch.

Ok.

> 2) I've seen this before in your work and now again, so unfortunately I
> have to say something :) - please don't put a space before the opening
> bracket in function declarations and definitions, this violates the
> coding standards.

Sorry. That's my Ada coding standard at work, so my fingers just do it.
I do try to fix it when I notice it.

> 3) Please also implement put_public_key. This way we can safely
> deprecate "automate read_packets" and the whole packet machinery for
> file deltas, revisions, and so on, while still giving the people an
> upgrade path.

Ok.

> 4) The docs on drop_public_key should be tweaked a bit, i.e. right now
> the first sentence in the Purpose section reads as if we're still
> dropping private keys for which it would indeed very harmful to execute
> the command, but we don't do that any longer. Even if we drop the public
> key from the server, we can simply read in the private key to get it
> back or sync it from another node. Furthermore it would be cool to
> document that just as well, i.e. that a public key comes back on the
> next sync and that the main consequence is that signatures can no longer
> be verified for this particular key (point them to the new k: selector
> here as well, so they have a mean to figure out if the key they want to
> drop is actually in use).

Ok.

> 5) Would it be a good idea to rename common/test_utils_inventory.lua to
> something more generic now that this is used a couple of times outside
> the basic inventory tests? Alternatively the common stuff could be moved
> out of this file.

Right.

I'll get to all of this later.

-- 
-- Stephe