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

Declare a cipher unsupported for direct AD integration #3549

Merged
merged 6 commits into from
Jan 16, 2025

Conversation

asteflova
Copy link
Member

What changes are you introducing?

I'm adding a note about the TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 cipher suite being unsupported. For Satellite, I'm also adding a link to a KBase solution.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

https://issues.redhat.com/browse/SAT-25882 reports errors in user environments that rely on the cipher.

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

N/A

Checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.13/Katello 4.15
  • Foreman 3.12/Katello 4.14 (Satellite 6.16)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only; orcharhino 7.0 on EL8+EL9)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • We do not accept PRs for Foreman older than 3.7.

@asteflova asteflova added the Needs tech review Requires a review from the technical perspective label Jan 6, 2025
@asteflova
Copy link
Member Author

Hi @adamruzicka and @lhellebr, can you please review? Looking at the Jira (see this PR's description), I'm wondering whether we should also mention a specific Windows Server version that this applies to.

@lhellebr
Copy link

lhellebr commented Jan 6, 2025

More of a questions than suggestions:

  1. Should we instead list supported ciphers positively? Sounds more sane than maintaining a list of everything we don't support.

  2. Should we instead specify that the user should let the Satellite decide on what cipher to use, not changing the default configuration? (But maybe not since the other side can already initiate the connection enforcing certain cipher? OTOH Satellite should handle that and refuse such communication.)

@adamruzicka
Copy link
Contributor

Should we instead list supported ciphers positively?

I'm not sure if that would be feasible. We were operating under the assumption that as long as both sides can agree on a cipher, it should work, until now we never really had to care about such low level details. Apparently there is a bug in the net-ldap/ruby-openssl/openssl or AD (or both) which makes the connections using this particular cipher occasionally break. Listing all that is supported would be better, if we actually knew. On a stock rhel9, openssl ciphers -s gives me 35 ciphers enabled by default, testing all of them would be rather time consuming.

Should we instead specify that the user should let the Satellite decide on what cipher to use, not changing the default configuration? (But maybe not since the other side can already initiate the connection enforcing certain cipher?

Now this is where it gets interesting. Both sides have a list of allowed ciphers and they agree on one to use. Even with stock settings, DHE-RSA-AES256-GCM-SHA384 is allowed to be used by default (at least on rhel side) but usually other ciphers from the list are preferred.

If it works ootb, should we maybe reword this to state that this is only relevant when applying security hardening?

OTOH Satellite should handle that and refuse such communication.

openssl does indeed handle this. The connection won't be established if both sides can't agree on connection parameters.

@lhellebr
Copy link

lhellebr commented Jan 6, 2025

openssl does indeed handle this. The connection won't be established if both sides can't agree on connection parameters.

I know, but perhaps Satellite should disable it since it's known now not to be supported?

@adamruzicka
Copy link
Contributor

I don't think it is worth the effort. As far as I know we've only had one or two people run into this over the last ten years. We may reconsider in the future if people start running into it again, but for now I'd say having it documented is good enough

@ekohl
Copy link
Member

ekohl commented Jan 6, 2025

Looking at https://ciphersuite.info/cs/TLS_DHE_RSA_WITH_AES_256_GCM_SHA384/ it's declared weak, so IMHO it's best to disable it.

From reading this change itself, it's not obvious to me how I (as a user) would disable it. The linked KB article is also unclear. Do I need to perform any action on the client (=Satellite) or Server (=LDAP) side?

@adamruzicka
Copy link
Contributor

Do I need to perform any action on the client (=Satellite) or Server (=LDAP) side?

On either end should be enough.

On the ldap side, you have to do whatever you have to do, depending on the implementation you have.

On client side (and on el*, no idea how this works on debian), you change /etc/crypto-policies/back-ends/openssl.config and the CipherString line in /etc/crypto-policies/back-ends/opensslcnf.config. The syntax is somewhat obscure and hard to get right. Maybe there are more sophisticated ways of going about this, but this already is way outside of what I'm sure about. Additionally, whatever you do in there will apply system-wide. Is this really something we want to do?

@ekohl
Copy link
Member

ekohl commented Jan 6, 2025

FYI: I think you can disable it system wide on RHEL 8 using https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/8/html/security_hardening/using-the-system-wide-cryptographic-policies_security-hardening#customizing-system-wide-cryptographic-policies-with-subpolicies_using-the-system-wide-cryptographic-policies (though https://www.redhat.com/en/blog/configuring-rhel-8-compliance-crypto-policy-related-cipher-block-chaining is IMHO a bit more concrete). Given it's a weak cipher, I'd expect it to be safe.

Edit: to be explicit: don't modify the default policy but rather refine it by removing a single cipher.

@adamruzicka
Copy link
Contributor

Edit: to be explicit: don't modify the default policy but rather refine it by removing a single cipher.

That might be a good thing to do in general, but it still wouldn't address the original issue where the user modified the default policy to explicitly include this problematic cipher by hand. From brief testing on one of my machines, you can craft a configuration in crypto policy backends that will win over the subpolicy

@ekohl
Copy link
Member

ekohl commented Jan 6, 2025

At first I didn't see there was a customer case attached (because I wasn't logged in), so I assumed this cipher was enabled by default.

For those non-Red Hatters reading along: this really is a case where the user manually enabled an insecure cipher that led to problems.

There's only a limited amount we can do and we can't possibly enumerate all cases where the user did something that breaks. On the other hand, it led to problems and reading the case this is a relatively common occurrence. Now I wonder how to best deal with this.

I'd love to understand why users add this insecure cipher to their configuration and also don't follow the documentation on how to refine the policy properly.

@asteflova asteflova force-pushed the ad-unsupported-cipher branch from 7691e5c to 5baf3d2 Compare January 7, 2025 07:56
@asteflova
Copy link
Member Author

Thanks for all the comments! I added another sentence to (hopefully) clarify that the cipher being enabled is a result of user actions and that in such situations, it needs to be disabled on either side.

I'm not adding any details on how exactly to disable the cipher because it being enabled is not the default state. This is not exactly a mainstream scenario so I'd prefer not to overdo how much information we provide. Let's assume that if users enabled it at some point, they can figure out how to disable it (I know that's not the safest of assumptions to make, but I think it might just be good enough in this situation).

