Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client ...................................................................... Patch Set 5: (24 comments) http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.h File src/kudu/hms/mini_hms.h: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.h@82 PS5, Line 82: "127.0.0.1" > Are we sure that this won't clash with some other test services that may be Yes, it uses an ephemeral port. If it's shutdown and started again it will use the same port, but ephemeral ports tend not to get used again immediately. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc@68 PS5, Line 68: CHECK(!krb5_conf.empty()); > worth CHECK(!hms_process_) here too to ensure that it's set up before start Done http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc@240 PS5, Line 240: : <property> : <name>hadoop.rpc.protection</name> : <value>$5</value> : </property> > odd this is in hive-site and not core-site... but guess it works Despite the property name this is applying to the HMS thrift server interface, not an HRPC interface. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc File src/kudu/hms/sasl_client_transport.cc: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@53 PS5, Line 53: Width > "width" strikes me as odd relative to "Size" Done http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@56 PS5, Line 56: kFrameHeaderWidth > nit: Same as Todd's comment above. Done http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@107 PS5, Line 107: sasl_helper_.EnableGSSAPI(); > Does this mean this won't work with SASL plain? Correct, the HMS doesn't appear to use SASL plain. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@120 PS5, Line 120: transport_->open(); > DCHECK(!isOpen()); Done http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123 PS5, Line 123: ... > catch(TException& e) ? That doesn't catch all exceptions. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@157 PS5, Line 157: ResetReadBuf(); > are we guaranteed that read_slice_.data() != read_buf_.data()? ie it's alwa Done http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@177 PS5, Line 177: // If we have some data in the read buffer, then return it. : if (!read_slice_.empty()) { : return read_fast(); : } : : // Otherwise, read a new frame from the wire and return it. : ReadFrame(); : return read_fast(); > this code is a bit odd to me. why not just do: Done http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@224 PS5, Line 224: ResetWriteBuf(); > the shrink_to_fit() in here seems a bit odd -- if you're doing a multi-fram Yes, this is the only place I could find that was appropriate for releasing the write buffer. Otherwise the connection will hold on to the buffer indefinitely, and never shrink it. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@254 PS5, Line 254: !s.IsIncomplete() && !s.ok()) > sasl_client_step() could return SASL_OK or SASL_CONTINUE, both of which are WrapSaslCall will convert SASL_CONTINUE to a Status::Incomplete value, and SASL_OK to a Status::OK, both of which are handled here. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@274 PS5, Line 274: needs_wrap_ = rpc::NeedsWrap(sasl_conn_.get()); > Based on how you're initializing the transport, this looks like it should b I'm not following, in this line needs_wrap_ is being retrieved from the SASL connection, which negotiates it with the remote server. Ultimately, whether auth-conf, auth-int or no wrap is needed is determined by server configuration. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@284 PS5, Line 284: payload.size() > nit: Not that it's likely, but Slice objects can have lengths greater than Done http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@292 PS5, Line 292: // Read the fixed-length message header. > DCHECK_EQ(payload.size(), 0); I've changed it so that payload gets overwritten, so the caller doesn't need to clear. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@343 PS5, Line 343: s = rpc::AllowProtection(sasl_conn_.get()); : if (!s.ok()) { : throw SaslException(s); : } > Integrity and confidentiality is set as a strict requirement here. Is that This is not setting a strict requirement, it's allowing for integrity and confidentiality. We leave it up to the server to set the requirement (min_ssf). http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@363 PS5, Line 363: "hive" > #define this with a comment stating that we'll only be talking to hive for I've added a TODO. I don't think there's any point in defining a constant since we'll ultimately want to pass it in anyway. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@378 PS5, Line 378: resize(0); > nit: clear() Done http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h File src/kudu/rpc/sasl_common.h: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@116 PS5, Line 116: // The ciphertext data must not be greater than the maximum buffer size. > not sure how to interpret this comment, since ciphertext is an out-param This is a typo. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@122 PS5, Line 122: Slice* ciphertext) WARN_UNUSED_RESULT; > think this param needs a bit more doc - it's an in-out param, right? is it Done http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@126 PS5, Line 126: // The encoded data must not be greater than the maximum buffer size. > same This is a little subtle - it's saying that the plaintext must not be longer than the maximum negotiated buffer size. This is entirely controlled by whomever encoded the data to begin with, but getting it wrong will cause the decode to fail. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc File src/kudu/rpc/sasl_common.cc: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@48 PS5, Line 48: extern const size_t kSaslMaxOutBufLen = 64 * 1024; > if this is our max frame length for thrift connections to the HMS, is this Yes, unfortunately right now we'll fail to receive large objects sent by the HMS when auth-int or auth-conf is enabled. Ultimately this is due to a bug in the Thrift Java implementation, THRIFT-4483. At this point I don't know what to do about this, since this has been broken for years and I doubt it will change. I don't know of a workaround on our side, since it's caused by Thrift not being RFC 4422 compliant, while Cyrus SASL is. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@356 PS5, Line 356: No max output buffer size > wrong message here Done http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@367 PS5, Line 367: Status SaslEncode(sasl_conn_t* conn, Slice plaintext, Slice* ciphertext) { > we seem to have lost the looping behavior in the case that the input is lar Yeah, turns out the looping never worked, anyway. I've updated the Status returned in this case to hint that it may be due to a too-long message. -- To view, visit http://gerrit.cloudera.org:8080/8692 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345 Gerrit-Change-Number: 8692 Gerrit-PatchSet: 5 Gerrit-Owner: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 31 Jan 2018 20:08:30 +0000 Gerrit-HasComments: Yes
