Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

externalconn: grant ALL on CREATE EXTERNAL CONNECTION #86336

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

adityamaru
Copy link
Contributor

Previously, the user that created the external connection
was not granted any privileges as the creator/owner of the
object. This diff changes this by granting the user ALL
privileges.

When synthetic privileges have a concept of owners, we should
also set the owner of the External Connection object to the
creator. This can happen once #86181
is addressed.

There was also a small bug in the grant/revoke code where
external connection names that required to be wrapped in
"" egs: "foo-kms" were being incorrectly stringified when
creating the synthetic privilege path. This resulted in:

CREATE EXTERNAL CONNECTION "foo-kms" AS ...
GRANT USAGE ON EXTERNAL CONNECTION "foo-kms" TO baz

To fail with a "foo-kms" cannot be found, even though it was
infact created before attempting the grant. Tests have been added to
test this case.

Fixes: #86261

Release note (bug fix): Users that create an external connection
are now granted ALL privileges on the object.

Release justification: bug fix for new functionality

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru removed the request for review from a team August 17, 2022 21:25
}

// Grant user `ALL` on the newly created External Connection.
grantStatement := fmt.Sprintf(`GRANT ALL ON EXTERNAL CONNECTION "%s" TO %s`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Normalized and not SQLIdentifier

// Normalized returns the normalized username, suitable for equality
// comparison and lookups. The username is unquoted.
func (s SQLUsername) Normalized() string { return s.u }

// SQLIdentifier returns the normalized username in a form
// suitable for parsing as a SQL identifier.
// The identifier is quoted if it contains special characters
// or it is a reserved keyword.
func (s SQLUsername) SQLIdentifier() string {
	var buf bytes.Buffer
	lexbase.EncodeRestrictedSQLIdent(&buf, s.u, lexbase.EncNoFlags)
	return buf.String()
}

I think if you add a test by creating a username with "SELECT" it'll error out here

@adityamaru
Copy link
Contributor Author

@RichardJCai if we use Normalized usernames with special identifiers aren't quoted. So for example:

CREATE USER "bar-foo";
GRANT SYSTEM EXTERNALCONNECTION TO "bar-foo";

# Login as "bar-foo"
CREATE EXTERNAL CONNECTION foo AS 'nodelocal://foo';

--- FAILS cause the GRANT statement is `GRANT ALL ON EXTERNAL CONNECTION "foo" TO bar-foo` and the `-` is unquoted.

Is the correct pattern to use Normalized but to quote the username in the grant statement itself? fwiw I couldn't reproduce the "SELECT" example you mentioned above but maybe I'm missing a step.

@adityamaru adityamaru requested a review from RichardJCai August 18, 2022 13:56
Previously, the user that created the external connection
was not granted any privileges as the creator/owner of the
object. This diff changes this by granting the user `ALL`
privileges.

When synthetic privileges have a concept of owners, we should
also set the owner of the External Connection object to the
creator. This can happen once cockroachdb#86181
is addressed.

There was also a small bug in the grant/revoke code where
external connection names that required to be wrapped in
`""` egs: `"foo-kms"` were being incorrectly stringified when
creating the synthetic privilege path. This resulted in:

```
CREATE EXTERNAL CONNECTION "foo-kms" AS ...
GRANT USAGE ON EXTERNAL CONNECTION "foo-kms" TO baz
```

To fail with a "foo-kms" cannot be found, even though it was
infact created before attempting the grant. Tests have been added to
test this case.

Fixes: cockroachdb#86261

Release note (bug fix): Users that create an external connection
are now granted `ALL` privileges on the object.
@RichardJCai
Copy link
Contributor

@RichardJCai if we use Normalized usernames with special identifiers aren't quoted. So for example:

CREATE USER "bar-foo";
GRANT SYSTEM EXTERNALCONNECTION TO "bar-foo";

# Login as "bar-foo"
CREATE EXTERNAL CONNECTION foo AS 'nodelocal://foo';

--- FAILS cause the GRANT statement is `GRANT ALL ON EXTERNAL CONNECTION "foo" TO bar-foo` and the `-` is unquoted.

Is the correct pattern to use Normalized but to quote the username in the grant statement itself? fwiw I couldn't reproduce the "SELECT" example you mentioned above but maybe I'm missing a step.

Oops you're right, I was thinking of it in the context of actually writing it to the system.privileges table, you're right for using this in the GRANT statement.

@adityamaru
Copy link
Contributor Author

Failures are unrelated and being tracked in #86376.

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 18, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 18, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 18, 2022

Build succeeded:

@craig craig bot merged commit 6e7e8e9 into cockroachdb:master Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

externalconn: user on creation should get ALL privileges
4 participants