Skip to content

Conversation

yanokwa
Copy link
Member

@yanokwa yanokwa commented Sep 17, 2025

Currently, Central fails SSL Labs' test with

Assessment failed: Internal Server Error

People (including us) refer to this test to show that our configs are secure. The problem seems to be that the dummy cert we use is too dumb.

What has been done to verify that this works as intended?

ChatGPT told me we need X.509 v3, a subjectAltName and be reasonably valid to work with SSL scanners, so I made those changes and confirmed that everything works.

Screenshot 2025-09-16 at 18 58 40

The lack of SNI means we don't support these old browsers: Android 2.3.7, IE 6 / XP, IE 8 / XP, Java 6u45.

Why is this the best possible solution? Were any other approaches considered?

We could get rid of the dummy block that was added at #814 and make the real vhost the default. Not 100% sure why that was strictly necessary to begin with.

We could also add do something like this...

server {
  listen 443 ssl default_server;
  server_name ${CERT_DOMAIN} _;

  ssl_certificate         /etc/${SSL_TYPE}/live/${CERT_DOMAIN}/fullchain.pem;
  ssl_certificate_key     /etc/${SSL_TYPE}/live/${CERT_DOMAIN}/privkey.pem;
  ssl_trusted_certificate /etc/${SSL_TYPE}/live/${CERT_DOMAIN}/fullchain.pem;

  ssl_protocols TLSv1.2 TLSv1.3;
  ssl_prefer_server_ciphers off;
  ssl_session_tickets off;

  # 🔒 Hard stop: only serve the real host
  if ($host != '${CERT_DOMAIN}') { return 421; }

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It doesn't

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@yanokwa yanokwa requested a review from alxndrsn September 17, 2025 02:16
@brontolosone
Copy link
Contributor

We could get rid of the dummy block that was added at #814 and make the real vhost the default. Not 100% sure why that was strictly necessary to begin with.

I'm not sure I have the full context, but in case it might help: the idea might be to filter out misdirected requests. It's indeed not strictly necessary if you don't mind any and all requests, with any and all Host: headers, including those by bots, including those for strange domains (clients arriving here by way of DNS records of any number of previous owners of that VPS's IP still pointing at it) to end up in access logs / error logs and (depending on the shape of the request) even hitting Node (which is way more expensive than having it be stopped at the gate, eg by Nginx).

@matthew-white matthew-white changed the base branch from master to next October 3, 2025 17:32
@matthew-white
Copy link
Member

Since this PR changes code, I think we should merge into next rather than master. I just updated the target branch.

I'm also going to add this PR to the project just to keep track of it in the lead-up to releasing v2025.3.

@github-project-automation github-project-automation bot moved this to 🕒 backlog in ODK Central Oct 3, 2025
@matthew-white matthew-white moved this from 🕒 backlog to ✏️ in progress in ODK Central Oct 3, 2025
@matthew-white
Copy link
Member

@yanokwa, it'd be great if this PR could be merged in the next week in order to go out with the v2025.3 release. I see that you tagged @alxndrsn for review, but I bet @brontolosone or Sadiq could review as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✏️ in progress

Development

Successfully merging this pull request may close these issues.

3 participants