Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[THREESCALE-9193] upstream TLS v1.3 #1400

Merged
merged 7 commits into from
Apr 11, 2023
Merged

Conversation

eguzki
Copy link
Member

@eguzki eguzki commented Mar 31, 2023

What

Fixes https://issues.redhat.com/browse/THREESCALE-9193

This PR enables upstream powered by TLS v1.3 in two scenarios:

  • APIcast <> upstream TLS v1.3
  • APIcast <> HTTP proxy <> upstream TLS v1.3

Adds two docker environments: docker-compose.upstream-tls.yml and docker-compose.forward-proxy.yml to reproduce both scenarios.

Addition of proxy_ssl_protocols was somewhat straightforward. From proxy_ssl_protocols doc, the TLSv1.3 parameter is used by default since 1.23.4. Currently APIcast is using 1.19.3.

When a proxy is used, APIcast opens the (plain) connection to the proxy and sends CONNECT. Then APIcast starts the ssl_handshake using lua-resty-http library. The ssl_handshake docs leads to the tcpsock:sslhandshake doc, which in turn leads to lua-nginx-module lua_ssl_protocols

Verification steps for APIcast <> upstream TLS v1.3

  • run docker-compose.upstream-tls.yml env
make upstream-tls-gateway
  • Get APIcast IP
❯ docker ps
CONTAINER ID   IMAGE          COMMAND                  CREATED          STATUS          PORTS                NAMES
dd1336772d1e   apicast-test   "container-entrypoin…"   52 seconds ago   Up 51 seconds   8080/tcp, 8090/tcp   apicast_build_0-gateway-run-e12671d1fe0a
9891071e103c   nginx:1.23.4   "/docker-entrypoint.…"   52 seconds ago   Up 51 seconds   80/tcp, 443/tcp      apicast_build_0-one.upstream-1

❯ APICAST_IP=$(docker inspect apicast_build_0-gateway-run-e12671d1fe0a | yq e -P '.[0].NetworkSettings.Networks.apicast_build_0_default.IPAddress' -)
  • Send a request to APIcast
❯ curl -v -H "Host: one" http://${APICAST_IP}:8080/?user_key=foo
*   Trying 172.30.0.3:8080...
* Connected to 172.30.0.3 (172.30.0.3) port 8080 (#0)
> GET /?user_key=foo HTTP/1.1
> Host: one
> User-Agent: curl/7.81.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Server: openresty
< Date: Fri, 31 Mar 2023 13:25:32 GMT
< Content-Type: application/json; charset=utf-8
< Content-Length: 333
< Connection: keep-alive
< ETag: W/"14d-4XpxjQtexgDK6lLcaiW7RM635PE"
< set-cookie: sails.sid=s%3A8n7fKPlPROUPFmfb_PGaEqj2ABv9GCH0.TggZde%2FNU%2FZp935XWhVc7tm5cipaiJHL%2Fa%2B3bfxE0es; Path=/; HttpOnly
< 
{
  "args": {
    "user_key": "foo"
  },
  "headers": {
    "x-forwarded-proto": "https",
    "x-forwarded-port": "443",
    "host": "postman-echo.com",
    "x-amzn-trace-id": "Root=1-6426df4c-1baa11694682e34726752e8a",
    "user-agent": "curl/7.81.0",
    "accept": "*/*"
  },
  "url": "https://postman-echo.com/get/?user_key=foo"
* Connection #0 to host 172.30.0.3 left intact
}

The 200 Ok is enough to know that TLS v1.3 connection has been create between APIcast and upstream

Verification steps for APIcast <> HTTP proxy <> upstream TLS v1.3

  • run docker-compose.forward-proxy.yml env
make forward-proxy-gateway
  • Get APIcast IP
❯ docker ps
CONTAINER ID   IMAGE                   COMMAND                  CREATED          STATUS         PORTS                NAMES
b507dd5e3b51   apicast-test            "container-entrypoin…"   9 seconds ago    Up 9 seconds   8080/tcp, 8090/tcp   apicast_build_0-gateway-run-a32123385dce
7b3107db8aa4   nginx:1.23.4            "/docker-entrypoint.…"   10 seconds ago   Up 9 seconds   80/tcp, 443/tcp      apicast_build_0-upstream-1
f0b18fa16dda   apicast_build_0-proxy   "/usr/bin/tinyproxy …"   10 seconds ago   Up 9 seconds   0/tcp                apicast_build_0-proxy-1

❯ APICAST_IP=$(docker inspect apicast_build_0-gateway-run-a32123385dce | yq e -P '.[0].NetworkSettings.Networks.apicast_build_0_default.IPAddress' -)
  • Send a request to APIcast
❯ curl -v -H "Host: one" http://${APICAST_IP}:8080/?user_key=foo
*   Trying 172.31.0.4:8080...
* Connected to 172.31.0.4 (172.31.0.4) port 8080 (#0)
> GET /?user_key=foo HTTP/1.1
> Host: one
> User-Agent: curl/7.81.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: application/json; charset=utf-8
< Transfer-Encoding: chunked
< Connection: keep-alive
< Date: Fri, 31 Mar 2023 13:31:20 GMT
< ETag: W/"14d-J6jrY01zz6KIsAdbBP5a167xBus"
< Server: nginx/1.23.4
< set-cookie: sails.sid=s%3AQ1GARo5KiemVZwwYT2FU8u-pOn9Q8X4v.jsvztQbxM53hjHl3T5zdRdDmiEZaqLFTKt3YQzKYh14; Path=/; HttpOnly
< 
{
  "args": {
    "user_key": "foo"
  },
  "headers": {
    "x-forwarded-proto": "https",
    "x-forwarded-port": "443",
    "host": "postman-echo.com",
    "x-amzn-trace-id": "Root=1-6426e0a8-37e6d14240320fb47ce94d8d",
    "accept": "*/*",
    "user-agent": "curl/7.81.0"
  },
  "url": "https://postman-echo.com/get/?user_key=foo"
* Connection #0 to host 172.31.0.4 left intact
}

The 200 Ok is enough to know that TLS v1.3 connection has been create between APIcast and upstream through the proxy

@eguzki eguzki force-pushed the THREESCALE-9193-upstream-tlsv1.3 branch from d5e4a20 to a0b5dd0 Compare March 31, 2023 13:18
@eguzki eguzki marked this pull request as ready for review March 31, 2023 14:43
@eguzki eguzki requested a review from a team as a code owner March 31, 2023 14:43
Comment on lines 86 to 89
if not port then
port = default_port(uri)
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for being nosy. But wouldn't removing this block make the port on the next line nil and cause the http and https uri to use the same pool?

I think we should make it local instead, ie

local port = default_port(uri)

Above is just my observation, please correct me if I'm wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

be my guest and noisy anytime :) You are totally right.

Fixing it now

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

@eguzki eguzki force-pushed the THREESCALE-9193-upstream-tlsv1.3 branch from c280eb9 to a94c9ca Compare April 4, 2023 09:24
@eguzki eguzki requested review from thomasmaas and pehala April 4, 2023 09:25
if not port then
port = default_port(uri)
end
local port = default_port(uri)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this change made? It seems to break existing behaviour

Copy link
Member Author

Choose a reason for hiding this comment

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

no, it's not breaking existing behavior.

port was not declared local, hence it was accessing some (not existing) global variable. Nginx was rightfully complaining about that because it could be causing race conditions. It seems me that there used to be some local port declared and it was removed. 🤷

@@ -1,3 +1,4 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make it multi-doc?

Copy link
Member Author

Choose a reason for hiding this comment

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

This header does not make it multi-doc, it is just a document start marker that does not do any harm and pleases my IDE because I have a lint tool that requires document start markers. https://yamllint.readthedocs.io/en/stable/rules.html#module-yamllint.rules.document_start

- $(DOCKER_COMPOSE) down --volumes --remove-orphans
- $(DOCKER_COMPOSE) -f $(PROVE_DOCKER_COMPOSE_FILE) down --volumes --remove-orphans
- $(DOCKER_COMPOSE) -f $(DEVEL_DOCKER_COMPOSE_FILE) -f $(DEVEL_DOCKER_COMPOSE_VOLMOUNT_FILE) down --volumes --remove-orphans
$(DOCKER) compose down --volumes --remove-orphans
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this removes any potential compatibility with podman-compose (not sure if it was working beforehand), as it is a separate command and not a subcommand for podman.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this repo, we use docker, not podman. I do not think it works with podman, even before this change. Check this PR for a discussion regarding Podman vs Docker. I would be happy to move to podman/buildah. But it needs work and maybe it cannot be done.

This is about moving forward to docker compose V2. Docker compose V1 is deprecated https://www.docker.com/blog/new-docker-compose-v2-and-v1-deprecation/

@@ -106,10 +98,9 @@ executors:
docker:
working_directory: /opt/app-root/apicast
docker:
- image: docker:stable
- image: docker:23.0.2-cli-alpine3.17
Copy link
Member

Choose a reason for hiding this comment

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

as in, docker stable is not good enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

docker:stable does not implement docker V2 (there is not docker compose command)

GKmV2MkFbWsoDCf1Z6I9h877Dpi+ilT5aX20Fv/eMpfUXCXwkciV2Yk9gtDmVJdR
emAt/f7ldcVZ1dymTZIoKTVQCXexZliISq88ZwlmHbphF8LHC/0awC4XOvsDFHmg
iAa0ukVxqPWNmpGp4wuRoa+Tpg==
-----END PRIVATE KEY-----
Copy link
Member

Choose a reason for hiding this comment

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

We're adding a private key to repo? (I'm guessing it's for test purposes but want to make sure it's by accident)

Copy link
Member Author

Choose a reason for hiding this comment

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

it is a self generated cert key used to deploy testing env. Not shipped in the apicast image. It will expire in 10 years.

@eguzki eguzki force-pushed the THREESCALE-9193-upstream-tlsv1.3 branch from 47cf0dc to f2d8897 Compare April 11, 2023 13:55
@eguzki eguzki merged commit e851732 into master Apr 11, 2023
@eguzki eguzki deleted the THREESCALE-9193-upstream-tlsv1.3 branch April 11, 2023 14:41
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.

4 participants