Change ldap_id_use_start_tls default to True #6963

justin-stephenson · 2023-09-27T19:43:27Z

No description provided.

sumit-bose · 2023-09-28T06:25:30Z

Hi,

I wonder if we can call the new option value allow or even allow_fallback instead of try to be better aligned the option values of ldap_tls_reqcert which we took from OpenLDAP's configuration.

bye,
Sumit

justin-stephenson · 2023-09-28T15:08:10Z

Hi,

I wonder if we can call the new option value allow or even allow_fallback instead of try to be better aligned the option values of ldap_tls_reqcert which we took from OpenLDAP's configuration.

bye, Sumit

Yes no problem, I will change it.

justin-stephenson · 2023-10-02T20:40:26Z

associated PR for sssd-2-9 branch to change default to "allow" is here #6968

alexey-tikhonov · 2023-10-03T06:26:22Z

Please add 'resolves #6681'

justin-stephenson · 2023-10-03T13:32:33Z

Please add 'resolves #6681'

Done.

sumit-bose · 2023-10-06T08:26:59Z

Hi,

wouldn't it be easier to add a fourth patch to switch from allow to true #6968, commit all 4 to master and backport only the first 3?

bye,
Sumit

alexey-tikhonov · 2023-10-06T15:32:49Z

Both PRs lacks RNs in commit messages (:config: at the very least).

For this reason / once added,

wouldn't it be easier to add a fourth patch to switch from allow to true #6968, commit all 4 to master and backport only the first 3?

-- I don't think this is a good idea as we will have to conflicting (and thus confusing) RNs from ldap: Switch ldap_id_use_start_tls default to... commits.
I think backporting 2 main patches and having different 'change default value' patch is reasonable. Or do I miss anything?

And also PRs need a rebase.

alexey-tikhonov · 2023-10-06T15:37:48Z

src/tests/intg/test_infopipe.py

@@ -213,6 +213,7 @@ def format_basic_conf(ldap_conn, schema, config):
        ldap_search_base    = {ldap_conn.ds_inst.base_dn}
        ldap_user_extra_attrs = extraName:uid
        ldap_user_certificate = userCert
+        ldap_id_use_start_tls = allow


Why is this needed?
LDAP server used in tests doesn't support TLS?

alexey-tikhonov · 2023-10-06T15:56:42Z

Do we need 'allow' variant in 2.10+ at all?

Wouldn't it be sufficient for 2.10+ to merely change default to 'true' and to be done with it?

I mean, 'allow' only has meaning as a default value. There is no reason anyone would like to configure it explicitly (like, "I don't know if my server supprts TLS so let's try"?).

Having 'allow' variant with default set to 'true' doesn't make sense, imo.

Of course, it's a bit weird that 2.9.3 will introduce a new variant that will disappear in 2.10+. But it could be explained in RNs that this is a transitional option only to avoid breaking existing setups and it will disappear soon.

sumit-bose · 2023-10-06T16:25:46Z

Do we need 'allow' variant in 2.10+ at all?

Wouldn't it be sufficient for 2.10+ to merely change default to 'true' and to be done with it?

I mean, 'allow' only has meaning as a default value. There is no reason anyone would like to configure it explicitly (like, "I don't know if my server supprts TLS so let's try"?).

Having 'allow' variant with default set to 'true' doesn't make sense, imo.

Of course, it's a bit weird that 2.9.3 will introduce a new variant that will disappear in 2.10+. But it could be explained in RNs that this is a transitional option only to avoid breaking existing setups and it will disappear soon.

Hi,

then we should make sure that allow cannot be set manually and is only internally used as a default. Additionally the sssd-2.9.x man page should say that the default will change to True in the next major release. And maybe we should consider a Fedora change for this as well to make sure it is properly announced.

bye,
Sumit

alexey-tikhonov · 2023-10-06T18:31:10Z

