Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16833 )
Change subject: [WiP] SAML implementation in Impala ...................................................................... Patch Set 17: (7 comments) http://gerrit.cloudera.org:8080/#/c/16833/15/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/16833/15/be/src/common/global-flags.cc@347 PS15, Line 347: // HS2 SAML2.0 configuration > I guess some of these configurations have place holder descriptions and you Yes - have extended the descriptions, mainly based on the Hive solution. http://gerrit.cloudera.org:8080/#/c/16833/15/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/16833/15/be/src/rpc/authentication.cc@599 PS15, Line 599: purposes > nit, spelling Done http://gerrit.cloudera.org:8080/#/c/16833/15/be/src/transport/THttpServer.h File be/src/transport/THttpServer.h: http://gerrit.cloudera.org:8080/#/c/16833/15/be/src/transport/THttpServer.h@165 PS15, Line 165: // SAML can be used alongside LDAP or Kerberos - if the SAML related path of headers : // are not detected, Impala fall back to other authentications. : bool has_saml_ = false; > Does this mean that Impala can support both SAML and Ldap concurrently? Has I always planned to do it like this - my assumption is that while SAML browser profile is very useful when a user can do manual interactions like logging in in the browser, it is not suitable for programmatic access, e.g. when a tool like Hue authenticates the users and connects to Impala. For this reason I kept trusted domain + Ldap + Kerberos auth as fallbacks if they are configured. http://gerrit.cloudera.org:8080/#/c/16833/15/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/16833/15/be/src/transport/THttpServer.cpp@137 PS15, Line 137: > Are you planning to add engine specific identifiers in these header names? No, I plan to use the exact same names as Hive to make client development easier. http://gerrit.cloudera.org:8080/#/c/16833/15/be/src/transport/THttpServer.cpp@261 PS15, Line 261: TryStripP > if the bearer token is present but not valid, do we reach here? I have excluded this case in patch 16 + 17: 1. if X-Token-Response-Port is present, we always redirect 2. if not then we check for bearer token 3. if there is no X-Token-Response-Port nor bearer token we fallback to other mechanisms http://gerrit.cloudera.org:8080/#/c/16833/15/common/thrift/BackendGflags.thrift File common/thrift/BackendGflags.thrift: http://gerrit.cloudera.org:8080/#/c/16833/15/common/thrift/BackendGflags.thrift@185 PS15, Line 185: token > s/roken/token/ Done http://gerrit.cloudera.org:8080/#/c/16833/15/fe/pom.xml File fe/pom.xml: http://gerrit.cloudera.org:8080/#/c/16833/15/fe/pom.xml@491 PS15, Line 491: <dependency> > pac4j brings in tons of dependencies. We should exclude as much as possible I excluded some dependencies similarly to Hive, but I couldn't exclude some others, as Impala does not depend on them yet, so excluding would mean that pac4j would miss a dependency. I will do another pass to bring the dependencies closer to Hive's. About reusing Hive classes: that would be great but would need the new classes to be present in a CDP version we use for dependencies. We can discuss whether this is possible in the near future. -- 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: 17 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: Fri, 05 Feb 2021 01:13:29 +0000 Gerrit-HasComments: Yes
