Split Message into Request and Response

[wiktor-k]

As it says on the tin.

It makes the type system check if we're not sending requests as replies in the agent and vice versa.

I think some cleanup is still needed (e.g. not using message as a variable name, or duplication in Request::SignRequest) but I'm filing it earlier to gather your feedback :)

[wiktor-k]

Okay folks, I packed a bit more features in here than I wanted but I guess going commit-by-commit shows what's going on.

The end result is having direct functions on the Session trait instead of manually wrapping and unwrapping Messages.

Other change is extracting the Codec. I thought it may be useful for writing clients but after a while I'm not so sure 🤔

And the first change is a Message split into Request and Response. Which should make clients a bit more robust.

[jcspencer]

Hi @wiktor-k, this is a great start!

Personally, I feel @baloo's proposed changes in #43 (using traits instead of enums) may be a little nicer just in terms of extensibility - for example, being able to implement Request or Response traits for custom message types.

On that note, it could be worth implementing some mechanism to handle "unknown" message types in the Session for types that implement Encode + Decode, rather than having to overwrite async fn handle().

@baloo's implementation will return ProtoError::UnsupportedCommand { command }) in the instance where an unsupported message code is hit - exposing this to a client handler to deal with rather than eating it up could be a way to tackle this?

[wiktor-k]

Personally, I feel @baloo's proposed changes in #43 (using traits instead of enums) may be a little nicer just in terms of extensibility - for example, being able to implement Request or Response traits for custom message types.

Yup, definitely it's a step into the right direction. I'm wondering how would one "register" which types to use for which response codes but let's wait until @baloo implements it further and it takes more shape :)

[baloo]

yeah, I don't know how to deal with the router either. This was a good idea until it was not.

I need a piece of code to do the routing between message types.

[wiktor-k]

On that note, it could be worth implementing some mechanism to handle "unknown" message types in the Session for types that implement Encode + Decode, rather than having to overwrite async fn handle().

Hmm... well this could be relatively easily implemented in the enum design with a variant that captures unknown messages and the payload (similar to the Extension details) and a Session function unknown_message. Even though this is simple and OK to add I'm not sure that in the real-world people would use this mechanism.

I've seen people using custom extensions but not entire message types. (I'm not saying it's a bad idea or discourage anyone from using it, just stating my PoV).

One way or another I'm happy to wait with this PR until @baloo emerges with insights from his side-quest :) That said I'm happy that we're discussing design decisions here so at least it's clear what alternatives have been considered. 👍

[jcspencer]

Yeah very fair - with the new EXTENSION_RESPONSE response message about to land in the specification, it does leave the need for custom message types not really necessary - fingers crossed the new RFC draft lands soon.

[wiktor-k]

Yeah very fair - with the new EXTENSION_RESPONSE response message about to land in the specification, it does leave the need for custom message types not really necessary - fingers crossed the new RFC draft lands soon.

Indeed, that's great to hear! 🚀

Is there some venue where one can track the progress or is it in 1:1 e-mails between with you and Damien?

I'd be interested if the Query extension will be migrated to this new EXTENSION_RESPONSE scheme... :)

[jcspencer]

@wiktor-k @baloo v14 is here: djmdjm/drafts@fb5055b

See both the new SSH_AGENT_EXTENSION_RESPONSE message type which can enclose arbitrary data (with an extension string) and the updated query extension which uses the new mechanism (rather than sending a success message with trailing data)

I can make a PR this week to merge into this branch to accommodate for this

[jcspencer]

The new draft has been published: https://www.ietf.org/archive/id/draft-miller-ssh-agent-14.html#section-3.8

The contents of successful extension reply messages are specific to the extension type. Extension requests may return SSH_AGENT_SUCCESS on success or the extension-specific response message:

byte             SSH_AGENT_EXTENSION_RESPONSE
string           extension type
byte[]           extension response-specific contents

For example, the "query" extension looks like:

    byte             SSH_AGENT_EXTENSION_RESPONSE
    string           "query"
    string[]         supported extension types

The response message code for SSH_AGENT_EXTENSION_RESPONSE is 29.

Noting that:

• Extension requests "may return SSH_AGENT_SUCCESS on success or the extension-specific response message"
• However, for example, "[email protected]" requests don't return anything at all (edit: my bad, this actually does return success or failure)

