-
Notifications
You must be signed in to change notification settings - Fork 815
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 RFC 7919 DH groups + Remove DH generation #904
Conversation
Co-authored-by: polarathene <5098581+polarathene@users.noreply.github.com>
Both in nginx-proxy/acme-companion and nginx-proxy/nginx-proxy
Replaces existing group if it does not match the DHPARAM_BITS key size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides a few nit-picks.
Also it might not have been visible in your editor due to your tab-width but Github renders a wider tab-width where it's rather obvious that there is mixed indentation of some lines with spaces and others with tabs, you probably want to normalize that.
|
||
for f in /app/dhparam/ffdhe*.pem; do | ||
local FFDHE_HASH; FFDHE_HASH=$(sha256sum "$f" | cut -d ' ' -f1) | ||
if [[ "$DHPARAM_HASH" == "$FFDHE_HASH" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some inconsistency throughout the diff with vars.
General advice you'd get from a linter like shellcheck is to prefer "${VAR_NAME}"
, and within [[ ]]
conditionals you can avoid the quotes IIRC (but not within the other kind [ ]
that I generally avoid myself).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm already using shellcheck 🙂
As you can see it's not complaining about "${VAR_NAME}"
vs "$VAR_NAME"
and require quotes on the right-hand side of ==
inside [[ ]]
. Quoting the left-hand side is just an habit I picked up from the defensive / safe / whatever bash programming tenet of "quote all uses of variables".
I'm personally not a big fan of using "${VAR_NAME}"
over "$VAR_NAME"
, my personal rule of thumb is to use {}
only when I'm doing variable manipulation / expansion / interpolation, like described here.
I'll make at least the check_dh_group
function internally consistant but clearly there are inconsistencies here and there on the whole code base. 😦
No, I'm aware of this, I think it's been present in the code since before I started contributing to this project and I never got to normalize the situation. |
+ removal of an unneeded local keyword
This PR brings acme-companion's DH group creation in line with nginx-proxy : nginx-proxy/nginx-proxy#1797
Thanks to @polarathene for the original idea and work.