[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11331 Change subject: Add missing authorization in KRPC .. Add missing authorization in KRPC In 2.12.0, Impala adopted Kudu RPC library for certain backened services (TransmitData(), EndDataStream()). While the implementation uses Kerberos for authenticating users connecting to the backend services, there is no authorization implemented. This is a regression from the Thrift based implementation because it registered a SASL callback (SaslAuthorizeInternal) to be invoked during the connection negotiation. With this regression, an unauthorized but authenticated user may invoke RPC calls to Impala backend services. This change fixes the issue above by overriding the default authorization method for the DataStreamServices. The authorization method will only let authenticated principal which matches the expected service name to access the service. Testing done: 1. Added a new test case in rpc-mgr-kerberized-test.cc to confirm an unauthorized user is not allowed to access the service. 2. Ran some queries in a Kerberos enabled cluster to make sure there is no error. 3. Exhaustive builds. Thanks to Todd Lipcon for pointing out the problem and his guidance on the fix. Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 --- M be/src/common/global-flags.cc M be/src/rpc/authentication.cc M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc R be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/rpc/thrift-server-test.cc M be/src/runtime/data-stream-test.cc M be/src/service/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/testutil/mini-kdc-wrapper.cc M be/src/testutil/mini-kdc-wrapper.h M be/src/util/auth-util.cc M be/src/util/auth-util.h M common/protobuf/data_stream_service.proto A common/protobuf/kudu M common/protobuf/rpc_test.proto 18 files changed, 314 insertions(+), 143 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11331/1 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/485/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Sun, 26 Aug 2018 05:16:43 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3086/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Sun, 26 Aug 2018 07:04:19 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 1: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3086/ -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Sun, 26 Aug 2018 10:18:59 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11331/1/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/11331/1/be/src/rpc/authentication.cc@608 PS1, Line 608: sasl::TSaslServer::SaslInit(GENERAL_CALLBACKS, APP_NAME.c_str()); I thought I had already taken care of this but looking at the code again, I realize I made a mistake. I think the easiest and quickest fix for this is to just check if kerberos is enabled and if it is, call TSaslServer::SaslInit() and TSaslClient::SaslInit() with 'KERB_INT_CALLBACKS' instead of 'GENERAL_CALLBACKS', which is what I thought I had done. That way on every new SASL GSSAPI authentication, the 'SaslAuthorizeInternal' callback will be called. For the client side, L831 should override the internal callbacks with its own external callbacks. If kerberos is enabled, we require all the connections to be kerberos enabled, so I expect that this should just work without any issues. This is a less intuitive fix than the one you've done though, since this will happen in the context of some call in the SASL library, whereas in your patch you're doing it as part of the RpcMgr. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Sun, 26 Aug 2018 16:42:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11331/1/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/11331/1/be/src/rpc/authentication.cc@608 PS1, Line 608: sasl::TSaslServer::SaslInit(GENERAL_CALLBACKS, APP_NAME.c_str()); > I thought I had already taken care of this but looking at the code again, I Alternatively, you could add these callbacks to the Kudu callback list for both the client and the server: https://github.com/apache/impala/blob/ac4acf1b77ccad95528741c255834d8ccdb84518/be/src/kudu/rpc/server_negotiation.cc#L148-L152 https://github.com/apache/impala/blob/540611e863fe99b3d3ae35f8b94a745a68b9eba2/be/src/kudu/rpc/client_negotiation.cc#L125-L131 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Sun, 26 Aug 2018 16:56:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11331/1/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/11331/1/be/src/rpc/authentication.cc@608 PS1, Line 608: sasl::TSaslServer::SaslInit(GENERAL_CALLBACKS, APP_NAME.c_str()); > Alternatively, you could add these callbacks to the Kudu callback list for Todd pointed out during an offline discussion that KRPC internally implements authorization by overriding the authz_method defined in the RPC service. The authz_method is invoked for all RPCs. So, adding a SASL callback in some sense is adding another path which authorization is done. The argument for it is that it's a per-connection authorization instead of a per-RPC authorization. The latter may be needed if we need finer grained authorization (e.g. RPC method level authorization) so we can save some CPU overhead of having to parse the principal repeatedly for each RPC. It's unclear how much we are saving in the grand scheme of things. I will explore the alternative of overriding the SASL callbacks but IMHO, using the existing mechanism in KRPC to do the authorization (which is what's implemented in this patch) seems to be the easiest path for now. May help if Todd or others can chime in on their opinion. I think it may make sense to explore the option of adding a way for caller to specify Sasl callbacks as part of constructing a messenger. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 27 Aug 2018 18:57:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11331/1/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/11331/1/be/src/rpc/authentication.cc@608 PS1, Line 608: sasl::TSaslServer::SaslInit(GENERAL_CALLBACKS, APP_NAME.c_str()); > Todd pointed out during an offline discussion that KRPC internally implemen Wasn't very clear in my previous update. I was trying to say: The per-connection authorization (instead of per-RPC authorization) may save some CPU overhead by avoiding the need to parse the principal on every RPC call. It's unclear how much we are saving in the grand scheme of things though. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 27 Aug 2018 19:05:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Hello Sailesh Mukil, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11331 to look at the new patch set (#2). Change subject: Add missing authorization in KRPC .. Add missing authorization in KRPC In 2.12.0, Impala adopted Kudu RPC library for certain backened services (TransmitData(), EndDataStream()). While the implementation uses Kerberos for authenticating users connecting to the backend services, there is no authorization implemented. This is a regression from the Thrift based implementation because it registered a SASL callback (SaslAuthorizeInternal) to be invoked during the connection negotiation. With this regression, an unauthorized but authenticated user may invoke RPC calls to Impala backend services. This change fixes the issue above by overriding the default authorization method for the DataStreamServices. The authorization method will only let authenticated principal which matches the expected service name to access the service. Testing done: 1. Added a new test case in rpc-mgr-kerberized-test.cc to confirm an unauthorized user is not allowed to access the service. 2. Ran some queries in a Kerberos enabled cluster to make sure there is no error. 3. Exhaustive builds. Thanks to Todd Lipcon for pointing out the problem and his guidance on the fix. Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 --- M be/src/common/global-flags.cc M be/src/rpc/authentication.cc M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc R be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/rpc/thrift-server-test.cc M be/src/runtime/data-stream-test.cc M be/src/service/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/testutil/mini-kdc-wrapper.cc M be/src/testutil/mini-kdc-wrapper.h M be/src/util/auth-util.cc M be/src/util/auth-util.h M bin/rat_exclude_files.txt M common/protobuf/data_stream_service.proto A common/protobuf/kudu M common/protobuf/rpc_test.proto 19 files changed, 315 insertions(+), 143 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11331/2 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/495/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 27 Aug 2018 23:30:22 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 2: Yea, I think passing through SASL callbacks into the KRPC code will be somewhat messy and incomplete. Currently Impala only makes use of the SASL-based authentication, but KRPC also supports TOKEN authentication which avoids SASL entirely. So, using SASL callbacks would only be a partial solution in the general case. Adding the ability to do connection-scoped authorization through a C++-style callback passed into the messenger seems reasonable enough, but keep in mind that, when we accept a connection, we don't know yet which particular service it is destined for. Currently, I guess Impala only uses KRPC for "internal" services, but if you ever start wanting to implement an ODBC/JDBC driver based on krpc instead of Thrift this would also start to fall apart a bit. I wouldn't imagine that the authz check per RPC will be a noticeable bottleneck. If you look at the request context of the RPC, I think we already include the remote username (parsed from the principal) and that's already parsed at the connection level, isn't it? Given that, I don't think you need to do any expensive parsing such as running auth-to-local rules, but rather just need to compare the short name to the configured whitelist. Haven't taken a look at the code yet but will do so now. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 28 Aug 2018 02:31:12 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/11331 )
Change subject: Add missing authorization in KRPC
..
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:
http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.h@216
PS2, Line 216: authorized_service_name_
I think this is somewhat ambiguous. When I read this first, I thought you meant
service name like 'CatalogService'. Maybe "service_username" or
"service_short_username"?
http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@118
PS2, Line 118: authorized_service_name_
I think this should actually be hardcoded to "impala" or something rather than
being dependent on the principal. It would break rolling upgrade but I guess
Impala doesn't really support rolling upgrade anyway?
http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@161
PS2, Line 161: if (IsKerberosEnabled()) {
invert this condition so that you can un-nest the majority of the function.
http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@164
PS2, Line 164: ParseKerberosPrincipal(remote_user.principal().value_or(""),
: &service_name, &unused_hostname, &unused_realm
why not use remote_user.username() here? It's already parsed for you and mapped
using auth_to_local rules. You can use
kudu::security::GetLoggedInUsernameFromKeytab to do this for you.
http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/testutil/mini-kdc-wrapper.h
File be/src/testutil/mini-kdc-wrapper.h:
http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/testutil/mini-kdc-wrapper.h@43
PS2, Line 43: static Status SetupAndStartMiniKDC(const std::string& realm,
: const std::string& ticket_lifetime, const std::string&
renew_lifetime,
why these changes? it's better to take the arguments by copy, and then
std::move() them into the member variable in the initializer list
--
To view, visit http://gerrit.cloudera.org:8080/11331
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5
Gerrit-Change-Number: 11331
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 28 Aug 2018 02:46:14 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/11331 )
Change subject: Add missing authorization in KRPC
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@164
PS2, Line 164: ParseKerberosPrincipal(remote_user.principal().value_or(""),
: &service_name, &unused_hostname, &unused_realm
> why not use remote_user.username() here? It's already parsed for you and ma
sorry I skipped half of what I meant to say in this comment. What I meant is
that you can use remote_user.username() and then compare for equality with the
service's short username, which you can get from the above-mentioned method.
--
To view, visit http://gerrit.cloudera.org:8080/11331
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5
Gerrit-Change-Number: 11331
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 28 Aug 2018 03:20:22 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc@50 PS2, Line 50: krb5_ccname nit: Maybe krb5_ccpath ? Or krb5ccname_path http://gerrit.cloudera.org:8080/#/c/11331/1/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/11331/1/be/src/rpc/authentication.cc@608 PS1, Line 608: sasl::TSaslServer::SaslInit(GENERAL_CALLBACKS, APP_NAME.c_str()); > Wasn't very clear in my previous update. I was trying to say: I think the SASL callbacks are invoked only on SASL handshakes, so they're technically once per connection too. However, I'm all for building it on top of the KRPC auth as well. Edit: I guess another difference is that with the approach that this patch takes, we'll obtain a kerberos ticket, but fail backend RPC communication, whereas a SASL auth callback failure will not provide a ticket as well IIRC. I can't think of security issues with that though since we're the only service they can talk to with the ticket and we'll refuse to talk if they're not authorized, so I think it should be fine. http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.h@216 PS2, Line 216: authorized_service_name_ > I think this is somewhat ambiguous. When I read this first, I thought you m Just to add, if we're going to keep the service name fixed, this should probably be a const. That's your call however. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 28 Aug 2018 03:56:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Hello Sailesh Mukil, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11331 to look at the new patch set (#3). Change subject: Add missing authorization in KRPC .. Add missing authorization in KRPC In 2.12.0, Impala adopted Kudu RPC library for certain backened services (TransmitData(), EndDataStream()). While the implementation uses Kerberos for authenticating users connecting to the backend services, there is no authorization implemented. This is a regression from the Thrift based implementation because it registered a SASL callback (SaslAuthorizeInternal) to be invoked during the connection negotiation. With this regression, an unauthorized but authenticated user may invoke RPC calls to Impala backend services. This change fixes the issue above by overriding the default authorization method for the DataStreamServices. The authorization method will only let authenticated principal which matches the expected service name to access the service. Testing done: 1. Added a new test case in rpc-mgr-kerberized-test.cc to confirm an unauthorized user is not allowed to access the service. 2. Ran some queries in a Kerberos enabled cluster to make sure there is no error. 3. Exhaustive builds. Thanks to Todd Lipcon for pointing out the problem and his guidance on the fix. Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 --- M be/src/common/global-flags.cc M be/src/rpc/authentication.cc M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc R be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/rpc/thrift-server-test.cc M be/src/runtime/data-stream-test.cc M be/src/service/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/testutil/mini-kdc-wrapper.cc M be/src/testutil/mini-kdc-wrapper.h M be/src/util/auth-util.cc M be/src/util/auth-util.h M bin/rat_exclude_files.txt M common/protobuf/data_stream_service.proto A common/protobuf/kudu M common/protobuf/rpc_test.proto 19 files changed, 311 insertions(+), 142 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11331/3 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc@50 PS2, Line 50: krb5_ccname > nit: Maybe krb5_ccpath ? Or krb5ccname_path Seems more intuitive to have a name which resembles the env var KRB5CCNAME. That env variable indicates the location of the credentials cache so it seems a bit redundant to add path to its name as we also don't use krb_conf_path below. Please let me know if you feel strongly about it though. KRB5CCNAME Location of the Kerberos 5 credentials (ticket) cache. http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.h@216 PS2, Line 216: > Just to add, if we're going to keep the service name fixed, this should pro Variable removed as this is no longer necessary per suggestion to use GetLoggedInUserNameFromKeytab(). http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@118 PS2, Line 118: nused_realm)); > I think this should actually be hardcoded to "impala" or something rather t For all practical purposes, that's true. However, doing so will also require hardcoding part of FLAGS_principal / FLAGS_be_principal as that's what's used for kinit. Hardcoding the principal is almost the same as deprecating FLAGS_principal / FLAGS_be_principal and switching to using a boolean flag FLAGS_kerberos_enabled. This seems to be a larger scope than I feel comfortable with for this change. I need more thought about this as I have to understand the historical context for having FLAGS_principal / FLAGS_be_principal to begin with. http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@161 PS2, Line 161: // Authorization is enforced iff Kerberos is enabled. > invert this condition so that you can un-nest the majority of the function. Done http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@164 PS2, Line 164: ername matches that of the kinit'ed principal. : const RemoteUser& remote_user = context->remote_user > sorry I skipped half of what I meant to say in this comment. What I meant i Good point. Done. http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/testutil/mini-kdc-wrapper.h File be/src/testutil/mini-kdc-wrapper.h: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/testutil/mini-kdc-wrapper.h@43 PS2, Line 43: static Status SetupAndStartMiniKDC(std::string realm, : std::string ticket_lifetime, std::string renew_lifetime, > why these changes? it's better to take the arguments by copy, and then std: Done -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 28 Aug 2018 18:27:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/508/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 28 Aug 2018 18:53:22 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 3: (6 comments) Didn't review the test changes much since I'm not as familiar with that code, hoping sailesh looked at those. http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc@50 PS2, Line 50: krb5_ccname > Seems more intuitive to have a name which resembles the env var KRB5CCNAME. Should this be hidden? I dont think we'd want to support users changing it, and it's just for tests, right? http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.h@25 PS3, Line 25: #include "rpc/authentication.h" : #include "rpc/impala-service-pool.h" : #include "util/auth-util.h" are these new includes necessary? I don't see any new usages of types in this file. Would be better to include them from the .cc if necessary to cut down on compilation time. http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/rpc/rpc-mgr.cc@118 PS2, Line 118: nused_realm)); > For all practical purposes, that's true. Oh, I forgot that the sasl protocol name ends up being the expected remote principal for mutual auth. Never mind my comment, you're right. http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.cc@172 PS3, Line 172: mem_tracker->Release(context->GetTransferSize()); slight nit (though I guess this happens elsewhere also): shouldn't we wait until we've responded before we release memtracker consumption? the actual memory isn't freed until we have responded. I guess the code becomes a little messier because we have to grab context->GetTransferSize() before we respond. Not worth addressing here, but worth considering in other spots perhaps? http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.cc@173 PS3, Line 173: context->RespondFailure(kudu::Status::NotAuthorized( should we also log the unauthorized access attempt (perhaps including the expected username, for easier debugging if this goes wrong?) http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/util/auth-util.cc File be/src/util/auth-util.cc: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/util/auth-util.cc@84 PS3, Line 84: Status ParseKerberosPrincipal(const string& principal, string* service_name, I wonder whether we should just be using krb5_parse_name here instead of implementing our own parsing? According to http://web.mit.edu/kerberos/krb5-1.15/doc/appdev/refs/api/krb5_parse_name.html there are various escapings, etc, that this function isn't currently supporting. That said, why did you have to change this function for this patch? -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 28 Aug 2018 20:28:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Hello Sailesh Mukil, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11331 to look at the new patch set (#4). Change subject: Add missing authorization in KRPC .. Add missing authorization in KRPC In 2.12.0, Impala adopted Kudu RPC library for certain backened services (TransmitData(), EndDataStream()). While the implementation uses Kerberos for authenticating users connecting to the backend services, there is no authorization implemented. This is a regression from the Thrift based implementation because it registered a SASL callback (SaslAuthorizeInternal) to be invoked during the connection negotiation. With this regression, an unauthorized but authenticated user may invoke RPC calls to Impala backend services. This change fixes the issue above by overriding the default authorization method for the DataStreamServices. The authorization method will only let authenticated principal which matches the expected service name to access the service. Testing done: 1. Added a new test case in rpc-mgr-kerberized-test.cc to confirm an unauthorized user is not allowed to access the service. 2. Ran some queries in a Kerberos enabled cluster to make sure there is no error. 3. Exhaustive builds. Thanks to Todd Lipcon for pointing out the problem and his guidance on the fix. Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 --- M be/src/common/global-flags.cc M be/src/rpc/authentication.cc M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc R be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/rpc/thrift-server-test.cc M be/src/runtime/data-stream-test.cc M be/src/service/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/testutil/mini-kdc-wrapper.cc M be/src/testutil/mini-kdc-wrapper.h M be/src/util/auth-util.cc M be/src/util/auth-util.h M bin/rat_exclude_files.txt M common/protobuf/data_stream_service.proto A common/protobuf/kudu M common/protobuf/rpc_test.proto 19 files changed, 315 insertions(+), 146 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11331/4 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc@50 PS2, Line 50: krb5_ccname > Should this be hidden? I dont think we'd want to support users changing it, I actually prefer to keep it public so the user can configure it for cases such as KUDU-2545. I will add a remark about its being advanced option. http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.h@25 PS3, Line 25: #include "rpc/authentication.h" : #include "rpc/impala-service-pool.h" : #include "util/auth-util.h" > are these new includes necessary? I don't see any new usages of types in th Left over from previous patch. Removed. http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.cc@172 PS3, Line 172: mem_tracker->Release(context->GetTransferSize()); > slight nit (though I guess this happens elsewhere also): shouldn't we wait Sure. Will do it as a clean-up in a follow-on patch. That said, my understanding is that the reply itself is processed asynchronously. http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/rpc/rpc-mgr.cc@173 PS3, Line 173: context->RespondFailure(kudu::Status::NotAuthorized( > should we also log the unauthorized access attempt (perhaps including the e Done http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/util/auth-util.cc File be/src/util/auth-util.cc: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/util/auth-util.cc@84 PS3, Line 84: Status ParseKerberosPrincipal(const string& principal, string* service_name, > I wonder whether we should just be using krb5_parse_name here instead of im When writing this patch, I noticed the previous implementation made two calls split() which seems unnecessary. I fixed it as part of this patch as I was calling it in the previous version of this patch but this is not needed anymore in the new version. It doesn't hurt to keep it though. I am not sure why krb5_parse_name() wasn't used here in the previous implementation. Given the Kudu code has better infrastructure, it seems easier for me to add a utility function to Kudu and call it here instead. Filed IMPALA-7504 for this. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 00:24:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/512/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 00:56:15 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc@50 PS2, Line 50: krb5_ccname > I actually prefer to keep it public so the user can configure it for cases That's fair. Maybe we should add some validation, then, that the path is an absolute path before attempting to use it? My fear is that there are valid CCNAMEs such as MEMORY:foo which a user might be familiar with, and if they try to use that, Impala will have some hard-to-debug issues, since those CCNAMEs are supported by the C++ side but not the Java side. http://gerrit.cloudera.org:8080/#/c/11331/4/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/11331/4/be/src/rpc/rpc-mgr.cc@174 PS4, Line 174: LOG(ERROR) << err_msg; the log message probably include the 'requestor_string' which includes the remote IP address, since the admin probably wants as much detail as possible on the unauthorized access attempt. In the log message maybe also inlcude 'logged_in_username' so it's clear what username was _expected_ for this service (but don't send that back to the "attacker" to avoid giving them any extra info) http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/util/auth-util.cc File be/src/util/auth-util.cc: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/util/auth-util.cc@84 PS3, Line 84: Status ParseKerberosPrincipal(const string& principal, string* service_name, > When writing this patch, I noticed the previous implementation made two cal In terms of whether to keep the change in the patch, I figured that since this patch has some security implications we may want to keep it minimal. The behavior of the new function and the old function are slightly different, eg in the case that for some reason you had a @ that came before the / or something. I dont think that would be a valid principal, but probably better to avoid any potential for behavior change even in a strange edge case when addressing a bug. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 01:05:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Hello Sailesh Mukil, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11331 to look at the new patch set (#5). Change subject: Add missing authorization in KRPC .. Add missing authorization in KRPC In 2.12.0, Impala adopted Kudu RPC library for certain backened services (TransmitData(), EndDataStream()). While the implementation uses Kerberos for authenticating users connecting to the backend services, there is no authorization implemented. This is a regression from the Thrift based implementation because it registered a SASL callback (SaslAuthorizeInternal) to be invoked during the connection negotiation. With this regression, an unauthorized but authenticated user may invoke RPC calls to Impala backend services. This change fixes the issue above by overriding the default authorization method for the DataStreamServices. The authorization method will only let authenticated principal which matches the expected service name to access the service. Testing done: 1. Added a new test case in rpc-mgr-kerberized-test.cc to confirm an unauthorized user is not allowed to access the service. 2. Ran some queries in a Kerberos enabled cluster to make sure there is no error. 3. Exhaustive builds. Thanks to Todd Lipcon for pointing out the problem and his guidance on the fix. Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 --- M be/src/common/global-flags.cc M be/src/rpc/authentication.cc M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc R be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/rpc/thrift-server-test.cc M be/src/runtime/data-stream-test.cc M be/src/service/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/testutil/mini-kdc-wrapper.cc M be/src/testutil/mini-kdc-wrapper.h M be/src/util/auth-util.cc M be/src/util/auth-util.h M bin/rat_exclude_files.txt M common/protobuf/data_stream_service.proto A common/protobuf/kudu M common/protobuf/rpc_test.proto 19 files changed, 328 insertions(+), 135 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11331/5 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11331/2/be/src/common/global-flags.cc@50 PS2, Line 50: krb5_ccname > That's fair. Maybe we should add some validation, then, that the path is an Good point. Validation added in AuthManager::InitKerberosEnv() http://gerrit.cloudera.org:8080/#/c/11331/4/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/11331/4/be/src/rpc/rpc-mgr.cc@174 PS4, Line 174: mem_tracker->Release(context->GetTransferSize()); > the log message probably include the 'requestor_string' which includes the Good point. This definitely helps debugging. http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/util/auth-util.cc File be/src/util/auth-util.cc: http://gerrit.cloudera.org:8080/#/c/11331/3/be/src/util/auth-util.cc@84 PS3, Line 84: Status ParseKerberosPrincipal(const string& principal, string* service_name, > In terms of whether to keep the change in the patch, I figured that since t Fair enough. Reverted the change and left a TODO. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 05:00:34 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/516/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 05:35:52 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/11331 )
Change subject: Add missing authorization in KRPC
..
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/authentication.cc@734
PS5, Line 734: return Status(Substitute("FLAGS_krb5_ccname '$0' is not an
absolute file path",
nit: use the user-facing name --krb5_ccname instead of FLAGS_... which is
internal-only
http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/rpc-mgr.cc@172
PS5, Line 172: LOG(ERROR) << Substitute("Unauthorized access from $0.
Expected user: $1",
sorry, think I wasn't entirely clear. I think the error message should still
also include the service. Maybe it should also be slightly more clear to say
"rejecting unauthorized access" or something, since a user might be scared into
thinking "uh oh! there was unauthorized access!"
--
To view, visit http://gerrit.cloudera.org:8080/11331
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5
Gerrit-Change-Number: 11331
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 29 Aug 2018 08:29:59 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 5: Code-Review+1 (1 comment) Testing side LGTM. http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/rpc-mgr-kerberized-test.cc File be/src/rpc/rpc-mgr-kerberized-test.cc: http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/rpc-mgr-kerberized-test.cc@114 PS5, Line 114: rpc_status = : FromKuduStatus(scan_proxy->ScanMem(scan_request, &scan_response, &controller)); : EXPECT_TRUE(rpc_status.ok()); So, this is passing only because the Authorize() function for ScanMemServiceImpl always returns true right? I had to read across files to figure that out. I think adding a comment like the following should make the test easier to understand: "ScanMemServiceImpl authorizes everyone connecting to the service, so we expect it to pass even though we have a different principal name". And the opposite for PingServiceImpl. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 16:55:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Hello Sailesh Mukil, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11331 to look at the new patch set (#6). Change subject: Add missing authorization in KRPC .. Add missing authorization in KRPC In 2.12.0, Impala adopted Kudu RPC library for certain backened services (TransmitData(), EndDataStream()). While the implementation uses Kerberos for authenticating users connecting to the backend services, there is no authorization implemented. This is a regression from the Thrift based implementation because it registered a SASL callback (SaslAuthorizeInternal) to be invoked during the connection negotiation. With this regression, an unauthorized but authenticated user may invoke RPC calls to Impala backend services. This change fixes the issue above by overriding the default authorization method for the DataStreamService. The authorization method will only let authenticated principal which matches FLAGS_principal / FLAGS_be_principal to access the service. Also added a new startup flag --krb5_ccname to allow users to customize the locations of the Kerberos credentials cache. Testing done: 1. Added a new test case in rpc-mgr-kerberized-test.cc to confirm an unauthorized user is not allowed to access the service. 2. Ran some queries in a Kerberos enabled cluster to make sure there is no error. 3. Exhaustive builds. Thanks to Todd Lipcon for pointing out the problem and his guidance on the fix. Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 --- M be/src/common/global-flags.cc M be/src/rpc/authentication.cc M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc R be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/rpc/thrift-server-test.cc M be/src/runtime/data-stream-test.cc M be/src/service/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/testutil/mini-kdc-wrapper.cc M be/src/testutil/mini-kdc-wrapper.h M be/src/util/auth-util.cc M be/src/util/auth-util.h M bin/rat_exclude_files.txt M common/protobuf/data_stream_service.proto A common/protobuf/kudu M common/protobuf/rpc_test.proto 19 files changed, 330 insertions(+), 135 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11331/6 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 19:09:17 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 6: Carry Sailesh's +1 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 19:09:11 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/11331 )
Change subject: Add missing authorization in KRPC
..
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/authentication.cc@734
PS5, Line 734: return Status(Substitute("Bad --krb5_ccname value: $0 is not
an absolute file path",
> nit: use the user-facing name --krb5_ccname instead of FLAGS_... which is i
Done
http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/rpc-mgr-kerberized-test.cc
File be/src/rpc/rpc-mgr-kerberized-test.cc:
http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/rpc-mgr-kerberized-test.cc@114
PS5, Line 114: controller.Reset();
: rpc_status =
: FromKuduStatus(scan_proxy
> So, this is passing only because the Authorize() function for ScanMemServic
Yes, I have some comments about it on line 99-100 but yes, I should move them
to a location closer to the actual execution of the test to make it clearer.
http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/11331/5/be/src/rpc/rpc-mgr.cc@172
PS5, Line 172: LOG(ERROR) << Substitute("Rejecting unauthorized access to
$0 from $1. Expected "
> sorry, think I wasn't entirely clear. I think the error message should stil
Lol. It's indeed a bit scary to not have the word "Reject" in the error
message. Fixed. Thanks.
--
To view, visit http://gerrit.cloudera.org:8080/11331
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5
Gerrit-Change-Number: 11331
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 29 Aug 2018 19:08:56 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add missing authorization in KRPC
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 19:19:30 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/525/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 19:57:03 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 6: Will merge the patch after more testing. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 29 Aug 2018 22:11:03 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Hello Sailesh Mukil, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11331 to look at the new patch set (#7). Change subject: Add missing authorization in KRPC .. Add missing authorization in KRPC In 2.12.0, Impala adopted Kudu RPC library for certain backened services (TransmitData(), EndDataStream()). While the implementation uses Kerberos for authenticating users connecting to the backend services, there is no authorization implemented. This is a regression from the Thrift based implementation because it registered a SASL callback (SaslAuthorizeInternal) to be invoked during the connection negotiation. With this regression, an unauthorized but authenticated user may invoke RPC calls to Impala backend services. This change fixes the issue above by overriding the default authorization method for the DataStreamService. The authorization method will only let authenticated principal which matches FLAGS_principal / FLAGS_be_principal to access the service. Also added a new startup flag --krb5_ccname to allow users to customize the locations of the Kerberos credentials cache. Testing done: 1. Added a new test case in rpc-mgr-kerberized-test.cc to confirm an unauthorized user is not allowed to access the service. 2. Ran some queries in a Kerberos enabled cluster to make sure there is no error. 3. Exhaustive builds. Thanks to Todd Lipcon for pointing out the problem and his guidance on the fix. Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 --- M be/src/common/global-flags.cc M be/src/rpc/authentication.cc M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc R be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/rpc/thrift-server-test.cc M be/src/runtime/data-stream-test.cc M be/src/service/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/testutil/mini-kdc-wrapper.cc M be/src/testutil/mini-kdc-wrapper.h M be/src/util/auth-util.cc M be/src/util/auth-util.h M bin/rat_exclude_files.txt M common/protobuf/data_stream_service.proto A common/protobuf/kudu M common/protobuf/rpc_test.proto 19 files changed, 332 insertions(+), 135 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11331/7 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 7: PS7 adds 2 lines of code in rpc-mgr-kerberized-test.cc to set / clear FLAGS_krb5_ccname. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 30 Aug 2018 00:31:03 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 7: Code-Review+2 Carry Todd's +2 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 30 Aug 2018 00:31:31 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 30 Aug 2018 00:32:22 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3095/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 30 Aug 2018 00:32:23 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/530/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 30 Aug 2018 01:14:29 + Gerrit-HasComments: No
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Add missing authorization in KRPC In 2.12.0, Impala adopted Kudu RPC library for certain backened services (TransmitData(), EndDataStream()). While the implementation uses Kerberos for authenticating users connecting to the backend services, there is no authorization implemented. This is a regression from the Thrift based implementation because it registered a SASL callback (SaslAuthorizeInternal) to be invoked during the connection negotiation. With this regression, an unauthorized but authenticated user may invoke RPC calls to Impala backend services. This change fixes the issue above by overriding the default authorization method for the DataStreamService. The authorization method will only let authenticated principal which matches FLAGS_principal / FLAGS_be_principal to access the service. Also added a new startup flag --krb5_ccname to allow users to customize the locations of the Kerberos credentials cache. Testing done: 1. Added a new test case in rpc-mgr-kerberized-test.cc to confirm an unauthorized user is not allowed to access the service. 2. Ran some queries in a Kerberos enabled cluster to make sure there is no error. 3. Exhaustive builds. Thanks to Todd Lipcon for pointing out the problem and his guidance on the fix. Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Reviewed-on: http://gerrit.cloudera.org:8080/11331 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/common/global-flags.cc M be/src/rpc/authentication.cc M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc R be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/rpc/thrift-server-test.cc M be/src/runtime/data-stream-test.cc M be/src/service/data-stream-service.cc M be/src/service/data-stream-service.h M be/src/testutil/mini-kdc-wrapper.cc M be/src/testutil/mini-kdc-wrapper.h M be/src/util/auth-util.cc M be/src/util/auth-util.h M bin/rat_exclude_files.txt M common/protobuf/data_stream_service.proto A common/protobuf/kudu M common/protobuf/rpc_test.proto 19 files changed, 332 insertions(+), 135 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] Add missing authorization in KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11331 ) Change subject: Add missing authorization in KRPC .. Patch Set 8: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11331 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f82dee5e721f2ed23e75fd91abbc6ab7addd4c5 Gerrit-Change-Number: 11331 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 30 Aug 2018 04:06:07 + Gerrit-HasComments: No
