Skip to content

Conversation

panteparak
Copy link
Contributor

I would like to add the ability to change the default Diffie-Hellman parameters of 2048 bits through the environment variable. This would allow DH Param to be future proof where the user would be able to pick a desired value such as 3072 or 4096 bits

@JrCs
Copy link
Collaborator

JrCs commented Sep 22, 2017

@XPLOT1ON what is your use case to need to change the number of bits for DH keys. It's already 2048 and give us a A+ grade on Qualsys SSL Server Test ?

@panteparak
Copy link
Contributor Author

@JrCs Yes, 2048 does give A+ SSL raitings, but when I generate DH params, I would prefer for a bigger bit size such as 4096. and as mentioned before, if 2048 bit size is cracked (well, not very soon, I hope) a change to the DH param bit size can override the default value as needed.

Copy link
Member

@buchdag buchdag left a comment

Choose a reason for hiding this comment

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

@XPLOT1ON > could you change the variable name to DHPARAM_BITS to mimic the one used by nginx-proxy for the same purpose ?

IMO, the input check shouldn't be described in the doc, and naming the variable DHPARAM_BITS has the advantage of being explicit about what should go there.

Speaking of input check openssl dhparam -out dhparam.pem 02048 works just fine, at least on the 0.9.8 I tried it with (and on the latest libressl too). So maybe the regex should just be '^[0-9]*$' to avoid triggering on a valid input.

The example should be with a non default value eg 1024 or 4096.

@JrCs JrCs requested review from JrCs and buchdag November 23, 2017 12:17
Copy link
Collaborator

@JrCs JrCs left a comment

Choose a reason for hiding this comment

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

Don't force the user to add DH_PARAM_SIZE environment variable. If not set the default must be 2048

Dockerfile Outdated
DOCKER_GEN_VERSION=0.7.3 \
DOCKER_HOST=unix:///var/run/docker.sock
DOCKER_HOST=unix:///var/run/docker.sock \
DH_PARAM_SIZE=2048
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't force the user to add DH_PARAM_SIZE environment variable. If not set the default must be 2048

Copy link
Contributor Author

@panteparak panteparak Nov 24, 2017

Choose a reason for hiding this comment

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

Isn't ENV DH_PARAM_SIZE would be a default value for docker environment? User can override the bits size via compose or docker run. @JrCs

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer not to depend of the content of the dockerfile.
Closer to the code is better.

}

function check_dh_group {
re='^[1-9][0-9]*$'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add something like

: "${DH_PARAM_SIZE:=2048}"

to have a default value for DH_PARAM_SIZE variable

@buchdag
Copy link
Member

buchdag commented Dec 18, 2017

@XPLOT1ON > let me know if you need help to complete this :)

@panteparak
Copy link
Contributor Author

@buchdag sure. I've got sidetracked from completing this due to other commitment.

@buchdag
Copy link
Member

buchdag commented Feb 10, 2018

@JrCs I think this is good to go now, awaiting your approval.

@buchdag buchdag merged commit cf77c7e into nginx-proxy:master Feb 18, 2018
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.

3 participants