Skip to content

Commit

Permalink
cli: fix username semantics for userfile
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@adityamaru
adityamaru committed Oct 28, 2020
1 parent 8652b7a commit e988766
Show file tree
Hide file tree
Showing 6 changed files with 128 additions and 43 deletions.
62 changes: 37 additions & 25 deletions pkg/cli/userfile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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{
Expand Down Expand Up @@ -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
Expand All @@ -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()
}
Expand All @@ -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()
}
Expand All @@ -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()
Expand All @@ -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,
}

Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down
91 changes: 82 additions & 9 deletions pkg/cli/userfiletable_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand Down Expand Up @@ -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)
})
}
})
Expand Down Expand Up @@ -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", "*"})
Expand All @@ -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})
}
})
}
4 changes: 2 additions & 2 deletions pkg/sql/lexbase/encode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/lexbase/predicates.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
1 change: 0 additions & 1 deletion pkg/storage/cloudimpl/file_table_storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 5 additions & 4 deletions pkg/storage/cloudimpl/filetable/file_table_read_writer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit e988766

Please sign in to comment.