From 939a14544c008f245113f9f831a9de35fd1aba84 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Tue, 18 Apr 2023 11:02:33 -0400 Subject: [PATCH] SA: Check MariaDB system variables at startup (#6791) Adds a new function to the `//sa` to ensure that a MariaDB config passed in via SA `setDefault` or via DSN perform the following validations: 1. Correct quoting for strings and string enums to prevent future problems such as PR #6683 from occurring. 2. Each system variable we care to use is scoped as SESSION, rather than strictly GLOBAL. 3. Detect system variables passed in that are not in a curated list of variables we care about. 4. Validate that values for booleans, floats, integers, and strings at least pass basic a regex. This change is in a bit of a weird place. The ideal place for this change would be `go-sql-driver/mysql`, but since that driver handles the general case of MySQL-compatible connections to the database, we're implementing this validation in Boulder instead. We're confident about the specific versions of MariaDB running in staging/prod and that the database vendor won't change underneath us, which is why I decided to take this approach. However, this change will bind us tighter to MariaDB than MySQL due to the specific variables we're checking. An up-to-date list of MariaDB system variables can be found [here.](https://mariadb.com/kb/en/server-system-variables/) Fixes https://github.com/letsencrypt/boulder/issues/6687. --- sa/database.go | 20 +++- sa/database_test.go | 28 +++++- sa/sysvars.go | 235 ++++++++++++++++++++++++++++++++++++++++++++ sa/sysvars_test.go | 46 +++++++++ 4 files changed, 321 insertions(+), 8 deletions(-) create mode 100644 sa/sysvars.go create mode 100644 sa/sysvars_test.go diff --git a/sa/database.go b/sa/database.go index a0b7e50d4..eef2b2ae5 100644 --- a/sa/database.go +++ b/sa/database.go @@ -138,7 +138,10 @@ var setConnMaxIdleTime = func(db *sql.DB, connMaxIdleTime time.Duration) { // NewDbMapFromConfig functions similarly to NewDbMap, but it takes the // decomposed form of the connection string, a *mysql.Config. func NewDbMapFromConfig(config *mysql.Config, settings DbSettings) (*boulderDB.WrappedMap, error) { - adjustMySQLConfig(config) + err := adjustMySQLConfig(config) + if err != nil { + return nil, err + } db, err := sqlOpen("mysql", config.FormatDSN()) if err != nil { @@ -156,12 +159,11 @@ func NewDbMapFromConfig(config *mysql.Config, settings DbSettings) (*boulderDB.W dbmap := &gorp.DbMap{Db: db, Dialect: dialect, TypeConverter: BoulderTypeConverter{}} initTables(dbmap) - return &boulderDB.WrappedMap{DbMap: dbmap}, nil } // adjustMySQLConfig sets certain flags that we want on every connection. -func adjustMySQLConfig(conf *mysql.Config) { +func adjustMySQLConfig(conf *mysql.Config) error { // Required to turn DATETIME fields into time.Time conf.ParseTime = true @@ -179,7 +181,7 @@ func adjustMySQLConfig(conf *mysql.Config) { conf.Params = make(map[string]string) } - // If a given parameter is not already set in conf.Params, set it. + // If a given parameter is not already set in conf.Params from the DSN, set it. setDefault := func(name, value string) { _, ok := conf.Params[name] if !ok { @@ -214,6 +216,16 @@ func adjustMySQLConfig(conf *mysql.Config) { omitZero("max_statement_time") omitZero("long_query_time") + + // Finally, perform validation over all variables set by the DSN and via Boulder. + for k, v := range conf.Params { + err := checkMariaDBSystemVariables(k, v) + if err != nil { + return err + } + } + + return nil } // SetSQLDebug enables GORP SQL-level Debugging diff --git a/sa/database_test.go b/sa/database_test.go index 758181654..69b068fb2 100644 --- a/sa/database_test.go +++ b/sa/database_test.go @@ -15,6 +15,22 @@ import ( func TestInvalidDSN(t *testing.T) { _, err := NewDbMap("invalid", DbSettings{}) test.AssertError(t, err, "DB connect string missing the slash separating the database name") + + DSN := "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&stringVarThatDoesntExist=%27whoopsidaisies" + _, err = NewDbMap(DSN, DbSettings{}) + test.AssertError(t, err, "Variable does not exist in curated system var list, but didn't return an error and should have") + + DSN = "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&concurrent_insert=2" + _, err = NewDbMap(DSN, DbSettings{}) + test.AssertError(t, err, "Variable is unable to be set in the SESSION scope, but was declared") + + DSN = "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&optimizer_switch=incorrect-quoted-string" + _, err = NewDbMap(DSN, DbSettings{}) + test.AssertError(t, err, "Variable declared with incorrect quoting") + + DSN = "policy:password@tcp(boulder-proxysql:6033)/boulder_policy_integration?readTimeout=800ms&writeTimeout=800ms&concurrent_insert=%272%27" + _, err = NewDbMap(DSN, DbSettings{}) + test.AssertError(t, err, "Integer enum declared, but should not have been quoted") } var errExpected = errors.New("expected") @@ -158,13 +174,15 @@ func TestAutoIncrementSchema(t *testing.T) { func TestAdjustMySQLConfig(t *testing.T) { conf := &mysql.Config{} - adjustMySQLConfig(conf) + err := adjustMySQLConfig(conf) + test.AssertNotError(t, err, "unexpected err setting server variables") test.AssertDeepEquals(t, conf.Params, map[string]string{ "sql_mode": "'STRICT_ALL_TABLES'", }) conf = &mysql.Config{ReadTimeout: 100 * time.Second} - adjustMySQLConfig(conf) + err = adjustMySQLConfig(conf) + test.AssertNotError(t, err, "unexpected err setting server variables") test.AssertDeepEquals(t, conf.Params, map[string]string{ "sql_mode": "'STRICT_ALL_TABLES'", "max_statement_time": "95", @@ -177,7 +195,8 @@ func TestAdjustMySQLConfig(t *testing.T) { "max_statement_time": "0", }, } - adjustMySQLConfig(conf) + err = adjustMySQLConfig(conf) + test.AssertNotError(t, err, "unexpected err setting server variables") test.AssertDeepEquals(t, conf.Params, map[string]string{ "sql_mode": "'STRICT_ALL_TABLES'", "long_query_time": "80", @@ -188,7 +207,8 @@ func TestAdjustMySQLConfig(t *testing.T) { "max_statement_time": "0", }, } - adjustMySQLConfig(conf) + err = adjustMySQLConfig(conf) + test.AssertNotError(t, err, "unexpected err setting server variables") test.AssertDeepEquals(t, conf.Params, map[string]string{ "sql_mode": "'STRICT_ALL_TABLES'", }) diff --git a/sa/sysvars.go b/sa/sysvars.go new file mode 100644 index 000000000..6039c82e7 --- /dev/null +++ b/sa/sysvars.go @@ -0,0 +1,235 @@ +package sa + +import ( + "fmt" + "regexp" +) + +var ( + checkStringQuoteRE = regexp.MustCompile(`^'[0-9A-Za-z_\-=:]+'$`) + checkIntRE = regexp.MustCompile(`^\d+$`) + checkImproperIntRE = regexp.MustCompile(`^'\d+'$`) + checkNumericRE = regexp.MustCompile(`^\d+(\.\d+)?$`) + checkBooleanRE = regexp.MustCompile(`^([0-1])|(?i)(true|false)|(?i)(on|off)`) +) + +// checkMariaDBSystemVariables validates a MariaDB config passed in via SA +// setDefault or DSN. This manually curated list of system variables was +// partially generated by a tool in issue #6687. An overview of the validations +// performed are: +// +// - Correct quoting for strings and string enums prevent future +// problems such as PR #6683 from occurring. +// +// - Regex validation is performed for the various booleans, floats, integers, and strings. +// +// Only session scoped variables should be included. A session variable is one +// that affects the current session only. Passing a session variable that only +// works in the global scope causes database connection error 1045. +// https://mariadb.com/kb/en/set/#global-session +func checkMariaDBSystemVariables(name string, value string) error { + // System variable names will be indexed into the appropriate hash sets + // below and can possibly exist in several sets. + + // Check the list of currently known MariaDB string type system variables + // and determine if the value is a properly formatted string e.g. + // sql_mode='STRICT_TABLES' + mariaDBStringTypes := map[string]struct{}{ + "character_set_client": {}, + "character_set_connection": {}, + "character_set_database": {}, + "character_set_filesystem": {}, + "character_set_results": {}, + "character_set_server": {}, + "collation_connection": {}, + "collation_database": {}, + "collation_server": {}, + "debug/debug_dbug": {}, + "debug_sync": {}, + "enforce_storage_engine": {}, + "external_user": {}, + "lc_messages": {}, + "lc_time_names": {}, + "old_alter_table": {}, + "old_mode": {}, + "optimizer_switch": {}, + "proxy_user": {}, + "session_track_system_variables": {}, + "sql_mode": {}, + "time_zone": {}, + } + + if _, found := mariaDBStringTypes[name]; found { + if checkStringQuoteRE.FindString(value) != value { + return fmt.Errorf("%s=%s string is not properly quoted", name, value) + } + return nil + } + + // MariaDB numerics which may either be integers or floats. + // https://mariadb.com/kb/en/numeric-data-type-overview/ + mariaDBNumericTypes := map[string]struct{}{ + "bulk_insert_buffer_size": {}, + "default_week_format": {}, + "eq_range_index_dive_limit": {}, + "error_count": {}, + "expensive_subquery_limit": {}, + "group_concat_max_len": {}, + "histogram_size": {}, + "idle_readonly_transaction_timeout": {}, + "idle_transaction_timeout": {}, + "idle_write_transaction_timeout": {}, + "in_predicate_conversion_threshold": {}, + "insert_id": {}, + "interactive_timeout": {}, + "join_buffer_size": {}, + "join_buffer_space_limit": {}, + "join_cache_level": {}, + "last_insert_id": {}, + "lock_wait_timeout": {}, + "log_slow_min_examined_row_limit": {}, + "log_slow_query_time": {}, + "log_slow_rate_limit": {}, + "long_query_time": {}, + "max_allowed_packet": {}, + "max_delayed_threads": {}, + "max_digest_length": {}, + "max_error_count": {}, + "max_heap_table_size": {}, + "max_join_size": {}, + "max_length_for_sort_data": {}, + "max_recursive_iterations": {}, + "max_rowid_filter_size": {}, + "max_seeks_for_key": {}, + "max_session_mem_used": {}, + "max_sort_length": {}, + "max_sp_recursion_depth": {}, + "max_statement_time": {}, + "max_user_connections": {}, + "min_examined_row_limit": {}, + "mrr_buffer_size": {}, + "net_buffer_length": {}, + "net_read_timeout": {}, + "net_retry_count": {}, + "net_write_timeout": {}, + "optimizer_extra_pruning_depth": {}, + "optimizer_max_sel_arg_weight": {}, + "optimizer_prune_level": {}, + "optimizer_search_depth": {}, + "optimizer_selectivity_sampling_limit": {}, + "optimizer_trace_max_mem_size": {}, + "optimizer_use_condition_selectivity": {}, + "preload_buffer_size": {}, + "profiling_history_size": {}, + "progress_report_time": {}, + "pseudo_slave_mode": {}, + "pseudo_thread_id": {}, + "query_alloc_block_size": {}, + "query_prealloc_size": {}, + "rand_seed1": {}, + "range_alloc_block_size": {}, + "read_rnd_buffer_size": {}, + "rowid_merge_buff_size": {}, + "sql_select_limit": {}, + "tmp_disk_table_size": {}, + "tmp_table_size": {}, + "transaction_alloc_block_size": {}, + "transaction_prealloc_size": {}, + "wait_timeout": {}, + "warning_count": {}, + } + + if _, found := mariaDBNumericTypes[name]; found { + if checkNumericRE.FindString(value) != value { + return fmt.Errorf("%s=%s requires a numeric value, but is not formatted like a number", name, value) + } + return nil + } + + // Certain MariaDB enums can have both string and integer values. + mariaDBIntEnumTypes := map[string]struct{}{ + "completion_type": {}, + "query_cache_type": {}, + } + + mariaDBStringEnumTypes := map[string]struct{}{ + "completion_type": {}, + "default_regex_flags": {}, + "default_storage_engine": {}, + "default_tmp_storage_engine": {}, + "histogram_type": {}, + "log_slow_filter": {}, + "log_slow_verbosity": {}, + "optimizer_trace": {}, + "query_cache_type": {}, + "session_track_transaction_info": {}, + "transaction_isolation": {}, + "tx_isolation": {}, + "use_stat_tables": {}, + } + + // Check the list of currently known MariaDB enumeration type system + // variables and determine if the value is either: + // 1) A properly formatted integer e.g. completion_type=1 + if _, found := mariaDBIntEnumTypes[name]; found { + if checkIntRE.FindString(value) == value { + return nil + } + if checkImproperIntRE.FindString(value) == value { + return fmt.Errorf("%s=%s integer enum is quoted, but should not be", name, value) + } + } + + // 2) A properly formatted string e.g. completion_type='CHAIN' + if _, found := mariaDBStringEnumTypes[name]; found { + if checkStringQuoteRE.FindString(value) != value { + return fmt.Errorf("%s=%s string enum is not properly quoted", name, value) + } + return nil + } + + // MariaDB booleans can be (0, false) or (1, true). + // https://mariadb.com/kb/en/boolean/ + mariaDBBooleanTypes := map[string]struct{}{ + "autocommit": {}, + "big_tables": {}, + "check_constraint_checks": {}, + "foreign_key_checks": {}, + "in_transaction": {}, + "keep_files_on_create": {}, + "log_slow_query": {}, + "low_priority_updates": {}, + "old": {}, + "old_passwords": {}, + "profiling": {}, + "query_cache_strip_comments": {}, + "query_cache_wlock_invalidate": {}, + "session_track_schema": {}, + "session_track_state_change": {}, + "slow_query_log": {}, + "sql_auto_is_null": {}, + "sql_big_selects": {}, + "sql_buffer_result": {}, + "sql_if_exists": {}, + "sql_log_off": {}, + "sql_notes": {}, + "sql_quote_show_create": {}, + "sql_safe_updates": {}, + "sql_warnings": {}, + "standard_compliant_cte": {}, + "tcp_nodelay": {}, + "transaction_read_only": {}, + "tx_read_only": {}, + "unique_checks": {}, + "updatable_views_with_limit": {}, + } + + if _, found := mariaDBBooleanTypes[name]; found { + if checkBooleanRE.FindString(value) != value { + return fmt.Errorf("%s=%s expected boolean value", name, value) + } + return nil + } + + return fmt.Errorf("%s=%s was unexpected", name, value) +} diff --git a/sa/sysvars_test.go b/sa/sysvars_test.go new file mode 100644 index 000000000..8c39b6235 --- /dev/null +++ b/sa/sysvars_test.go @@ -0,0 +1,46 @@ +package sa + +import ( + "testing" + + "github.com/letsencrypt/boulder/test" +) + +func TestCheckMariaDBSystemVariables(t *testing.T) { + type testCase struct { + key string + value string + expectErr string + } + + for _, tc := range []testCase{ + {"sql_select_limit", "'0.1", "requires a numeric value"}, + {"max_statement_time", "0", ""}, + {"myBabies", "kids_I_tell_ya", "was unexpected"}, + {"sql_mode", "'STRICT_ALL_TABLES", "string is not properly quoted"}, + {"sql_mode", "%27STRICT_ALL_TABLES%27", "string is not properly quoted"}, + {"completion_type", "1", ""}, + {"completion_type", "'2'", "integer enum is quoted, but should not be"}, + {"completion_type", "RELEASE", "string enum is not properly quoted"}, + {"completion_type", "'CHAIN'", ""}, + {"autocommit", "0", ""}, + {"check_constraint_checks", "1", ""}, + {"log_slow_query", "true", ""}, + {"foreign_key_checks", "false", ""}, + {"sql_warnings", "TrUe", ""}, + {"tx_read_only", "FalSe", ""}, + {"sql_notes", "on", ""}, + {"tcp_nodelay", "off", ""}, + {"autocommit", "2", "expected boolean value"}, + } { + t.Run(tc.key, func(t *testing.T) { + err := checkMariaDBSystemVariables(tc.key, tc.value) + if tc.expectErr == "" { + test.AssertNotError(t, err, "Unexpected error received") + } else { + test.AssertError(t, err, "Error expected, but not found") + test.AssertContains(t, err.Error(), tc.expectErr) + } + }) + } +}