sql: properly handle NULL hashedPassword column in system.users #48773
Conversation
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
Fixes cockroachdb#48769. Before this change, attempts to log in as a user with a NULL value in their "hashedPassword" column in the system.users table would cause the server to crash. This was because `retrieveUserAndPassword` was not properly handling NULL values. This change fixes this. Release note (bug fix): Manually writing a NULL value into the system.users table for the "hashedPassword" column will no longer cause a server crash during user authentication.
|
This change is |
|
❌ The GitHub CI (Cockroach) build has failed on 0f5b7e9b. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
nvanbenschoten
force-pushed
the
nvanbenschotenn/nullUser
branch
from
0f5b7e9
to
d2fa1bc
Compare
|
❌ The GitHub CI (Cockroach) build has failed on d2fa1bc4. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
Before this, nil byte slices were being treated as empty byte slices, which resulted in confusing behavior where a nil byte slice passed to an InternalExecutor would not be NULL. Also, fix TestGolangQueryArgs, which was completely broken and asserting that `reflect.Type(*types.T) == reflect.Type(*types.T)`. This change is what caught the bug fixed by the previous change.
nvanbenschoten
force-pushed
the
nvanbenschotenn/nullUser
branch
from
d2fa1bc
to
9d64636
Compare
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
May 13, 2020
Most of cockroachdb#47904. First commit from cockroachdb#48504. Second and third commit from cockroachdb#48773. This commit implements the initial structure for tenant creation and deletion. It does so by introducing a new system table to track tenants in a multi-tenant cluster and two new builtin functions to manipulate this table and the overall multi-tenant state. The change starts by introducing a new `system.tenants` table with the following schema: ``` CREATE TABLE system.tenants ( id INT8 NOT NULL PRIMARY KEY, active BOOL NOT NULL DEFAULT true, info BYTES ); ``` The `id` column is self-explanatory. The `active` column is used to coordinate tenant destruction in an asynchronous job. The `info` column is an opaque byte slice to allow users to associate arbitrary information with specific tenants. I don't know exactly how this third field will be used (mapping tenants back to CockroachCloud user IDs?), but it seems like a good idea to add some flexibility since we do intend to eventually expose this externally. The table is given an ID of 8, which means that it falls within the "system config span". I believe this is ok, because the entire concept of the "system config span" should be going away with cockroachdb#47150. The table is also only exposed to the system-tenant and is never created for secondary tenants. The change then introduces two new builtin functions: `crdb_internal.create_tenant` and `crdb_internal.destroy_tenant`. These do what you would expect - creating and destroying tenant keyspaces, along with updating metadata in system.tenants.
knz
approved these changes
May 13, 2020
There was a problem hiding this comment.
I'm really glad you found all these. All this code LGTM!
I do believe that the NULL handling fix (possibly the entire PR!) should be backported to 20.1, and the NULL handling fix specifically backported to 19.2. Was that your plan already?
Reviewed 3 of 3 files at r1, 6 of 6 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained
|
TFTR!
I intend to backport the first commit, but not the second. I did try to manually audit all uses of the bors r+ |
Build succeeded |
This was referenced May 13, 2020
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
May 14, 2020
Most of cockroachdb#47904. First commit from cockroachdb#48504. Second and third commit from cockroachdb#48773. This commit implements the initial structure for tenant creation and deletion. It does so by introducing a new system table to track tenants in a multi-tenant cluster and two new builtin functions to manipulate this table and the overall multi-tenant state. The change starts by introducing a new `system.tenants` table with the following schema: ``` CREATE TABLE system.tenants ( id INT8 NOT NULL PRIMARY KEY, active BOOL NOT NULL DEFAULT true, info BYTES ); ``` The `id` column is self-explanatory. The `active` column is used to coordinate tenant destruction in an asynchronous job. The `info` column is an opaque byte slice to allow users to associate arbitrary information with specific tenants. I don't know exactly how this third field will be used (mapping tenants back to CockroachCloud user IDs?), but it seems like a good idea to add some flexibility since we do intend to eventually expose this externally. The table is given an ID of 8, which means that it falls within the "system config span". I believe this is ok, because the entire concept of the "system config span" should be going away with cockroachdb#47150. The table is also only exposed to the system-tenant and is never created for secondary tenants. The change then introduces two new builtin functions: `crdb_internal.create_tenant` and `crdb_internal.destroy_tenant`. These do what you would expect - creating and destroying tenant keyspaces, along with updating metadata in system.tenants.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #48769.
Before this change, attempts to log in as a user with a NULL value in their "hashedPassword" column in the
system.userstable would cause the server to crash. This was becauseretrieveUserAndPasswordwas not properly handling NULL values.This change fixes this. It then fixes the bug that led me to the first one - we were not properly handling nil byte slices in
golangFillQueryArguments. Nil byte slices were being treated as empty byte slices, which resulted in confusing behavior where a nil byte slice passed to an InternalExecutor would not be NULL. I'm considered backporting this fix, but I don't think we should. I fear that doing so will have unintended consequences by tickling other bugs like what is fixed in the first commit.Finally, this fixes
TestGolangQueryArgs, which was completely broken and asserting thatreflect.Type(*types.T) == reflect.Type(*types.T).Release note (bug fix): Manually writing a NULL value into the system.users table for the "hashedPassword" column will no longer cause a server crash during user authentication.