250916 - LDAP TLS support

Status: RESOLVED DUPLICATE of bug 282687

(Bugzilla :: User Accounts, enhancement)

Description

[Nahor]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040710 Firefox/0.9.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040710 Firefox/0.9.0+

Added support for TLS encryption when authenticating agains a LDAP server

Reproducible: Always
Steps to Reproduce:

Comment 1

[Nahor]

Attachment: Add TLS support to LDAP

Comment 2

[Dave Miller [:justdave]]

Related: bug 216902 (LDAPS).  I was just going to add the same there pointing
back here, but Nahor beat me to it :)

Comment 3

[Jouni Heikniemi]

Comment on attachment 152873 [details] [diff] [review]
Add TLS support to LDAP

I don't have the domain expertise to review LDAP code, but sending a request
over to Mike Morgan -- I hope he'll be able to take a look.

Some generic comments on the code; I'll mark my review- based on these, but
please wait for Mike's comments on the patch before you work more on this, so
you don't have to fix my comments if other developers think the approach must
be changed more radically.

>+sub check_LDAPencryption {
...
>+    if ($encryption eq 'none') {
>+        # No params
>+    } elsif ($encryption eq 'TLS') {

Why are you doing this first if block?

>+        return "LDAPCACertFile is empty" unless Param("LDAPCACertFile");

You might want to verify the existence of that file as well?

>+   name => 'LDAPuseEncryption',
>+   desc => 'Encrypt the connection to the LDAP server',
>+   type => 's',
>+   choices => ['none', 'TLS'],
>+   default => 'none',
>+   checker => \&check_LDAPencryption

Hmm... The desc makes it sound like a boolean, but it's actually a select box.
If there are other auth mechanisms apart from TLS, you'll want to make it
"LDAPEncryption" and perhaps use a desc like 'The encryption method used for
LDAP connections.'

>+   name => 'LDAPCACertFile',
>+   desc => 'Certificate of the CA who signed the server\'s certificate. ' .
>+           'The certificates must be in PEM format',
>+   type => 't',
>+   default => ''

TLS should be mentioned here clearly; people using non-secured LDAP connections
shouldn't be confused by this. Also, you'll want to make it clear that it's a
file name you're inputting here.

Comment 4

[Nahor]

(In reply to comment #3)

> >+sub check_LDAPencryption {
> ...
> >+    if ($encryption eq 'none') {
> >+        # No params
> >+    } elsif ($encryption eq 'TLS') {
> 
> Why are you doing this first if block?

I copied the test from check_loginmethod() function. The idea is that later one
can add SSL support so one should see the if() like a C switch() statement. It
may not be required but that way we know we didn't actually forget any test.

I agree with the other comments.

Comment 5

[bugzilla]

Attachment: patch to add START TLS support to Auth/Verify/LDAP.pm

Comment 6

[Gervase Markham [:gerv]]

IMO, LDAP support is the cause of enough random Params already without adding
another five!

Surely there must be some other way. Can we have an LDAP config file, or something?

Gerv

Comment 7

[bugzilla]

(In reply to comment #6)
> IMO, LDAP support is the cause of enough random Params already without adding
> another five!

These extra five params aren't required.  They're only there for people that
need them.  Even if there was an extra file it would still need to have all
these settings (to be complete) and it would be an *extra* file.  It would
probably be more confusing for the user since they'd wonder why they have to set
these values in a different place from all the other settings.

Many setups that require TLS to connect to the LDAP server will only need to
turn on TLS and specify a CA cert.  The other settings are only if the admin
wants the LDAP server to verify the client as well.

Comment 8

[Joel Peshkin]

Comment on attachment 160203 [details] [diff] [review]
patch to add START TLS support to Auth/Verify/LDAP.pm

I think that, as soon as multi-panel parameters are working, nobody will mind
the extra parameters. Therefore, I dont think the extra params should keep us
from landing this patch.

I'll leave that call up to justdave though.

Comment 9

[Dave Miller [:justdave]]

(In reply to comment #8)
> (From update of attachment 160203 [details] [diff] [review])
> I think that, as soon as multi-panel parameters are working, nobody will mind
> the extra parameters. Therefore, I dont think the extra params should keep us
> from landing this patch.

I agree.

Note that this has already missed the boat for both 2.18 and 2.20, and we still
have about 5 months to get the params cleaned up in 2.22.

Comment 10

[Frédéric Buclin]

Comment on attachment 160203 [details] [diff] [review]
patch to add START TLS support to Auth/Verify/LDAP.pm

defparams.pl no longer exists

Comment 11

[guillomovitch]

There is some overlap with the patch I submitted at #282687. Mine is more general (allowing any kind of LDAP access scheme), and more up-to-date. However, they are fewer options, and it while allowing SSL access, it does't allow TLS. Maybe we should try to merge them to ease integration ?

Comment 12

[Nahor]

I'm making this bug depend on bug #282687 instead of just marking it as DUPLICATE until it actually does support TLS.

Comment 13

[Max Kanat-Alexander]

*** This bug has been marked as a duplicate of 282687 ***