[THREESCALE-1430] Avoid trying to use path-based routing when using HTTPS #938

davidor · 2018-10-19T14:44:54Z

When APICAST_HTTPS_ env vars are used to configure SSL certs, enabling path routing does not work. The reason is that when those envs are configured, the find_service policy runs in the ssl_certificate phase which does not have access to the http method nor the path, needed for the path-based routing.

Apart from fixing the issue, this PR refactors the find_service by extracting a couple of classes and reorganizing the tests. I think that properly testing the ssl_certiticate phase without this refactor would have needed lots of duplicated tests.

Ref: https://issues.jboss.org/browse/THREESCALE-1430

I think this makes the tests clearer now. More importantly, it will allow us to avoid duplicating tests when testing the "ssl_certificate" phase of the "find_service" policy.

codeclimate · 2018-10-19T14:45:23Z

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

@@ -0,0 +1,23 @@
+local _M = {}
+
+function _M.find_service(config_store, host)


cyclomatic complexity of function '_M.find_service' is too high (8 > 7)

codeclimate · 2018-10-19T14:45:23Z

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

+
+local _M = {}
+
+function _M.find_service(config_store, host)


cyclomatic complexity of function '_M.find_service' is too high (8 > 7)

mikz · 2018-10-19T15:47:46Z

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

+  end
+
+  context.service = context.service or
+                    host_based_finder.find_service(context.configuration, context.host)


Maybe you can store just find_service in the local variable, so the JIT has it easier and does not have to check if the table changed.

But I see that because of the testing code and stubs you actually need it to be in a table. That is unfortunate.

mikz · 2018-10-19T15:48:52Z

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


 function _M.new(...)
  local self = new(...)

  if configuration_store.path_routing then
    ngx.log(ngx.WARN, 'apicast path routing enabled')
-    self.find_service = find_service_cascade
+    self.find_service = function(configuration, host)


This function can be defined in a local variable when loading the file, right?

That would be problematic to test as you mentioned above.
This only runs when the configuration is loaded, so it should be fine.

mayorova · 2018-10-22T11:45:08Z

@davidor
It says "Apart from fixing the issue", but from the code it seems that it doesn't fully fix it, but instead the service finder falls back to finding service only by host, and not by host + path.

While it doesn't matter for SSL in current version, wouldn't it cause unexpected behavior for the rest of the functionality? (as, from what I understand, the found service is stored in the context)

mikz · 2018-10-22T11:57:40Z

@mayorova during TLS handshake there is no information about the request method, path or any other HTTP data. It is just the TLS protocol, not HTTP. So the only piece of information available is the hostname through the SNI.

Regarding the context, it makes sense that this context should not be reused for the normal request because it wasn't part of the HTTP protocol. Not sure that is the case now and it is definitely something to verify.

mayorova · 2018-10-22T12:14:48Z

@mikz Yes, I understand that it's not possible to perform method/path-based matching at the TLS-handshake stage.
But if that's the case we need to:

make it clear in the docs, and at some point when SSL is customizable per-service, this needs to be very clear
exactly what you've said:

it makes sense that this context should not be reused for the normal request because it wasn't part of the HTTP protocol.

I'm also not sure if it's taken into account in this PR :)

mayorova · 2018-11-16T16:58:13Z

@davidor Can you please have a look at the comments above?

Regarding the context, it makes sense that this context should not be reused for the normal request because it wasn't part of the HTTP protocol. Not sure that is the case now and it is definitely something to verify.

Basically, I think now Path routing might not work correctly with HTTPS (if there are >1 services with the same host).

nmasse-itix · 2018-11-29T09:13:52Z

Hello,

We can agree that we cannot use Path Routing to find the matching service to find the correct TLS certificate during the TLS negociation.

However, I do not agree that we should disable Path Routing with HTTPS. We can still use the ssl_certificate phase with global policies. That way, the customer can still craft a global policy that fetch the correct certificate and have path routing.

Since finding the correct TLS certificate is quite a standard mechanism, we could also provide the global policy that implement this mechanism :

extract the host from the TLS negociation via the SNI extension
find the first certificate that has a SAN or CN with an exact match
if not found, find the first certificate that has a SAN or CN with wildcard match
if not found, offer a default configured certificate

Certificates can be supplied as a Kubernetes secret and mounted in a well-known or configured folder.