Do we need 'allow' variant in 2.10+ at all?
Wouldn't it be sufficient for 2.10+ to merely change default to 'true' and to be done with it?
I mean, 'allow' only has meaning as a default value. There is no reason anyone would like to configure it explicitly (like, "I don't know if my server supprts TLS so let's try"?).
Having 'allow' variant with default set to 'true' doesn't make sense, imo.
Of course, it's a bit weird that 2.9.3 will introduce a new variant that will disappear in 2.10+. But it could be explained in RNs that this is a transitional option only to avoid breaking existing setups and it will disappear soon.

Hi,

then we should make sure that allow cannot be set manually

Makes sense.

and is only internally used as a default. Additionally the sssd-2.9.x man page should say that the default will change to True in the next major release. And maybe we should consider a Fedora change for this as well to make sure it is properly announced.

Another thing to consider: what is the plan for C9S/RHEL9?
C9S will pick sssd-2.10+ eventually.
If it can't switch to 'true' as default, then we need 'allow' in 2.10+ releases and C9S will have to use downstream patch to set default to 'allow'. But I'd prefer to avoid this if possible.

alexey-tikhonov · 2023-10-06T19:52:22Z

And maybe we should consider a Fedora change for this as well to make sure it is properly announced.

Imo, it doesn't justify a Change process, RNs would be enough.

Moreover, F40 should (hopefully) get sssd-2.10
F39 can get sssd-2.9.3+, and it's of course way too late for a Change. But it's a proper time for F39 RNs.

justin-stephenson · 2023-10-09T19:38:16Z

Another thing to consider: what is the plan for C9S/RHEL9? C9S will pick sssd-2.10+ eventually. If it can't switch to 'true' as default, then we need 'allow' in 2.10+ releases and C9S will have to use downstream patch to set default to 'allow'. But I'd prefer to avoid this if possible.

Downstream patch enabling 'allow' is the only viable option if we want to change the default from 'false' in RHEL9 however, yes? It seems too disruptive to switch from 'false' to 'true' within a major release. Otherwise we leave it as 'false' and only makes these changes to improve security for RHEL10+.

I don't have a strong opinion either way i'll implement it whichever way we all decide.

All communication, including the identity provided must be encrypted to prevent attacks. Resolves: SSSD#6681

src/tests/multihost/basic/test_ldapapi.py

alexey-tikhonov · 2023-10-17T17:06:43Z

CodeQL warning looks a valid one.
Also please add RN.

:relnote: Default `ldap_id_use_start_tls` value changed from `false` to `true` for improved security. Resolves: SSSD#6681

sumit-bose

Hi,

thanks for the update and your patience, ACK.

bye,
Sumit

pbrezina · 2023-10-18T13:29:50Z

Pushed PR: #6963

master
- 847aa71 - ldap: Switch ldap_id_use_start_tls default to True
- 45e06b7 - man: Improve LDAP security wording

justin-stephenson force-pushed the start_tls_try_default_master branch from 446c142 to e2bb694 Compare September 28, 2023 15:17

justin-stephenson changed the title ~~Change ldap_id_use_start_tls default to True and add "try" option value~~ Change ldap_id_use_start_tls default to True and add "allow" option value Sep 29, 2023

justin-stephenson force-pushed the start_tls_try_default_master branch 7 times, most recently from f530139 to 8c41682 Compare October 2, 2023 18:04

justin-stephenson mentioned this pull request Oct 2, 2023

Improve security wording for ldap_id_use_start_tls option #6968

Closed

justin-stephenson marked this pull request as ready for review October 2, 2023 20:41

justin-stephenson added the Waiting for review label Oct 2, 2023

andreboscatto requested a review from alexey-tikhonov October 3, 2023 12:34

andreboscatto assigned alexey-tikhonov Oct 3, 2023

andreboscatto requested a review from sumit-bose October 3, 2023 12:35

andreboscatto assigned sumit-bose Oct 3, 2023

justin-stephenson force-pushed the start_tls_try_default_master branch from 8c41682 to af083b2 Compare October 3, 2023 13:32

