You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/03/10 21:17:07 UTC
[kudu-CR] KUDU-1918. Prevent hijacking of scanners by other users
Hello Dan Burkert, David Ribeiro Alves,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6348
to review the following change.
Change subject: KUDU-1918. Prevent hijacking of scanners by other users
......................................................................
KUDU-1918. Prevent hijacking of scanners by other users
This makes the scanner remember its RemoteUser, and ensures that when
continuing a scan, the new requestor matches the original requestor.
This prevents one user from somehow obtaining a scanner ID from another
and then "hijacking" the in-progress scan.
Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
---
M src/kudu/tserver/scanners-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
M src/kudu/tserver/tserver-path-handlers.cc
7 files changed, 73 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/1
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] KUDU-1918. Prevent hijacking of scanners by other users
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1918. Prevent hijacking of scanners by other users
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6348/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
PS1, Line 1664: // Check that the user requesting the scanner is the same one who started it, to
: // avoid the possibility of scanner hijacking.
: if (PREDICT_FALSE(scanner->remote_user().username() != rpc_context->remote_user().username())) {
: LOG(WARNING) << "User " << rpc_context->requestor_string() << " attempted to access a scanner "
: << "started by user " << scanner->remote_user().username();
: return Status::NotAuthorized("scanner not owned by requestor");
: }
should we do this inside of ScannerManager, by making LookupSacnner return a Status, so that we get this automatically for all lookups? or would that be non-trivial? I know that keepalive not checking this isn't important, thinking of get, when/if we do it.
Feel free to ignore or leave a TODO if it makes sense.
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#2) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
KUDU-1918 Prevent hijacking of scanner IDs
This makes the scanner remember its RemoteUser, and ensures that when
continuing a scan, the new requestor matches the original requestor.
This prevents one user from somehow obtaining a scanner ID from another
and then "hijacking" the in-progress scan.
Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
---
M src/kudu/tserver/scanners-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver_path_handlers.cc
8 files changed, 110 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/2
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 6:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/6348/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/6348/6//COMMIT_MSG@15
PS6, Line 15: scans
Including fault tolerant ones, right?
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/client/client-test.cc@5753
PS6, Line 5753: Open
Can you test with a fault tolerant scan as well?
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/scanners-test.cc
File src/kudu/tserver/scanners-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/scanners-test.cc@45
PS6, Line 45: using rpc::RemoteUser;
nit: move it to L44.
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/scanners.h
File src/kudu/tserver/scanners.h:
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/scanners.h@69
PS6, Line 69: The client
nit: mention it has to be the same client.
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/scanners.h@82
PS6, Line 82: // Create a new scanner with a unique ID, inserting it into the map.
nit: update it to reflect how 'remote_user' is used.
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/tablet_server-test.cc@3406
PS6, Line 3406: TEST_F(TabletServerTest, TestScannerCheckMatchingUser) {
nit: add a doc for this?
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/tablet_service.cc@1302
PS6, Line 1302: PREDICT_FALSE
I think it is probably rare to get an NOT_AUTHORIZED error, but do you think it is rare to get SCANNER_EXPIRED error as well?
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 23:15:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#6) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
KUDU-1918 Prevent hijacking of scanner IDs
This makes the scanner remember its RemoteUser, and ensures that when
continuing a scan, the new requestor matches the original requestor.
This prevents one user from somehow obtaining a scanner ID from another
and then "hijacking" the in-progress scan.
This restricts scans, checksum scans, and keep-alive requests.
Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/tserver/scanners-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
M src/kudu/tserver/tserver_path_handlers.cc
13 files changed, 265 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/6
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 05 Nov 2018 17:44:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#4) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
KUDU-1918 Prevent hijacking of scanner IDs
This makes the scanner remember its RemoteUser, and ensures that when
continuing a scan, the new requestor matches the original requestor.
This prevents one user from somehow obtaining a scanner ID from another
and then "hijacking" the in-progress scan.
Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
---
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/tserver/scanners-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
M src/kudu/tserver/tserver_path_handlers.cc
11 files changed, 207 insertions(+), 60 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/4
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc
File src/kudu/tserver/tserver_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc@540
PS3, Line 540: json->Set("remote user", scan.remote_user.username());
> This is going to break the web UI; there should be a corresponding change i
woops, this is wrong. You just need to make the corresponding change in scans.mustache. The title of the column is already hardcoded to 'Requestor', so that won't change.
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 18:01:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc
File src/kudu/tserver/tserver_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc@540
PS3, Line 540: json->Set("remote user", scan.remote_user.username());
> woops, this is wrong. You just need to make the corresponding change in sc
Also add an underscore, I think that's the prevailing style for our JSON keys.
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 18:01:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#5) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
KUDU-1918 Prevent hijacking of scanner IDs
This makes the scanner remember its RemoteUser, and ensures that when
continuing a scan, the new requestor matches the original requestor.
This prevents one user from somehow obtaining a scanner ID from another
and then "hijacking" the in-progress scan.
Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/tserver/scanners-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
M src/kudu/tserver/tserver_path_handlers.cc
13 files changed, 262 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/5
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 3:
> One of Todd's comments from KUDU-1843 was:
>
> Caching the original username turns out to be a little tricky, since the WAL doesn't record the original username, and thus when reconstructing the request cache during tablet bootstrap we don't have enough information to do so. I think making the UUIDs unpredictable is probably a better approach.
>
> That's still an issue, no?
My mistake; I had followed KUDU-1918 to KUDU-1843, and didn't realize that the conversation shifted to talking about _writes_ (which are cached in the request cache). Scans aren't cached, so this isn't an issue here.
KUDU-1843 remains at large.
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 17:52:18 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test-base.h
File src/kudu/tserver/tablet_server-test-base.h:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test-base.h@125
PS3, Line 125: Status FillNewScanRequest(ReadMode read_mode, NewScanRequestPB* scan) const;
> Could it be a static method (or at least const)?
Done, const since it's using `schema_`
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3400
PS3, Line 3400: // bytes should be read by the BM if storing keys in the rowset metadata).
> Is it worth adding a higher-level test to make sure this works as expected?
I'll keep this in mind for further authz testing.
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3407
PS3, Line 3407: uy");
> Does it make sense to add a scenario to verify that without user_credential
Done
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc@1292
PS3, Line 1292: void TabletServiceImpl::ScannerKeepAlive(const ScannerKeepAliveRequestPB *req,
> I'd suggest going ahead and doing this as a part of this change, unless it'
Done
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc@2066
PS3, Line 2066: &scanner);
> Why not populate *error_code with something more detailed? Is UNKNOWN_ERROR
Done
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc
File src/kudu/tserver/tserver_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc@540
PS3, Line 540: json->Set("requestor", scan.remote_user.username());
> Also add an underscore, I think that's the prevailing style for our JSON ke
Hrm, I think I'll keep requestor, even if it isn't exactly consistent; in the user-facing context of a scan, it's clear what a Requestor is.
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 22:17:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 8:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/client/client-test.cc@5765
PS7, Line 5765: ASSERT_OK(bad_guy_scanner.Open());
> nit: does it make sense to test the behavior of the KuduScanner::Open() met
Hrm, I'm not sure what exactly it would be testing, but I can tell you what would happen: the call to Open() would correctly buffer some rows (since the scanner is correctly authenticated). Then, we would try to hijack a scanner id that we're not suppose to have, and upon calling NextBatch(), we would correctly see those rows buffered at Open().
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc
File src/kudu/tserver/scanners.cc:
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc@168
PS7, Line 168: if (username != ret->remote_user().username()) {
: *error_code = TabletServerErrorPB::NOT_AUTHORIZED;
: return Status::NotAuthorized(Substitute("User $0 doesn't own scanner $1",
: username, scanner_id));
: }
> nit: do we prefer exposing the fact that the scanner exists but the caller
Interesting question; I don't think so because scanner ids AFAICT aren't treated as sensitive information.
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc@173
PS7, Line 173: std
> nit: wrap into std::move() ?
Done
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/tablet_server-test.cc@3406
PS7, Line 3406: te
> nit: them ?
Rephrased slightly
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/tablet_server-test.cc@3437
PS7, Line 3437: const
> nit: might it be constexpr?
Done
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 8
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 06 Nov 2018 18:23:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc@1292
PS3, Line 1292: // TODO(awong): consider validating that the scanner was created by the
I'd suggest going ahead and doing this as a part of this change, unless it's a large change in its own right.
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc
File src/kudu/tserver/tserver_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc@540
PS3, Line 540: json->Set("remote user", scan.remote_user.username());
This is going to break the web UI; there should be a corresponding change in scans.mustache. I wouldn't change it though, I think 'remote user' is not as clear as 'requestor' (although I think 'user' might be better than both).
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 18:00:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3400
PS3, Line 3400: // bytes should be read by the BM if storing keys in the rowset metadata).
> On second thought, this could probably use a scanner-side test.
Done
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 01:35:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 8: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/client/client-test.cc@5765
PS7, Line 5765: ASSERT_OK(bad_guy_scanner.Open());
> Hrm, I'm not sure what exactly it would be testing, but I can tell you what
Hrm, SGTM: I just wanted to make sure the client code behaves as expected, i.e. at some point it would return Status::RemoteError(). After some consideration, I think that's not crucial to test since the whole point of this test is to make sure the server-side behavior (not the client-side) is correct.
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc
File src/kudu/tserver/scanners.cc:
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc@168
PS7, Line 168: if (username != ret->remote_user().username()) {
: *error_code = TabletServerErrorPB::NOT_AUTHORIZED;
: return Status::NotAuthorized(Substitute("User $0 doesn't own scanner $1",
: username, scanner_id));
: }
> Interesting question; I don't think so because scanner ids AFAICT aren't tr
SGTM
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 8
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 06 Nov 2018 19:15:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6348/1/src/kudu/tserver/scanners.cc
File src/kudu/tserver/scanners.cc:
http://gerrit.cloudera.org:8080/#/c/6348/1/src/kudu/tserver/scanners.cc@29
PS1, Line 29: #include "kudu/common/scan_spec.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/6348/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/6348/1/src/kudu/tserver/tablet_service.cc@1664
PS1, Line 1664: RETURN_NOT_OK_PREPEND(EncodedKey::DecodeEncodedString(tablet_schema, scanner->arena(),
: scan_pb.last_primary_key(), &start),
: "Failed to decode last primary key");
: // Increment the start key, so we don't return the last row again.
: RETURN_NOT_OK_PREPEND(EncodedKey::IncrementEncodedKey(tablet_schema, &start, scanner->arena()),
: "Failed to increment encoded last row key");
: }
> should we do this inside of ScannerManager, by making LookupSacnner return
Added a TODO for this in KeepAlive, since it's the only other place we'd do this atm
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 15:48:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
KUDU-1918 Prevent hijacking of scanner IDs
This makes the scanner remember its RemoteUser, and ensures that when
continuing a scan, the new requestor matches the original requestor.
This prevents one user from somehow obtaining a scanner ID from another
and then "hijacking" the in-progress scan.
This restricts scans, checksum scans, and keep-alive requests.
Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Reviewed-on: http://gerrit.cloudera.org:8080/6348
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/tserver/scanners-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
M src/kudu/tserver/tserver_path_handlers.cc
13 files changed, 272 insertions(+), 56 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 9
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3400
PS3, Line 3400: // bytes should be read by the BM if storing keys in the rowset metadata).
> I'll keep this in mind for further authz testing.
On second thought, this could probably use a scanner-side test.
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 22:58:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6348/2/src/kudu/tserver/scanners.cc
File src/kudu/tserver/scanners.cc:
http://gerrit.cloudera.org:8080/#/c/6348/2/src/kudu/tserver/scanners.cc@145
PS2, Line 145: remote_user,
> warning: 'remote_user' used after it was moved [bugprone-use-after-move]
Done
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 16:31:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 5:
Fixed IWYU
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 02:24:11 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 7: Code-Review+2
(5 comments)
LGTM, just a few nits.
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/client/client-test.cc@5765
PS7, Line 5765: ASSERT_OK(bad_guy_scanner.Open());
nit: does it make sense to test the behavior of the KuduScanner::Open() method in the case when the scanner tries to pre-fetch some rows (i.e. set the initial batch size to non-zero and see how it works)?
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc
File src/kudu/tserver/scanners.cc:
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc@168
PS7, Line 168: if (username != ret->remote_user().username()) {
: *error_code = TabletServerErrorPB::NOT_AUTHORIZED;
: return Status::NotAuthorized(Substitute("User $0 doesn't own scanner $1",
: username, scanner_id));
: }
nit: do we prefer exposing the fact that the scanner exists but the caller is not authorized to access it versus reporting NotFound error and logging into the log about non-authorized attempt to access the scanner?
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/scanners.cc@173
PS7, Line 173: ret
nit: wrap into std::move() ?
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/tablet_server-test.cc@3406
PS7, Line 3406: it
nit: them ?
http://gerrit.cloudera.org:8080/#/c/6348/7/src/kudu/tserver/tablet_server-test.cc@3437
PS7, Line 3437: const
nit: might it be constexpr?
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 06 Nov 2018 02:56:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/6348/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/6348/6//COMMIT_MSG@15
PS6, Line 15: scans
> Including fault tolerant ones, right?
Right. I'm not going to call that out specifically since fault-tolerant scans and non-fault-tolerant scans will be caught by the same endpoint.
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/client/client-test.cc@5753
PS6, Line 5753: the
> Can you test with a fault tolerant scan as well?
Done
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/scanners-test.cc
File src/kudu/tserver/scanners-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/scanners-test.cc@45
PS6, Line 45:
> nit: move it to L44.
Done
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/scanners.h
File src/kudu/tserver/scanners.h:
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/scanners.h@69
PS6, Line 69: The client
> nit: mention it has to be the same client.
It doesn't have to be the same client ID. It just has to be the same user.
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/scanners.h@82
PS6, Line 82: // Create a new scanner with a unique ID, inserting it into the map.
> nit: update it to reflect how 'remote_user' is used.
Done
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/tablet_server-test.cc@3406
PS6, Line 3406: // Test that scanners can only be accessed by the user who created it.
> nit: add a doc for this?
Done
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/6348/6/src/kudu/tserver/tablet_service.cc@1302
PS6, Line 1302: !s.ok()) {
> I think it is probably rare to get an NOT_AUTHORIZED error, but do you thin
Yeah good point. Done
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 03 Nov 2018 00:38:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#8) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
KUDU-1918 Prevent hijacking of scanner IDs
This makes the scanner remember its RemoteUser, and ensures that when
continuing a scan, the new requestor matches the original requestor.
This prevents one user from somehow obtaining a scanner ID from another
and then "hijacking" the in-progress scan.
This restricts scans, checksum scans, and keep-alive requests.
Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/tserver/scanners-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
M src/kudu/tserver/tserver_path_handlers.cc
13 files changed, 272 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/8
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 8
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test-base.h
File src/kudu/tserver/tablet_server-test-base.h:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test-base.h@125
PS3, Line 125: Status FillNewScanRequest(ReadMode read_mode, NewScanRequestPB* scan);
Could it be a static method (or at least const)?
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3400
PS3, Line 3400: TEST_F(TabletServerTest, TestScannerCheckMatchingUser) {
Is it worth adding a higher-level test to make sure this works as expected? It seems to be too early at this phase, but after the authz token stuff is there, it might make sense.
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3407
PS3, Line 3407: different user
Does it make sense to add a scenario to verify that without user_credentials set for the scanner it's not possible to re-use the original scanner?
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 17:28:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#3) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
KUDU-1918 Prevent hijacking of scanner IDs
This makes the scanner remember its RemoteUser, and ensures that when
continuing a scan, the new requestor matches the original requestor.
This prevents one user from somehow obtaining a scanner ID from another
and then "hijacking" the in-progress scan.
Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
---
M src/kudu/tserver/scanners-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver_path_handlers.cc
8 files changed, 110 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/3
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
Patch Set 3:
(1 comment)
One of Todd's comments from KUDU-1843 was:
Caching the original username turns out to be a little tricky, since the WAL doesn't record the original username, and thus when reconstructing the request cache during tablet bootstrap we don't have enough information to do so. I think making the UUIDs unpredictable is probably a better approach.
That's still an issue, no?
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc@2066
PS3, Line 2066: return Status::NotAuthorized(Substitute("User $0 requested scanner it doesn't own", requestor));
Why not populate *error_code with something more detailed? Is UNKNOWN_ERROR the best we can do?
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 17:17:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#7) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
......................................................................
KUDU-1918 Prevent hijacking of scanner IDs
This makes the scanner remember its RemoteUser, and ensures that when
continuing a scan, the new requestor matches the original requestor.
This prevents one user from somehow obtaining a scanner ID from another
and then "hijacking" the in-progress scan.
This restricts scans, checksum scans, and keep-alive requests.
Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/tserver/scanners-test.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
M src/kudu/tserver/tserver_path_handlers.cc
13 files changed, 272 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/7
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)