From e6ebfbaea124ee0a8e08dc91054df1778327fd73 Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Mon, 18 Dec 2023 09:44:31 +0530 Subject: [PATCH] fix: Remove Server header and allow all on port 80 (#29585) Another attempt at #29550, which was reverted. Fallback is not happening if cert provisioning fails _despite_ having the correct header. But with the changes in this PR, since we'll listen on `:80`, fallback _will_ happen when cert provisioning fails due to incorrect domain configuration. We're also adding [Hurl](https://hurl.dev) based tests. They're not run in any CI yet. That'll come in soon. --- .../fs/opt/appsmith/caddy-reconfigure.mjs | 38 ++++++--- deploy/docker/route-tests/Dockerfile | 9 ++ .../common-https/forwarded-headers.hurl | 33 ++++++++ .../route-tests/common/forwarded-headers.hurl | 33 ++++++++ .../common/index-html-response.hurl | 41 +++++++++ .../docker/route-tests/common/static-404.hurl | 14 ++++ deploy/docker/route-tests/echo.caddyfile | 12 +++ deploy/docker/route-tests/entrypoint.sh | 83 +++++++++++++++++++ deploy/docker/route-tests/run.sh | 16 ++++ .../route-tests/spec-2/custom-domain.hurl | 27 ++++++ .../route-tests/spec-3/custom-domain.hurl | 29 +++++++ 11 files changed, 324 insertions(+), 11 deletions(-) create mode 100644 deploy/docker/route-tests/Dockerfile create mode 100644 deploy/docker/route-tests/common-https/forwarded-headers.hurl create mode 100644 deploy/docker/route-tests/common/forwarded-headers.hurl create mode 100644 deploy/docker/route-tests/common/index-html-response.hurl create mode 100644 deploy/docker/route-tests/common/static-404.hurl create mode 100644 deploy/docker/route-tests/echo.caddyfile create mode 100644 deploy/docker/route-tests/entrypoint.sh create mode 100755 deploy/docker/route-tests/run.sh create mode 100644 deploy/docker/route-tests/spec-2/custom-domain.hurl create mode 100644 deploy/docker/route-tests/spec-3/custom-domain.hurl diff --git a/deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs b/deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs index 02d7c121aa..33f2892cb6 100644 --- a/deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs +++ b/deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs @@ -3,17 +3,20 @@ import {dirname} from "path" import {spawnSync} from "child_process" import {X509Certificate} from "crypto" -const APPSMITH_CUSTOM_DOMAIN = process.env.APPSMITH_CUSTOM_DOMAIN ?? null +// The custom domain is expected to only have the domain. So if it has a protocol, we ignore the whole value. +// This was the effective behaviour before Caddy. +const CUSTOM_DOMAIN = (process.env.APPSMITH_CUSTOM_DOMAIN || "").replace(/^https?:\/\/.+$/, "") + const CaddyfilePath = process.env.TMP + "/Caddyfile" let certLocation = null -if (APPSMITH_CUSTOM_DOMAIN != null) { +if (CUSTOM_DOMAIN !== "") { try { fs.accessSync("/appsmith-stacks/ssl/fullchain.pem", fs.constants.R_OK) certLocation = "/appsmith-stacks/ssl" - } catch (_) { + } catch { // no custom certs, see if old certbot certs are there. - const letsEncryptCertLocation = "/etc/letsencrypt/live/" + APPSMITH_CUSTOM_DOMAIN + const letsEncryptCertLocation = "/etc/letsencrypt/live/" + CUSTOM_DOMAIN const fullChainPath = letsEncryptCertLocation + `/fullchain.pem` try { fs.accessSync(fullChainPath, fs.constants.R_OK) @@ -21,7 +24,7 @@ if (APPSMITH_CUSTOM_DOMAIN != null) { if (!isCertExpired(fullChainPath)) { certLocation = letsEncryptCertLocation } - } catch (_) { + } catch { // no certs there either, ignore. } } @@ -114,19 +117,32 @@ parts.push(` handle_errors { respond "{err.status_code} {err.status_text}" {err.status_code} + header -Server } } -localhost:80 127.0.0.1:80 { +# We bind to http on 80, so that localhost requests don't get redirected to https. +:80 { import all-config } - -${APPSMITH_CUSTOM_DOMAIN || ":80"} { - import all-config - ${tlsConfig} -} `) +if (CUSTOM_DOMAIN !== "") { + // If no custom domain, no extra routing needed. + // We have to own the http-to-https redirect, since we need to remove the `Server` header from the response. + parts.push(` + https://${CUSTOM_DOMAIN} { + import all-config + ${tlsConfig} + } + http://${CUSTOM_DOMAIN} { + redir https://{host}{uri} + header -Server + header Connection close + } + `) +} + fs.mkdirSync(dirname(CaddyfilePath), { recursive: true }) fs.writeFileSync(CaddyfilePath, parts.join("\n")) spawnSync("/opt/caddy/caddy", ["fmt", "--overwrite", CaddyfilePath]) diff --git a/deploy/docker/route-tests/Dockerfile b/deploy/docker/route-tests/Dockerfile new file mode 100644 index 0000000000..075747550b --- /dev/null +++ b/deploy/docker/route-tests/Dockerfile @@ -0,0 +1,9 @@ +FROM node:lts-alpine + +RUN apk add --no-cache bash caddy \ + && apk add --no-cache hurl mkcert --repository=http://dl-cdn.alpinelinux.org/alpine/edge/testing/ \ + && mkcert -install + +WORKDIR /code + +ENTRYPOINT [ "bash", "entrypoint.sh" ] diff --git a/deploy/docker/route-tests/common-https/forwarded-headers.hurl b/deploy/docker/route-tests/common-https/forwarded-headers.hurl new file mode 100644 index 0000000000..8a1069918b --- /dev/null +++ b/deploy/docker/route-tests/common-https/forwarded-headers.hurl @@ -0,0 +1,33 @@ +GET https://custom-domain.com/oauth2/google/authorize +HTTP 200 +``` +Scheme = 'http' +X-Forwarded-Proto = 'https' +Host = 'custom-domain.com' +X-Forwarded-Host = 'custom-domain.com' +Forwarded = '' +``` + +# This is the CloudRun test. That the `Forwarded` header should be removed when proxying to upstream. +GET https://custom-domain.com/oauth2/google/authorize +Forwarded: something something +HTTP 200 +``` +Scheme = 'http' +X-Forwarded-Proto = 'https' +Host = 'custom-domain.com' +X-Forwarded-Host = 'custom-domain.com' +Forwarded = '' +``` + +GET https://custom-domain.com/oauth2/google/authorize +X-Forwarded-Proto: http +X-Forwarded-Host: overridden.com +HTTP 200 +``` +Scheme = 'http' +X-Forwarded-Proto = 'http' +Host = 'custom-domain.com' +X-Forwarded-Host = 'overridden.com' +Forwarded = '' +``` diff --git a/deploy/docker/route-tests/common/forwarded-headers.hurl b/deploy/docker/route-tests/common/forwarded-headers.hurl new file mode 100644 index 0000000000..21ba9091f8 --- /dev/null +++ b/deploy/docker/route-tests/common/forwarded-headers.hurl @@ -0,0 +1,33 @@ +GET http://local.com/oauth2/google/authorize +HTTP 200 +``` +Scheme = 'http' +X-Forwarded-Proto = 'http' +Host = 'local.com' +X-Forwarded-Host = 'local.com' +Forwarded = '' +``` + +# This is the CloudRun test. That the `Forwarded` header should be removed when proxying to upstream. +GET http://local.com/oauth2/google/authorize +Forwarded: something something +HTTP 200 +``` +Scheme = 'http' +X-Forwarded-Proto = 'http' +Host = 'local.com' +X-Forwarded-Host = 'local.com' +Forwarded = '' +``` + +GET http://local.com/oauth2/google/authorize +X-Forwarded-Proto: https +X-Forwarded-Host: overridden.com +HTTP 200 +``` +Scheme = 'http' +X-Forwarded-Proto = 'https' +Host = 'local.com' +X-Forwarded-Host = 'overridden.com' +Forwarded = '' +``` diff --git a/deploy/docker/route-tests/common/index-html-response.hurl b/deploy/docker/route-tests/common/index-html-response.hurl new file mode 100644 index 0000000000..068f55e6d8 --- /dev/null +++ b/deploy/docker/route-tests/common/index-html-response.hurl @@ -0,0 +1,41 @@ +GET http://localhost +HTTP 200 +Content-Type: text/html; charset=utf-8 +[Asserts] +header "Server" not exists +body == "index.html body" + +GET http://127.0.0.1 +HTTP 200 +Content-Type: text/html; charset=utf-8 +[Asserts] +header "Server" not exists +body == "index.html body" + +GET http://local.com +HTTP 200 +Content-Type: text/html; charset=utf-8 +[Asserts] +header "Server" not exists +body == "index.html body" + +GET http://localhost/some/non/handled/path +HTTP 200 +Content-Type: text/html; charset=utf-8 +[Asserts] +header "Server" not exists +body == "index.html body" + +GET http://127.0.0.1/some/non/handled/path +HTTP 200 +Content-Type: text/html; charset=utf-8 +[Asserts] +header "Server" not exists +body == "index.html body" + +GET http://local.com/some/non/handled/path +HTTP 200 +Content-Type: text/html; charset=utf-8 +[Asserts] +header "Server" not exists +body == "index.html body" diff --git a/deploy/docker/route-tests/common/static-404.hurl b/deploy/docker/route-tests/common/static-404.hurl new file mode 100644 index 0000000000..e995243267 --- /dev/null +++ b/deploy/docker/route-tests/common/static-404.hurl @@ -0,0 +1,14 @@ +GET http://localhost/static/missing +HTTP 404 +[Asserts] +header "Server" not exists + +GET http://127.0.0.1/static/missing +HTTP 404 +[Asserts] +header "Server" not exists + +GET http://local.com/static/missing +HTTP 404 +[Asserts] +header "Server" not exists diff --git a/deploy/docker/route-tests/echo.caddyfile b/deploy/docker/route-tests/echo.caddyfile new file mode 100644 index 0000000000..45ff0dc875 --- /dev/null +++ b/deploy/docker/route-tests/echo.caddyfile @@ -0,0 +1,12 @@ +{ + admin off +} + +http://:5050 + +respond "Scheme = '{scheme}' +X-Forwarded-Proto = '{header.x-forwarded-proto}' +Host = '{host}' +X-Forwarded-Host = '{header.x-forwarded-host}' +Forwarded = '{header.forwarded}' +" diff --git a/deploy/docker/route-tests/entrypoint.sh b/deploy/docker/route-tests/entrypoint.sh new file mode 100644 index 0000000000..916a061448 --- /dev/null +++ b/deploy/docker/route-tests/entrypoint.sh @@ -0,0 +1,83 @@ +#!/usr/bin/env bash + +set -o errexit +set -o nounset +set -o pipefail +#set -o xtrace + +new-spec() { + echo "-----------" "$@" "-----------" + unset APPSMITH_CUSTOM_DOMAIN + mkdir -p /appsmith-stacks/ssl + find /appsmith-stacks/ssl -type f -delete +} + +reload-caddy() { + sed -i 's/127.0.0.1:{args\[0]}/127.0.0.1:5050/' "$TMP/Caddyfile" + caddy fmt --overwrite "$TMP/Caddyfile" + caddy reload --config "$TMP/Caddyfile" + sleep 1 +} + +run-hurl() { + hurl --test \ + --resolve local.com:80:127.0.0.1 \ + --resolve custom-domain.com:80:127.0.0.1 \ + --resolve custom-domain.com:443:127.0.0.1 \ + "$@" +} + +if [[ "${OPEN_SHELL-}" == 1 ]]; then + # Open shell for debugging after this script is done. + trap bash EXIT +fi + +echo +echo "caddy version: $(caddy --version)" +echo "hurl version: $(hurl --version)" +echo "mkcert version: $(mkcert --version)" +echo + +export TMP=/tmp/appsmith +export WWW_PATH="$TMP/www" + +mkdir -p "$WWW_PATH" +echo -n 'index.html body' > "$WWW_PATH/index.html" +mkcert -install + +# Start echo server +( + export XDG_DATA_HOME="$TMP/echo-data" + export XDG_CONFIG_HOME="$TMP/echo-conf" + mkdir -p "$XDG_DATA_HOME" "$XDG_CONFIG_HOME" + caddy start --config echo.caddyfile --adapter caddyfile >> "$TMP/echo-caddy.log" 2>&1 +) + +# Start Caddy for use with our config to test +echo localhost > "$TMP/Caddyfile" +caddy start --config "$TMP/Caddyfile" >> "$TMP/caddy.log" 2>&1 + +sleep 1 + + +new-spec "Spec 1: With no custom domain" +node /caddy-reconfigure.mjs +reload-caddy +run-hurl common/*.hurl + + +new-spec "Spec 2: With a custom domain, cert obtained (because of internal CA)" +export APPSMITH_CUSTOM_DOMAIN=custom-domain.com +node /caddy-reconfigure.mjs +#sed -i '2i acme_ca https://acme-staging-v02.api.letsencrypt.org/directory' "$TMP/Caddyfile" +sed -i '/https:\/\/'"$APPSMITH_CUSTOM_DOMAIN"' {$/a tls internal' "$TMP/Caddyfile" +reload-caddy +run-hurl common/*.hurl common-https/*.hurl spec-2/*.hurl + + +new-spec "Spec 3: With a custom domain, certs given in ssl folder" +export APPSMITH_CUSTOM_DOMAIN=custom-domain.com +mkcert -cert-file "/appsmith-stacks/ssl/fullchain.pem" -key-file "/appsmith-stacks/ssl/privkey.pem" "$APPSMITH_CUSTOM_DOMAIN" +node /caddy-reconfigure.mjs +reload-caddy +run-hurl common/*.hurl spec-3/*.hurl diff --git a/deploy/docker/route-tests/run.sh b/deploy/docker/route-tests/run.sh new file mode 100755 index 0000000000..85abf91847 --- /dev/null +++ b/deploy/docker/route-tests/run.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash + +set -o errexit +set -o nounset + +loc="$(dirname "$0")" +docker build -f "$loc/Dockerfile" --tag ar "$loc/.." +docker run \ + --name ar \ + --rm \ + -it \ + --hostname ar \ + -e OPEN_SHELL=${OPEN_SHELL-} \ + --volume "$loc/../fs/opt/appsmith/caddy-reconfigure.mjs:/caddy-reconfigure.mjs:ro" \ + --volume "$loc:/code:ro" \ + ar diff --git a/deploy/docker/route-tests/spec-2/custom-domain.hurl b/deploy/docker/route-tests/spec-2/custom-domain.hurl new file mode 100644 index 0000000000..c48772343f --- /dev/null +++ b/deploy/docker/route-tests/spec-2/custom-domain.hurl @@ -0,0 +1,27 @@ +GET http://custom-domain.com +HTTP 302 +Location: https://custom-domain.com/ +[Asserts] +header "Server" not exists + +GET http://custom-domain.com/random/path +HTTP 302 +Location: https://custom-domain.com/random/path +[Asserts] +header "Server" not exists + +GET https://custom-domain.com +HTTP 200 +[Asserts] +header "Server" not exists +certificate "Issuer" == "CN = Caddy Local Authority - ECC Intermediate" + +GET https://custom-domain.com/random/path +HTTP 200 +[Asserts] +header "Server" not exists + +GET https://custom-domain.com/static/x +HTTP 404 +[Asserts] +header "Server" not exists diff --git a/deploy/docker/route-tests/spec-3/custom-domain.hurl b/deploy/docker/route-tests/spec-3/custom-domain.hurl new file mode 100644 index 0000000000..a1a391bef9 --- /dev/null +++ b/deploy/docker/route-tests/spec-3/custom-domain.hurl @@ -0,0 +1,29 @@ +GET http://custom-domain.com +HTTP 302 +Location: https://custom-domain.com/ +Connection: close +[Asserts] +header "Server" not exists + +GET http://custom-domain.com/random/path +HTTP 302 +Location: https://custom-domain.com/random/path +Connection: close +[Asserts] +header "Server" not exists + +GET https://custom-domain.com +HTTP 200 +[Asserts] +header "Server" not exists +certificate "Issuer" == "O = mkcert development CA, OU = root@ar, CN = mkcert root@ar" + +GET https://custom-domain.com/random/path +HTTP 200 +[Asserts] +header "Server" not exists + +GET https://custom-domain.com/static/x +HTTP 404 +[Asserts] +header "Server" not exists