Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16833 )
Change subject: IMPALA-10496: SAML implementation in Impala ...................................................................... Patch Set 23: (14 comments) I mostly have some minor suggestions, but overall the patch looks good to me and I can +1 once the comments are resolved. Will wait for Thomas to look at this patch as well. http://gerrit.cloudera.org:8080/#/c/16833/23//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16833/23//COMMIT_MSG@10 PS23, Line 10: POC in Hive I think mentioning the upstream HIVE jira (HIVE-24543) here would be a useful reference since it links to the design doc. http://gerrit.cloudera.org:8080/#/c/16833/23/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/16833/23/be/src/common/global-flags.cc@352 PS23, Line 352: nit, blank new line. http://gerrit.cloudera.org:8080/#/c/16833/23/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/16833/23/be/src/rpc/authentication.cc@1338 PS23, Line 1338: sap->InitSaml(); do we need to add a similar msg as above for SAML? http://gerrit.cloudera.org:8080/#/c/16833/23/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/16833/23/be/src/service/frontend.h@201 PS23, Line 201: Status GetSaml2Redirect(const TWrappedHttpRequest& request, : TWrappedHttpResponse* response); : : Status ValidateSaml2Response( : const TWrappedHttpRequest& request, TWrappedHttpResponse* response); Can we add some comments here? http://gerrit.cloudera.org:8080/#/c/16833/23/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/16833/23/common/thrift/metrics.json@1318 PS23, Line 1318: "CATALOGSERVER", : "STATESTORE" Do we need this for catalogd and statestored? http://gerrit.cloudera.org:8080/#/c/16833/23/common/thrift/metrics.json@1330 PS23, Line 1330: "CATALOGSERVER", : "STATESTORE" Do we need this for catalogd and statestored? http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/AuthTokenGenerator.java File fe/src/main/java/org/apache/impala/authentication/saml/AuthTokenGenerator.java: http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/AuthTokenGenerator.java@20 PS23, Line 20: https://github.com/vihangk1/hive/blob/master_saml This points to a private branch. Unfortunately, we have not merged this yet in the master branch. This comment might need an update once the Hive patch is merged. http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlHttpServlet.java File fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlHttpServlet.java: http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlHttpServlet.java@58 PS23, Line 58: //TODO(Vihang) do we need a https://localhost here? This can be removed. Also, the latest hive patch uses 127.0.0.1 instead of localhost as per the specification RFC 8252 section 7.3 (https://tools.ietf.org/html/rfc8252#section-7.3). http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlHttpServlet.java@66 PS23, Line 66: //TODO(Vihang) do we need a https://localhost her this can be removed. Also, please replace localhost with 127.0.0.1 http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlRelayStateInfo.java File fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlRelayStateInfo.java: http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlRelayStateInfo.java@23 PS23, Line 23: codeVerifier the latest hive patch renames this field to clientIdentifier which is more readable in my opinion. http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClient.java File fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClient.java: http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClient.java@76 PS23, Line 76: conf.getSaml2SpCallbackUrl() Do we need to validate this call back URL? e.g. the port number is same as http port? and it is using https? http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/ImpalaSamlClient.java@196 PS23, Line 196: codeVerifier may be rename this to clientIdentifier. http://gerrit.cloudera.org:8080/#/c/16833/23/tests/custom_cluster/test_saml2_sso.py File tests/custom_cluster/test_saml2_sso.py: http://gerrit.cloudera.org:8080/#/c/16833/23/tests/custom_cluster/test_saml2_sso.py@37 PS23, Line 37: class NoRedirection(urllib2.HTTPErrorProcessor): I think it would be generally more readable if we have a class level comment giving a high level overview of what the test is doing. http://gerrit.cloudera.org:8080/#/c/16833/23/tests/custom_cluster/test_saml2_sso.py@71 PS23, Line 71: wIT nit, WITH -- 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: 23 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Tue, 16 Feb 2021 18:33:36 +0000 Gerrit-HasComments: Yes
