Make ssl_prefer_server_ciphers configurable in server / mailhost #1067
Conversation
| @@ -21,7 +21,9 @@ | |||
| <%- if defined? @ssl_password_file -%> | |||
| ssl_password_file <%= @ssl_password_file %>; | |||
| <%- end -%> | |||
| ssl_prefer_server_ciphers on; | |||
| <%- if @ssl_prefer_server_ciphers -%> | |||
| ssl_prefer_server_ciphers <%= @ssl_prefer_server_ciphers%>; | |||
There was a problem hiding this comment.
minor nitpick: missing space after ciphers?
| @@ -26,7 +26,9 @@ | |||
| <% end -%> | |||
| ssl_protocols <%= @ssl_protocols %>; | |||
| ssl_ciphers <%= @ssl_ciphers %>; | |||
| ssl_prefer_server_ciphers on; | |||
| <%- if @ssl_prefer_server_ciphers -%> | |||
| ssl_prefer_server_ciphers <%= @ssl_prefer_server_ciphers%>; | |||
|
The server spec doesn't check for the off case? Maybe you should add that. It's also interesting that the other spec uses a regex, and the other doesn't, but maybe that's just historical. Anyway, other than that, looks good to me. |
| @@ -26,7 +26,9 @@ | |||
| <% end -%> | |||
| ssl_protocols <%= @ssl_protocols %>; | |||
| ssl_ciphers <%= @ssl_ciphers %>; | |||
| ssl_prefer_server_ciphers on; | |||
| <%- if @ssl_prefer_server_ciphers -%> | |||
There was a problem hiding this comment.
I'm not up to date on Puppet 4 syntax. But if Enum only allows yes or no why have a conditional in the template? It will always be either "yes" or "no" and with that logic the conditional will always be true, so the directive will always end up in the generated config.
There was a problem hiding this comment.
You're right, of course. I think I had originally implemented it as a string (and had been debating whether it was feasible to have it set to undef to suppress the line entirely, possibly useful in cases where you want a tidy config and it's already set at another scope). I'm not sure if this is possible with an Enum, but open to suggestions. For now, I've pulled this out. I didn't test the spacing, but passes tests.
…urces (#1032). Default to 'on', vs. the nginx default of 'off'
|
@oranenj Yeah, you're right... I added back in the test for the off case. My thinking was, the current tests are kind of simplistic, and not sure that testing both on and off adds much vs. a more complex test, but this should work. I've addressed the other comments / feedback in the updated version. |
Make ssl_prefer_server_ciphers configurable in server / mailhost
Attempt at #1032.
This preserves the module's previous behavior of having it on; we could rework it to default to undef and inherit the default nginx behavior of 'off', though I think on is the better behavior.
I wasn't sure what the wrap conventions for the inline docs are, but I rewrapped server @ 140 characters and realigned the dashes.
On an unrelated note, now that we're switching to parameters, I wonder if it makes sense to treat on / off as booleans w/ a function like @3flex had been suggesting, however for now, I'm making it an Enum and trying to follow the new conventions.
Let me know how the tests look, I'm thinking it might be better to test the default behavior also / instead (vs. setting the param explicitly), but this seems to follow what's there now.