HTTP-REST sql query ID problem

[Alexey Kuznetsov-2]

Igniters,

I found that we have following problems with HTTP-REST sql query API.

After user execute sql query he will receive queryId to be able to fetch
next page.
See docs: https://apacheignite.readme.io/docs/rest-api#sql-query-execute

And current implementation of queryId is a long that simply incremented.

First problem:
  1. client1 execute query and get queryId = 1.
  2. node where query was executed is restarted (queryId generator
initialized to zero).
  3. client2 execute some query and also get queryId=1.
  4. client1 fetch next page for queryId=1 and GETS results of client2!!!!

Second problem:
  As queryId is generated sequentially it is very easy to brute force and
some client could get data of other clients too easy.

What we could do:
 1) Add nodeId to execute sql query response and fetch next page should
pass queryId + nodeId to get next page.
 2) Generate queryId as random long.

OR

Generate queryId as random UUID in this case it will be globally random, no
need for nodeId.

But I'm afraid this will break backward compatibility.

Thoughts?

--
Alexey Kuznetsov
GridGain Systems
www.gridgain.com

[dsetrakyan]

I am assuming we are storing the query IDs per client on the server side,
right? How about storing client ID on the server side as well, together
with the query-id and returning an error whenever the clientID does not
match?

On Wed, Nov 4, 2015 at 1:13 AM, Alexey Kuznetsov <[hidden email]>
wrote:

> Igniters,
>
> I found that we have following problems with HTTP-REST sql query API.
>
> After user execute sql query he will receive queryId to be able to fetch
> next page.
> See docs: https://apacheignite.readme.io/docs/rest-api#sql-query-execute
>
> And current implementation of queryId is a long that simply incremented.
>
> First problem:
>   1. client1 execute query and get queryId = 1.
>   2. node where query was executed is restarted (queryId generator
> initialized to zero).
>   3. client2 execute some query and also get queryId=1.
>   4. client1 fetch next page for queryId=1 and GETS results of client2!!!!
>
> Second problem:
>   As queryId is generated sequentially it is very easy to brute force and
> some client could get data of other clients too easy.
>
> What we could do:
>  1) Add nodeId to execute sql query response and fetch next page should
> pass queryId + nodeId to get next page.
>  2) Generate queryId as random long.
>
> OR
>
> Generate queryId as random UUID in this case it will be globally random, no
> need for nodeId.
>
> But I'm afraid this will break backward compatibility.
>
> Thoughts?
>
> --
> Alexey Kuznetsov
> GridGain Systems
> www.gridgain.com
>

[endianignite]

How about a sequence number pattern like in the FIX protocol? So the
restarted node carries on from where it left off. To make that work you
would also need a client id (equivalent to a CompId in FIX) to make each
request unique.
On 4 Nov 2015 09:13, "Alexey Kuznetsov" <[hidden email]> wrote:

> Igniters,
>
> I found that we have following problems with HTTP-REST sql query API.
>
> After user execute sql query he will receive queryId to be able to fetch
> next page.
> See docs: https://apacheignite.readme.io/docs/rest-api#sql-query-execute
>
> And current implementation of queryId is a long that simply incremented.
>
> First problem:
>   1. client1 execute query and get queryId = 1.
>   2. node where query was executed is restarted (queryId generator
> initialized to zero).
>   3. client2 execute some query and also get queryId=1.
>   4. client1 fetch next page for queryId=1 and GETS results of client2!!!!
>
> Second problem:
>   As queryId is generated sequentially it is very easy to brute force and
> some client could get data of other clients too easy.
>
> What we could do:
>  1) Add nodeId to execute sql query response and fetch next page should
> pass queryId + nodeId to get next page.
>  2) Generate queryId as random long.
>
> OR
>
> Generate queryId as random UUID in this case it will be globally random, no
> need for nodeId.
>
> But I'm afraid this will break backward compatibility.
>
> Thoughts?
>
> --
> Alexey Kuznetsov
> GridGain Systems
> www.gridgain.com
>

[Alexey Kuznetsov-2]

M G,

Could you please, give a link to FIX protocol description?
I would like to read about it to be on same page with you.

Thanks!

On Wed, Nov 4, 2015 at 4:17 PM, M G <[hidden email]> wrote:

> How about a sequence number pattern like in the FIX protocol? So the
> restarted node carries on from where it left off. To make that work you
> would also need a client id (equivalent to a CompId in FIX) to make each
> request unique.
> On 4 Nov 2015 09:13, "Alexey Kuznetsov" <[hidden email]> wrote:
>
> > Igniters,
> >
> > I found that we have following problems with HTTP-REST sql query API.
> >
> > After user execute sql query he will receive queryId to be able to fetch
> > next page.
> > See docs: https://apacheignite.readme.io/docs/rest-api#sql-query-execute
> >
> > And current implementation of queryId is a long that simply incremented.
> >
> > First problem:
> >   1. client1 execute query and get queryId = 1.
> >   2. node where query was executed is restarted (queryId generator
> > initialized to zero).
> >   3. client2 execute some query and also get queryId=1.
> >   4. client1 fetch next page for queryId=1 and GETS results of
> client2!!!!
> >
> > Second problem:
> >   As queryId is generated sequentially it is very easy to brute force and
> > some client could get data of other clients too easy.
> >
> > What we could do:
> >  1) Add nodeId to execute sql query response and fetch next page should
> > pass queryId + nodeId to get next page.
> >  2) Generate queryId as random long.
> >
> > OR
> >
> > Generate queryId as random UUID in this case it will be globally random,
> no
> > need for nodeId.
> >
> > But I'm afraid this will break backward compatibility.
> >
> > Thoughts?
> >
> > --
> > Alexey Kuznetsov
> > GridGain Systems
> > www.gridgain.com
> >
>

--
Alexey Kuznetsov
GridGain Systems
www.gridgain.com

[Alexey Kuznetsov-2]

Dmitriy,

I like idea of passing clientID, how it should look? UUID?

On Wed, Nov 4, 2015 at 4:16 PM, Dmitriy Setrakyan <[hidden email]>
wrote:

> I am assuming we are storing the query IDs per client on the server side,
> right? How about storing client ID on the server side as well, together
> with the query-id and returning an error whenever the clientID does not
> match?
>
> On Wed, Nov 4, 2015 at 1:13 AM, Alexey Kuznetsov <[hidden email]>
> wrote:
>
> > Igniters,
> >
> > I found that we have following problems with HTTP-REST sql query API.
> >
> > After user execute sql query he will receive queryId to be able to fetch
> > next page.
> > See docs: https://apacheignite.readme.io/docs/rest-api#sql-query-execute
> >
> > And current implementation of queryId is a long that simply incremented.
> >
> > First problem:
> >   1. client1 execute query and get queryId = 1.
> >   2. node where query was executed is restarted (queryId generator
> > initialized to zero).
> >   3. client2 execute some query and also get queryId=1.
> >   4. client1 fetch next page for queryId=1 and GETS results of
> client2!!!!
> >
> > Second problem:
> >   As queryId is generated sequentially it is very easy to brute force and
> > some client could get data of other clients too easy.
> >
> > What we could do:
> >  1) Add nodeId to execute sql query response and fetch next page should
> > pass queryId + nodeId to get next page.
> >  2) Generate queryId as random long.
> >
> > OR
> >
> > Generate queryId as random UUID in this case it will be globally random,
> no
> > need for nodeId.
> >
> > But I'm afraid this will break backward compatibility.
> >
> > Thoughts?
> >
> > --
> > Alexey Kuznetsov
> > GridGain Systems
> > www.gridgain.com
> >
>

--
Alexey Kuznetsov
GridGain Systems
www.gridgain.com

[dsetrakyan]

On Wed, Nov 4, 2015 at 1:27 AM, Alexey Kuznetsov <[hidden email]>
wrote:

> Dmitriy,
>
> I like idea of passing clientID, how it should look? UUID?
>

I think Michael has suggested similar approach. I am not sure we need to
pass the clientID with the message, given that we should know who sent the
message anyway. ClientID can be the ID of the client node, which is UUID.

D.