@adamruzicka
Copy link
Contributor

so I assumed this cipher was enabled by default.

Well, unless I'm misreading what I'm seeing, it is enabled by default. On a fresh RHEL9 machine:

# openssl ciphers -s | sed "s/:/\n/g"  | grep DHE-RSA-AES256-GCM-SHA384
ECDHE-RSA-AES256-GCM-SHA384
DHE-RSA-AES256-GCM-SHA384

but it is pretty low on the list.

@asteflova
Copy link
Member Author

asteflova commented Jan 13, 2025

There is an error that users encounter when the cipher is enabled and used:

{ "error": { "message": "ERF77-7629 [Foreman::LdapException]: Error while connecting to 'server.com' LDAP server at 'ldap.example.com' during authentication ([Net::LDAP::Error]: Connection reset by peer - SSL_connect)" } }

@adamruzicka Do you think it would be better to turn this around and document the error? So, more or less: "If you hit the following error, make sure that this cipher is disabled." Would that be a more suitable approach than the note that this PR currently proposes?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think documenting exact errors is a good thing for troubleshooting. Sysadmins are trained in doing so (whether they know it or not). Today we mostly do so in KCS articles, but I think we should preempt errors if we know about them.

@adamruzicka
Copy link
Contributor

Do you think it would be better to turn this around and document the error?

In general yes, but I'm afraid you can get the same error in other scenarios so it is not 100% reliable.

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
@asteflova
Copy link
Member Author

Based on the feedback, I tried to come up with something that would help both prevent the issue and troubleshoot it. Can you please re-review? Is this helpful, or did I just create a horrible catdog and should just revert back to the note?

@asteflova asteflova force-pushed the ad-unsupported-cipher branch from 6f42072 to 1a6807a Compare January 13, 2025 18:00
@asteflova
Copy link
Member Author

@adamruzicka @ekohl @lhellebr Do you think that the current state is good enough? Is this something you feel comfortable approving?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think this is good. Long term I'd like to use th ERF error codes more. We used to maintain https://projects.theforeman.org/projects/foreman/wiki/ErrorCodes to make troubleshooting easier

@asteflova asteflova added the Needs style review Requires a review from docs style/grammar perspective label Jan 14, 2025
@asteflova asteflova marked this pull request as ready for review January 14, 2025 10:15
@asteflova asteflova added tech review done No issues from the technical perspective and removed Needs tech review Requires a review from the technical perspective labels Jan 15, 2025
@asteflova
Copy link
Member Author

Dear writers, can I get a style review?

@asteflova asteflova added style review done No issues from docs style/grammar perspective and removed Needs style review Requires a review from docs style/grammar perspective labels Jan 16, 2025
@asteflova asteflova merged commit 3be4471 into theforeman:master Jan 16, 2025
8 of 9 checks passed
asteflova added a commit that referenced this pull request Jan 16, 2025
---------

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
(cherry picked from commit 3be4471)
asteflova added a commit that referenced this pull request Jan 16, 2025
---------

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
(cherry picked from commit 3be4471)
asteflova added a commit that referenced this pull request Jan 16, 2025
---------

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
(cherry picked from commit 3be4471)
asteflova added a commit that referenced this pull request Jan 16, 2025
---------

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
(cherry picked from commit 3be4471)
asteflova added a commit that referenced this pull request Jan 16, 2025
---------

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
(cherry picked from commit 3be4471)
asteflova added a commit that referenced this pull request Jan 16, 2025
---------

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
(cherry picked from commit 3be4471)
asteflova added a commit that referenced this pull request Jan 16, 2025
---------

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
(cherry picked from commit 3be4471)
@asteflova
Copy link
Member Author

Merged to "master" and cherry-picked:

ba9c20b..96f477a 3.13 -> 3.13
b33fab5..fa8153f 3.12 -> 3.12
dd41db4..c01557a 3.11 -> 3.11
c95106c..2c776ff 3.10 -> 3.10
39e8361..4b3fe36 3.9 -> 3.9
1224de8..5be2f40 3.8 -> 3.8
f6d8f24..cd2be70 3.7 -> 3.7

@asteflova asteflova deleted the ad-unsupported-cipher branch January 16, 2025 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style review done No issues from docs style/grammar perspective tech review done No issues from the technical perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants