updated sssd with smart cards now brick systems without full cert chain

[Karl Grindley]

[ Impact ]

With the latest sssd release supporting OpenSSL PKI authentication for Ubuntu 20.04, the behavior between nssdb and OpenSSL has adversely affected many systems which are configured for PKI only authentication.

The NSSDB implementation of sssd/p11_child ONLY requires the issuing certificate to be populated to the nssdb and marked as trusted. While this may be considered a poorly configured system, it is still technically valid.

The OpenSSL implementation of the sssd/p11_child requires the FULL cert chain to the root cert (which is then also trusted by the system root chain) in order to allow a certificate to authenticate.

By upgrading to the latest packages, the conversion process from nssdb to the OpenSSL pam file fails to check the chain of trust, thereby creating a denial of service for some systems configured to require smart card/PKI authentication in the pam stack via pam_sss and require_cert_auth flag.

Note that this is a popular configuration due to many organizations are required to follow NIST 800-171 (and other) security derived policy. Often policy requires PKI based authentication to be enforced and all other authentication methods disabled.

[ Test case ]

Testing this fix in any system is complex because it depends on certificates with partial authentication, so ideally we should:

1. Configure SSSD to include an intermediate certificate for the smart card in use in
   /etc/sssd/pki/sssd_auth_ca_db.pem

2. Launch:
   sudo /usr/libexec/sssd/p11_child --pre -d 10 --debug-fd=2 \
     --nssdb=/etc/sssd/pki/sssd_auth_ca_db.pem

   And this should NOT return a certificate, then launch it with:

   sudo /usr/libexec/sssd/p11_child --pre -d 10 --debug-fd=2 \
     --nssdb=/etc/sssd/pki/sssd_auth_ca_db.pem --verify=partial_chain

   And this MUST return the card certificate.

Alternatively, you should try to login. Ensuring that /etc/sssd/sssd.conf contains:

[pam]
pam_cert_verification = partial_chain #or other_option, partial_chain

---

However, given that testing this is complex without specific hardware, I've setup a test case that automates all this, creating keyrings with partially trusted certificates and a software-generated smartcard (using softhsm2) so that this can be all tested easily, see:

https://gist.github.com/3v1n0/287d02ca8e03936f1c7bba992173d47a

So, basically you only have to:

 0. sudo apt install gnutls-bin openssl softhsm2 && \
    sudo apt-mark auto gnutls-bin openssl softhsm2
 1. wget https://gist.githubusercontent.com/3v1n0/287d02ca8e03936f1c7bba992173d47a/raw/sssd-softhism2-certificates-tests.sh
 2. sudo bash sssd-softhism2-certificates-tests.sh
    (sudo can be avoided by copying /usr/libexec/sssd/p11_child to an user
    local path and calling the script with
    SSSD_P11_CHILD=$HOME/path/to/p11_child env variable)
 3. Ensure that "Test completed, Root CA and intermediate issued certificates verified!"
    is printed and the script returns properly

This will:
 - Generate a test Root Certificate Authority (and will emit a cert from it)
 - Generate a test Intermediate Certificate Authority (and will emit a cert)
 - Generate a test Sub Intermediate Certificate Authority (and will emit a cert)
 - Test the certificates themselves with openssl
 - For each certificate will create various fake smartcards
 - Will test each smartcard how it behaves when used via p11_child with both
   partial and full verification, and doing full p11_child authentication.

Before to this SRU, the script fails with this error:

