Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20591 )
Change subject: IMPALA-12505: Define a new impala flag that runs the trusted domain check on the origin address if the trusted_domain_use_xff_header flag is enabled and no X-Forwarded-For header is received ...................................................................... Patch Set 2: (4 comments) looks good, few comments http://gerrit.cloudera.org:8080/#/c/20591/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20591/2//COMMIT_MSG@7 PS2, Line 7: Define a new impala flag that runs the trusted domain : check on the origin address if the trusted_domain_use_xff_header : flag is enabled and no X-Forwarded-For header is received It would be nice to have a shorter first line, e.g. "Add flag for trusted domain check to use 'origin' if xff header is not set" http://gerrit.cloudera.org:8080/#/c/20591/2//COMMIT_MSG@11 PS2, Line 11: flag The flag name could be mentioned http://gerrit.cloudera.org:8080/#/c/20591/2/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java File fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java: http://gerrit.cloudera.org:8080/#/c/20591/2/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@506 PS2, Line 506: // Case 1: Authenticate as 'Test1Ldap' without password, send X-Forwarded-For header Can you add some negative tests too (also in webserver tests)? I see two cases to test: a. trusted_domain_empty_xff_header_use_origin is enabled, there is no xff header, and the origin does not match trusted_domain b. trusted_domain_empty_xff_header_use_origin is enabled and the origin matches trusted_domain, but xff header is set to a value that does not match trusted_domain http://gerrit.cloudera.org:8080/#/c/20591/2/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@518 PS2, Line 518: verifyTrustedDomainMetrics(1); This could be also verified to be 0 at the beginning. -- To view, visit http://gerrit.cloudera.org:8080/20591 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58e5d1119527139eafaa411b55517b10bf394bb2 Gerrit-Change-Number: 20591 Gerrit-PatchSet: 2 Gerrit-Owner: Gergely Farkas <gfar...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Mon, 13 Nov 2023 11:40:17 +0000 Gerrit-HasComments: Yes
