Merge pull request #148 from jvperrin/allow-non-keytab-ldap-changes

Allow LDAP changes without a keytab

ocflib/account/creation.py

@@ -19,7 +19,7 @@
 import ocflib.account.validators as validators
 from ocflib.infra.kerberos import create_kerberos_principal_with_keytab
 from ocflib.infra.kerberos import get_kerberos_principal_with_keytab
-from ocflib.infra.ldap import create_ldap_entry_with_keytab
+from ocflib.infra.ldap import create_ldap_entry
 from ocflib.infra.ldap import ldap_ocf
 from ocflib.infra.ldap import OCF_LDAP_PEOPLE
 from ocflib.misc.mail import jinja_mail_env
@@ -126,8 +126,11 @@ def create_account(request, creds, report_status, known_uid=_KNOWN_UID):
             attrs['callinkOid'] = request.callink_oid
 
         with report_status('Creating', 'Created', 'LDAP entry'):
-            create_ldap_entry_with_keytab(
-                dn, attrs, creds.kerberos_keytab, creds.kerberos_principal,
+            create_ldap_entry(
+                dn,
+                attrs,
+                keytab=creds.kerberos_keytab,
+                admin_principal=creds.kerberos_principal,
             )
 
             # invalidate passwd cache so that we can immediately chown files

ocflib/account/manage.py

@@ -86,7 +86,7 @@ def change_password_with_keytab(username, password, keytab, principal, comment=N
     _notify_password_change(username, comment=comment)
 
 
-def modify_ldap_attributes(username, attributes, keytab, principal):
+def modify_ldap_attributes(username, attributes, **kwargs):
     """Adds or modifies arbitrary attributes of a user's LDAP record subject to
     minor validation beyond the LDAP schema.
 
@@ -98,11 +98,10 @@ def modify_ldap_attributes(username, attributes, keytab, principal):
         if not misc.validators.valid_login_shell(value):
             raise ValueError("Invalid login shell '{}'".format(value))
 
-    ldap_ocf.modify_ldap_entry_with_keytab(
+    ldap_ocf.modify_ldap_entry(
         utils.dn_for_username(username),
         attributes,
-        keytab,
-        principal,
+        **kwargs,
     )
 
 

ocflib/account/submission.py

@@ -47,7 +47,6 @@
 from ocflib.account.creation import send_rejected_mail
 from ocflib.account.creation import validate_request
 from ocflib.account.manage import change_password_with_keytab
-from ocflib.account.manage import modify_ldap_attributes as real_modify_ldap_attributes
 
 
 Base = declarative_base()
@@ -333,22 +332,6 @@ def change_password(username, new_password, comment=None):
             comment=comment,
         )
 
-    @celery_app.task
-    def modify_ldap_attributes(username, attributes):
-        """Modify the ldap attributes of a username.
-
-        Validation is applied for e.g. the 'mail' and 'loginShell' fields, but
-        this operation is not guaranteed to be safe.
-
-        :param attributes: dictionary of attribute names and values
-        """
-        real_modify_ldap_attributes(
-            username=username,
-            attributes=attributes,
-            keytab=credentials.kerberos_keytab,
-            principal=credentials.kerberos_principal,
-        )
-
     @celery_app.task
     def status():
         """A testing route."""
@@ -364,7 +347,6 @@ def status():
         approve_request=approve_request,
         reject_request=reject_request,
         change_password=change_password,
-        modify_ldap_attributes=modify_ldap_attributes,
         status=status,
     )
 
@@ -376,7 +358,6 @@ def status():
     'approve_request',
     'reject_request',
     'change_password',
-    'modify_ldap_attributes',
     'status',
 ])
 

ocflib/infra/ldap.py

@@ -1,12 +1,12 @@
 import string
+import subprocess
 from base64 import b64encode
 from contextlib import contextmanager
 from datetime import datetime
 from itertools import chain
 from textwrap import dedent
 
 import ldap3
-import pexpect
 
 from ocflib.misc.mail import send_problem_report
 
@@ -89,90 +89,107 @@ def format_value(value):
     return lines
 
 
-def _write_ldif(lines, dn, keytab, admin_principal):
+def _write_ldif(lines, dn, keytab=None, admin_principal=None):
     """Issue an update to LDAP via ldapmodify in the form of lines of an LDIF
-    file.
+    file. This could be a new addition to LDAP, a modification of an existing
+    item, or even a deletion depending on the changetype attribute given as
+    part of the sequence of lines.
 
     :param lines: ldif file as a sequence of lines
-    """
 
-    cmd = 'kinit -t {keytab} {principal} ldapmodify'.format(
-        keytab=keytab,
-        principal=admin_principal,
-    )
+    A ldif file looks something like this:
 
-    child = pexpect.spawn(cmd, timeout=10)
-    child.expect('SASL data security layer installed.')
-
-    for line in lines:
-        child.sendline(line)
-
-    child.sendeof()
-    child.expect('entry "{}"'.format(dn))
-    child.expect(pexpect.EOF)
-
-    output_after_adding = child.before.decode('utf8').strip()
-
-    if 'Already exists (68)' in output_after_adding:
-        raise ValueError('Tried to create duplicate entry.')
-    elif 'No such object (32)' in output_after_adding:
-        raise ValueError('Tried to modify nonexistent entry.')
-
-    if output_after_adding != '':
-        send_problem_report(
-            dedent(
-                '''\
-                Unknown problem occured when trying to write to LDAP; the code
-                should be updated to handle this case.
-
-                dn: {dn}
-                keytab: {keytab}
-                principal: {principal}
-
-                Unexpected output:
-                {output_after_adding}
-
-                Lines passed to ldapadd:
-                {lines}
-                '''
-            ).format(
-                dn=dn,
-                keytab=keytab,
-                principal=admin_principal,
-                output_after_adding=output_after_adding,
-                lines='\n'.join('    ' + line for line in lines)
-            )
+        dn: uid=jvperrin,ou=People,dc=OCF,dc=Berkeley,dc=EDU
+        changetype: modify
+        replace: loginShell
+        loginShell: /bin/zsh
+
+    It specifies the record or records to change, the type of change, and the
+    changes to make. To handle special characters (e.g. anything unprintable)
+    we base64-encode the dn and the values we set to get something more like
+    this (note the two colons instead of one to designate base64 data):
+
+        dn:: dWlkPWp2cGVycmluLG91PVBlb3BsZSxkYz1PQ0YsZGM9QmVya2VsZXksZGM9RURV
+        changetype: modify
+        replace: loginShell
+        loginShell:: L2Jpbi96c2g=
+    """
+
+    # Authenticate if these options are given. Otherwise, assume that
+    # authentication has already been done and that a valid kerberos ticket
+    # for the current user already exists
+    if keytab and admin_principal:
+        command = ('/usr/bin/kinit', '-t', keytab, admin_principal, '/usr/bin/ldapmodify', '-Q')
+    else:
+        command = ('/usr/bin/ldapmodify', '-Q')
+
+    try:
+        subprocess.check_output(
+            command,
+            input='\n'.join(lines),
+            universal_newlines=True,
+            timeout=10,
         )
-        raise ValueError('Unknown LDAP failure was encountered.')
+    except subprocess.CalledProcessError as e:
+        if e.returncode == 32:
+            raise ValueError('Tried to modify nonexistent entry.')
+        elif e.returncode == 68:
+            raise ValueError('Tried to create duplicate entry.')
+        else:
+            send_problem_report(
+                dedent(
+                    '''\
+                    Unknown problem occured when trying to write to LDAP; the
+                    code should be updated to handle this case.
+
+                    dn: {dn}
+                    keytab: {keytab}
+                    principal: {principal}
+
+                    Error code: {returncode}
+
+                    Unexpected output:
+                    {output}
+
+                    Lines passed to ldapmodify:
+                    {lines}
+                    '''
+                ).format(
+                    dn=dn,
+                    keytab=keytab,
+                    principal=admin_principal,
+                    returncode=e.returncode,
+                    output=e.output,
+                    lines='\n'.join('    ' + line for line in lines)
+                )
+            )
+            raise ValueError('Unknown LDAP failure was encountered.')
 
 
-def create_ldap_entry_with_keytab(
+def create_ldap_entry(
     dn,
     attributes,
-    keytab,
-    admin_principal,
+    **kwargs  # TODO: Add a trailing comma here in Python 3.6+
 ):
     """Creates an LDAP entry by shelling out to ldapadd.
 
     :param dn: distinguished name of the new entry
     :param attributes: dict mapping attribute name to list of values
-    :param keytab: path to the admin keytab
-    :param admin_principal: admin principal to use with the keytab
+    :param **kwargs: any additional keyword arguments to pass on to _write_ldif
     """
     lines = chain(
         _format_attr('dn', [dn]),
         ('changetype: add',),
-        *(_format_attr(key, values) for key, values in attributes.items())
+        *(_format_attr(key, values) for key, values in sorted(attributes.items()))
     )
 
-    _write_ldif(lines, dn, keytab, admin_principal)
+    _write_ldif(lines, dn, **kwargs)
 
 
-def modify_ldap_entry_with_keytab(
+def modify_ldap_entry(
     dn,
     attributes,
-    keytab,
-    admin_principal,
+    **kwargs  # TODO: Add a trailing comma here in Python 3.6+
 ):
     """Modifies the attributes of an existing LDAP entry by shelling out to
     ldapmodify.
@@ -182,8 +199,7 @@ def modify_ldap_entry_with_keytab(
 
     :param dn: distinguished name of the entry to modify
     :param attributes: dict mapping attribute name to list of values
-    :param keytab: path to the admin keytab
-    :param admin_principal: admin principal to use with the keytab
+    :param **kwargs: any additional keyword arguments to pass on to _write_ldif
     """
     lines = chain(
         _format_attr('dn', [dn]),
@@ -193,11 +209,11 @@ def modify_ldap_entry_with_keytab(
                 ('replace: {}'.format(key),),
                 _format_attr(key, values),
                 ('-',),
-            ) for key, values in attributes.items()
+            ) for key, values in sorted(attributes.items())
         )
     )
 
-    _write_ldif(lines, dn, keytab, admin_principal)
+    _write_ldif(lines, dn, **kwargs)
 
 
 def format_timestamp(timestamp):

tests-manual/infra/create-ldap-keytab

@@ -2,13 +2,13 @@
 from datetime import datetime
 from datetime import timezone
 
-from ocflib.infra.ldap import create_ldap_entry_with_keytab
+from ocflib.infra.ldap import create_ldap_entry
 
 
 # Run with:
 #   sudo /path/to/ocflib/venv/bin/python3 create-ldap-keytab
 if __name__ == '__main__':
-    create_ldap_entry_with_keytab(
+    create_ldap_entry(
         'uid=ggroup2,ou=People,dc=OCF,dc=Berkeley,dc=EDU',
         {
             'objectClass': ['ocfAccount', 'account', 'posixAccount'],
@@ -20,6 +20,6 @@ if __name__ == '__main__':
             'gidNumber': 1000,
             'creationTime': datetime.now(timezone.utc).astimezone(),
         },
-        '/etc/ocf-create/create.keytab',
-        'create/admin',
+        keytab='/etc/ocf-create/create.keytab',
+        admin_principal='create/admin',
     )

tests/account/creation_test.py

@@ -537,7 +537,7 @@ def test_create(
         with mock.patch('ocflib.account.creation.create_kerberos_principal_with_keytab') as kerberos, \
                 mock.patch('ocflib.account.creation.get_kerberos_principal_with_keytab',
                            return_value=None) as kerberos_get, \
-                mock.patch('ocflib.account.creation.create_ldap_entry_with_keytab') as ldap, \
+                mock.patch('ocflib.account.creation.create_ldap_entry') as ldap, \
                 mock.patch('ocflib.account.creation.create_home_dir') as home_dir, \
                 mock.patch('ocflib.account.creation.ensure_web_dir') as web_dir, \
                 mock.patch('ocflib.account.creation.send_created_mail') as send_created_mail, \
@@ -582,8 +582,8 @@ def test_create(
                     'userPassword': '{SASL}someuser@OCF.BERKELEY.EDU',
                     'creationTime': datetime.now(timezone.utc).astimezone(),
                 }, **expected),
-                fake_credentials.kerberos_keytab,
-                fake_credentials.kerberos_principal,
+                keytab=fake_credentials.kerberos_keytab,
+                admin_principal=fake_credentials.kerberos_principal,
             )
             call.assert_called_once_with(('sudo', 'nscd', '-i', 'passwd'))
             home_dir.assert_called_once_with(fake_new_account_request.user_name)