[Monotone-devel] Re: nvm.ssh-agent review

[Justin Patrin]

On 2/11/07, Zack Weinberg <address@hidden> wrote:
> I read through the new files - ssh_agent.hh, ssh_agent.cc, cmd_agent.cc.
>
> My biggest concern has to do with the way you are serializing and
> deserializing packets -- I think you're working way, way too hard.
> You have a stream interface, and the packet format seems geared to
> serial reading -- why do you read the entire packet in one go and then
> chop it up with string manipulations?  On the write side, you do need
> to construct the packet in memory so you can set its length fields,
> but I think stringstreams would be a big help.

Well, the problem is that the packet construction is recursive. You
need the length of the entire key, for example, before inserting it
into the packet data, then the length of all of that data before
inserting it into the packet itself. It's possible that stringstreams
could be better but what is there works now. I'd suggest if we want to
rewrite this we do it after this branch lands.

> A number of other, more minor concerns, in no particular order:
>
> No using-declarations in headers!

Ok, fixed.

> Brace style is wrong in a few places - don't put the open squiggle on
> the same line as the if

Yep, I'd noticed that. I think I got the last few places this was, let
me know where any further are if you find them.

> Please make sure to wrap all lines before 80 columns.

Done in most places.

> Documentation of protocol could be improved. Did you check whether the
> u32 elements are always big-endian?

I've added that.

> Construction of ssh_agent object connects to the agent, and
> destruction disconnects.  Why are there separate connect() and
> disconnect() methods?  Why is the constructor ignoring the return
> value from connect()?

connect/disconnect moved into constructor/destructor.

> Why are you using a boolean to indicate errors
> instead of throwing an exception?  (For only some errors?!)

I've changed the connect method to L() if the env var doesn't exist
and W() if the connrection fails.

> Bare use of sockets API in connect() - can't netio do this for you?

Netxx doesn't do unix sockets, no. It should probably be moved to
unix.cc at some point.

> Should have a comment on the fcntl operation, so readers don't have to
>  look up what F_SETFD/1 means.  Should use FD_CLOEXEC instead of 1.

Ok.

> Consider use of widen<> in e.g. get_long.

Hmmm, widen<> seems to convert a single char to a wider format. How
would I use it to widen 4 chars to a long?

> Users don't know what "PKCS8 PEM format" means.  It's fine to mention
> it in the manual, but the short usage message should probably not.
> Suggest "export your monotone key in a format that ssh-add
> understands"

Ok.

> I don't see why we need cmd_agent.cc.  Why not put the agent_export
> function in ssh_agent.cc and the command function in cmd_key_cert.cc?

Done.

> People can have more than one key - ssh_agent_export should accept the
> --key option.  (I believe it will Just Work.)

Yep, it does.

> Is there a real reason to make users give a new password for use of
> their key with ssh-agent?  Making people remember different passwords
> for the same key in different contexts is Bad.  Why not just decrypt
> the key, encrypt it again with the same passphrase in the new format,
> and spit it out?

Well, people can enter the same password if they want. We're subtly
suggesting that people use a new password here because previously they
likely had their password in ~/.monotone/monotonerc in plaintext on
the disk.

> Text describing the --ssh-sign option belongs elsewhere in the manual,
> probably next to the discussion of get_passphrase hooks.

Done.

Thanks for the thorough review. I think that nvm.ssh-agent is ready to
land in mainline. :-)

--
Justin Patrin