fix: unknown request types should return failure

[cquintana92]

Fix: Reply SSH_AGENT_FAILURE to unknown request types instead of closing the connection

Issue

When a client sends a request whose message type is not recognised, the agent closes the connection without replying. Per draft-miller-ssh-agent-14 § 3.1:

SSH_AGENT_FAILURE messages are also sent in reply to requests with unknown types.

OpenSSH's ssh-agent behaves that way, but agents built on this crate currently do not, because the connection is dropped at the codec layer before any session handling runs.

Impact

Some clients probe the agent with a legacy request type that is not implemented by modern agents. For example Ruby's net-ssh opens negotiation with SSH2_AGENT_REQUEST_VERSION (message type 1). When the agent closes the socket instead of replying, net-ssh raises a FrozenError and deploy tools built on it cannot connect at all, even though ssh/ssh-add work fine against the same agent.

Root cause

The framing Codec decodes every incoming frame into a Request before the Session is invoked. For an unknown message type, Request::decode returned Err(UnsupportedCommand). That decoder error is fatal to the stream: tokio_util's FramedImpl enters an errored state after a decode error and yields EOF on the next poll, so the socket is torn down before Session::handle is ever called. No Session override can prevent this with the current code.

Proposed solution

• Adding a Request::Unknown(u8) variant that captures the raw message type byte. Unknown message types now decode successfully (the payload following the type byte is skipped, bounded by themessage length prefix), encode back to the original type byte, and report the correct message_id.
• In the default Session::handle implementation, respond to Request::Unknown with Response::Failure (i.e. SSH_AGENT_FAILURE, message type 5) and keep the connection open, matching OpenSSH's behaviour.

The default implementation suits all existing agents; because handle is the documented override point, users who need custom handling for a particular unknown type can still intercept it themselves.

Testing

New integration test tests/unknown_request.rs starts a real agent over a Unix socket, replays the exact net-ssh probe (SSH2_AGENT_REQUEST_VERSION, type 1, body "2.0"), and asserts that:

1. The agent replies with SSH_AGENT_FAILURE (message type 5) instead of closing the connection.
2. The connection remains usable for a subsequent supported request (SSH_AGENTC_REQUEST_IDENTITIES).

The test fails against the previous behaviour (connection closed without a reply) and passes with this change.

Validation

• cargo test reports all suites pass (unit, roundtrip, doc, new integration test)
• cargo clippy --workspace --no-deps --all-targets -- -D warnings comes clean
• cargo fmt --check comes clean

Compatibility

This is a backward-compatible additive change: Request gains one new variant (the compiler will flag exhaustive matches, which is desirable since callers must now decide how to handle unknown messages). No existing behaviour changes.

[jcspencer]

Sorry for the delay in reviewing this one @cquintana92 - thanks for this!

I think this is a good addition (and helps with spec compliance) - however, in terms of public facing API surface, I think it'd be worth extending the Session trait to implement a handler for unknown messages.

We have a construct of Unparsed for the body of extension-related messages already, so passing this over to that handler would be nice as well.

I've put together #114, which builds on this PR and:

• exposes an unknown_message() method in the Session trait
  • by default, this will simply return Response::Failure, but could be used to implement "unknown" messages if users have a need to do that.
• I've also refactored the test case into a standard round-trip test case to keep things consistent; this now checks that we capture the net-ssh message as Unknown, but retain the body 😄

Let me know if this works for you!

[cquintana92]

@jcspencer made the changes and also adapted the tests so they pass on Windows, let me know if this is enough. Also there is a failing job regarding a yanked dep, I'll leave that one to the project maintainers 👍

[wiktor-k]

Hi @cquintana92, thanks for the PR, it looks really nice 👌

This is a backward-compatible additive change

Sadly Request wasn't marked with #[non_exhaustive] so we'd need to bump the version number, but it's definitely not a blocker.

I've updated the deps in #115 if you could rebase this on top of main it'd be really great, thanks! 👋

[cquintana92]

@wiktor-k rebased 👍

[wiktor-k]

@cquintana92 thanks!

@jcspencer is that okay with you if we merge this one and then continue on with #114 ?

[jcspencer]

@wiktor-k sure thing! I’ll rebase once this is merged 🎉