diff --git a/docs/defender.md b/docs/defender.md index c97b7e44..736989ed 100644 --- a/docs/defender.md +++ b/docs/defender.md @@ -2,14 +2,17 @@ The built-in `defender` allows you to configure an auto-blocking policy for SFTPGo and thus helps to prevent DoS (Denial of Service) and brute force password guessing. -If enabled it will protect SFTP, HTTP, FTP and WebDAV services and it will automatically block hosts (IP addresses) that continually fail to log in or attempt to connect. +If enabled it will protect SFTP, HTTP (WebClient and user API), FTP and WebDAV services and it will automatically block hosts (IP addresses) that continually fail to log in or attempt to connect. You can configure a score for the following events: - `score_valid`, defines the score for valid login attempts, eg. user accounts that exist. Default `1`. -- `score_invalid`, defines the score for invalid login attempts, eg. non-existent user accounts or client disconnected for inactivity without authentication attempts. Default `2`. +- `score_invalid`, defines the score for invalid login attempts, eg. non-existent user accounts. Default `2`. +- `score_no_auth`, defines the score for clients disconnected without any authentication attempt. Default `0`. - `score_limit_exceeded`, defines the score for hosts that exceeded the configured rate limits or the configured max connections per host. Default `3`. +You can set the score to `0` to not penalize some events. + And then you can configure: - `observation_time`, defines the time window, in minutes, for tracking client errors. diff --git a/docs/full-configuration.md b/docs/full-configuration.md index 9a289da5..43105a6f 100644 --- a/docs/full-configuration.md +++ b/docs/full-configuration.md @@ -83,15 +83,16 @@ The configuration file contains the following sections: - `defender`, struct containing the defender configuration. See [Defender](./defender.md) for more details. - `enabled`, boolean. Default `false`. - `driver`, string. Supported drivers are `memory` and `provider`. The `provider` driver will use the configured data provider to store defender events and it is supported for `MySQL`, `PostgreSQL` and `CockroachDB` data providers. Using the `provider` driver you can share the defender events among multiple SFTPGO instances. For a single instance the `memory` driver will be much faster. Default: `memory`. - - `ban_time`, integer. Ban time in minutes. - - `ban_time_increment`, integer. Ban time increment, as a percentage, if a banned host tries to connect again. - - `threshold`, integer. Threshold value for banning a client. - - `score_invalid`, integer. Score for invalid login attempts, eg. non-existent user accounts or client disconnected for inactivity without authentication attempts. - - `score_valid`, integer. Score for valid login attempts, eg. user accounts that exist. - - `score_limit_exceeded`, integer. Score for hosts that exceeded the configured rate limits or the maximum, per-host, allowed connections. - - `observation_time`, integer. Defines the time window, in minutes, for tracking client errors. A host is banned if it has exceeded the defined threshold during the last observation time minutes. - - `entries_soft_limit`, integer. Ignored for `provider` driver. Default: 100. - - `entries_hard_limit`, integer. The number of banned IPs and host scores kept in memory will vary between the soft and hard limit for `memory` driver. If you use the `provider` driver, this setting will limit the number of entries to return when you ask for the entire host list from the defender. Default: 150. + - `ban_time`, integer. Ban time in minutes. Default: `30`. + - `ban_time_increment`, integer. Ban time increment, as a percentage, if a banned host tries to connect again. Default: `50`. + - `threshold`, integer. Threshold value for banning a client. Default: `15`. + - `score_invalid`, integer. Score for invalid login attempts, eg. non-existent user accounts. Default: `2`. + - `score_valid`, integer. Score for valid login attempts, eg. user accounts that exist. Default: `1`. + - `score_limit_exceeded`, integer. Score for hosts that exceeded the configured rate limits or the maximum, per-host, allowed connections. Default: `3`. + - `score_no_auth`, defines the score for clients disconnected without any authentication attempt. Default: `0`. + - `observation_time`, integer. Defines the time window, in minutes, for tracking client errors. A host is banned if it has exceeded the defined threshold during the last observation time minutes. Default: `30`. + - `entries_soft_limit`, integer. Ignored for `provider` driver. Default: `100`. + - `entries_hard_limit`, integer. The number of banned IPs and host scores kept in memory will vary between the soft and hard limit for `memory` driver. If you use the `provider` driver, this setting will limit the number of entries to return when you ask for the entire host list from the defender. Default: `150`. - `safelist_file`, string. Path to a file containing a list of ip addresses and/or networks to never ban. - `blocklist_file`, string. Path to a file containing a list of ip addresses and/or networks to always ban. The lists can be reloaded on demand sending a `SIGHUP` signal on Unix based systems and a `paramchange` request to the running service on Windows. An host that is already banned will not be automatically unbanned if you put it inside the safe list, you have to unban it using the REST API. - `safelist`, list of IP addresses and/or IP ranges and/or networks to never ban. Invalid entries will be silently ignored. For large lists prefer `safelist_file`. `safelist` and `safelist_file` will be merged so that you can set both. diff --git a/go.mod b/go.mod index 4c72ed77..0e9c7a0d 100644 --- a/go.mod +++ b/go.mod @@ -159,8 +159,8 @@ require ( golang.org/x/tools v0.5.0 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect google.golang.org/appengine v1.6.7 // indirect - google.golang.org/genproto v0.0.0-20230124163310-31e0e69b6fc2 // indirect - google.golang.org/grpc v1.52.0 // indirect + google.golang.org/genproto v0.0.0-20230125152338-dcaf20b6aeaa // indirect + google.golang.org/grpc v1.52.1 // indirect google.golang.org/protobuf v1.28.1 // indirect gopkg.in/ini.v1 v1.67.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/go.sum b/go.sum index 28256631..028cfe45 100644 --- a/go.sum +++ b/go.sum @@ -2719,8 +2719,8 @@ google.golang.org/genproto v0.0.0-20221109142239-94d6d90a7d66/go.mod h1:rZS5c/ZV google.golang.org/genproto v0.0.0-20221118155620-16455021b5e6/go.mod h1:rZS5c/ZVYMaOGBfO68GWtjOw/eLaZM1X6iVtgjZ+EWg= google.golang.org/genproto v0.0.0-20221201164419-0e50fba7f41c/go.mod h1:rZS5c/ZVYMaOGBfO68GWtjOw/eLaZM1X6iVtgjZ+EWg= google.golang.org/genproto v0.0.0-20221201204527-e3fa12d562f3/go.mod h1:rZS5c/ZVYMaOGBfO68GWtjOw/eLaZM1X6iVtgjZ+EWg= -google.golang.org/genproto v0.0.0-20230124163310-31e0e69b6fc2 h1:O97sLx/Xmb/KIZHB/2/BzofxBs5QmmR0LcihPtllmbc= -google.golang.org/genproto v0.0.0-20230124163310-31e0e69b6fc2/go.mod h1:RGgjbofJ8xD9Sq1VVhDM1Vok1vRONV+rg+CjzG4SZKM= +google.golang.org/genproto v0.0.0-20230125152338-dcaf20b6aeaa h1:qQPhfbPO23fwm/9lQr91L1u62Zo6cm+zI+slZT+uf+o= +google.golang.org/genproto v0.0.0-20230125152338-dcaf20b6aeaa/go.mod h1:RGgjbofJ8xD9Sq1VVhDM1Vok1vRONV+rg+CjzG4SZKM= google.golang.org/grpc v0.0.0-20160317175043-d3ddb4469d5a/go.mod h1:yo6s7OP7yaDglbqo1J04qKzAhqBH6lvTonzMVmEdcZw= google.golang.org/grpc v1.17.0/go.mod h1:6QZJwpn2B+Zp71q/5VxRsJ6NXXVCE5NRUHRo+f3cWCs= google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c= @@ -2766,8 +2766,8 @@ google.golang.org/grpc v1.49.0/go.mod h1:ZgQEeidpAuNRZ8iRrlBKXZQP1ghovWIVhdJRyCD google.golang.org/grpc v1.50.0/go.mod h1:ZgQEeidpAuNRZ8iRrlBKXZQP1ghovWIVhdJRyCDK+GI= google.golang.org/grpc v1.50.1/go.mod h1:ZgQEeidpAuNRZ8iRrlBKXZQP1ghovWIVhdJRyCDK+GI= google.golang.org/grpc v1.51.0/go.mod h1:wgNDFcnuBGmxLKI/qn4T+m5BtEBYXJPvibbUPsAIPww= -google.golang.org/grpc v1.52.0 h1:kd48UiU7EHsV4rnLyOJRuP/Il/UHE7gdDAQ+SZI7nZk= -google.golang.org/grpc v1.52.0/go.mod h1:pu6fVzoFb+NBYNAvQL08ic+lvB2IojljRYuun5vorUY= +google.golang.org/grpc v1.52.1 h1:2NpOPk5g5Xtb0qebIEs7hNIa++PdtZLo2AQUpc1YnSU= +google.golang.org/grpc v1.52.1/go.mod h1:pu6fVzoFb+NBYNAvQL08ic+lvB2IojljRYuun5vorUY= google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0/go.mod h1:6Kw0yEErY5E/yWrBtf03jp27GLLJujG4z/JK95pnjjw= google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8= google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0= diff --git a/internal/common/common.go b/internal/common/common.go index 1ec32289..de09187a 100644 --- a/internal/common/common.go +++ b/internal/common/common.go @@ -154,8 +154,9 @@ var ( ProtocolHTTP, ProtocolHTTPShare, ProtocolOIDC} disconnHookProtocols = []string{ProtocolSFTP, ProtocolSCP, ProtocolSSH, ProtocolFTP} // the map key is the protocol, for each protocol we can have multiple rate limiters - rateLimiters map[string][]*rateLimiter - isShuttingDown atomic.Bool + rateLimiters map[string][]*rateLimiter + isShuttingDown atomic.Bool + ftpLoginCommands = []string{"PASS", "USER"} ) // Initialize sets the common configuration @@ -926,7 +927,7 @@ func (conns *ActiveConnections) Remove(connectionID string) { metric.UpdateActiveConnectionsSize(lastIdx) logger.Debug(conn.GetProtocol(), conn.GetID(), "connection removed, local address %#v, remote address %#v close fs error: %v, num open connections: %v", conn.GetLocalAddress(), conn.GetRemoteAddress(), err, lastIdx) - if conn.GetProtocol() == ProtocolFTP && conn.GetUsername() == "" { + if conn.GetProtocol() == ProtocolFTP && conn.GetUsername() == "" && !util.Contains(ftpLoginCommands, conn.GetCommand()) { ip := util.GetIPFromRemoteAddress(conn.GetRemoteAddress()) logger.ConnectionFailedLog("", ip, dataprovider.LoginMethodNoAuthTryed, conn.GetProtocol(), dataprovider.ErrNoAuthTryed.Error()) diff --git a/internal/common/common_test.go b/internal/common/common_test.go index 2c0d1375..ad756fd0 100644 --- a/internal/common/common_test.go +++ b/internal/common/common_test.go @@ -324,6 +324,7 @@ func TestDefenderIntegration(t *testing.T) { Threshold: 0, ScoreInvalid: 2, ScoreValid: 1, + ScoreNoAuth: 2, ObservationTime: 15, EntriesSoftLimit: 100, EntriesHardLimit: 150, diff --git a/internal/common/defender.go b/internal/common/defender.go index ed709121..a5aa4292 100644 --- a/internal/common/defender.go +++ b/internal/common/defender.go @@ -78,14 +78,16 @@ type DefenderConfig struct { BanTimeIncrement int `json:"ban_time_increment" mapstructure:"ban_time_increment"` // Threshold value for banning a client Threshold int `json:"threshold" mapstructure:"threshold"` - // Score for invalid login attempts, eg. non-existent user accounts or - // client disconnected for inactivity without authentication attempts + // Score for invalid login attempts, eg. non-existent user accounts ScoreInvalid int `json:"score_invalid" mapstructure:"score_invalid"` // Score for valid login attempts, eg. user accounts that exist ScoreValid int `json:"score_valid" mapstructure:"score_valid"` // Score for limit exceeded events, generated from the rate limiters or for max connections // per-host exceeded ScoreLimitExceeded int `json:"score_limit_exceeded" mapstructure:"score_limit_exceeded"` + // ScoreNoAuth defines the score for clients disconnected without authentication + // attempts + ScoreNoAuth int `json:"score_no_auth" mapstructure:"score_no_auth"` // Defines the time window, in minutes, for tracking client errors. // A host is banned if it has exceeded the defined threshold during // the last observation time minutes @@ -157,8 +159,10 @@ func (d *baseDefender) getScore(event HostEvent) int { score = d.config.ScoreValid case HostEventLimitExceeded: score = d.config.ScoreLimitExceeded - case HostEventUserNotFound, HostEventNoLoginTried: + case HostEventUserNotFound: score = d.config.ScoreInvalid + case HostEventNoLoginTried: + score = d.config.ScoreNoAuth } return score } @@ -198,11 +202,33 @@ type hostScore struct { Events []hostEvent } +func (c *DefenderConfig) checkScores() error { + if c.ScoreInvalid < 0 { + c.ScoreInvalid = 0 + } + if c.ScoreValid < 0 { + c.ScoreValid = 0 + } + if c.ScoreLimitExceeded < 0 { + c.ScoreLimitExceeded = 0 + } + if c.ScoreNoAuth < 0 { + c.ScoreNoAuth = 0 + } + if c.ScoreInvalid == 0 && c.ScoreValid == 0 && c.ScoreLimitExceeded == 0 && c.ScoreNoAuth == 0 { + return fmt.Errorf("invalid defender configuration: all scores are disabled") + } + return nil +} + // validate returns an error if the configuration is invalid func (c *DefenderConfig) validate() error { if !c.Enabled { return nil } + if err := c.checkScores(); err != nil { + return err + } if c.ScoreInvalid >= c.Threshold { return fmt.Errorf("score_invalid %v cannot be greater than threshold %v", c.ScoreInvalid, c.Threshold) } @@ -212,6 +238,9 @@ func (c *DefenderConfig) validate() error { if c.ScoreLimitExceeded >= c.Threshold { return fmt.Errorf("score_limit_exceeded %v cannot be greater than threshold %v", c.ScoreLimitExceeded, c.Threshold) } + if c.ScoreNoAuth >= c.Threshold { + return fmt.Errorf("score_no_auth %d cannot be greater than threshold %d", c.ScoreNoAuth, c.Threshold) + } if c.BanTime <= 0 { return fmt.Errorf("invalid ban_time %v", c.BanTime) } diff --git a/internal/common/defender_test.go b/internal/common/defender_test.go index 20946ede..11847a59 100644 --- a/internal/common/defender_test.go +++ b/internal/common/defender_test.go @@ -62,6 +62,7 @@ func TestBasicDefender(t *testing.T) { Threshold: 5, ScoreInvalid: 2, ScoreValid: 1, + ScoreNoAuth: 2, ScoreLimitExceeded: 3, ObservationTime: 15, EntriesSoftLimit: 1, @@ -140,7 +141,7 @@ func TestBasicDefender(t *testing.T) { assert.True(t, hosts[0].BanTime.IsZero()) assert.Empty(t, hosts[0].GetBanTime()) } - defender.AddEvent(testIP, HostEventNoLoginTried) + defender.AddEvent(testIP, HostEventUserNotFound) defender.AddEvent(testIP, HostEventNoLoginTried) assert.Equal(t, 0, defender.countHosts()) assert.Equal(t, 1, defender.countBanned()) @@ -511,6 +512,11 @@ func TestDefenderConfig(t *testing.T) { require.Error(t, err) c.ScoreValid = 1 + c.ScoreNoAuth = 10 + err = c.validate() + require.Error(t, err) + + c.ScoreNoAuth = 2 c.BanTime = 0 err = c.validate() require.Error(t, err) @@ -540,6 +546,20 @@ func TestDefenderConfig(t *testing.T) { c.EntriesHardLimit = 20 err = c.validate() require.NoError(t, err) + + c = DefenderConfig{ + Enabled: true, + ScoreInvalid: -1, + ScoreLimitExceeded: -1, + ScoreNoAuth: -1, + ScoreValid: -1, + } + err = c.validate() + require.Error(t, err) + assert.Equal(t, 0, c.ScoreInvalid) + assert.Equal(t, 0, c.ScoreValid) + assert.Equal(t, 0, c.ScoreLimitExceeded) + assert.Equal(t, 0, c.ScoreNoAuth) } func BenchmarkDefenderBannedSearch(b *testing.B) { diff --git a/internal/common/defenderdb_test.go b/internal/common/defenderdb_test.go index 3e6cb3fe..5abf3b81 100644 --- a/internal/common/defenderdb_test.go +++ b/internal/common/defenderdb_test.go @@ -39,6 +39,7 @@ func TestBasicDbDefender(t *testing.T) { Threshold: 5, ScoreInvalid: 2, ScoreValid: 1, + ScoreNoAuth: 2, ScoreLimitExceeded: 3, ObservationTime: 15, EntriesSoftLimit: 1, @@ -161,9 +162,9 @@ func TestBasicDbDefender(t *testing.T) { testIP2 := "123.45.67.91" testIP3 := "123.45.67.92" for i := 0; i < 3; i++ { - defender.AddEvent(testIP, HostEventNoLoginTried) + defender.AddEvent(testIP, HostEventUserNotFound) defender.AddEvent(testIP1, HostEventNoLoginTried) - defender.AddEvent(testIP2, HostEventNoLoginTried) + defender.AddEvent(testIP2, HostEventUserNotFound) } hosts, err = defender.GetHosts() assert.NoError(t, err) diff --git a/internal/config/config.go b/internal/config/config.go index 7dc03b7c..d197adaf 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -220,6 +220,7 @@ func Init() { ScoreInvalid: 2, ScoreValid: 1, ScoreLimitExceeded: 3, + ScoreNoAuth: 2, ObservationTime: 30, EntriesSoftLimit: 100, EntriesHardLimit: 150, @@ -1943,6 +1944,7 @@ func setViperDefaults() { viper.SetDefault("common.defender.score_invalid", globalConf.Common.DefenderConfig.ScoreInvalid) viper.SetDefault("common.defender.score_valid", globalConf.Common.DefenderConfig.ScoreValid) viper.SetDefault("common.defender.score_limit_exceeded", globalConf.Common.DefenderConfig.ScoreLimitExceeded) + viper.SetDefault("common.defender.score_no_auth", globalConf.Common.DefenderConfig.ScoreNoAuth) viper.SetDefault("common.defender.observation_time", globalConf.Common.DefenderConfig.ObservationTime) viper.SetDefault("common.defender.entries_soft_limit", globalConf.Common.DefenderConfig.EntriesSoftLimit) viper.SetDefault("common.defender.entries_hard_limit", globalConf.Common.DefenderConfig.EntriesHardLimit) diff --git a/internal/httpd/httpd_test.go b/internal/httpd/httpd_test.go index 10201f55..27c5ba1f 100644 --- a/internal/httpd/httpd_test.go +++ b/internal/httpd/httpd_test.go @@ -6374,6 +6374,7 @@ func TestDefenderAPI(t *testing.T) { cfg.DefenderConfig.Driver = driver cfg.DefenderConfig.Threshold = 3 cfg.DefenderConfig.ScoreLimitExceeded = 2 + cfg.DefenderConfig.ScoreNoAuth = 0 err := common.Initialize(cfg, 0) assert.NoError(t, err) @@ -6390,6 +6391,10 @@ func TestDefenderAPI(t *testing.T) { common.AddDefenderEvent(ip, common.HostEventNoLoginTried) hosts, _, err = httpdtest.GetDefenderHosts(http.StatusOK) assert.NoError(t, err) + assert.Len(t, hosts, 0) + common.AddDefenderEvent(ip, common.HostEventUserNotFound) + hosts, _, err = httpdtest.GetDefenderHosts(http.StatusOK) + assert.NoError(t, err) if assert.Len(t, hosts, 1) { host := hosts[0] assert.Empty(t, host.GetBanTime()) @@ -6401,7 +6406,7 @@ func TestDefenderAPI(t *testing.T) { assert.Empty(t, host.GetBanTime()) assert.Equal(t, 2, host.Score) - common.AddDefenderEvent(ip, common.HostEventNoLoginTried) + common.AddDefenderEvent(ip, common.HostEventUserNotFound) hosts, _, err = httpdtest.GetDefenderHosts(http.StatusOK) assert.NoError(t, err) if assert.Len(t, hosts, 1) { @@ -6421,8 +6426,8 @@ func TestDefenderAPI(t *testing.T) { _, _, err = httpdtest.GetDefenderHostByIP(ip, http.StatusNotFound) assert.NoError(t, err) - common.AddDefenderEvent(ip, common.HostEventNoLoginTried) - common.AddDefenderEvent(ip, common.HostEventNoLoginTried) + common.AddDefenderEvent(ip, common.HostEventUserNotFound) + common.AddDefenderEvent(ip, common.HostEventUserNotFound) hosts, _, err = httpdtest.GetDefenderHosts(http.StatusOK) assert.NoError(t, err) assert.Len(t, hosts, 1) diff --git a/internal/sftpd/internal_test.go b/internal/sftpd/internal_test.go index 6147b3f0..a257e1bd 100644 --- a/internal/sftpd/internal_test.go +++ b/internal/sftpd/internal_test.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "io" + "io/fs" "net" "os" "path/filepath" @@ -2298,3 +2299,21 @@ func TestCanReadSymlink(t *testing.T) { err = connection.canReadLink("/denied/file.txt") assert.ErrorIs(t, err, sftp.ErrSSHFxNoSuchFile) } + +func TestAuthenticationErrors(t *testing.T) { + err := newAuthenticationError(fmt.Errorf("cannot validate credentials: %w", util.NewRecordNotFoundError("not found"))) + assert.ErrorIs(t, err, sftpAuthError) + assert.ErrorIs(t, err, util.ErrNotFound) + err = newAuthenticationError(fmt.Errorf("cannot validate credentials: %w", fs.ErrPermission)) + assert.ErrorIs(t, err, sftpAuthError) + assert.NotErrorIs(t, err, util.ErrNotFound) + err = newAuthenticationError(fmt.Errorf("cert has wrong type %d", ssh.HostCert)) + assert.ErrorIs(t, err, sftpAuthError) + assert.NotErrorIs(t, err, util.ErrNotFound) + err = newAuthenticationError(errors.New("ssh: certificate signed by unrecognized authority")) + assert.ErrorIs(t, err, sftpAuthError) + assert.NotErrorIs(t, err, util.ErrNotFound) + err = newAuthenticationError(nil) + assert.ErrorIs(t, err, sftpAuthError) + assert.NotErrorIs(t, err, util.ErrNotFound) +} diff --git a/internal/sftpd/server.go b/internal/sftpd/server.go index 430518db..0cc83c20 100644 --- a/internal/sftpd/server.go +++ b/internal/sftpd/server.go @@ -92,6 +92,8 @@ var ( revokedCertManager = revokedCertificates{ certs: map[string]bool{}, } + + sftpAuthError = newAuthenticationError(nil) ) // Binding defines the configuration for a network listener @@ -208,11 +210,26 @@ type Configuration struct { } type authenticationError struct { - err string + err error } func (e *authenticationError) Error() string { - return fmt.Sprintf("Authentication error: %s", e.err) + return fmt.Sprintf("Authentication error: %v", e.err) +} + +// Is reports if target matches +func (e *authenticationError) Is(target error) bool { + _, ok := target.(*authenticationError) + return ok +} + +// Unwrap returns the wrapped error +func (e *authenticationError) Unwrap() error { + return e.err +} + +func newAuthenticationError(err error) *authenticationError { + return &authenticationError{err: err} } // ShouldBind returns true if there is at least a valid binding @@ -236,7 +253,7 @@ func (c *Configuration) getServerConfig() *ssh.ServerConfig { return sp, err } if err != nil { - return nil, &authenticationError{err: fmt.Sprintf("could not validate public key credentials: %v", err)} + return nil, newAuthenticationError(fmt.Errorf("could not validate public key credentials: %w", err)) } return sp, nil @@ -256,7 +273,7 @@ func (c *Configuration) getServerConfig() *ssh.ServerConfig { serverConfig.PasswordCallback = func(conn ssh.ConnMetadata, pass []byte) (*ssh.Permissions, error) { sp, err := c.validatePasswordCredentials(conn, pass) if err != nil { - return nil, &authenticationError{err: fmt.Sprintf("could not validate password credentials: %v", err)} + return nil, newAuthenticationError(fmt.Errorf("could not validate password credentials: %w", err)) } return sp, nil @@ -470,7 +487,7 @@ func (c *Configuration) configureKeyboardInteractiveAuth(serverConfig *ssh.Serve serverConfig.KeyboardInteractiveCallback = func(conn ssh.ConnMetadata, client ssh.KeyboardInteractiveChallenge) (*ssh.Permissions, error) { sp, err := c.validateKeyboardInteractiveCredentials(conn, client) if err != nil { - return nil, &authenticationError{err: fmt.Sprintf("could not validate keyboard interactive credentials: %v", err)} + return nil, newAuthenticationError(fmt.Errorf("could not validate keyboard interactive credentials: %w", err)) } return sp, nil @@ -666,20 +683,16 @@ func (c *Configuration) createHandlers(connection *Connection) sftp.Handlers { func checkAuthError(ip string, err error) { if authErrors, ok := err.(*ssh.ServerAuthError); ok { - // check public key auth errors here + event := common.HostEventLoginFailed for _, err := range authErrors.Errors { - if err != nil { - // these checks should be improved, we should check for error type and not error strings - if strings.Contains(err.Error(), "public key credentials") { - event := common.HostEventLoginFailed - if strings.Contains(err.Error(), "not found") { - event = common.HostEventUserNotFound - } - common.AddDefenderEvent(ip, event) - break + if errors.Is(err, sftpAuthError) { + if errors.Is(err, util.ErrNotFound) { + event = common.HostEventUserNotFound } + break } } + common.AddDefenderEvent(ip, event) } else { logger.ConnectionFailedLog("", ip, dataprovider.LoginMethodNoAuthTryed, common.ProtocolSSH, err.Error()) metric.AddNoAuthTryed() diff --git a/internal/util/errors.go b/internal/util/errors.go index a9c11169..88e9d762 100644 --- a/internal/util/errors.go +++ b/internal/util/errors.go @@ -24,6 +24,11 @@ const ( "sftpgo serve -c \"\"" ) +// errors definitions +var ( + ErrNotFound = NewRecordNotFoundError("") +) + // ValidationError raised if input data is not valid type ValidationError struct { err string @@ -55,6 +60,12 @@ func (e *RecordNotFoundError) Error() string { return fmt.Sprintf("not found: %s", e.err) } +// Is reports if target matches +func (e *RecordNotFoundError) Is(target error) bool { + _, ok := target.(*RecordNotFoundError) + return ok +} + // NewRecordNotFoundError returns a not found error func NewRecordNotFoundError(error string) *RecordNotFoundError { return &RecordNotFoundError{ diff --git a/internal/webdavd/server.go b/internal/webdavd/server.go index 8290b24e..4d653548 100644 --- a/internal/webdavd/server.go +++ b/internal/webdavd/server.go @@ -188,7 +188,6 @@ func (s *webDavServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { } user, isCached, lockSystem, loginMethod, err := s.authenticate(r, ipAddr) if err != nil { - updateLoginMetrics(&user, ipAddr, loginMethod, err) if !s.binding.DisableWWWAuthHeader { w.Header().Set("WWW-Authenticate", "Basic realm=\"SFTPGo WebDAV\"") } diff --git a/sftpgo.json b/sftpgo.json index ff16472e..0d85e130 100644 --- a/sftpgo.json +++ b/sftpgo.json @@ -28,6 +28,7 @@ "score_invalid": 2, "score_valid": 1, "score_limit_exceeded": 3, + "score_no_auth": 2, "observation_time": 30, "entries_soft_limit": 100, "entries_hard_limit": 150,