golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.17k stars 17.56k forks source link

proposal: x/crypto/ssh: add BannerSender interface #68688

Open awly opened 1 month ago

awly commented 1 month ago

Proposal Details

SSH server has 2 methods of sending banners (SSH_MSG_USERAUTH_BANNER) back to the client:

  1. BannerCallback, which runs before any auth handlers
  2. BannerError return, which can be returned from any auth handler

However, the SSH spec allows banners to be sent at any point in the connection until authentication is complete, not bound to auth attempts. While we could add a new method on ssh.ConnMetadata (which is passed to auth handlers) or ssh.Conn (which can be type-asserted from ssh.ConnMetadata), this would break backwards-compatibility for custom implementations of those interfaces.

I propose we add a new single-purpose interface:

// BannerSender sends banner messages from the server to the client. Banners
// can only be sent before authentication is complete, from callbacks in
// ServerConfig. Calls to SendBanner after authentication always fail. Callers
// can access a BannerSender using a checked type assertion on the ConnMetadata
// value passed to the callbacks in ServerConfig. 
type BannerSender interface {
    SendBanner(message string) error
}

This new method would be implemented on the unexported *x/crypto/ssh.connection type, which is passed as ConnMetadata in authentication handlers. This is not very discoverable, but is the least disruptive API change I could think of.

In https://github.com/golang/go/issues/64962#issuecomment-1909172085 I claimed that this was sufficient for Tailscale's use case, but turns out it was not, that's my bad. For example, a server can print a custom prompt or instruction to the user while an authentication attempt is pending, which is required for the user to finish that attempt.

cc @drakkan @oxtoacart @bradfitz

gabyhelp commented 1 month ago

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

ianlancetaylor commented 1 month ago

CC @golang/security

gopherbot commented 1 week ago

Change https://go.dev/cl/613856 mentions this issue: ssh: add ServerConfig.GetPreAuthConn, ServerPreAuthConn (banner) interface

gopherbot commented 1 week ago

Change https://go.dev/cl/614416 mentions this issue: ssh: add BannerSender interface