[kudu-CR] tserver: sanitize write op types

[Andrew Wong (Code Review)]

Andrew Wong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/12815 )

Change subject: tserver: sanitize write op types
..

tserver: sanitize write op types

We previously wouldn't make sure that Write requests actually contained
write operations (i.e. INSERT, UPDATE, UPSERT, DELETE). The result is
that a malicious user could send a bad write request to crash a tablet
server.

This patch addresses this by adding different decoder modes (e.g. for
writes, for split rows), and using the appropriate decoding mode for
writes.

Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Reviewed-on: http://gerrit.cloudera.org:8080/12815
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/master/catalog_manager.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/tablet_server-test.cc
7 files changed, 138 insertions(+), 43 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Adar Dembo: Looks good to me, approved

--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tserver: sanitize write op types

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12815 )

Change subject: tserver: sanitize write op types
..


Patch Set 4: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sun, 24 Mar 2019 18:17:53 +
Gerrit-HasComments: No

[kudu-CR] tserver: sanitize write op types

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12815 )

Change subject: tserver: sanitize write op types
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12815/3/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:

http://gerrit.cloudera.org:8080/#/c/12815/3/src/kudu/common/row_operations.h@81
PS3, Line 81:
: class
> This is only a convenience for tests, right? Is it absolutely necessary? Co
Done


