[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-31 Thread Impala Public Jenkins (Code Review)
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):
```

  http://localhost:45000/set_glog_level;>

  
glog

  

http://localhost:45000/set_glog_level;>
  

```

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 
Tested-by: Impala Public Jenkins 
---
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-31 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Tue, 31 Jan 2023 14:40:49 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-31 Thread Csaba Ringhofer (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Tue, 31 Jan 2023 09:29:09 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-31 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Tue, 31 Jan 2023 09:31:22 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-31 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Tue, 31 Jan 2023 09:31:21 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-30 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Mon, 30 Jan 2023 23:44:57 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-30 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Mon, 30 Jan 2023 18:51:27 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-30 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Mon, 30 Jan 2023 18:32:12 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-30 Thread Michael Smith (Code Review)
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):
```

  http://localhost:45000/set_glog_level;>

  
glog

  

http://localhost:45000/set_glog_level;>
  

```

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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Sat, 28 Jan 2023 03:02:35 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Fri, 27 Jan 2023 21:54:35 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Michael Smith (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Fri, 27 Jan 2023 21:54:19 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Fri, 27 Jan 2023 21:52:52 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Michael Smith (Code Review)
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):
```

  http://localhost:45000/set_glog_level;>

  
glog

  

http://localhost:45000/set_glog_level;>
  

```

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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Michael Smith (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Fri, 27 Jan 2023 21:32:36 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Andrew Sherman (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Fri, 27 Jan 2023 20:29:25 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Fri, 27 Jan 2023 17:16:56 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Michael Smith (Code Review)
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):
```

  http://localhost:45000/set_glog_level;>

  
glog

  

http://localhost:45000/set_glog_level;>
  

```

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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Michael Smith (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Fri, 27 Jan 2023 16:54:42 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-27 Thread Csaba Ringhofer (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Fri, 27 Jan 2023 08:16:28 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-26 Thread Csaba Ringhofer (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Thu, 26 Jan 2023 16:53:50 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-26 Thread Csaba Ringhofer (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Thu, 26 Jan 2023 13:43:33 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-25 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Wed, 25 Jan 2023 20:06:35 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-25 Thread Michael Smith (Code Review)
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):
```

  http://localhost:45000/set_glog_level;>

  
glog

  

http://localhost:45000/set_glog_level;>
  

```

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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-25 Thread Impala Public Jenkins (Code Review)
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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Comment-Date: Wed, 25 Jan 2023 19:42:47 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-11856: Use POST requests to set log level

2023-01-25 Thread Michael Smith (Code Review)
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):
```

  http://localhost:45000/set_glog_level;>

  
glog

  

http://localhost:45000/set_glog_level;>
  

```

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 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Wenzhe Zhou