From d11f15704df252bb724d1252559f152ce4735f51 Mon Sep 17 00:00:00 2001 From: Michael Golowka <72365+pcman312@users.noreply.github.com> Date: Fri, 12 Jun 2020 11:09:38 -0600 Subject: [PATCH] Validate physical CockroachDB table config value before using it (#9191) * Validate table name (and database if specified) prior to using it in SQL --- physical/cockroachdb/cockroachdb.go | 76 +++- physical/cockroachdb/cockroachdb_test.go | 69 +++- physical/cockroachdb/keywords.go | 440 +++++++++++++++++++++++ 3 files changed, 575 insertions(+), 10 deletions(-) create mode 100644 physical/cockroachdb/keywords.go diff --git a/physical/cockroachdb/cockroachdb.go b/physical/cockroachdb/cockroachdb.go index 3900708e2734..587146f2a59f 100644 --- a/physical/cockroachdb/cockroachdb.go +++ b/physical/cockroachdb/cockroachdb.go @@ -8,11 +8,13 @@ import ( "strconv" "strings" "time" + "unicode" metrics "github.com/armon/go-metrics" "github.com/cockroachdb/cockroach-go/crdb" "github.com/hashicorp/errwrap" log "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/vault/sdk/helper/strutil" "github.com/hashicorp/vault/sdk/physical" @@ -21,8 +23,14 @@ import ( ) // Verify CockroachDBBackend satisfies the correct interfaces -var _ physical.Backend = (*CockroachDBBackend)(nil) -var _ physical.Transactional = (*CockroachDBBackend)(nil) +var ( + _ physical.Backend = (*CockroachDBBackend)(nil) + _ physical.Transactional = (*CockroachDBBackend)(nil) +) + +const ( + defaultTableName = "vault_kv_store" +) // CockroachDBBackend Backend is a physical backend that stores data // within a CockroachDB database. @@ -44,14 +52,18 @@ func NewCockroachDBBackend(conf map[string]string, logger log.Logger) (physical. return nil, fmt.Errorf("missing connection_url") } - dbTable, ok := conf["table"] - if !ok { - dbTable = "vault_kv_store" + dbTable := conf["table"] + if dbTable == "" { + dbTable = defaultTableName + } + + err := validateDBTable(dbTable) + if err != nil { + return nil, errwrap.Wrapf("invalid table: {{err}}", err) } maxParStr, ok := conf["max_parallel"] var maxParInt int - var err error if ok { maxParInt, err = strconv.Atoi(maxParStr) if err != nil { @@ -239,3 +251,55 @@ func (c *CockroachDBBackend) transaction(tx *sql.Tx, txns []*physical.TxnEntry) } return nil } + +// validateDBTable against the CockroachDB rules for table names: +// https://www.cockroachlabs.com/docs/stable/keywords-and-identifiers.html#identifiers +// +// - All values that accept an identifier must: +// - Begin with a Unicode letter or an underscore (_). Subsequent characters can be letters, +// - underscores, digits (0-9), or dollar signs ($). +// - Not equal any SQL keyword unless the keyword is accepted by the element's syntax. For example, +// name accepts Unreserved or Column Name keywords. +// +// The docs do state that we can bypass these rules with double quotes, however I think it +// is safer to just require these rules across the board. +func validateDBTable(dbTable string) (err error) { + // Check if this is 'database.table' formatted. If so, split them apart and check the two + // parts from each other + split := strings.SplitN(dbTable, ".", 2) + if len(split) == 2 { + merr := &multierror.Error{} + merr = multierror.Append(merr, wrapErr("invalid database: %w", validateDBTable(split[0]))) + merr = multierror.Append(merr, wrapErr("invalid table name: %w", validateDBTable(split[1]))) + return merr.ErrorOrNil() + } + + // Disallow SQL keywords as the table name + if sqlKeywords[strings.ToUpper(dbTable)] { + return fmt.Errorf("name must not be a SQL keyword") + } + + runes := []rune(dbTable) + for i, r := range runes { + if i == 0 && !unicode.IsLetter(r) && r != '_' { + return fmt.Errorf("must use a letter or an underscore as the first character") + } + + if !unicode.IsLetter(r) && r != '_' && !unicode.IsDigit(r) && r != '$' { + return fmt.Errorf("must only contain letters, underscores, digits, and dollar signs") + } + + if r == '`' || r == '\'' || r == '"' { + return fmt.Errorf("cannot contain backticks, single quotes, or double quotes") + } + } + + return nil +} + +func wrapErr(message string, err error) error { + if err == nil { + return nil + } + return fmt.Errorf(message, err) +} diff --git a/physical/cockroachdb/cockroachdb_test.go b/physical/cockroachdb/cockroachdb_test.go index 5760f6aa109f..43853d560743 100644 --- a/physical/cockroachdb/cockroachdb_test.go +++ b/physical/cockroachdb/cockroachdb_test.go @@ -18,8 +18,9 @@ import ( func prepareCockroachDBTestContainer(t *testing.T) (cleanup func(), retURL, tableName string) { tableName = os.Getenv("CR_TABLE") if tableName == "" { - tableName = "vault_kv_store" + tableName = defaultTableName } + t.Logf("Table name: %s", tableName) retURL = os.Getenv("CR_URL") if retURL != "" { return func() {}, retURL, tableName @@ -45,8 +46,8 @@ func prepareCockroachDBTestContainer(t *testing.T) (cleanup func(), retURL, tabl } retURL = fmt.Sprintf("postgresql://root@localhost:%s/?sslmode=disable", resource.GetPort("26257/tcp")) - database := "database" - tableName = database + ".vault_kv" + database := "vault" + tableName = fmt.Sprintf("%s.%s", database, tableName) // exponential backoff-retry if err = pool.Retry(func() error { @@ -56,7 +57,7 @@ func prepareCockroachDBTestContainer(t *testing.T) (cleanup func(), retURL, tabl return err } defer db.Close() - _, err = db.Exec("CREATE DATABASE database") + _, err = db.Exec(fmt.Sprintf("CREATE DATABASE %s", database)) return err }); err != nil { cleanup() @@ -99,3 +100,63 @@ func truncate(t *testing.T, b physical.Backend) { t.Fatalf("Failed to drop table: %v", err) } } + +func TestValidateDBTable(t *testing.T) { + type testCase struct { + table string + expectErr bool + } + + tests := map[string]testCase{ + "first character is letter": {"abcdef", false}, + "first character is underscore": {"_bcdef", false}, + "exclamation point": {"ab!def", true}, + "at symbol": {"ab@def", true}, + "hash": {"ab#def", true}, + "percent": {"ab%def", true}, + "carrot": {"ab^def", true}, + "ampersand": {"ab&def", true}, + "star": {"ab*def", true}, + "left paren": {"ab(def", true}, + "right paren": {"ab)def", true}, + "dash": {"ab-def", true}, + "digit": {"a123ef", false}, + "dollar end": {"abcde$", false}, + "dollar middle": {"ab$def", false}, + "dollar start": {"$bcdef", true}, + "backtick prefix": {"`bcdef", true}, + "backtick middle": {"ab`def", true}, + "backtick suffix": {"abcde`", true}, + "single quote prefix": {"'bcdef", true}, + "single quote middle": {"ab'def", true}, + "single quote suffix": {"abcde'", true}, + "double quote prefix": {`"bcdef`, true}, + "double quote middle": {`ab"def`, true}, + "double quote suffix": {`abcde"`, true}, + "underscore with all runes": {"_bcd123__a__$", false}, + "all runes": {"abcd123__a__$", false}, + "default table name": {defaultTableName, false}, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + err := validateDBTable(test.table) + if test.expectErr && err == nil { + t.Fatalf("err expected, got nil") + } + if !test.expectErr && err != nil { + t.Fatalf("no error expected, got: %s", err) + } + }) + t.Run(fmt.Sprintf("database: %s", name), func(t *testing.T) { + dbTable := fmt.Sprintf("%s.%s", test.table, test.table) + err := validateDBTable(dbTable) + if test.expectErr && err == nil { + t.Fatalf("err expected, got nil") + } + if !test.expectErr && err != nil { + t.Fatalf("no error expected, got: %s", err) + } + }) + } +} diff --git a/physical/cockroachdb/keywords.go b/physical/cockroachdb/keywords.go new file mode 100644 index 000000000000..b7ecb05d7dd0 --- /dev/null +++ b/physical/cockroachdb/keywords.go @@ -0,0 +1,440 @@ +package cockroachdb + +var ( + // sqlKeywords is a reference of all of the keywords that we do not allow for use as the table name + // Referenced from: + // https://www.cockroachlabs.com/docs/stable/keywords-and-identifiers.html#identifiers + // -> https://www.cockroachlabs.com/docs/stable/keywords-and-identifiers.html#keywords + // -> https://www.cockroachlabs.com/docs/stable/sql-grammar.html + sqlKeywords = map[string]bool{ + // reserved_keyword + // https://www.cockroachlabs.com/docs/stable/sql-grammar.html#reserved_keyword + "ALL": true, + "ANALYSE": true, + "ANALYZE": true, + "AND": true, + "ANY": true, + "ARRAY": true, + "AS": true, + "ASC": true, + "ASYMMETRIC": true, + "BOTH": true, + "CASE": true, + "CAST": true, + "CHECK": true, + "COLLATE": true, + "COLUMN": true, + "CONCURRENTLY": true, + "CONSTRAINT": true, + "CREATE": true, + "CURRENT_CATALOG": true, + "CURRENT_DATE": true, + "CURRENT_ROLE": true, + "CURRENT_SCHEMA": true, + "CURRENT_TIME": true, + "CURRENT_TIMESTAMP": true, + "CURRENT_USER": true, + "DEFAULT": true, + "DEFERRABLE": true, + "DESC": true, + "DISTINCT": true, + "DO": true, + "ELSE": true, + "END": true, + "EXCEPT": true, + "FALSE": true, + "FETCH": true, + "FOR": true, + "FOREIGN": true, + "FROM": true, + "GRANT": true, + "GROUP": true, + "HAVING": true, + "IN": true, + "INITIALLY": true, + "INTERSECT": true, + "INTO": true, + "LATERAL": true, + "LEADING": true, + "LIMIT": true, + "LOCALTIME": true, + "LOCALTIMESTAMP": true, + "NOT": true, + "NULL": true, + "OFFSET": true, + "ON": true, + "ONLY": true, + "OR": true, + "ORDER": true, + "PLACING": true, + "PRIMARY": true, + "REFERENCES": true, + "RETURNING": true, + "SELECT": true, + "SESSION_USER": true, + "SOME": true, + "SYMMETRIC": true, + "TABLE": true, + "THEN": true, + "TO": true, + "TRAILING": true, + "TRUE": true, + "UNION": true, + "UNIQUE": true, + "USER": true, + "USING": true, + "VARIADIC": true, + "WHEN": true, + "WHERE": true, + "WINDOW": true, + "WITH": true, + + // cockroachdb_extra_reserved_keyword + // https://www.cockroachlabs.com/docs/stable/sql-grammar.html#cockroachdb_extra_reserved_keyword + "INDEX": true, + "NOTHING": true, + + // type_func_name_keyword + // https://www.cockroachlabs.com/docs/stable/sql-grammar.html#type_func_name_keyword + "COLLATION": true, + "CROSS": true, + "FULL": true, + "INNER": true, + "ILIKE": true, + "IS": true, + "ISNULL": true, + "JOIN": true, + "LEFT": true, + "LIKE": true, + "NATURAL": true, + "NONE": true, + "NOTNULL": true, + "OUTER": true, + "OVERLAPS": true, + "RIGHT": true, + "SIMILAR": true, + "FAMILY": true, + + // col_name_keyword + // https://www.cockroachlabs.com/docs/stable/sql-grammar.html#col_name_keyword + "ANNOTATE_TYPE": true, + "BETWEEN": true, + "BIGINT": true, + "BIT": true, + "BOOLEAN": true, + "CHAR": true, + "CHARACTER": true, + "CHARACTERISTICS": true, + "COALESCE": true, + "DEC": true, + "DECIMAL": true, + "EXISTS": true, + "EXTRACT": true, + "EXTRACT_DURATION": true, + "FLOAT": true, + "GREATEST": true, + "GROUPING": true, + "IF": true, + "IFERROR": true, + "IFNULL": true, + "INT": true, + "INTEGER": true, + "INTERVAL": true, + "ISERROR": true, + "LEAST": true, + "NULLIF": true, + "NUMERIC": true, + "OUT": true, + "OVERLAY": true, + "POSITION": true, + "PRECISION": true, + "REAL": true, + "ROW": true, + "SMALLINT": true, + "SUBSTRING": true, + "TIME": true, + "TIMETZ": true, + "TIMESTAMP": true, + "TIMESTAMPTZ": true, + "TREAT": true, + "TRIM": true, + "VALUES": true, + "VARBIT": true, + "VARCHAR": true, + "VIRTUAL": true, + "WORK": true, + + // unreserved_keyword + // https://www.cockroachlabs.com/docs/stable/sql-grammar.html#unreserved_keyword + "ABORT": true, + "ACTION": true, + "ADD": true, + "ADMIN": true, + "AGGREGATE": true, + "ALTER": true, + "AT": true, + "AUTOMATIC": true, + "AUTHORIZATION": true, + "BACKUP": true, + "BEGIN": true, + "BIGSERIAL": true, + "BLOB": true, + "BOOL": true, + "BUCKET_COUNT": true, + "BUNDLE": true, + "BY": true, + "BYTEA": true, + "BYTES": true, + "CACHE": true, + "CANCEL": true, + "CASCADE": true, + "CHANGEFEED": true, + "CLUSTER": true, + "COLUMNS": true, + "COMMENT": true, + "COMMIT": true, + "COMMITTED": true, + "COMPACT": true, + "COMPLETE": true, + "CONFLICT": true, + "CONFIGURATION": true, + "CONFIGURATIONS": true, + "CONFIGURE": true, + "CONSTRAINTS": true, + "CONVERSION": true, + "COPY": true, + "COVERING": true, + "CREATEROLE": true, + "CUBE": true, + "CURRENT": true, + "CYCLE": true, + "DATA": true, + "DATABASE": true, + "DATABASES": true, + "DATE": true, + "DAY": true, + "DEALLOCATE": true, + "DELETE": true, + "DEFERRED": true, + "DISCARD": true, + "DOMAIN": true, + "DOUBLE": true, + "DROP": true, + "ENCODING": true, + "ENUM": true, + "ESCAPE": true, + "EXCLUDE": true, + "EXECUTE": true, + "EXPERIMENTAL": true, + "EXPERIMENTAL_AUDIT": true, + "EXPERIMENTAL_FINGERPRINTS": true, + "EXPERIMENTAL_RELOCATE": true, + "EXPERIMENTAL_REPLICA": true, + "EXPIRATION": true, + "EXPLAIN": true, + "EXPORT": true, + "EXTENSION": true, + "FILES": true, + "FILTER": true, + "FIRST": true, + "FLOAT4": true, + "FLOAT8": true, + "FOLLOWING": true, + "FORCE_INDEX": true, + "FUNCTION": true, + "GLOBAL": true, + "GRANTS": true, + "GROUPS": true, + "HASH": true, + "HIGH": true, + "HISTOGRAM": true, + "HOUR": true, + "IMMEDIATE": true, + "IMPORT": true, + "INCLUDE": true, + "INCREMENT": true, + "INCREMENTAL": true, + "INDEXES": true, + "INET": true, + "INJECT": true, + "INSERT": true, + "INT2": true, + "INT2VECTOR": true, + "INT4": true, + "INT8": true, + "INT64": true, + "INTERLEAVE": true, + "INVERTED": true, + "ISOLATION": true, + "JOB": true, + "JOBS": true, + "JSON": true, + "JSONB": true, + "KEY": true, + "KEYS": true, + "KV": true, + "LANGUAGE": true, + "LAST": true, + "LC_COLLATE": true, + "LC_CTYPE": true, + "LEASE": true, + "LESS": true, + "LEVEL": true, + "LIST": true, + "LOCAL": true, + "LOCKED": true, + "LOGIN": true, + "LOOKUP": true, + "LOW": true, + "MATCH": true, + "MATERIALIZED": true, + "MAXVALUE": true, + "MERGE": true, + "MINUTE": true, + "MINVALUE": true, + "MONTH": true, + "NAMES": true, + "NAN": true, + "NAME": true, + "NEXT": true, + "NO": true, + "NORMAL": true, + "NO_INDEX_JOIN": true, + "NOCREATEROLE": true, + "NOLOGIN": true, + "NOWAIT": true, + "NULLS": true, + "IGNORE_FOREIGN_KEYS": true, + "OF": true, + "OFF": true, + "OID": true, + "OIDS": true, + "OIDVECTOR": true, + "OPERATOR": true, + "OPT": true, + "OPTION": true, + "OPTIONS": true, + "ORDINALITY": true, + "OTHERS": true, + "OVER": true, + "OWNED": true, + "PARENT": true, + "PARTIAL": true, + "PARTITION": true, + "PARTITIONS": true, + "PASSWORD": true, + "PAUSE": true, + "PHYSICAL": true, + "PLAN": true, + "PLANS": true, + "PRECEDING": true, + "PREPARE": true, + "PRESERVE": true, + "PRIORITY": true, + "PUBLIC": true, + "PUBLICATION": true, + "QUERIES": true, + "QUERY": true, + "RANGE": true, + "RANGES": true, + "READ": true, + "RECURSIVE": true, + "REF": true, + "REGCLASS": true, + "REGPROC": true, + "REGPROCEDURE": true, + "REGNAMESPACE": true, + "REGTYPE": true, + "REINDEX": true, + "RELEASE": true, + "RENAME": true, + "REPEATABLE": true, + "REPLACE": true, + "RESET": true, + "RESTORE": true, + "RESTRICT": true, + "RESUME": true, + "REVOKE": true, + "ROLE": true, + "ROLES": true, + "ROLLBACK": true, + "ROLLUP": true, + "ROWS": true, + "RULE": true, + "SETTING": true, + "SETTINGS": true, + "STATUS": true, + "SAVEPOINT": true, + "SCATTER": true, + "SCHEMA": true, + "SCHEMAS": true, + "SCRUB": true, + "SEARCH": true, + "SECOND": true, + "SERIAL": true, + "SERIALIZABLE": true, + "SERIAL2": true, + "SERIAL4": true, + "SERIAL8": true, + "SEQUENCE": true, + "SEQUENCES": true, + "SERVER": true, + "SESSION": true, + "SESSIONS": true, + "SET": true, + "SHARE": true, + "SHOW": true, + "SIMPLE": true, + "SKIP": true, + "SMALLSERIAL": true, + "SNAPSHOT": true, + "SPLIT": true, + "SQL": true, + "START": true, + "STATISTICS": true, + "STDIN": true, + "STORE": true, + "STORED": true, + "STORING": true, + "STRICT": true, + "STRING": true, + "SUBSCRIPTION": true, + "SYNTAX": true, + "SYSTEM": true, + "TABLES": true, + "TEMP": true, + "TEMPLATE": true, + "TEMPORARY": true, + "TESTING_RELOCATE": true, + "TEXT": true, + "TIES": true, + "TRACE": true, + "TRANSACTION": true, + "TRIGGER": true, + "TRUNCATE": true, + "TRUSTED": true, + "TYPE": true, + "THROTTLING": true, + "UNBOUNDED": true, + "UNCOMMITTED": true, + "UNKNOWN": true, + "UNLOGGED": true, + "UNSPLIT": true, + "UNTIL": true, + "UPDATE": true, + "UPSERT": true, + "UUID": true, + "USE": true, + "USERS": true, + "VALID": true, + "VALIDATE": true, + "VALUE": true, + "VARYING": true, + "VIEW": true, + "WITHIN": true, + "WITHOUT": true, + "WRITE": true, + "YEAR": true, + "ZONE": true, + } +)