Fixes #39405 - Add tls_min_version and tls_ciphers settings by adamlazik1 · Pull Request #947 · theforeman/smart-proxy

adamlazik1 · 2026-06-08T23:48:34Z

Made with Cursor.

adamlazik1 · 2026-06-08T23:54:03Z

Since tls_disabled_ciphers became dead code because of tls_ciphers, I decided to remove the deprecated opts in the same PR.

ekohl

This is a breaking change so perhaps this is good to share in the community. Perhaps an upstream RFC, even if it's just informational? Then in the release note you can refer to it.

adamlazik1 · 2026-06-10T11:29:56Z

This is a breaking change so perhaps this is good to share in the community. Perhaps an upstream RFC, even if it's just informational? Then in the release note you can refer to it.

Done. https://community.theforeman.org/t/rfc-new-tls-configuration-options-in-smart-proxy-tls-ciphers-tls-min-version/46777

ekohl · 2026-06-10T11:34:07Z

https://github.com/theforeman/puppet-candlepin/blob/033a4f1d8d1e5bdfefdc9f3d01ebb9fe374ab747/spec/acceptance/basic_candlepin_spec.rb#L14-L28 is my tip on how to verify the outcome.

adamruzicka

Seems to work well, except for the empty string.

For what I checked see https://gist.github.com/adamruzicka/4b6370339dd36b9f9836aeba2080f40a . There are more combinations that could be made, but the essentials should be covered.

adamlazik1 · 2026-06-11T15:53:46Z

If I explicitly set tls_min_version, but I leave tls_ciphers unset and autodetection kicks in and crypto policies get applied, then I'd expect to get (and do get) an intersection of the ciphers allowed by the min version and by the crypto policy. This expectation (and behaviour) is correct, right?

I'd say yes.

adamruzicka · 2026-06-12T09:53:47Z

An array with arbitrary values can be passed in as a value for tls_ciphers. It gets passed around, doesn't seem to raise any errors anywhere, but ultimately doesn't seem to have any effect on anything. This is misleading at best

ekohl · 2026-06-12T14:34:19Z

https://github.com/theforeman/puppet-candlepin/blob/033a4f1d8d1e5bdfefdc9f3d01ebb9fe374ab747/spec/acceptance/basic_candlepin_spec.rb#L14-L28 is my tip on how to verify the outcome.

And I also found https://testssl.sh/

lhellebr · 2026-06-18T10:12:29Z

-        :SSLCiphers => CIPHERS - Proxy::SETTINGS.ssl_disabled_ciphers,
+        :SSLOptions => build_ssl_options,
+        :SSLCiphers => tls_ciphers,
+        :SSLMinVersion => resolve_tls_min_version,


If we set crypto-policy to DEFAULT:TLS13-ONLY (TLS13-ONLY being a custom subpolicy) but in the config we set tls_min_version to 1.2, this would keep the correct cipher set but would degrade TLS min version to 1.2 (in conflict with crypto-policies).

Chmm, in theory we could go back to the old approach of disabling individual versions which should interact with crypto policies in a slightly better way

I think it's a bit of an edge case. I'd prefer to have 2 use cases:

crypto-policies (RPM-based installs)

explicitly configured (where crypto-policies is unavailable, like Debian, FreeBSD and Windows)
The case where you configure 1 option, but not the other is IMHO up to the user who does that.

We can keep min_tls version in repo and remove it in the foreman-packaging rpm patch so that rpm builds rely only on crypto policies. Would that work @adamruzicka @ekohl ?

I'd prefer to have as few patches in packaging as possible. How about making PROFILE=SYSTEM special and refusing to start if tls_ciphers == 'PROFILE=SYSTEM' and tls_min_version is set?

Yeah, I can do that.

Agree, and please add a test for it.

We can keep min_tls version in repo and remove it in the foreman-packaging rpm patch so that rpm builds rely only on crypto policies.

If we're confident enough in the auto-detection then we wouldn't need any packaging patch. I'd be OK with that. Emitting a log message on startup that can be asserted in automated tests is probably enough.

I'd prefer to have as few patches in packaging as possible. How about making PROFILE=SYSTEM special and refusing to start if tls_ciphers == 'PROFILE=SYSTEM' and tls_min_version is set?

Is it really a problem? If the user configures it that way, what will break? If foremanctl doesn't provide you any option to set that then IMHO it's really up to the user. Perhaps emit a log warning?

I'd prefer to have as few patches in packaging as possible. How about making PROFILE=SYSTEM special and refusing to start if tls_ciphers == 'PROFILE=SYSTEM' and tls_min_version is set?

Applied.

lhellebr · 2026-06-22T15:08:51Z

-        :SSLOptions => ssl_options,
-        :SSLCiphers => CIPHERS - Proxy::SETTINGS.ssl_disabled_ciphers,
+        :SSLOptions => build_ssl_options,
+        :SSLCiphers => tls_ciphers,


