Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16833 )
Change subject: [WiP] SAML implementation in Impala ...................................................................... Patch Set 15: (10 comments) Thanks for taking this up! I took a first pass and left some questions/suggestions. I will take another pass at it once you respond/update the patch. 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: DEFINE_string(saml2_keystore_path, "", "Keystore path to the saml2 client."); I guess some of these configurations have place holder descriptions and you plan to add more details in subsequent patchset? 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: porpuses nit, spelling 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 it always been the case or something which was added in this patch? 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: Token Are you planning to add engine specific identifiers in these header names? eg. X-Impala-Token-Response-Port instead of X-Token-Response-Port. In case of Hive, we use X-Hive-Token-Response-Port http://gerrit.cloudera.org:8080/#/c/16833/15/be/src/transport/THttpServer.cpp@261 PS15, Line 261: authorized if the bearer token is present but not valid, do we reach here? 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: roken s/roken/token/ 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. Also, I found that it includes some dependencies which have known security vulnerabilities. I have excluded those and separated added the updated versions here: https://github.com/apache/hive/pull/1791/commits/e92e911dd33a19f938abb44aa8d7364d9fcc0129 You may want to do the same. the other way (may be a followup later) could be to add the Hive classes with the hive-exec reduced jar and try to reuse them. http://gerrit.cloudera.org:8080/#/c/16833/15/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlAuthTokenGenerator.java File fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlAuthTokenGenerator.java: http://gerrit.cloudera.org:8080/#/c/16833/15/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlAuthTokenGenerator.java@39 PS15, Line 39: HiveSamlAuthTokenGenerator Ideally, it would great to use these classes by importing them via hive-exec reduced jar that we already have in the classpath. That way updates to this class would be available for free to us. On the flip side, hive side should make sure changes to these files don't break Impala. I can annotate the classes as LimitedPrivate("Apache Impala") if that helps. I understand the need to copy was due to the fact that we are having the configs from BackendConfig. May be I can introduce a build class so that we can pass the values appropriately. Thoughts? http://gerrit.cloudera.org:8080/#/c/16833/15/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/16833/15/fe/src/main/java/org/apache/impala/service/JniFrontend.java@716 PS15, Line 716: frontend_.getSaml2Client().setRedirect(webContext); I think it would help to improve the readability if we state that the this method adds the http status code and the redirect location in the response object. http://gerrit.cloudera.org:8080/#/c/16833/15/tests/custom_cluster/test_saml2_sso.py File tests/custom_cluster/test_saml2_sso.py: http://gerrit.cloudera.org:8080/#/c/16833/15/tests/custom_cluster/test_saml2_sso.py@70 PS15, Line 70: # SAML2 should not affect non http protocols. : args = ["--protocol=%s" % protocol, "-q", "select 1 + 2"] : run_impala_shell_cmd(vector, args, expect_success=True) : continue Does this mean that when SAML is configured, users can continue to use binary protocol over LDAP/Kerberos? -- 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: Fri, 29 Jan 2021 01:32:13 +0000 Gerrit-HasComments: Yes
