Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16833 )
Change subject: [WiP] SAML implementation in Impala ...................................................................... Patch Set 15: (11 comments) Sorry for the many small patchsets, I missed some comments on the first run. Note that the Hive implementation is still on review (https://github.com/apache/hive/pull/1791 ), so some parts can change, e.g. header/cookie names. 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: saml2_keystore_password > Its probably better to make this 'saml2_keystore_password_cmd', similar to Thanks for spotting this - I postponed changing it for now, as the plan is to make this as similar with Hive as possible, so we should find a similar solution there too. http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/common/global-flags.cc@349 PS11, Line 349: keystore > typo Done http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/common/global-flags.cc@361 PS11, Line 361: --hs2_http_por > Might be worth specifically saying this is the --hs2_http_port flag. Done 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' Yes, it should be false only during testing, as the signature proves that the user actually signed in at identity provider. 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? Added seconds to the description. 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: of > typo Done 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: se if (THRIFT_strncasecmp(header, "Expect", sz) == 0) { > Any reason to have two copies of this string, rather than just having 'head I moved setting this to the SAML part of headersDone(). auth_value_ is used by Kerberos/LDAP too, which do not use wrapped_request_, so I think it is simpler to use auth_value_ until the point where we actually need the wrapped_request_. http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/transport/THttpServer.cpp@219 PS11, Line 219: cookies, as t > As noted elsewhere, I find the name 'readWholeBody_' to be confusing. Maybe I choose readWholeBodyForAuth_ to make it more informative, but I can switch to other names. http://gerrit.cloudera.org:8080/#/c/16833/11/be/src/transport/THttpServer.cpp@232 PS11, Line 232: (!cookie_v > This will always be 'false' here Done 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 (readWholeBodyForAuth_) { > Are we guaranteed that the saml response won't be chunked? I guess presumab I think that it shouldn't be chunked - the content is a small compressed xml, max a few KB I guess. Unlike other http requests, this comes from the SP provider, which will not send further requests. Added a check to bodyDone() to throw an exception if chunked_ is true - I think that this is less error prone than adding logic to actually support. 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: // Contains all information from a HTTP request. > Needs some comments, at least for the overall struct, not necessarily every TSamlAuthnResponse was actually no longer used. Added comments for TWrappedHttpRequest and TWrappedHttpResponse. -- 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: 15 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: Tue, 19 Jan 2021 21:26:25 +0000 Gerrit-HasComments: Yes
