feat(proxy) add configuration options to hide server tokens and latency tokens #2259

bungle · 2017-03-25T01:22:52Z

Summary

Implementation of #1009, taking care of hiding Kong proxy headers using configuration properties.

Full changelog

Add server_tokens and latency_tokens Kong configuration properties.

Issues resolved

Fix #1009

…cy tokens * Add server_tokens and latency_tokens Kong configuration properties. Fix #1009

Tieske · 2017-03-27T10:48:12Z

kong/conf_loader.lua

@@ -61,6 +61,8 @@ local CONF_INFERENCES = {
  cluster_advertise = {typ = "string"},
  nginx_worker_processes = {typ = "string"},
  upstream_keepalive = {typ = "number"},
+  server_tokens = {typ = "boolean"},
+  latency_tokens = {typ = "boolean"},



shouldn't those be called 'headers' instead of 'tokens'

@Tieske it is debatable (both works for me): http://nginx.org/en/docs/http/ngx_http_core_module.html#server_tokens

same here...

What's the conclusion, server_tokens vs. server_headers? Or something different?

server_header is kinda bad as it says "no headers at all", when off. Something like kong_version_headers and kong_latency_headers could be better.

just leave it at tokens.

Tieske · 2017-03-27T11:06:28Z

kong/conf_loader.lua

@@ -61,6 +61,8 @@ local CONF_INFERENCES = {
  cluster_advertise = {typ = "string"},
  nginx_worker_processes = {typ = "string"},
  upstream_keepalive = {typ = "number"},
+  server_tokens = {typ = "boolean"},
+  latency_tokens = {typ = "boolean"},



same here...

Tieske · 2017-03-27T11:06:39Z

spec/02-integration/05-proxy/10-server_tokens_spec.lua

+      assert.res_status(404, res)
+      assert.equal(nil, res.headers[constants.HEADERS.UPSTREAM_LATENCY])
+      assert.equal(nil, res.headers[constants.HEADERS.PROXY_LATENCY])
+    end)


fyi, you could have done

assert.response(res).has.status(404) assert.response(res).has.header(constants.HEADERS.UPSTREAM_LATENCY) assert.response(res).has.header(constants.HEADERS.PROXY_LATENCY)

tests exactly the same, but provides more context when they fail.

Tieske · 2017-03-27T11:07:40Z

kong/templates/kong_defaults.lua

@@ -19,6 +19,8 @@ admin_ssl = on
 admin_ssl_cert = NONE
 admin_ssl_cert_key = NONE
 upstream_keepalive = 60
+server_tokens = on
+latency_tokens = on



those also need to be added to the kong/kong.conf.default file including a description.

…on fields. See: Kong/kong#2259

thibaultcha · 2017-03-27T17:36:34Z

spec/02-integration/05-proxy/10-server_tokens_spec.lua

+      })
+
+      assert.response(res).has.status(404)
+      assert.response(res).has_not.header "via"


Those assertions are locking us in and will make it harder for us to switch to another mocking service. We should stop using them, as already raised in the style guide PR.

Oh, I just changed them based on comments by @Tieske.

thibaultcha · 2017-03-27T17:39:50Z

spec/02-integration/05-proxy/10-server_tokens_spec.lua

