Add config properties for HTTP security headers #97158

jportner · 2021-04-14T17:18:20Z

Overview

This PR adds the following config properties (each prefixed by server.securityResponseHeaders):

Config property	Default value	Notes
`.strictTransportSecurity`	`null` (Header is not enabled)	This controls the Strict-Transport-Security header, which is an excellent feature to support on your site and strengthens your implementation of TLS by getting the User Agent to enforce the use of HTTPS. Can be set to `null` to disable this header.
`.xContentTypeOptions`	`'nosniff'` (Header is enabled)	This controls the X-Content-Type-Options header, which stops a browser from trying to MIME-sniff the content type and forces it to stick with the declared content-type. The only valid value for this header is "X-Content-Type-Options: nosniff". Can be set to `null` to disable this header.
`.referrerPolicy`	`'no-referrer-when-downgrade'` (Header is enabled)	This controls the Referrer-Policy header, which is a new header that allows a site to control how much information the browser includes with navigations away from a document and should be set by all sites. Can be set to `null` to disable this header.
`.permissionsPolicy`	`null` (Header is not enabled)	This controls the Permissions-Policy header, which is a new header that allows a site to control which features and APIs can be used in the browser. Can be set to `null` to disable this header.
`.disableEmbedding`	`false` (Header is not enabled, embedding is allowed)	This influences the default Content-Security-Policy header and controls the X-Frame-Options header, which tells the browser whether you want to allow your site to be framed or not. By preventing a browser from framing your site you can defend against attacks like clickjacking.

New scan result from securityheaders.com with default settings ("C" rating):

Docs

Docs preview: link (diff)

Note concerning the docs: the asciidoc hacks that I needed to make for the settings table in bb8e2aa are kind of awful, but it seems to be the only solution that will work. The key names for the new config options are too long, and they stretch the left column of the table out to the maximum size as a result. Regular columns in AsciiDoc will not preserve leading whitespace characters, so I had to utilize an AsciiDoc block element column style with the a operator.

Screenshots before and after `bb8e2aa`

Before:

After:

This was recently renamed to `server.maxPayload`, I just updated the docs and other references accordingly. Unrelated to the other work in this PR besides the fact that I am mucking around in the `src/core/server/http/http_config.ts` file.

jportner

Author's notes for reviewers

jportner · 2021-04-15T14:26:17Z

src/core/server/http/lifecycle_handlers.ts

    const additionalHeaders = {
-      ...customHeaders,
+      ...securityResponseHeaders,
+      ...customResponseHeaders,
      [KIBANA_NAME_HEADER]: serverName,
    };


We explicitly allow customResponseHeaders to override any values in securityResponseHeaders. This is so the new config properties do not break existing configurations that already set the same headers using customResponseHeaders.

We could detect this situation when the config is set and throw an error, but we'd have to make that change in the 8..0 release, I think.

jportner · 2021-04-15T14:31:33Z

src/core/server/http/security_response_headers_config.ts

+const strictTransportSecuritySchema = schema.object({
+  // See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security
+  maxAge: schema.duration({ defaultValue: '1Y' }),
+  includeSubDomains: schema.boolean({ defaultValue: false }),
+  preload: schema.boolean({ defaultValue: false }),
+});


By default, Strict-Transport-Security is enabled with a max age of 1 year. This is recommended by MDN and most other authoritative sources. We could decrease this but I'm not sure we should.

I omitted includeSubDomains by default because of #52809 (comment).

Note: if Kibana is served over HTTP, this header is still sent, but browsers ignore it. It only affects browsers once they see this header in an HTTPS response.

jportner · 2021-04-15T14:32:24Z

src/core/server/http/security_response_headers_config.ts

+  xContentTypeOptions: schema.oneOf([schema.literal('nosniff'), schema.literal(null)], {
+    // See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Content-Type-Options
+    defaultValue: 'nosniff',
+  }),


By default, X-Content-Type-Options is enabled. It only has one valid value. This shouldn't have any negative impact on users.

jportner · 2021-04-15T14:36:00Z

src/core/server/http/security_response_headers_config.ts

