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.
2023-04-18 11:02:33 -04:00 · 2023-04-18 11:02:33 -04:00 · 939a14544c
parent bd2da9b830
commit 939a14544c
4 changed files with 321 additions and 8 deletions
--- 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
--- 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'",
 	})
--- a/sa/sysvars.go
+++ 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)
+}
--- a/sa/sysvars_test.go
+++ 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)
+			}
+		})
+	}
+}