Merge lp:~ibeardslee/openobject-addons/users_ldap-tls into lp:~openerp-community/openobject-addons/stefan-therp_lp794584

[Ian Beardslee]

Adding STARTTLS support to users_ldap

Changes to users_ldap to allow connections to a server requiring STARTTLS.

Change in ..
* users_ldap/users_ldap_view.xml to allow the views in the interface.
* users_ldap/users_ldap.py to create the new database field (ldap_tls), collect that field in the query and to get the application to connect with STARTTLS if the value is true.
* users_ldap/__openerp__.py to describe the Configuration and Security Considerations.

This has NOT been tested against a non-STARTTLS LDAP server.

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Ian,

the code looks good, but you can prevent the duplicity by changing the res_company_ldap.connect() method itself instead of triggering STARTTLS on the result of this method twice (diff lines 45:46 and 54:55).

Personally, I would not put this detail in the tree view but that is a matter of taste.

If I merge this branch with my code, it will be less obvious that you contributed this feature once my branch gets merged with openobject-addons/trunk. Therefore, I presume that you will want to fire another merge request with the official branch and not have it merged here.

Thanks,
Stefan.

review: Needs Fixing

[Ian Beardslee (ibeardslee)]

Thanks for that feedback Stefan. I'm still wandering around the code, python, launchpad, bzr etc working out the best way to do things, pointers in the right direction help :)

I was trying to work out how to get it working in res_company_ldap.connect() I'll experiment further.

The detail in the tree view, hmm, yeah probably not required there. Made some sense at the time, but happy to defer to someone more familiar with the code and look and feel of the application.

When I get it sorted, I should resubmit with ..

Target Branch: ~openerp/openobject-addons/trunk
Prerequisite Branch: ~openerp-community/openobject-addons/stefan-therp_lp794584

Cheers,
Ian

[Ian Beardslee (ibeardslee)]

Hi Stefan,

I've started looking into moving the start_tls_s() into res_company_ldap.connect(). Aware that I am picking up Python while working on this, I thought I'd check on some things.

* initialize(uri) returns an LDAPObject object.
* res_company_ldap.connect() returns that object as 'conn'
* start_tls_s acts on that object and doesn't return an object.
* simple_bind_s does the same.

If we were to include the start_tls_s in with res_company_ldap.connect() we could probably try and do the same with the simple_bind_s statement that would follow it.

Things still seem to work properly when the conn.simple_bind_s(dn, password) statement in res_company_ldap.authenticate() is replaced with the conn.simple_bind_s(conf['ldap_binddn'] or '', conf['ldap_password'] or '') in res_company_ldap.query().

Would it be sane to use the following stanza in both res_company_ldap.authenticate() and res_company_ldap.query()? Making them the same, and therefore hopefully making it easier for people following to recognise that both would need to be updated.

  conn = self.connect(conf)
  if conf['ldap_tls']:
      conn.start_tls_s()
  conn.simple_bind_s(conf['ldap_binddn'] or '',
                     conf['ldap_password'] or '')

or is there a smart way of add those 4 lines to res_company_ldap.connect()?

Is there something I'm overlooking?

Cheers,
Ian

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Ian,

you cannot replace simple_bind_s(dn, password) with anything else, because this is where the user is authenticated ;-)

In the other instance of simple_bind_s, we are authenticating the configured LDAP user for querying the database. You should definitely not change this around.

I remain by my suggestion that you change res_company_ldap.connect() as follows:

connection = ldap.initialize(uri)
if conf['ldap_tls']:
      connection.start_tls_s()
return connection

Cheers,
Stefan.

[Ian Beardslee (ibeardslee)]

Ahh the confusion from using one's own login as the bind user. That makes sense. Thanks for the pointers :)

Made the changes, tested with the STARTTLS enabled LDAP server will resubmit the merge.

[Stefan Rijnhart (Opener) (stefan-opener)]

Your work landed on the correct branch, so I 'reject' this proposal to get it out of the way.

review: Disapprove

=== modified file 'users_ldap/__openerp__.py'
--- users_ldap/__openerp__.py	2011-06-15 17:36:15 +0000
+++ users_ldap/__openerp__.py	2011-08-17 09:35:16 +0000
@@ -49,6 +49,11 @@
 LDAP account that is used to verify if a user exists before
 attempting to authenticate it.
 
+Securing the connection with STARTTLS is available for LDAP
+servers supporting STARTTLS. The default is to require STARTTLS.
+For further options configuring the LDAP settings, refer to the
+ldap.conf manpage :manpage:`ldap.conf(5)`.
+
 Security Considerations
 +++++++++++++++++++++++
 Users' LDAP passwords are never stored in the OpenERP database,
@@ -77,6 +82,9 @@
 always fails and the LDAP server is queried to do the
 authentication.
 
+Enabling STARTTLS ensures that the authentication query to the
+LDAP server is encrypted.
+
 User Template
 +++++++++++++
 In the LDAP configuration on the Company form, it is possible to

=== modified file 'users_ldap/users_ldap.py'
--- users_ldap/users_ldap.py	2011-07-27 22:03:38 +0000
+++ users_ldap/users_ldap.py	2011-08-17 09:35:16 +0000
@@ -50,7 +50,8 @@
             args = []
         cr.execute("""
             SELECT id, company, ldap_server, ldap_server_port, ldap_binddn,
-                   ldap_password, ldap_filter, ldap_base, "user", create_user
+                   ldap_password, ldap_filter, ldap_base, "user", create_user,
+                   ldap_tls
             FROM res_company_ldap
             WHERE ldap_server != '' """ + id_clause + """ ORDER BY sequence
         """, args)
@@ -67,7 +68,11 @@
 
         uri = 'ldap://%s:%d' % (conf['ldap_server'],
                                 conf['ldap_server_port'])
-        return ldap.initialize(uri)
+
+        connection = ldap.initialize(uri)
+        if conf['ldap_tls']:
+            connection.start_tls_s()
+        return connection
 
     def authenticate(self, conf, login, password):
         """
@@ -211,12 +216,15 @@
             help="Model used for user creation"),
         'create_user': fields.boolean('Create user',
             help="Create the user if not in database"),
+        'ldap_tls': fields.boolean('Use TLS',
+            help="Use STARTTLS to connect to the LDAP server"),
     }
     _defaults = {
         'ldap_server': '127.0.0.1',
         'ldap_server_port': 389,
         'sequence': 10,
         'create_user': True,
+        'ldap_tls': True,
     }
 
 CompanyLDAP()

=== modified file 'users_ldap/users_ldap_view.xml'
--- users_ldap/users_ldap_view.xml	2011-06-22 10:21:18 +0000
+++ users_ldap/users_ldap_view.xml	2011-08-17 09:35:16 +0000
@@ -20,6 +20,7 @@
                             <field name="user"/>
                             <newline/>
                             <field name="sequence"/>
+                            <field name="ldap_tls"/>
                         </form>
                         <tree string="LDAP Configuration">
                             <field name="sequence"/>