From e98876605757acf3126aee0f99333054227b11ae Mon Sep 17 00:00:00 2001 From: Aditya Maru Date: Wed, 28 Oct 2020 09:00:06 -0400 Subject: [PATCH] cli: fix username semantics for userfile Previously, userfile would break if a user had a username with special characters, which is otherwise supported by CRDB. This is because, unless specified, userfile uses the username to generate the underlying storage table names. This change introduces a new name generation scheme which accounts for all usernames supported by the database. The details are explained in: https://github.com/cockroachdb/cockroach/issues/55389#issuecomment-712800826 Fixes: #55389 Release note: None --- pkg/cli/userfile.go | 62 ++++++++----- pkg/cli/userfiletable_test.go | 91 +++++++++++++++++-- pkg/sql/lexbase/encode.go | 4 +- pkg/sql/lexbase/predicates.go | 4 +- pkg/storage/cloudimpl/file_table_storage.go | 1 - .../filetable/file_table_read_writer.go | 9 +- 6 files changed, 128 insertions(+), 43 deletions(-) diff --git a/pkg/cli/userfile.go b/pkg/cli/userfile.go index 987059769981..c0027aba435e 100644 --- a/pkg/cli/userfile.go +++ b/pkg/cli/userfile.go @@ -23,6 +23,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql" + "github.com/cockroachdb/cockroach/pkg/sql/lexbase" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/storage/cloud" "github.com/cockroachdb/cockroach/pkg/storage/cloudimpl" @@ -31,10 +32,11 @@ import ( ) const ( - defaultUserfileScheme = "userfile" - defaultQualifiedNamePrefix = "defaultdb.public.userfiles_" - tmpSuffix = ".tmp" - fileTableNameSuffix = "_upload_files" + defaultUserfileScheme = "userfile" + defaultQualifiedNamePrefix = "defaultdb.public.userfiles_" + defaultQualifiedHexNamePrefix = "defaultdb.public.userfilesx_" + tmpSuffix = ".tmp" + fileTableNameSuffix = "_upload_files" ) var userFileUploadCmd = &cobra.Command{ @@ -167,6 +169,23 @@ func openUserFile(source string) (io.ReadCloser, error) { return f, nil } +// getDefaultQualifiedTableName returns the default table name prefix for the +// tables backing userfile. +// To account for all supported usernames, we adopt a naming scheme whereby if +// the normalized username remains unquoted after encoding to a SQL identifier, +// we use it as is. Otherwise we use its hex representation. +// +// This schema gives us the two properties we desire from this table name prefix: +// - Uniqueness amongst users with different usernames. +// - Support for all current and future valid usernames. +func getDefaultQualifiedTableName(user security.SQLUsername) string { + normalizedUsername := user.Normalized() + if lexbase.IsBareIdentifier(normalizedUsername) { + return defaultQualifiedNamePrefix + normalizedUsername + } + return defaultQualifiedHexNamePrefix + fmt.Sprintf("%x", normalizedUsername) +} + // Construct the userfile ExternalStorage URI from CLI args. func constructUserfileDestinationURI(source, destination string, user security.SQLUsername) string { // User has not specified a destination URI/path. We use the default URI @@ -175,10 +194,8 @@ func constructUserfileDestinationURI(source, destination string, user security.S sourceFilename := path.Base(source) userFileURL := url.URL{ Scheme: defaultUserfileScheme, - // TODO(knz): This looks suspicious, see - // https://github.com/cockroachdb/cockroach/issues/55389 - Host: defaultQualifiedNamePrefix + user.Normalized(), - Path: sourceFilename, + Host: getDefaultQualifiedTableName(user), + Path: sourceFilename, } return userFileURL.String() } @@ -194,9 +211,7 @@ func constructUserfileDestinationURI(source, destination string, user security.S if userfileURI, err = url.ParseRequestURI(destination); err == nil { if userfileURI.Scheme == defaultUserfileScheme { if userfileURI.Host == "" { - // TODO(knz): This looks suspicious, see - // https://github.com/cockroachdb/cockroach/issues/55389 - userfileURI.Host = defaultQualifiedNamePrefix + user.Normalized() + userfileURI.Host = getDefaultQualifiedTableName(user) } return userfileURI.String() } @@ -206,21 +221,19 @@ func constructUserfileDestinationURI(source, destination string, user security.S // userfile URI schema and host, and the destination as the path. userFileURL := url.URL{ Scheme: defaultUserfileScheme, - // TODO(knz): This looks suspicious, see - // https://github.com/cockroachdb/cockroach/issues/55389 - Host: defaultQualifiedNamePrefix + user.Normalized(), - Path: destination, + Host: getDefaultQualifiedTableName(user), + Path: destination, } return userFileURL.String() } -func constructUserfileListURI(glob, user string) string { +func constructUserfileListURI(glob string, user security.SQLUsername) string { // User has not specified a glob pattern and so we construct a URI which will // list all the files stored in the UserFileTableStorage. if glob == "" || glob == "*" { userFileURL := url.URL{ Scheme: defaultUserfileScheme, - Host: defaultQualifiedNamePrefix + user, + Host: getDefaultQualifiedTableName(user), Path: "", } return userFileURL.String() @@ -239,7 +252,7 @@ func constructUserfileListURI(glob, user string) string { // userfile URI schema and host, and the glob as the path. userfileURL := url.URL{ Scheme: defaultUserfileScheme, - Host: defaultQualifiedNamePrefix + user, + Host: getDefaultQualifiedTableName(user), Path: glob, } @@ -256,14 +269,14 @@ func listUserFile(ctx context.Context, conn *sqlConn, glob string) ([]string, er return nil, err } - userfileListURI := constructUserfileListURI(glob, connURL.User.Username()) + reqUsername, _ := security.MakeSQLUsernameFromUserInput(connURL.User.Username(), security.UsernameValidation) + + userfileListURI := constructUserfileListURI(glob, reqUsername) unescapedUserfileListURI, err := url.PathUnescape(userfileListURI) if err != nil { return nil, err } - reqUsername, _ := security.MakeSQLUsernameFromUserInput(connURL.User.Username(), security.UsernameValidation) - userFileTableConf, err := cloudimpl.ExternalStorageConfFromURI(unescapedUserfileListURI, reqUsername) if err != nil { return nil, err @@ -288,14 +301,14 @@ func deleteUserFile(ctx context.Context, conn *sqlConn, glob string) ([]string, return nil, err } - userfileListURI := constructUserfileListURI(glob, connURL.User.Username()) + reqUsername, _ := security.MakeSQLUsernameFromUserInput(connURL.User.Username(), security.UsernameValidation) + + userfileListURI := constructUserfileListURI(glob, reqUsername) unescapedUserfileListURI, err := url.PathUnescape(userfileListURI) if err != nil { return nil, err } - reqUsername, _ := security.MakeSQLUsernameFromUserInput(connURL.User.Username(), security.UsernameValidation) - userFileTableConf, err := cloudimpl.ExternalStorageConfFromURI(unescapedUserfileListURI, reqUsername) if err != nil { return nil, err @@ -426,7 +439,6 @@ func uploadUserFile( if err != nil { return "", err } - // Construct the userfile URI as the destination for the CopyIn stmt. // Currently we hardcode the db.schema prefix, in the future we might allow // users to specify this. diff --git a/pkg/cli/userfiletable_test.go b/pkg/cli/userfiletable_test.go index f9c1f309d813..8c7773e1787a 100644 --- a/pkg/cli/userfiletable_test.go +++ b/pkg/cli/userfiletable_test.go @@ -24,6 +24,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/stretchr/testify/require" ) @@ -139,7 +140,8 @@ func TestUserFileUpload(t *testing.T) { t.Run(tc.name, func(t *testing.T) { destination := fmt.Sprintf("/test/file%d.csv", i) - _, err = c.RunWithCapture(fmt.Sprintf("userfile upload %s %s", filePath, destination)) + _, err = c.RunWithCapture(fmt.Sprintf("userfile upload %s %s", filePath, + destination)) require.NoError(t, err) checkUserFileContent(ctx, t, c.ExecutorConfig(), security.RootUserName(), @@ -171,8 +173,8 @@ func TestUserFileUpload(t *testing.T) { } } -func checkListedFiles(t *testing.T, c cliTest, uri string, expectedFiles []string) { - cliOutput, err := c.RunWithCaptureArgs([]string{"userfile", "list", uri}) +func checkListedFiles(t *testing.T, c cliTest, uri string, args string, expectedFiles []string) { + cliOutput, err := c.RunWithCaptureArgs([]string{"userfile", "list", uri, args}) require.NoError(t, err) cliOutput = strings.TrimSpace(cliOutput) @@ -185,8 +187,8 @@ func checkListedFiles(t *testing.T, c cliTest, uri string, expectedFiles []strin require.Equal(t, expectedFiles, listedFiles) } -func checkDeletedFiles(t *testing.T, c cliTest, uri string, expectedFiles []string) { - cliOutput, err := c.RunWithCaptureArgs([]string{"userfile", "delete", uri}) +func checkDeletedFiles(t *testing.T, c cliTest, uri, args string, expectedFiles []string) { + cliOutput, err := c.RunWithCaptureArgs([]string{"userfile", "delete", uri, args}) require.NoError(t, err) cliOutput = strings.TrimSpace(cliOutput) @@ -312,7 +314,7 @@ func TestUserFileList(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - checkListedFiles(t, c, tc.URI, tc.resultList) + checkListedFiles(t, c, tc.URI, "", tc.resultList) }) } }) @@ -453,13 +455,13 @@ func TestUserFileDelete(t *testing.T) { } // List files prior to deletion. - checkListedFiles(t, c, "", abs(tc.writeList)) + checkListedFiles(t, c, "", "", abs(tc.writeList)) // Delete files. - checkDeletedFiles(t, c, tc.URI, tc.expectedDeleteList) + checkDeletedFiles(t, c, tc.URI, "", tc.expectedDeleteList) // List files after deletion. - checkListedFiles(t, c, "", abs(tc.postDeleteList)) + checkListedFiles(t, c, "", "", abs(tc.postDeleteList)) // Cleanup all files for next test run. _, err = c.RunWithCaptureArgs([]string{"userfile", "delete", "*"}) @@ -469,3 +471,74 @@ func TestUserFileDelete(t *testing.T) { }) require.NoError(t, os.RemoveAll(dir)) } + +func TestUsernameUserfileInteraction(t *testing.T) { + defer leaktest.AfterTest(t)() + + c := newCLITest(cliTestParams{t: t}) + c.omitArgs = true + defer c.cleanup() + + dir, cleanFn := testutils.TempDir(t) + defer cleanFn() + + localFilePath := filepath.Join(dir, "test.csv") + fileContent := []byte("a") + err := ioutil.WriteFile(localFilePath, []byte("a"), 0666) + require.NoError(t, err) + + rootURL, cleanup := sqlutils.PGUrl(t, c.ServingSQLAddr(), t.Name(), + url.User(security.RootUser)) + defer cleanup() + + conn := makeSQLConn(rootURL.String()) + defer conn.Close() + + ctx := context.Background() + + t.Run("usernames", func(t *testing.T) { + for _, tc := range []struct { + name string + username string + }{ + { + "simple-username", + "foo", + }, + { + "digit-username", + "123foo", + }, + { + "special-char-username", + "foo.foo", + }, + } { + createUserQuery := fmt.Sprintf(`CREATE USER "%s" WITH PASSWORD 'a'`, tc.username) + err = conn.Exec(createUserQuery, nil) + require.NoError(t, err) + + privsUserQuery := fmt.Sprintf(`GRANT CREATE ON DATABASE defaultdb TO "%s"`, tc.username) + err = conn.Exec(privsUserQuery, nil) + require.NoError(t, err) + + userURL, cleanup2 := sqlutils.PGUrlWithOptionalClientCerts(t, c.ServingSQLAddr(), t.Name(), + url.UserPassword(tc.username, "a"), false) + defer cleanup2() + + _, err := c.RunWithCapture(fmt.Sprintf("userfile upload %s %s --url=%s", + localFilePath, tc.name, userURL.String())) + require.NoError(t, err) + + user, err := security.MakeSQLUsernameFromUserInput(tc.username, security.UsernameCreation) + require.NoError(t, err) + uri := constructUserfileDestinationURI("", tc.name, user) + checkUserFileContent(ctx, t, c.ExecutorConfig(), user, uri, fileContent) + + checkListedFiles(t, c, "", fmt.Sprintf("--url=%s", userURL.String()), []string{uri}) + + checkDeletedFiles(t, c, "", fmt.Sprintf("--url=%s", userURL.String()), + []string{uri}) + } + }) +} diff --git a/pkg/sql/lexbase/encode.go b/pkg/sql/lexbase/encode.go index e0412f3a9b67..213fe2c730cb 100644 --- a/pkg/sql/lexbase/encode.go +++ b/pkg/sql/lexbase/encode.go @@ -51,7 +51,7 @@ const ( // contains special characters, or the identifier is a reserved SQL // keyword. func EncodeRestrictedSQLIdent(buf *bytes.Buffer, s string, flags EncodeFlags) { - if flags.HasFlags(EncBareIdentifiers) || (!isReservedKeyword(s) && isBareIdentifier(s)) { + if flags.HasFlags(EncBareIdentifiers) || (!isReservedKeyword(s) && IsBareIdentifier(s)) { buf.WriteString(s) return } @@ -62,7 +62,7 @@ func EncodeRestrictedSQLIdent(buf *bytes.Buffer, s string, flags EncodeFlags) { // The identifier is only quoted if the flags don't tell otherwise and // the identifier contains special characters. func EncodeUnrestrictedSQLIdent(buf *bytes.Buffer, s string, flags EncodeFlags) { - if flags.HasFlags(EncBareIdentifiers) || isBareIdentifier(s) { + if flags.HasFlags(EncBareIdentifiers) || IsBareIdentifier(s) { buf.WriteString(s) return } diff --git a/pkg/sql/lexbase/predicates.go b/pkg/sql/lexbase/predicates.go index 408759a256b8..3fa9a720e75c 100644 --- a/pkg/sql/lexbase/predicates.go +++ b/pkg/sql/lexbase/predicates.go @@ -67,9 +67,9 @@ func isReservedKeyword(s string) bool { return ok } -// isBareIdentifier returns true if the input string is a permissible bare SQL +// IsBareIdentifier returns true if the input string is a permissible bare SQL // identifier. -func isBareIdentifier(s string) bool { +func IsBareIdentifier(s string) bool { if len(s) == 0 || !IsIdentStart(int(s[0])) || (s[0] >= 'A' && s[0] <= 'Z') { return false } diff --git a/pkg/storage/cloudimpl/file_table_storage.go b/pkg/storage/cloudimpl/file_table_storage.go index c96d676cce1e..a6c8e3f47e87 100644 --- a/pkg/storage/cloudimpl/file_table_storage.go +++ b/pkg/storage/cloudimpl/file_table_storage.go @@ -86,7 +86,6 @@ func makeFileTableStorage( // cfg.User is already a normalized SQL username. username := security.MakeSQLUsernameFromPreNormalizedString(cfg.User) - executor := filetable.MakeInternalFileToTableExecutor(ie, db) fileToTableSystem, err := filetable.NewFileToTableSystem(ctx, cfg.QualifiedTableName, executor, username) diff --git a/pkg/storage/cloudimpl/filetable/file_table_read_writer.go b/pkg/storage/cloudimpl/filetable/file_table_read_writer.go index 34f65067ec68..af49762570a1 100644 --- a/pkg/storage/cloudimpl/filetable/file_table_read_writer.go +++ b/pkg/storage/cloudimpl/filetable/file_table_read_writer.go @@ -788,7 +788,7 @@ func (f *FileToTableSystem) grantCurrentUserTablePrivileges( ctx context.Context, txn *kv.Txn, ie *sql.InternalExecutor, ) error { grantQuery := fmt.Sprintf(`GRANT SELECT, INSERT, DROP, DELETE ON TABLE %s, %s TO %s`, - f.GetFQFileTableName(), f.GetFQPayloadTableName(), f.username) + f.GetFQFileTableName(), f.GetFQPayloadTableName(), f.username.SQLIdentifier()) _, err := ie.QueryEx(ctx, "grant-user-file-payload-table-access", txn, sessiondata.InternalExecutorOverride{User: security.RootUserName()}, grantQuery) @@ -815,14 +815,15 @@ users WHERE NOT "username" = 'root' AND NOT "username" = 'admin' AND NOT "userna return errors.Wrap(err, "failed to get all the users of the cluster") } - var users []string + var users []security.SQLUsername for _, row := range rows { - users = append(users, string(tree.MustBeDString(row[0]))) + username := security.MakeSQLUsernameFromPreNormalizedString(string(tree.MustBeDString(row[0]))) + users = append(users, username) } for _, user := range users { revokeQuery := fmt.Sprintf(`REVOKE ALL ON TABLE %s, %s FROM %s`, - f.GetFQFileTableName(), f.GetFQPayloadTableName(), user) + f.GetFQFileTableName(), f.GetFQPayloadTableName(), user.SQLIdentifier()) _, err = ie.QueryEx(ctx, "revoke-user-privileges", txn, sessiondata.InternalExecutorOverride{User: security.RootUserName()}, revokeQuery)