From 9da01f8edb1c220f7b0cfd031a8b3dbe09c1c567 Mon Sep 17 00:00:00 2001 From: Matthew Peveler Date: Tue, 21 Nov 2023 10:39:12 -0500 Subject: [PATCH 1/2] Fix following 301 redirects for https upgrades --- .github/workflows/test.yml | 11 +---------- docker-compose.yml | 11 +++++++++++ index.spec.js | 32 ++++++++++++++++++++++++++++++++ lib/presto-client/index.js | 28 +++++++++++++--------------- nginx/nginx.conf | 31 +++++++++++++++++++++++++++++++ nginx/ssl/nginx-selfsigned.crt | 21 +++++++++++++++++++++ nginx/ssl/nginx-selfsigned.key | 28 ++++++++++++++++++++++++++++ 7 files changed, 137 insertions(+), 25 deletions(-) create mode 100644 nginx/nginx.conf create mode 100644 nginx/ssl/nginx-selfsigned.crt create mode 100644 nginx/ssl/nginx-selfsigned.key diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3a69224..6841d97 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -8,22 +8,13 @@ on: jobs: test: - services: - presto: - image: ghcr.io/popsql/prestodb-sandbox:0.284 - ports: - - "18080:8080" - trino: - image: trinodb/trino:418 - ports: - - "18081:8080" - runs-on: ubuntu-latest strategy: matrix: node-version: [ 16.x, 18.x, 20.x ] steps: - uses: actions/checkout@v3 + - run: docker compose up --detach --wait - uses: actions/setup-node@v3 with: node-version: ${{ matrix.node-version }} diff --git a/docker-compose.yml b/docker-compose.yml index 1e47f38..1beba77 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -11,3 +11,14 @@ services: ports: - "18081:8080" + nginx: + image: nginx:1.25-alpine + depends_on: + - presto + ports: + - "80:80" + - "443:443" + volumes: + - ./nginx/nginx.conf:/etc/nginx/nginx.conf:ro + - ./nginx/ssl/nginx-selfsigned.crt:/etc/nginx/ssl/nginx-selfsigned.crt:ro + - ./nginx/ssl/nginx-selfsigned.key:/etc/nginx/ssl/nginx-selfsigned.key:ro diff --git a/index.spec.js b/index.spec.js index 10daaf7..5813bb7 100644 --- a/index.spec.js +++ b/index.spec.js @@ -418,4 +418,36 @@ describe('redirect tests', function(){ }, }); }); + + describe('when using reverse proxy that does not forward protocol', function(){ + // The first request will go to https://localhost:443/v1/statement, but the + // nextUri will respond with http://localhost/v1/statement/... where then + // the nginx proxy will 301 redirect us to https again. + const client = new Client({ + host: 'localhost', + port: 443, + catalog: 'tpch', + schema: 'tiny', + ssl: { + rejectUnauthorized: false, + } + }); + + test('that querying works', function(done){ + expect.assertions(5); + client.execute({ + query: 'SELECT 1 AS col', + data: function(error, data, columns){ + expect(error).toBeNull(); + expect(data).toEqual([[1]]); + expect(columns).toHaveLength(1); + expect(columns[0]).toEqual(expect.objectContaining({ name: 'col', type: 'integer' })); + }, + callback: function(error){ + expect(error).toBeNull(); + done(); + }, + }); + }, 10000); + }); }); diff --git a/lib/presto-client/index.js b/lib/presto-client/index.js index 3cf4363..98f9636 100644 --- a/lib/presto-client/index.js +++ b/lib/presto-client/index.js @@ -1,15 +1,11 @@ const { URL } = require('url') ; +const http = require('follow-redirects/http'); +const https = require('follow-redirects/https'); -var adapterFor = (function() { - var adapters = { - 'http:': require('follow-redirects/http'), - 'https:': require('follow-redirects/https'), - }; - - return function(protocol) { - return adapters[protocol]; - }; -}()); +var adapters = { + 'http:': http, + 'https:': https, +}; var PrestoHeaders = require('./headers').Headers; var TrinoHeaders = require('./headers').TrinoHeaders; @@ -70,12 +66,12 @@ var Client = exports.Client = function(args){ Client.prototype.request = function(opts, callback) { var client = this; - var adapter = adapterFor(client.protocol); var contentBody = null; if (opts instanceof Object) { opts.host = client.host; opts.port = client.port; + opts.protocol = client.protocol; if (! opts.user) opts.user = client.user if (! opts.headers) @@ -90,14 +86,13 @@ Client.prototype.request = function(opts, callback) { port: href.port || (href.protocol === 'https:' ? '443' : '80'), path: href.pathname + href.search, headers: {}, + protocol: href.protocol, }; } catch (error) { return callback(error); } } - opts.protocol = client.protocol; - - opts = Object.assign({}, opts, client.ssl); + var adapter = adapters[opts.protocol]; if (opts.user) opts.headers[client.headers.USER] = opts.user; @@ -113,7 +108,10 @@ Client.prototype.request = function(opts, callback) { if (opts.body) contentBody = opts.body; - opts.agent = new adapter.Agent(opts); // Otherwise SSL params are ignored. + opts.agents = { + http: new http.Agent({ keepAlive: false }), + https: new https.Agent({ ...client.ssl, keepAlive: false }), + }; var parser = this.jsonParser; diff --git a/nginx/nginx.conf b/nginx/nginx.conf new file mode 100644 index 0000000..8bd94d1 --- /dev/null +++ b/nginx/nginx.conf @@ -0,0 +1,31 @@ +events { + worker_connections 1024; +} + +http { + server { + listen 80; + server_name _; + return 301 https://localhost:443$request_uri; + } + + server { + listen 443 ssl; + server_name _; + + ssl_certificate /etc/nginx/ssl/nginx-selfsigned.crt; + ssl_certificate_key /etc/nginx/ssl/nginx-selfsigned.key; + + location / { + proxy_pass http://presto:8080/; + proxy_connect_timeout 3; + + proxy_set_header Host $http_host; + proxy_set_header X-Real-IP $remote_addr; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + # don't forward the protocol, so that presto will default to responding with + # http:// URLs, to test the 301 redirect logic. + # proxy_set_header X-Forwarded-Proto $scheme; + } + } +} diff --git a/nginx/ssl/nginx-selfsigned.crt b/nginx/ssl/nginx-selfsigned.crt new file mode 100644 index 0000000..d855179 --- /dev/null +++ b/nginx/ssl/nginx-selfsigned.crt @@ -0,0 +1,21 @@ +-----BEGIN CERTIFICATE----- +MIIDbTCCAlWgAwIBAgIUeYV6hie38aYPRLqSqkgUdB1efQwwDQYJKoZIhvcNAQEL +BQAwRTELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoM +GEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDAgFw0yMzExMjExNzM2MDlaGA8zMDIz +MDMyNDE3MzYwOVowRTELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUx +ITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDCCASIwDQYJKoZIhvcN +AQEBBQADggEPADCCAQoCggEBAJbNHsmo/jSyNKdSaXlIn9ZCgjTN2wp8yH5Kcwx4 +RHwx8ZVPQIVz9aSnmmjwKZfpLtvNU7jrgBH8h4qxTw/bbETluTeH9asA71cliZyz +qZUVEXeQqXRlzDDafElWRyEQT+X9vTp7ApKfFRC6p64sZFeXgQtNkhfxWLrVuxTo +WoebROjHXo78M27i76InaorNDDgYoHByEeWqPcOrATT9v3e1UbbD8zrL28Qh23+R +hddefeB7CbwyIeGOEh6jde0lHEVkrv1u6jbFuvpalrtETyPKcT5wi056YjKgbA4m +2EULLwKhwki3/QMYEjLO/h9vnAiiWvemC+8U6aZGyGvS/00CAwEAAaNTMFEwHQYD +VR0OBBYEFOUr3sfsSTx293Ab72TMajKL1+xPMB8GA1UdIwQYMBaAFOUr3sfsSTx2 +93Ab72TMajKL1+xPMA8GA1UdEwEB/wQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEB +AA8we9GzV6j6jdvAyfnkRjdVB//L5VQzIbpCHQC8sZnLnJt0mzhdIcl+KnIq8v9x +AlUHJxMme2Xb22z15SBJWm2blg4M7izzKfsI27DjmnEFAaRSBFoLyqlyW9MJhS42 +pWT8ltVzTnPwN/VMphrBOCZ7i5tNdzy4NcExvPuMVx3qrB5H0+Hoofjqo+pAe01+ +yG6pdQyBOf4/MiePl5QR3E6vriMU0ddRaVnGot3wPw9sIWxjSVTGNTXSvo/PVyO/ +XV1IaXaw8srYgH78pYkAa27QUIsx6m9EIdyE8yK3nXQHswYdNxP720p5GINZYKM9 +37hfkpczTTBdK6jdHpFexYE= +-----END CERTIFICATE----- diff --git a/nginx/ssl/nginx-selfsigned.key b/nginx/ssl/nginx-selfsigned.key new file mode 100644 index 0000000..57324b2 --- /dev/null +++ b/nginx/ssl/nginx-selfsigned.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCWzR7JqP40sjSn +Uml5SJ/WQoI0zdsKfMh+SnMMeER8MfGVT0CFc/Wkp5po8CmX6S7bzVO464AR/IeK +sU8P22xE5bk3h/WrAO9XJYmcs6mVFRF3kKl0Zcww2nxJVkchEE/l/b06ewKSnxUQ +uqeuLGRXl4ELTZIX8Vi61bsU6FqHm0Tox16O/DNu4u+iJ2qKzQw4GKBwchHlqj3D +qwE0/b93tVG2w/M6y9vEIdt/kYXXXn3gewm8MiHhjhIeo3XtJRxFZK79buo2xbr6 +Wpa7RE8jynE+cItOemIyoGwOJthFCy8CocJIt/0DGBIyzv4fb5wIolr3pgvvFOmm +Rshr0v9NAgMBAAECggEAEaO4wDoGUj+uLQxUzh0SpUtuU6LoxldGVI982a6PxD9L +VP3KPFIOH86DH1dIZj6efpOMAYt6laAGctC+wMoX5g9BFR/QOsqHNJhtemkBozCt +tGC1kan+spA8DZAMDfAMiIifw+FzsZbuLeDHkHYc3qoYLCxMtIRErsYldhKf1FDF +S1eQ2Oam3deHDRQy/NTwwTrC322CLgB5XO63Ustdc30rfulpX1WvxUz9udmHxZZV +32gbUv54hr6/AzN6BLPUUvzO5VZeizssM9RR97oBH/fALbiOnCCf7UyEB19wX4ev +dmQLXr/KpRvYAzKYg7UAmt2Ut4Cm0V+TqQgvNivxQQKBgQDIHbObobdNyuEi4aie +tSJX5pEG30+zHdtTA0XhkXG+7l223xVM+PTfPSRxMgGIef14hfA1yS1JslNf5vpb +l/HJ7Bz65I5ENwi9LjxILSjDpJ2R95eoBFa0AqzdpkUkDHWb3qWhvsE624JXXHlU +6t2Lk3Rq/b8cny8DMEFwBJHFxQKBgQDA6eebo8B2ny9i8FdBWQrCiepPSrn+OIZ5 +B9shUy1r51wL+rdBctO+YZAdd91Gy3FV16M/gjhatxnpQLo6DSQPRBICb2SgVdaG +AyJ0WxUJulIRhO+VubObjk8DFsBikr8mm1osVX8G6qhTjC3dW2QJYfAsDLDTU7Ea +VdWERXzz6QKBgQCT/js85pzcQCS9misMrCJo5U/tyCp16aewvaPpjJmVPU07F8H1 ++cuGaP5RZEzz9Fu6zTr3W/9NGD9Gllgicr1SunY4Kdz4n8aruczFB/i0r0IEmBml +HQhN+giDpxpM7ZXwnvjZJGxcrce3+eCVJ1iOh486LMwwS50F+6L5R7fSjQKBgBj/ +9vjPfsiglnZ+6P/Z9zAAyXGfIH1We+7BWt91tQQvxljzE76Sj+gzIob/GpjrEnPq +bwhy6rSu19fHgJq3Sz4DN0ZDg0nX9eYGD/f+Obq5/5qvnJDNsP3uskSXNTVRCR6K +sWPfbIfL7yZsmyD9j/g7TfGXb27jgKcGCpaKnsfJAoGAATLeEAD8rHMyxmBmYdcO +GUxLaqOtODEje/8zOn5hUFbISUEk4MKw0kT3XHtIFqhxIFJrV0Au4urMNuauneur +OEyVN7Y9RreGgzt0rPvpFvyv2kTBbhihd0lJ86UrhUDsllNphpb43UPurZM/kp0h +djFSBC+GgWwuWFqsRntRhFg= +-----END PRIVATE KEY----- From bcd7789c50a823291441821818b3b41ea63e433c Mon Sep 17 00:00:00 2001 From: Matthew Peveler Date: Tue, 28 Nov 2023 13:21:31 -0500 Subject: [PATCH 2/2] add comment --- lib/presto-client/index.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/presto-client/index.js b/lib/presto-client/index.js index 98f9636..e89e5cc 100644 --- a/lib/presto-client/index.js +++ b/lib/presto-client/index.js @@ -108,6 +108,11 @@ Client.prototype.request = function(opts, callback) { if (opts.body) contentBody = opts.body; + // `agents` is a custom property that follow-redirects supports, where + // it's similar to setting `agent`, but allows for setting it per protocol, + // so that if we redirect from http -> https (or vice versa) we use the right + // agent, instead of trying to reuse http.Agent for https (or vice versa) which + // will error. opts.agents = { http: new http.Agent({ keepAlive: false }), https: new https.Agent({ ...client.ssl, keepAlive: false }),