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.

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

x/crypto/ssh: add BannerError #64962

Closed
awly opened this issue Jan 4, 2024 · 15 comments
Closed

x/crypto/ssh: add BannerError #64962

awly opened this issue Jan 4, 2024 · 15 comments
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@awly
Copy link
Contributor

awly commented Jan 4, 2024

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
Copy link
Member

drakkan commented Jan 6, 2024

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
Copy link
Contributor Author

awly commented Jan 8, 2024

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
Copy link
Contributor Author

awly commented Jan 8, 2024

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

@FiloSottile FiloSottile added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jan 9, 2024
@hanwen
Copy link
Contributor

hanwen commented Jan 12, 2024

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
Copy link
Member

drakkan commented Jan 14, 2024

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
Copy link
Contributor Author

awly commented Jan 25, 2024

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
Copy link
Contributor Author

awly commented Jan 26, 2024

@drakkan
Copy link
Member

drakkan commented Jan 27, 2024

@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

@ianlancetaylor ianlancetaylor changed the title proposal: x/crypto/ssh: add ConnMetaData.SendAuthBanner proposal: x/crypto/ssh: add BannerError Feb 6, 2024
@ianlancetaylor
Copy link
Contributor

CC @golang/security

@rsc
Copy link
Contributor

rsc commented Apr 4, 2024

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

@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

What is the specific, complete proposal? Is it only the things listed in #64962 (comment)?

@FiloSottile
Copy link
Contributor

FiloSottile commented Apr 10, 2024

I believe it's just

// 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
}

func (b *BannerError) Unwrap() error
func (b *BannerError) Error() string

from #64962 (comment) and https://golang.org/cl/558695 (although I made the Unwrap and Error receivers pointers.

@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is in #64962 (comment).

@rsc
Copy link
Contributor

rsc commented May 8, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is in #64962 (comment).

@rsc rsc changed the title proposal: x/crypto/ssh: add BannerError x/crypto/ssh: add BannerError May 8, 2024
@rsc rsc modified the milestones: Proposal, Backlog May 8, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/558695 mentions this issue: ssh: allow server auth callbacks to send additional banners

@dmitshur dmitshur added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label May 21, 2024
@dmitshur dmitshur removed this from the Backlog milestone May 21, 2024
@dmitshur dmitshur added this to the Unreleased milestone May 21, 2024
gopherbot pushed a commit to golang/crypto that referenced this issue May 22, 2024
Add a new BannerError error type that auth callbacks can return to send
banner to the client. While the BannerCallback can send the initial
banner message, auth callbacks might want to communicate more
information to the client to help them diagnose failures.

Updates golang/go#64962

Change-Id: I97a26480ff4064b95a0a26042b0a5e19737cfb62
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/558695
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Nicola Murino <nicola.murino@gmail.com>
Auto-Submit: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
@awly awly closed this as completed May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Accepted
Development

No branches or pull requests

8 participants