Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16833 )
Change subject: IMPALA-10496: SAML implementation in Impala ...................................................................... Patch Set 24: (13 comments) 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: implementa > I think mentioning the upstream HIVE jira (HIVE-24543) here would be a usef Done 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. Done 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: LOG(INFO) << "External communication can be also authenticated with SAML2 SSO"; > do we need to add a similar msg as above for SAML? Done 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: // Call FE to create a http response that redirects to the SSO service. : Status GetSaml2Redirect(const TWrappedHttpRequest& request, : TWrappedHttpResponse* response); : : // Call FE to validate the SAML2 AuthNResponse. > Can we add some comments here? Done 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: ], : "label": "Hive > Do we need this for catalogd and statestored? Thanks for spotting - actually the description and the label were also wrong. http://gerrit.cloudera.org:8080/#/c/16833/23/common/thrift/metrics.json@1330 PS23, Line 1330: "units": "NONE", : "kind": "COUNT > Do we need this for catalogd and statestored? Done 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 Yes, I plan to update these once the Hive patch is merged, but this may happen in a follow up patch if this change is merged before Hive. 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: generateFormData(webContext, "http://127.0.0.1:"; + > This can be removed. Also, the latest hive patch uses 127.0.0.1 instead of Done http://gerrit.cloudera.org:8080/#/c/16833/23/fe/src/main/java/org/apache/impala/authentication/saml/HiveSamlHttpServlet.java@66 PS23, Line 66: tokenGenerator.get(nameId, relayState), true, > this can be removed. Also, please replace localhost with 127.0.0.1 Done 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: g clientIden > the latest hive patch renames this field to clientIdentifier which is more Done 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: > Do we need to validate this call back URL? e.g. the port number is same as This path is already parsed in the backend, so I think that it is easier to check this there. I will add validation in a later patch. 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: return response > I think it would be generally more readable if we have a class level commen Done http://gerrit.cloudera.org:8080/#/c/16833/23/tests/custom_cluster/test_saml2_sso.py@71 PS23, Line 71: ge > nit, WITH Done -- 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: 24 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: Wed, 17 Feb 2021 00:26:18 +0000 Gerrit-HasComments: Yes
