You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2022/03/02 17:46:07 UTC
[kudu-CR] [www] Add CSP header to web UI
Hello Alexey Serbin, Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/18285
to review the following change.
Change subject: [www] Add CSP header to web UI
......................................................................
[www] Add CSP header to web UI
CSP (Content Security Policy) headers provide a way to tell the browser
where assets can be loaded from to prevent XSS attacks. Kudu's web UI is
read-only, at least for now, so it's not susceptible for XSS attacks,
but some security scanners still flag it as vulnerable due to not having
this header.
This patch adds a CSP header that allows loading assets on the same
host, and some inline styles and images in jQuery. It also removes all
inline style definitions from first-party files and moves them to
kudu.css.
There's no good way to write a unit test for this, as it requires a
GUI browser (curl doesn't load external resources and doesn't use
JavaScript), but I tested it manually both through HTTP and HTTPS and
confirmed there are no related errors in the JS console.
Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
---
M src/kudu/server/webserver.cc
M www/kudu.css
M www/startup.mustache
3 files changed, 21 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/18285/1
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@94
PS3, Line 94: webserver will remove
nit: use present tense here for clarity
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 14 Jul 2022 18:44:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 3:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver-test.cc@463
PS3, Line 463: //Basic sanity check: the page should have the expected title.
nit: add a space after the comment
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver-test.cc@464
PS3, Line 464: ASSERT_STR_CONTAINS(buf_.ToString(),"Kudu");
nit: missed space after the comma
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver-test.cc@473
PS3, Line 473: ASSERT_STR_NOT_CONTAINS(buf_.ToString(),kCspHeader);
nit: missed space after the comma
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@93
PS3, Line 93: webserver_disable_csp
We do have a bunch of boolean flags like that, and the majority of those named 'xxx_enabled' or 'xxx_enable_xxx'. For uniformity, rename this into 'webserver_enable_csp_header' and set the default value to 'true'.
That would help to have clearer description for the flag as well :)
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@94
PS3, Line 94: Currently"
: "the webserver is already impervious to XSS attacks due to being read-only.
This is not very relevant for the description of the flag. You could add that into the commit description.
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@646
PS3, Line 646: char *
style nit: stick the ampersand to the type
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@649
PS3, Line 649: string &
style nit: stick the ampersand to the type
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@689
PS3, Line 689: FLAGS_webserver_disable_csp
Since we are aiming to have the CSP header added by default, wrap this into PREDICT_TRUE or PREDICT_FALSE, as necessary.
http://gerrit.cloudera.org:8080/#/c/18285/3/src/kudu/server/webserver.cc@692
PS3, Line 692: The easiest way to obtain this hash is through browser/js
: // console. It is embedded in the error message.
OK, I guess there is a way to do so using command-line tools, but we can update the comment later on as needed. Essentially, there should be a command that people could just copy-paste, get the new hash and update it here in the code.
All right, let's aim for the follow-up patch to add those instructions.
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 14 Jul 2022 18:30:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18285/1//COMMIT_MSG
Commit Message:
PS1:
Does it make sense to add a test into webserver-test.cc to check for the presence of the CSP header in the webserver's response?
http://gerrit.cloudera.org:8080/#/c/18285/1/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/18285/1/src/kudu/server/webserver.cc@684
PS1, Line 684: Content-Security-Policy
Does it make sense to add a kill-switch flag to disable adding the CSP header? I guess that by default the header should be present, but in case of unexpected compatibility it might be a good idea to have a control knob to disable the header.
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 08 Jun 2022 19:14:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [www] Add CSP header to web UI
Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.
Khazar Mammadli has uploaded a new patch set (#4) to the change originally created by Attila Bukor. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
[www] Add CSP header to web UI
CSP (Content Security Policy) headers provide a way to tell the browser
where assets can be loaded from to prevent XSS attacks. Kudu's web UI is
read-only, at least for now, so it's not susceptible for XSS attacks,
but some security scanners still flag it as vulnerable due to not having
this header.
This patch adds a CSP header that allows loading assets on the same
host, and some inline styles and images in jQuery. It also removes all
inline style definitions from first-party files and moves them to
kudu.css.
There's no good way to write a unit test for this, as it requires a
GUI browser (curl doesn't load external resources and doesn't use
JavaScript), but I tested it manually both through HTTP and HTTPS and
confirmed there are no related errors in the JS console.
Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
---
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M www/kudu.css
M www/startup.mustache
4 files changed, 51 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/18285/4
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
[www] Add CSP header to web UI
CSP (Content Security Policy) headers provide a way to tell the browser
where assets can be loaded from to prevent XSS attacks. Kudu's web UI is
read-only, at least for now, so it's not susceptible for XSS attacks,
but some security scanners still flag it as vulnerable due to not having
this header.
This patch adds a CSP header that allows loading assets on the same
host, and some inline styles and images in jQuery. It also removes all
inline style definitions from first-party files and moves them to
kudu.css.
There's no good way to write a unit test for this, as it requires a
GUI browser (curl doesn't load external resources and doesn't use
JavaScript), but I tested it manually both through HTTP and HTTPS and
confirmed there are no related errors in the JS console.
Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Reviewed-on: http://gerrit.cloudera.org:8080/18285
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M www/kudu.css
M www/startup.mustache
4 files changed, 50 insertions(+), 5 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 4: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18285/4/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/18285/4/src/kudu/server/webserver.cc@648
PS4, Line 648: for (string& encoding: encodings) {
style nit: put the space back
Overall, please refrain from updating non-relevant parts of the code -- that didn't help in reviewing and tracking changes in posterity. If you want to update some aspects of the code which are not directly related to the essence of one patch, you can always put together a separate patch and post it for review.
http://gerrit.cloudera.org:8080/#/c/18285/4/src/kudu/server/webserver.cc@689
PS4, Line 689: oss << "Content-Security-Policy: default-src 'self';"
: // This hash has to be updated whenever kudu.css, bootstrap.min.css ,
: // bootstrap-table.min.css files change. The easiest way to obtain this hash is through browser/js
: // console. It is embedded in the error message.
: << "style-src 'self' 'unsafe-hashes' 'sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=';"
: << "img-src 'self' data:;\r\n";
nit: the indent is incorrect for this block
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 14 Jul 2022 19:27:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18285/4/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/18285/4/src/kudu/server/webserver.cc@96
PS4, Line 96:
This new flag is also run-time by the way how it's implemented, so it makes sense to add the 'runtime' tag as well.
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 14 Jul 2022 19:29:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 14 Jul 2022 22:18:28 +0000
Gerrit-HasComments: No
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18285/1/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/18285/1/src/kudu/server/webserver.cc@685
PS1, Line 685: sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=
What function/element is this for?
Will we need to update this hash when updating some of the JS/CSS files under the 'www' directory? If yes, what exact files and how to recalculate the hash after the update?
It would be great to add some information on that, at least here in the code.
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 08 Jun 2022 19:04:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [www] Add CSP header to web UI
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.
Change subject: [www] Add CSP header to web UI
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [www] Add CSP header to web UI
Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.
Khazar Mammadli has uploaded a new patch set (#2) to the change originally created by Attila Bukor. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
[www] Add CSP header to web UI
CSP (Content Security Policy) headers provide a way to tell the browser
where assets can be loaded from to prevent XSS attacks. Kudu's web UI is
read-only, at least for now, so it's not susceptible for XSS attacks,
but some security scanners still flag it as vulnerable due to not having
this header.
This patch adds a CSP header that allows loading assets on the same
host, and some inline styles and images in jQuery. It also removes all
inline style definitions from first-party files and moves them to
kudu.css.
There's no good way to write a unit test for this, as it requires a
GUI browser (curl doesn't load external resources and doesn't use
JavaScript), but I tested it manually both through HTTP and HTTPS and
confirmed there are no related errors in the JS console.
Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
---
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M www/kudu.css
M www/startup.mustache
4 files changed, 53 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/18285/2
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [www] Add CSP header to web UI
Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.
Khazar Mammadli has uploaded a new patch set (#3) to the change originally created by Attila Bukor. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
[www] Add CSP header to web UI
CSP (Content Security Policy) headers provide a way to tell the browser
where assets can be loaded from to prevent XSS attacks. Kudu's web UI is
read-only, at least for now, so it's not susceptible for XSS attacks,
but some security scanners still flag it as vulnerable due to not having
this header.
This patch adds a CSP header that allows loading assets on the same
host, and some inline styles and images in jQuery. It also removes all
inline style definitions from first-party files and moves them to
kudu.css.
There's no good way to write a unit test for this, as it requires a
GUI browser (curl doesn't load external resources and doesn't use
JavaScript), but I tested it manually both through HTTP and HTTPS and
confirmed there are no related errors in the JS console.
Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
---
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M www/kudu.css
M www/startup.mustache
4 files changed, 53 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/18285/3
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [www] Add CSP header to web UI
Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.
Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 3: Code-Review+1
Build failure (ASAN) due to unrelated Test:ReplicatedAlterTableTest.AlterReplicationFactorWhileWriting
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 14 Jul 2022 17:45:22 +0000
Gerrit-HasComments: No
[kudu-CR] [www] Add CSP header to web UI
Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.
Khazar Mammadli has uploaded a new patch set (#5) to the change originally created by Attila Bukor. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
[www] Add CSP header to web UI
CSP (Content Security Policy) headers provide a way to tell the browser
where assets can be loaded from to prevent XSS attacks. Kudu's web UI is
read-only, at least for now, so it's not susceptible for XSS attacks,
but some security scanners still flag it as vulnerable due to not having
this header.
This patch adds a CSP header that allows loading assets on the same
host, and some inline styles and images in jQuery. It also removes all
inline style definitions from first-party files and moves them to
kudu.css.
There's no good way to write a unit test for this, as it requires a
GUI browser (curl doesn't load external resources and doesn't use
JavaScript), but I tested it manually both through HTTP and HTTPS and
confirmed there are no related errors in the JS console.
Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
---
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M www/kudu.css
M www/startup.mustache
4 files changed, 51 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/18285/5
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [www] Add CSP header to web UI
Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.
Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18285/1//COMMIT_MSG
Commit Message:
PS1:
> Does it make sense to add a test into webserver-test.cc to check for the pr
I think this should be possible, a unit test would suffice just by knowing that the response contains a content security policy, but any kind of real csp checking is needed to be done through graphical user interface
http://gerrit.cloudera.org:8080/#/c/18285/1/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/18285/1/src/kudu/server/webserver.cc@684
PS1, Line 684: Content-Security-Policy
> Does it make sense to add a kill-switch flag to disable adding the CSP head
Don't think that adding the header might cause an issue, but it is a good idea to have a control knob to fall back to just in case
http://gerrit.cloudera.org:8080/#/c/18285/1/src/kudu/server/webserver.cc@685
PS1, Line 685: sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=
> What function/element is this for?
CSP blocks the inline styles when its enabled, this is a workaround to be able to use them.
Style-src defines valid sources of stylesheets or CSS, and 'self' allows for the usage of the files contained in the www folder.
We have kudu.css, bootstrap.min.css , bootstrap-table.min.css marked as stylesheet so any modifications to them will result in a change of this hash to my understanding.
The easiest way to get the hash is from the developer console of the browser/js console, it results in an error and shares the said hash in the error message. There are other ways to generate this hash code(such as openssl/some other online tools), but I haven't been able to get this exact hash as of now, only through the error console
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 13 Jul 2022 17:40:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [www] Add CSP header to web UI
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 02 Mar 2022 18:24:32 +0000
Gerrit-HasComments: No
[kudu-CR] [www] Add CSP header to web UI
Posted by "Khazar Mammadli (Code Review)" <ge...@cloudera.org>.
Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18285/4/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/18285/4/src/kudu/server/webserver.cc@96
PS4, Line 96: TAG_FLAG(webserver_enable_csp, runtime);
> This new flag is also run-time by the way how it's implemented, so it makes
Done
http://gerrit.cloudera.org:8080/#/c/18285/4/src/kudu/server/webserver.cc@648
PS4, Line 648: vector<string> encodings = strings::Split(accept_encoding_str, ",");
> style nit: put the space back
I think my IDE is up to some shady business as I've not modified the said lines, I'll take a closer look, as these don't even pop up on git diff
http://gerrit.cloudera.org:8080/#/c/18285/4/src/kudu/server/webserver.cc@689
PS4, Line 689: if (PREDICT_TRUE(FLAGS_webserver_enable_csp)) {
: oss << "Content-Security-Policy: default-src 'self';"
: // This hash has to be updated whenever kudu.css, bootstrap.min.css ,
: // bootstrap-table.min.css files change. The easiest way to obtain this hash is through browser/js
: // console. It is embedded in the error message.
: << "style-src 'self' 'unsafe-
> nit: the indent is incorrect for this block
done
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 14 Jul 2022 19:43:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 14 Jul 2022 19:47:03 +0000
Gerrit-HasComments: No
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#6) to the change originally created by Attila Bukor. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
[www] Add CSP header to web UI
CSP (Content Security Policy) headers provide a way to tell the browser
where assets can be loaded from to prevent XSS attacks. Kudu's web UI is
read-only, at least for now, so it's not susceptible for XSS attacks,
but some security scanners still flag it as vulnerable due to not having
this header.
This patch adds a CSP header that allows loading assets on the same
host, and some inline styles and images in jQuery. It also removes all
inline style definitions from first-party files and moves them to
kudu.css.
There's no good way to write a unit test for this, as it requires a
GUI browser (curl doesn't load external resources and doesn't use
JavaScript), but I tested it manually both through HTTP and HTTPS and
confirmed there are no related errors in the JS console.
Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
---
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M www/kudu.css
M www/startup.mustache
4 files changed, 50 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/18285/6
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [www] Add CSP header to web UI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18285 )
Change subject: [www] Add CSP header to web UI
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18285/1//COMMIT_MSG
Commit Message:
PS1:
> I think this should be possible, a unit test would suffice just by knowing
Right -- the idea is to have a unit test to make sure the expected header is present in the responses and spot future regressions, if any. The scope of tests in webserver-test.cc is exactly that.
http://gerrit.cloudera.org:8080/#/c/18285/1/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/18285/1/src/kudu/server/webserver.cc@684
PS1, Line 684: Content-Security-Policy
> Don't think that adding the header might cause an issue, but it is a good i
Right -- you never know when you encounter next incompatibility down the road, and having a kill switch for a new feature helps to lessen the risk of making Kudu webUI unusable in such situations.
http://gerrit.cloudera.org:8080/#/c/18285/1/src/kudu/server/webserver.cc@685
PS1, Line 685: sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=
> CSP blocks the inline styles when its enabled, this is a workaround to be a
The ask is to provide at least instructions in the comment: when to update the hash and how.
--
To view, visit http://gerrit.cloudera.org:8080/18285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I411d8f4ca079bfd5584f563aeeaa867833eb1106
Gerrit-Change-Number: 18285
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 13 Jul 2022 18:22:19 +0000
Gerrit-HasComments: Yes