Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/21780 )
Change subject: IMPALA-13312: Use client address from X-Forwarded-For Header in Ranger Audit Logs ...................................................................... Patch Set 3: (5 comments) Thanks for working on this Abhishek! I left some minor comments. I have a question regarding whether IPv6 addresses are supported by this patch. If IPv6 addresses could not be passed to Impala's frontend, it's also okay. We could create a follow-up JIRA for it. :-) http://gerrit.cloudera.org:8080/#/c/21780/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21780/3//COMMIT_MSG@15 PS3, Line 15: Also, added a new function 'GetXFFOriginClientAddress' for parsing XFF : header with comma separated IP addresses, which is the most common form : of XFF header representing client and intermediate proxies: : X-Forwarded-For: <client>, <proxy1>, <proxy2> Should we mention that checking the validity of a given IP address is out of the scope of this patch? http://gerrit.cloudera.org:8080/#/c/21780/3/be/src/rpc/authentication-util.h File be/src/rpc/authentication-util.h: http://gerrit.cloudera.org:8080/#/c/21780/3/be/src/rpc/authentication-util.h@56 PS3, Line 56: Status GetXFFOriginClientAddress(const std::string_view& xff_addresses, : std::string& origin); I am wondering if IPv6 addresses are supported by this patch. For instance, I tried the following on the command line to connect to Impala server via impala-shell using a valid IPv6 address. impala-shell.sh --protocol=hs2-http --hs2_x_forward='2001:db8:3333:4444:5555:6666:7777:8888' -u admin I ran "select count(*) from functional.alltypes limit 5" and found the following audit event in impalad.INFO. It looks like the value of 'cliIP' was an empty string. Not very sure if I missed anything. I0925 16:17:48.625419 29319 Log4JAuditDestination.java:107] {"repoType":3,"repo":"test_impala","reqUser":"admin","evtTime":"2024-09-25 16:17:47.385","access":"select","resource":"functional/alltypes","resType":"@table","action":"select","result":1,"agent":"impala","policy":2,"enforcer":"ranger-acl","cliIP":"","reqData":"select count(*) from functional.alltypes limit 5","agentHost":"fangyu-upstream-dev.gce.cloudera.com","logType":"RangerAudit","id":"b769016b-295a-4d18-ae61-756add066f5b-0","seq_num":0,"event_count":1,"event_dur_ms":0,"tags":[],"cluster_name":"test-cluster","policy_version":1} http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py@82 PS3, Line 82: vector It seems that 'vector' is not needed as an input argument. http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py@117 PS3, Line 117: # Invalid XFF headers Is my understanding correct that '--hs2_x_forward= ' is considered an invalid XFF header? http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py@135 PS3, Line 135: expected_strings_valid_xff_1 = [ : '"access":"select"', : '"resource":"functional/alltypes"', : '"resType":"@table"', : '"action":"select"', : '"agent":"impala"', : '"enforcer":"ranger-acl"', : '"cliIP":"10.20.30.40"', : '"reqData":"select count(*) from functional.alltypes"', : '"logType":"RangerAudit"' : ] : expected_strings_valid_xff_2 = [ : '"access":"select"', : '"resource":"functional/alltypes"', : '"resType":"@table"', : '"action":"select"', : '"agent":"impala"', : '"enforcer":"ranger-acl"', : '"cliIP":"10.20.30.40"', : '"reqData":"select count(1) from functional.alltypes"', : '"logType":"RangerAudit"' : ] : expected_strings_valid_xff_3 = [ : '"access":"select"', : '"resource":"functional/alltypes"', : '"resType":"@table"', : '"action":"select"', : '"agent":"impala"', : '"enforcer":"ranger-acl"', : '"cliIP":"10.20.30.40"', : '"reqData":"select count(2) from functional.alltypes"', : '"logType":"RangerAudit"' : ] : expected_strings_no_xff = [ : '"access":"select"', : '"resource":"functional/alltypes"', : '"resType":"@table"', : '"action":"select"', : '"agent":"impala"', : '"enforcer":"ranger-acl"', : '"cliIP":"127.0.0.1"', : '"reqData":"select count(3) from functional.alltypes"', : '"logType":"RangerAudit"' : ] : expected_strings_empty_xff = [ : '"access":"select"', : '"resource":"functional/alltypes"', : '"resType":"@table"', : '"action":"select"', : '"agent":"impala"', : '"enforcer":"ranger-acl"', : '"cliIP":"127.0.0.1"', : '"reqData":"select count(4) from functional.alltypes"', : '"logType":"RangerAudit"' : ] : expected_strings_invalid_xff = [ : '"access":"select"', : '"resource":"functional/alltypes"', : '"resType":"@table"', : '"action":"select"', : '"agent":"impala"', : '"enforcer":"ranger-acl"', : '"cliIP":"foobar"', : '"reqData":"select count(5) from functional.alltypes"', : '"logType":"RangerAudit"' : ] I was wondering if the test would be a bit more concise if we use a base list containing the constant fields in an audit event in our test cases. To construct a list of fields to check for a given query, we append to the base list the varying fields, i.e., 'cliIP' and 'reqData'. expected_strings_base = [ '"access":"select"', '"resource":"functional/alltypes"', '"resType":"@table"', '"action":"select"', '"agent":"impala"', '"enforcer":"ranger-acl"', '"logType":"RangerAudit"' ] expected_strings_example = list(expected_strings_base) expected_strings_example.append('"cliIP":"foobar"') expected_strings_example\ .append('"reqData":"select count(5) from functional.alltypes"') self.assert_log_file(self.LOG_DIR_RANGER_AUDIT_XFF, "impalad.INFO", expected_strings_example, "Log4JAuditDestination.java") -- To view, visit http://gerrit.cloudera.org:8080/21780 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib784ad805c649e9576ef34f125509c904b7773ab Gerrit-Change-Number: 21780 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Wed, 25 Sep 2024 23:59:36 +0000 Gerrit-HasComments: Yes
