Mailing-List: contact reviews-help@impala.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711281500.vASF03Qd003870@ip-10-146-233-104.ec2.internal>
Date: Tue, 28 Nov 2017 15:00:03 +0000
From: "Sailesh Mukil (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Michael Ho <kwho@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5053=3A_=5BSECURITY=5D_Make_KRPC_work_with_Kerberos=0A?=
In-Reply-To: <gerrit.1507872939000.I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd@gerrit.cloudera.org>
References: <gerrit.1507872939000.I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd@gerrit.cloudera.org>
Reply-To: sailesh@cloudera.com, impala-cr@cloudera.com, marcelk@gmail.com,
        kwho@cloudera.com, reviews@impala.incubator.apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="dsNul+HAHVQ="; charset=UTF-8
archived-at: Tue, 28 Nov 2017 15:00:09 -0000

--dsNul+HAHVQ=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Sailesh Mukil has posted comments on this change=2E ( http://gerrit=2Ecloud=
era=2Eorg:8080/8270 )

Change subject: IMPALA-5053: [SECURITY] Make KRPC wo=
rk with Kerberos
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E


Patch Set 6:

(13 comments)

http://gerrit=2Ecloudera=2Eorg:8080/#/c/=
8270/5/be/src/rpc/auth-provider=2Eh
File be/src/rpc/auth-provider=2Eh:

htt=
p://gerrit=2Ecloudera=2Eorg:8080/#/c/8270/5/be/src/rpc/auth-provider=2Eh@27=

PS5, Line 27: #include "util/thread=2Eh"
> What is this for ?
This is for =
the 'kinit_thread_' below=2E We hadn't IWYU'd before=2E


http://gerrit=2Ec=
loudera=2Eorg:8080/#/c/8270/4/be/src/rpc/auth-provider=2Eh
File be/src/rpc/=
auth-provider=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8270/4/be/src/r=
pc/auth-provider=2Eh@78
PS4, Line 78:   /// Wrap the client transport with =
a new TSaslClientTransport=2E  This is only for
            :   /// interna=
