Support HTTPS and introduce ssl_certificate policy phase #622

mikz · 2018-02-23T07:50:31Z

Allow APIcast to listen for HTTPS connections. Either by using static certificate or allow policy to supply one.

This PR has two parts:

Listen on HTTPS

Controlled by APICAST_HTTPS_PORT ENV variable.
Path to the certificate and key is defined by APICAST_HTTPS_CERTIFICATE and APICAST_HTTPS_CERTIFICATE_KEY ENV variables.

`ssl_certificate` phase

New policy phase can select a certificate. It can load it from network or a file. See lua-resty-autossl how to get letsencrypt certificates automatically.

Fixes #609

Depends on 3scale/Test-APIcast#9
Depends on 3scale/Test-APIcast#10

davidor · 2018-06-06T08:48:07Z

gateway/src/apicast/executor.lua

@@ -52,7 +52,7 @@ end
 -- @tparam string phase Nginx phase
 -- @treturn linked_list The context. Note: The list returned is 'read-write'.
 function _M:context(phase)
-    if phase == 'init' then
+    if phase == 'init' or phase == 'ssl_certificate' then


Good question. That is probably a leftover from a debugging session. Will take a look.

davidor · 2018-06-06T08:53:21Z

gateway/conf/nginx.conf.liquid

+    {%- assign http_port = port.apicast | default: 8080 %}
+    {%- assign https_port = env.APICAST_HTTPS_PORT %}
+
+    {% if http_port != https_port -%}


Have you tried this with https_port = nil ?
I remember problems when comparing nils in Liquid, but maybe it was a different issue.

I'd add a new test case in t/listen-https.t just to be sure.

So like the variable not set at all? That works - normal blackbox tests run with https_port not set.

The issue was that {% if env.somevar %} would be true even if it is empty.
This works just fine because http_port always has value.

davidor · 2018-06-06T09:02:13Z

gateway/conf/nginx.conf.liquid

