Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proposal: x/crypto/ssh: add deadlines support for channels #65930

Open
drakkan opened this issue Feb 25, 2024 · 5 comments
Open

proposal: x/crypto/ssh: add deadlines support for channels #65930

drakkan opened this issue Feb 25, 2024 · 5 comments
Labels
Proposal
Milestone
Proposal

Comments

@drakkan
Copy link
Member

drakkan commented Feb 25, 2024

Proposal Details

Currently ssh channels does not support deadlines, so the only way to unblock reads/writes is to set a deadline on the underlying net.Conn, but this will affect all channels using the connection.

Channels are typically blocked on reads while waiting for data and on writes while waiting for window capacity.

I propose adding deadlines to channels to fix these typically use cases, I don't think we can unblock reads/writes blocked on the underlying net.Conn.

Proposed API

// ChannelWithDeadlines is a channel with deadlines support.
type ChannelWithDeadlines interface {
	Channel

	// SetDeadline sets the read and write deadlines associated with the
	// channel. It is equivalent to calling both SetReadDeadline and
	// SetWriteDeadline. Deadlines errors are not fatal, the Channel can be used
	// again after resetting the deadlines.
	SetDeadline(deadline time.Time) error

	// SetReadDeadline sets the deadline for future Read calls and unblock Read
	// calls waiting for data. A zero value for t means Read will not time out.
	SetReadDeadline(deadline time.Time) error

	// SetWriteDeadline sets the deadline for future Write calls and unblock
	// Write calls waiting for window capacity. A zero value for t means Write
	// will not time out.
	SetWriteDeadline(deadline time.Time) error
}

cc @golang/proposal-review
@gopherbot gopherbot added this to the Proposal milestone Feb 25, 2024
@gopherbot
Copy link

Change https://go.dev/cl/562756 mentions this issue: ssh: add deadlines support for channels

@ianlancetaylor
Copy link
Contributor

CC @golang/security

@hdm
Copy link

hdm commented Apr 8, 2024

The handshake (version, kex, etc) can stall unless the underlying Conn is closed. I would love to see deadlines on channels, but it may make sense to have a general read/write timeout capability, otherwise we still need the conn.Close() to unblock when communicating with misbehaving peers.

@espadolini
Copy link

espadolini commented Apr 8, 2024 •
edited

but it may make sense to have a general read/write timeout capability

@hdm isn't that just Set(Read|Write)Deadline on the underlying connection?

@hdm
Copy link

hdm commented Apr 8, 2024 •
edited

but it may make sense to have a general read/write timeout capability

@hdm isn't that just Set(Read|Write)Deadline on the underlying connection?

The challenge is that the deadlines need to be reset on each read/write and setting it at the connection level doesn't allow re-extension of the deadline by operation. For an example, with a long-lived session, you may want want a relatively short deadline for version exchange and kex, but then to re-extend it after each input line is passed in via the stdin reader.

I get your point though, if a deadline on the Conn is enough to get us into the session, then a Channel deadline could take over from there for most use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
5 participants
@drakkan @espadolini @hdm @ianlancetaylor @gopherbot