(Thu Jan 26 04:36:16:676491 2023) [p11_child[257107]] [read_certs] (0x4000): found cert[Test Organization Intermediate Trusted Certificate 0001][/O=Test Organization/OU=Test Organization Unit/CN=Test Organization Intermediate Trusted Certificate 0001]
(Thu Jan 26 04:36:16:676970 2023) [p11_child[257107]] [do_verification] (0x0040): X509_verify_cert failed [0].
(Thu Jan 26 04:36:16:677197 2023) [p11_child[257107]] [do_verification] (0x0040): X509_verify_cert failed [2][unable to get issuer certificate].
(Thu Jan 26 04:36:16:677438 2023) [p11_child[257107]] [read_certs] (0x0040): Certificate [Test Organization Intermediate Trusted Certificate 0001][/O=Test Organization/OU=Test Organization Unit/CN=Test Organization Intermediate Trusted Certificate 0001] not valid, skipping.
(Thu Jan 26 04:36:16:677709 2023) [p11_child[257107]] [do_card] (0x4000): No certificate found.
+ grep -qs 00112233445566778899FFAABBCCDDEEFF012345 /tmp/sssd-softhsm2-sGxAXC/SSSD-child-2678.output
+ return 2
+ echo 'Unexpected failure!'

[ Regression potential ]

SSSD p11_child functionalities did not change by default and they're now strictly tested (they were not fully before this SRU).

However we may set some systems to use a weaker auth mode for PAM authentication with smart cards, but this is still a secure mode.

[Karl Grindley (karlg100)]

this was caused by the changes in https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1905790

[Robie Basak (racb)]

Karl, thank you for your report.

We now need to decide an appropriate course of action here. An update to sssd to revert the change is a possibility, but there's also risk there that we will break users twice.

Do you have a deployment affected by this? How many other users might be affected by this? In bug 1905790, our assessment was that it was unlikely that anyone was using NSS+sssd -based smartcards on 20.04. Was this assessment wrong? What's the real world impact here?

[Marco Trevisan (Treviño) (3v1n0)]

Could you maybe provide an example setup we can reuse to simulate such scenario?

[Karl Grindley (karlg100)]

To speak to real world assessment here - there's a big push across many (US) gov't orgs and industry to deploy MFA. These requirements are not new, but many have not been enforced due to lack of compliance checks/certifications.

This is changing with new efforts in the US Gov't industry circles with regard to CMMC. This is an assessment/certification that industry must meet and maintain for contactual compliance, starting to roll out in the next year or so.

Likewise there's been a lot of focus lately on unclassified compliance with NIST policy. We have a number of customers, working toward or maintaining an MFA solution. All are struggling.

Many have lagged with pam_pkcs11 providing/satisfying most compliance requirements. But with RHEL8 and Ubuntu 20.04 adoption underway (with RHEL6 and 14.04 end of life) many are stuck working to cobble together an implementation.

Of course with the uptick in remote work, MFA has also resurged, also pushing along adoption of sssd MFA.

We noticed with the latest round of patching something was a-miss. and today tracked it down to this change. We're working with our customers to come up with a workaround.

I think there's a larger number of folks impacted here, but unfortunately, the number of possible ways to do MFA is very large, and because no one maintainer has completely documented/supported MFA well, sysadmins typically develop their MFA craft using what they can.

I don't discourage this change, in fact, will help push along the MFA adoption.

However, I think perhaps some preflight checks in the package could solve someone bricking their machine. (or a large quantity of machines). I'd also suggest that MFA support in general should be considered a core requirement for future versions of the LTS, and well tested, supported and documented. Adoption will only grow with time, and become more critical. This will help reduce the variations of implementations, and help drive folks to a known and supported configuration.

Reproduction of the issue:
In our circles, we see a fully Microsoft AD integrated Smartcard (with kerberos and PKINIT) implementation. This also bleeds over into pam_sss configuration issues with U20.04, (for which I should file another ticket)

Based on my diagnosis today, I think this is isolated to p11_child, and those with a nssdb with only issuing CA certs populated in the database. I don't think this issue matters for which directory is being used and if PKINIT is functioning, since all the MFA magic happens within p11_child.

I'm going to assume that you folks have some way test AD with MFA, and will try to summarize.

To reproduce, you'll need (at least) a 2 tier CA PKI chain. Root -> Issuing CA -> End user cert

