[SSSD] [sssd PR#5558][comment] p11_child: Add partial verification support

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

sumit-bose commented:
"""
Hi,

thank you for the patches and especially for the extensive tests. I think both `partial_chain` and `pam_cert_verification` are useful. I will run some tests, but at a first glace you patches are looking quite complete.

Recently I came across a similar issue with respect to a CRL check. Currently there is

X509_VERIFY_PARAM_set_flags(verify_param, (X509_V_FLAG_CRL_CHECK
| X509_V_FLAG_CRL_CHECK_ALL));

and here `X509_V_FLAG_CRL_CHECK_ALL` enforces a CRL check of the whole chain, i.e. you need the CRL of each CA in the chain. I wonder if `partial_chain` should have an effect on the CRL check as well or if it would be better to have a separate option to toggle the `X509_V_FLAG_CRL_CHECK_ALL` flag?

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-809194963

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5558/head:pr5558
git checkout pr5558

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5558/head:pr5558
git checkout pr5558

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

sumit-bose commented:
"""
Hi,

can you add something like
```
diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c
index ab47e2986..8d69eaf07 100644
--- a/src/tests/cmocka/test_pam_srv.c
+++ b/src/tests/cmocka/test_pam_srv.c
@@ -290,6 +290,7 @@ static int pam_test_setup(void **state)

struct sss_test_conf_param pam_params[] = {
{ "p11_child_timeout", "30" },
+ { "pam_cert_verification", NULL },
{ NULL, NULL }, /* Sentinel */
};

```
to make sure all tests start with unset `pam_cert_verification`?

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-810218411

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: edited

Changed field: body
Original value:
"""
From the main commit:

<blockquote>
As per the switch to libcrypto by default, the CA certificates DB needs
to contain the whole certificates key-chain in order to verify a leaf
certificate. This means that if an intermediate CA authority signed a
leaf certificate the CA DB we provide to SSSD needs to contain the whole
key-chain, up to the root CA cert in order to verify the leaf one.

Now, while this is indeed more secure, it may break previous
configurations that were based on an NSS database that contained only
trusted intermediate CA certificates.

To allow such setups to continue working (once the NSS db is migrated)
we need to permit a "weaker" setup where an x509 certificate is verified
when the CA database we test against contains only the intermediate CA
certificate that was used to sign it.

As per this, support `partial_chain` value to be used as
`certification_verification` parameter that will add the
`X509_V_FLAG_PARTIAL_CHAIN` verify param flag to the store, as the
openssl's verify `-partial-chain` parameter works.

This setup can still be considered secure as it's still needed to have
configured the SSSD ca db to contain the trusted certs.

Add tests to check that we can verify a leaf certificate against its
parent (only) when using such option.
</blockquote>

In particular in Ubuntu we [switched to use libcrypto by default in our current LTS](https://bugs.launchpad.net/ubuntu/focal/+source/sssd/+bug/1905790), even if we never supported properly the usage of NSS system DB, it was possible to setup one and so we did a [simple tool](https://github.com/3v1n0/nss-database-pem-exporter) to export all the trusted NSS certificates to the SSSD's `ca_db` and [migrate them](https://git.launchpad.net/ubuntu/+source/sssd/commit/?id=884ac4234540ed31f3e13520985f1cffa39dde36).

However, there are still some custom setups in which [may break when using openssl based implementation](https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1919563), because their NSS db was only containing the issuing CA certificates (and no their parent certs) and so it's not possible to verify their certificate.

So using `pam_cert_verification = partial_chain` (or just `certificate_verification`) on upgrades (only and only if migrated) we can ensure that no such system will be broken.

Last commit is somewhat optional in fact, but I thought that having an overriding method just for PAM values can be useful.
"""

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: edited

Changed field: body
Original value:
"""
From the main commit:

<blockquote>
As per the switch to libcrypto by default, the CA certificates DB needs
to contain the whole certificates key-chain in order to verify a leaf
certificate. This means that if an intermediate CA authority signed a
leaf certificate the CA DB we provide to SSSD needs to contain the whole
key-chain, up to the root CA cert in order to verify the leaf one.

Now, while this is indeed more secure, it may break previous
configurations that were based on an NSS database that contained only
trusted intermediate CA certificates.

To allow such setups to continue working (once the NSS db is migrated)
we need to permit a "weaker" setup where an x509 certificate is verified
when the CA database we test against contains only the intermediate CA
certificate that was used to sign it.

As per this, support `partial_chain` value to be used as
`certification_verification` parameter that will add the
`X509_V_FLAG_PARTIAL_CHAIN` verify param flag to the store, as the
openssl's verify `-partial-chain` parameter works.

This setup can still be considered secure as it's still needed to have
configured the SSSD ca db to contain the trusted certs.

Add tests to check that we can verify a leaf certificate against its
parent (only) when using such option.
</blockquote>

In particular in Ubuntu we [switched to use libcrypto by default in our current LTS](https://bugs.launchpad.net/ubuntu/focal/+source/sssd/+bug/1905790), even if we never supported properly the usage of NSS system DB, it was possible to setup one and so we did a [simple tool](https://github.com/3v1n0/nss-database-pem-exporter) to export all the trusted NSS certificates to the SSSD's `ca_db` and [migrate them](https://git.launchpad.net/ubuntu/+source/sssd/commit/?id=884ac4234540ed31f3e13520985f1cffa39dde36).

However, there are still some custom setups in which [may break when using openssl based implementation](https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1919563), because their NSS db was only containing the issuing CA certificates (and no their parent certs) and so it's not possible to verify their issued certificate.

So using `pam_cert_verification = partial_chain` (or just `certificate_verification`) on upgrades (only and only if migrated) we can ensure that no such system will be broken.

Last commit is somewhat optional in fact, but I thought that having an overriding method just for PAM values can be useful.
"""

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5558/head:pr5558
git checkout pr5558

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5558/head:pr5558
git checkout pr5558

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

3v1n0 commented:
"""

Post by sumit-bose
to make sure all tests start with unset `pam_cert_verification`?

Done!
"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-810670606

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5558/head:pr5558
git checkout pr5558

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: edited

Changed field: body
Original value:
"""
From the main commit:

<blockquote>
As per the switch to libcrypto by default, the CA certificates DB needs
to contain the whole certificates key-chain in order to verify a leaf
certificate. This means that if an intermediate CA authority signed a
leaf certificate the CA DB we provide to SSSD needs to contain the whole
key-chain, up to the root CA cert in order to verify the leaf one.

Now, while this is indeed more secure, it may break previous
configurations that were based on an NSS database that contained only
trusted intermediate CA certificates.

To allow such setups to continue working (once the NSS db is migrated)
we need to permit a "weaker" setup where an x509 certificate is verified
when the CA database we test against contains only the intermediate CA
certificate that was used to sign it.

As per this, support `partial_chain` value to be used as
`certification_verification` parameter that will add the
`X509_V_FLAG_PARTIAL_CHAIN` verify param flag to the store, as the
openssl's verify `-partial-chain` parameter works.

This setup can still be considered secure as it's still needed to have
configured the SSSD ca db to contain the trusted certs.

Add tests to check that we can verify a leaf certificate against its
parent (only) when using such option.
</blockquote>

In particular in Ubuntu we [switched to use libcrypto by default in our current LTS](https://bugs.launchpad.net/ubuntu/focal/+source/sssd/+bug/1905790), even if we never supported properly the usage of NSS system DB, it was possible to setup one and so we did a [simple tool](https://github.com/3v1n0/nss-database-pem-exporter) to export all the trusted NSS certificates to the SSSD's `ca_db` and [migrate them](https://git.launchpad.net/ubuntu/+source/sssd/commit/?id=884ac4234540ed31f3e13520985f1cffa39dde36).

However, there are still some custom setups in which [may break when using openssl based implementation](https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1919563), because their NSS db was only containing the issuing CA certificates (and no their parent certs) and so it's not possible to verify their issued certificates.

So using `pam_cert_verification = partial_chain` (or just `certificate_verification`) on upgrades (only and only if migrated) we can ensure that no such system will be broken.

Last commit is somewhat optional in fact, but I thought that having an overriding method just for PAM values can be useful.
"""

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

3v1n0 commented:
"""

Post by sumit-bose
and here `X509_V_FLAG_CRL_CHECK_ALL` enforces a CRL check of the whole chain, i.e. you need the CRL of each CA in the chain. I wonder if `partial_chain` should have an effect on the CRL check as well or if it would be better to have a separate option to toggle the `X509_V_FLAG_CRL_CHECK_ALL` flag?

I'd say it's better to use another flag, while the outcome could be the same, I think that having more granularity on these flags isn't expensive but will avoid troubles in case you want to have mixed configs.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-809521156

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: edited

Changed field: body
Original value:
"""
From the main commit:

<blockquote>
As per the switch to libcrypto by default, the CA certificates DB needs
to contain the whole certificates key-chain in order to verify a leaf
certificate. This means that if an intermediate CA authority signed a
leaf certificate the CA DB we provide to SSSD needs to contain the whole
key-chain, up to the root CA cert in order to verify the leaf one.

Now, while this is indeed more secure, it may break previous
configurations that were based on an NSS database that contained only
trusted intermediate CA certificates.

To allow such setups to continue working (once the NSS db is migrated)
we need to permit a "weaker" setup where an x509 certificate is verified
when the CA database we test against contains only the intermediate CA
certificate that was used to sign it.

As per this, support `partial_chain` value to be used as
`certification_verification` parameter that will add the
`X509_V_FLAG_PARTIAL_CHAIN` verify param flag to the store, as the
openssl's verify `-partial-chain` parameter works.

This setup can still be considered secure as it's still needed to have
configured the SSSD ca db to contain the trusted certs.

Add tests to check that we can verify a leaf certificate against its
parent (only) when using such option.
</blockquote>

In particular in Ubuntu we [switched to use libcrypto by default in our current LTS](https://bugs.launchpad.net/ubuntu/focal/+source/sssd/+bug/1905790), even if we never supported properly the usage of NSS system DB, it was possible to setup one and so we did a [simple migration tool](https://github.com/3v1n0/nss-database-pem-exporter) to export all the trusted NSS certificates to the SSSD's `ca_db`.

However, there are still some custom setups in which [may break when using openssl based implementation](https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1919563), because their NSS db was only containing the issuing CA certificates (and no their parent certs) and so it's not possible to verify their certificate.

So using `pam_cert_verification = partial_chain` on upgrades (only and only if migrated) we can ensure that no such system will be broken.
"""

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: edited

Changed field: body
Original value:
"""
From the main commit:

<blockquote>
As per the switch to libcrypto by default, the CA certificates DB needs
to contain the whole certificates key-chain in order to verify a leaf
certificate. This means that if an intermediate CA authority signed a
leaf certificate the CA DB we provide to SSSD needs to contain the whole
key-chain, up to the root CA cert in order to verify the leaf one.

Now, while this is indeed more secure, it may break previous
configurations that were based on an NSS database that contained only
trusted intermediate CA certificates.

To allow such setups to continue working (once the NSS db is migrated)
we need to permit a "weaker" setup where an x509 certificate is verified
when the CA database we test against contains only the intermediate CA
certificate that was used to sign it.

As per this, support `partial_chain` value to be used as
`certification_verification` parameter that will add the
`X509_V_FLAG_PARTIAL_CHAIN` verify param flag to the store, as the
openssl's verify `-partial-chain` parameter works.

This setup can still be considered secure as it's still needed to have
configured the SSSD ca db to contain the trusted certs.

Add tests to check that we can verify a leaf certificate against its
parent (only) when using such option.
</blockquote>

In particular in Ubuntu we [switched to use libcrypto by default in our current LTS](https://bugs.launchpad.net/ubuntu/focal/+source/sssd/+bug/1905790), even if we never supported properly the usage of NSS system DB, it was possible to setup one and so we did a [simple migration tool](https://github.com/3v1n0/nss-database-pem-exporter) to export all the trusted NSS certificates to the SSSD's `ca_db`.

However, there are still some custom setups in which [may break when using openssl based implementation](https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1919563), because their NSS db was only containing the issuing CA certificates (and no their parent certs) and so it's not possible to verify their certificate.

So using `pam_cert_verification = partial_chain` (or just `certificate_verification`) on upgrades (only and only if migrated) we can ensure that no such system will be broken.

Last commit is somewhat optional in fact, but I thought that having an overriding method just for PAM values can be useful.
"""

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5558/head:pr5558
git checkout pr5558

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

sumit-bose commented:
"""
Hi,

the RHEL/Fedora package is called `libfaketime`, if you change this hopefully more CI tests should pass.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-814846741

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5558/head:pr5558
git checkout pr5558

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

3v1n0 commented:
"""

Post by sumit-bose
the RHEL/Fedora package is called `libfaketime`

Yeah, sorry... I think it is somewhat aliased with faketime as when `dnf install`ing that was working too.

Post by sumit-bose
if you change this hopefully more CI tests should pass.

However, let's hope this, as from what I notice locally some tests using crl files are not working afterwards... In such case I may just revert this change and maybe propose it in another PR if I figure out what's wrong with the tests :)
"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-815075203

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

3v1n0 commented:
"""
Mhmh, not sure what's this failure is about:

```
rpmbuild --define "_topdir /shared/sssd/rpmbuild" -ba SPECS/sssd.spec
setting SOURCE_DATE_EPOCH=1611187200
error: Failed build dependencies:
libfaketime is needed by sssd-2.4.3-0.fc34.x86_64
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-815355506

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

sumit-bose commented:
"""

Post by 3v1n0
```
rpmbuild --define "_topdir /shared/sssd/rpmbuild" -ba SPECS/sssd.spec
setting SOURCE_DATE_EPOCH=1611187200
libfaketime is needed by sssd-2.4.3-0.fc34.x86_64
```

Hi,

this means that the package is missing in the CI VM running the tests. I was under the assumption that this package is installed since it is already required in the general CI dependencies `contrib/ci/deps.sh`. However the CI is executed with `--no-deps`, so it is not checked at all.

Currently there is a configure check if `faketime` is available and the tests needing it are skipped if it is not installed. Do I understand correctly that your patches do not need `faketime` and you add the BuildRequires to just run the missing existing tests? If that's the case I would recommend to drop this BuildRequires for the time being.

I will create a pull-request for sssd-test-suite to get faketime installed to the CI VMs.

bye,
Sumit


"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-815868956

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5558/head:pr5558
git checkout pr5558

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5558/head:pr5558
git checkout pr5558

[3v1n0]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

3v1n0 commented:
"""

Post by sumit-bose
Do I understand correctly that your patches do not need `faketime` and you add the BuildRequires to just run the missing existing tests? If that's the case I would recommend to drop this BuildRequires for the time being.

Yeah, I added since I noticed was missing, but I think it's better to handle it in anther PR, so removed here.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-815915597

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

sumit-bose commented:
"""
Hi,

CI looks better now :-), ACK.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-816542437

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

Label: +Accepted

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

Label: +Ready to push

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

pbrezina commented:
"""
Pushed PR: https://github.com/SSSD/sssd/pull/5558

* `master`
* 65c90d8f98b3ed17574972f1e1bd77d2f2fcf64f - sssd.spec: BuildRequires on openssl tool
* 7e3edb0622b57ab31dd7fe7f634fe4ebb0fcc45c - pam: Add custom pam_cert_verification setting to override default
* 018043bbd58df3b7e9f76a6a4e0bf2356f003b74 - p11_child: Add support for 'partial_chain' certificate_verification option
* 5ed48d2f8aef444e08a33044ba5bf5f9b1887948 - p11_child_openssl: Free X509_VERIFY_PARAM if initialized
* 05e75dba38249827e7c506e83924916bd25863da - test_pam_srv: Add test for CA certificate check using intermediate CA

"""

See the full comment at https://github.com/SSSD/sssd/pull/5558#issuecomment-817732786

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

Label: -Accepted

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

Label: -Ready to push

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5558
Title: #5558: p11_child: Add partial verification support

Label: +Pushed

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5558
Author: 3v1n0
Title: #5558: p11_child: Add partial verification support
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5558/head:pr5558
git checkout pr5558