The method signature of async fn extension(&mut self, mut extension: Extension) would need to accommodate four distinct return types to cover all the different uses in the spec / in the wild (that I can think of):

• no response (like [email protected])
• SSH_AGENT_SUCCESS - generic extension success with no body
• SSH_AGENT_EXTENSION_RESPONSE - custom extension response with body
• SSH_AGENT_EXTENSION_FAILURE - generic extension failure

---

One way could be to encapsulate these response types into an enum that can be returned from extension() calls?:

enum ExtensionReponse {
  Success,
  Response(Extension)
}

pub trait Session: 'static + Sync + Send + Sized {
    async fn extension(
        &mut self,
        mut extension: Extension,
    ) -> Result<ExtensionResponse, Box<dyn std::error::Error>> {
    }
}

But very open to suggestions!

[baloo]

The new draft has been published: https://www.ietf.org/archive/id/draft-miller-ssh-agent-14.html#section-3.8

Hey thanks for following up on that!

However, for example, "[email protected]" requests don't return anything at all

How is this not a violation of the protocol? If the non-support of an extension is signaled by agent failure:

An agent that does not support extensions of the supplied type MUST reply with an empty SSH_AGENT_FAILURE message. This reply is also sent by agents that do not support the extension mechanism at all.

How can a client know whether the agent supported the extension or not if no reply is expected this one specific command?! This can't be a specific use-case.

[jcspencer]

No worries @baloo! It definitely helps my use-case where I'm layering a request/response based protocol over the agent protocol - hope it helps some others too!

On the "[email protected]" case; I think it actually does respond; it sends either SSH_AGENT_SUCCESS or SSH_AGENT_FAILURE based on the outcome. I missed the bounce from here to here. So I think that rules out the "empty response" case - my bad!

[baloo]

No worries @baloo! It definitely helps my use-case where I'm layering a request/response based protocol over the agent protocol - hope it helps some others too!

I'm in the exact same boat hey.

[baloo]

How about:

pub trait Session: 'static + Sync + Send + Sized {
    async fn extension(
        &mut self,
        mut extension: Extension,
    ) -> Result<Option<Extension>, Box<dyn std::error::Error>> {
    }
}

Return Ok(None) -> SSH_AGENT_SUCCESS
return Ok(Some(ext)) -> SSH_AGENT_EXTENSION_RESPONSE with the serialized content of ext
return Err(_) -> SSH_AGENT_EXTENSION_FAILURE ?

[baloo]

Just traced an ssh-agent version 9.6p1, and yes it does reply the expected SSH_AGENT_SUCCESS to the [email protected] extension.

[jcspencer]

Wait, [email protected] is never used as the payload of a SSH_AGENTC_EXTENSION

I'm thinking of the non-RFCed version that appears in the OpenSSH built-in agent, where [email protected] is send as an extension. But; either way, I think using an Option<Extension> covers the way they've implemented it.

FWIW, the stock OpenSSH agent doesn't seem to support the "query" extension in the draft RFC, so 🤷

pub trait Session: 'static + Sync + Send + Sized {
    async fn extension(
        &mut self,
        mut extension: Extension,
    ) -> Result<Option<Extension>, Box<dyn std::error::Error>> {
    }
}

return Ok(None) -> SSH_AGENT_SUCCESS
return Ok(Some(ext)) -> SSH_AGENT_EXTENSION_RESPONSE with the serialized content of ext
return Err(_) -> SSH_AGENT_EXTENSION_FAILURE

Looks great to me - and makes it easy to implement arbitrary extensions.

Since we will need to return SSH_AGENT_FAILURE in the case an extension is not supported, we could expose a trait for a user to implement on their error types to map them to one failure message or the other (general failure / extension failure)?

[baloo]

I'm thinking of the non-RFCed version that appears in the OpenSSH built-in agent, where [email protected] is send as an extension. But; either way, I think using an Option<Extension> covers the way they've implemented it.

Yeah, I deleted that comment immediately after posting it, realizing I was mixing things up. Sorry about the noise.

[baloo]

Let's revisit the draft-14 in a followup PR.

We'll merge this and proceed with the license change in #36