(with old sssd version) configure for smart card auth
* do as you always do to join/setup sssd to a directory service
* verify user ID lookups, and login works as expected with password
* add any mapping/filter rules to the /etc/sssd/sssd.conf for p11_child
* upadte /usr/share/pam-configs/sss to Priority 800, rebuild pam stack, dpkg-divert /usr/share/pam-configs/sss
* add the root and issuing certs to /usr/local/share/ca-certificates, rebuild system trust st...

[Marco Trevisan (Treviño) (3v1n0)]

So, if I didn't get it wrong, if we'd just use /etc/ssl/certs/ca-certificates.crt as the SSSD pam certificate in such case would work?

I mean having this in /etc/sssd/sssd.conf

[pam]
pam_cert_db_path = /etc/ssl/certs/ca-certificates.crt

And then what was into /etc/sssd/pki/sssd_auth_ca_db.pem to be added to .crt's under /usr/local/share/ca-certificates/sssd_auth_ca_db/ and eventually calling update-ca-certificates maybe?

We could even do the other way around probably, by adding an hook to /etc/ca-certificates/update.d/ so that we ensure that /etc/ssl/certs/ca-certificates.crt is always in sync with the system ring?

As Robie said, we could revert this change but this would not be ideal for various reasons IMHO:
 1. As you said this is going to be used more and more, and so we'll have to end up to keep supporting
    a growing number of systems with an outdated method that is going to be dropped in future
    (i.e. better to do it now that its usage is limited than having to do it in future when the audience
     is bigger)
 2. We would like to have a single documented method to have smartcard auth in ubuntu using SSSD that can
    be validated from 20.04 onward and that keep working in future LTSs (and for sure next LTS will have to drop
    NSS anyways, so it's just about delaying a problem making it bigger).

[Robie Basak (racb)]

Karl, thank you for the detailed writeup? This looks very useful. I'll
leave Marco to respond as he drove the change in question, but a couple
of less technical comments:

On Thu, Mar 18, 2021 at 01:16:28AM -0000, Karl Grindley wrote:
> I don't discourage this change, in fact, will help push along the MFA
> adoption.

[I rearranged ordering of your sentences a bit for context]

> I'd also suggest that MFA support in general should be considered a core
> requirement for future versions of the LTS, and well tested, supported
> and documented. Adoption will only grow with time, and become more
> critical. This will help reduce the variations of implementations, and
> help drive folks to a known and supported configuration.

Thank you for the support! I believe this was exactly Marco's intention.

> However, I think perhaps some preflight checks in the package could
> solve someone bricking their machine. (or a large quantity of machines).

It sounds to me that there's some scope for improvement then, if that
can be figured out between you and Marco, and that a revert isn't
required.

So to ensure there's no misunderstanding about expectations about this
bug, the way I see it now is that we're going to keep what we have.
Marco will (presumably) consider your suggestion and that might lead to
a further upload to add some further sanity checks depending on how the
details pan out. We can use this bug to track and communicate about
that.

Does that work for everyone?

[Robie Basak (racb)]

> Karl, thank you for the detailed writeup?

That was intended to say:

Karl, thank you for the detailed writeup!

[Karl Grindley (karlg100)]

I agree and concur.

Just need some checks here, as this is a pretty big change in behavior for a mid-life LTS release.

That said, the new configuration is in line with RHEL8, and will help reduce the configuration scope for a working solution.

I'll also comment, (and perhaps a bit of scope creap, but...) we've found a number of unfixed issues with sssd, specifically with PKINIT and LDAP optimizations. We're working with the upstream maintainers to help address these. We would like to see these brought into 20.04 LTS, as all directory users can benefit here. Are you or Marco the best to help us bring these into a general release down the road?

[Marco Trevisan (Treviño) (3v1n0)]

Karl, I'm the responsible for what it concerns this specific change and so feel free to use me a reference for this (maybe ping me on IRC so we can have some better interaction regarding the best solution we can take).

However, the SSSD maintainer in ubuntu is Sergio Durigan Junior, so he's the one can help you better with the other relevant issues.

[Robie Basak (racb)]

On Thu, Mar 18, 2021 at 02:14:23AM -0000, Karl Grindley wrote:
> I'll also comment, (and perhaps a bit of scope creap, but...) we've
> found a number of unfixed issues with sssd, specifically with PKINIT and
> LDAP optimizations. We're working with the upstream maintainers to help
> address these. We would like to see these brought into 20.04 LTS, as
> all directory users can benefit here. Are you or Marco the best to help
> us bring these into a general release down the road?

Your assistance in getting fixes landed would be most welcome!

We do have to take great care to minimise risk of regression to existing
users of course, as you've very well demonstrated in this bug :)

Our process for getting fixes in is documented at
https://wiki.ubuntu.com/StableReleaseUpdates. Assuming that based on
that policy and the specifics of the fixes, cherry-picks are most
appropriate, the main steps would be to have a bug for each issue, get
the Ubuntu development release fixed, and then prepare an update for
20.04. https://wiki.ubuntu.com/StableReleaseUpdates#Procedure has the
details. You can do most of this yourself, and we'd be happy to help
with sponsorship and any rough edges in the process.

You can reach us on the ubuntu-devel@ mailing list
(https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel) or
#ubuntu-devel on Freenode on IRC.

[Karl Grindley (karlg100)]

> On Mar 17, 2021, at 10:01 PM, Marco Trevisan (Treviño) <email address hidden> wrote:
>
> So, if I didn't get it wrong, if we'd just use /etc/ssl/certs/ca-
> certificates.crt as the SSSD pam certificate in such case would work?

While this would technically work, it would be really bad news. This would allow anyone with any user cert issued by a CA in the system wide cert store (by any CA in the world) to be trusted and pass authorization checks by p11_child. (Albeit, some directory attributes would have to line up, depending on your match rules)

You only want the full chain of the issuing, intermediate and root CA(s) that are authorized to issue certs to users.

Really, going to OpenSSL is a bit of a downgrade. Nssdb, allows one to flag which certs you want to trust, and which certs you don't. There’s no way that I’m aware of to flag a cert in the /etc/sssd/pki/sssd_auth_ca_db.pem to say I trust this cert, but not this other cert.

Perhaps the right fix would be that p11_child is updated upstream to use the system cert store to complete chain of trust, and only the authorized issuing CAs should be put in the /etc/sssd/pki/sssd_auth_ca_db.pem file. Perhaps Summit would be willing to add this config option, for which could be set to default to the system store in Ubuntu in the Ubuntu packages.

This still assumes a user has properly setup the system store to trust corporate root CAs (and intermediate, since there’s no way to differentiate between trusted root vs untrusted supporting certs in the system store in Ubuntu)

> I mean having this in /etc/sssd/sssd.conf
>
> [pam]
> pam_cert_db_path = /etc/ssl/certs/ca-certificates.crt
>
> And then what was into /etc/sssd/pki/sssd_auth_ca_db.pem to be added to
> .crt's under /usr/local/share/ca-certificates/sssd_auth_ca_db/ and
> eventually calling update-ca-certificates maybe?
>
> We could even do the other way around probably, by adding an hook to
> /etc/ca-certificates/update.d/ so that we ensure that /etc/ssl/certs/ca-
> certificates.crt is always in sync with the system ring?
>
>
> As Robie said, we could revert this change but this would not be ideal for various reasons IMHO:
> 1. As you said this is going to be used more and more, and so we'll have to end up to keep supporting
> a growing number of systems with an outdated method that is going to be dropped in future
> (i.e. better to do it now that its usage is limited than having to do it in future when the audience
> is bigger)
> 2. We would like to have a single documented method to have smartcard auth in ubuntu using SSSD that can
> be validated from 20.04 onward and that keep working in future LTSs (and for sure next LTS will have to drop
> NSS anyways, so it's just about delaying a problem making it bigger).

Agreed. We just need something to prevent a user from bricking a system by just applying updates from upstream. This is kind of a high priority issue, given folks I’m sure are upgrading daily. By now this issue is starting to creep into offline synced repos.

Karl

[Marco Trevisan (Treviño) (3v1n0)]

> While this would technically work, it would be really bad news. This would allow anyone with any user cert issued by a CA in the system wide cert store (by any CA in the world) to be trusted and pass authorization checks by p11_child. (Albeit, some directory attributes would have to line up, depending on your match rules)

Well, that's just partially true since as you said:
 - Without a match rule (that has to be configured) there's no access anyways

However as I was saying, maybe the other way around can be safer?
I mean, SSSD will still use /etc/sssd/pki/sssd_auth_ca_db.pem for the trusted certs, but we will populate it adding also the ones trusted by the system.

Maybe providing a way to filter them out.

I'm talking only of upgrades from NSS installs though, for new installations people will have to manually add their trusted CAs to /etc/sssd/pki/sssd_auth_ca_db.pem.

The point here is, I suppose, that if the system trusts a CA, then we can't just not trust it for some specific operation, this can be still filtered out (if needed) by using proper sssd config parameters.

[Dimitri John Ledkov (xnox)]

I think that is a long standing openssl bug that it demands full chains, and more so it trips up not only when the chain is incomplete, but also where there are extra chains, which are redundant; and if any of them have untrusted certs, or certs of small sizes / old hashes (aka legacy chains) it also refuses to establish connections.

[Dimitri John Ledkov (xnox)]

Re: certs.

Ideally we should be shipping a bundle of certificates, which are well known roots of trust for smarcards. Aka the DOD, National ID cards/passports, etc. In a new path locations.

Because the smartcard roots of trusts are not the same as for https:// connections.

But that's not immediately fixable.

[Dimitri John Ledkov (xnox)]

re conversion / upgrades => we should really find the full chain if we can to inject it into openssl.

I'm not sure if there are any ways to force openssl to be happy with trusted issuer without a full chain.

I would have thought there is a way to make openssl do that.

[Marco Trevisan (Treviño) (3v1n0)]

So, I've done some work on SSSD upstream to make this to happen: https://github.com/SSSD/sssd/pull/5558

With that we'll just be able to set on upgraders the option `certification_verification = partial_chain`, and this will just make the SSSD's PEM ring to work as the NSS db used to work: and so verify a certificate if its only its issuer is in the SSSD's CA certificates DB.

This comes with unit tests covering the case with generated certificates, not sure if I can personally test this with real hardware (for SRU purposes) though... We may still need to simulate it.

At the end, it's just as doing:
  openssl verify -partial_chain -CAfile intermediate_CA.pem intermediate_CA_issued_cert.pem

Karl, will this be enough for you?

[Karl Grindley (karlg100)]

Marco,

Great! This should be easy for me to test, and I’d be happy to do so.

I may be able to do a regression test to make sure the automated NSSDB -> openssl upgrade works as well. This would mean however that the upgrade would need to drop the appropriate sssd.conf.d to configure the partial_chain config option on upgrade.

I assume partial_chain will work even if the full chain is present?

Karl

> On Mar 28, 2021, at 4:15 PM, Marco Trevisan (Treviño) <email address hidden> wrote:
>
> So, I've done some work on SSSD upstream to make this to happen:
> https://github.com/SSSD/sssd/pull/5558
>
> With that we'll just be able to set on upgraders the option
> `certification_verification = partial_chain`, and this will just make
> the SSSD's PEM ring to work as the NSS db used to work: and so verify a
> certificate if its only its issuer is in the SSSD's CA certificates DB.
>
> This comes with unit tests covering the case with generated
> certificates, not sure if I can personally test this with real hardware
> (for SRU purposes) though... We may still need to simulate it.
>
> At the end, it's just as doing:
> openssl verify -partial_chain -CAfile intermediate_CA.pem intermediate_CA_issued_cert.pem
>
> Karl, will this be enough for you?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1919563
>
> Title:
> updated sssd with smart cards now brick systems without full cert
> chain
>
> Status in sssd package in Ubuntu:
> New
>
> Bug description:
> With the latest sssd release supporting OpenSSL PKI authentication for
> Ubuntu 20.04, the behavior between nssdb and OpenSSL has adversely
> affected many systems which are configured for PKI only
> authentication.
>
> The NSSDB implementation of sssd/p11_child ONLY requires the issuing
> certificate to be populated to the nssdb and marked as trusted. While
> this may be considered a poorly configured system, it is still
> technically valid.
>
> The OpenSSL implementation of the sssd/p11_child requires the FULL
> cert chain to the root cert (which is then also trusted by the system
> root chain) in order to allow a certificate to authenticate.
>
> By upgrading to the latest packages, the conversion process from nssdb
> to the OpenSSL pam file fails to check the chain of trust, thereby
> creating a denial of service for some systems configured to require
> smart card/PKI authentication in the pam stack via pam_sss and
> require_cert_auth flag.
>
> Note that this is a popular configuration due to many organizations
> are required to follow NIST 800-171 (and other) security derived
> policy. Often policy requires PKI based authentication to be enforced
> and all other authentication methods disabled.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1919563/+subscriptions

[Marco Trevisan (Treviño) (3v1n0)]

Yeah, sure...

As per man page:

  -partial_chain
           Allow verification to succeed even if a complete chain cannot be built to a self-signed trust-anchor,
           provided it is possible to construct a chain to a trusted certificate that might not be self-signed.

And you can test it quite easily with the attached generated certs using:

  openssl verify [-partial_chain] \
    -CAfile test_CA/intermediate_CA/SSSD_test_intermediate_CA.pem \
    test_CA/intermediate_CA/SSSD_test_intermediate_CA_cert_x509_0001.pem

While when using -partial_chain will only match when using test_CA/intermediate_CA/SSSD_test_intermediate_CA_full_db.pem as CAfile

[Marco Trevisan (Treviño) (3v1n0)]

when *not* using -partial_chain

[Marco Trevisan (Treviño) (3v1n0)]

Sorry, it took a while, but this should be handled now.

You can see https://launchpad.net/~3v1n0/+archive/ubuntu/gnome-smartcard-support-20.04/ for some test packages

[Andreas Hasenack (ahasenack)]

I'm reviewing this for SRU, and am currently looking at the sssd-common.postinst changes. One question came to mind: what about config file snippets that can exist in /etc/sssd/conf.d?

From the sssd.conf(5) manpage:

CONFIGURATION SNIPPETS FROM INCLUDE DIRECTORY
       The configuration file sssd.conf will include configuration snippets using the include directory conf.d. This feature is available if SSSD was
       compiled with libini version 1.3.0 or later.

       Any file placed in conf.d that ends in “.conf” and does not begin with a dot (“.”) will be used together with sssd.conf to configure SSSD.

       The configuration snippets from conf.d have higher priority than sssd.conf and will override sssd.conf when conflicts occur. If several snippets are
       present in conf.d, then they are included in alphabetical order (based on locale). Files included later have higher priority. Numerical prefixes
       (01_snippet.conf, 02_snippet.conf etc.) can help visualize the priority (higher number means higher priority).

I note that focal has libini-config5 0.6.1-2 (and sssd-common depends on it), so does that mean sssd in focal does not support that conf.d directory because the manpage cites 1.3.0? I have a feeling they may be talking about a different libini, because even in mantic, libini-config5 is just at 0.6.2-1.

[Andreas Hasenack (ahasenack)]

I'll add other comments here, one per LP bug comment

a) get_config_value() will fail if the config value has "=", like ldap DNs. For example, "ldap_search_base = dc=example,dc=com".

This is fine in this upload because all the keys that this function is given, none contain such values.

[Andreas Hasenack (ahasenack)]

b) disable_setting() doesn't check if the sssd config file exists, and instead relies on the || true to never fail:

 disable_setting()
 {
     echo "Disabling sssd.conf setting using invalid value: '$1'"
- sed -i 's/^[^#;]*'"$1"'\b/#&/' /etc/sssd/sssd.conf || true
+ sed -i 's/^[^#;]*'"$1"'\b/#&/' "$SSSD_CONF" || true
 }

