Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11753 )
Change subject: authz: verify tokens on scans ...................................................................... Patch Set 9: (3 comments) http://gerrit.cloudera.org:8080/#/c/11753/9/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11753/9/src/kudu/tserver/tablet_service.cc@419 PS9, Line 419: schema.column_ids().end()); > Nit: lost the nice indenting here. Done http://gerrit.cloudera.org:8080/#/c/11753/9/src/kudu/tserver/tablet_service.cc@436 PS9, Line 436: respond_not_authorized(scan_pb.projected_columns(i).name()); > How about enforcing that the presence of a virtual column (you can test for We chatted about this in depth in Slack. I'll try to summarize the conclusions here: Diff scans work with the user supplying a virtual column in the request of column type IS_DELETED with a name that does not exist (something like "__kudu__is_deleted"). We expect such scans to be permitted, and so this block of code with prevent a user from doing a diff scan, since column "__kudu__is_deleted" doesn't exist in the tablet. So there must be special handling for such virtual columns; otherwise, they will be rejected here. We considered maybe being permissive and always allowing scans on virtual columns, but this seems like a vulnerability because a malicious user could modify their request to contain virtual columns and then sidestep authorization, and potentially learn about the presence of a column. We gave similar thought to requiring PK columns in the presence of virtual columns, but this seemed equally vulnerable. So we concluded that the safest thing we could do is to require _all_ scan column privileges in the presence of virtual columns. Another benefit of this approach is that it is easier to loosen a strict restriction than it is to tighten a loose one. http://gerrit.cloudera.org:8080/#/c/11753/9/src/kudu/tserver/tablet_service.cc@530 PS9, Line 530: req_type, context->requestor_string());; > Nit: extra semicolon here. Done -- To view, visit http://gerrit.cloudera.org:8080/11753 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7a5d81cf215a5d936f8853feba05778038764905 Gerrit-Change-Number: 11753 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 21 Mar 2019 17:50:31 +0000 Gerrit-HasComments: Yes
