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

Update EL8+ and Debian SSL defaults #2336

Merged
merged 3 commits into from
Oct 27, 2022

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Oct 13, 2022

It has been a long time since the defaults were updated. This results in a safer out-of-the-box deployment, matching what the vendors do.

For reference, EL7 has SSLCipherSuite HIGH:3DES:!aNULL:!MD5:!SEED:!IDEA and SSLProtocol all -SSLv2 -SSLv3. The 3DES looks to be a mistake and I'd expect them to have meant !3DES. I have not updated that default nor checked SLES.

@ekohl ekohl requested a review from a team as a code owner October 13, 2022 17:32
@puppet-community-rangefinder
Copy link

apache::mod::ssl is a class

Breaking changes to this file WILL impact these 28 modules (exact match):
Breaking changes to this file MAY impact these 2 modules (near match):

apache::params is a class

Breaking changes to this file WILL impact these 7 modules (exact match):
Breaking changes to this file MAY impact these 4 modules (near match):

This module is declared in 174 of 579 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@ekohl
Copy link
Collaborator Author

ekohl commented Oct 13, 2022

Looks like I'll need to update the tests to match the expected values.

@ekohl ekohl force-pushed the update-ssl-defaults branch from 3245abf to 47614f4 Compare October 21, 2022 10:19
@ekohl
Copy link
Collaborator Author

ekohl commented Oct 21, 2022

Includes #2335 since EL8+ sets SSLProxyCipherSuite by default.

@ekohl ekohl marked this pull request as ready for review October 21, 2022 10:19
@ekohl ekohl force-pushed the update-ssl-defaults branch from 47614f4 to 0c9591e Compare October 24, 2022 23:54
@ekohl
Copy link
Collaborator Author

ekohl commented Oct 24, 2022

@david22swan @chelnak it would help me a lot if this was reviewed and merged this week. Currently the tests are still running, hoping they'll pass but I'd appreciate if you take a look at the general design.

@ekohl ekohl added the feature label Oct 25, 2022
@ekohl ekohl force-pushed the update-ssl-defaults branch from 0c9591e to 681665f Compare October 25, 2022 10:54
@ekohl
Copy link
Collaborator Author

ekohl commented Oct 25, 2022

I do not know why mod_md has started to fail. I've also never used mod_md myself so not sure what's wrong. Sadly the error log is not very useful. I've pushed some changes that I hope will help.

On EL8+ the default ssl.conf file doesn't specify SSLProtocol at all,
which implies using the system profile where it can be changed. This
changes the template to deal with ssl_protocol set to an empty array,
which was previously generating invalid syntax anyway.
@ekohl ekohl force-pushed the update-ssl-defaults branch from 681665f to 298b0ba Compare October 25, 2022 13:18
@ekohl
Copy link
Collaborator Author

ekohl commented Oct 25, 2022

I decided to dive into how one would do this properly in rspec. voxpupuli/beaker-rspec#115 is the result. I'll have a stab at porting this to Litmus, which should be very straight forward. That should allow me to provide sufficient debug info.

@ekohl
Copy link
Collaborator Author

ekohl commented Oct 26, 2022

Now we're getting somewhere:

AH02311: Fatal error initialising mod_ssl, exiting. See /var/log/httpd/example.com_error_ssl.log for more information

@ekohl ekohl force-pushed the update-ssl-defaults branch 3 times, most recently from 02e9f37 to 1206141 Compare October 26, 2022 11:03
@ekohl
Copy link
Collaborator Author

ekohl commented Oct 26, 2022

So it fails with:

Wed Oct 26 10:48:29.313335 2022] [ssl:warn] [pid 13085] AH10085: Init: example.com:443 will respond with '503 Service Unavailable' for now. There are no SSL certificates configured and no other module contributed any.
[Wed Oct 26 10:48:29.313646 2022] [ssl:emerg] [pid 13085] AH01898: Unable to configure permitted SSL ciphers
[Wed Oct 26 10:48:29.313661 2022] [ssl:emerg] [pid 13085] SSL Library Error: error:1410D0B9:SSL routines:SSL_CTX_set_cipher_list:no cipher match

So it looks like mod_md and using PROFILE=system is incompatible.