+    helpers.dao.apis:insert {
+      name = "api-1",
+      upstream_url = "http://localhost:9999/headers-inspect",
+      hosts = "inexistent.com",


It's really confusing to have this value set to inexistent.com. Especially when also using 404.com in the tests. How about header-inspect? Also, this value should be a table for consistency's sake and potentially forward compatibility.

thibaultcha · 2017-03-27T17:41:26Z

spec/02-integration/05-proxy/10-server_tokens_spec.lua

+      assert.response(res).has_not.header(constants.HEADERS.PROXY_LATENCY)
+    end)
+
+    it("should not be returned", function()


... when no API matched (no proxy)

thibaultcha · 2017-03-27T17:41:40Z

spec/02-integration/05-proxy/10-server_tokens_spec.lua

+      helpers.stop_kong()
+    end)
+
+    it("should not be returned", function()


... when request was proxied

thibaultcha · 2017-03-27T17:42:10Z

spec/02-integration/05-proxy/10-server_tokens_spec.lua

+      assert.response(res).has.header(constants.HEADERS.PROXY_LATENCY)
+    end)
+
+    it("should not be returned", function()


when no API matched (no proxy)

thibaultcha · 2017-03-27T17:42:40Z

spec/02-integration/05-proxy/10-server_tokens_spec.lua

+
+    teardown(helpers.stop_kong)
+
+    it("should be returned", function()


... when request was proxied

thibaultcha · 2017-03-27T17:43:28Z

spec/02-integration/05-proxy/10-server_tokens_spec.lua

+      assert.not_equal(default_server_header, res.headers["server"])
+    end)
+
+    it("should not return Kong 'Server' or 'Via' headers", function()


when no API matched (no proxy)

See: #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment)

…t/hide-kong-headers

See: #2259 (comment)

…t/hide-kong-headers

thibaultcha · 2017-03-29T22:05:45Z

CHANGELOG.md

+- :fireworks: `server_tokens` and `latency_tokens` configuration fields.
+  Check the [0.10 Configuration Guide](https://getkong.org/docs/0.10.x/configuration/#server_tokens)
+  to learn more.
+  [#2259](https://github.com/Mashape/kong/pull/2259)


We could explain a bit more what this change is about and respect the 80 cols limit as well. Maybe:

- Ability to hide Kong-specific response headers. Two new configuration fields: `server_tokens` and `latency_tokens` will respectively toggle whether the `Server` and `X-Kong-*-Latency` headers should be sent to downstream clients.

Also in general, there is no need to point to the configuration guide for new config fields, since the guide does not elaborate more on properties than the kong.conf.default file,which already has a comment for each property.

#2259 (review)

coopr · 2017-04-27T17:48:42Z

@bungle @thibaultcha I don't see this mentioned in https://github.com/Mashape/kong/blob/master/CHANGELOG.md#unreleased - is it coming up soon?

thibaultcha · 2017-04-27T18:19:33Z

This is merged to next, and as such, is in the changelog of the next branch, and not the master one.

Add two new configuration properties `server_tokens` and `latency_tokens` to toggle `Server` and `X-Kong-*-Latency` response headers. From #2259 Fix #1009

feat(proxy) add configuration options to hide server tokens and laten…

df51142

…cy tokens * Add server_tokens and latency_tokens Kong configuration properties. Fix #1009

bungle added the pr/status/needs review label Mar 25, 2017

Tieske reviewed Mar 27, 2017

View reviewed changes

Tieske requested changes Mar 27, 2017

View reviewed changes

bungle added a commit to Kong/docs.konghq.com that referenced this pull request Mar 27, 2017

docs(conf) documents server_tokens and latency_tokens configurati…

dcba8cc

…on fields. See: Kong/kong#2259

bungle mentioned this pull request Mar 27, 2017

docs(conf) add documentation for server_tokens and latency_tokens configuration fields Kong/docs.konghq.com#361

Merged

bungle added 2 commits March 27, 2017 14:45

Fix #2259 (comment)

1b766c2

Fix #2259 (comment)

f62edfd

Tieske approved these changes Mar 27, 2017

View reviewed changes

Tieske added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/needs review labels Mar 27, 2017

thibaultcha reviewed Mar 27, 2017

View reviewed changes

bungle added 7 commits March 27, 2017 21:56

Fix issues based on review comments by @thibault

f4c607c

See: #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment)

Fix issues based on review comments by @thibaultcha

1a3c9bd

See: #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment) #2259 (comment)

Merge remote-tracking branch 'origin/feat/hide-kong-headers' into fea…

b851ca9

…t/hide-kong-headers

Fix issues based on review comments by @thibaultcha

1870c79

See: #2259 (comment)

Fix issues based on review comments by @thibaultcha

59bfa28

See: #2259 (comment)

Merge remote-tracking branch 'origin/feat/hide-kong-headers' into fea…

2b42ab1

…t/hide-kong-headers

Hide "Server" header also from possible error page.

7f10580

bungle mentioned this pull request Mar 28, 2017

Request path url containing NULL (%00) character results in OpenResty response (400 HTTP Response code) #1816

Closed

Add server_tokens and latency_tokens to changelog

0372b38

thibaultcha reviewed Mar 29, 2017

View reviewed changes

bungle added 2 commits March 30, 2017 01:24

Fix review comment:

d769504

#2259 (review)

Merge branch 'next' into feat/hide-kong-headers

dc51940

thibaultcha approved these changes Mar 31, 2017

View reviewed changes

thibaultcha merged commit fd18d0d into next Mar 31, 2017

thibaultcha deleted the feat/hide-kong-headers branch March 31, 2017 20:19

thibaultcha mentioned this pull request Mar 31, 2017

Add configuration flag to hide kong headers #1009

Closed

thibaultcha pushed a commit that referenced this pull request Apr 28, 2017

feat(proxy) toggle server and latency tokens

9d76679

Add two new configuration properties `server_tokens` and `latency_tokens` to toggle `Server` and `X-Kong-*-Latency` response headers. From #2259 Fix #1009

thibaultcha pushed a commit that referenced this pull request Apr 29, 2017

feat(proxy) toggle server and latency tokens

72d0138

Add two new configuration properties `server_tokens` and `latency_tokens` to toggle `Server` and `X-Kong-*-Latency` response headers. From #2259 Fix #1009

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(proxy) add configuration options to hide server tokens and latency tokens #2259

feat(proxy) add configuration options to hide server tokens and latency tokens #2259

bungle commented Mar 25, 2017

Tieske Mar 27, 2017

bungle Mar 27, 2017 •

edited

Loading

Tieske Mar 27, 2017

bungle Mar 27, 2017

bungle Mar 27, 2017 •

edited

Loading

Tieske Mar 27, 2017

Tieske Mar 27, 2017

Tieske Mar 27, 2017

Tieske Mar 27, 2017

bungle Mar 27, 2017

thibaultcha Mar 27, 2017

bungle Mar 27, 2017

thibaultcha Mar 27, 2017

thibaultcha Mar 27, 2017

thibaultcha Mar 27, 2017

thibaultcha Mar 27, 2017

thibaultcha Mar 27, 2017

thibaultcha Mar 27, 2017

thibaultcha Mar 29, 2017

coopr commented Apr 27, 2017

thibaultcha commented Apr 27, 2017


		teardown(helpers.stop_kong)

		it("should be returned", function()

feat(proxy) add configuration options to hide server tokens and latency tokens #2259

feat(proxy) add configuration options to hide server tokens and latency tokens #2259

Conversation

bungle commented Mar 25, 2017

Summary

Full changelog

Issues resolved

Choose a reason for hiding this comment

bungle Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungle Mar 27, 2017 • 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

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

coopr commented Apr 27, 2017

thibaultcha commented Apr 27, 2017

bungle Mar 27, 2017 •

edited

Loading

bungle Mar 27, 2017 •

edited

Loading