>
> On Wed, Nov 4, 2015 at 4:16 PM, Dmitriy Setrakyan <[hidden email]>
> wrote:
>
> > I am assuming we are storing the query IDs per client on the server side,
> > right? How about storing client ID on the server side as well, together
> > with the query-id and returning an error whenever the clientID does not
> > match?
> >
> > On Wed, Nov 4, 2015 at 1:13 AM, Alexey Kuznetsov <
> [hidden email]>
> > wrote:
> >
> > > Igniters,
> > >
> > > I found that we have following problems with HTTP-REST sql query API.
> > >
> > > After user execute sql query he will receive queryId to be able to
> fetch
> > > next page.
> > > See docs:
> https://apacheignite.readme.io/docs/rest-api#sql-query-execute
> > >
> > > And current implementation of queryId is a long that simply
> incremented.
> > >
> > > First problem:
> > >   1. client1 execute query and get queryId = 1.
> > >   2. node where query was executed is restarted (queryId generator
> > > initialized to zero).
> > >   3. client2 execute some query and also get queryId=1.
> > >   4. client1 fetch next page for queryId=1 and GETS results of
> > client2!!!!
> > >
> > > Second problem:
> > >   As queryId is generated sequentially it is very easy to brute force
> and
> > > some client could get data of other clients too easy.
> > >
> > > What we could do:
> > >  1) Add nodeId to execute sql query response and fetch next page should
> > > pass queryId + nodeId to get next page.
> > >  2) Generate queryId as random long.
> > >
> > > OR
> > >
> > > Generate queryId as random UUID in this case it will be globally
> random,
> > no
> > > need for nodeId.
> > >
> > > But I'm afraid this will break backward compatibility.
> > >
> > > Thoughts?
> > >
> > > --
> > > Alexey Kuznetsov
> > > GridGain Systems
> > > www.gridgain.com
> > >
> >
>
>
>
> --
> Alexey Kuznetsov
> GridGain Systems
> www.gridgain.com
>

[endianignite]

Alexey,

No problem, here is a link that is relatively simple to understand :
http://javarevisited.blogspot.co.uk/2011/02/fix-protocol-session-or-admin-messages.html

However, a simplified approach of just adding a client ID seems sufficient - the sequence number reset functionality of FIX is overly complex for this requirement.

Regards
Mike

[yzhdanov]

We can use IgniteUuid qryId = IgniteUuid.fromUuid(clientId);

and restrict page requests for queries submitted from other clients. I.e.
throw exception if qryId.globalId() != clientId

--Yakov

2015-11-04 12:21 GMT+03:00 endian675 <[hidden email]>:

> Alexey,
>
> No problem, here is a link that is relatively simple to understand :
>
> http://javarevisited.blogspot.co.uk/2011/02/fix-protocol-session-or-admin-messages.html
>
> However, a simplified approach of just adding a client ID seems sufficient
> -
> the sequence number reset functionality of FIX is overly complex for this
> requirement.
>
> Regards
> Mike
>
>
>
> --
> View this message in context:
> http://apache-ignite-developers.2346864.n4.nabble.com/HTTP-REST-sql-query-ID-problem-tp4524p4531.html
> Sent from the Apache Ignite Developers mailing list archive at Nabble.com.
>

[dsetrakyan]

On Wed, Nov 4, 2015 at 2:46 AM, Yakov Zhdanov <[hidden email]> wrote:

> We can use IgniteUuid qryId = IgniteUuid.fromUuid(clientId);
>
> and restrict page requests for queries submitted from other clients. I.e.
> throw exception if qryId.globalId() != clientId
>

Yakov, I think it is inconvenient to pass UUID in a query string. How about
we use node order, defined by ClusterNode.order() method? Essentially,
instead of passing just the queryID, REST client will also pass the
nodeOrder parameter.

On the server side, we check that the received node order should be equal
to the local node order. If not, then error. This approach will have the
same behavior we do right now, and will also fix the bug mentioned by
Alexey.

> --Yakov
>
> 2015-11-04 12:21 GMT+03:00 endian675 <[hidden email]>:
>
> > Alexey,
> >
> > No problem, here is a link that is relatively simple to understand :
> >
> >
> http://javarevisited.blogspot.co.uk/2011/02/fix-protocol-session-or-admin-messages.html
> >
> > However, a simplified approach of just adding a client ID seems
> sufficient
> > -
> > the sequence number reset functionality of FIX is overly complex for this
> > requirement.
> >
> > Regards
> > Mike
> >
> >
> >
> > --
> > View this message in context:
> >
> http://apache-ignite-developers.2346864.n4.nabble.com/HTTP-REST-sql-query-ID-problem-tp4524p4531.html
> > Sent from the Apache Ignite Developers mailing list archive at
> Nabble.com.
> >
>

[Sergi]

Guys,

It looks like that if we need state on server side it is not REST anymore.
We really need user sessions.
Everything else you will invent here is just a hack.

Sergi

2015-11-05 1:31 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:

> On Wed, Nov 4, 2015 at 2:46 AM, Yakov Zhdanov <[hidden email]> wrote:
>
> > We can use IgniteUuid qryId = IgniteUuid.fromUuid(clientId);
> >
> > and restrict page requests for queries submitted from other clients. I.e.
> > throw exception if qryId.globalId() != clientId
> >
>
> Yakov, I think it is inconvenient to pass UUID in a query string. How about
> we use node order, defined by ClusterNode.order() method? Essentially,
> instead of passing just the queryID, REST client will also pass the
> nodeOrder parameter.
>
> On the server side, we check that the received node order should be equal
> to the local node order. If not, then error. This approach will have the
> same behavior we do right now, and will also fix the bug mentioned by
> Alexey.
>
>
> > --Yakov
> >
> > 2015-11-04 12:21 GMT+03:00 endian675 <[hidden email]>:
> >
> > > Alexey,
> > >
> > > No problem, here is a link that is relatively simple to understand :
> > >
> > >
> >
> http://javarevisited.blogspot.co.uk/2011/02/fix-protocol-session-or-admin-messages.html
> > >
> > > However, a simplified approach of just adding a client ID seems
> > sufficient
> > > -
> > > the sequence number reset functionality of FIX is overly complex for
> this
> > > requirement.
> > >
> > > Regards
> > > Mike
> > >
> > >
> > >
> > > --
> > > View this message in context:
> > >
> >
> http://apache-ignite-developers.2346864.n4.nabble.com/HTTP-REST-sql-query-ID-problem-tp4524p4531.html
> > > Sent from the Apache Ignite Developers mailing list archive at
> > Nabble.com.
> > >
> >
>

[dsetrakyan]

On Wed, Nov 4, 2015 at 10:50 PM, Sergi Vladykin <[hidden email]>
wrote:

> Guys,
>
> It looks like that if we need state on server side it is not REST anymore.
> We really need user sessions.
> Everything else you will invent here is just a hack.
>

Sergi, we already support nodeLocal state per query with REST. Not ideal,
but it works. Alexey K. noticed a bug in this implementation. All we need
to do is pick one of many proposed solutions and fix it.

>
> Sergi
>
> 2015-11-05 1:31 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
>
> > On Wed, Nov 4, 2015 at 2:46 AM, Yakov Zhdanov <[hidden email]>
> wrote:
> >
> > > We can use IgniteUuid qryId = IgniteUuid.fromUuid(clientId);
> > >
> > > and restrict page requests for queries submitted from other clients.
> I.e.
> > > throw exception if qryId.globalId() != clientId
> > >
> >
> > Yakov, I think it is inconvenient to pass UUID in a query string. How
> about
> > we use node order, defined by ClusterNode.order() method? Essentially,
> > instead of passing just the queryID, REST client will also pass the
> > nodeOrder parameter.
> >
> > On the server side, we check that the received node order should be equal
> > to the local node order. If not, then error. This approach will have the
> > same behavior we do right now, and will also fix the bug mentioned by
> > Alexey.
> >
> >
> > > --Yakov
> > >
> > > 2015-11-04 12:21 GMT+03:00 endian675 <[hidden email]>:
> > >
> > > > Alexey,
> > > >
> > > > No problem, here is a link that is relatively simple to understand :
> > > >
> > > >
> > >
> >
> http://javarevisited.blogspot.co.uk/2011/02/fix-protocol-session-or-admin-messages.html
> > > >
> > > > However, a simplified approach of just adding a client ID seems
> > > sufficient
> > > > -
> > > > the sequence number reset functionality of FIX is overly complex for
> > this
> > > > requirement.
> > > >
> > > > Regards
> > > > Mike
> > > >
> > > >
> > > >
> > > > --
> > > > View this message in context:
> > > >
> > >
> >
> http://apache-ignite-developers.2346864.n4.nabble.com/HTTP-REST-sql-query-ID-problem-tp4524p4531.html
> > > > Sent from the Apache Ignite Developers mailing list archive at
> > > Nabble.com.
> > > >
> > >
> >
>