Merge #56046

56046: cli: fix username semantics for userfile r=knz a=adityamaru 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: #55389 (comment) Fixes: #55389 Release note: None Co-authored-by: Aditya Maru <[email protected]>
cockroachdb · Oct 29, 2020 · c40c007 · c40c007
2 parents aba7e3a + e2130f7
commit c40c007
Show file tree

Hide file tree

Showing 8 changed files with 131 additions and 45 deletions.
diff --git a/DEPS.bzl b/DEPS.bzl
@@ -405,8 +405,8 @@ def go_deps():
         name = "com_github_cockroachdb_datadriven",
         build_file_proto_mode = "disable_global",
         importpath = "github.com/cockroachdb/datadriven",
-        sum = "h1:KQqSlwJmTr7NmxtVOl6nPNSZQEk8XuLoLyNDYcmniBo=",
-        version = "v1.0.1-0.20200826112548-92602d883b11",
+        sum = "h1:p4AOBShzCogHTl1n5Ff16j5a24tvk1lc7fzRqduSpKM=",
+        version = "v1.0.1-0.20201022032720-3e27adf87325",
     )
     go_repository(
         name = "com_github_cockroachdb_errors",

diff --git a/pkg/cli/BUILD.bazel b/pkg/cli/BUILD.bazel
@@ -118,6 +118,7 @@ go_library(
         "//pkg/sql/doctor",
         "//pkg/sql/execinfrapb",
         "//pkg/sql/lex",
+        "//pkg/sql/lexbase:lex",
         "//pkg/sql/parser",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)