6.0.0-git
2019-07-20

[#13730] Implementation of peer verification in TLS connections
Summary Implementation of peer verification in TLS connections
Queue Horde Framework Packages
Type Enhancement
State Assigned
Priority 1. Low
Owners jan (at) horde (dot) org, slusarz (at) horde (dot) org
Requester m_horde (at) secure (dot) mailbox (dot) org
Created 2014-12-01 (1692 days ago)
Due
Updated 2015-06-23 (1488 days ago)
Assigned 2015-06-23 (1488 days ago)
Resolved
Milestone
Patch Yes

History
2015-06-23 13:57:22 Michael Rubinsky Comment #17
Assigned to Jan Schneider
Assigned to Michael Slusarz
State ⇒ Assigned
Reply to this comment
Sure, once one of the maintainers has the time to review it properly.
2015-06-23 06:27:11 m_horde (at) secure (dot) mailbox (dot) org Comment #16 Reply to this comment
Is there any chance that my patch becomes part of horde in the future? 
Do I have to make further changes?
2015-03-13 16:05:01 m_horde (at) secure (dot) mailbox (dot) org Comment #15
New Attachment: 0001-Implementation-of-peer-verification-in-TLS-connectio[3].patch Download
Reply to this comment
I updated my patch to use the context parameter. The patch works 
mainly the same way it does before. However, the logic to create the 
context array was moved to /imp/lib/Imap.php.

The pinning of a certificate by its fingerprint cannot be done with 
the context parameter of Socket/Client, because the server certificate 
is not availabe when the connection is established. It becomes 
availabe when the starttls command is sent. Therefore I added the 
items "sha1", "md5" and "sha256" to the array $params in Client.php. 
These will be used to check the certificate fingerprints during TLS 
handshake.
2015-02-12 12:45:51 Jan Schneider Comment #14 Reply to this comment
Please forgive my ignorance, but what was wrong with my last patch 
(comment #6). There were no regressions, because socket_client works 
as usually as long as tls_params is not set.
Nothing was wrong with it, but I requested it to be more generic. This 
has been implemented now, so you can update your patch to use the new 
context parameter.
2015-02-07 09:38:32 m_horde (at) secure (dot) mailbox (dot) org Comment #13 Reply to this comment
Please forgive my ignorance, but what was wrong with my last patch 
(comment #6). There were no regressions, because socket_client works 
as usually as long as tls_params is not set.

Further I do not see the probem with the new context parameter 
(comment #9). As long as this paramter is an empty array, 
socket_client will create the stream_context the same way it did before.
2015-02-07 08:00:03 Michael Slusarz Comment #12 Reply to this comment
Ummmm.... never mind.

Although it would be useful to re-add the comment to indicate why this 
is the default setting.
2015-02-07 07:57:27 Michael Slusarz Comment #11 Reply to this comment
As currently implemented, this is BC breaking for Horde applications.

See: 
https://github.com/horde/horde/commit/e3b73e4bdc91580b720f42a573e598c69ee4c808#diff-ae0fe3dbb130e4ee96821ffd084420e4R95

Horde 5 doesn't require these context parameters and this will break 
PHP 5.6 installations.
2015-02-04 12:25:49 Jan Schneider Comment #10 Reply to this comment
The generic stream context parameter has been implemented.
2015-02-04 12:25:37 Git Commit Comment #9 Reply to this comment
Changes have been made in Git (master):

commit e3b73e4bdc91580b720f42a573e598c69ee4c808
Author: Jan Schneider <jan@horde.org>
Date:   Wed Feb 4 13:19:06 2015 +0100

     Add parameter for stream context settings (Request #13730).

  .../Imap_Client/doc/Horde/Imap/Client/UPGRADING    |   10 ++
  .../Imap_Client/lib/Horde/Imap/Client/Base.php     |    3 +
  .../Imap_Client/lib/Horde/Imap/Client/Socket.php   |    1 +
  .../Horde/Imap/Client/Socket/Connection/Base.php   |    4 +-
  .../lib/Horde/Imap/Client/Socket/Pop3.php          |    1 +
  framework/Imap_Client/package.xml                  |   16 ++--
  framework/Smtp/doc/Horde/Smtp/UPGRADING            |   10 ++
  framework/Smtp/lib/Horde/Smtp.php                  |   98 
++++++++------------
  framework/Smtp/package.xml                         |    2 +
  .../doc/Horde/Socket/Client/UPGRADING              |    2 +
  .../Socket_Client/lib/Horde/Socket/Client.php      |   32 ++++---
  framework/Socket_Client/package.xml                |    2 +
  12 files changed, 99 insertions(+), 82 deletions(-)

http://github.com/horde/horde/commit/e3b73e4bdc91580b720f42a573e598c69ee4c808
2015-01-26 16:40:24 Jan Schneider Comment #8
State ⇒ Feedback
Reply to this comment
Instead of limiting the stream context parameters to TLS 
configuration, I suggest you allow to pass a generic 'context' item in 
Socket\Client's $params. This way people can specify arbitrary stream 
context parameters.
2015-01-19 15:18:18 rb (at) stylite (dot) de Comment #7 Reply to this comment
Just to let you know, I'm also interested in implementing peer 
verification and pinning of certificates in EGroupware.

I will follow the ticket to get notified once this feature is 
available in Horde_Imap_Client.

Cheers

Ralf
2014-12-26 00:44:06 m_horde (at) secure (dot) mailbox (dot) org Comment #6
New Attachment: 0001-Implementation-of-peer-verification-in-TLS-connectio[2].patch Download
Reply to this comment
On my server I need the peer verification to connect to a remote IMAP 
server. Therefore I focused my effort on that. To avoid using $GLOBALS 
I moved all configuration options in the backend config of IMP. This 
way every backend can be configured seperately.

=== How does it work? ===
There is a new array in the backend config file named "tls_params". 
This stores all necessary information for the TLS connection. Please 
refer to the documentation in backends.conf for details. The involved 
libraries are modified to pass tls_params to Socket Client, where they 
are used to create the SSL context. A new feature is the fingerprint 
check. This cannot be done directly by setting the SSL context, 
because the encryption is enabled after the socket is created. 
Therefore this check has to be done seperatly after the starttls 
command.

=== Benefits ===
Peer verification with a trusted bundle of certificate authorities can 
be enabled for remote IMAP servers. Further the connection can be 
limited to a specified certificate by its fingerprint.

=== Regressions ===
There are no regression I am aware of. As long as the new 
configuration array (tls_params) is not used, the library will work as 
usually.
2014-12-20 20:26:46 Michael Slusarz Comment #5 Reply to this comment
I see the point that an independent library must not rely on horde's 
configuration. However, I see no point in storing the same 
information multiple times. As long as the Socket Client library is 
used within horde, horde's configuration will be available. 
Therefore this information should be used if its available.
That's fine to want to not reproduce configs, but you simply can't 
assume globals exist outside of Horde applications and Horde_Core.   
The presence of $conf is 100% reliant on Horde_Registry being both 
available and invoked.

So this is getting better, but there simply cannot be any reference to 
global variables in Socket Client code.  These dependencies must be 
injected into Socket Client solely by configuration parameters.  This 
is the whole point of Factories and dependency injection (see 
Horde_Core).  In fact, much of Horde can be seen as the configuration 
layer to our various libraries.
2014-12-08 09:19:37 m_horde (at) secure (dot) mailbox (dot) org Comment #4
New Attachment: 0001-Implementation-of-peer-verification-in-TLS-connectio[1].patch Download
Reply to this comment
There was a problem with the attached file. So, I try again.
2014-12-08 09:15:46 m_horde (at) secure (dot) mailbox (dot) org Comment #3 Reply to this comment
I see the point that an independent library must not rely on horde's 
configuration. However, I see no point in storing the same information 
multiple times. As long as the Socket Client library is used within 
horde, horde's configuration will be available. Therefore this 
information should be used if its available.

For those who do not want to store the information for peer 
verification globally or those who use the library idependently from 
horde, I implemented the possibility to override the information in 
$GLOBALS.

=== How does it work? ===
There is a new parameter named $tls_params. As far as I can see there 
is no use for $params, but since I am not familiar with horde's code I 
avoided to use it. $tls_params may be used to override the parameters 
from $GLOBALS['conf']['openssl']. Whene $tls_params['source'] is set 
to 'override', $GLOBALS will not be used in any case, even if no other 
values in $tls_params are given. Otherwise every given configuration 
will be used, whereas $tls_params overrules $GLOBALS.

=== Benefits ===
The global configuration will be used if it is available. If it is not 
available or the peer verification is not configured the libraray 
works as before, which means the peers will not be verified. $GLOBALS 
can be overwritten or disabled by $tls_params.

=== Regressions ===
The library will not change in behavior until a deliberate change by 
an admin running the horde installation is made. As long as the peer 
verification is not enabled globally and $tls_params is not set during 
instantiation of a client object, the peer verification will be 
disabled as it was until now. So, there should be no regressions.
2014-12-01 22:44:38 Michael Slusarz Comment #2 Reply to this comment
$GLOBALS['conf'] does not exist in Socket_Client.  Socket_Client is an 
independent library from the Horde application, so it cannot use 
Horde-specific variables like $conf.  So you will need to add these as 
config options to the Horde_Socket constructor (which, in turn, means 
they need to be added as config options in both the Horde_Imap_Client 
library and the Horde_Smtp library).

And this means that these options cannot be globally defined in 
Horde's conf.xml.  Well, actually they can (and be used as defaults), 
but there also has to be options to be able to override these defaults 
in every location where we are using the Socket Client library.  i.e. 
these defaults can be defined with IMAP authentication, POP3 
atuhentication, SMTP authentication, and in IMP both when configuring 
the IMAP/POP3 backend and the SMTP backend.

That's really the main reason this hasn't been added to Horde 
previously - since it's the configuration code that is both messy and 
a PITA.  (Unfortunately, this is an issue with security/encryption 
options in general, so there's not a ton we can do about that).
2014-12-01 00:02:59 m_horde (at) secure (dot) mailbox (dot) org Comment #1
Type ⇒ Enhancement
State ⇒ New
Priority ⇒ 1. Low
Summary ⇒ Implementation of peer verification in TLS connections
Queue ⇒ Horde Framework Packages
Milestone ⇒
Patch ⇒ Yes
New Attachment: 0001-Implementation-of-peer-verification-in-TLS-connectio.patch Download
Reply to this comment
Horde does not verify the peer certificate during TLS handshake and 
accepts all ciphers when acting as client. This is probably to enable 
self-signed certificates. However, in an environment where a secure 
connection matters this behavior is not acceptable. Therefore I added 
the possibility to enable peer verification and cipher selection. This 
options can be found in the openssl tab in horde's configuration. If 
enabled, this options will be used by client.php to verify the peer 
certificate during the TLS handshake.

=== Benefits and Limitations ===
The main benefit is to enforce a secure connection between the server 
running horde and the backend server. Other security meassurements 
(e.g. fingerprint checking) provided by PHP are not part of this 
implementation.

=== Regressions ===
The patch was only checked with an IMAP server as backend. However, 
the changes have no effect until the peer verification is deliberately 
enabled. So, there should not be any regressions.

Saved Queries