http://gerrit.cloudera.org:8080/#/c/12815/3/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/12815/3/src/kudu/tserver/tablet_server-test.cc@1092
PS3, Line 1092:   const vector wrong_op_types = {
> Maybe also add UNKNOWN and test that you get its error message?
Done



--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sun, 24 Mar 2019 08:35:28 +
Gerrit-HasComments: Yes

[kudu-CR] tserver: sanitize write op types

[Andrew Wong (Code Review)]

Hello Tidy Bot, Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12815

to look at the new patch set (#4).

Change subject: tserver: sanitize write op types
..

tserver: sanitize write op types

We previously wouldn't make sure that Write requests actually contained
write operations (i.e. INSERT, UPDATE, UPSERT, DELETE). The result is
that a malicious user could send a bad write request to crash a tablet
server.

This patch addresses this by adding different decoder modes (e.g. for
writes, for split rows), and using the appropriate decoding mode for
writes.

Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
---
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/master/catalog_manager.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/tablet_server-test.cc
7 files changed, 138 insertions(+), 43 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/12815/4
-- 
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tserver: sanitize write op types

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12815 )

Change subject: tserver: sanitize write op types
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12815/3/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:

http://gerrit.cloudera.org:8080/#/c/12815/3/src/kudu/common/row_operations.h@81
PS3, Line 81:   // Decode any rows.
:   ANY,
This is only a convenience for tests, right? Is it absolutely necessary? Could 
we do without it?


http://gerrit.cloudera.org:8080/#/c/12815/3/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/12815/3/src/kudu/tserver/tablet_server-test.cc@1092
PS3, Line 1092:   const vector wrong_op_types = {
Maybe also add UNKNOWN and test that you get its error message?



--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 23 Mar 2019 22:55:06 +
Gerrit-HasComments: Yes

[kudu-CR] tserver: sanitize write op types

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12815 )

Change subject: tserver: sanitize write op types
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12815/1/src/kudu/tablet/transactions/write_transaction.cc
File src/kudu/tablet/transactions/write_transaction.cc:

http://gerrit.cloudera.org:8080/#/c/12815/1/src/kudu/tablet/transactions/write_transaction.cc@301
PS1, Line 301: void WriteTransactionState::StartApplying() {
> yea I guess I wouldn't try to do authorization in the decoder.. suppose it
Opted for the different validation modes. I'll still build a set for authz, but 
not in this patch.



--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 23 Mar 2019 05:50:23 +
Gerrit-HasComments: Yes

[kudu-CR] tserver: sanitize write op types

[Andrew Wong (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Todd Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12815

to look at the new patch set (#3).

Change subject: tserver: sanitize write op types
..

tserver: sanitize write op types

We previously wouldn't make sure that Write requests actually contained
write operations (i.e. INSERT, UPDATE, UPSERT, DELETE). The result is
that a malicious user could send a bad write request to crash a tablet
server.

This patch addresses this by adding different decoder modes (e.g. for
writes, for split rows), and using the appropriate decoding mode for
writes.

Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
---
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/master/catalog_manager.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/tablet_server-test.cc
7 files changed, 148 insertions(+), 43 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/12815/3
--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tserver: sanitize write op types

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12815 )

Change subject: tserver: sanitize write op types
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12815/2/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:

http://gerrit.cloudera.org:8080/#/c/12815/2/src/kudu/common/row_operations-test.cc@22
PS2, Line 22: #include 
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/12815/2/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:

http://gerrit.cloudera.org:8080/#/c/12815/2/src/kudu/common/row_operations.cc@562
PS2, Line 562:   // TODO: there's a bug here, in that if a client passes some 
column
> warning: missing username/bug in TODO [google-readability-todo]
Done



--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 23 Mar 2019 04:48:18 +
Gerrit-HasComments: Yes

[kudu-CR] tserver: sanitize write op types

[Andrew Wong (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/12815

to look at the new patch set (#2).

Change subject: tserver: sanitize write op types
..

tserver: sanitize write op types

We previously wouldn't make sure that Write requests actually contained
write operations (i.e. INSERT, UPDATE, UPSERT, DELETE). The result is
that a malicious user could send a bad write request to crash a tablet
server.

This patch addresses this by adding different decoder modes when
decoding.

Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
---
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/master/catalog_manager.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/tablet_server-test.cc
9 files changed, 191 insertions(+), 56 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/12815/2
--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tserver: sanitize write op types

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12815 )

Change subject: tserver: sanitize write op types
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12815/1/src/kudu/tablet/transactions/write_transaction.cc
File src/kudu/tablet/transactions/write_transaction.cc:

http://gerrit.cloudera.org:8080/#/c/12815/1/src/kudu/tablet/transactions/write_transaction.cc@301
PS1, Line 301: EmplaceIfNotPresent(_types_, op.type);
> Yeah, just for the purposes of sanitizing, doing this in in the decoder mak
yea I guess I wouldn't try to do authorization in the decoder.. suppose it 
makes sense to build this set for authz purposes.



--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 20 Mar 2019 20:27:08 +
Gerrit-HasComments: Yes

[kudu-CR] tserver: sanitize write op types

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12815 )

Change subject: tserver: sanitize write op types
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12815/1/src/kudu/tablet/transactions/write_transaction.cc
File src/kudu/tablet/transactions/write_transaction.cc:

http://gerrit.cloudera.org:8080/#/c/12815/1/src/kudu/tablet/transactions/write_transaction.cc@301
PS1, Line 301: EmplaceIfNotPresent(_types_, op.type);
> curious why you took this approach of storing it in a map vs just doing val
Yeah, just for the purposes of sanitizing, doing this in in the decoder makes 
sense.

This is actually fallout from authorization work, where having a set of the 
write operations is slightly more useful (i.e. it'd be relatively 
straightforward to compare the op types you're allowed to do with what op types 
you're trying to do). There's a case to be made for doing that in the decoder 
too that I hadn't considered, but it starts to blur the abstractions; I'll 
think about it.



--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 20 Mar 2019 19:18:00 +
Gerrit-HasComments: Yes

[kudu-CR] tserver: sanitize write op types

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12815 )

Change subject: tserver: sanitize write op types
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12815/1/src/kudu/tablet/transactions/write_transaction.cc
File src/kudu/tablet/transactions/write_transaction.cc:

http://gerrit.cloudera.org:8080/#/c/12815/1/src/kudu/tablet/transactions/write_transaction.cc@301
PS1, Line 301: EmplaceIfNotPresent(_types_, op.type);
curious why you took this approach of storing it in a map vs just doing 
validation here that the ops are valid? Or change the op decoder to take a flag 
indicating whether only write ops should be allowed?

I'm not sure if it will show up for real, but the cost of inserting into a 
std::set here seems a bit unnecessary.



--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 20 Mar 2019 19:03:41 +
Gerrit-HasComments: Yes

[kudu-CR] tserver: sanitize write op types

[Andrew Wong (Code Review)]

Andrew Wong has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/12815


Change subject: tserver: sanitize write op types
..

tserver: sanitize write op types

We previously wouldn't make sure that Write requests actually contained
write operations (i.e. INSERT, UPDATE, UPSERT, DELETE). The result is
that a malicious user could send a bad write request to crash a tablet
server.

Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
---
M src/kudu/common/row_operations.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/tablet_server-test.cc
4 files changed, 80 insertions(+), 23 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/12815/1
--
To view, visit http://gerrit.cloudera.org:8080/12815
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5
Gerrit-Change-Number: 12815
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong