There are numerous places in the 389 DS code that assume that NSS is being used beneath the OpenLDAP libraries. This means things like replication over SSL won't work if 389 DS is build against OpenLDAP that uses a different crypto implementation, such as OpenSSL or GnuTLS.
We should make 389 DS work properly regardless of the crypto implementation that is used beneath the LDAP libraries.
The server already passes in the TLS_ arguments to ldap_set_option, but it assumes the values either come from global values, or are specific to the openldap nss crypto implementation: {{{ static int setup_ol_tls_conn(LDAP ld, int clientauth) { char certdir = config_get_certdir(); ... if ((rc = ldap_set_option(ld, LDAP_OPT_X_TLS_CACERTDIR, certdir))) { }}} This one should be ok. However, it will have to be in OpenLDAP/openssl cacertdir format, which means it needs to have {{{ TLS_CACERTDIR <path> Specifies the path of a directory that contains Certificate Authority certificates in separate individual files. The TLS_CACERT is always used before TLS_CACERTDIR. The specified directory must be managed with the OpenSSL c_rehash utility. }}} I'm not sure what will happen if you put these CA cert hash files in the same directory as the one containing the regular NSS cert/key db (the config_get_certdir() value). If the openssl ca cert hash files cannot co-exist, then there will have to be a new, separate cacertdir attribute/config parameter (nsslapd-tls-cacertdir or something like that).
We will also need to support the LDAP_OPT_X_TLS_CACERTFILE parameter, which will require a new, separate attribute/config parameter (nsslapd-tls-cacertfile or something like that).
{{{ int slapd_SSL_client_auth (LDAP* ld) ... rc = ldap_set_option(ld, LDAP_OPT_X_TLS_KEYFILE, SERVER_KEY_NAME); if (rc) { slapd_SSL_warn("SSL client authentication cannot be used " "unable to set the key to use to %s", SERVER_KEY_NAME); } rc = ldap_set_option(ld, LDAP_OPT_X_TLS_CERTFILE, cert_name); if (rc) { slapd_SSL_warn("SSL client authentication cannot be used " "unable to set the cert to use to %s", cert_name); } }}} For OpenLDAP+openssl, these will both have to be actual filenames, which means there will need to be attributes/config params (e.g. nsslapd-tls-client-certfile, nsslapd-tls-client-keyfile).
We set other TLS parameters with ldap_set_option (LDAP_OPT_X_TLS_REQUIRE_CERT, LDAP_OPT_X_TLS_PROTOCOL_MIN, LDAP_OPT_X_TLS_NEWCTX) and these should be fine as is. I don't think it will be necessary to support all of the OpenLDAP LDAP_OPT_X_TLS_* options, but we should consider it.
One issue for migration - will it be possible for us to not require the user to generate these files? For example, one way to do this would be, when the server starts up, to create temp files for the CA cert, the cert, and the key. I believe the tmpfile(3) function can create a temporary file that is not visible outside of the server process, and will be deleted when the server process exits. Then the server would just need to 1) iterate over the CA cert db and dump them to a single CA cert file 2) dump the server cert to the certfile 3) dump the server key to the keyfile. The last one might be very difficult - NSS does not usually allow you to get the actual key, but perhaps there is some programmatic way to call pk12util -o, then split the .p12 file into the cert and key.
git patch file (master) -- experimental patch 2 0001-Experimental-Ticket-47536-Allow-usage-of-OpenLDAP-li.patch
{{{ - Adding a new config parameter to the agreement: nsDS5ReplicaCACert: /path/to/cacert.pem or cacert.pem }}}
Right now, replica CA's are based on the NSS ca trust in our db. Why make a per agreement CA? Why not have an entry in cn=encryption like nsslap-client-trust-cert: /path/to/cadir or /path/to/cacert.pem
This would maintain a similarity to current behaviour IMO.
{{{ - In slapi_ldap_init_ext, if (!secure && filename), it is LDAPI; If (secure && filename), it is considered as a cacert and pass it to setup_ol_tls_conn(ld, 0, filename), in which it calls ldap_set_option(ld, LDAP_OPT_X_TLS_CACERTFILE, filename); }}}
IMO this is confusing an un-intuitive as an API. Cert should be seperate from the LDAPI socket path, even if the two are mutually exclusive.
Additionally, if we change to nsslap-client-trust-cert in my previous suggest, we don't need cert as a param to slapi_ldap_init_ext, we only need to call a "get" on the config for the ldap ca trust config item.
{{{ Note: Instead of setting nsDS5ReplicaCACert, tried to extract the path to cacert using /etc/openldap/ldap.conf, /root/.ldaprc, as well as ~dirsrv/.ldaprc. But none of them has the impact. }}}
We should probably investigate this, as this is an important possible feature for us.
I haven't built or tested the code provided. I'm reviewing by eye only. I'll need to do a better test of this later.
We probably need some python test cases for this too in the future. I'm happy to work with you on them if you need.
Replying to [comment:10 firstyear]:
{{{ - Adding a new config parameter to the agreement: nsDS5ReplicaCACert: /path/to/cacert.pem or cacert.pem }}} Right now, replica CA's are based on the NSS ca trust in our db. Why make a per agreement CA? Why not have an entry in cn=encryption like nsslap-client-trust-cert: /path/to/cadir or /path/to/cacert.pem This would maintain a similarity to current behaviour IMO.
Yes. We should have both. OpenLDAP (OpenSSL) allows a different CA per SSL context. There aren't many use cases for that, but we should support that. In the default case, if nsDS5ReplicaCACert is not specified, it should fallback to cn=encryption nsslapd-ca-cert.
{{{ - In slapi_ldap_init_ext, if (!secure && filename), it is LDAPI; If (secure && filename), it is considered as a cacert and pass it to setup_ol_tls_conn(ld, 0, filename), in which it calls ldap_set_option(ld, LDAP_OPT_X_TLS_CACERTFILE, filename); }}} IMO this is confusing an un-intuitive as an API. Cert should be seperate from the LDAPI socket path, even if the two are mutually exclusive. Additionally, if we change to nsslap-client-trust-cert in my previous suggest, we don't need cert as a param to slapi_ldap_init_ext, we only need to call a "get" on the config for the ldap ca trust config item. {{{ Note: Instead of setting nsDS5ReplicaCACert, tried to extract the path to cacert using /etc/openldap/ldap.conf, /root/.ldaprc, as well as ~dirsrv/.ldaprc. But none of them has the impact. }}} We should probably investigate this, as this is an important possible feature for us. I haven't built or tested the code provided. I'm reviewing by eye only. I'll need to do a better test of this later.
Replying to [comment:12 rmeggins]:
Replying to [comment:10 firstyear]: {{{ - Adding a new config parameter to the agreement: nsDS5ReplicaCACert: /path/to/cacert.pem or cacert.pem }}} Right now, replica CA's are based on the NSS ca trust in our db. Why make a per agreement CA? Why not have an entry in cn=encryption like nsslap-client-trust-cert: /path/to/cadir or /path/to/cacert.pem This would maintain a similarity to current behaviour IMO. Yes. We should have both. OpenLDAP (OpenSSL) allows a different CA per SSL context. There aren't many use cases for that, but we should support that. In the default case, if nsDS5ReplicaCACert is not specified, it should fallback to cn=encryption nsslapd-ca-cert.
I haven't started working on it yet, but my concern is WinSync. If the AD is configured with a cert issued by its own cert server, the DS side needs to have/trust the CA cert. I thought in such a case, each agreement needs to have an ability to specify a cert pem file.
It may not be a rare case, but even a replica on linux could have a cert issued by other CA.
And yes, I agree falling back to cn=encryption nsslapd-ca-cert should be the default behaviour.
{{{ - In slapi_ldap_init_ext, if (!secure && filename), it is LDAPI; If (secure && filename), it is considered as a cacert and pass it to setup_ol_tls_conn(ld, 0, filename), in which it calls ldap_set_option(ld, LDAP_OPT_X_TLS_CACERTFILE, filename); }}} IMO this is confusing an un-intuitive as an API. Cert should be seperate from the LDAPI socket path, even if the two are mutually exclusive.
??? Could you elaborate some more? You'd like to have a new argument for the CA cert path? I rather like to share one abstract arg specifying the attribute by some other way like flags... But if you don't like it, it's not a big deal. :)
Again, we may need to cover more cases...
{{{ Note: Instead of setting nsDS5ReplicaCACert, tried to extract the path to cacert using /etc/openldap/ldap.conf, /root/.ldaprc, as well as ~dirsrv/.ldaprc. But none of them has the impact. }}} We should probably investigate this, as this is an important possible feature for us. I haven't built or tested the code provided. I'm reviewing by eye only. I'll need to do a better test of this later.
I haven't investigated this in depth yet. This means we are having another set of configuration. For instance, we could configure the 2 sets that conflict each other... We'd better be careful to commit on this requirement, I think.
Yes. We should have both. OpenLDAP (OpenSSL) allows a different CA per SSL context. There aren't many use cases for that, but we should support that. In the default case, if nsDS5ReplicaCACert is not specified, it should fallback to cn=encryption nsslapd-ca-cert. I haven't started working on it yet, but my concern is WinSync. If the AD is configured with a cert issued by its own cert server, the DS side needs to have/trust the CA cert. I thought in such a case, each agreement needs to have an ability to specify a cert pem file.
How do we currently handle this? Are we installing the CA cert from AD to the nss db. How would this be different from requiring the user to add the winsync ca cert to a "cacertdir" instead.
It may not be a rare case, but even a replica on linux could have a cert issued by other CA. And yes, I agree falling back to cn=encryption nsslapd-ca-cert should be the default behaviour.
The reason I propose it, is because it's simple for us to implement, we don't need to muck around with what cert in what agreement, and passing the names around etc. We have one cacertdir, and we are done. It's the current status quo anyway, as we have one nssdb for ca certs.
{{{ Note: Instead of setting nsDS5ReplicaCACert, tried to extract the path to cacert using /etc/openldap/ldap.conf, /root/.ldaprc, as well as ~dirsrv/.ldaprc. But none of them has the impact. }}} We should probably investigate this, as this is an important possible feature for us. I haven't built or tested the code provided. I'm reviewing by eye only. I'll need to do a better test of this later. I haven't investigated this in depth yet. This means we are having another set of configuration. For instance, we could configure the 2 sets that conflict each other... We'd better be careful to commit on this requirement, I think.
I think it depends on on the openldap library behaviour, how it handles multiple ca paths. If it unions them, or if it takes only one of them.
Here's my CA certs in the NSS db. NSS figures out which cert is to be used... (maybe by trying one by one?)
CA certificate CTu,u,u test-AUTOWS2012R2-CA CT,,
There is one more implicit use of NSS that just happens - revocation checking. If you are using NSS for everything, the certs are automatically checked against any CRLs or OCSPs that NSS knows about. We also need to consider how openldap+openssl clients will check server certs (or CA certs too?) for revocation.
Replying to [comment:16 rmeggins]:
Hi Rich,
In OpenLDAP this option can be set in ldap.conf. '''TLS_CRLCHECK <level>''' Specifies if the Certificate Revocation List (CRL) of the CA should be used to verify if the server certificates have not been revoked. This requires TLS_CACERTDIR parameter to be set. This parameter is ignored with GNUtls and Mozilla NSS. <level> can be specified as one of the following keywords: {{{ none - No CRL checks are performed peer - Check the CRL of the peer certificate all - Check the CRL for a whole certificate chain }}} In the openldap library, it could be set by ldap_set_option with this key: '''LDAP_OPT_X_TLS_CRLCHECK''' Sets the CRL evaluation strategy, one of LDAP_OPT_X_TLS_CRL_NONE, LDAP_OPT_X_TLS_CRL_PEER, or LDAP_OPT_X_TLS_CRL_ALL. invalue must be const int *. (Requires OpenSSL.)
Like this: + const int crlcheck = LDAP_OPT_X_TLS_CRL_ALL; + rc = ldap_set_option(ld, LDAP_OPT_X_TLS_CRLCHECK, &crlcheck);
Do you think by setting this, openssl does what we are hoping?
Also, is the value '''all''' appropriate for our purpose?
Replying to [comment:18 nhosoi]:
Like this: + const int crlcheck = LDAP_OPT_X_TLS_CRL_ALL; + rc = ldap_set_option(ld, LDAP_OPT_X_TLS_CRLCHECK, &crlcheck); Do you think by setting this, openssl does what we are hoping?
I don't think so. How would openssl know what the CRL is, unless it is downloading the CRL from the information in the CA cert, or using OCSP?
I have no idea how openssl (with or without openldap) does CRL/OCSP checking.
Replying to [comment:19 rmeggins]:
Replying to [comment:18 nhosoi]: Like this: + const int crlcheck = LDAP_OPT_X_TLS_CRL_ALL; + rc = ldap_set_option(ld, LDAP_OPT_X_TLS_CRLCHECK, &crlcheck); Do you think by setting this, openssl does what we are hoping? I don't think so. How would openssl know what the CRL is, unless it is downloading the CRL from the information in the CA cert, or using OCSP? This is the openldap code where the option is set. (HAVE_OPENSSL_CRL is enabled.) {{{
I don't think so. How would openssl know what the CRL is, unless it is downloading the CRL from the information in the CA cert, or using OCSP? This is the openldap code where the option is set. (HAVE_OPENSSL_CRL is enabled.) {{{
if ( lo->ldo_tls_crlcheck ) { X509_STORE *x509_s = SSL_CTX_get_cert_store( ctx ); if ( lo->ldo_tls_crlcheck == LDAP_OPT_X_TLS_CRL_PEER ) { X509_STORE_set_flags( x509_s, X509_V_FLAG_CRL_CHECK ); } else if ( lo->ldo_tls_crlcheck == LDAP_OPT_X_TLS_CRL_ALL ) { X509_STORE_set_flags( x509_s, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL ); } }
}}} But now sure how it is being used...
Checked out OpenSSL code: git clone https://github.com/openssl/openssl It turned out X509_STORE_set_flags is a thin wrapper of X509_VERIFY_PARAM_set_flags. The man page of X509_VERIFY_PARAM_set_flags says: '''VERIFICATION FLAGS''' The verification flags consists of zero or more of the following flags ored together. '''X509_V_FLAG_CRL_CHECK''' enables CRL checking for the certificate chain leaf certificate. An error occurs if a suitable CRL cannot be found. '''X509_V_FLAG_CRL_CHECK_ALL''' enables CRL checking for the entire certificate chain.
I don't see further codes using the context x509_s in "libraries/libldap/tls_o.c"...
As mentioned in this http://www.openldap.org/lists/openldap-technical/201404/msg00083.html, it seems we need to put the '''CRL file''' in the cert dir... That suggests the man page is wrong and it is not expecting a CA there. If I remove the CA from /home/manu/openssl/ca/ and copy the CRL in /path/to/openssl/ca/0726b466.r0, it reads it without a complain, then tries to read /path/to/openssl/ca/0726b466.r1 and fail there.
Both openssl and NSS provide the method to manage CRL file. https://jamielinux.com/docs/openssl-certificate-authority/certificate-revocation-lists.html https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/tools/NSS_Tools_crlutil
The current slapd_extract_key code commented by William looks like this. I did not replace 0 with '\0' in the line 2891, but it should be ok. {{{ 2885 static int 2886 slapd_extract_key(Slapi_Entry entry, char token, PK11SlotInfo slot) 2887 { ... 2891 unsigned char randomPassword[RAND_PASS_LEN] = {0}; ... 2945 rv = PK11_GenerateRandom(randomPassword, sizeof((const char )randomPassword) - 1); }}}
The patch is specific to openssl. One of our primary use cases is support on Debian/Ubuntu where the openldap implementation uses gnutls.
Instead of checking for "openldap uses openssl" - we should check for "openldap does not use nss".
Replying to [comment:25 rmeggins]:
The patch is specific to openssl. One of our primary use cases is support on Debian/Ubuntu where the openldap implementation uses gnutls. Instead of checking for "openldap uses openssl" - we should check for "openldap does not use nss".
Sure. Instead of if (!rc && !PL_strcasecmp(package_name, "OpenSSL")) { we could do if (!rc && PL_strcasecmp(package_name, "MozNSS")) {
I'm a bit concerned at some differences among the underlying libraries.
Although we may not end up using this, but, for instance, this feature is OpenSSL only. {{{ $ man ldap_set_option LDAP_OPT_X_TLS_CRLCHECK Sets/gets the CRL evaluation strategy, one of LDAP_OPT_X_TLS_CRL_NONE, LDAP_OPT_X_TLS_CRL_PEER, or LDAP_OPT_X_TLS_CRL_ALL. invalue must be const int ; outvalue must be int . Requires OpenSSL. }}}
In most cases it shouldn't matter - the options like ca cert, cert, key, strength, etc. are the same if openssl or gnutls is used.
the change in ldap/servers/slapd/dn.c seems unrelated - should probably be in a different ticket/patch
otherwise, this looks good enough for tjaalton to try out.
Replying to [comment:28 rmeggins]:
That's true. I guess I noticed this when I was working on other ticket (#48492). I'm going to remove the change from this patch. Thanks!
git patch file (master) -- another revision -- instead of checking with "OpenSSL", check with "Not MozNSS" for non-Fedora/RHEL platforms; removed dn.c from the patch and put CRLCHECK in the isOpenSSL clause. 0001-Ticket-47536-Allow-usage-of-OpenLDAP-libraries-that-.patch
I'm really happy with this now. It passes all my tests, and looks good. I'm happy to ack this with two tweaks:
First, need a slightly longer delay in the test to let replication happen.
{{{ 507 time.sleep(10) }}}
Second, I don't think this is needed:
{{{ 517 db2ldifpl = '%s/sbin/db2ldif.pl' % os.getenv('PREFIX') 518 cmdline = [db2ldifpl, '-n', 'userRoot', '-Z', SERVERID_MASTER_1, '-D', DN_DM, '-w', PASSWORD] 519 log.info("##### db2ldif.pl -- %s" % (cmdline)) 520 doAndPrintIt(cmdline) }}}
Replying to [comment:30 firstyear]:
I'm really happy with this now. It passes all my tests, and looks good. Thank you so much for reviewing this lengthy patch so many times... I'm happy to ack this with two tweaks: First, need a slightly longer delay in the test to let replication happen. {{{ 507 time.sleep(10) }}} All right. I'm adding it. Second, I don't think this is needed: {{{ 517 db2ldifpl = '%s/sbin/db2ldif.pl' % os.getenv('PREFIX') 518 cmdline = [db2ldifpl, '-n', 'userRoot', '-Z', SERVERID_MASTER_1, '-D', DN_DM, '-w', PASSWORD] 519 log.info("##### db2ldif.pl -- %s" % (cmdline)) 520 doAndPrintIt(cmdline) }}} Not needed because this command line fails for you? I had this issue: https://fedorahosted.org/389/ticket/48756, and I wanted to make sure my patch does not break the perl utilities...
I'm really happy with this now. It passes all my tests, and looks good. Thank you so much for reviewing this lengthy patch so many times...
I'm happy to ack this with two tweaks:
{{{ 507 time.sleep(10) }}} All right. I'm adding it.
{{{ 517 db2ldifpl = '%s/sbin/db2ldif.pl' % os.getenv('PREFIX') 518 cmdline = [db2ldifpl, '-n', 'userRoot', '-Z', SERVERID_MASTER_1, '-D', DN_DM, '-w', PASSWORD] 519 log.info("##### db2ldif.pl -- %s" % (cmdline)) 520 doAndPrintIt(cmdline) }}} Not needed because this command line fails for you? I had this issue: https://fedorahosted.org/389/ticket/48756, and I wanted to make sure my patch does not break the perl utilities...
It's failing in the doAndPrintIt, but I think I forgot to apply my own lib389 fix for the issue.
git patch file (master) -- CI test; revised following the comments from ​wibrown@redhat.com (Thank you, William!) 0002-Ticket-47536-CI-test-added-test-cases-for-ticket-475.patch
Reviewed by William (Thank you!!)
Pushed to master: 4c66307..fa620fc master -> master commit b170243 commit e3c5335
Metadata Update from @rmeggins: - Issue assigned to nhosoi - Issue set to the milestone: 1.3.5 backlog
This patch fixes the CI test
<img alt="0001-Issue-47536-Fix-CI-testcase.patch" src="/389-ds-base/issue/raw/files/69163233d259e14e639f219b591bfdd0f6003baa9278a5702fda4764a5cb2a24-0001-Issue-47536-Fix-CI-testcase.patch" />
Metadata Update from @mreynolds: - Custom field component reset - Custom field reviewstatus adjusted to review (was: ack) - Issue close_status updated to: None (was: Fixed)
FQDN can be obtained using os.uname()[1], the rest looks good, test passes. Thanks Mark!
os.uname()[1]
Metadata Update from @spichugi: - Custom field component adjusted to None - Issue set to the milestone: None (was: 1.3.5 backlog)
<img alt="0001-Issue-47536-Add-Python-3-support-and-move-test-case-.patch" src="/389-ds-base/issue/raw/files/028acfb8db6dee6795bef4551d001bb562a2f1f708fabc60a5c339b7af6d3148-0001-Issue-47536-Add-Python-3-support-and-move-test-case-.patch" />
150 + m1.stop(10)
Can you remove the timeouts on the .stop()?
Otherwise I think I'm happy with this code :)
<img alt="0001-Issue-47536-Add-Python-3-support-and-move-test-case-.patch" src="/389-ds-base/issue/raw/files/e023d32821b13103e662b94b1348a0a3016b3ea15fd24b4e2bebfefbdae9e8e6-0001-Issue-47536-Add-Python-3-support-and-move-test-case-.patch" />
Metadata Update from @firstyear: - Custom field reviewstatus adjusted to ack (was: review)
Ack for python 3 test case.
Thanks!
commit 938dfb7 Author: Simon Pichugin spichugi@redhat.com Date: Tue Nov 21 19:51:57 2017 +0100
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/873
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix
Login to comment on this ticket.