forked from cockroachdb/cockroach
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
*: make conversions between
string
and usernames more explicit
tldr: the conversions between "external" strings and internal usernames was unprincipled, and it turns out, incorrect in some cases. This patch cleans this up by introducing a strict conversion API. **Background** CockroachDB currently performs case folding and unicode NFC normalization upon receiving usernames, specifically usernames received from as SQL login principals. Internally, usernames are often—but not always—considered pre-normalized for the purpose of comparisons, privilege checks, role membership checks and the like. Finally, sometimes usernames are reported "back to outside". In error messages, log files etc, but also: - in the SQL syntax produced by SHOW CREATE, SHOW SYNTAX etc. - to generate OS-level file names for exported files. **New API** This patch introduces a new data type `security.SQLUsername`. It is incomparable and non-convertible with the Go `string`. Engineers must now declare intent about how to do the conversion: - `security.MakeSQLUsernameFromUserInput` converts an "outside" string to a username. - `security.MakeSQLUsernameFromPreNormalizedString` promises that its argument has already been previously normalized. To output usernames, the following APIs are also available. - `username.Normalized()` produces the username itself, without decorations. These corresponds to the raw string (after normalization). - `username.SQLIdentifier()` produces the username in valid SQL syntax, so that it can be injected safely in a SQL statement. - `(*tree.FmtCtx).FormatUsername()` takes a username and properly handles quoting and anonymization, like `FormatName()` does for `tree.Name` already. Likewise, conversion from/to protobuf is now regulated, via the new APIs `username.EncodeProto()` and `usernameproto.Decode()`. **Problems being solved** - the usernames "from outside" were normalized sometimes, *but not consistently*: 1. they were in the arguments of CREATE/DROP/ALTER ROLE. This was not changed. 2. they were not consistently converted in `cockroach cert`. This was corrected. 3. they were not in the `cockroach userfile` commands. This has been adjusted with a reference to issue cockroachdb#55389. 4. they are *not* in GRANT/REVOKE. This patch does not change this behavior, but highlights it by spelling out `MakeSQLUsernameFromPreNormalizedString()` in the implementation. 5. ditto for CREATE SCHEMA ... AUTHORIZATION and ALTER ... OWNER TO 6. they were in the argument to `LoginRequest`. This was not changed. 7. they were not in the argument of the other API requests that allow usernames, for example `ListSessions` or `CancelQuery`. This was not changed, but is now documented in the API. - the usernames "inside" were incorrectly directly injected in SQL statements, even though they may contain special characters that create SQL syntax errors. This has been corrected by suitable uses of the new `SQLIdentifier()` method. - There was an outright bug in a call to `CreateGCJobRec` (something about GCing jobs), where a `Description` field was passed in lieu of a username for a `User` field. The implications of this are unclear. **Status after this change** The new API makes it possible to audit exactly where "sensitive" username/string conversion occurs. After this patch, we find the following uses: - `MakeSQLUsernameFromUserInput`: - pgwire user auth - CLI URL parsing - `cockroach userfile` - `cockroach cert` - `(*rpc.SecurityContext).PGURL()` (unsure whether that's a good thing) - CREATE/DROP/ALTER ROLE - when using literal strings as `role_spec` in the SQL grammar - `MakeSQLUsernameFromPreNormalizedString`: - when intepreting the username in API query parameters, for those API documented as using pre-normalized usernames. - role membership checks inside SQL based on data read from `system` tables. - in GRANT/REVOKE (this is surprising, see above) Release note: None
- Loading branch information
Showing
259 changed files
with
3,294 additions
and
2,300 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.