l connections=2E  Since, as a daemon, we only do Kerberos and not LDAP,
   =
         :   /// we can go straight to Kerberos=2E
            :   /// This=
 is only applicable to Thrift connections and not KRPC connections=2E
     =
       :   virtual Status WrapClientTransport(const std::string& hostname,
=
            :       boost::shared_ptr<apache::thrift::transport::TTransport=
> raw_transport,
            :       const std::string& service_name,
> Is =
this only used for thrift connections ? May be it helps to state it now 
Do=
ne


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8270/5/be/src/rpc/authenticati=
on=2Ecc
File be/src/rpc/authentication=2Ecc:

http://gerrit=2Ecloudera=2Eor=
g:8080/#/c/8270/5/be/src/rpc/authentication=2Ecc@643
PS5, Line 643:     // =
Kudu Client and Kudu RPC shouldn't attempt to initialize SASL which would c=
onflict
             :     // with Impala's SASL initialization=2E This mus=
t be call
> Please add a small comment that this overlaps with the initiali=
zation below
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8270/5/be/src/r=
pc/authentication=2Ecc@658
PS5, Line 658:     sasl::TSaslServer::SaslInit(G=
ENERAL_CALLBACKS, appname);
             :     sasl::TSaslClient::SaslInit(=
GENERAL_CALLBACKS);
             :   } catch (sasl::SaslServerImplException=
& e) {
> Once we completely move away from Thrift, do we rely on Kudu to in=
itialize 
We will still have Thrift for external connections, so my take is=
 that we can leave all the SASL initialization in Impala's control so we ca=
n add the kinds of callbacks, etc=2E that we like=2E


http://gerrit=2Eclou=
dera=2Eorg:8080/#/c/8270/5/be/src/rpc/authentication=2Ecc@1034
PS5, Line 10=
34:   RETURN_IF_ERROR(GetExternalKerberosPrincipal(&kerberos_external_princ=
ipal));
              :   if (!kerberos_internal_principal=2Eempty()) {
> T=
he internal and external principals look like good candidates to be stored
=
We can do that, however, I think it adds some unnecessary complexity for th=
e authentication classes to depend on the ExecEnv=2E

For eg, some of the b=
ackend tests (including the rpc-mgr-test and thrift-server-test) do not hav=
e an ExecEnv but use the authentication classes=2E In that case, we'd need =
to add ExecEnv objects to the tests just for this=2E

Let me know what you =
think=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8270/5/be/src/rpc/authen=
tication=2Ecc@1036
PS5, Line 1036:     RETURN_IF_ERROR(InitKerberosEnv());
=
> nit: blank line not needed=2E
Done


http://gerrit=2Ecloudera=2Eorg:8080/=
#/c/8270/5/be/src/rpc/rpc-mgr=2Ecc
File be/src/rpc/rpc-mgr=2Ecc:

http://ge=
rrit=2Ecloudera=2Eorg:8080/#/c/8270/5/be/src/rpc/rpc-mgr=2Ecc@63
PS5, Line =
63: FLAGS_num_reactor_thread
> IsKerberosEnabled()
Done


http://gerrit=2Ec=
loudera=2Eorg:8080/#/c/8270/5/be/src/rpc/rpc-mgr=2Ecc@70
PS5, Line 70:     =
string service_name, unused_hostname, unused_realm;
> It may be better to s=
et FLAGS_rpc_authentication (defined by Kudu) to "requ
Done


http://gerrit=
=2Ecloudera=2Eorg:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper=2Eh
File=
 be/src/testutil/mini-kdc-wrapper=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080=
/#/c/8270/5/be/src/testutil/mini-kdc-wrapper=2Eh@53
PS5, Line 53: //
> nit:=
 ///
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8270/5/be/src/testutil/=
mini-kdc-wrapper=2Eh@86
PS5, Line 86:   /// Stops the KDC by terminating th=
e krb5kdc subprocess=2E
> Please add comments about what clean up it may do=
=2E
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8270/5/be/src/testutil/m=
ini-kdc-wrapper=2Ecc
File be/src/testutil/mini-kdc-wrapper=2Ecc:

http://ge=
rrit=2Ecloudera=2Eorg:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper=2Ecc=
@67
PS5, Line 67: !
> !=3D seems to make less assumption about the ordering=
 of the ENUM values=2E
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8270/=
5/be/src/testutil/mini-kdc-wrapper=2Ecc@88
PS5, Line 88: !
> !=3D
Done


ht=
tp://gerrit=2Ecloudera=2Eorg:8080/#/c/8270/5/be/src/util/auth-util=2Ecc
Fil=
e be/src/util/auth-util=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8270=
/5/be/src/util/auth-util=2Ecc@86
PS5, Line 86:   split(names, principal, is=
_any_of("/"));
            :   if (names=2Esize() !=3D 2) return Status(TEr=
rorCode::BAD_PRINCIPAL_FORMAT, principal);
            : 
> Should this liv=
e in generate_error_codes=2Epy ?
Done=2E Added to generate_error_codes=2E

=


-- 
To view, visit http://gerrit=2Ecloudera=2Eorg:8080/8270
To unsubscrib=
e, visit http://gerrit=2Ecloudera=2Eorg:8080/settings

Gerrit-Project: Impa=
la-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: =
I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd
Gerrit-Change-Number: 8270
Gerrit=
-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera=2Ecom>
Gerrit-Re=
viewer: Michael Ho <kwho@cloudera=2Ecom>
Gerrit-Reviewer: Sailesh Mukil <sa=
ilesh@cloudera=2Ecom>
Gerrit-Comment-Date: Tue, 28 Nov 2017 15:00:03 +0000
=
Gerrit-HasComments: Yes

--dsNul+HAHVQ=--