@@ -77,6 +77,9 @@ http {
    {% include tracer_conf %}
  {% endif %}

+  ssl_session_fetch_by_lua_block { require('apicast.executor'):ssl_session_fetch() }


I don't see ssl_session_fetch() and ssl_session_store() defined anywhere. What I am missing?

Actually I did not want to commit those lines just say. Will remove them. Want to keep it for the future.

davidor · 2018-06-06T09:08:16Z

gateway/src/apicast/policy.lua

@@ -14,6 +14,8 @@ local PHASES = {
    'content', 'balancer',
    'header_filter', 'body_filter',
    'post_action',  'log', 'metrics',
+
+    'ssl_certificate',


Can you add this new phase to the list that we have in policies.md ?

davidor · 2018-06-06T09:08:43Z

gateway/src/apicast/policy/find_service/find_service.lua

 end

+_M.rewrite = find_service
+_M.ssl_certificate = find_service


ssl_certificate executes first and needs to have access to the service so it can execute the chain

I'm going to verify the context is shared or not after removing #622 (comment). But then it still needs to find the service in both (but can skip if the service is already found).

davidor · 2018-06-06T09:20:57Z

gateway/conf/nginx.conf.liquid

@@ -131,8 +134,10 @@ http {

    {% if https_port -%}
    listen {{ https_port }} ssl;
-    ssl_certificate {{ env.APICAST_HTTPS_CERTIFICATE }};
-    ssl_certificate_key {{ env.APICAST_HTTPS_CERTIFICATE_KEY }};
+    ssl_certificate {{ env.APICAST_HTTPS_CERTIFICATE | default: "conf/server.crt" | filesystem | first }};


Why is first needed?

because | filesystem produces a table and it can't be serialized.

davidor · 2018-06-06T09:22:59Z

gateway/src/apicast/policy/load_configuration/load_configuration.lua

@@ -30,4 +31,8 @@ function _M:rewrite(context)
    context.configuration = configuration_loader.rewrite(self.configuration, context.host)
 end

+function _M.ssl_certificate(_, context)
+    context.host = ssl.server_name()


ssl.server_name() can return an error: https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/ssl.md#server_name
Shouldn't we handle it?

If it returns error then context.host is going to be nil and no service policy chain will be found.
In that case we serve whatever certificate was set to APICAST_HTTPS_CERTIFICATE or the provided default.

Maybe we should just log the error with some low log level like debug.

davidor · 2018-06-06T13:25:09Z

doc/policies.md

@@ -53,6 +53,9 @@ Notice that we did not indicate what APIcast does in the `init` and the
 request. `init` is executed when APIcast boots, and `init_worker` when each
 of each of its workers start.

+Another phase that is not executed for every request is `ssl_certificate' because


That ' should be a `.

davidor · 2018-06-06T13:30:37Z

gateway/conf/nginx.conf.liquid

+    listen {{ https_port }} ssl;
+
+    {%- assign https_certificate = env.APICAST_HTTPS_CERTIFICATE -%}
+    ssl_certificate {% if https_certificate or https_certificate != empty -%}


I'm not sure I understand this. Shouldn't it be an and instead of an or ?
Does an empty string pass the first part of the or in Liquid?

I was trying to work around an issue with the liquid library. {% "somestring" == empty %} crashes. Will remove the or part as that would still not execute when using magic tables like in other parts (where we have to check for == empty).

davidor · 2018-06-06T14:01:55Z

gateway/conf/nginx.conf.liquid

+
+    {%- assign https_certificate = env.APICAST_HTTPS_CERTIFICATE -%}
+    ssl_certificate {% if https_certificate -%}
+      {{  https_certificate }}


This one does not need | filesystem | first ?

This is path that was given by user in ENV, so we should not need to do any transformations.

Also, it does not really work. The | filesystem filter does not really work with absolute paths (yet).

davidor · 2018-06-06T14:05:55Z

👍

includes 3scale/Test-APIcast#9 includes 3scale/Test-APIcast#10

* when APICAST_HTTPS_PORT is set * APICAST_HTTPS_CERTIFICATE needs to point to a certificate * APICAST_HTTPS_CERTIFICATE_KEY needs to point to a private key

* allows policies to serve ssl certificate * service is detected by host from SNI * policy should clear certificates before setting them

octobot assigned mikz Feb 23, 2018

mikz force-pushed the listen-https branch 3 times, most recently from ab81e24 to 1df0f9a Compare June 5, 2018 12:52

mikz changed the title ~~[wip] listen on https~~ Support HTTPS and introduce ssl_certificate policy phase Jun 5, 2018

mikz force-pushed the listen-https branch from 1df0f9a to 7c64dbb Compare June 5, 2018 13:36

mikz mentioned this pull request Jun 6, 2018

An APIcast policy that enables SSL/TLS client certificate authentication #603

Closed

davidor reviewed Jun 6, 2018

View reviewed changes

mikz requested a review from a team as a code owner June 6, 2018 12:20

davidor reviewed Jun 6, 2018

View reviewed changes

mikz added 6 commits June 6, 2018 16:06

[t] update Test::APIcast

7e86746

includes 3scale/Test-APIcast#9 includes 3scale/Test-APIcast#10

[example] update expired certificates

ca8e23a

[liquid] allow accessing ENV by key

82f0d35

[nginx] allow listening on https

08e81e5

* when APICAST_HTTPS_PORT is set * APICAST_HTTPS_CERTIFICATE needs to point to a certificate * APICAST_HTTPS_CERTIFICATE_KEY needs to point to a private key

[echo] echo policy should return json when asked

90c06af

[policy] introduce ssl_certificate phase

20989fc

* allows policies to serve ssl certificate * service is detected by host from SNI * policy should clear certificates before setting them

mikz force-pushed the listen-https branch from 5b0da4c to 20989fc Compare June 6, 2018 14:07

davidor approved these changes Jun 6, 2018

View reviewed changes

mikz merged commit 017d4d3 into master Jun 6, 2018

mikz deleted the listen-https branch June 6, 2018 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support HTTPS and introduce ssl_certificate policy phase #622

Support HTTPS and introduce ssl_certificate policy phase #622

mikz commented Feb 23, 2018 •

edited

Loading

davidor Jun 6, 2018

mikz Jun 6, 2018

davidor Jun 6, 2018

davidor Jun 6, 2018

mikz Jun 6, 2018

davidor Jun 6, 2018

davidor Jun 6, 2018

mikz Jun 6, 2018

davidor Jun 6, 2018

mikz Jun 6, 2018

davidor Jun 6, 2018

mikz Jun 6, 2018

davidor Jun 6, 2018

mikz Jun 6, 2018

davidor Jun 6, 2018

mikz Jun 6, 2018

davidor Jun 6, 2018

davidor Jun 6, 2018

mikz Jun 6, 2018

davidor Jun 6, 2018

mikz Jun 6, 2018

mikz Jun 6, 2018

davidor Jun 6, 2018

davidor commented Jun 6, 2018

Support HTTPS and introduce ssl_certificate policy phase #622

Support HTTPS and introduce ssl_certificate policy phase #622

Conversation

mikz commented Feb 23, 2018 • edited Loading

Listen on HTTPS

ssl_certificate phase

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

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

davidor commented Jun 6, 2018

mikz commented Feb 23, 2018 •

edited

Loading

`ssl_certificate` phase