+  referrerPolicy: schema.oneOf(
+    // See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referrer-Policy
+    [
+      schema.literal('no-referrer'),
+      schema.literal('no-referrer-when-downgrade'),
+      schema.literal('origin'),
+      schema.literal('origin-when-cross-origin'),
+      schema.literal('same-origin'),
+      schema.literal('strict-origin'),
+      schema.literal('strict-origin-when-cross-origin'),
+      schema.literal('unsafe-url'),
+      schema.literal(null),
+    ],
+    { defaultValue: 'no-referrer-when-downgrade' }
+  ),


By default, Referrer-Policy is enabled with a value of "no-referrer-when-downgrade". This is not very strict, it does the bare minimum to prevent the Referer header from being sent to unsafe (HTTP) destinations when Kibana is served over HTTPS. See also #52809 (comment)

jportner · 2021-04-15T14:44:39Z

src/core/server/http/security_response_headers_config.ts

+  permissionsPolicy: schema.oneOf([schema.string(), schema.literal(null)], {
+    defaultValue: 'camera=(), microphone=()', // disables camera and microphone access in Kibana and any embedded pages
+  }),


By default, Permissions-Policy is enabled with a value of "camera=(), microphone=()". There is not a lot of authoritative info on this header yet (the header was previously Feature-Policy but has been slightly reworked and renamed). I figured we can set it with some safe feature policies as a starting point.

Note that Permissions-Policy is subtractive, so if we do not specify a directive, that is not affected. Kibana shouldn't ever need access to a user's camera or microphone, so what we are saying here is that Kibana (and any embedded pages) are not allowed to access these features.

See also: https://github.com/w3c/webappsec-permissions-policy/blob/main/permissions-policy-explainer.md

Edit: I believe another, more clear way of writing this permissions policy is: 'camera=(none), microphone=(none)'. While the W3C draft mentions the none keyword, the explainer document above does not.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy#example leads me to believe that we should be using Permissions-Policy: camera 'none'; microphone 'none'. It also looks like this isn't very widely supported per https://caniuse.com/permissions-policy. Perhaps we should just hold off on this one until it matures? Does the lack of this header get flagged by scanners right now?

The page is a bit misleading / outdated. That's the format for the old Feature-Policy header, it hasn't been updated to the Permissions-Policy format.

At any rate based on #97158 (comment) below, I think we will leave this header disabled by default and mark the config prop as experimental, if that sounds good to you.

Sounds good to me! Thanks.

jportner · 2021-04-15T14:48:40Z

src/core/server/http/security_response_headers_config.ts

+  permissionsPolicy: schema.oneOf([schema.string(), schema.literal(null)], {
+    defaultValue: 'camera=(), microphone=()', // disables camera and microphone access in Kibana and any embedded pages
+  }),
+  disableEmbedding: schema.boolean({ defaultValue: false }), // is used to control X-Frame-Options and CSP headers


When enabled, this changes the default Content-Security-Policy header (by adding the frame-ancestors directive) and adds the X-Frame-Options header.

Even though it appears that all of Kibana's target browsers support frame-ancestors, we still include X-Frame-Options for a couple of reasons:

If the user has customized the Content-Security-Policy for some reason, we won't add the frame-ancestors directive -- so including X-Frame-Options as a backup is a good idea

Naive security scanning tools can see a missing X-Frame-Options as a problem, even if there is a Content-Security-Policy present with a frame-ancestors directive

In the future (8.0 release?) I would love to change this to be enabled by default.

I came looking for answers to these questions - thanks for taking the time to explain!

jportner · 2021-04-15T14:49:14Z

src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker

