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

proposal: x/crypto/ssh: add BannerError #64962

Open
awly opened this issue Jan 4, 2024 · 13 comments
Open

proposal: x/crypto/ssh: add BannerError #64962

awly opened this issue Jan 4, 2024 · 13 comments
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-FinalCommentPeriod
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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-FinalCommentPeriod
Projects
Status: Likely Accept
Development

No branches or pull requests

7 participants