Skip to content

Conversation

@krishna-pandey
Copy link
Contributor

@krishna-pandey krishna-pandey commented Apr 4, 2020

What is this PR for?

Zeppelin when installed with default configuration options doesn't enable the common web application security headers, e.g. zeppelin.server.xframe.options, zeppelin.server.xxss.protection, zeppelin.server.jetty.name, zeppelin.server.xcontent.type.options. This leaves the Zeppelin installation vulnerable.

What type of PR is it?

Improvement

Todos

  • Discuss HSTS header config (zeppelin.server.strict.transport) which if enabled requires TLS to be configured for Zeppelin to work

What is the Jira issue?

How should this be tested?

  • Below headers can be verified with received HTTP response
    Server:
    X-Content-Type-Options: nosniff
    X-FRAME-OPTIONS: SAMEORIGIN
    X-XSS-Protection: 1; mode=block

Here is Travis test run link which passed: https://travis-ci.org/github/krishna-pandey/zeppelin/builds/670946421

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@krishna-pandey
Copy link
Contributor Author

@prabhjyotsingh @jongyoul @Leemoonsoo @zjffdu
Can you please help review this? Thanks.

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From first look, it could be ok, but we need to test. Please configure travis & do the build.

Also, we need to update documentation about uncommented properties to say that they are on by default.

@krishna-pandey
Copy link
Contributor Author

@alexott It's happening as we speak at https://travis-ci.org/github/krishna-pandey/zeppelin/builds/670946421, also I will verify locally. Thanks.

@alexott
Copy link
Contributor

alexott commented Apr 4, 2020

Great! Thank you!
Can you add the link to build to the description of PR? And please update docs...

@krishna-pandey krishna-pandey requested a review from alexott April 4, 2020 18:10
@krishna-pandey
Copy link
Contributor Author

@alexott made the suggested changes, please review. Thanks.

Copy link
Contributor

@prabhjyotsingh prabhjyotsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alexott
Copy link
Contributor

alexott commented Apr 5, 2020

@krishna-pandey it looks good for me, but the question is following - we updated template for configuration, not configuration itself. If user will just download & unpack, these changes won't be applicable, because zeppelin-site.xml.template isn't copied into zeppelin-site.xml. Does it make sense to update source code with defaults as well?

@prabhjyotsingh WDYT?

@zjffdu
Copy link
Contributor

zjffdu commented Apr 5, 2020

Agree with @alexott We should update ZeppelinConfiguration as well

@Reamer
Copy link
Contributor

Reamer commented Apr 6, 2020

Discuss HSTS header config (zeppelin.server.strict.transport) which if enabled requires TLS to be configured for Zeppelin to work

I would love to see this in Zeppelin. Of course this should be off in default.

Can we completely suppress the zeppelin.server.jetty.name header in a production mode?

@alexott
Copy link
Contributor

alexott commented Apr 6, 2020

I think that we can create a followup ticket for HSTS - usually the introduction of TLS will require special build instructions, or pre-generated keys/certificates (bad idea)...

@krishna-pandey
Copy link
Contributor Author

Updated ZeppelinConfiguration with default values as suggested. Verified the same reflecting locally in absence of zeppelin-site.xml and zeppelin-env.sh.

@krishna-pandey
Copy link
Contributor Author

Also, Travis just finished running tests and build passed with last commit. (https://travis-ci.org/github/krishna-pandey/zeppelin/builds/671511015)
@alexott can you please review?
@Reamer IMHO not sending Server version at all or leaving it blank doesn't make much difference as everyone knows there must be a Server sending the response. A determined attacker can figure out other way to find Server details or will fire everything he had anyway.

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@asfgit asfgit closed this in 8952b27 Apr 6, 2020
asfgit pushed a commit that referenced this pull request Apr 6, 2020
### What is this PR for?
Zeppelin when installed with default configuration options doesn't enable the common web application security headers, e.g. zeppelin.server.xframe.options,  zeppelin.server.xxss.protection, zeppelin.server.jetty.name, zeppelin.server.xcontent.type.options. This leaves the Zeppelin installation vulnerable.

### What type of PR is it?
Improvement

### Todos
* Discuss HSTS header config (zeppelin.server.strict.transport) which if enabled requires TLS to be configured for Zeppelin to work

### What is the Jira issue?
* [ZEPPELIN-4723](https://issues.apache.org/jira/browse/ZEPPELIN-4723)

### How should this be tested?
* Below headers can be verified with received HTTP response
Server:
X-Content-Type-Options: nosniff
X-FRAME-OPTIONS: SAMEORIGIN
X-XSS-Protection: 1; mode=block

Here is Travis test run link which passed: https://travis-ci.org/github/krishna-pandey/zeppelin/builds/670946421

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Krishna Pandey <[email protected]>

Closes #3716 from krishna-pandey/ZEPPELIN-4723 and squashes the following commits:

afde17f [Krishna Pandey] Added default config values
01d3040 [Krishna Pandey] Added documentation for enabled headers with default values
f7578ad [Krishna Pandey] removed HSTS header, requires TLS config for Zeppelin to start
6fbd30e [Krishna Pandey] Enable HTTP security headers by default

(cherry picked from commit 8952b27)
Signed-off-by: Alex Ott <[email protected]>
@krishna-pandey krishna-pandey deleted the ZEPPELIN-4723 branch April 7, 2020 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants