Abhishek Rawat 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 4: (7 comments) It appears IPV6 addresses aren't supported in Impala at many places and would probably require separate effort to support them. 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: if the deployment environment protects against spoofing. : : Also, added a new function 'GetXFFOriginClientAddress' for parsing XFF : header with comma separated IP addresses, whi > Should we mention that checking the validity of a given IP address is out o Updated commit message mainly inspired from IMPALA-13310 http://gerrit.cloudera.org:8080/#/c/21780/3/be/src/rpc/authentication-test.cc File be/src/rpc/authentication-test.cc: http://gerrit.cloudera.org:8080/#/c/21780/3/be/src/rpc/authentication-test.cc@308 PS3, Line 308: xff_addresses = " 10.90.11.23, "; > case: space between the first ip and the comma. Done 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. Thanks for pointing this out. It appears there are gaps in code and most of our logic assumes IPV4 addresses. As I was researching, I found IMPALA-12713 and updated it with my findings. Supporting IPV6 addresses properly would probably be separate effort. 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: > It seems that 'vector' is not needed as an input argument. Removed vector. http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py@117 PS3, Line 117: '--hs2_x_forw > Is my understanding correct that '--hs2_x_forward= ' is considered an in Maybe not since it's treated as an empty header which is similar to not including the XFF header? http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py@135 PS3, Line 135: '"resType":"@table",' : '"action":"select",' : '"result":1,' : '"agent":"impala",' : '"policy":\d,' : '"enforcer":"ranger-acl",' : '"cliIP":"%s",' : '"reqData":"%s",' : '".+":".+","logType":"RangerAudit"' : ) : > Are these strings exist in single line or multiline? Why not craft regex fo Added regex. http://gerrit.cloudera.org:8080/#/c/21780/3/tests/authorization/test_ranger.py@135 PS3, Line 135: '"resType":"@table",' : '"action":"select",' : '"result":1,' : '"agent":"impala",' : '"policy":\d,' : '"enforcer":"ranger-acl",' : '"cliIP":"%s",' : '"reqData":"%s",' : '".+":".+","logType":"RangerAudit"' : ) : : # Ensure audit logs were logged in coordinator logs : self.assert_impalad_log_contains("INFO", expected_strings_valid_xff % : ("10.20.30.40", "select count\(1\) from functional.alltypes"), expected_count=3) : self.assert_impalad_log_contains("INFO", expected_strings_valid_xff % : ("127.0.0.1", "select count\(2\) from functional.alltypes"), expected_count=2) : self.assert_impalad_log_contains("INFO", expected_strings_valid_xff % : ("foobar", "select count\(3\) from functional.alltypes"), expected_count=1) : : @pytest.mark.execute_serially : @CustomClusterTestSuite.with_args( : impalad_args=IMPALAD_ARGS, catalogd_args=CATALOGD_ARGS) : def test_grant_revoke_with_catalog_v1(self, unique_name): : """Tests grant/revoke with catalog v1.""" : self._test_grant_revoke(unique_name, [None, "invalidate metadata", : "refresh authorization"]) : : @pytest.mark.execute_serially : @SkipIfFS.hdfs_acls : @CustomClusterTestSuite.with_args( : impalad_args=IMPALAD_ARGS, catalogd_args=CATALOGD_ARGS) : def test_insert_with_catalog_v1(self, unique_name): : """ : Test that when Ranger is the authorization provider in the legacy catalog mode, : Impala does not throw an AnalysisException when an authorized user tries to execute : an INSERT query against a partitioned table of which the respective table path and : the partition path are not writable according to HDFS permission. : """ : user = getuser() : admin_client = self.create_impala_client() : unique_database = unique_name + "_db" : unique_table = unique_name + "_tbl" : partition_column = "year" : partition_value = "2008" : table_path = "test-warehouse/{0}.db/{1}".format(unique_database, unique_table) : table_partition_path = "{0}/{1}={2}"\ : .format(table_path, partition_column, partition_value) : insert_statement = "insert into {0}.{1} (name) partition ({2}) " \ : "values (\"Adam\", {3})".format(unique_database, unique_table, partition_column, : partition_value) : authz_err = "AuthorizationException: User '{0}' does not have privileges to " \ : "execute 'INSERT' on: {1}.{2}".format(user, unique_database, unique_table) : try: : admin_client.execute("drop database if exists {0} cascade" : .format(unique_database), user=ADMIN) : admin_client.execute("create database {0}".format(unique_database), user=ADMIN) : admin_client.execute("create table {0}.{1} (name string) partitioned by ({2} int)" : .format(unique_database, unique_table, partition_column), user=ADMIN) : admin_client.execute("alter table {0}.{1} add partition ({2}={3})" : .format(unique_database, unique_table, partition_column, partition_value), : user=ADMIN) : : # Change the owner user and group of the HDFS paths corresponding to the table and : # the partition so that according to Impala's FsPermissionChecker, the table is not : # writable to the user that loads the table. This user usually is the one : # > I was wondering if the test would be a bit more concise if we use a base li I added regex based on other review comment. Please take a look if the latest test looks good. -- 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: 4 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: Thu, 26 Sep 2024 02:47:49 +0000 Gerrit-HasComments: Yes