That will likely produce an ugly "sed: can't read <file>: No such file or directory" in stderr, though. Since this function is being updated to use $SSSD_CONF, could it perhaps also be updated to check for the conf file?

I checked callers, and maybe it would never be called if there is no sssd.conf file present. The callers are gated on the upgrade logic that starts with:

  if dpkg --compare-versions "$2" lt-nl 2.2.3-3ubuntu0.4; then

and then basically the existence of /etc/pki/nssdb. If this is true, then I guess it's ok to not have to check for the existence of sssd.conf, but if not, or if we have to request another upload of this package due to another reason, it would be good to add that simple check to disable_setting() to avoid a possible error message.

[Andreas Hasenack (ahasenack)]

c) replace_setting()

c1) if a setting is disabled by this sed:
sed -i '/^\['"$1"'\]$/,/^\[/ s/^[^#;]*'"$2".*'\b/#& # disabled by dpkg\n'"$setting"'/g' "$SSSD_CONF" || true

That is also adding a blank line at the end (see the last \n) and breaks a bit the flow of the config file. For example, for kicks, I disabled "config_file_version" in [sssd], and the section became:

[sssd]
#config_file_version = 2 # disabled by dpkg

services = nss, pam
domains = LDAP # here is the domain

Just esthetics. If we happen to get another upload, then this \n could be dropped, unless it's handling a case I haven't seen.

