x/crypto/ssh: add BannerError

[awly]

Note: update proposal is #64962 (comment)

---

Proposal Details

According to https://datatracker.ietf.org/doc/html/rfc4252#section-5.4:

The SSH server may send an SSH_MSG_USERAUTH_BANNER message at any
time after this authentication protocol starts and before
authentication is successful.

Currently, x/crypto/ssh allows servers to send a banner before authentication begins. I propose to add:

type ConnMetadata interface {
	...

	// SendAuthBanner sends a banner to the client. This is useful for sending
	// messages during authentication. It is only valid to call this before
	// authentication succeeds.
	SendAuthBanner(banner string) error

This is useful for sending dynamic banner messages to clients from auth callbacks.
For example, a server can send a link to perform out-of-band authentication of the user if e.g. public key authentication fails. See for example the Check mode in Tailscale SSH: https://tailscale.com/kb/1193/tailscale-ssh#configure-tailscale-ssh-with-check-mode

cc @bradfitz @maisem

[drakkan]

Hello, I like this idea, thank you for the proposal!

What's wrong with something like this?

diff --git a/ssh/server.go b/ssh/server.go
index 1b7d0a9..ed73736 100644
--- a/ssh/server.go
+++ b/ssh/server.go
@@ -473,6 +473,21 @@ func (p *PartialSuccessError) Is(target error) bool {
        return ok
 }
 
+// BannerError is an error wrapper that can be returned from an authentication
+// callback to send a banner message to the client.
+type BannerError struct {
+       Err     error
+       Message string
+}
+
+func (e *BannerError) Unwrap() error {
+       return e.Err
+}
+
+func (e *BannerError) Error() string {
+       return e.Err.Error()
+}
+
 // ErrNoAuth is the error value returned if no
 // authentication method has been passed yet. This happens as a normal
 // part of the authentication loop, since the client first tries
@@ -756,6 +771,14 @@ userAuthLoop:
                        break userAuthLoop
                }
 
+               var bannerError *BannerError
+               if errors.As(authErr, &bannerError) {
+                       bannerMsg := &userAuthBannerMsg{Message: bannerError.Message}
+                       if err := s.transport.writePacket(Marshal(bannerMsg)); err != nil {
+                               return nil, err
+                       }
+               }
+
                var failureMsg userAuthFailureMsg
 
                var partialSuccess *PartialSuccessError

I see that you initially implemented the feature in this way in your branch.

If we want to extend ConnMetadata we should do it in a backward compatible way. Something like this:

type ConnMetadataBanner interface {
	ConnMetadata

	// SendAuthBanner sends a banner to the client. This is useful for sending
	// messages during authentication. It is only valid to call this before
	// authentication succeeds.
	SendAuthBanner(banner string) error
}

[awly]

We could even combine it with https://go-review.googlesource.com/c/crypto/+/516355 by adding a BannerCallback (or just a Banner string field) into PartialSuccessError.
The limitation is that you can only send 1 banner per callback, and it cannot also be a successful authn return.

You can see https://github.com/tailscale/tailscale/pull/5883/files for why the initial WithBannerError was removed.

And good point about backwards-compatibility of ConnMetadata, we can make SendAuthBanner accessible via a type assertion. It can be a separate type AuthBannerSender interface too.

[awly]

Here is the proposed implementation, just to facilitate discussion: https://go-review.googlesource.com/c/crypto/+/554775

[hanwen]

as a comment to the original proposal: note that ConnMetadata only has functions that have no side effects. It's also shared between client and server, so server-only functionality should not be added there.

[drakkan]

I agree with @hanwen

I think the use cases are:

1. sending an initial banner
2. sending a banner after an authentication error
3. sending a banner after successful authentication and before SSH_MSG_USERAUTH_FAILURE

We already have BannerCallback for the initial banner.

We may add something like this

type ServerConfig struct {
    ....
    // AuthBannerCallback, if present, is called and the return string is sent
   // to the client after a successfull authentication before
   // SSH_MSG_USERAUTH_SUCCESS.
   AuthBannerCallback func(conn ConnMetadata) string
}

// BannerError is an error wrapper that can be returned from an authentication
// callback to send a banner message to the client.
type BannerError struct {
	Err     error
	Message string
}

This way it is also clearer when the banner is sent (the server calls tha callback) and we don't have to check that we are still in the authentication phase.

[awly]

Sorry for the delay.
@drakkan @hanwen yes, adding just BannerError would be sufficient for our current needs.
It seems compatible with https://go-review.googlesource.com/c/crypto/+/516355/6/ssh/server.go too, I can wrap BannerError around PartialSuccessError.

I can send a new CL tomorrow 👍

[awly]

Here we go: https://go-review.googlesource.com/c/crypto/+/558695

[drakkan]

@awly thank you. Your patch looks good to me but we need this proposal approved first.
So if a banner after successful authentication is not required for your use case (we can always add it later if there is a real use case), the proposal is this one

// BannerError is an error that can be returned by authentication handlers in
// ServerConfig to send a banner message to the client.
type BannerError struct {
	Err     error
	Message string
}

cc @golang/proposal-review