[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 --- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 06 Nov 2018 19:15:33 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 06 Nov 2018 18:23:33 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 06 Nov 2018 02:56:41 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 05 Nov 2018 17:44:51 + Gerrit-HasComments: No
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 03 Nov 2018 00:38:31 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 23:15:12 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 02 Nov 2018 02:24:11 + Gerrit-HasComments: No
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 02 Nov 2018 01:35:31 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 22:58:05 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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:
);
> 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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 22:17:11 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 18:01:57 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 18:01:14 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 18:00:06 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 26 Oct 2018 17:52:18 + Gerrit-HasComments: No
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 17:28:23 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Oct 2018 17:17:43 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 26 Oct 2018 16:31:45 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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(), ), : "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, , 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 26 Oct 2018 15:48:36 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
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 Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