@@ -116,12 +116,20 @@ kibana_vars=(
    server.compression.referrerWhitelist
    server.cors
    server.cors.origin
-    server.customResponseHeaders


This was a duplicate value so I removed it (it still exists below on line 127)

jportner · 2021-04-15T14:50:24Z

src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker

    server.maxPayloadBytes
+    server.maxPayload


I added server.maxPayload but left the deprecated server.maxPayloadBytes for now (see 8cc60bf)

jportner · 2021-04-15T14:51:56Z

src/core/server/csp/csp_config.ts

+    if (!rawCspConfig.rules?.length && source.disableEmbedding) {
+      this.rules.push(FRAME_ANCESTORS_RULE);
+    }


It might be a good idea to log a warning if disableEmbedding is enabled but custom CSP rules are used that do not include the frame-ancestors directive. But I think we can leave that for a future enhancement, as we don't have the ability to log when config is ingested at the moment.

jbudz

kibana-docker LGTM!

KOTungseth

I have a handful of nits, but otherwise, LGTM!

docs/setup/settings.asciidoc

thomheymann

Looks great! Tested locally and can confirm that Kibana responds with correct security headers.

Couple minor comments below.

docs/setup/settings.asciidoc

thomheymann · 2021-04-15T16:06:13Z

docs/setup/settings.asciidoc

+a|
+----
+server.securityResponseHeaders:
+  strictTransportSecurity.preload:


@elastic/kibana-docs What's the best way of printing long settings like this? The table is too narrow to fit both the name and description. However, breaking it up like this makes it a bit confusing for users too.

@thomheymann the table capabilities in Asciidoc are limited, and I agree that breaking up the setting names is confusing. These would be best displayed in a definition list instead of a table. I propose that we merge these changes, then I'll fix the formatting before 7.13 releases.

Previously, this property contained three sub-properties that each controlled individual directives in the header. This is really more complexity than is necessary, we can simply allow users to enter in free text for this header.

legrego · 2021-04-15T18:25:56Z

src/core/server/http/security_response_headers_config.ts

+  permissionsPolicy: schema.oneOf([schema.string(), schema.literal(null)], {
+    defaultValue: 'camera=(), microphone=()', // disables camera and microphone access in Kibana and any embedded pages
+  }),
+  disableEmbedding: schema.boolean({ defaultValue: false }), // is used to control X-Frame-Options and CSP headers


I came looking for answers to these questions - thanks for taking the time to explain!

legrego · 2021-04-15T18:38:51Z

src/core/server/http/security_response_headers_config.ts

+    ],
+    { defaultValue: 'no-referrer-when-downgrade' }
+  ),
+  permissionsPolicy: schema.oneOf([schema.string(), schema.literal(null)], {


I'm wary of supporting production deployments of an experimental web technology, with very mixed browser support:

I'd be more comfortable if we omitted this option for the time being. Users who want to control this have a way to do so via custom response headers, and they can do so at their own risk.

If we have a compelling reason to keep this (aside from the score card), then I'd like to see this setting marked as experimental so that we aren't committing to support of this specific header.

I briefly looked around to see if there were high profile sites using this header, and I haven't found any usages of it yet. I know they exist somewhere, I just haven't found them 😉 . I was trying to get an idea of how comfortable other companies were using this experimental header in production.

Sure, I was on the fence about this as well. I just wanted to do as much as possible to get ahead of scanning tools, but I think the only one that checks for this atm is securityheaders.com, and I think if you don't use this header it just caps your score at "A" (instead of "A+").

Perhaps we can leave the config property in, but change it to null/disabled by default?

I'd still like to mark the property as experimental -- we don't know what the future holds for deprecating/removing features after the 8.0 release, so this would give us some more flexibility

legrego

One followup thought -- we should update core telemetry to indicate if these settings have been altered. It'd be great to track this over time in case we need data about future changes we want to make.

kobelb · 2021-04-15T20:31:48Z

src/core/server/http/security_response_headers_config.ts

+export const securityResponseHeadersSchema = schema.object({
+  strictTransportSecurity: schema.oneOf([schema.string(), schema.literal(null)], {
+    // See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security
+    defaultValue: 'max-age=31536000', // 1 year in seconds


How does Strict-Transport-Security: max-age=31536000 affect Kibana when it's hosted on a base-path? For example, if Kibana is hosted at https://foo.com/kibana/ will it apply to websites at https://foo.com/bar? If it's in effect for the entire domain, at a minimum, we won't want to set this in 7.13.

http://gph.is/1lX4Z3B

I'll make an issue to change the defaults for strictTransportSecurity and disableEmbedding in the 8.0 release.

Edit: added in #97348

jportner · 2021-04-16T02:04:11Z

Removed defaults for strictTransportSecurity and permissionsPolicy, and added usage data collection.

New securityheaders.com scan result:

jportner · 2021-04-16T02:37:53Z

One API integration test is failing with the following message:

     │1)    apis
     │       Telemetry
     │         /api/telemetry/v2/clusters/_stats
     │           validate data types
     │             should pass the schema validation:
     │
     │      Error: The telemetry schemas in 'src/plugins/telemetry/schema/' are out-of-date, please update it as required: [stack_stats.kibana.plugins.core.config.http.securityResponseHeaders.strictTransportSecurity]: expected value of type [string] but got [null]
     │       at ObjectType.validate (packages/kbn-config-schema/src/types/type.ts:70:13)
     │       at assertTelemetryPayload (test/api_integration/apis/telemetry/utils/schema_to_config_schema.ts:137:34)
     │       at Context.apply (test/api_integration/apis/telemetry/telemetry_local.ts:58:11)
     │       at Object.apply (node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:30)

I set the type for this and some other security headers in the usage data config as string | null (reference), and I set the ES field mapping to "keyword" (reference). It seems that the existing schema validation in the API integration tests does not support nullable values, though.

I can think of two obvious solutions:

Allow keyword and test values to be nullable in the API integration tests:

diff --git a/test/api_integration/apis/telemetry/utils/schema_to_config_schema.ts b/test/api_integration/apis/telemetry/utils/schema_to_config_schema.ts
index b45930682e3..84409cfe1f7 100644
--- a/test/api_integration/apis/telemetry/utils/schema_to_config_schema.ts
+++ b/test/api_integration/apis/telemetry/utils/schema_to_config_schema.ts
@@ -59,7 +59,7 @@ function valueSchemaToConfigSchema(value: TelemetrySchemaValue): Type<unknown> {
       case 'keyword':
       case 'text':
       case 'date':
-        return schema.string();
+        return schema.nullable(schema.string());
       case 'byte':
       case 'double':
       case 'float':

Convert null values to "NULL" strings in the usage collector, and change the string | null type to string

@elastic/kibana-telemetry what do you think?

pgayvallet

Technically, LGTM

src/core/server/csp/csp_config.ts

jportner · 2021-04-19T14:37:07Z

I can think of two obvious solutions:
Allow keyword and test values to be nullable in the API integration tests:
 ...
Convert null values to "NULL" strings in the usage collector, and change the string | null type to string
@elastic/kibana-telemetry what do you think?

I spoke to @Bamieh and @afharo offline and this point was raised:

my only concern with allowing null to types is that it'll bring more unexpected scenarios that it'll do good. it is also mean that we'd need to treat every null we get in the ES indexing since ES does not index null and it cannot be searched.

So we agreed that option (2) above is the best path forward. I updated this accordingly in a501b8c.

afharo

Just one note around the descriptions :)

afharo · 2021-04-19T14:49:26Z

src/plugins/telemetry/schema/oss_plugins.json

+                    "strictTransportSecurity": {
+                      "type": "keyword",
+                      "_meta": {
+                        "description": "The strictTransportSecurity response header, if it is configured."


if it is configured

I don't think this is true. We are always reporting it. Either the configured value or "NULL". Maybe I misread the code though. Can you confirm @jportner?

Oh, yes sorry about that, I forgot to update the descriptions when I added the null coalescing. I'll update those now.

Edit: done in fd78745.

Edit 2: s/coalescing/coercing/

kibanamachine · 2021-04-19T17:12:40Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`core`	2172	2174	+2

API count missing comments

id	before	after	diff
`core`	991	992	+1

History

💔 Build #120589 failed a501b8c
💔 Build #120243 failed d06cd8c
💔 Build #120238 failed a0ecb67
💚 Build #120061 succeeded 34d1650
💔 Build #120056 failed 84828e3

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2021-04-19T17:35:21Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 97158

# Conflicts: # src/core/server/csp/csp_config.test.ts

elasticmachine · 2021-04-26T09:08:47Z

Pinging @elastic/kibana-security (Team:Security)

jportner added release_note:enhancement v8.0.0 v7.13.0 labels Apr 14, 2021

jportner linked an issue Apr 14, 2021 that may be closed by this pull request

Update docs and other usage of deprecated server.maxPayloadBytes config #97153

Closed

jportner force-pushed the add-security-headers-config branch 3 times, most recently from a9feda4 to 0d90a4f Compare April 15, 2021 00:49

jportner force-pushed the add-security-headers-config branch from 0d90a4f to 62864b1 Compare April 15, 2021 01:18

Add security response headers

5745057

jportner force-pushed the add-security-headers-config branch from 62864b1 to 5745057 Compare April 15, 2021 02:43

Fix docs format

bb8e2aa

jportner commented Apr 15, 2021

View reviewed changes

jportner marked this pull request as ready for review April 15, 2021 14:52

jportner requested review from a team as code owners April 15, 2021 14:52

jbudz approved these changes Apr 15, 2021

View reviewed changes

jportner requested a review from a team April 15, 2021 14:58

Merge branch 'master' into pr/jportner/97158

3c2671d

KOTungseth approved these changes Apr 15, 2021

View reviewed changes

thomheymann approved these changes Apr 15, 2021

View reviewed changes

jportner added 3 commits April 15, 2021 12:09

Docs team PR review feedback

84828e3

Fix type check 🤦

34d1650

legrego reviewed Apr 15, 2021

View reviewed changes

kobelb reviewed Apr 15, 2021

View reviewed changes

jportner added 2 commits April 15, 2021 20:38

Merge branch 'master' into pr/jportner/97158

a5e8070

More PR review feedback

db05411

Add usage data collection

a0ecb67

jportner requested a review from a team as a code owner April 16, 2021 01:59

jportner mentioned this pull request Apr 16, 2021

Educate users on HTTP security headers #97348

Open

Update API Docs

d06cd8c

pgayvallet approved these changes Apr 19, 2021

View reviewed changes

src/core/server/csp/csp_config.ts Outdated Show resolved Hide resolved

jportner added 3 commits April 19, 2021 09:36

Merge branch 'master' into pr/jportner/97158

ba20102

PR review feedback

ec84e09

Coalesce nullable string values in usage data to "NULL" strings

a501b8c

jportner enabled auto-merge (squash) April 19, 2021 14:40

afharo reviewed Apr 19, 2021

View reviewed changes

Fix type check error and usage data descriptions

fd78745

jportner requested a review from afharo April 19, 2021 15:37

jportner mentioned this pull request Apr 19, 2021

[actions] adds config allowing per-host networking options #96630

Merged

15 tasks

jportner merged commit 978f5ec into elastic:master Apr 19, 2021

jportner added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 19, 2021

jportner mentioned this pull request Apr 19, 2021

[7.x] Add config properties for HTTP security headers (#97158) #97503

Merged

jportner added a commit that referenced this pull request Apr 19, 2021

Add config properties for HTTP security headers (#97158) (#97503)

8d8b0f3

# Conflicts: # src/core/server/csp/csp_config.test.ts

jportner deleted the add-security-headers-config branch April 20, 2021 13:10

timroes added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Apr 26, 2021

legrego mentioned this pull request Aug 11, 2022

Defend against undesired forms of iframe embedding #8519

Closed

This was referenced Oct 24, 2022

Consider including Cross-Origin-Opener-Policy: same-origin to default response headers #141780

Closed

Apply Content-Security-Policy to all server responses #143871

Closed

legrego mentioned this pull request Feb 10, 2023

Harden security response headers for serverless offering #150884

Closed

Add config properties for HTTP security headers #97158

Add config properties for HTTP security headers #97158

Conversation

jportner commented Apr 14, 2021 • edited Loading

Overview

New scan result from securityheaders.com with default settings ("C" rating):

Docs

Screenshots before and after bb8e2aa

jportner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbudz left a comment

Choose a reason for hiding this comment

KOTungseth left a comment

Choose a reason for hiding this comment

thomheymann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

jportner commented Apr 16, 2021

jportner commented Apr 16, 2021 • edited Loading

pgayvallet left a comment

Choose a reason for hiding this comment

jportner commented Apr 19, 2021 • edited Loading

afharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Apr 19, 2021

💚 Build Succeeded

Metrics [docs]

API count

API count missing comments

History

kibanamachine commented Apr 19, 2021

💔 Backport failed

elasticmachine commented Apr 26, 2021

jportner commented Apr 14, 2021 •

edited

Loading

Screenshots before and after `bb8e2aa`

jportner Apr 15, 2021 •

edited

Loading

thomheymann left a comment •

edited

Loading

jportner Apr 16, 2021 •

edited

Loading

jportner commented Apr 16, 2021 •

edited

Loading

jportner commented Apr 19, 2021 •

edited

Loading

jportner Apr 19, 2021 •

edited

Loading