From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 5BE232D64D for ; Fri, 12 Oct 2018 07:41:15 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JaTQSYCI947l for ; Fri, 12 Oct 2018 07:41:15 -0400 (EDT) Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 196992D639 for ; Fri, 12 Oct 2018 07:41:15 -0400 (EDT) Date: Fri, 12 Oct 2018 14:41:09 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH] sql: check read access while executing SQL query Message-ID: <20181012114109.fdvmjko63x2zpdlh@tarantool.org> References: <6dc6ee8b0356676cf6176dd6310109a09ce7b23d.1539247804.git.kyukhin@tarantool.org> <4D536713-510B-4F25-9455-F4090A29AF67@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4D536713-510B-4F25-9455-F4090A29AF67@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" Cc: tarantool-patches@freelists.org Hello Nikita, Thanks a lot for review! My answers inlined. Re-factored patchset send as new thread. Branch force-pushed. On 11 Oct 14:32, n.pettik wrote: > > > Since SQL front-end is not using box API, > > no checkes for read access are performed by VDBE engine. > > Fix this by introducing dedicated opcode which is executed > > But in your patch I don’t see any new opcodes. Fixed comment. > > to perform read access check. > > Note, that there's is no need to perform DML/DDL checkes as > > they're performed by Tarantool's core. > > > > Closes #2362 > > Please, provide doc-bot request (especially taking into consideration > the fact that ‘read’ privilege turns out to be not real ’select’ privilege). Done. > > +box.sql.execute("PRAGMA sql_default_engine='"..engine.."'") > > +box.sql.execute("CREATE TABLE t1 (s1 INT PRIMARY KEY, s2 INT UNIQUE);") > > +box.sql.execute("CREATE TABLE t2 (s1 INT PRIMARY KEY);") > > +box.sql.execute("INSERT INTO t1 VALUES (1, 1);") > > +box.sql.execute("INSERT INTO t2 VALUES (1);") > > + > > +box.schema.user.grant('guest','read', 'space', '_space’) > > Hmm, why do you need to grand read privilege to _space? > Without it test fails: > > error: 'Failed to execute SQL statement: no such table: T1’ > > I guess somewhere in SQL internals invocation to _space appears to fetch space, > but , it is not obvious. Can you come up with any workaround to avoid it? > > Another problem is: > > box.schema.user.revoke('guest’,'read', 'space', 'T1') > box.schema.user.grant('guest’,’write', 'space', 'T1') > c:execute("delete from t1 where s1 = 1;”) > --- > - error: 'Failed to execute SQL statement: Read access to space ''T1'' is denied for > user ''guest''' > ... > > In terms of Tarantool, it is OK (since we need to iterate through the space to find > tuple to be deleted). But for SQL I guess it is not acceptable: this query doesn’t > give user any data, so read (i.e. SELECT) privilege is not needed. > The same for example involving FK constraints which you provided below. I've refactored space pointer retrival mechanism. It is now used space name -> pointer hash table w/o any access checks. Actual checks are only performed when iterator op-code is executed. -- Regards, Kirill Yukhin