282687 - LDAP: TLS Support

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[guillomovitch]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041009 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041009 Firefox/1.0

The following patch allow to use ldap uri instead of just serveur:port format.
It has the advantage of making ldapi and ldaps connection possible. It keep the
actual LDAPserver parameter name for compatibility, altough changing it to
LDAPuri would be more coherent. It also add LDAPcafile parameter, needed for ldaps. 

Reproducible: Always

Comment 1

[guillomovitch]

Attachment: Patch against 2.18 version

Comment 2

[Dave Miller [:justdave]]

hmm, there's an existing bug with patches for LDAPS on it somewhere, but I think
it just adds more params...

Comment 3

[guillomovitch]

Attachment: More complete patch against 2.19.2

The new patch is much less conservative than the previous one, replacing
LDAPserver with LDAPuri parameter, and providing updated documentation for the
new options.

BTW, having generated documentation (txt, html, pdf) in CVS instead of just
source documentation is quite... weird, and makes diffing uneasy.

Comment 4

[Dave Miller [:justdave]]

(In reply to comment #3)
> The new patch is much less conservative than the previous one, replacing
> LDAPserver with LDAPuri parameter, and providing updated documentation for the
> new options.

(I haven't looked at the patch) does it upgrade the existing parameters to the
new one when checksetup.pl runs if someone using the existing LDAP stuff upgrades?

> BTW, having generated documentation (txt, html, pdf) in CVS instead of just
> source documentation is quite... weird, and makes diffing uneasy.

We don't have the generated docs in CVS.  Where do you see that?  The xml files
and the related image files are all we have in cvs.

Comment 5

[guillomovitch]

>Does it upgrade the existing parameters to the
>new one when checksetup.pl runs if someone using the existing LDAP stuff upgrades?
Good point. Here is an additional patch to Config.pm that should take care of
that (I'm not 100% however).

>We don't have the generated docs in CVS.  Where do you see that?  The xml files
>and the related image files are all we have in cvs.
I feel a bit stupid here, they were acutally in my own CVS where I imported the
whole 2.19.2 archive :/
I guess they are distributed on purpose to avoid to user the need to build them ?

Comment 6

[guillomovitch]

Attachment: Convert previous LDAPserver parameter to new LDAPuri parameter

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Patches need review before they can be approved for check-in.  Please re-request
approval once this bug has a reviewed patch ready to be checked in.

Comment 8

[guillomovitch]

Attachment: Patch against CVS head

Here is an updated patch against CVS head, so as to make reviewing easier.

Comment 9

[Frédéric Buclin]

Comment on attachment 174751 [details] [diff] [review]
More complete patch against 2.19.2

removing pending requests on obsolete patches...

Comment 10

[guillomovitch]

Attachment: Patch against CVS head with spaces instead of tabs

Comment 11

[Christian Reis]

Comment on attachment 177650 [details] [diff] [review]
Patch against CVS head with spaces instead of tabs

Sorry for taking so long to look at this.

>Index: webtools/bugzilla/defparams.pl
>+   desc => 'The URI your LDAP server. (e.g. ' .

The URI of your LDAP server.

>+   name => 'LDAPcafile',
>+   desc => 'The certificate file needed for LDAPS connection',

Needed for an LDAPS connection. This is only used if your LDAP URI uses LDAPS.

>Index: webtools/bugzilla/Bugzilla/Config.pm
>+    # switch from ldap server name to ldap uri
>+    if (exists $param{'LDAPserver'} && !exists $param{'LDAPuri'}) {
>+        $param{'LDAPuri'} = 'ldap://' . $param{'LDAPserver'};
>+        delete $param{'LDAPserver'};
>+    }

I think this could be done in checksetup.pl, couldn't it? That's the graveyard
for migration code.

>Index: webtools/bugzilla/Bugzilla/Auth/Verify/LDAP.pm
>+    my $LDAPcafile = Param("LDAPcafile");
>+
>+    my $LDAPconn;
>+    if ($LDAPcafile) {
>+       $LDAPconn = Net::LDAP->new(
>+           $LDAPuri,
>+           cafile => $LDAPcafile,

Do we need to do any verification on cafile permissions, or does LDAP->new()
give us acceptable error output?

>+           version => 3

Is LDAP always version 3?

>Index: webtools/bugzilla/docs/xml/installation.xml
>+            <para>This parameter should be set to the URI of your LDAP
>+              server. All kind of URI schemes (ldap://, ldaps://,

I'd use "All URI schemes" here.

>+              ldapuri://) are supported, with optional port

with an optional

>-            <para>Ex. <quote>ldap.company.com</quote>
>-             or <quote>ldap.company.com:3268</quote>
>+            <para>Ex. <quote>ldap://company.com</quote>
>+              or <quote>ldap://company.com:3268</quote>

Do these URIs need to and in slashes?

>+            <para>This parameter should be set to the path to the
>+              certification authority certificate file needed for
>+              LDAPS connections.

Do we need to tell the user what format the file needs to be or is this common
sense? 

>Index: webtools/bugzilla/template/en/default/account/auth/ldap-error.html.tmpl
>-  [% CASE "server_not_defined" %]
>-    The LDAP server for authentication has not been defined.
>+  [% CASE "uri_not_defined" %]
>+    The LDAP URI for authentication has not been defined.

This looks good apart from that. Are no traces of LDAPserver left in the
codebase? Has this been tested enough?

Comment 12

[guillomovitch]

Attachment: Updated patch with minor corrections

>Index: webtools/bugzilla/Bugzilla/Config.pm
>>+    # switch from ldap server name to ldap uri
>>+    if (exists $param{'LDAPserver'} && !exists $param{'LDAPuri'}) {
>>+	  $param{'LDAPuri'} = 'ldap://' . $param{'LDAPserver'};
>>+	  delete $param{'LDAPserver'};
>>+    }


>I think this could be done in checksetup.pl, couldn't it? That's the graveyard

>for migration code.
Actually, it seems checksetup.pl calls this method (UpdateParams) for managing
parameters change, so it seems to be the correct way (tm).
>Index: webtools/bugzilla/Bugzilla/Auth/Verify/LDAP.pm
>>+    my $LDAPcafile = Param("LDAPcafile");
>>+
>>+    my $LDAPconn;
>>+    if ($LDAPcafile) {
>>+	 $LDAPconn = Net::LDAP->new(
>>+	     $LDAPuri,
>>+	     cafile => $LDAPcafile,


>Do we need to do any verification on cafile permissions, or does LDAP->new()
>give us acceptable error output?
I tested, it just return undef instead of a real objet, with no explicit error
message. I added a check method in defparams.pl for ensuring the file exists
and is readable.

>>+	     version => 3

>Is LDAP always version 3?
No, but that's just the default value. I removed it.

>-	      <para>Ex. <quote>ldap.company.com</quote>
>>-	       or <quote>ldap.company.com:3268</quote>
>>+	      <para>Ex. <quote>ldap://company.com</quote>
>>+		or <quote>ldap://company.com:3268</quote>

>Do these URIs need to and in slashes?
No, according to Net::LDAPS man page and my own tests.

>Are no traces of LDAPserver left in the
codebase?
According to grep, no.

>Has this been tested enough?
I personaly use it on a production server with ldapi:// without troubles for
some month now.

Comment 13

[Frédéric Buclin]

Comment on attachment 188786 [details] [diff] [review]
Updated patch with minor corrections

defparams.pl no longer exists

Comment 14

[guillomovitch]

Attachment: Updated patch

Comment 15

[guillomovitch]

Attachment: Updated patch

Previous diff was wrong, here is a correct one.

Comment 16

[Nahor]

If you want to support TLS, it's fairly easy: if the URI is ldap:// and there is a LDAPcafile, then try to use TLS when initializing the LDAP connection.

From my old patch (Bug #250916), to put between "LDAP->new" and "$LDAPconn->bind":
> $mesg = $LDAPconn->start_tls(verify => "require", cafile => $LDAPcafile);
> if($mesg->code) {
>   return (AUTH_ERROR, undef, "tls_connect_failed", { errstr => $mesg->error });
> }

Also, when creating the connection (LDAP->new), version 3 of LDAP is required so you may want to add the "version => 3" back, at least if TLS is going to be used.


My patch is also doing some check on Net::SSLeay and IO::Socket::SSL. I don't know if it's actually useful or not.


It would also be cool if you could support a CA directory instead of a specific CA file. So if "LDAPcafile" is actually a directory, use "capath => $LDAPcafile" instead of "cafile => $LDAPcafile" for both SSL and TLS.


On a side note, reading the documention about Net::LDAPS I saw that:
  You cannot have more than one LDAPS connection at any one time, due to restrictions in the underlying Net::SSLeay code.
I don't know if it applies to TLS too (likely) and if it is actually a big issue. But I'm just letting you know :)

Comment 17

[guillomovitch]

Attachment: Enhanced patch

This new version include Nahor's suggestions:
- TLS support
- certificate can be given as a file or as a directory

Morevoer, it add configurable protocol version.

Comment 18

[Nahor]

>Index: Bugzilla/Auth/Verify/LDAP.pm
...
>+           version => Param("LDAPversion")

Possible values for LDAPversion are 'LDAPv2', 'LDAPv3'. Is that accepted by Net::LDAP? The documentation and older Bugzilla code use just 3.


>Index: contrib/syncLDAP.pl

contrib/syncLDAP.pl has not been updated to handle TLS
Can it use the code in Bugzilla/Auth/Verify/LDAP.pm to avoid that kind of issue in the future?

Comment 19

[guillomovitch]

Attachment: fixed patch

>Possible values for LDAPversion are 'LDAPv2', 'LDAPv3'. Is that accepted by
>Net::LDAP? The documentation and older Bugzilla code use just 3.
You're right, I wrongly interpreted the man page of Net::LDAP.

>contrib/syncLDAP.pl has not been updated to handle TLS
Fixed too

>Can it use the code in Bugzilla/Auth/Verify/LDAP.pm to avoid that kind of issue
>in the future?
Absolutly, it would be a good idea, especially regarding my other pending LDAP patch (#320751). However, they are some issues, such as different error handling mechanism in command line and in CGI context :/

Comment 20

[Nahor]

>Index: Bugzilla/Config/Common.pm
...
>+sub check_ldap_cert {
>+    my ($value) = @_;
>+    return "LDAP version 3 is required"
>+        unless Param("LDAPversion") eq 'LDAPv3';

This is bound to fail since the value is now 2 or 3 ;)


>Index: docs/xml/installation.xml
...
>+	 <varlistentry id="param-LDAPversion">
>+          <term>LDAPversion</term>
>+          <listitem>
>+	    <para>This is the LDAP protocol version used to bind the LDAP
>+	      server.
>+            </para>
>+            <para>Either <quote>LDAPv2</quote>
>+              or <quote>LDAPv3</quote>
>+            </para>
>+           </listitem>
>+         </varlistentry>

The comment has not been updated either. It's probably not even necessary to specify the values since the user has a limited choice anyway.

And just to be picky, the indentation is wrong in several places in the file. Or it's just that tabs and spaces are mixed.


>Index: template/en/default/admin/params/ldap.html.tmpl
...
>+  LDAPcacert => "The certificate file or directory needed for encrypted LDAP " _
>+                "connection. It will use TLS with normal LDAP URIs (ldap://), " _
>+                "and SSL with secure LDAP URIs (ldaps://).",

What about rephrasing the second sentence to something like:
"It is required for ldaps:// URIs (SSL) and will trigger TLS for ldap:// URIs"
It's not clear otherwise that setting the certificate is enough to start using TLS.

Comment 21

[guillomovitch]

Attachment: fixed patch

>This is bound to fail since the value is now 2 or 3 ;)
Ooops, fixed.

>The comment has not been updated either. It's probably not even necessary to
>specify the values since the user has a limited choice anyway.
Done.

>And just to be picky, the indentation is wrong in several places in the file.
>Or it's just that tabs and spaces are mixed.
Strange, I thought checks would have detected tabs there ? I removed them.

>What about rephrasing the second sentence to something like:
>"It is required for ldaps:// URIs (SSL) and will trigger TLS for ldap:// URIs"
Done.

Comment 22

[Vlad Dascalu]

Comment on attachment 206574 [details] [diff] [review]
fixed patch

+if ($LDAPcacert) {
+   $LDAPconn = Net::LDAP->new(

This identation is wrong, 3 spaces only instead of 4.

-   print "Connecting to LDAP server failed. Check LDAPserver setting.\n";
+   print "Connecting to LDAP server failed. Check LDAPuri setting.\n";

Same here.

+    );
+   print "TLS initialization failed. Check LDAPcacert setting.\n";

Same here.

+              certificate needed for encrypted LDAP connection, wether SSL
+              (ldaps://), or TLS (ldap://) ones.

'wether' should probably be 'whether'.

Question: do we automatically set the port to 636 in case of SSL? There is a patch in bug 216902 that does this, and it is most likely to conflict with this one. I like the one there because it makes SSL look much easier, and it automatically switches to 636.

However this one offers support for v2, TLS, cacerts and has more features (but also more complexity).

It looks good, and since we're opening up the development cycle for 2.24 now, we'll have time to iron it out until Sep 2006.

Comment 23

[Vlad Dascalu]

The nits can be fixed upon checkin, if approved.

Comment 24

[Vlad Dascalu]

Comment on attachment 206574 [details] [diff] [review]
fixed patch

In addition to comment #22:

+    # switch from ldap server name to ldap uri

Here LDAP and URI should be capitalized.


Sorry for not catching this earlier:

+sub check_ldap_cert {
+    my ($value) = @_;
+    return "LDAP version 3 is required"
+        unless Param("LDAPversion") == 3;
+    return "non existing file or directory $value"
+        unless -f $value || -d $value;
+    return "non readable file or directory $value"
+        unless -r $value;
+    return "";
+}
+

This is incorrect:

-> we shouldn't require version 3 if the field is empty.
-> we should allow an empty field.

This produces strange errors, like a fatal error only by clicking the "Reset" option in editparams.cgi for the cacert field.

I'd like a new patch, with this fixed (and the previous comments).

Comment 25

[guillomovitch]

Attachment: Corrected patch

Here is a new patch, that address all your comments. Feel free to fix yourself any other remaining detail if needed.

The patch from #216902 is quite uncomplete (I really don't know how using ldaps without a certificate is supposed to work), and what it does (extracting protocol, server and port) is useless, as already handledy by Net::LDAP itself.
 
This one is much more complete, with documentation, parameter checking, support of external sync_ldap script, TLS and SSL support, etc... I don't think it adds complexity, as all you need to do is to use a ldaps:// URI, and a certificate.

Of course, they will conflict.

Comment 26

[u97006]

Just tried the patch from comment #25 against bugzilla 2.22-rc1 and get the following:

...
patching file template/en/default/admin/params/ldap.html.tmpl
Hunk #1 FAILED at 25.

Afterwards the bugzilla is not working anymore (error 500). Can anyone confirm this or has a resolution?

Comment 27

[u97006]

... still the same with 2.22

Comment 28

[guillomovitch]

Attachment: Corrected patch

I did not had problems applying the patch on current trunk... excepted it didn't work, as I forgot the part related to the LDAP autentification itself :/

This one should fix the issue.

BTW, it supersedes bug #216902.

Comment 29

[Vlad Dascalu]

Comment on attachment 219782 [details] [diff] [review]
Corrected patch

I'll have some free time on February 2007, which is too late in order to handle this review, requesting it from the wind (but feel free to reassign it to a specific reviewer)

Comment 30

[Frédéric Buclin]

Comment on attachment 219782 [details] [diff] [review]
Corrected patch

There has been a heavy rewrite of the Auth stuff in bug 300410. Bugzilla/Auth/Verify/LDAP.pm must be updated accordingly. And template/en/default/account/auth/ldap-error.html.tmpl no longer exists.

Comment 31

[guillomovitch]

Attachment: Updated patch

Updated to latest Auth.pm rewrite

Comment 32

[Max Kanat-Alexander]

Comment on attachment 223072 [details] [diff] [review]
Updated patch

No need to be able to change the LDAP version.

LDAPserver already supports the URI format, so there's no need to change the name of the param. There's also no need to require admins to put ldap:// at the front of their URLs.

Are there LDAP servers that support SSL that don't support TLS? Is there any reason to support both versions of the SSL connection?

Could you give me an actual use case for when somebody would want to specify the CA Cert for their LDAP server?

Comment 33

[guillomovitch]

Attachment: Simplified patch

This is a much simplified patch, dropping LDAP protocol, certs management, and the  mandatory change from server name to server URI, and doesn't handle contrib script neither. It just add TLS and ldapi protocol support, as well as documentation clar
ification.

I still think than the whole parsing of LDAPserver parameter to avoid explicitely asking an LDAP URI to bugzilla admins duplicate Net::LDAP, brings added complexity in bugzilla code base, and troubles to LDAP admins used to the distinction between both syntax in OpenLDAP world. I'd rather educate admins by explicit documentation and parameters checking than substituting to them.

Comment 34

[Max Kanat-Alexander]

Comment on attachment 223093 [details] [diff] [review]
Simplified patch

>+  LDAPstartls => "Wether to require TLS communication once normal LDAP " _
>+                 "connection achieved with the server.",

  Nit: "Whether."

  Also, if you want to explain what TLS is in this description, and post another patch, you can carry forward r+.

Comment 35

[guillomovitch]

Attachment: Corrected patch

Comment 36

[Nahor]

> Are there LDAP servers that support SSL that don't support TLS? Is there any
> reason to support both versions of the SSL connection?

Beside old servers that may not support the newer TLS but only the older SSL, it's certainly a configuration option and some server may be configured to not accept TLS because they never had any TLS client.


> Could you give me an actual use case for when somebody would want to specify
> the CA Cert for their LDAP server?

The CA cert is not for the LDAP server but for the LDAP client (i.e. Bugzilla) to verify the certificate returned by the LDAP server. Like your browser has a list of CA certs to verify the certificates returned by the secure websites.

If you don't have it, Bugzilla will have to trust blindly the LDAP server certificate allowing for man-in-the-middle attacks.
Unless the LDAP perl module has some kind of global configuration file where one can indicates where their CA cert dir is, this is then a required option.

And a CA cert file option is also pratically necessary in case the LDAP server uses a self-signed certificate (you usually don't want to put your own CA cert file with the other official CA certs)

Comment 37

[Kevin Benton]

Comment on attachment 223097 [details] [diff] [review]
Corrected patch

First, thanks for the continued work toward making these changes.

From my perspective, it looks like some of the conversions of ldap(s) to ldap(s|i) were missed in Bguzilla/Auth/Verify/LDAP.pm, potentially confusing developers in the future.

In the docs, it's good that you list ldap and ldaps, but you don't mention ldapi and what it's used for.  In this case and the next diff set (for ...ldap.html.tmpl), you chose to remove help that describes ldap vs ldaps but doesn't explain ldapi.  Users aren't going to know that ldapi is a possible selection unless we tell them.

As I see it, these issues create usability problems for those what are just getting started with LDAP in Bugzilla, especially if they're used to using an ActiveDirectory server on the other end.  I think that a little more documentation is better than less in this case, especially since we now support ldapi with your update (but we didn't before).

Please be aware that I have not tested this code's functionality at this point.

Comment 38

[Max Kanat-Alexander]

Comment on attachment 223097 [details] [diff] [review]
Corrected patch

Oh, I'm sorry. Kevin's right--I didn't notice that you removed the documentation about ldaps from the parameter. That should stay.

Comment 39

[guillomovitch]

From Kevin and Max comments, there seems to be disagreement about what should be in documentation.

My point is that bugzilla documentation should avoid discussing generic LDAP connections issues, such as ldap vs ldaps vs ldapi, as it would just duplicate poorly standard LDAP admin guides, and focuses instead on specific bugzilla handling issues, such as the fact than the misnamed "server" option accept both server and URI syntax. I'm ok to add an ldapi-based exemple, not to explain what it does here. See for instance Net::LDAP man page, which mention the various available values without dicussing them. The same is true for TLS: if you don't know what it is, you probably don't need it.

On the other hand, we could enhance current installation guide, and add pointers to existing guides, such as OpenLDAP admin guide for instance 'http://www.openldap.org/doc/admin23/).

BTW, I forgot to mention hostnames could be postfixed by port numbers also :)

Comment 40

[Max Kanat-Alexander]

(In reply to comment #39)
> as it would just duplicate poorly standard LDAP admin guides,

  That's okay. We're here to be nice to the user. Extremely nice. More nice to the user than you could possibly imagine. That's the goal: "More nice to the user than you could possibly imagine."


> On the other hand, we could enhance current installation guide, and add
> pointers to existing guides, such as OpenLDAP admin guide for instance
> 'http://www.openldap.org/doc/admin23/).

  There are many LDAP servers in the world besides OpenLDAP. Many, many. I actually know very few people using Bugzilla with OpenLDAP. Most are using Active Directory.

> BTW, I forgot to mention hostnames could be postfixed by port numbers also :)

  Sure, you can add that information in there.

  It's okay to have more text there.

Comment 41

[Kevin Benton]

(In reply to comment #40)
> (In reply to comment #39)
> > as it would just duplicate poorly standard LDAP admin guides,
> 
>   That's okay. We're here to be nice to the user. Extremely nice. More nice to
> the user than you could possibly imagine. That's the goal: "More nice to the
> user than you could possibly imagine."
...

I have to say that I greatly benefitted from that kind of viewpoint, especially during my initial months as a Bugzilla developer.  Rather than going to Perl and MySQL resources to ask certain questions, I relied on our IRC channel #mozwebtools for the assistance I needed and I was rewarded nicely with lots of help.

As Max hinted to above, just because others don't document their "stuff" well, doesn't mean we should follow suit.  If it's hard to learn about how to use LDAP properly, no matter if it's our documentation or LDAP documentation, it's still hard to learn.  We need to do our part to make it easier for our users to use it.  If we miss that mark, we'll loose users to other products and they will tell others that our product is hard to use even though the real issue is with LDAP documentation (in this case).

I'll say it again - your contributions are appreciated.  Thanks for working on this update!  :)

Comment 42

[Frédéric Buclin]

re-request approval when you have an updated patch with r+ on it.

Comment 43

[Max Kanat-Alexander]

*** Bug 250916 has been marked as a duplicate of this bug. ***

Comment 44

[Max Kanat-Alexander]

*** Bug 339727 has been marked as a duplicate of this bug. ***

Comment 45

[guillomovitch]

Attachment: Simplified patch

This patch just deals with LDAP connection itself. It removed useless code, by letting Net::LDAP handle with ldap connection string directly. And it just add documentation above existing one.

TLS support has been excluded, so as to be dealt separatly.

Comment 46

[Max Kanat-Alexander]

Let's keep this bug focused on the TLS support that we added, and file another bug for fixing the code.

Comment 47

[guillomovitch]

Attachment: pure TLS support

Comment 48

[Max Kanat-Alexander]

Comment on attachment 223841 [details] [diff] [review]
pure TLS support

Great!

Comment 49

[Frédéric Buclin]

Checking in Bugzilla/Auth/Verify/LDAP.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/LDAP.pm,v  <--  LDAP.pm
new revision: 1.11; previous revision: 1.10
done
Checking in Bugzilla/Config/LDAP.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/LDAP.pm,v  <--  LDAP.pm
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/admin/params/ldap.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/ldap.html.tmpl,v  <--  ldap.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.68; previous revision: 1.67
done

Comment 50

[Kevin Benton]

(In reply to comment #47)
> Created an attachment (id=223841) [edit]
> pure TLS support
> 

Nice job! :)

Comment 51

[Max Kanat-Alexander]

Added to the relnotes currently attached to bug 349423.