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

Use background DH group creation #394

Merged
merged 1 commit into from
Jun 8, 2018

Conversation

buchdag
Copy link
Member

@buchdag buchdag commented Jun 5, 2018

This PR mirrors what is being done by jwilder/nginx-proxy : create the DH group in the background with a low priority job and use a temporary pre-generated 2048 bits DH group while it's being done. This way the container can start working immediately while still being secure.

Credits goes to @kamermans for the original code contibution to jwilder/nginx-proxy.

@buchdag buchdag requested a review from JrCs June 5, 2018 11:55
@buchdag
Copy link
Member Author

buchdag commented Jun 7, 2018

Might fix #396

@buchdag buchdag merged commit 7c07356 into nginx-proxy:master Jun 8, 2018
@buchdag buchdag deleted the background-dhparam branch June 8, 2018 13:09
local GEN_LOCKFILE="/tmp/le_companion_dhparam_generating.lock"

# The hash of the pregenerated dhparam file is used to check if the pregen dhparam is already in use
local PREGEN_HASH=$(md5sum "$PREGEN_DHPARAM_FILE" | cut -d ' ' -f1)

Choose a reason for hiding this comment

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

why md5sum and not sha256sum ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we're just doing a quick internal file comparison and an md5 hash is more than enough for that purpose.

Worst case and extremely improbable scenario is you generate a dh params file whose md5 collide with the pre generated dh params and a new one is generated again at container restart.

Copy link

@NicolasDorier NicolasDorier Jun 14, 2018

Choose a reason for hiding this comment

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

sha256sum is 2 letters longer and actually safe.

Also unrelated, if I understand using this new PR means that for 1 deployment there will be 2 certificate requests right? It need to be documented so people don't go over the limits for their domain unexpectedly.

Copy link
Member Author

@buchdag buchdag Jun 14, 2018

Choose a reason for hiding this comment

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

No, this is completely unrelated to certificate requests.

What this PR does is outlined in the first message:

  • check if a custom (ie different from the bundled one) DH parameters file is present.
  • if not, use the pre generated DH parameters file bundled with the image and start generating a new one in the background, with a low priority process.
  • when the new DH parameters file is generated, reload nginx so it start using it.

The goal here was to remove the initial wait and service unavailability due to foreground DH parameters file generation (which could be quite long on some low end hardware) whilst still using a strong 2048 bits DH parameters on first start.

Choose a reason for hiding this comment

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

Awesome, I thought the DH parameter was used for the private key generation of the certificate, but it is used only in the diffie hellman.

@buchdag buchdag added the kind/feature-request Issue requesting a new feature label Oct 10, 2019
bingozb pushed a commit to bingozb/docker-letsencrypt-nginx-proxy-companion that referenced this pull request Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature-request Issue requesting a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants