From c9e8a924e0782d6c2e6a8241e8cb7d0565084ebd Mon Sep 17 00:00:00 2001 From: James Elliott Date: Thu, 30 Apr 2020 12:03:05 +1000 Subject: [PATCH] [FEATURE] Buffer size configuration and additional http error handling (#944) * implement read buffer size config option * implement write buffer size config option * implement fasthttp ErrorHandler so we can log errors to Authelia as well * add struct/schema validation * add default value * add docs * add config key to validator * refactoring * apply suggestions from code review Co-authored-by: Amir Zarrinkafsh --- config.template.yml | 9 +++++ docs/configuration/server.md | 29 +++++++++++++++ docs/configuration/session.md | 2 +- .../configuration/schema/configuration.go | 31 ++++++++-------- internal/configuration/schema/server.go | 13 +++++++ .../configuration/validator/configuration.go | 7 ++-- internal/configuration/validator/const.go | 4 +++ internal/configuration/validator/server.go | 25 +++++++++++++ .../configuration/validator/server_test.go | 35 +++++++++++++++++++ internal/server/error_handler.go | 26 ++++++++++++++ internal/server/server.go | 6 +++- 11 files changed, 166 insertions(+), 21 deletions(-) create mode 100644 docs/configuration/server.md create mode 100644 internal/configuration/schema/server.go create mode 100644 internal/configuration/validator/server.go create mode 100644 internal/configuration/validator/server_test.go create mode 100644 internal/server/error_handler.go diff --git a/config.template.yml b/config.template.yml index 881c6d6a..28fc1273 100644 --- a/config.template.yml +++ b/config.template.yml @@ -8,6 +8,15 @@ port: 9091 # tls_key: /var/lib/authelia/ssl/key.pem # tls_cert: /var/lib/authelia/ssl/cert.pem +# Configuration options specific to the internal http server +server: + # Buffers usually should be configured to be the same value. + # Explanation at https://docs.authelia.com/configuration/server.html + # Read buffer size configures the http server's maximum incoming request size in bytes. + read_buffer_size: 4096 + # Write buffer size configures the http server's maximum outgoing response size in bytes. + write_buffer_size: 4096 + # Level of verbosity for logs: info, debug, trace log_level: debug ## File path where the logs will be written. If not set logs are written to stdout. diff --git a/docs/configuration/server.md b/docs/configuration/server.md new file mode 100644 index 00000000..83dd38b3 --- /dev/null +++ b/docs/configuration/server.md @@ -0,0 +1,29 @@ +--- +layout: default +title: Server +parent: Configuration +nav_order: 9 +--- + +# Server + +The server section configures and tunes the http server module Authelia uses. + +## Configuration + +```yaml +# Configuration options specific to the internal http server +server: + # Buffers usually should be configured to be the same value. + # Explanation at https://docs.authelia.com/configuration/server.html + # Read buffer size configures the http server's maximum incoming request size in bytes. + read_buffer_size: 4096 + # Write buffer size configures the http server's maximum outgoing response size in bytes. + write_buffer_size: 4096 +``` + +### Buffer Sizes + +The read and write buffer sizes generally should be the same. This is because when Authelia verifies +if the user is authorized to visit a URL, it also sends back nearly the same size response +(write_buffer_size) as the request (read_buffer_size). diff --git a/docs/configuration/session.md b/docs/configuration/session.md index 41790b7a..ac73b675 100644 --- a/docs/configuration/session.md +++ b/docs/configuration/session.md @@ -2,7 +2,7 @@ layout: default title: Session parent: Configuration -nav_order: 9 +nav_order: 10 --- # Session diff --git a/internal/configuration/schema/configuration.go b/internal/configuration/schema/configuration.go index f4178ca7..da6fc047 100644 --- a/internal/configuration/schema/configuration.go +++ b/internal/configuration/schema/configuration.go @@ -2,27 +2,24 @@ package schema // Configuration object extracted from YAML configuration file. type Configuration struct { - Host string `mapstructure:"host"` - Port int `mapstructure:"port"` - TLSCert string `mapstructure:"tls_cert"` - TLSKey string `mapstructure:"tls_key"` - - LogLevel string `mapstructure:"log_level"` - LogFilePath string `mapstructure:"log_file_path"` - - // This secret is used by the identity validation process to forge JWT tokens - // representing the permission to proceed with the operation. + Host string `mapstructure:"host"` + Port int `mapstructure:"port"` + TLSCert string `mapstructure:"tls_cert"` + TLSKey string `mapstructure:"tls_key"` + LogLevel string `mapstructure:"log_level"` + LogFilePath string `mapstructure:"log_file_path"` JWTSecret string `mapstructure:"jwt_secret"` DefaultRedirectionURL string `mapstructure:"default_redirection_url"` GoogleAnalyticsTrackingID string `mapstructure:"google_analytics"` + // TODO: Consider refactoring the following pointers as they don't seem to need to be pointers: TOTP, Notifier, Regulation AuthenticationBackend AuthenticationBackendConfiguration `mapstructure:"authentication_backend"` Session SessionConfiguration `mapstructure:"session"` - - TOTP *TOTPConfiguration `mapstructure:"totp"` - DuoAPI *DuoAPIConfiguration `mapstructure:"duo_api"` - AccessControl AccessControlConfiguration `mapstructure:"access_control"` - Regulation *RegulationConfiguration `mapstructure:"regulation"` - Storage StorageConfiguration `mapstructure:"storage"` - Notifier *NotifierConfiguration `mapstructure:"notifier"` + TOTP *TOTPConfiguration `mapstructure:"totp"` + DuoAPI *DuoAPIConfiguration `mapstructure:"duo_api"` + AccessControl AccessControlConfiguration `mapstructure:"access_control"` + Regulation *RegulationConfiguration `mapstructure:"regulation"` + Storage StorageConfiguration `mapstructure:"storage"` + Notifier *NotifierConfiguration `mapstructure:"notifier"` + Server ServerConfiguration `mapstructure:"server"` } diff --git a/internal/configuration/schema/server.go b/internal/configuration/schema/server.go new file mode 100644 index 00000000..2dfe29fc --- /dev/null +++ b/internal/configuration/schema/server.go @@ -0,0 +1,13 @@ +package schema + +// ServerConfiguration represents the configuration of the http server. +type ServerConfiguration struct { + ReadBufferSize int `mapstructure:"read_buffer_size"` + WriteBufferSize int `mapstructure:"write_buffer_size"` +} + +// DefaultServerConfiguration represents the default values of the ServerConfiguration. +var DefaultServerConfiguration = ServerConfiguration{ + ReadBufferSize: 4096, + WriteBufferSize: 4096, +} diff --git a/internal/configuration/validator/configuration.go b/internal/configuration/validator/configuration.go index e4377752..8e0bc09d 100644 --- a/internal/configuration/validator/configuration.go +++ b/internal/configuration/validator/configuration.go @@ -11,6 +11,7 @@ var defaultPort = 8080 var defaultLogLevel = "info" // ValidateConfiguration and adapt the configuration read from file. +//nolint:gocyclo // This function is likely to always have lots of if/else statements, as long as we keep the flow clean it should be understandable func ValidateConfiguration(configuration *schema.Configuration, validator *schema.StructValidator) { if configuration.Host == "" { configuration.Host = "0.0.0.0" @@ -42,7 +43,7 @@ func ValidateConfiguration(configuration *schema.Configuration, validator *schem } if configuration.TOTP == nil { - configuration.TOTP = &schema.TOTPConfiguration{} + configuration.TOTP = &schema.DefaultTOTPConfiguration } ValidateTOTP(configuration.TOTP, validator) @@ -55,10 +56,12 @@ func ValidateConfiguration(configuration *schema.Configuration, validator *schem ValidateSession(&configuration.Session, validator) if configuration.Regulation == nil { - configuration.Regulation = &schema.RegulationConfiguration{} + configuration.Regulation = &schema.DefaultRegulationConfiguration } ValidateRegulation(configuration.Regulation, validator) + ValidateServer(&configuration.Server, validator) + ValidateStorage(configuration.Storage, validator) if configuration.Notifier == nil { diff --git a/internal/configuration/validator/const.go b/internal/configuration/validator/const.go index 70f6d341..019892b3 100644 --- a/internal/configuration/validator/const.go +++ b/internal/configuration/validator/const.go @@ -12,6 +12,10 @@ var validKeys = []string{ "tls_cert", "google_analytics", + // Server Keys. + "server.read_buffer_size", + "server.write_buffer_size", + // TOTP Keys "totp.issuer", "totp.period", diff --git a/internal/configuration/validator/server.go b/internal/configuration/validator/server.go new file mode 100644 index 00000000..5c6f670c --- /dev/null +++ b/internal/configuration/validator/server.go @@ -0,0 +1,25 @@ +package validator + +import ( + "fmt" + + "github.com/authelia/authelia/internal/configuration/schema" +) + +var defaultReadBufferSize = 4096 +var defaultWriteBufferSize = 4096 + +// ValidateServer checks a server configuration is correct. +func ValidateServer(configuration *schema.ServerConfiguration, validator *schema.StructValidator) { + if configuration.ReadBufferSize == 0 { + configuration.ReadBufferSize = defaultReadBufferSize + } else if configuration.ReadBufferSize < 0 { + validator.Push(fmt.Errorf("server read buffer size must be above 0")) + } + + if configuration.WriteBufferSize == 0 { + configuration.WriteBufferSize = defaultWriteBufferSize + } else if configuration.WriteBufferSize < 0 { + validator.Push(fmt.Errorf("server write buffer size must be above 0")) + } +} diff --git a/internal/configuration/validator/server_test.go b/internal/configuration/validator/server_test.go new file mode 100644 index 00000000..2bd10577 --- /dev/null +++ b/internal/configuration/validator/server_test.go @@ -0,0 +1,35 @@ +package validator + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authelia/authelia/internal/configuration/schema" +) + +func TestShouldSetDefaultConfig(t *testing.T) { + validator := schema.NewStructValidator() + config := schema.ServerConfiguration{} + + ValidateServer(&config, validator) + + require.Len(t, validator.Errors(), 0) + assert.Equal(t, defaultReadBufferSize, config.ReadBufferSize) + assert.Equal(t, defaultWriteBufferSize, config.WriteBufferSize) +} + +func TestShouldRaiseOnNegativeValues(t *testing.T) { + validator := schema.NewStructValidator() + config := schema.ServerConfiguration{ + ReadBufferSize: -1, + WriteBufferSize: -1, + } + + ValidateServer(&config, validator) + + require.Len(t, validator.Errors(), 2) + assert.EqualError(t, validator.Errors()[0], "server read buffer size must be above 0") + assert.EqualError(t, validator.Errors()[1], "server write buffer size must be above 0") +} diff --git a/internal/server/error_handler.go b/internal/server/error_handler.go new file mode 100644 index 00000000..bc1254f3 --- /dev/null +++ b/internal/server/error_handler.go @@ -0,0 +1,26 @@ +package server + +import ( + "net" + + "github.com/valyala/fasthttp" + + "github.com/authelia/authelia/internal/logging" +) + +// Replacement for the default error handler in fasthttp. +func autheliaErrorHandler(ctx *fasthttp.RequestCtx, err error) { + if _, ok := err.(*fasthttp.ErrSmallBuffer); ok { + // Note: Getting X-Forwarded-For or Request URI is impossible for ths error. + logging.Logger().Tracef("Request was too large to handle from client %s. Response Code %d.", ctx.RemoteIP().String(), fasthttp.StatusRequestHeaderFieldsTooLarge) + ctx.Error("Request header too large", fasthttp.StatusRequestHeaderFieldsTooLarge) + } else if netErr, ok := err.(*net.OpError); ok && netErr.Timeout() { + // TODO: Add X-Forwarded-For Check here. + logging.Logger().Tracef("Request timeout occurred while handling from client %s: %s. Response Code %d.", ctx.RemoteIP().String(), ctx.RequestURI(), fasthttp.StatusRequestTimeout) + ctx.Error("Request timeout", fasthttp.StatusRequestTimeout) + } else { + // TODO: Add X-Forwarded-For Check here. + logging.Logger().Tracef("An unknown error occurred while handling a request from client %s: %s. Response Code %d.", ctx.RemoteIP().String(), ctx.RequestURI(), fasthttp.StatusBadRequest) + ctx.Error("Error when parsing request", fasthttp.StatusBadRequest) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 6b2658a9..fee0fdda 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -115,8 +115,12 @@ func StartServer(configuration schema.Configuration, providers middlewares.Provi router.NotFound = ServeIndex(embeddedAssets) server := &fasthttp.Server{ - Handler: middlewares.LogRequestMiddleware(router.Handler), + Handler: middlewares.LogRequestMiddleware(router.Handler), + ErrorHandler: autheliaErrorHandler, + ReadBufferSize: configuration.Server.ReadBufferSize, + WriteBufferSize: configuration.Server.WriteBufferSize, } + addrPattern := fmt.Sprintf("%s:%d", configuration.Host, configuration.Port) if configuration.TLSCert != "" && configuration.TLSKey != "" {