I'm debating options now. One option is to modify the test to explicitly set a cipher list. I'll try to set up a testing system to verify.

On EL8+ OpenSSL is patched to support a unified crypto policy. This is
also the default when installing mod_ssl. Users of RHEL Insights will
also receive warnings when the defaults differ.
@ekohl ekohl force-pushed the update-ssl-defaults branch from 1206141 to 3cb3266 Compare October 26, 2022 15:12
@ekohl
Copy link
Collaborator Author

ekohl commented Oct 26, 2022

So it looks like mod_md and using PROFILE=system is incompatible.

Turns out it was much simpler than I thought. It only showed up with mod_md since that's the only HTTPS vhost. That means the ciphers aren't evaluated elsewhere and our mod_ssl acceptance test happily accepts it since it's syntactically valid. It's actually PROFILE=SYSTEM, not PROFILE=system.

@ekohl
Copy link
Collaborator Author

ekohl commented Oct 26, 2022

@david22swan @chelnak test failures are now provisioning errors: it never even got to running the test suite. Can this be merged?

bastelfreak
bastelfreak previously approved these changes Oct 26, 2022
Debian 10 and Ubuntu 18.04 (oldest of supported Debian-based distros)
default to these values. This gives a safer out-of-the-box experience.
@ekohl ekohl force-pushed the update-ssl-defaults branch from 3cb3266 to 67a8a17 Compare October 26, 2022 17:49
@ekohl
Copy link
Collaborator Author

ekohl commented Oct 26, 2022

No change, just forcing a new CI run.

Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

LGTM
Thank's for getting this in

@david22swan david22swan merged commit 47f0d72 into puppetlabs:main Oct 27, 2022
@ekohl ekohl deleted the update-ssl-defaults branch October 27, 2022 09:55
dduportal added a commit to jenkins-infra/jenkins-infra that referenced this pull request Feb 24, 2023
dduportal added a commit to jenkins-infra/jenkins-infra that referenced this pull request Feb 24, 2023
dduportal added a commit to jenkins-infra/jenkins-infra that referenced this pull request Feb 27, 2023
* chore: Updated the content of the file "/tmp/updatecli/github/jenkins...

... -infra/jenkins-infra/Puppetfile"

Made with ❤️️ by updatecli

* chore: Updated the content of the file "/tmp/updatecli/github/jenkins...

... -infra/jenkins-infra/Puppetfile"

Made with ❤️️ by updatecli

* chore: Updated the content of the file "/tmp/updatecli/github/jenkins...

... -infra/jenkins-infra/Puppetfile"

Made with ❤️️ by updatecli

* chore: Updated the content of the file "/tmp/updatecli/github/jenkins...

... -infra/jenkins-infra/Puppetfile"

Made with ❤️️ by updatecli

* chore: Updated the content of the file "/tmp/updatecli/github/jenkins...

... -infra/jenkins-infra/Puppetfile"

Made with ❤️️ by updatecli

* chore: Updated the content of the file "/tmp/updatecli/github/jenkins...

... -infra/jenkins-infra/Puppetfile"

Made with ❤️️ by updatecli

* chore: Updated the content of the file "/tmp/updatecli/github/jenkins...

... -infra/jenkins-infra/Puppetfile"

Made with ❤️️ by updatecli

* chore(Jenkinsfile) no need to fail fast the unit tests

Signed-off-by: Damien Duportal <[email protected]>

* chore(tests) map to the new SSL default config introduced in puppetlabs/puppetlabs-apache#2336

Signed-off-by: Damien Duportal <[email protected]>

* fix(jenkinscontroller) correct apache::vhost::override types

Signed-off-by: Damien Duportal <[email protected]>

* fix(pkgrepo,usage) correct apache::vhost::options types

Signed-off-by: Damien Duportal <[email protected]>

* cleanup(edamame) remove apache-related setup

Signed-off-by: Damien Duportal <[email protected]>

* fix(census,usage) correct apache::vhost::port types

Signed-off-by: Damien Duportal <[email protected]>

---------

Signed-off-by: Damien Duportal <[email protected]>
Co-authored-by: GitHub Actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Damien Duportal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants