URL: https://github.com/SSSD/sssd/pull/5558 Author: 3v1n0 Title: #5558: p11_child: Add partial verification support Action: opened
PR body: """ 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. """
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
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. """
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. """
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=884ac4234540ed31f3e...).
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. """
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=884ac4234540ed31f3e...).
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. """
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
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
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=884ac4234540ed31f3e...).
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. """
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
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
URL: https://github.com/SSSD/sssd/pull/5558 Title: #5558: p11_child: Add partial verification support
3v1n0 commented: """
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
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
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
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
URL: https://github.com/SSSD/sssd/pull/5558 Title: #5558: p11_child: Add partial verification support
3v1n0 commented: """
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
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
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
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
URL: https://github.com/SSSD/sssd/pull/5558 Title: #5558: p11_child: Add partial verification support
3v1n0 commented: """
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.
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
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
URL: https://github.com/SSSD/sssd/pull/5558 Title: #5558: p11_child: Add partial verification support
sumit-bose 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
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
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
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
URL: https://github.com/SSSD/sssd/pull/5558 Title: #5558: p11_child: Add partial verification support
3v1n0 commented: """
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
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
URL: https://github.com/SSSD/sssd/pull/5558 Title: #5558: p11_child: Add partial verification support
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/5558 Title: #5558: p11_child: Add partial verification support
Label: +Ready to push
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
URL: https://github.com/SSSD/sssd/pull/5558 Title: #5558: p11_child: Add partial verification support
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/5558 Title: #5558: p11_child: Add partial verification support
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/5558 Title: #5558: p11_child: Add partial verification support
Label: -Ready to push
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
sssd-devel@lists.fedorahosted.org
-
3v1n0 -
pbrezina
-
sumit-bose