c2) this function will fail if the section name ([name]) contains a "/", which is quite common:

[sssd]
config_file_version = 2
services = nss, pam
domains = LDAP

[domain/LDAP]
id_provider = ldap
ldap_uri = ldap://localhost
cache_credentials = True
ldap_search_base = dc=example,dc=com

That's not the case in any of the uses in postinst, however, so no need to complicate it now to fix a problem that hasn't happened yet.

I was wondering if the certificate_verification and pam_cert_verification could appear inside a [domain/] section, but looks like that's not a valid config.

[Andreas Hasenack (ahasenack)]

These look OK so far. I'll focus next on upgrade sccenarios, and check in detail your test script to see what is covered.

[Andreas Hasenack (ahasenack)]

I'll continue this review today, but wanted to highlight this issue:

> One question came to mind: what about config file snippets that can exist in /etc/sssd/conf.d?

I think this needs consideration. What are your thoughts on it, Marco?

[Marco Trevisan (Treviño) (3v1n0)]

So... I indeed was considering the config snippets but the versioning tricked me.

I don't think that libini 1.3.0 was ever released as such... In fact that's https://github.com/SSSD/ding-libs/tree/master/ini

But looking at the packaging in fedora, it seems they're hardcoded such version there: https://src.fedoraproject.org/rpms/ding-libs/blob/rawhide/f/ding-libs.spec#_17

And then yes, I think we should adapt the code to also check the snippets :(

Thanks for the through checks you did on your review.

As per a) and c2), I wanted to keep things as simplest as possible without worrying too much about handling edge cases that are not valid for our scenario. Indeed having something better than posix sh would have been nice... Indeed one option could be even using the very same libini to write a simple c tool that does it instead of relying on more complex shell code.

I'm fine with b) and c). Indeed IIRC they were not a big deal in my tests nor causing any error msg, but I may have missed a case.