sql, server: allow admin UI to view table details for non-admins

[otan]

Resolves #44033

Two changes:

• Introduced a crdb_internal.namespaces table that mirrors
  system.namespaces but only displays tables if the user has
  permissions to view the database or tables involved.
• Changed the admin queries to go through crdb_internal.namespaces and
  crdb_internal.zones instead of system.namespaces and system.zones
  so that the table details URI works for users who are not admin.

Release note (admin ui change, security update, bug fix): We previously
introduced a fix on the admin UI to prevent non-admin users from
executing queries - however, this accidentally made certain pages
requiring table details not to display. This PR allows the table details
to be displayed in its former glory.

[cockroach-teamcity]

This change is 

[knz]

Thanks for this first take. You're fast!
Yet the review reveals there's a fly in the ointment. See comment below.

Reviewed 4 of 6 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @otan)

---

pkg/server/admin.go, line 2162 at r1 (raw file):

	ctx context.Context, userName string, id sqlbase.ID,
) (zonepb.ZoneConfig, bool, error) {
	const query = `SELECT raw_config_protobuf FROM crdb_internal.zones WHERE zone_id = $1`

So this PR is not just about restoring access to a feature previously available. We're intending to backport this fix back to 2.1 and we don't want to introduce a serious perf regression in this process.

Therefore, I think the solution of using a vtable is inadequate: where the previous code was able to use a point lookup, this new code is scanning all the configs, load them in memory, and only after that filters them (we don't do predicate push-down on vtables).

Instead, I would recommend introducing equivalent lookup functions as SQL built-ins. This type of built-in was supported also in 2.1, we used e.g. to implement has_column_privilege (in sem/tree/builtins/pg_builtin.go).

The architecture would go (approximately) as follows:

1. make an accessor for this data on (*planner) in the sql package. The accessor does the lookup using the internal executor and then filters out using privilege check, much like a one-row version of the current vtable populate function.
2. make an interface called EvalXXXAccessor like we already have EvalSessionAccessor
3. make a field of that type in EvalContext like we already have SessionAccessor
4. use it in your builtin, and initialize it in the right places

---

pkg/server/admin.go, line 2208 at r1 (raw file):

	ctx context.Context, userName string, parentID sqlbase.ID, name string,
) (sqlbase.ID, error) {
	const query = `SELECT id FROM crdb_internal.namespaces WHERE parent_id = $1 AND name = $2`

same as above

---

pkg/server/admin_test.go, line 236 at r1 (raw file):

func TestAdminAPIDatabases(t *testing.T) {
	defer leaktest.AfterTest(t)()

The changes to this test are probably OK. No comment here.

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @otan)

---

pkg/server/admin.go, line 2162 at r1 (raw file):

Previously, knz (kena) wrote…

So this PR is not just about restoring access to a feature previously available. We're intending to backport this fix back to 2.1 and we don't want to introduce a serious perf regression in this process.

Therefore, I think the solution of using a vtable is inadequate: where the previous code was able to use a point lookup, this new code is scanning all the configs, load them in memory, and only after that filters them (we don't do predicate push-down on vtables).

Instead, I would recommend introducing equivalent lookup functions as SQL built-ins. This type of built-in was supported also in 2.1, we used e.g. to implement has_column_privilege (in sem/tree/builtins/pg_builtin.go).

The architecture would go (approximately) as follows:

1. make an accessor for this data on (*planner) in the sql package. The accessor does the lookup using the internal executor and then filters out using privilege check, much like a one-row version of the current vtable populate function.
2. make an interface called EvalXXXAccessor like we already have EvalSessionAccessor
3. make a field of that type in EvalContext like we already have SessionAccessor
4. use it in your builtin, and initialize it in the right places

The builtin would be named crdb_internal.your_function_name. It's possible for a builtin to have a name starting with crdb_internal.

[otan]

---

pkg/server/admin.go, line 2162 at r1 (raw file):

Previously, knz (kena) wrote…

The builtin would be named crdb_internal.your_function_name. It's possible for a builtin to have a name starting with crdb_internal.

Just to make sure I understand, the difference between the way described here and the way that's implemented is that we don't load all configs into memory and then filter then -- instead, we load as a SQL table AND THEN discard, saving a layer of SQL? Seems negligible but I guess if you have a crazy enough database....

[otan]

---

pkg/server/admin.go, line 2162 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

Just to make sure I understand, the difference between the way described here and the way that's implemented is that we don't load all configs into memory and then filter then -- instead, we load as a SQL table AND THEN discard, saving a layer of SQL? Seems negligible but I guess if you have a crazy enough database....

Oops, did not intend to send that as that sentence did not make sense.

In the way you describe, the only difference here is avoiding holding it all in memory as a SQL table, is that right?

[knz]

In the way you describe, the only difference here is avoiding holding it all in memory as a SQL table, is that right?

There is no semantic difference indeed, it's just performance. Your approach was correct from a purely functional perspective.

Seems negligible but I guess if you have a crazy enough database....

We do have users with "crazy enough databases". Remember the UI is performing a per-ID lookup for every item it finds. If there are 1000 tables, it will call the endpoint 1000 times. If each of these lookups then scans 1000 descriptors, that means you're loading 1000000 descriptors in memory.

Now, multiply that by 10 or a 100. It's simply not scalable.

[otan]

It sounds like we may also want to cache this and/or add a condition that if you are an admin user, use the SQL query instead -- this still involves a massive lookup since the GetAllDescriptors() function sounds pretty expensive.

[knz]

Where would GetAllDescriptors be used? -- Verstuurd vanaf mijn Android apparaat met K-9 Mail. Excuseer mijn beknoptheid.

[otan]

I think I misunderstood the solution you have suggested. The builtins you're suggesting to build should not be a O(n) scan of all descriptors is what you're saying, correct?

Let me make sure it's possible to build those things in :P

[knz]

The builtins you're suggesting to build should not be a O(n) scan of all descriptors is what you're saying, correct?

Yes, correct. These builtins would essentially do the following:

1. run the original admin UI query on system, with the root account, ensure the query is a point lookup
2. after the query succeeds, validate if the current user has permission over the results of the query

[knz]

Note however my recommendation above: the code to do so would live in the sql package, not builtins. There needs to be an interface between the two. Look at SessionAccessor for a precedent.

[otan]

Any reason not to execute the system namespace table lookup as admin in the admin server then executing a has_privilege built in for the descriptor id as the non-admin?

…

On Fri, 17 Jan 2020, 9:24 am kena, ***@***.***> wrote: Note however my recommendation above: the code to do so would live in the sql package, not builtins. There needs to be an interface between the two. Look at SessionAccessor for a precedent. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44093?email_source=notifications&email_token=AA32FQZT47VTPW6KQ6GHPLTQ6HSVBA5CNFSM4KH3WNMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJIMOIA#issuecomment-575719200>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA32FQ4R7Z5VZNAZOAYVGETQ6HSVBANCNFSM4KH3WNMA> .

[knz]

Software layering: we stack admin endpoint - > public sql that clients could enter in a shell - > kv The idea is that there's no more available through the admin ui than available to SQL clients -- Verstuurd vanaf mijn Android apparaat met K-9 Mail. Excuseer mijn beknoptheid.

[knz]

@otan do you think this would be done and ready to backport by tomorrow EOD? If you're unsure, we can hold hands to get this there together. We're likely to want to rush a 2.1.x and 19.1.x out of the door Very Soon Now ™ due to an unrelated rocksdb fix and it would be great to have this fix in there.

[otan]

I've been going at it for about an hour, very close. The main problem here is that I'm having a lot of difficulty not introducing a circular import between sqlbase and sql / tree package (why is sqlbase so meaty?) but have a path forward.

[knz]

you can do dependency injection

[otan]

Not quite -- it's the argument/return type of sqlbase.ID / sqlbase.ResultColumn that is providing me trouble. But I have found sql/sqlutil ...

[knz]

gotcha, thanks

[otan]

using #44167 instead, since it's different and wanted to preserve this.