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

Upgrade to nginx 1.23.3 #397

Merged
merged 8 commits into from
Feb 28, 2023
Merged

Upgrade to nginx 1.23.3 #397

merged 8 commits into from
Feb 28, 2023

Conversation

yanokwa
Copy link
Member

@yanokwa yanokwa commented Feb 28, 2023

Replaces #391

docker-compose.yml

  • Adding letsencrypt volume so rebuilds don't throw away certs and risk hitting LetsEncrypt limits.
  • Upgrade requires removal of old container or mount won't work. Steps: docker-compose stop && docker-compose rm nginx && docker-compose build && docker-compose stop && docker-compose up -d
  • I'm also persist dh, selfsign, customssl to /etc/odk/nginx/ to save time and keep that in a named volume rather than leave random volumes lying about.

odk-setup.sh

  • One subtle change here is that if you don't have a valid ssl_type, nginx won't start. It will print the "writing fresh nginx templates" forever. I tried changing docker-compose.yml's restart from restart: always to restart: on-failure:3 but it'd print the log message once, then stop the container entirely, hiding it from docker ps. I don't love that because you don't see that something has gone wrong.

inflate_body.lua

  • The lua infrastructure that the script relies on aren’t easy to get in this current build of nginx.
  • Also means we can get rid of default file since it was added with nginx-extras was added
  • Deflating compressed request bodies is not usually supported in web servers due to possibility of zip bombs.
  • This feature is used by front-end to zip up CSV attachments to forms

redirector/odk.conf

  • I store a local copy because we rewrite it when changing ssl types in start-odk.sh. Copying the file each time ensures we have known good copy. I don't want to trust that the image or some script has changed that file.
  • It's probably safe to keep lines 7-14, but I delete then to guarantee certbot's response protocol isn't triggered.

certbot.conf

  • Was removed in nginx-certbot v4 in favor of putting everything in redirector.conf, so I'm removing it here too.

openssl

  • nginx already has openssl. Installing it here forces an upgrade. Said upgrade could be dangerous because that update hasn't been tested with our known copy of nginx.

netcat

  • There's now only netcat-traditional and netcat-openbsd. The latter has the -z flag we need and is up-to-date. netcat-traditional is unmaintained.

You might be wondering why we don't use docker-nginx-certbot's local CA. I've explored integrating that, but since we already have the customssl process, it's easier for us generate a quick cert.

Tested upgrade...

  • Start with working letsencrypt
  • Checkout out this branch, build, and upgrade
    • docker-compose build && docker-compose stop && docker-compose rm -f nginx && docker-compose up -d
    • If you don't rm nginx, build works fine, but you'll get this error on up. WARNING: Service "nginx" is using volume "/etc/letsencrypt" from the previous container. Host mapping "central_letsencrypt" has no effect.
  • Switch to self-signed, and confirm working
  • Switch to letsencrypt and confirm working and no letsencrypt renewal needed

@yanokwa yanokwa force-pushed the update-nginx branch 3 times, most recently from 61c6c92 to 1c183d8 Compare February 28, 2023 18:52
@yanokwa yanokwa marked this pull request as ready for review February 28, 2023 22:16
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

I have reviewed this with @yanokwa and can explain the various decisions made.

@lognaturel lognaturel merged commit 975b620 into getodk:next Feb 28, 2023
@yanokwa yanokwa deleted the update-nginx branch February 28, 2023 23:44
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.

2 participants