Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/16833 )
Change subject: [WiP] SAML implementation in Impala ...................................................................... Patch Set 11: (11 comments) http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/common/global-flags.cc@349 PS11, Line 349: keystorp typo http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/common/global-flags.cc@349 PS11, Line 349: saml2_keystore_password Its probably better to make this 'saml2_keystore_password_cmd', similar to how we do flags like 'ssl_private_key_password_cmd', to allow users to keep passwords out of configs/logs. You can also tag it as sensitive so that it will be redacted, see 'ldap_bind_password_cmd' for an example. And of course the same for 'saml2_private_key_password' http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/common/global-flags.cc@361 PS11, Line 361: hs2-http port. Might be worth specifically saying this is the --hs2_http_port flag. http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/common/global-flags.cc@363 PS11, Line 363: DEFINE_bool(saml2_want_assertations_signed, true, Is there any situation where it would be reasonable to set this to 'false' other than just for testing? If not, might be worth explicitly saying 'just for testing' in the description. http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/common/global-flags.cc@372 PS11, Line 372: saml2_callback_roken_ttl What's the units on this, seconds? http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/transport/THttpServer.h File be/src/transport/THttpServer.h: http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/transport/THttpServer.h@151 PS11, Line 151: ot typo http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/transport/THttpServer.cpp@132 PS11, Line 132: wrapped_request_->headers["Authorization"] = auth_value_; Any reason to have two copies of this string, rather than just having 'headersDone()' inspect the value from wrapped_request_? http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/transport/THttpServer.cpp@219 PS11, Line 219: readWholeBody_ As noted elsewhere, I find the name 'readWholeBody_' to be confusing. Maybe just 'isSamlResp_', or if you want to keep it more general than just for SAML something like 'needBodyForAuth'. http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/transport/THttpServer.cpp@232 PS11, Line 232: authorized This will always be 'false' here http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/transport/THttpTransport.cpp File be/src/transport/THttpTransport.cpp: http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/transport/THttpTransport.cpp@104 PS11, Line 104: if (readWholeBody_) { Are we guaranteed that the saml response won't be chunked? I guess presumably the saml response is pretty small and therefore shouldn't ever be chunked, but might be worth checking for this and at least logging an error in that case. Or better, modify it to work in the chunked case too, just in case, eg. by calling bodyDone() here is readHeaders_ is true. I also don't think its necessary to have the 'readWholeBody_' variable in THttpTransport - we can just always call bodyDone() when the body is done, and the logic in THttpServer::bodyDone can decide if it actually needs to process the body, especially since I find the name 'readWholeBody' confusing (it brings up the question - don't we always read the whole body?) http://gerrit.cloudera.org:8080/#/c/16833/11/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/16833/11/common/thrift/Frontend.thrift@988 PS11, Line 988: struct TSamlAuthnResponse { Needs some comments, at least for the overall struct, not necessarily every param, here and below -- To view, visit http://gerrit.cloudera.org:8080/16833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia0c026cba1b90e7ff6ec5ae49be78b0d1edd8dfa Gerrit-Change-Number: 16833 Gerrit-PatchSet: 11 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Thu, 14 Jan 2021 21:47:52 +0000 Gerrit-HasComments: Yes
