(MODULES-10391) ssl_protocol includes SSLv2 and SSLv3 on all platforms #1990
Conversation
|
Thanks for reporting this... I just discovered this commit and am baffled that this kind of change could ever pass review with this kind of inconspicuous commit message. Also this should never have been committed as combined commit but as seperate commits to keep things atomic. This also raises questions about quality control at puppetlabs and I would indeed appreciate a statement how those kind of dangerous changes can be avoided in the future. But to also be totally clear on the front of responsibility: The fact that this hit me is MY fault for not reviewing carefully enough. I just wanted to point out that these kind of changes are bad practice and the harmless commit message puts the cherry on the pie. /cc ing @sheenaajay here! |
|
@legooolas Thanks for submitting this PR. Apologise for this. Will review them and get it in asap.Thanks again. |
|
@TwizzyDizzy Thanks for the valuable feedback. Apologies, Will get the changes reviewed and merged. Running them on Adhoc pipeline. |
| $ssl_protocol = ['all'] # Implementations of the SSLv2 and SSLv3 protocol versions have been removed from OpenSSL (and hence mod_ssl) because these are no longer considered secure. For additional documentation https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/deploying_different_types_of_servers/setting-apache-web-server_deploying-different-types-of-servers | ||
| } else { | ||
| $ssl_protocol = [ 'all', '-SSLv2', '-SSLv3' ] | ||
| } |
There was a problem hiding this comment.
Since this module has a dependency on Puppet >= 5.5.10 I'd love for new parameters to be added to Hiera files instead of using params.pp.
I'm not sure what your policy is, so it might be out of the question, but it seems a bit silly to continue using the deprecated params.pp style defaults when you can use Hiera instead. I'd even prefer to keep params.pp around (until parameters can be migrated to Hiera), and use Hiera for any new parameters, even if it means that both systems will be used for a while.
There was a problem hiding this comment.
...and as per https://puppet.com/docs/puppet/latest/hiera_migrate.html#adding_hiera_data_to_a_module using Hiera is the preferred way:
Modules need default values for their class parameters. Before, the preferred way to do this was the “params.pp” pattern. With Hiera 5, you can use the “data in modules” approach instead.
There was a problem hiding this comment.
Again this is just to match the existing manifests, and updating to use hiera module data should probably be a separate PR to change this for all params at once.
There was a problem hiding this comment.
Yes, I understand that. I'd simply prefer to start moving parameters to Hiera now, instead of doing it at Optional[String] $future_date -- Hiera is just so much easier to comprehend and maintain than params.pp 😄
|
@legooolas Apologies for the regression. |
sheenaajay
changed the title
|
@legooolas Following changes are committed to this PR.
|
(MODULES-10391) ssl_protocol includes SSLv2 and SSLv3 on all platforms
(Reported as MODULES-10391)
The default for the
apache::mod::ssl_protocolparameter was changed to allow SSLv2 and SSLv3 on all platforms, which isn't desirable on anything with an older version of OpenSSL. Noticed on CentOS 7 hosts after upgrading this module, without changing any parameters.Hence this PR is to move the default setting to the params class and only set it for RHEL 8, which appears to be in the intention of the prior change in FM-8721 (PR #1691
This then fixes this regression on other platforms.