justin-stephenson force-pushed the start_tls_try_default_master branch from af083b2 to e80ca15 Compare October 5, 2023 13:45

alexey-tikhonov reviewed Oct 6, 2023

View reviewed changes

alexey-tikhonov added the no-backport This should go to target branch only. label Oct 6, 2023

man: Improve LDAP security wording

b3d96b8

All communication, including the identity provided must be encrypted to prevent attacks. Resolves: SSSD#6681

justin-stephenson force-pushed the start_tls_try_default_master branch from e80ca15 to adda7b7 Compare October 17, 2023 15:49

github-advanced-security bot found potential problems Oct 17, 2023

View reviewed changes

src/tests/multihost/basic/test_ldapapi.py Fixed Show fixed Hide fixed

justin-stephenson changed the title ~~Change ldap_id_use_start_tls default to True and add "allow" option value~~ Change ldap_id_use_start_tls default to True Oct 17, 2023

alexey-tikhonov added Changes requested and removed Waiting for review labels Oct 17, 2023

ldap: Switch ldap_id_use_start_tls default to True

cb39201

:relnote: Default `ldap_id_use_start_tls` value changed from `false` to `true` for improved security. Resolves: SSSD#6681

justin-stephenson force-pushed the start_tls_try_default_master branch from adda7b7 to cb39201 Compare October 17, 2023 18:00

alexey-tikhonov approved these changes Oct 17, 2023

View reviewed changes

sumit-bose approved these changes Oct 18, 2023

View reviewed changes

sumit-bose added Accepted and removed Changes requested labels Oct 18, 2023

pbrezina added the Ready to push Ready to push label Oct 18, 2023

pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Oct 18, 2023

pbrezina closed this Oct 18, 2023

alexey-tikhonov mentioned this pull request Oct 18, 2023

Default hardening - id_provider channel defaults unencrypted with starttls #6681

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change ldap_id_use_start_tls default to True #6963

Change ldap_id_use_start_tls default to True #6963

justin-stephenson commented Sep 27, 2023

sumit-bose commented Sep 28, 2023

justin-stephenson commented Sep 28, 2023

justin-stephenson commented Oct 2, 2023

alexey-tikhonov commented Oct 3, 2023

justin-stephenson commented Oct 3, 2023

sumit-bose commented Oct 6, 2023

alexey-tikhonov commented Oct 6, 2023

alexey-tikhonov Oct 6, 2023

alexey-tikhonov commented Oct 6, 2023 •

edited

Loading

sumit-bose commented Oct 6, 2023

alexey-tikhonov commented Oct 6, 2023

alexey-tikhonov commented Oct 6, 2023 •

edited

Loading

justin-stephenson commented Oct 9, 2023

alexey-tikhonov commented Oct 17, 2023

sumit-bose left a comment

pbrezina commented Oct 18, 2023

Change ldap_id_use_start_tls default to True #6963

Change ldap_id_use_start_tls default to True #6963

Conversation

justin-stephenson commented Sep 27, 2023

sumit-bose commented Sep 28, 2023

justin-stephenson commented Sep 28, 2023

justin-stephenson commented Oct 2, 2023

alexey-tikhonov commented Oct 3, 2023

justin-stephenson commented Oct 3, 2023

sumit-bose commented Oct 6, 2023

alexey-tikhonov commented Oct 6, 2023

alexey-tikhonov Oct 6, 2023

Choose a reason for hiding this comment

alexey-tikhonov commented Oct 6, 2023 • edited Loading

sumit-bose commented Oct 6, 2023

alexey-tikhonov commented Oct 6, 2023

alexey-tikhonov commented Oct 6, 2023 • edited Loading

justin-stephenson commented Oct 9, 2023

alexey-tikhonov commented Oct 17, 2023

sumit-bose left a comment

Choose a reason for hiding this comment

pbrezina commented Oct 18, 2023

alexey-tikhonov commented Oct 6, 2023 •

edited

Loading

alexey-tikhonov commented Oct 6, 2023 •

edited

Loading