Skip to content

Secure nextUri with a slug#13110

Merged
rschlussel merged 1 commit intoprestodb:masterfrom
rschlussel:nexturi_fix
Jul 23, 2019
Merged

Secure nextUri with a slug#13110
rschlussel merged 1 commit intoprestodb:masterfrom
rschlussel:nexturi_fix

Conversation

@rschlussel
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown

@mayankgarg1990 mayankgarg1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me and @wenleix were actually talking about something similar for worker communication to avoid people from guessing the worker URL to fetch intermediate data.

Copy link
Copy Markdown
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % please wait for the build to pass

@rschlussel
Copy link
Copy Markdown
Contributor Author

  1. not sure what's going on with that test, doesn't seem like a related failure, but I've restarted it 3 or 4 times at this point
  2. @rongrong raised the point that it would be more secure to generate a new slug for each request. That way if the client leaks the request uri (e.g. logs it), you still won't be able to access the rest of the results. Implemenation-wise i think that means updating the slug in createNextResultsUri (and synchronizing it to keep it threadsafe). Does anybody have concerns with this approach?

@mayankgarg1990
Copy link
Copy Markdown

Re: new slug every time - as of now - if somehow there is an out of order request we would end up getting "query not found" as getQuery() would return a null. As per my look at the code - if you issued the same request again (maybe client side retry) - we would just return the same result set:

        // is the a repeated request for the last results?
        String requestedPath = uriInfo.getAbsolutePath().getPath();
        if (requestedPath.equals(lastResultPath)) {
            if (submissionFuture.isDone()) {
                // tell query manager we are still interested in the query
                queryManager.recordHeartbeat(queryId);
            }
            return Optional.of(lastResult);
        }

In my opinion we should maintain this behavior with the new implementation as well

@rschlussel
Copy link
Copy Markdown
Contributor Author

You already can't get results out of order, but I didn't think about getting the same result twice. Seems reasonable to leave as is for now. We can do more investigation about whether it's worth breaking this feature.

@rschlussel
Copy link
Copy Markdown
Contributor Author

We could also cache the previous slug, though for that case.

@rongrong
Copy link
Copy Markdown
Contributor

This PR is strictly better than before so I don't see reasons to block it. But if the client logs the url then a unique slug per query is not very helpful. We can improve that in a separate PR though if people feel more discussions are needed.

@rschlussel rschlussel merged commit c269470 into prestodb:master Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants