[Monotone-devel] nvm.ssh-agent review

[Zack Weinberg]

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.

A number of other, more minor concerns, in no particular order:

No using-declarations in headers!

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

Please make sure to wrap all lines before 80 columns.

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

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()?  Why are you using a boolean to indicate errors
instead of throwing an exception?  (For only some errors?!)

Bare use of sockets API in connect() - can't netio do this for you?
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.

Consider use of widen<> in e.g. get_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"

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?

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

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?

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

zw