From 40ff35d6071aaf8e74c654e124a81bc7f43bf981 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 23 Mar 2022 14:41:31 -0700 Subject: [PATCH 1/5] ssh: add ServerConfig.NoClientAuthCallback It was possible to accept auth type "none" before, but not dynamically at runtime as a function of the ConnMetadata like the other auth types' callback hooks. See https://github.com/golang/go/issues/51994 and https://go-review.googlesource.com/c/crypto/+/395314 Change-Id: I83ea80901d4977d8f78523e3d1e16e0a7df5b172 (cherry picked from commit 4a431fab27b09acb1458fbb8709e12b2760e58a2) --- ssh/server.go | 14 ++++++++++++- ssh/session_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/ssh/server.go b/ssh/server.go index 70045bdfd8..2260b20afc 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -68,8 +68,16 @@ type ServerConfig struct { // NoClientAuth is true if clients are allowed to connect without // authenticating. + // To determine NoClientAuth at runtime, set NoClientAuth to true + // and the optional NoClientAuthCallback to a non-nil value. NoClientAuth bool + // NoClientAuthCallback, if non-nil, is called when a user + // attempts to authenticate with auth method "none". + // NoClientAuth must also be set to true for this be used, or + // this func is unused. + NoClientAuthCallback func(ConnMetadata) (*Permissions, error) + // MaxAuthTries specifies the maximum number of authentication attempts // permitted per connection. If set to a negative number, the number of // attempts are unlimited. If set to zero, the number of attempts are limited @@ -455,7 +463,11 @@ userAuthLoop: switch userAuthReq.Method { case "none": if config.NoClientAuth { - authErr = nil + if config.NoClientAuthCallback != nil { + perms, authErr = config.NoClientAuthCallback(s) + } else { + authErr = nil + } } // allow initial attempt of 'none' without penalty diff --git a/ssh/session_test.go b/ssh/session_test.go index 1009affddd..c421adfa46 100644 --- a/ssh/session_test.go +++ b/ssh/session_test.go @@ -780,3 +780,54 @@ func TestHostKeyAlgorithms(t *testing.T) { t.Fatal("succeeded connecting with unknown hostkey algorithm") } } + +func TestServerClientAuthCallback(t *testing.T) { + c1, c2, err := netPipe() + if err != nil { + t.Fatalf("netPipe: %v", err) + } + defer c1.Close() + defer c2.Close() + + userCh := make(chan string, 1) + + serverConf := &ServerConfig{ + NoClientAuth: true, + NoClientAuthCallback: func(conn ConnMetadata) (*Permissions, error) { + userCh <- conn.User() + return nil, nil + }, + } + const someUsername = "some-username" + + serverConf.AddHostKey(testSigners["ecdsa"]) + clientConf := &ClientConfig{ + HostKeyCallback: InsecureIgnoreHostKey(), + User: someUsername, + } + + go func() { + _, chans, reqs, err := NewServerConn(c1, serverConf) + if err != nil { + t.Errorf("server handshake: %v", err) + userCh <- "error" + return + } + go DiscardRequests(reqs) + for ch := range chans { + ch.Reject(Prohibited, "") + } + }() + + conn, _, _, err := NewClientConn(c2, "", clientConf) + if err != nil { + t.Fatalf("client handshake: %v", err) + return + } + conn.Close() + + got := <-userCh + if got != someUsername { + t.Errorf("username = %q; want %q", got, someUsername) + } +} From 0992def3c281ab33418c37be9c42de4994af2ca1 Mon Sep 17 00:00:00 2001 From: Maisem Ali Date: Tue, 19 Apr 2022 18:25:01 -0700 Subject: [PATCH 2/5] ssh: add ErrDenied as a way to indicate auth termination Signed-off-by: Maisem Ali --- ssh/server.go | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/ssh/server.go b/ssh/server.go index 2260b20afc..1a9cbbbd74 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -76,6 +76,7 @@ type ServerConfig struct { // attempts to authenticate with auth method "none". // NoClientAuth must also be set to true for this be used, or // this func is unused. + // If the function returns ErrDenied, the connection is terminated. NoClientAuthCallback func(ConnMetadata) (*Permissions, error) // MaxAuthTries specifies the maximum number of authentication attempts @@ -86,6 +87,7 @@ type ServerConfig struct { // PasswordCallback, if non-nil, is called when a user // attempts to authenticate using a password. + // If the function returns ErrDenied, the connection is terminated. PasswordCallback func(conn ConnMetadata, password []byte) (*Permissions, error) // PublicKeyCallback, if non-nil, is called when a client @@ -96,6 +98,7 @@ type ServerConfig struct { // offered is in fact used to authenticate. To record any data // depending on the public key, store it inside a // Permissions.Extensions entry. + // If the function returns ErrDenied, the connection is terminated. PublicKeyCallback func(conn ConnMetadata, key PublicKey) (*Permissions, error) // KeyboardInteractiveCallback, if non-nil, is called when @@ -105,6 +108,7 @@ type ServerConfig struct { // Challenge rounds. To avoid information leaks, the client // should be presented a challenge even if the user is // unknown. + // If the function returns ErrDenied, the connection is terminated. KeyboardInteractiveCallback func(conn ConnMetadata, client KeyboardInteractiveChallenge) (*Permissions, error) // AuthLogCallback, if non-nil, is called to log all authentication @@ -397,12 +401,19 @@ func (l ServerAuthError) Error() string { return "[" + strings.Join(errs, ", ") + "]" } -// 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 -// 'none' authentication to discover available methods. -// It is returned in ServerAuthError.Errors from NewServerConn. -var ErrNoAuth = errors.New("ssh: no auth passed yet") +var ( + // ErrDenied can be returned from an authentication callback to inform the + // client that access is denied and that no further attempt will be accepted + // on the connection. + ErrDenied = errors.New("ssh: access denied") + + // 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 + // 'none' authentication to discover available methods. + // It is returned in ServerAuthError.Errors from NewServerConn. + ErrNoAuth = errors.New("ssh: no auth passed yet") +) func (s *connection) serverAuthenticate(config *ServerConfig) (*Permissions, error) { sessionID := s.transport.getSessionID() @@ -651,6 +662,15 @@ userAuthLoop: break userAuthLoop } + if errors.Is(authErr, ErrDenied) { + var failureMsg userAuthFailureMsg + if err := s.transport.writePacket(Marshal(failureMsg)); err != nil { + return nil, err + } + + return nil, nil + } + authFailures++ if config.MaxAuthTries > 0 && authFailures >= config.MaxAuthTries { // If we have hit the max attempts, don't bother sending the From c62ca52e1f25161c2f8dd6ca72919b30ff5c14a6 Mon Sep 17 00:00:00 2001 From: Maisem Ali Date: Tue, 19 Apr 2022 18:34:00 -0700 Subject: [PATCH 3/5] ssh: add ImplicitAuthMethod to ServerConfig This allows specifying an ImplicitAuthMethod associated with NoClientAuthCallback. Signed-off-by: Maisem Ali --- ssh/server.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/ssh/server.go b/ssh/server.go index 1a9cbbbd74..076cbf4662 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -66,6 +66,12 @@ type ServerConfig struct { hostKeys []Signer + // ImplictAuthMethod is sent to the client in the list of acceptable + // authentication methods. To make an authentication decision based on + // connection metadata use NoClientAuthCallback. If NoClientAuthCallback is + // nil, the value is unused. + ImplictAuthMethod string + // NoClientAuth is true if clients are allowed to connect without // authenticating. // To determine NoClientAuth at runtime, set NoClientAuth to true @@ -664,6 +670,9 @@ userAuthLoop: if errors.Is(authErr, ErrDenied) { var failureMsg userAuthFailureMsg + if config.ImplictAuthMethod != "" { + failureMsg.Methods = []string{config.ImplictAuthMethod} + } if err := s.transport.writePacket(Marshal(failureMsg)); err != nil { return nil, err } @@ -698,6 +707,9 @@ userAuthLoop: } var failureMsg userAuthFailureMsg + if config.NoClientAuthCallback != nil && config.ImplictAuthMethod != "" { + failureMsg.Methods = append(failureMsg.Methods, config.ImplictAuthMethod) + } if config.PasswordCallback != nil { failureMsg.Methods = append(failureMsg.Methods, "password") } From 665e2916cd9ac55496d223f0af434f3814a6cb98 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 20 Apr 2022 15:42:00 -0700 Subject: [PATCH 4/5] ssh: add WithBannerError Co-Authored-By: Maisem Ali Signed-off-by: Brad Fitzpatrick --- ssh/server.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ssh/server.go b/ssh/server.go index 076cbf4662..ebd52c8c56 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -310,6 +310,19 @@ func isAcceptableAlgo(algo string) bool { return false } +// WithBannerError is an error wrapper type that can be returned from an authentication +// function to additionally write out a banner error message. +type WithBannerError struct { + Err error + Message string +} + +func (e WithBannerError) Unwrap() error { + return e.Err +} + +func (e WithBannerError) Error() string { return e.Err.Error() } + func checkSourceAddress(addr net.Addr, sourceAddrs string) error { if addr == nil { return errors.New("ssh: no address known for client, but source-address match required") @@ -668,6 +681,13 @@ userAuthLoop: break userAuthLoop } + var w WithBannerError + if errors.As(authErr, &w) && w.Message != "" { + bannerMsg := &userAuthBannerMsg{Message: w.Message} + if err := s.transport.writePacket(Marshal(bannerMsg)); err != nil { + return nil, err + } + } if errors.Is(authErr, ErrDenied) { var failureMsg userAuthFailureMsg if config.ImplictAuthMethod != "" { From a0e3d8407552a4497bd50a62737028ef278784c0 Mon Sep 17 00:00:00 2001 From: Maisem Ali Date: Thu, 28 Apr 2022 13:21:39 -0700 Subject: [PATCH 5/5] ssh: return authErr on ErrDenied Signed-off-by: Maisem Ali --- ssh/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssh/server.go b/ssh/server.go index ebd52c8c56..5e9d019952 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -697,7 +697,7 @@ userAuthLoop: return nil, err } - return nil, nil + return nil, authErr } authFailures++