proposal: x/crypto/ssh: add ServerConfig.PreAuthConnCallback, ServerPreAuthConn (banner) interface

[awly]

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 #64962 (comment) 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]

Related Issues and Documentation

• x/crypto/ssh: add BannerError #64962 (closed)

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

[ianlancetaylor]

CC @golang/security

[gopherbot]

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

[gopherbot]

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

[drakkan]

I would like to discuss also the proposal implemented by @bradfitz in CL 613856.

Here is the proposal, I just change the name of the callback to make it more similar to the existing ones:

// ServerPreAuthConn is the interface available on an incoming server
// connection before authentication has completed.
type ServerPreAuthConn interface {
	unexportedMethodForFutureProofing() // permits growing ServerPreAuthConn safely later, ala testing.TB

	ConnMetadata

	// SendAuthBanner sends a banner message to the client.
	// It returns an error once the authentication phase has ended.
	SendAuthBanner(string) error
}

// ServerConfig holds server specific configuration data.
type ServerConfig struct {
     .....
     // PreAuthConnCallback, if non-nil, is called upon receiving a new connection
     // before any authentication has started. The provided ServerPreAuthConn
     // can be used before authentication is complete.
     PreAuthConnCallback func(ServerPreAuthConn)
}

The main difference with the other proposal is that here we add interface extension of the ConnMetadata that allows to send banners and we can obtain it by adding a PreAuthConnCallback to ServerConfig.

The other approach allows to send banners using the ConnMetadata interface received in the authentication callbacks.

PreAuthConnCallback seems more flexible, in the future we may consider reusing it to expose other methods or properties as well

[oxtoacart]

@FiloSottile @drakkan Is there anything I can do to help move this forward?

[bradfitz]

I dusted off my CL above, did the rename, and added some tests: https://go-review.googlesource.com/c/crypto/+/613856

[bradfitz]

@FiloSottile, @drakkan, https://go-review.googlesource.com/c/crypto/+/613856 contains the latest proposal (with @drakkan's renames from above) and the approved implementation, on Hold for approval here.

When does Crypto Proposal Committee meet? :)

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[aclements]

The original post says "the SSH spec allows banners to be sent at any point in the connection until authentication is complete", which suggests to me that this can be done totally asynchronously by the server, but maybe I misunderstood. In @drakkan 's API, are users allowed to hold on to the ServerPreAuthConn object and call SendAuthBanner after the callback returns? If so, is there a danger is racing with authentication completing?

[drakkan]

@aclements you are right, the initial version of the CL actually had a race and this is a fatal error, see the comment I left in the linked CL. I think patchset 3 is fine

[aclements]

It seems a bit unfortunate to have three ways to do this, but so it goes. Do we want to deprecate BannerError?

The proposed API seems fine.

@aclements you are right, the initial version of the CL actually had a race and this is a fatal error, see the comment I left in the linked CL. I think patchset 3 is fine

Thanks! I think the documentation should be a bit more clear about this.

[aclements]

Have all remaining concerns about this proposal been addressed?

The proposal is to add the following interface type and configurable callback to golang.org/x/crypto/ssh

// ServerPreAuthConn is the interface available on an incoming server
// connection before authentication has completed.
type ServerPreAuthConn interface {
	unexportedMethodForFutureProofing() // permits growing ServerPreAuthConn safely later, ala testing.TB

	ConnMetadata

	// SendAuthBanner sends a banner message to the client.
	// It returns an error if the authentication phase has ended.
	SendAuthBanner(string) error
}

type ServerConfig struct {
	...

	// PreAuthConnCallback, if non-nil, is called upon receiving a new connection
	// before any authentication has started. The provided ServerPreAuthConn
	// can be used at any time before authentication is complete, including
	// after this callback has returned.
	PreAuthConnCallback func(ServerPreAuthConn)

	...
}

[drakkan]

@aclements thanks for improving the doc, it's clearer now.

I agree that it's unfortunate that we added BannerError few months ago and this callback is a better alternative, but I think these two APIs can coexist without the need for deprecation until we can create a v2

[aclements]

Based on the discussion above, this proposal seems like a likely accept.

The proposal is to add the following interface type and configurable callback to golang.org/x/crypto/ssh

// ServerPreAuthConn is the interface available on an incoming server
// connection before authentication has completed.
type ServerPreAuthConn interface {
	unexportedMethodForFutureProofing() // permits growing ServerPreAuthConn safely later, ala testing.TB

	ConnMetadata

	// SendAuthBanner sends a banner message to the client.
	// It returns an error if the authentication phase has ended.
	SendAuthBanner(string) error
}

type ServerConfig struct {
	...

	// PreAuthConnCallback, if non-nil, is called upon receiving a new connection
	// before any authentication has started. The provided ServerPreAuthConn
	// can be used at any time before authentication is complete, including
	// after this callback has returned.
	PreAuthConnCallback func(ServerPreAuthConn)

	...
}