You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Smith (Code Review)" <ge...@cloudera.org> on 2023/01/25 19:22:53 UTC
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Michael Smith has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19199
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
IMPALA-11856: Use POST requests to set log level
Set and reset loglevel handlers now require a POST. Implements
Cross-Site Request Forgery (CSRF) prevention in Impala's webserver using
the Double Submit Cookie pattern - where POST requests must include a
csrf_token field in their post with the random value from the cookie -
or a custom header.
CSRF attacks rely on the browser always sending a cookie or
'Authorization: Basic' header.
- With cookies, attacks don't have access to default form values or the
original cookie, so we can include the cookie's random value in the
form as a cross-check. As the cookie is cryptographically signed, they
also can't be replaced with one that would match an attack's forms.
- When not using cookies, a custom header (X-Requested-By) is required
as CSRFs are unable to add custom headers. This approach is also used
by Jersey; see
http://blog.alutam.com/2011/09/14/jersey-and-cross-site-request-forgery-csrf
In a broader implementation this would require a separate cookie so it
can be used to protect logins as well, but login is handled external to
Impala so we re-use the cookie the page already has.
Cookies are now generated for the HTPASSWD authentication method.
Authenticating via JWT still omits cookies because the JWT is already
provided via custom header (preventing CSRF) and disabling
authentication (NONE) means anyone could directly send a request so CSRF
protection is meaningless.
We also start an additional Webserver instance with authentication NONE
when metrics_webserver_port > 0, and the Webserver metric
"impala.webserver.total-cookie-auth-success" can only be registered
once. Additional changes would be necessary to make metric names unique
in Webserver (based on port); for the moment we avoid that by ensuring
all metrics counters are only instantiated for Webservers that use
authentication.
Cookie generation and authentication were updated to provide access to
the random value.
Adds flag to enable SameSite=Strict for defense in depth as mentioned in
https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis.
This can be enabled if another CSRF attack method is found.
Verified that this prevents CSRF attacks by disabling SameSite=Strict
and visiting (via https://security.love/CSRF-PoC-Genorator):
```
<html>
<form enctype="application/x-www-form-urlencoded" method="POST"
action="http://localhost:45000/set_glog_level">
<table>
<tr>
<td>glog</td>
<td><input type="text" value="1" name="glog"></td>
</tr>
</table>
<input type="submit" value="http://localhost:45000/set_glog_level">
</form>
</html>
```
Adds tests for the webserver with basic authentication, LDAP, and SPNEGO
that authorization fails on POST unless
- using a cookie and csrf_token is correctly set in the POST body
- the X-Requested-By header is set
Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
---
M be/src/rpc/authentication-util.cc
M be/src/rpc/authentication-util.h
M be/src/util/logging-support.cc
M be/src/util/webserver-test.cc
M be/src/util/webserver.cc
M be/src/util/webserver.h
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpylaHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/testutil/WebClient.java
D fe/src/test/java/org/apache/impala/util/Metrics.java
M www/form-hidden-inputs.tmpl
M www/log_level.tmpl
18 files changed, 844 insertions(+), 337 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/19199/18
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 18
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 18:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/12234/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 18
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 19:42:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 21:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8993/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 21
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jan 2023 21:54:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 19: Code-Review+1
(3 comments)
Went through the tests too, looks great!
http://gerrit.cloudera.org:8080/#/c/19199/19//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19199/19//COMMIT_MSG@11
PS19, Line 11: where POST requests must include a
: csrf_token field in their post with the random value from the cookie -
: or a custom header.
This means that POST requests will always need csrf_token or X-requested-by, even if it is not POST with side-effects (e.g. changing logs, just reading a page), right? Is it possible that some tools query the Impala webui with POST while they could use GET?
http://gerrit.cloudera.org:8080/#/c/19199/19/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
File fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java:
http://gerrit.cloudera.org:8080/#/c/19199/19/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@414
PS19, Line 414: does nothing
This also returns an error like the previous block, right?
http://gerrit.cloudera.org:8080/#/c/19199/19/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@425
PS19, Line 425: new WebClient(TEST_USER_1, TEST_PASSWORD_1);
just an idea to reduce noise: WebClient could have a reset function or a resetAfterRequest property that leads to dropping cookies + opening new connection
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jan 2023 08:16:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 24:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9006/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 24
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jan 2023 09:31:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 19:
(1 comment)
Looks good to me in general, but I couldn't process all the tests yet.
http://gerrit.cloudera.org:8080/#/c/19199/15/be/src/util/webserver.cc
File be/src/util/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/19199/15/be/src/util/webserver.cc@703
PS15, Line 703: LOG(INFO) << "Inv
> I think I get it now - what confused me was the handling of AuthMode::HTPAS
Oops. I didn't see the latest patches when writing this. The HTPASSWD handling looks much clearer now.
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 16:53:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 21: Code-Review+2
Carrying +2 after addressing test and comment nits.
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 21
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jan 2023 21:54:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 23:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12266/ : 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/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 23
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jan 2023 18:51:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 23:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8999/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 23
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jan 2023 18:32:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Attila Bukor, Wenzhe Zhou, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19199
to look at the new patch set (#19).
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
IMPALA-11856: Use POST requests to set log level
Set and reset loglevel handlers now require a POST. Implements
Cross-Site Request Forgery (CSRF) prevention in Impala's webserver using
the Double Submit Cookie pattern - where POST requests must include a
csrf_token field in their post with the random value from the cookie -
or a custom header.
CSRF attacks rely on the browser always sending a cookie or
'Authorization: Basic' header.
- With cookies, attacks don't have access to default form values or the
original cookie, so we can include the cookie's random value in the
form as a cross-check. As the cookie is cryptographically signed, they
also can't be replaced with one that would match an attack's forms.
- When not using cookies, a custom header (X-Requested-By) is required
as CSRFs are unable to add custom headers. This approach is also used
by Jersey; see
http://blog.alutam.com/2011/09/14/jersey-and-cross-site-request-forgery-csrf
In a broader implementation this would require a separate cookie so it
can be used to protect logins as well, but login is handled external to
Impala so we re-use the cookie the page already has.
Cookies are now generated for the HTPASSWD authentication method.
Authenticating via JWT still omits cookies because the JWT is already
provided via custom header (preventing CSRF) and disabling
authentication (NONE) means anyone could directly send a request so CSRF
protection is meaningless.
We also start an additional Webserver instance with authentication NONE
when metrics_webserver_port > 0, and the Webserver metric
"impala.webserver.total-cookie-auth-success" can only be registered
once. Additional changes would be necessary to make metric names unique
in Webserver (based on port); for the moment we avoid that by ensuring
all metrics counters are only instantiated for Webservers that use
authentication.
Cookie generation and authentication were updated to provide access to
the random value.
Adds flag to enable SameSite=Strict for defense in depth as mentioned in
https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis.
This can be enabled if another CSRF attack method is found.
Verified that this prevents CSRF attacks by disabling SameSite=Strict
and visiting (via https://security.love/CSRF-PoC-Genorator):
```
<html>
<form enctype="application/x-www-form-urlencoded" method="POST"
action="http://localhost:45000/set_glog_level">
<table>
<tr>
<td>glog</td>
<td><input type="text" value="1" name="glog"></td>
</tr>
</table>
<input type="submit" value="http://localhost:45000/set_glog_level">
</form>
</html>
```
Adds tests for the webserver with basic authentication, LDAP, and SPNEGO
that authorization fails on POST unless
- using a cookie and csrf_token is correctly set in the POST body
- the X-Requested-By header is set
Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
---
M be/src/rpc/authentication-util.cc
M be/src/rpc/authentication-util.h
M be/src/util/logging-support.cc
M be/src/util/webserver-test.cc
M be/src/util/webserver.cc
M be/src/util/webserver.h
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpylaHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/testutil/WebClient.java
D fe/src/test/java/org/apache/impala/util/Metrics.java
M www/form-hidden-inputs.tmpl
M www/log_level.tmpl
18 files changed, 844 insertions(+), 337 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/19199/19
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 19:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12235/ : 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/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 20:06:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 24: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 24
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jan 2023 14:40:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 23: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 23
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jan 2023 23:44:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Attila Bukor, Wenzhe Zhou, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19199
to look at the new patch set (#23).
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
IMPALA-11856: Use POST requests to set log level
Set and reset loglevel handlers now require a POST. Implements
Cross-Site Request Forgery (CSRF) prevention in Impala's webserver using
the Double Submit Cookie pattern - where POST requests must include a
csrf_token field in their post with the random value from the cookie -
or a custom header.
CSRF attacks rely on the browser always sending a cookie or
'Authorization: Basic' header.
- With cookies, attacks don't have access to default form values or the
original cookie, so we can include the cookie's random value in the
form as a cross-check. As the cookie is cryptographically signed, they
also can't be replaced with one that would match an attack's forms.
- When not using cookies, a custom header (X-Requested-By) is required
as CSRFs are unable to add custom headers. This approach is also used
by Jersey; see
http://blog.alutam.com/2011/09/14/jersey-and-cross-site-request-forgery-csrf
In a broader implementation this would require a separate cookie so it
can be used to protect logins as well, but login is handled external to
Impala so we re-use the cookie the page already has.
Cookies are now generated for the HTPASSWD authentication method.
Authenticating via JWT still omits cookies because the JWT is already
provided via custom header (preventing CSRF) and disabling
authentication (NONE) means anyone could directly send a request so CSRF
protection is meaningless.
We also start an additional Webserver instance with authentication NONE
when metrics_webserver_port > 0, and the Webserver metric
"impala.webserver.total-cookie-auth-success" can only be registered
once. Additional changes would be necessary to make metric names unique
in Webserver (based on port); for the moment we avoid that by ensuring
all metrics counters are only instantiated for Webservers that use
authentication.
Cookie generation and authentication were updated to provide access to
the random value.
Adds flag to enable SameSite=Strict for defense in depth as mentioned in
https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis.
This can be enabled if another CSRF attack method is found.
Verified that this prevents CSRF attacks by disabling SameSite=Strict
and visiting (via https://security.love/CSRF-PoC-Genorator):
```
<html>
<form enctype="application/x-www-form-urlencoded" method="POST"
action="http://localhost:45000/set_glog_level">
<table>
<tr>
<td>glog</td>
<td><input type="text" value="1" name="glog"></td>
</tr>
</table>
<input type="submit" value="http://localhost:45000/set_glog_level">
</form>
</html>
```
Adds tests for the webserver with basic authentication, LDAP, and SPNEGO
that authorization fails on POST unless
- using a cookie and csrf_token is correctly set in the POST body
- the X-Requested-By header is set
Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
---
M be/src/rpc/authentication-util.cc
M be/src/rpc/authentication-util.h
M be/src/util/logging-support.cc
M be/src/util/webserver-test.cc
M be/src/util/webserver.cc
M be/src/util/webserver.h
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpylaHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/testutil/WebClient.java
D fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/webserver/test_web_pages.py
M www/form-hidden-inputs.tmpl
M www/log_level.tmpl
19 files changed, 900 insertions(+), 369 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/19199/23
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 23
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 20: Code-Review+2
(3 comments)
LGTM, thanks for this complex change
http://gerrit.cloudera.org:8080/#/c/19199/20/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
File fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java:
http://gerrit.cloudera.org:8080/#/c/19199/20/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@475
PS20, Line 475: * Test that we can set glog level.
Nit: java log level
http://gerrit.cloudera.org:8080/#/c/19199/20/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@563
PS20, Line 563: assertTrue("Expected cookie to contain random number", false);
Nit: or just use fail()
http://gerrit.cloudera.org:8080/#/c/19199/20/fe/src/test/java/org/apache/impala/testutil/WebClient.java
File fe/src/test/java/org/apache/impala/testutil/WebClient.java:
http://gerrit.cloudera.org:8080/#/c/19199/20/fe/src/test/java/org/apache/impala/testutil/WebClient.java@107
PS20, Line 107: * @param data Parameters to include in the POST
Nit: I know you are just copying code but this parameter is wrong
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 20
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jan 2023 20:29:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 19:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19199/19//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19199/19//COMMIT_MSG@11
PS19, Line 11: where POST requests must include a
: csrf_token field in their post with the random value from the cookie -
: or a custom header.
> This means that POST requests will always need csrf_token or X-requested-by
Someone POSTing just to read a page would have been possible, but seems unlikely. curl requires a body when POSTing, and browsers require a form, so they would have had to jump through hoops to do it.
http://gerrit.cloudera.org:8080/#/c/19199/19/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
File fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java:
http://gerrit.cloudera.org:8080/#/c/19199/19/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@414
PS19, Line 414: does nothing
> This also returns an error like the previous block, right?
Yeah, I'll update the comment to match.
http://gerrit.cloudera.org:8080/#/c/19199/19/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@425
PS19, Line 425: new WebClient(TEST_USER_1, TEST_PASSWORD_1);
> just an idea to reduce noise: WebClient could have a reset function or a re
Doing that correctly turned out to be tricky. I had to recreate the internal client anyway because it doesn't have public methods that allow sufficient cleanup. I think this is clear enough.
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jan 2023 16:54:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 20:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19199/20/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
File fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java:
http://gerrit.cloudera.org:8080/#/c/19199/20/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@475
PS20, Line 475: * Test that we can set glog level.
> Nit: java log level
Ack
http://gerrit.cloudera.org:8080/#/c/19199/20/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@563
PS20, Line 563: assertTrue("Expected cookie to contain random number", false);
> Nit: or just use fail()
Ack
http://gerrit.cloudera.org:8080/#/c/19199/20/fe/src/test/java/org/apache/impala/testutil/WebClient.java
File fe/src/test/java/org/apache/impala/testutil/WebClient.java:
http://gerrit.cloudera.org:8080/#/c/19199/20/fe/src/test/java/org/apache/impala/testutil/WebClient.java@107
PS20, Line 107: * @param data Parameters to include in the POST
> Nit: I know you are just copying code but this parameter is wrong
Just an earlier version of my function.
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 20
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jan 2023 21:32:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 21:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12249/ : 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/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 21
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jan 2023 21:52:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 23: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 23
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jan 2023 09:29:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 20:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12247/ : 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/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 20
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jan 2023 17:16:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 21: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8993/
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 21
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sat, 28 Jan 2023 03:02:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 19:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19199/15/be/src/util/webserver.cc
File be/src/util/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/19199/15/be/src/util/webserver.cc@703
PS15, Line 703: LOG(INFO) << "Inv
> This is a relocation of lines 665-667. It didn't have a comment before; I'l
I think I get it now - what confused me was the handling of AuthMode::HTPASSWD. In that case the authentication is handled by Squeasel based on a password file, so I guess we won't even get here if the password was wrong. If it was correct, then it is true that the authentication is finished. and the cookie should be added / checked.
I still think that the code is quite confusing, but I don't mind if it is kept as it is.
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 13:43:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Attila Bukor, Wenzhe Zhou, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19199
to look at the new patch set (#20).
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
IMPALA-11856: Use POST requests to set log level
Set and reset loglevel handlers now require a POST. Implements
Cross-Site Request Forgery (CSRF) prevention in Impala's webserver using
the Double Submit Cookie pattern - where POST requests must include a
csrf_token field in their post with the random value from the cookie -
or a custom header.
CSRF attacks rely on the browser always sending a cookie or
'Authorization: Basic' header.
- With cookies, attacks don't have access to default form values or the
original cookie, so we can include the cookie's random value in the
form as a cross-check. As the cookie is cryptographically signed, they
also can't be replaced with one that would match an attack's forms.
- When not using cookies, a custom header (X-Requested-By) is required
as CSRFs are unable to add custom headers. This approach is also used
by Jersey; see
http://blog.alutam.com/2011/09/14/jersey-and-cross-site-request-forgery-csrf
In a broader implementation this would require a separate cookie so it
can be used to protect logins as well, but login is handled external to
Impala so we re-use the cookie the page already has.
Cookies are now generated for the HTPASSWD authentication method.
Authenticating via JWT still omits cookies because the JWT is already
provided via custom header (preventing CSRF) and disabling
authentication (NONE) means anyone could directly send a request so CSRF
protection is meaningless.
We also start an additional Webserver instance with authentication NONE
when metrics_webserver_port > 0, and the Webserver metric
"impala.webserver.total-cookie-auth-success" can only be registered
once. Additional changes would be necessary to make metric names unique
in Webserver (based on port); for the moment we avoid that by ensuring
all metrics counters are only instantiated for Webservers that use
authentication.
Cookie generation and authentication were updated to provide access to
the random value.
Adds flag to enable SameSite=Strict for defense in depth as mentioned in
https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis.
This can be enabled if another CSRF attack method is found.
Verified that this prevents CSRF attacks by disabling SameSite=Strict
and visiting (via https://security.love/CSRF-PoC-Genorator):
```
<html>
<form enctype="application/x-www-form-urlencoded" method="POST"
action="http://localhost:45000/set_glog_level">
<table>
<tr>
<td>glog</td>
<td><input type="text" value="1" name="glog"></td>
</tr>
</table>
<input type="submit" value="http://localhost:45000/set_glog_level">
</form>
</html>
```
Adds tests for the webserver with basic authentication, LDAP, and SPNEGO
that authorization fails on POST unless
- using a cookie and csrf_token is correctly set in the POST body
- the X-Requested-By header is set
Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
---
M be/src/rpc/authentication-util.cc
M be/src/rpc/authentication-util.h
M be/src/util/logging-support.cc
M be/src/util/webserver-test.cc
M be/src/util/webserver.cc
M be/src/util/webserver.h
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpylaHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/testutil/WebClient.java
D fe/src/test/java/org/apache/impala/util/Metrics.java
M www/form-hidden-inputs.tmpl
M www/log_level.tmpl
18 files changed, 844 insertions(+), 337 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/19199/20
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 20
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Andrew Sherman, Attila Bukor, Wenzhe Zhou, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19199
to look at the new patch set (#21).
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
IMPALA-11856: Use POST requests to set log level
Set and reset loglevel handlers now require a POST. Implements
Cross-Site Request Forgery (CSRF) prevention in Impala's webserver using
the Double Submit Cookie pattern - where POST requests must include a
csrf_token field in their post with the random value from the cookie -
or a custom header.
CSRF attacks rely on the browser always sending a cookie or
'Authorization: Basic' header.
- With cookies, attacks don't have access to default form values or the
original cookie, so we can include the cookie's random value in the
form as a cross-check. As the cookie is cryptographically signed, they
also can't be replaced with one that would match an attack's forms.
- When not using cookies, a custom header (X-Requested-By) is required
as CSRFs are unable to add custom headers. This approach is also used
by Jersey; see
http://blog.alutam.com/2011/09/14/jersey-and-cross-site-request-forgery-csrf
In a broader implementation this would require a separate cookie so it
can be used to protect logins as well, but login is handled external to
Impala so we re-use the cookie the page already has.
Cookies are now generated for the HTPASSWD authentication method.
Authenticating via JWT still omits cookies because the JWT is already
provided via custom header (preventing CSRF) and disabling
authentication (NONE) means anyone could directly send a request so CSRF
protection is meaningless.
We also start an additional Webserver instance with authentication NONE
when metrics_webserver_port > 0, and the Webserver metric
"impala.webserver.total-cookie-auth-success" can only be registered
once. Additional changes would be necessary to make metric names unique
in Webserver (based on port); for the moment we avoid that by ensuring
all metrics counters are only instantiated for Webservers that use
authentication.
Cookie generation and authentication were updated to provide access to
the random value.
Adds flag to enable SameSite=Strict for defense in depth as mentioned in
https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis.
This can be enabled if another CSRF attack method is found.
Verified that this prevents CSRF attacks by disabling SameSite=Strict
and visiting (via https://security.love/CSRF-PoC-Genorator):
```
<html>
<form enctype="application/x-www-form-urlencoded" method="POST"
action="http://localhost:45000/set_glog_level">
<table>
<tr>
<td>glog</td>
<td><input type="text" value="1" name="glog"></td>
</tr>
</table>
<input type="submit" value="http://localhost:45000/set_glog_level">
</form>
</html>
```
Adds tests for the webserver with basic authentication, LDAP, and SPNEGO
that authorization fails on POST unless
- using a cookie and csrf_token is correctly set in the POST body
- the X-Requested-By header is set
Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
---
M be/src/rpc/authentication-util.cc
M be/src/rpc/authentication-util.h
M be/src/util/logging-support.cc
M be/src/util/webserver-test.cc
M be/src/util/webserver.cc
M be/src/util/webserver.h
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpylaHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/testutil/WebClient.java
D fe/src/test/java/org/apache/impala/util/Metrics.java
M www/form-hidden-inputs.tmpl
M www/log_level.tmpl
18 files changed, 848 insertions(+), 337 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/19199/21
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 21
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
IMPALA-11856: Use POST requests to set log level
Set and reset loglevel handlers now require a POST. Implements
Cross-Site Request Forgery (CSRF) prevention in Impala's webserver using
the Double Submit Cookie pattern - where POST requests must include a
csrf_token field in their post with the random value from the cookie -
or a custom header.
CSRF attacks rely on the browser always sending a cookie or
'Authorization: Basic' header.
- With cookies, attacks don't have access to default form values or the
original cookie, so we can include the cookie's random value in the
form as a cross-check. As the cookie is cryptographically signed, they
also can't be replaced with one that would match an attack's forms.
- When not using cookies, a custom header (X-Requested-By) is required
as CSRFs are unable to add custom headers. This approach is also used
by Jersey; see
http://blog.alutam.com/2011/09/14/jersey-and-cross-site-request-forgery-csrf
In a broader implementation this would require a separate cookie so it
can be used to protect logins as well, but login is handled external to
Impala so we re-use the cookie the page already has.
Cookies are now generated for the HTPASSWD authentication method.
Authenticating via JWT still omits cookies because the JWT is already
provided via custom header (preventing CSRF) and disabling
authentication (NONE) means anyone could directly send a request so CSRF
protection is meaningless.
We also start an additional Webserver instance with authentication NONE
when metrics_webserver_port > 0, and the Webserver metric
"impala.webserver.total-cookie-auth-success" can only be registered
once. Additional changes would be necessary to make metric names unique
in Webserver (based on port); for the moment we avoid that by ensuring
all metrics counters are only instantiated for Webservers that use
authentication.
Cookie generation and authentication were updated to provide access to
the random value.
Adds flag to enable SameSite=Strict for defense in depth as mentioned in
https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis.
This can be enabled if another CSRF attack method is found.
Verified that this prevents CSRF attacks by disabling SameSite=Strict
and visiting (via https://security.love/CSRF-PoC-Genorator):
```
<html>
<form enctype="application/x-www-form-urlencoded" method="POST"
action="http://localhost:45000/set_glog_level">
<table>
<tr>
<td>glog</td>
<td><input type="text" value="1" name="glog"></td>
</tr>
</table>
<input type="submit" value="http://localhost:45000/set_glog_level">
</form>
</html>
```
Adds tests for the webserver with basic authentication, LDAP, and SPNEGO
that authorization fails on POST unless
- using a cookie and csrf_token is correctly set in the POST body
- the X-Requested-By header is set
Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Reviewed-on: http://gerrit.cloudera.org:8080/19199
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/rpc/authentication-util.cc
M be/src/rpc/authentication-util.h
M be/src/util/logging-support.cc
M be/src/util/webserver-test.cc
M be/src/util/webserver.cc
M be/src/util/webserver.h
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapImpylaHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/testutil/WebClient.java
D fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/webserver/test_web_pages.py
M www/form-hidden-inputs.tmpl
M www/log_level.tmpl
19 files changed, 900 insertions(+), 369 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 25
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19199 )
Change subject: IMPALA-11856: Use POST requests to set log level
......................................................................
Patch Set 24: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19199
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4be8694492b8ba16737f644ac8c56d8124f19693
Gerrit-Change-Number: 19199
Gerrit-PatchSet: 24
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jan 2023 09:31:21 +0000
Gerrit-HasComments: No