You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Gergely Farkas (Code Review)" <ge...@cloudera.org> on 2023/08/02 14:24:19 UTC
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Gergely Farkas has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20301
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
IMPALA-12328: New option for impala-shell to send "auth" header in thrift
http request
This change introduces a new optional impala-shell option
(send_auth_header_for_hiveserver2) to add an 'auth' header to http
requests, which is expected in latest hiveserver2 builds
when supporting multiple authentication modes at once
(see HIVE-27352).
The 'auth' header shouldn't be a problem for the impala coordinator,
but unfortunately it may prevent authentication due to a buggy header
parser code. This change will also fix that bug and introduces new
java frontend tests to validate the changes.
Some information on the http header parsing bug in THttpServer:
Unfortunately, the THRIFT_strncasecmp() function used in the original
implementation was true even if the name of the header being processed
was a prefix of the header name that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
Unfortunately, this can break authentication if the http request
has a header with a name that is a prefix to the word "Authorization".
Tested with a snapshot build in a CDP PVC DS environment where
both LDAP and Kerberos authentication was enabled:
- connected to Impala Coordinator using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
- connected to Impala Coordinator using beeline and hive jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to Impala Coordinator using beeline and impala jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to hiveserver2 using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/impala_shell.py
M shell/option_parser.py
7 files changed, 89 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/1
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 1
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
IMPALA-12341: Fix http header parsing issue in thrift http server
This change fixes the following http header parsing bug in
THttpServer: The THRIFT_strncasecmp() function used in the
THttpServer::parseHeader() function returns true even if the name
of the header being processed is a prefix of the header constant
that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
This can break authentication if the http request has a header
with a name that is a prefix to the word "Authorization".
If the length of the checked header is included in the condition,
this problem is avoided, so this fix refactors the if conditions,
so that this check is present everywhere.
Tested with new custom cluster tests.
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Reviewed-on: http://gerrit.cloudera.org:8080/20301
Reviewed-by: Csaba Ringhofer <cs...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
3 files changed, 63 insertions(+), 18 deletions(-)
Approvals:
Csaba Ringhofer: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 16
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 12: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 12
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Aug 2023 19:25:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#9).
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
IMPALA-12328: New option for impala-shell to send "auth" header in thrift
http request
This change introduces a new optional impala-shell option
(send_auth_header_for_hiveserver2) to add an 'auth' header to http
requests, which is expected in latest hiveserver2 builds
when supporting multiple authentication modes at once
(see HIVE-27352).
The 'auth' header shouldn't be a problem for the impala coordinator,
but unfortunately it may prevent authentication due to a buggy header
parser code. This change will also fix that bug and introduces new
java frontend tests to validate the changes.
Some information on the http header parsing bug in THttpServer:
Unfortunately, the THRIFT_strncasecmp() function used in the original
implementation was true even if the name of the header being processed
was a prefix of the header name that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
Unfortunately, this can break authentication if the http request
has a header with a name that is a prefix to the word "Authorization".
Tested with a snapshot build in a CDP PVC DS environment where
both LDAP and Kerberos authentication was enabled:
- connected to Impala Coordinator using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
- connected to Impala Coordinator using beeline and hive jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to Impala Coordinator using beeline and impala jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to hiveserver2 using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/impala_shell.py
M shell/option_parser.py
7 files changed, 117 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/9
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 9
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 8: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 8
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Aug 2023 12:34:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#3).
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
IMPALA-12328: New option for impala-shell to send "auth" header in thrift
http request
This change introduces a new optional impala-shell option
(send_auth_header_for_hiveserver2) to add an 'auth' header to http
requests, which is expected in latest hiveserver2 builds
when supporting multiple authentication modes at once
(see HIVE-27352).
The 'auth' header shouldn't be a problem for the impala coordinator,
but unfortunately it may prevent authentication due to a buggy header
parser code. This change will also fix that bug and introduces new
java frontend tests to validate the changes.
Some information on the http header parsing bug in THttpServer:
Unfortunately, the THRIFT_strncasecmp() function used in the original
implementation was true even if the name of the header being processed
was a prefix of the header name that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
Unfortunately, this can break authentication if the http request
has a header with a name that is a prefix to the word "Authorization".
Tested with a snapshot build in a CDP PVC DS environment where
both LDAP and Kerberos authentication was enabled:
- connected to Impala Coordinator using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
- connected to Impala Coordinator using beeline and hive jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to Impala Coordinator using beeline and impala jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to hiveserver2 using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/impala_shell.py
M shell/option_parser.py
7 files changed, 91 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/3
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 3
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13704/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 8
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Aug 2023 08:05:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13707/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 9
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Aug 2023 16:33:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Jason Fehr, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#12).
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
IMPALA-12341: Fix http header parsing issue in thrift http server
This change fixes the following http header parsing bug in
THttpServer: The THRIFT_strncasecmp() function used in the
THttpServer::parseHeader() function returns true even if the name
of the header being processed is a prefix of the header constant
that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
This can break authentication if the http request has a header
with a name that is a prefix to the word "Authorization".
If the length of the checked header is included in the condition,
this problem is avoided, so this fix refactors the if conditions,
so that this check is present everywhere.
Tested with new custom cluster tests.
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
3 files changed, 61 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/12
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 12
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 11: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9573/
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 11
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Aug 2023 20:48:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Gergely Farkas has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 13:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20301/12/be/src/transport/THttpServer.h
File be/src/transport/THttpServer.h:
http://gerrit.cloudera.org:8080/#/c/20301/12/be/src/transport/THttpServer.h@162
PS12, Line 162: static const std::string HEADER_SAML2_TOKEN_RESPONSE_PORT;
> Not sure here, but prefixing with HEADER_ as the constants above would be m
Good point! I'll change the names to be more consistent.
http://gerrit.cloudera.org:8080/#/c/20301/12/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20301/12/be/src/transport/THttpServer.cpp@132
PS12, Line 132: inline const bool Matc
> We should simply use a function here IMO - THRIFT_strncasecmp is created wi
Okay, I'll introduce a new inline function instead of this.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 13
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Aug 2023 08:45:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 15:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13750/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 15
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Aug 2023 10:25:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Jason Fehr, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#14).
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
IMPALA-12341: Fix http header parsing issue in thrift http server
This change fixes the following http header parsing bug in
THttpServer: The THRIFT_strncasecmp() function used in the
THttpServer::parseHeader() function returns true even if the name
of the header being processed is a prefix of the header constant
that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
This can break authentication if the http request has a header
with a name that is a prefix to the word "Authorization".
If the length of the checked header is included in the condition,
this problem is avoided, so this fix refactors the if conditions,
so that this check is present everywhere.
Tested with new custom cluster tests.
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
3 files changed, 63 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/14
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 14
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 6: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9556/
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 6
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 19:23:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13722/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 11
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Aug 2023 16:27:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9573/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 11
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Aug 2023 16:31:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#4).
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
IMPALA-12328: New option for impala-shell to send "auth" header in thrift
http request
This change introduces a new optional impala-shell option
(send_auth_header_for_hiveserver2) to add an 'auth' header to http
requests, which is expected in latest hiveserver2 builds
when supporting multiple authentication modes at once
(see HIVE-27352).
The 'auth' header shouldn't be a problem for the impala coordinator,
but unfortunately it may prevent authentication due to a buggy header
parser code. This change will also fix that bug and introduces new
java frontend tests to validate the changes.
Some information on the http header parsing bug in THttpServer:
Unfortunately, the THRIFT_strncasecmp() function used in the original
implementation was true even if the name of the header being processed
was a prefix of the header name that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
Unfortunately, this can break authentication if the http request
has a header with a name that is a prefix to the word "Authorization".
Tested with a snapshot build in a CDP PVC DS environment where
both LDAP and Kerberos authentication was enabled:
- connected to Impala Coordinator using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
- connected to Impala Coordinator using beeline and hive jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to Impala Coordinator using beeline and impala jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to hiveserver2 using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/impala_shell.py
M shell/option_parser.py
7 files changed, 124 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/4
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 4
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9558/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 7
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 18:09:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9579/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 11
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Aug 2023 07:15:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 11: Code-Review+1
(2 comments)
lgtm, just some improvement ideas
http://gerrit.cloudera.org:8080/#/c/20301/11//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20301/11//COMMIT_MSG@7
PS11, Line 7: IMPALA-12341
Just realized that this issue should also appear in Apache Thrift's c++ lib, as we copied the original of this class from there:
https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/THttpServer.cpp
The issue looks relevant there too, so it would be nice to also create a THRIFT ticket.
http://gerrit.cloudera.org:8080/#/c/20301/11/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20301/11/be/src/transport/THttpServer.cpp@143
PS11, Line 143: static const char* TRANSFER_ENCODING = "Transfer-Encoding";
+1 for moving these to constants
If already doing this, it would be nice to unify the handling of these header names - these use const char* while HEADER_REQUEST_ID/HEADER_IMPALA_SESSION_ID/HEADER_IMPALA_QUERY_ID are const std::string
I see no reason to treat these differently and using the same type would allow the length checking logic to be merged to THRIFT_strncasecmp (or another functions that calls THRIFT_strncasecmp). I would vote on using std::string, as I am not sure whether strlen is calculated in compile time, end if not, then this can add more overhead than std::string.length().
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 11
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Aug 2023 09:51:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 15: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 15
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Aug 2023 10:45:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#10).
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
IMPALA-12328: New option for impala-shell to send "auth" header in thrift
http request
This change introduces a new optional impala-shell option
(send_auth_header_for_hiveserver2) to add an 'auth' header to http
requests, which is expected in latest hiveserver2 builds
when supporting multiple authentication modes at once
(see HIVE-27352).
The 'auth' header shouldn't be a problem for the impala coordinator,
but unfortunately it may prevent authentication due to a buggy header
parser code. This change will also fix that bug and introduces new
java frontend tests to validate the changes.
Some information on the http header parsing bug in THttpServer:
Unfortunately, the THRIFT_strncasecmp() function used in the original
implementation was true even if the name of the header being processed
was a prefix of the header name that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
Unfortunately, this can break authentication if the http request
has a header with a name that is a prefix to the word "Authorization".
Tested with a snapshot build in a CDP PVC DS environment where
both LDAP and Kerberos authentication was enabled:
- connected to Impala Coordinator using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
- connected to Impala Coordinator using beeline and hive jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to Impala Coordinator using beeline and impala jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to hiveserver2 using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/impala_shell.py
M shell/option_parser.py
7 files changed, 118 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/10
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 10
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Gergely Farkas has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20301/10/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20301/10/be/src/transport/THttpServer.cpp@150
PS10, Line 150: && THRIFT_strncasecmp(header, TRANSFER_ENCODING, sz) == 0) {
> Based on my reading of the docs and some experimentation with code, we shou
Unfortunately, when parsing the following header
"Transfer-Encoding-the: fake",
the condition that does not use the 'sz' variable will be true, because the first 17 characters will match:
THRIFT_strncasecmp(header, TRANSFER_ENCODING, strlen(TRANSFER_ENCODING)) == 0
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 10
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Aug 2023 20:03:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/13692/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 2
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 14:52:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 13:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/13745/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 13
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Aug 2023 09:06:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 15:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9600/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 15
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Aug 2023 14:43:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Gergely Farkas has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20301/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20301/8//COMMIT_MSG@10
PS8, Line 10: This change introduces a new optional impala-shell option
: (send_auth_header_for_hiveserver2) to add an 'auth' header to http
: requests, which is expected in latest hiveserver2 builds
: when supporting multiple authentication modes at once
: (see HIVE-27352).
> There is already an option called strict_hs2_protocol which is used when we
I didn't dare to put it under strict_hs2_protocol, because then if someone uses a new impala-shell with an old impala build and sets strict_hs2_protocol when connecting to the coordinator, then the impala-shell will send the 'auth' header and authentication in hs2-http requests will not work because of the old impala build in the coordinator. I don't know much about impala users, if this is not a real use-case, then this can of course be added under strict_hs2_protocol.
http://gerrit.cloudera.org:8080/#/c/20301/8/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20301/8/be/src/transport/THttpServer.cpp@139
PS8, Line 139: _
> Thanks for fixing this bug!
Okay!
This length check was my original idea too, but the conditions seemed pretty ugly to me, so I moved to the current one. I'll revert back to that solution.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 8
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Aug 2023 14:59:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13731/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 12
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Aug 2023 15:14:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 11: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 11
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Aug 2023 11:36:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20301/2/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/20301/2/shell/impala_shell.py@644
PS2, Line 644:
flake8: E251 unexpected spaces around keyword / parameter equals
http://gerrit.cloudera.org:8080/#/c/20301/2/shell/impala_shell.py@667
PS2, Line 667:
flake8: E251 unexpected spaces around keyword / parameter equals
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 2
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 14:29:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 3:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/13693/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 3
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 14:58:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/13694/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 4
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 15:00:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 5:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/13695/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 5
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 15:05:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13699/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 7
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 18:08:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9564/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 8
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Aug 2023 08:16:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#8).
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
IMPALA-12328: New option for impala-shell to send "auth" header in thrift
http request
This change introduces a new optional impala-shell option
(send_auth_header_for_hiveserver2) to add an 'auth' header to http
requests, which is expected in latest hiveserver2 builds
when supporting multiple authentication modes at once
(see HIVE-27352).
The 'auth' header shouldn't be a problem for the impala coordinator,
but unfortunately it may prevent authentication due to a buggy header
parser code. This change will also fix that bug and introduces new
java frontend tests to validate the changes.
Some information on the http header parsing bug in THttpServer:
Unfortunately, the THRIFT_strncasecmp() function used in the original
implementation was true even if the name of the header being processed
was a prefix of the header name that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
Unfortunately, this can break authentication if the http request
has a header with a name that is a prefix to the word "Authorization".
Tested with a snapshot build in a CDP PVC DS environment where
both LDAP and Kerberos authentication was enabled:
- connected to Impala Coordinator using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
- connected to Impala Coordinator using beeline and hive jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to Impala Coordinator using beeline and impala jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to hiveserver2 using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/impala_shell.py
M shell/option_parser.py
7 files changed, 106 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/8
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 8
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 8:
(3 comments)
Some high level, comments, I haven't checked all the tests yet.
http://gerrit.cloudera.org:8080/#/c/20301/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20301/8//COMMIT_MSG@7
PS8, Line 7: impala-shell
The change also affects the server side, so I would change this to something like "Support "auth" header in hs2-http requests"
http://gerrit.cloudera.org:8080/#/c/20301/8//COMMIT_MSG@10
PS8, Line 10: This change introduces a new optional impala-shell option
: (send_auth_header_for_hiveserver2) to add an 'auth' header to http
: requests, which is expected in latest hiveserver2 builds
: when supporting multiple authentication modes at once
: (see HIVE-27352).
There is already an option called strict_hs2_protocol which is used when we connect to hiveserver2 instead of Impala so impala-shell tries to do things the Hive way. Do you think that we could use that option to decide whether to add "auth" header instead of adding a new option?
http://gerrit.cloudera.org:8080/#/c/20301/8/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20301/8/be/src/transport/THttpServer.cpp@139
PS8, Line 139: _
Thanks for fixing this bug!
I would prefer a solution though that doesn't need a malloc here. For example THRIFT_strcasecmp could also check that the "sz" and length of the right string are equal or check if "sz" char of the right string is \0.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 8
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Aug 2023 14:38:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 10:
(1 comment)
Looks good, one small nit.
http://gerrit.cloudera.org:8080/#/c/20301/10/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20301/10/be/src/transport/THttpServer.cpp@150
PS10, Line 150: && THRIFT_strncasecmp(header, TRANSFER_ENCODING, sz) == 0) {
Based on my reading of the docs and some experimentation with code, we should be able to remove the `sz` variable completely and use the length of the expected header:
if (THRIFT_strncasecmp(header, TRANSFER_ENCODING, strlen(TRANSFER_ENCODING))
or use the `strcasecmp` function instead and remove all the string length checks.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 10
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Aug 2023 19:05:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13696/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 6
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 15:11:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#5).
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
IMPALA-12328: New option for impala-shell to send "auth" header in thrift
http request
This change introduces a new optional impala-shell option
(send_auth_header_for_hiveserver2) to add an 'auth' header to http
requests, which is expected in latest hiveserver2 builds
when supporting multiple authentication modes at once
(see HIVE-27352).
The 'auth' header shouldn't be a problem for the impala coordinator,
but unfortunately it may prevent authentication due to a buggy header
parser code. This change will also fix that bug and introduces new
java frontend tests to validate the changes.
Some information on the http header parsing bug in THttpServer:
Unfortunately, the THRIFT_strncasecmp() function used in the original
implementation was true even if the name of the header being processed
was a prefix of the header name that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
Unfortunately, this can break authentication if the http request
has a header with a name that is a prefix to the word "Authorization".
Tested with a snapshot build in a CDP PVC DS environment where
both LDAP and Kerberos authentication was enabled:
- connected to Impala Coordinator using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
- connected to Impala Coordinator using beeline and hive jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to Impala Coordinator using beeline and impala jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to hiveserver2 using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/impala_shell.py
M shell/option_parser.py
7 files changed, 106 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/5
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 5
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/20301/3/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/20301/3/shell/impala_shell.py@644
PS3, Line 644: \
flake8: E251 unexpected spaces around keyword / parameter equals
http://gerrit.cloudera.org:8080/#/c/20301/3/shell/impala_shell.py@644
PS3, Line 644: \
flake8: E502 the backslash is redundant between brackets
http://gerrit.cloudera.org:8080/#/c/20301/3/shell/impala_shell.py@667
PS3, Line 667: \
flake8: E251 unexpected spaces around keyword / parameter equals
http://gerrit.cloudera.org:8080/#/c/20301/3/shell/impala_shell.py@667
PS3, Line 667: \
flake8: E502 the backslash is redundant between brackets
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 3
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 14:35:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Gergely Farkas has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20301/11//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20301/11//COMMIT_MSG@7
PS11, Line 7: IMPALA-12341
> Just realized that this issue should also appear in Apache Thrift's c++ lib
I created a bug ticket in the Thrift project: https://issues.apache.org/jira/browse/THRIFT-5730
http://gerrit.cloudera.org:8080/#/c/20301/11/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20301/11/be/src/transport/THttpServer.cpp@143
PS11, Line 143: const std::string THttpServer::X_FORWARDED_FOR = "X-Forwarded-For";
> +1 for moving these to constants
I created the string constants and a new MATCHES_HEADER macro. (I didn't want to modify the original THRIFT_strncasecmp.)
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 12
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Aug 2023 14:57:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 15: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 15
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Aug 2023 19:00:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/20301/4/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/20301/4/shell/impala_shell.py@628
PS4, Line 628: ,
flake8: E501 line too long (91 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/20301/4/shell/impala_shell.py@637
PS4, Line 637: ,
flake8: E501 line too long (91 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/20301/4/shell/impala_shell.py@642
PS4, Line 642: ,
flake8: E501 line too long (91 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/20301/4/shell/impala_shell.py@644
PS4, Line 644:
flake8: E251 unexpected spaces around keyword / parameter equals
http://gerrit.cloudera.org:8080/#/c/20301/4/shell/impala_shell.py@667
PS4, Line 667:
flake8: E251 unexpected spaces around keyword / parameter equals
http://gerrit.cloudera.org:8080/#/c/20301/4/shell/impala_shell.py@671
PS4, Line 671: s
flake8: E501 line too long (93 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/20301/4/shell/impala_shell.py@672
PS4, Line 672: p
flake8: E501 line too long (92 > 90 characters)
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 4
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 14:40:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#6).
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
IMPALA-12328: New option for impala-shell to send "auth" header in thrift
http request
This change introduces a new optional impala-shell option
(send_auth_header_for_hiveserver2) to add an 'auth' header to http
requests, which is expected in latest hiveserver2 builds
when supporting multiple authentication modes at once
(see HIVE-27352).
The 'auth' header shouldn't be a problem for the impala coordinator,
but unfortunately it may prevent authentication due to a buggy header
parser code. This change will also fix that bug and introduces new
java frontend tests to validate the changes.
Some information on the http header parsing bug in THttpServer:
Unfortunately, the THRIFT_strncasecmp() function used in the original
implementation was true even if the name of the header being processed
was a prefix of the header name that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
Unfortunately, this can break authentication if the http request
has a header with a name that is a prefix to the word "Authorization".
Tested with a snapshot build in a CDP PVC DS environment where
both LDAP and Kerberos authentication was enabled:
- connected to Impala Coordinator using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
- connected to Impala Coordinator using beeline and hive jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to Impala Coordinator using beeline and impala jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to hiveserver2 using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/impala_shell.py
M shell/option_parser.py
7 files changed, 106 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/6
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 6
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/13691/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 1
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 14:47:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#7).
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
IMPALA-12328: New option for impala-shell to send "auth" header in thrift
http request
This change introduces a new optional impala-shell option
(send_auth_header_for_hiveserver2) to add an 'auth' header to http
requests, which is expected in latest hiveserver2 builds
when supporting multiple authentication modes at once
(see HIVE-27352).
The 'auth' header shouldn't be a problem for the impala coordinator,
but unfortunately it may prevent authentication due to a buggy header
parser code. This change will also fix that bug and introduces new
java frontend tests to validate the changes.
Some information on the http header parsing bug in THttpServer:
Unfortunately, the THRIFT_strncasecmp() function used in the original
implementation was true even if the name of the header being processed
was a prefix of the header name that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
Unfortunately, this can break authentication if the http request
has a header with a name that is a prefix to the word "Authorization".
Tested with a snapshot build in a CDP PVC DS environment where
both LDAP and Kerberos authentication was enabled:
- connected to Impala Coordinator using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
- connected to Impala Coordinator using beeline and hive jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to Impala Coordinator using beeline and impala jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to hiveserver2 using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/impala_shell.py
M shell/option_parser.py
7 files changed, 106 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/7
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 7
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9556/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 6
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 15:12:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 12: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20301/12/be/src/transport/THttpServer.h
File be/src/transport/THttpServer.h:
http://gerrit.cloudera.org:8080/#/c/20301/12/be/src/transport/THttpServer.h@162
PS12, Line 162: static const std::string SAML2_TOKEN_RESPONSE_PORT;
Not sure here, but prefixing with HEADER_ as the constants above would be more consistent.
http://gerrit.cloudera.org:8080/#/c/20301/12/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20301/12/be/src/transport/THttpServer.cpp@132
PS12, Line 132: #define MATCHES_HEADER
We should simply use a function here IMO - THRIFT_strncasecmp is created with define because it is platform dependent.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 12
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Aug 2023 09:43:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Jason Fehr, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#15).
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
IMPALA-12341: Fix http header parsing issue in thrift http server
This change fixes the following http header parsing bug in
THttpServer: The THRIFT_strncasecmp() function used in the
THttpServer::parseHeader() function returns true even if the name
of the header being processed is a prefix of the header constant
that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
This can break authentication if the http request has a header
with a name that is a prefix to the word "Authorization".
If the length of the checked header is included in the condition,
this problem is avoided, so this fix refactors the if conditions,
so that this check is present everywhere.
Tested with new custom cluster tests.
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
3 files changed, 63 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/15
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 15
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 14:
(3 comments)
Thanks for cleaning this up! Only a tiny comment.
http://gerrit.cloudera.org:8080/#/c/20301/14/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20301/14/be/src/transport/THttpServer.cpp@182
PS14, Line 182: c_str
c_str() is not needed, as this is already a map<string,string>
http://gerrit.cloudera.org:8080/#/c/20301/14/be/src/transport/THttpServer.cpp@188
PS14, Line 188: c_str
see line 182
http://gerrit.cloudera.org:8080/#/c/20301/14/be/src/transport/THttpServer.cpp@339
PS14, Line 339: c_str
see line 182
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 14
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Aug 2023 09:31:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 15:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9599/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 15
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Aug 2023 10:26:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#2).
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
IMPALA-12328: New option for impala-shell to send "auth" header in thrift
http request
This change introduces a new optional impala-shell option
(send_auth_header_for_hiveserver2) to add an 'auth' header to http
requests, which is expected in latest hiveserver2 builds
when supporting multiple authentication modes at once
(see HIVE-27352).
The 'auth' header shouldn't be a problem for the impala coordinator,
but unfortunately it may prevent authentication due to a buggy header
parser code. This change will also fix that bug and introduces new
java frontend tests to validate the changes.
Some information on the http header parsing bug in THttpServer:
Unfortunately, the THRIFT_strncasecmp() function used in the original
implementation was true even if the name of the header being processed
was a prefix of the header name that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
Unfortunately, this can break authentication if the http request
has a header with a name that is a prefix to the word "Authorization".
Tested with a snapshot build in a CDP PVC DS environment where
both LDAP and Kerberos authentication was enabled:
- connected to Impala Coordinator using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
- connected to Impala Coordinator using beeline and hive jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to Impala Coordinator using beeline and impala jdbc driver
with LDAP authentication and with Kerberos authentication,
- connected to hiveserver2 using the impala-shell client with
and without the new send_auth_header_for_hiveserver2 option
with LDAP authentication and with Kerberos authentication,
in python2 and also in python3 environment
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M shell/ImpalaHttpClient.py
M shell/impala_client.py
M shell/impala_shell.py
M shell/option_parser.py
7 files changed, 91 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/2
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 2
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Jason Fehr, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#11).
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
IMPALA-12341: Fix http header parsing issue in thrift http server
This change fixes the following http header parsing bug in
THttpServer: The THRIFT_strncasecmp() function used in the
THttpServer::parseHeader() function returns true even if the name
of the header being processed is a prefix of the header constant
that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
This can break authentication if the http request has a header
with a name that is a prefix to the word "Authorization".
If the length of the checked header is included in the condition,
this problem is avoided, so this fix refactors the if conditions,
so that this check is present everywhere.
Tested with new custom cluster tests.
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
2 files changed, 51 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/11
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 11
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Gergely Farkas has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20301/8/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20301/8/be/src/transport/THttpServer.cpp@139
PS8, Line 139:
> Okay!
Done.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 10
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Aug 2023 16:14:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 7: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9558/
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 7
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 22:28:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/20301/1/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
File fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java:
http://gerrit.cloudera.org:8080/#/c/20301/1/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java@156
PS1, Line 156: // 7. Valid username, password and send_auth_header_for_hiveserver2 option. Should succeed.
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/20301/1/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java@157
PS1, Line 157: String[] validCommandWithSendAuthHeaderForHiveserver2 = {"impala-shell.sh", "", "--ldap",
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/20301/1/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/20301/1/shell/impala_shell.py@644
PS1, Line 644:
flake8: E251 unexpected spaces around keyword / parameter equals
http://gerrit.cloudera.org:8080/#/c/20301/1/shell/impala_shell.py@667
PS1, Line 667:
flake8: E251 unexpected spaces around keyword / parameter equals
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 1
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Aug 2023 14:25:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12328: New option for impala-shell to send "auth" header in thrift http request
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13708/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 10
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Aug 2023 16:35:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9580/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 12
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Aug 2023 15:16:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 15: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9599/
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 15
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Aug 2023 14:41:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Gergely Farkas (Code Review)" <ge...@cloudera.org>.
Hello Jason Fehr, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20301
to look at the new patch set (#13).
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
IMPALA-12341: Fix http header parsing issue in thrift http server
This change fixes the following http header parsing bug in
THttpServer: The THRIFT_strncasecmp() function used in the
THttpServer::parseHeader() function returns true even if the name
of the header being processed is a prefix of the header constant
that is defined in the condition.
For example: In the original implementation when processing the
http header "auth: anyValue", we run into the code fragment where
the Authorization header content is processed, because the condition
THRIFT_strncasecmp("auth: anyValue", "Authorization", 4) == 0)
is true, since the first 4 characters of the two strings are the same.
This can break authentication if the http request has a header
with a name that is a prefix to the word "Authorization".
If the length of the checked header is included in the condition,
this problem is avoided, so this fix refactors the if conditions,
so that this check is present everywhere.
Tested with new custom cluster tests.
Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
---
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
3 files changed, 63 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/20301/13
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 13
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
[Impala-ASF-CR] IMPALA-12341: Fix http header parsing issue in thrift http server
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20301 )
Change subject: IMPALA-12341: Fix http header parsing issue in thrift http server
......................................................................
Patch Set 14:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13748/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I754639cfe3eab0016d09f71ded4821caa357bf87
Gerrit-Change-Number: 20301
Gerrit-PatchSet: 14
Gerrit-Owner: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Farkas <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Aug 2023 09:38:42 +0000
Gerrit-HasComments: No