Full_Name: Alec Cooper Version: HEAD of master branch OS: Ubuntu Linux 16.04 URL: ftp://ftp.openldap.org/incoming/Alec-Cooper-160827.patch Submission from: (NULL) (73.134.243.211) Adding support for encrypted server private keys. The meat of this patch is changes to tls2.c, tls_g.c, tls_m.c, and tls_o.c to send a password to the underlying TLS library. Changes to ldap.h, init.c and ldap-int.h expose the new LDAP_OPT_X_TLS_KEYPASSWORD in the main API. Changes in the contrib/ldapc++ directory expose the corresponding TlsOptions::KEYPASSWORD option in the C++ API. Changes in the servers directory expose equivalent options for servers as configuration file entries or environment variables. Changes in the doc directory add documentation about the new options and remove statements that indicated that encrypted keyfiles are not supported. New files in the test directory are for testing TLS connections, both in general and with encrypted keyfiles. Tests pass for OpenSSL and GnuTLS using PEM formatted certs and keys, and for MozNSS using cert/key databases. The new unit test (test065-tls) has been written to detect when using NSS, and use the cert/key databases in this case. I have been unable to get a working version of libnsspem, so I cannot test MozNSS with (or without) encrypted keyfiles - testing for this case would be welcome! Notice of origin: The attached patch file is derived from OpenLDAP Software. All of the modifications to OpenLDAP Software represented in the following patch were developed by Alec Cooper ahnolds@gmail.com. I have not assigned rights and/or interest in this work to any party. Rights statement: I, Alec Cooper, hereby place the following modifications to OpenLDAP Software (and only these modifications) into the public domain. Hence, these modifications may be freely used and/or redistributed for any purpose with or without attribution and/or other notice. The patch has been uploaded to the OpenLDAP FTP server and can be found at ftp://ftp.openldap.org/incoming/Alec-Cooper-160827.patch
This would be a very helpful feature for me - I have a web server that has to regularly interact with a corporately maintained secure LDAP server. I'd love to be able to store the certs for this interaction encrypted for added security.
ahnolds@gmail.com wrote: > Full_Name: Alec Cooper > Version: HEAD of master branch > OS: Ubuntu Linux 16.04 > URL: ftp://ftp.openldap.org/incoming/Alec-Cooper-160827.patch > Submission from: (NULL) (73.134.243.211) > > > Adding support for encrypted server private keys. Thank you for your submission. I must compliment you on providing such a comprehensive and well-written contribution. Unfortunately the feature itself is clearly not useful. Perhaps you should have raised this topic on the openldap-devel list for discussion before spending your time writing it. All your feature does is embed the encryption key in the filesystem, so an encrypted private key is still no more secure than an unencrypted one. It still just depends on setting proper file access permissions, and the current documentation already makes that point. Indeed, all it does is give the false illusion of enhancing security, and followups like balmerpeak92@gmail.com's shows that people will easily fall for such illusions. I regret having to reject such a well written contribution, but we cannot in good conscience accept a security feature that doesn't actually improve security. > > The meat of this patch is changes to tls2.c, tls_g.c, tls_m.c, and tls_o.c to > send a password to the underlying TLS library. > > Changes to ldap.h, init.c and ldap-int.h expose the new > LDAP_OPT_X_TLS_KEYPASSWORD in the main API. > > Changes in the contrib/ldapc++ directory expose the corresponding > TlsOptions::KEYPASSWORD option in the C++ API. > > Changes in the servers directory expose equivalent options for servers as > configuration file entries or environment variables. > > Changes in the doc directory add documentation about the new options and remove > statements that indicated that encrypted keyfiles are not supported. > > New files in the test directory are for testing TLS connections, both in general > and with encrypted keyfiles. Tests pass for OpenSSL and GnuTLS using PEM > formatted certs and keys, and for MozNSS using cert/key databases. The new unit > test (test065-tls) has been written to detect when using NSS, and use the > cert/key databases in this case. I have been unable to get a working version of > libnsspem, so I cannot test MozNSS with (or without) encrypted keyfiles - > testing for this case would be welcome! > > Notice of origin: The attached patch file is derived from OpenLDAP Software. All > of the modifications to OpenLDAP Software represented in the following patch > were developed by Alec Cooper ahnolds@gmail.com. I have not assigned rights > and/or interest in this work to any party. > Rights statement: I, Alec Cooper, hereby place the following modifications to > OpenLDAP Software (and only these modifications) into the public domain. Hence, > these modifications may be freely used and/or redistributed for any purpose with > or without attribution and/or other notice. > > The patch has been uploaded to the OpenLDAP FTP server and can be found at > ftp://ftp.openldap.org/incoming/Alec-Cooper-160827.patch > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
I completely agree that this doesn't add any security when running slapd. However, when I originally wrote this patch, I was more targeting API usage, and in particular, client-side API usage. My goal was to allow a user to present an encrypted key (stored on disk) and a password (only held in memory), and use these to query a secure LDAP server. The issue of how the password got into memory is certainly beyond the scope of OpenLDAP in this case, but it certainly could be done securely in a way that wouldn't leave it exposed to a third-party with file-system access. For example (in the use case that motivated me originally), an sysadmin could input the password through an online form, submit said form to a Python webserver over HTTPS, the Python code could pass the password to OpenLDAP through a Python interface like python-ldap ( https://www.python-ldap.org/), and then use the LDAPS or STARTTLS connection to talk securely to some LDAP server elsewhere. All without an unencrypted keyfile or a password ever existing on disk, hence the added security. If you feel that supporting encrypted keyfiles in slapd is a bad design choice, I'm willing to rework the patch to drop that part and only keep the API-level support. Personally, I would argue that it doesn't hurt to support the feature, although perhaps my documentation changes could do a better job emphasizing the inherent security limitations. Let me know what you think (or if this discussion should take place on a list, let me know which/where). On Wed, Feb 22, 2017 at 1:11 PM, Howard Chu <hyc@symas.com> wrote: > ahnolds@gmail.com wrote: > >> Full_Name: Alec Cooper >> Version: HEAD of master branch >> OS: Ubuntu Linux 16.04 >> URL: ftp://ftp.openldap.org/incoming/Alec-Cooper-160827.patch >> Submission from: (NULL) (73.134.243.211) >> >> >> Adding support for encrypted server private keys. >> > > Thank you for your submission. I must compliment you on providing such a > comprehensive and well-written contribution. Unfortunately the feature > itself is clearly not useful. Perhaps you should have raised this topic on > the openldap-devel list for discussion before spending your time writing it. > > All your feature does is embed the encryption key in the filesystem, so an > encrypted private key is still no more secure than an unencrypted one. It > still just depends on setting proper file access permissions, and the > current documentation already makes that point. > > Indeed, all it does is give the false illusion of enhancing security, and > followups like balmerpeak92@gmail.com's shows that people will easily > fall for such illusions. > > I regret having to reject such a well written contribution, but we cannot > in good conscience accept a security feature that doesn't actually improve > security. > >> >> The meat of this patch is changes to tls2.c, tls_g.c, tls_m.c, and >> tls_o.c to >> send a password to the underlying TLS library. >> >> Changes to ldap.h, init.c and ldap-int.h expose the new >> LDAP_OPT_X_TLS_KEYPASSWORD in the main API. >> >> Changes in the contrib/ldapc++ directory expose the corresponding >> TlsOptions::KEYPASSWORD option in the C++ API. >> >> Changes in the servers directory expose equivalent options for servers as >> configuration file entries or environment variables. >> >> Changes in the doc directory add documentation about the new options and >> remove >> statements that indicated that encrypted keyfiles are not supported. >> >> New files in the test directory are for testing TLS connections, both in >> general >> and with encrypted keyfiles. Tests pass for OpenSSL and GnuTLS using PEM >> formatted certs and keys, and for MozNSS using cert/key databases. The >> new unit >> test (test065-tls) has been written to detect when using NSS, and use the >> cert/key databases in this case. I have been unable to get a working >> version of >> libnsspem, so I cannot test MozNSS with (or without) encrypted keyfiles - >> testing for this case would be welcome! >> >> Notice of origin: The attached patch file is derived from OpenLDAP >> Software. All >> of the modifications to OpenLDAP Software represented in the following >> patch >> were developed by Alec Cooper ahnolds@gmail.com. I have not assigned >> rights >> and/or interest in this work to any party. >> Rights statement: I, Alec Cooper, hereby place the following >> modifications to >> OpenLDAP Software (and only these modifications) into the public domain. >> Hence, >> these modifications may be freely used and/or redistributed for any >> purpose with >> or without attribution and/or other notice. >> >> The patch has been uploaded to the OpenLDAP FTP server and can be found at >> ftp://ftp.openldap.org/incoming/Alec-Cooper-160827.patch >> >> >> > > -- > -- Howard Chu > CTO, Symas Corp. http://www.symas.com > Director, Highland Sun http://highlandsun.com/hyc/ > Chief Architect, OpenLDAP http://www.openldap.org/project/ >
moved from Incoming to Software Enhancements
I take it that this enhancement on the client side API isn't something you think would be useful. If that's the case, feel free to close this ticket, and best of luck with your ongoing development! On Wed, Feb 22, 2017 at 8:49 PM, Alec C <ahnolds@gmail.com> wrote: > I completely agree that this doesn't add any security when running slapd. > However, when I originally wrote this patch, I was more targeting API > usage, and in particular, client-side API usage. My goal was to allow a > user to present an encrypted key (stored on disk) and a password (only held > in memory), and use these to query a secure LDAP server. The issue of how > the password got into memory is certainly beyond the scope of OpenLDAP in > this case, but it certainly could be done securely in a way that wouldn't > leave it exposed to a third-party with file-system access. > > For example (in the use case that motivated me originally), an sysadmin > could input the password through an online form, submit said form to a > Python webserver over HTTPS, the Python code could pass the password to > OpenLDAP through a Python interface like python-ldap ( > https://www.python-ldap.org/), and then use the LDAPS or STARTTLS > connection to talk securely to some LDAP server elsewhere. All without an > unencrypted keyfile or a password ever existing on disk, hence the added > security. > > If you feel that supporting encrypted keyfiles in slapd is a bad design > choice, I'm willing to rework the patch to drop that part and only keep the > API-level support. Personally, I would argue that it doesn't hurt to > support the feature, although perhaps my documentation changes could do a > better job emphasizing the inherent security limitations. Let me know what > you think (or if this discussion should take place on a list, let me know > which/where). > > > On Wed, Feb 22, 2017 at 1:11 PM, Howard Chu <hyc@symas.com> wrote: > >> ahnolds@gmail.com wrote: >> >>> Full_Name: Alec Cooper >>> Version: HEAD of master branch >>> OS: Ubuntu Linux 16.04 >>> URL: ftp://ftp.openldap.org/incoming/Alec-Cooper-160827.patch >>> Submission from: (NULL) (73.134.243.211) >>> >>> >>> Adding support for encrypted server private keys. >>> >> >> Thank you for your submission. I must compliment you on providing such a >> comprehensive and well-written contribution. Unfortunately the feature >> itself is clearly not useful. Perhaps you should have raised this topic on >> the openldap-devel list for discussion before spending your time writing it. >> >> All your feature does is embed the encryption key in the filesystem, so >> an encrypted private key is still no more secure than an unencrypted one. >> It still just depends on setting proper file access permissions, and the >> current documentation already makes that point. >> >> Indeed, all it does is give the false illusion of enhancing security, and >> followups like balmerpeak92@gmail.com's shows that people will easily >> fall for such illusions. >> >> I regret having to reject such a well written contribution, but we cannot >> in good conscience accept a security feature that doesn't actually improve >> security. >> >>> >>> The meat of this patch is changes to tls2.c, tls_g.c, tls_m.c, and >>> tls_o.c to >>> send a password to the underlying TLS library. >>> >>> Changes to ldap.h, init.c and ldap-int.h expose the new >>> LDAP_OPT_X_TLS_KEYPASSWORD in the main API. >>> >>> Changes in the contrib/ldapc++ directory expose the corresponding >>> TlsOptions::KEYPASSWORD option in the C++ API. >>> >>> Changes in the servers directory expose equivalent options for servers as >>> configuration file entries or environment variables. >>> >>> Changes in the doc directory add documentation about the new options and >>> remove >>> statements that indicated that encrypted keyfiles are not supported. >>> >>> New files in the test directory are for testing TLS connections, both in >>> general >>> and with encrypted keyfiles. Tests pass for OpenSSL and GnuTLS using PEM >>> formatted certs and keys, and for MozNSS using cert/key databases. The >>> new unit >>> test (test065-tls) has been written to detect when using NSS, and use the >>> cert/key databases in this case. I have been unable to get a working >>> version of >>> libnsspem, so I cannot test MozNSS with (or without) encrypted keyfiles - >>> testing for this case would be welcome! >>> >>> Notice of origin: The attached patch file is derived from OpenLDAP >>> Software. All >>> of the modifications to OpenLDAP Software represented in the following >>> patch >>> were developed by Alec Cooper ahnolds@gmail.com. I have not assigned >>> rights >>> and/or interest in this work to any party. >>> Rights statement: I, Alec Cooper, hereby place the following >>> modifications to >>> OpenLDAP Software (and only these modifications) into the public domain. >>> Hence, >>> these modifications may be freely used and/or redistributed for any >>> purpose with >>> or without attribution and/or other notice. >>> >>> The patch has been uploaded to the OpenLDAP FTP server and can be found >>> at >>> ftp://ftp.openldap.org/incoming/Alec-Cooper-160827.patch >>> >>> >>> >> >> -- >> -- Howard Chu >> CTO, Symas Corp. http://www.symas.com >> Director, Highland Sun http://highlandsun.com/hyc/ >> Chief Architect, OpenLDAP http://www.openldap.org/project/ >> > >
Created attachment 652 [details] Alec-Cooper-2016-08-27.patch
I would still support merging this, just saying since it came up again. I think it does add valuable functionality on at least the client side where it allows the certificate to stay encrypted on-disk and the password could be supplied interactively by the user. If there is desire for it and any re-work is required, please let me know!
Do I understand it correctly that libldap doesn't currently support using encrypted keys at all, not even for client certificates? If that is so, there are scenarios where supporting that does improve security (e.g. interactive tools).
I concur that lacking support for encrypted private keys is a real deficiency! In general OpenLDAP should aim to reach more flexibility for the TLS configuration, e.g. like Apache httpd. Encrypted private keys for both server and client side is one aspect of that.
(In reply to Michael Ströder from comment #9) > I concur that lacking support for encrypted private keys is a real > deficiency! > > In general OpenLDAP should aim to reach more flexibility for the TLS > configuration, e.g. like Apache httpd. Encrypted private keys for both > server and client side is one aspect of that. We have never needed to add explicit support, since OpenSSL prompted for a passphrase itself, when needed. https://www.openldap.org/lists/openldap-software/200210/msg00718.html
(In reply to Howard Chu from comment #10) > (In reply to Michael Ströder from comment #9) > > I concur that lacking support for encrypted private keys is a real > > deficiency! > > > > In general OpenLDAP should aim to reach more flexibility for the TLS > > configuration, e.g. like Apache httpd. Encrypted private keys for both > > server and client side is one aspect of that. > > We have never needed to add explicit support, since OpenSSL prompted for > a passphrase itself, when needed. > > https://www.openldap.org/lists/openldap-software/200210/msg00718.html It prompts for the passphrase on the controlling terminal, which is only helpful for command-line based applications. For any application run through a GUI/web server/etc, there won't be any way for the user to enter the passphrase as is. And in fact, the call to use the key will hang (forever IIRC) waiting for a passphrase to be typed on the terminal.
There are not many things slapd can do that would work in the general case, but libldap could expose a callback as an ldap_set_option to interface with OpenSSL/GnuTLS and do the work that's necessary. If you could update the patch to that effect, you can load a module that hooks in there and implements your requirements.
*** Issue 9757 has been marked as a duplicate of this issue. ***
A patch along the lines on comment#12 welcome.