[tarantool-patches] Re: [PATCH v2] sql: check read access while executing SQL query

[Kirill Yukhin <kyukhin@tarantool.org>]

Hello Nikita,

Thanks for review. My answers inlined.

On 24 авг 12:50, n.pettik wrote:
> 
> > On 22 Aug 2018, at 09:53, Kirill Yukhin <kyukhin@tarantool.org> wrote:
> > 
> > Since SQL front-end is not using box API anymore,
> 
> Basically, AFAIK selects have never checked access privileges.
> Box API was used only to handle DML routine.

I'll update description.

> > no checkes for read access are performed by VDBE engine.
> > Fix this by introducing dedicated opcode which is executed
> > to perform read access check.
> 
> Please, mention that new opcode is generated for EACH
> OP_OpenWrite/Read appearance in VDBE program.

There'll be no new opcode at all. See below.
 
> > diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> > index da7668b..c25715c 100644
> > --- a/src/box/sql/build.c
> > +++ b/src/box/sql/build.c
> > @@ -55,6 +55,38 @@
> > +static bool
> > +sql_emit_access_checks(struct Vdbe *v)
> 
> AFAIR we decided to use vdbe_emit_ prefix for
> functions generating VDBE code.
> 
> Moreover, according to codestyle:
> 
> ‘''
> If the name of a function is an action or an imperative command,
> the function should return an error-code integer.  If the name
> is a predicate, the function should return a "succeeded" boolean.
> ‘’’
> Name of your function doesn’t seem to be predicate…
> 
> I don't insist on this rule, tho (but smb else might).

You are right.

> > +{
> > +	bool is_code_emitted = false;
> > +	for(int i = 0; i < v->nOp; ++i) {
> > +		uint8_t op = v->aOp[i].opcode;
> > +		/* FIXME: when write iterators will be removed,
> 
> Nitpicking: comment should be like:
> 
> /*
>  * TEXT STARTS HERE
>  */

Ditto.

> > +		 * remove checks for OP_OpenWrite. Note, that
> > +		 * DML routines don't need access checks.
> > +		 * See gh-3182.
> 
> Nitpicking: AFAIR references to issues are not desirable in source code.

Ditto.

> > +		 */
> > +		if (op == OP_OpenRead || op == OP_OpenWrite) {
> > +			sqlite3VdbeAddOp4(v, OP_AccessCheck,
> > +					  PRIV_R,
> > +					  0,
> > +					  0,
> 
> Smth wrong with indentation.

Ditto.

> > +					  (void *)v->aOp[i].p4.space,
> > +					  P4_SPACEPTR);
> > +			is_code_emitted = true;
> 
> Why do we need to generate these opcodes at the end of program?
> Please, leave explanation comment.
> Moreover, you emit OP_AccessCheck on each opcode, even if we
> have already checked access on this space. It may make sense in case
> you emit OP_AccessCheck opcode right before cursor opening, but
> in the beginning of VDBE execution it is wasting execution time.
> 
> E.g.:
> 
> SQL-trace: select * from t1, t1 as t2
>  101>   13 AccessCheck      1    0    0 space<name=T1> 00 Check 1 access rights for space<name=T1>.
>  101>   14 AccessCheck      1    0    0 space<name=T1> 00 Check 1 access rights for space<name=T1>.
>  101>   15 Goto             0    1    0               00 
>  101>    1 OpenWrite        0    0    0 space<name=T1> 00 index id = 0, space ptr = space<name=T1>; T1
>  101>    2 OpenWrite        1    0    0 space<name=T1> 00 index id = 0, space ptr = space<name=T1>; T1
>  101>    3 Explain          0    0    0 SCAN TABLE T1 00 
>  101>    4 Rewind           0   12    0               00 
>  101>   12 Halt 
> 
> One AccessCheck is definitely redundant.

I'd call it a prologue, not begin or end. Initial (v1) version of the patch
supported dedicated hash which was used to emit AccessCheck only once, but
@kostya rejected it saying that this is over-engineering.

After some re-thinking and taking that statement into account I've come up to
following idea: we do not need new opcode at all, we'll use OpenRead/OpenWrite
to do the check. When OpenWrite will be eliminated w/ #3182, access will be
performed for OpenRead opcode, which is our ultimate goal.

So, I'll resubmit v3 version of the patch.

> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -5286,6 +5286,21 @@ case OP_IncMaxid: {
> > 	break;
> > }
> > 
> > +/* Opcode: AccessCheck P1 * * P4 *
> > + * Synopsis: Check P1 access rights for P4.
> > + *
> > + * Check access rights for space pointed by register P1.
> 
> *Pointer by register P4*

Not relevant anymore.

> > diff --git a/test/sql/gh-2362-select-access-rights.test.lua b/test/sql/gh-2362-select-access-rights.test.lua
> > new file mode 100644
> > index 0000000..d5d0a8e
> > --- /dev/null
> > +++ b/test/sql/gh-2362-select-access-rights.test.lua
> > @@ -0,0 +1,18 @@
> > +test_run = require('test_run').new()
> > +engine = test_run:get_cfg('engine')
> > +nb = require('net.box')
> > +
> > +box.sql.execute("PRAGMA sql_default_engine='"..engine.."'")
> > +box.sql.execute("CREATE TABLE te37 (s1 INT PRIMARY KEY);")
> > +box.sql.execute("INSERT INTO te37 VALUES (1);")
> > +
> > +box.schema.user.grant('guest','read,write,execute','universe’)
> 
> Why do you give all privileges? To make this test be fair, you should
> give only ‘read’ access. I would also add tests with join involving several
> tables to make sure that read access is tested for all spaces.

Fixed. Case added.
 
> Next question is about SELECT from views: user gives view read privilege,
> but will get error when try to select from that view.

Case added.

> The last one - what should we do with FK: to check constraint violations
> we must implicitly open read cursor on parent space. Hence, user again
> would get ‘access denied’ error if he tried to insert in child space.

Case added.

> I propose to investiagate how these things are handled in other DBs.

I think there's nothing to investigate here: if view is referencing to
a table access to which is denied - then error should be reported and no
data retrieved from the table.

--
Regards, Kirill Yukhin