SSH agent client support

[jcspencer]

Given the work occurring in #26 (with encode/decode support now supported for the entire spec), the project is now relatively close to being able to implement support for acting as an SSH agent client.

I imagine this would entail:

• Splitting MessageCodec into it's own pub(crate) module, as it would be shared between the agent and client.
• Implementing some kind of generic ClientSocket trait to support Unix Sockets / TCP / Named Pipes as transports.
• Implementing a Client trait which provides helper methods for sending messages over the a ClientSocket.

It's probably worth noting that from a client perspective, one connection == one session, so the Agent/Session-style split would not be required.

I'm also unsure whether the interface needs to be async, or whether a synchronous interface would be sufficient?

Interested to hear your thoughts!

---

A sample trait for a client could look something like:

pub trait Client {
    fn request_identities(&self) -> Result<Message, AgentError> {
        let message = Message::RequestIdentities;
        todo!()
    }

    fn sign(
        &self,
        public_key: KeyData,
        data: Vec<u8>,
        flags: u32
    ) -> Result<Message, AgentError> {
        let message = Message::SignRequest(SignRequest {
            pubkey: public_key,
            data,
            flags
        });
        todo!()
    }

    fn add_identity(
        &self,
        private_key: KeypairData,
        comment: String,
        constraints: Option<Vec<KeyConstraint>>
    ) -> Result<Message, AgentError> {
        let identity = AddIdentity {
           privkey: private_key,
           comment 
        };

        let message = match constraints {
            Some(constraints) => {
                Message::AddIdConstrained(AddIdentityConstrained {
                    identity,
                    constraints
                })
            },
            None => {
                Message::AddIdentity(identity)
            }
        };
        todo!()
    }

    fn remove_identity(&self, public_key: KeyData) -> Result<Message, AgentError> {
        let message = Message::RemoveIdentity(RemoveIdentity {
            pubkey: public_key
        });
        todo!()
    }

    fn remove_all_identities(&self) -> Result<Message, AgentError> {
        let message = Message::RemoveAllIdentities;
        todo!()   
    }

    fn add_smartcard_key(
        &self,
        id: String,
        pin: String,
        constraints: Option<Vec<KeyConstraint>>
    ) -> Result<Message, AgentError> {
        let key = SmartcardKey { id, pin };
        let message = match constraints {
            Some(constraints) => {
                Message::AddSmartcardKeyConstrained(AddSmartcardKeyConstrained {
                    key,
                    constraints
                })
            },
            None => {
                Message::AddSmartcardKey(key)
            }
        };
        todo!()
    }

    fn remove_smartcard_key(&self, id: String, pin: String) -> Result<Message, AgentError> {
        let key = SmartcardKey { id, pin };
        let message = Message::RemoveSmartcardKey(key);
        todo!()
    }

    fn lock(&self, passphrase: Passphrase) -> Result<Message, AgentError> {
        let message = Message::Lock(passphrase);
        todo!()
    }
    
    fn unlock(&self, passphrase: Passphrase) -> Result<Message, AgentError> {
        let message = Message::Unlock(passphrase);
        todo!()
    }


    fn extension<T>(&self, name: String, data: T) -> Result<Message, AgentError>
    where
        T: Encode
    {
        let mut buffer: Vec<u8> = vec![];
        data.encode(&mut buffer)?;

        let extention = Extension {
            name,
            details: Unparsed(buffer)
        };

        let message = Message::Extension(extension);
        todo!()
    }
}

[wiktor-k]

I think it's a great idea and we should do that.

A couple of notes before we start working on this:

1. I've been thinking about splitting Message into Request and Response since then we can make some basic checks in the type system (that we're not sending Requests as responses etc.)

2. I'd want to migrate Session trait to use the same style you used in your Client trait: just direct functions that correspond to Messages (Requests). Then we can have nice parameters (just like you have) and nice returns (say, signing can just return a Signature).

3. When 2. is implemented I'd make the Message just a low-level construct, just in case someone wants more flexibility. A bit of the abstraction would still leak in case of extensions but this is OK.

As for async I don't have an opinion. We can keep it simple and sync for now (this could be useful for some people that don't want to bring tokio?).

I agree with everything else you've said 👍

[baloo]

There might be a need for async later on, but we can always introduce that when it comes up.

[jcspencer]

It’s probably worth noting too, I’ve been in touch with Damien Miller (the author of the agent protocol spec), and he’s been gracious enough to rework extension responses in the spec; there should be a new revision out soon.

This will add an ExtensionResponse message type which wraps an Extension object, and also updates the built-in query extension response in this new data type. This should hopefully simplify extension development - saving us from having to do too much in the way of exposing internals.

I think this addresses the concerns around trailing data in a Success message that @baloo alluded to in #16 as well!

[wiktor-k]

It’s probably worth noting too, I’ve been in touch with Damien Miller (the author of the agent protocol spec), and he’s been gracious enough to rework extension responses in the spec; there should be a new revision out soon.

Oh, that's great! I think I should send a patch with the minor spec thing we've found when working on key extension constraints.

This will add an ExtensionResponse message type which wraps an Extension object, and also updates the built-in query extension response in this new data type. This should hopefully simplify extension development - saving us from having to do too much in the way of exposing internals.

That's always very welcome 🙏

I think this addresses the concerns around trailing data in a Success message that @baloo alluded to in #16 as well!

Hmm... Just for the record: IMO Success is actually pretty simple, even with added strings (just read strings until the reader is exhausted). The real problems appear when using nested structures (say, keys with unknown extension constraints, or new keytypes). That being said I'm very happy that there's this effort to fix outstanding issues with the spec and the implementation.

I wonder if it would be beneficial to share some test vectors (serialized binary messages, like so: https://github.com/wiktor-k/ssh-agent-lib/tree/main/tests/messages) with the upstream?

Thanks for following up with this thread @jcspencer 👍

[wiktor-k]

🤔 I've refactored the Session a bit and introduced functions similar to what was proposed in the initial comment here: https://github.com/wiktor-k/ssh-agent-lib/pull/42/files#diff-3bab5cd53a1040a4a531afe1501403786256e9dea8731dc20028c9beabddd2d3R83

and that got me thinking that actually Session is the same trait that @jcspencer proposed as the Client trait (modulo the async thing).

That means one could implement Session as a simple forwarder which serializes input messages, send them somewhere and deserializes responses. I'll need to think about the idea some more, especially about the actual underlying I/O but maybe the client abstraction is closer than I initially thought 😅

Aaand... that's it! 👋