That way, we could also leverage the OpenShift Service Signing Certificates and have a fully automated HTTPS setup on OpenShift.

mikz · 2018-11-29T14:18:30Z

I imagine that when Path Routing is enabled, then the ssl_certificate phase should be executed for all the service configurations that match the TLS SNI.

nmasse-itix · 2018-11-30T15:54:49Z

Hi @mikz, @davidor, @mayorova,

I don't want to bother you but I really need to make sure I correctly understood this PR.

The issue was that we were looking for the service by host + path when path routing was enabled and when SSL was enabled we could not find the correct service since we cannot have access to the path at this stage.

I initially understood from both the changelog and the discussions in this PR that the proposed fix was to disable Path Routing when HTTPS was in use.

I tested this PR and it seems that we implemented host-based routing for the ssl_certificate phase but still use the correct service found through the path routing for the later stages.

Can you confirm the former or the later understanding is correct ?

mikz · 2018-12-04T10:34:49Z

@nmasse-itix you're not bothering. We should've explained it better.

The issue was that there was a crash because we tried to use API that wasn't available in that phase of a request. We could not access the request path.

So the mitigation was to use the same technique as without path routing: just compare the service host against the SNI and pick a first one that matches. That mitigation is applied only to the ssl_certifiate phase.

After the TLS is established it is business as usual and the request path is used to find the correct service.
What it means to a customer is that all the services that share the host should have the same policies that provide a certificate in the ssl_certificate phase.

davidor added 4 commits October 19, 2018 11:46

spec/policy/find_service: simplify by requiring once

8332fec

policy/find_service: extract HostBasedFinder and PathBasedFinder

8a8d1c6

I think this makes the tests clearer now. More importantly, it will allow us to avoid duplicating tests when testing the "ssl_certificate" phase of the "find_service" policy.

policy/find_service: use host-based in .ssl_certificate

0c36f2c

t/listen-https: add test with path-based routing

4f92a55

davidor requested a review from a team as a code owner October 19, 2018 14:44

codeclimate bot reviewed Oct 19, 2018

View reviewed changes

CHANGELOG: add fix for using path-based routing with SSL certs

7b59455

davidor force-pushed the path-based-with-ssl branch from 9e8c28c to 7b59455 Compare October 19, 2018 14:51

davidor requested a review from mikz October 19, 2018 15:05

mikz approved these changes Oct 19, 2018

View reviewed changes

davidor merged commit 2b48410 into master Oct 22, 2018

davidor deleted the path-based-with-ssl branch October 22, 2018 08:40

mikz mentioned this pull request Dec 4, 2018

[t] test that path routing works with HTTPS #965

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[THREESCALE-1430] Avoid trying to use path-based routing when using HTTPS #938

[THREESCALE-1430] Avoid trying to use path-based routing when using HTTPS #938

davidor commented Oct 19, 2018

codeclimate bot Oct 19, 2018

codeclimate bot Oct 19, 2018

mikz Oct 19, 2018

mikz Oct 19, 2018

davidor Oct 22, 2018

mayorova commented Oct 22, 2018

mikz commented Oct 22, 2018

mayorova commented Oct 22, 2018

mayorova commented Nov 16, 2018

nmasse-itix commented Nov 29, 2018

mikz commented Nov 29, 2018

nmasse-itix commented Nov 30, 2018

mikz commented Dec 4, 2018

		@@ -0,0 +1,23 @@
		local _M = {}

		function _M.find_service(config_store, host)

[THREESCALE-1430] Avoid trying to use path-based routing when using HTTPS #938

[THREESCALE-1430] Avoid trying to use path-based routing when using HTTPS #938

Conversation

davidor commented Oct 19, 2018

codeclimate bot Oct 19, 2018

Choose a reason for hiding this comment

codeclimate bot Oct 19, 2018

Choose a reason for hiding this comment

mikz Oct 19, 2018

Choose a reason for hiding this comment

mikz Oct 19, 2018

Choose a reason for hiding this comment

davidor Oct 22, 2018

Choose a reason for hiding this comment

mayorova commented Oct 22, 2018

mikz commented Oct 22, 2018

mayorova commented Oct 22, 2018

mayorova commented Nov 16, 2018

nmasse-itix commented Nov 29, 2018

mikz commented Nov 29, 2018

nmasse-itix commented Nov 30, 2018

mikz commented Dec 4, 2018