feat(api, sentinal): adds servername and proxy origin switch for sni …

[fardarter]

Description

Have designed a unified wrapper for request-promise-native to centralise global options and to add a servername by default. It's overridable in the case that you ever want to have a different servername or an empty one.

When proxying to HTTPS from HTTP (for example where an ingress does TLS termination), not including a 'servername' for a request to the HTTPS server (eg, def.org) the request produces the following error (where abc.com is the calling server):

'ERR_TLS_CERT_ALTNAME_INVALID'
"RequestError: Error [ERR_TLS_CERT_ALTNAME_INVALID]: Hostname/IP does not match certificate's altnames:
Host: abc.com. is not in the cert's altnames: DNS:def.org"

The addition of servername as an option passed to the HTTP agent resolves this error.

See docs for tls.connect(options[, callback]) (https://nodejs.org/api/tls.html): "Server name for the SNI (Server Name Indication) TLS extension. It is the name of the host being connected to, and must be a host name, and not an IP address.".

Have written unit tests and have run other tests with the changes included. Relying on your CI for E2E and other tests.

Note: Have not changed the use of request-promise-native outside of calls to Couch.

• [x] Readable: Concise, well named, follows the style guide, documented if necessary.
• [x] Tested: Unit and/or e2e where appropriate
• [x] Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

[garethbowen]

At Medic we're using ELB for TLS termination and haven't hit this issue, possibly because we bypass nginx entirely. Is this an option for you?

[fardarter]

Hi @garethbowen

The point of breakdown here is between node and haproxy, so I don't think the nginx instance I'm deploying is at issue.

The Azure equivalent of ELB is not an option for termination for a few reasons:

• We're going to be offboarding our data through a Databricks instance so HTTPS to the haproxy instance is critical.
• Deploying into a much larger network with a lot of access via VPN, so HTTPS as much as possible is useful as defense in depth strategy -- I'm more worried about attacks from inside the network than at my ingress.
• Deploying sentinel, the api, an nginx (+modsec) proxy and a config container for cht-conf (using container jobs for being able to run the conf in network on changes) into an Azure Container App (managed kubernetes) environment. It has a lot of really nice benefits, but it does (as is a good idea, tbh) wrap each container in an HTTPS ingress termination. So while the haproxy instance isn't running in this env, eventually if we want to add sidecars this will be an issue.

I'm not sure what's causing the e2e test failures. The changes work well in manual testing and it's unclear why nginx would be returning a bad gateway error on these changes. If you have any insight, please let me know. Otherwise I'll get back to the tests when I'm able -- aside from other workload, my environment isn't well configured for them at the moment (WSL) and I'll need to set up in a different space.

That said, I can't see us not needing this patch, so I'd really like to solve it for upstream so we don't have to maintain the differences.

As an aside, this change would also make swapping out request-promise-native easier later.

[garethbowen]

Ok thanks for the explanation - this makes sense. Could you please raise an issue which explains all of this? Issues are useful for tracking purposes and making sure it gets documented in the release notes and so on.

I'll try and find someone to help review this... we're all busy on other things right now.

In the meantime if you could have a look at fixing the e2e tests? In GH Actions you can download the artefact which is the logs from the run. In the API log I see...

Info: Starting CHT API
node:internal/modules/cjs/loader:1031
  throw err;
  ^

Error: Cannot find module 'request-promise-native'
Require stack:
- /service/shared-libs/couch-request/src/couch-request.js
- /service/shared-libs/server-checks/src/checks.js
- /service/api/server.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:1028:15)
    at Function.Module._load (node:internal/modules/cjs/loader:873:27)
    at Module.require (node:internal/modules/cjs/loader:1100:19)
    at require (node:internal/modules/cjs/helpers:108:18)
    at Object.<anonymous> (/service/shared-libs/couch-request/src/couch-request.js:1:17)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1252:10)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at Module.require (node:internal/modules/cjs/loader:1100:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/service/shared-libs/couch-request/src/couch-request.js',
    '/service/shared-libs/server-checks/src/checks.js',
    '/service/api/server.js'
  ]
}

This is probably due to request-promise-native not being copied into the docker image at compile which is due to the way npm workspaces installs dependencies. The easy fix is to include request-promise-native in the api/package.json.

[fardarter]

Ok thanks for the explanation - this makes sense. Could you please raise an issue which explains all of this? Issues are useful for tracking purposes and making sure it gets documented in the release notes and so on.

I'll try and find someone to help review this... we're all busy on other things right now.

In the meantime if you could have a look at fixing the e2e tests? In GH Actions you can download the artefact which is the logs from the run. In the API log I see...

Info: Starting CHT API
node:internal/modules/cjs/loader:1031
  throw err;
  ^

Error: Cannot find module 'request-promise-native'
Require stack:
- /service/shared-libs/couch-request/src/couch-request.js
- /service/shared-libs/server-checks/src/checks.js
- /service/api/server.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:1028:15)
    at Function.Module._load (node:internal/modules/cjs/loader:873:27)
    at Module.require (node:internal/modules/cjs/loader:1100:19)
    at require (node:internal/modules/cjs/helpers:108:18)
    at Object.<anonymous> (/service/shared-libs/couch-request/src/couch-request.js:1:17)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1252:10)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at Module.require (node:internal/modules/cjs/loader:1100:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/service/shared-libs/couch-request/src/couch-request.js',
    '/service/shared-libs/server-checks/src/checks.js',
    '/service/api/server.js'
  ]
}

This is probably due to request-promise-native not being copied into the docker image at compile which is due to the way npm workspaces installs dependencies. The easy fix is to include request-promise-native in the api/package.json.

Thanks, I'll try this and push later.

I'll make an issue as soon as I have some breathing space.

I'm not in a rush with this. I have a patching strategy that keeps our code very well separated so I'm not in a rush (and interestingly I'm not messing with package.json because I've not solved and easy merge for that yet, so there's a clue there too).

Just we're trying to stay on an easy upgrade path so I wanted to get this visible soon.

[fardarter]

@garethbowen Issue: https://github.com/medic/cht-core/issues/8591