sql: unify InternalExecutor and SessionBoundInternalExecutor by andreimatei · Pull Request #44170 · cockroachdb/cockroach

andreimatei · 2020-01-21T19:25:31Z

The internal executor is a library that started bad and got worse. It
came in two flavors: InternalExecutor and SessionBoundInternalExecutor.
The interface was confusing, only matched by the implementation. And
besides tomes of code, neither of them could do the basic thing of
allowing a higher layer to bind most of the session data for future
internal queries, but let the user be overriden at the query site.

This patch does away with the SessionBoundInternalExecutor. It's
functionality is absorbed into the InternalExecutor. The ie now gets a
SetSessionData() method, used to bind session data. An extended query
interface is added, allowing some aspects of this session data to be
overriden on a per-query basis.

Release note: None

cockroach-teamcity · 2020-01-21T19:25:38Z

This change is

knz

Thanks it was useful to extract this in its own PR because now TC is teaching you something useful. I think the tests are pointing out that the default db should not be set automatically in some cases (that's on purpose).

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

andreimatei · 2020-01-21T21:59:56Z

Well TC was teaching me the same thing on the old branch too.
But anyway, can you tell me more about what you suspect is going on with this failure? I thought it's because we now end up not setting some database in sessionData. You think it's the opposite?

knz · 2020-01-21T22:09:57Z

yes it is the opposite. The message cannot access virtual schema in anonymous database must occur when the two following conditions are met together:

the current database is the empty string
the test is querying info_schema or pg_catalog

If the current db is not the empty string, the vtable can be accessed properly and instead the regular SQL semantics on the result kick in.

knz · 2020-01-21T22:10:23Z

So the test is telling you "I want my current db to be empty" and your new internal executor is making it non-empty.

andreimatei · 2020-01-22T01:30:03Z

I've fixed the failing test by no longer running in the "system" database when running as root.
The implicit "run as root" behavior that we have really bothers me. I've done what I could to mark the respective methods as deprecated and have others require a user. PTAL.

knz

Reviewed 19 of 22 files at r1, 4 of 4 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)

pkg/sql/internal.go, line 178 at r2 (raw file):

//
// Query is deprecated because it may transparently execute a query as root. Use
// QueryEx instead.

Can you file an issue and assign it to @otan and me to review all the SQL builtins and check that they are not using this method in unsafe ways (i.e. giving access to privileged data to non-admin users).
This implicit-run-as-root behavior is indeed highly problematic and I'm not sure we fully realized that before. There may be a sec vulnerability hidden in there.

pkg/sql/sqlutil/internal_executor.go, line 57 at r2 (raw file):

	// Query executes the supplied SQL statement and returns the resulting rows.
	// The statement is executed as the root user.

Please update the mention about the user here.

The internal executor is a library that started bad and got worse. It came in two flavors: InternalExecutor and SessionBoundInternalExecutor. The interface was confusing, only matched by the implementation. And besides tomes of code, neither of them could do the basic thing of allowing a higher layer to bind most of the session data for future internal queries, but let the user be overriden at the query site. This patch does away with the SessionBoundInternalExecutor. It's functionality is absorbed into the InternalExecutor. The ie now gets a SetSessionData() method, used to bind session data. An extended query interface is added, allowing some aspects of this session data to be overriden on a per-query basis. Release note: None

s/QueryWithUser/QueryEx (or QueryWithCols) Release note: None

This patch changes many users of the internal executor to explicitly declare whether they want to run as root (or as node in rare cases) or if they want to inherit the user previously set on the InternalExecutor instance that they're using. If they want to inherit and no user has previously been set on that executor, they'll get a runtime error (which hopefully is not the case for any of the callsites). There's still remaining callers to the old interface which gives you either root or another user based on what the executor has been previously set; I've given up on converting all of them. And there's some that can't simply be converted: callers in the sem package can't use the new interface because of package dependencies reasons. Release note: None