This works for TLS 1.2 but not for TLS 1.3. Unfurtunately, SSL_CTX_set_cipher_list() is TLS 1.2 only and SSL_CTX_set_ciphersuites() is TLS 1.3 only and the Webrick/Ruby stack under the hood only seems to do the former.
The same hack as in https://github.com/theforeman/smart-proxy/pull/947/changes#diff-54572e87725df68a63244713ca93aa40c81de79d76cba589c0a1d0329812db04R160 might help.

This may make your work easier: warmcat/libwebsockets#1440

Currently trying to resolve this, for now I have no idea why the tests fail. I need to investigate further.

lhellebr · 2026-06-24T09:51:12Z

The CI failure is real, I've just reproduced on Debian with the current PR version. The proxy refuses to start because

Errors detected on startup, see log for details. Exiting: undefined method `ciphersuites=' for #<OpenSSL::SSL::SSLContext:0x00007f28a61b8890 @verify_mode=0, @verify_hostname=false>

We probably need to check whether ciphersuites= is understood by underlying ruby/openssl. If yes, call it, if not... those older versions probably still support TLS 1.3 so just not calling it that would mean the cipher setting gets silently ignored. Maybe just make that not silent and refuse to start if ciphers are explicitly set in the config but we can't set them?

Interestingly, it seems that Red Hat's version of Ruby has this patched in earlier Ruby versions (https://gitlab.com/redhat/centos-stream/rpms/ruby/-/commit/42815870), which is why it works on RHEL with Ruby 3.0.7, but fails in CI on Ruby 3.0 and 3.1 (upstream, it got patched in 3.2 ruby/openssl#493).

adamlazik1 · 2026-06-25T12:59:22Z

Maybe just make that not silent and refuse to start if ciphers are explicitly set in the config but we can't set them?

Applied. Went with this suggestion.

lhellebr

More issues found - I haven't really tested yet but this seems sus on reading. These things are getting quite complicated.

lhellebr · 2026-06-26T14:29:14Z

+        rescue OpenSSL::SSL::SSLError
+          nil
+        end
+      elsif settings.tls_ciphers && settings.tls_min_version == '1.3'


What if the user didn't specify TLS 1.3 explicitly (thus it's still allowed together with 1.2) but has specified some constraints for a cipher that is applicable to TLS 1.3? Wouldn't then those be ignored on a machine without ciphersuites= and wouldn't TLS 1.3 still run, but with any cipher available, even those that the user forbid?

What if the user allows both TLS 1.2 and 1.3 but specifies a cipher only recognized by ciphersuites= (because it's only in TLS 1.3)? ciphers= would return nil and allow any TLS 1.2 cipher, wouldn't it?

1
You are right. And to be honest I have no idea how to solve that. Consulting AI gave me these options:

a. Widen the elsif guard to settings.tls_ciphers (without the tls_min_version check) — raises whenever any explicit cipher is set and ciphersuites= is absent. This is safe but means PROFILE=SYSTEM without tls_min_version also refuses to start on Ruby 3.0, which is overly strict.

b. Accept the limitation and document it — on Ruby 3.0 without the Red Hat backport, TLS 1.3 cipher restrictions from tls_ciphers are silently not applied. Practically, PROFILE=SYSTEM is only meaningful on RHEL where ciphersuites= is always available anyway, so real-world exposure is low.

To me, b. seems more viable. Let me know your thoughts, or if you know about another solution.

2
Added logger warning for this case, let me know if I should change it to exception.

lhellebr · 2026-06-26T14:30:17Z

+          nil
+        end
+      elsif settings.tls_ciphers && settings.tls_min_version == '1.3'
+        raise "tls_min_version '1.3' is configured but this Ruby/OpenSSL build does not support TLS 1.3. " \


Misleading message. It supports TLS 1.3 but doesn't support constraining it by ciphersuites=.

adamlazik1 · 2026-07-01T17:13:06Z

Removed the packaging patch inclusion.

lhellebr · 2026-07-02T19:58:41Z

ACK — Verified on RHEL 9.8 (Satellite, packit build pr947.753.gb7d2025) and Debian 12 (upstream Foreman, manually patched).

I'm ACKing this PR because the known issues listed below are not regressions — the same behavior exists in the current code without this PR. However, I want to be transparent that this PR introduces a cipher configuration interface that does not fully deliver on its promise: an admin setting tls_ciphers would reasonably expect it to restrict ciphers for all TLS versions, but it silently leaves the other TLS version unrestricted in several scenarios. These are security concerns that should be addressed in follow-up work.

What was tested

Tested tls_ciphers and tls_min_version settings with various values (valid, invalid, edge cases including non-string types), their interaction with RHEL crypto-policies (ciphers, TLS versions, signatures), the auto-detection logic (PROFILE=SYSTEM on RHEL, HIGH fallback on Debian), auto-detection resilience (broken/missing/unreadable files), and the ciphersuites= compatibility fix on Debian where the method is missing.

Security concerns (not regressions, tracked separately)

On platforms where the ciphersuites= method is missing (e.g., Debian 12), TLS 1.3 ciphers cannot be controlled at all. When tls_ciphers is explicitly set, all TLS 1.3 ciphers are allowed regardless of the configured value.
Even on platforms where ciphersuites= is available (e.g., RHEL 9), when tls_ciphers is set to a value that only matches one TLS version, the other TLS version is left with unrestricted ciphers. This goes both ways: a TLS 1.2-only cipher leaves TLS 1.3 unrestricted, and a TLS 1.3-only cipher leaves TLS 1.2 unrestricted. The existing warning only covers the latter case — there is no warning for the TLS 1.2-only cipher case. Note that this does not apply when ciphers are controlled through crypto-policies — crypto-policies correctly restricts both TLS versions.

Notes

The packaging patch (foreman-packaging#13625) is no longer needed — auto-detection handles RHEL correctly.
The deprecated ssl_disabled_ciphers and tls_disabled_versions options seem to be effectively removed by this PR.
Deployment tooling (foreman-installer / foremanctl) needs updating to expose the new options and remove the deprecated ones from templates.

Removed ssl_disabled_ciphers and tls_disabled_versions.

adamlazik1 · 2026-07-08T11:46:36Z

Squashed.

pr-processor Bot added the Not yet reviewed label Jun 8, 2026

ekohl reviewed Jun 9, 2026

View reviewed changes

Comment thread lib/launcher.rb Outdated

pr-processor Bot removed the Not yet reviewed label Jun 9, 2026

adamruzicka reviewed Jun 10, 2026

View reviewed changes

Comment thread lib/launcher.rb Outdated

Comment thread lib/launcher.rb Outdated

Comment thread lib/launcher.rb Outdated

adamlazik1 force-pushed the tls-opts branch from 97abd5d to e8e2ad8 Compare June 10, 2026 15:30

ekohl reviewed Jun 10, 2026

View reviewed changes

Comment thread lib/launcher.rb Outdated

Comment thread lib/launcher.rb Outdated

ofedoren reviewed Jun 10, 2026

View reviewed changes

Comment thread lib/launcher.rb Outdated

adamruzicka reviewed Jun 11, 2026

View reviewed changes

Comment thread lib/launcher.rb Outdated

Comment thread lib/launcher.rb Outdated

pr-processor Bot added Waiting on contributor Needs re-review and removed Needs re-review labels Jun 11, 2026

adamruzicka reviewed Jun 12, 2026

View reviewed changes

Comment thread lib/launcher.rb Outdated

Comment thread lib/launcher.rb

Comment thread lib/launcher.rb Outdated

pr-processor Bot added Needs re-review and removed Needs re-review labels Jun 12, 2026

adamlazik1 mentioned this pull request Jun 15, 2026

Automatically use system openssl configuration on rpm builds theforeman/foreman-packaging#13625

Closed

pr-processor Bot added the Needs re-review label Jun 15, 2026

lhellebr reviewed Jun 18, 2026

View reviewed changes

pr-processor Bot removed the Needs re-review label Jun 18, 2026

lhellebr reviewed Jun 22, 2026

View reviewed changes

pr-processor Bot added the Needs re-review label Jun 23, 2026

adamlazik1 force-pushed the tls-opts branch 2 times, most recently from 9045fd8 to d1eb537 Compare June 23, 2026 13:55

pr-processor Bot removed the Needs re-review label Jun 23, 2026

pr-processor Bot added the Needs re-review label Jun 25, 2026

adamlazik1 force-pushed the tls-opts branch from b4e878a to 1934ad7 Compare June 25, 2026 13:30

lhellebr requested changes Jun 26, 2026

View reviewed changes

pr-processor Bot added Needs re-review and removed Needs re-review labels Jun 26, 2026

adamlazik1 force-pushed the tls-opts branch from 949c981 to 10db163 Compare June 26, 2026 16:33

pr-processor Bot added the Needs re-review label Jun 26, 2026

adamlazik1 force-pushed the tls-opts branch 2 times, most recently from 7048aa0 to b7d2025 Compare July 1, 2026 17:12

lhellebr approved these changes Jul 3, 2026

View reviewed changes

pr-processor Bot removed the Needs re-review label Jul 3, 2026

Fixes #39405 - Add tls_min_version and tls_ciphers settings

d7a67f1

Removed ssl_disabled_ciphers and tls_disabled_versions.

adamlazik1 force-pushed the tls-opts branch from b7d2025 to d7a67f1 Compare July 8, 2026 11:45

pr-processor Bot added Needs re-review and removed Waiting on contributor labels Jul 8, 2026

Uh oh!

Conversation

adamlazik1 commented Jun 8, 2026

Uh oh!

adamlazik1 commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamlazik1 commented Jun 10, 2026

Uh oh!

ekohl commented Jun 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adamlazik1 commented Jun 11, 2026

Uh oh!

adamruzicka commented Jun 12, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ekohl commented Jun 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhellebr commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamlazik1 commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lhellebr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamlazik1 Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamlazik1 commented Jul 1, 2026

Uh oh!

adamlazik1 commented Jun 8, 2026 •

edited

Loading

lhellebr commented Jun 24, 2026 •

edited

Loading

adamlazik1 commented Jun 25, 2026 •

edited

Loading

adamlazik1 Jun 26, 2026 •

edited

Loading