andreimatei

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

pkg/sql/internal.go, line 178 at r2 (raw file):

Previously, knz (kena) wrote…

Can you file an issue and assign it to @otan and me to review all the SQL builtins and check that they are not using this method in unsafe ways (i.e. giving access to privileged data to non-admin users).
This implicit-run-as-root behavior is indeed highly problematic and I'm not sure we fully realized that before. There may be a sec vulnerability hidden in there.

#44223

pkg/sql/sqlutil/internal_executor.go, line 57 at r2 (raw file):

Previously, knz (kena) wrote…

Please update the mention about the user here.

done

knz · 2020-01-23T15:45:17Z

👍

andreimatei

TFTR

bors r+

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

craig · 2020-01-23T17:16:02Z

Timed out (retrying...)

craig · 2020-01-23T17:28:57Z

Build failed (retrying...)

GitHub CI (Cockroach)

44170: sql: unify InternalExecutor and SessionBoundInternalExecutor r=andreimatei a=andreimatei The internal executor is a library that started bad and got worse. It came in two flavors: InternalExecutor and SessionBoundInternalExecutor. The interface was confusing, only matched by the implementation. And besides tomes of code, neither of them could do the basic thing of allowing a higher layer to bind most of the session data for future internal queries, but let the user be overriden at the query site. This patch does away with the SessionBoundInternalExecutor. It's functionality is absorbed into the InternalExecutor. The ie now gets a SetSessionData() method, used to bind session data. An extended query interface is added, allowing some aspects of this session data to be overriden on a per-query basis. Release note: None 44251: builtins: fix to_english(math.MinInt64) r=yuzefovich a=otan Resolves #44152. Release note (bug fix): `TO_ENGLISH(math.MinInt64)` previously errored. This is now fixed to return the correct result. Co-authored-by: Andrei Matei <andrei@cockroachlabs.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com>

craig · 2020-01-23T19:43:01Z

Build succeeded

GitHub CI (Cockroach)

andreimatei requested a review from knz January 21, 2020 19:25

knz reviewed Jan 21, 2020

View reviewed changes

andreimatei force-pushed the sql.internal-executor branch 2 times, most recently from c15bbfc to 5252ccc Compare January 22, 2020 01:28

knz approved these changes Jan 22, 2020

View reviewed changes

andreimatei added 2 commits January 22, 2020 12:51

sql: get rid of one of the deprecated InternalExecutor methods

7acdf83

s/QueryWithUser/QueryEx (or QueryWithCols) Release note: None

andreimatei force-pushed the sql.internal-executor branch from 5252ccc to b6bc2e3 Compare January 22, 2020 19:45

andreimatei mentioned this pull request Jan 22, 2020

sql: legacy InternalExecutor interface is dangerous; we should migrate away #44223

Open

andreimatei force-pushed the sql.internal-executor branch from b6bc2e3 to 6d9e5f1 Compare January 22, 2020 20:06

andreimatei force-pushed the sql.internal-executor branch from 6d9e5f1 to a636c92 Compare January 22, 2020 20:14

andreimatei commented Jan 22, 2020

View reviewed changes

andreimatei commented Jan 23, 2020

View reviewed changes

craig bot merged commit a636c92 into cockroachdb:master Jan 23, 2020

andreimatei deleted the sql.internal-executor branch January 29, 2020 18:21

Conversation

andreimatei commented Jan 21, 2020

Uh oh!

cockroach-teamcity commented Jan 21, 2020

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei commented Jan 21, 2020

Uh oh!

knz commented Jan 21, 2020

Uh oh!

knz commented Jan 21, 2020

Uh oh!

andreimatei commented Jan 22, 2020

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Jan 23, 2020

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Jan 23, 2020

Timed out (retrying...)

Uh oh!

craig bot commented Jan 23, 2020

Build failed (retrying...)

Uh oh!

craig bot commented Jan 23, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants