Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Active directory login accepted with empty string as password using sample application #9

Closed
fbongartz opened this issue May 22, 2013 · 4 comments

Comments

@fbongartz
Copy link

While trying to implement active directory auth with pyramid_ldap for a pyramid project, I came across an issue which I can replicate using code from the sample app.

When adapting the code to authenticate using our AD domain controller, any existing username can successfully log in using the empty string as password.

To replicate:

  • Create a virtual environment and a new pyramid project:
    virtualenv2 --no-site-packages pyramid_ldap_test_env
    cd pyramid_ldap_test_env
    bin/easy_install pyramid
    bin/pcreate -s alchemy ldap_test
    cd ldap_test
    ../bin/python setup.py develop
    ../bin/easy_install pyramid_ldap
  • Include pyramid_ldap in pyramid_includes in development.ini
  • Edit the projects "init,py":
    import ldap
    from pyramid.config import Configurator
    from pyramid.security import (Allow, Authenticated, DENY_ALL)
    from pyramid.authentication import AuthTktAuthenticationPolicy
    from pyramid.authorization import ACLAuthorizationPolicy
    from pyramid_ldap import groupfinder

    class RootFactory(object):
        __acl__ = [(Allow, Authenticated, 'view'), DENY_ALL]

        def __init__(self, request):
            pass

    def main(global_config, **settings):
        """ This function returns a Pyramid WSGI application.
        """
        engine = engine_from_config(settings, 'sqlalchemy.')
        DBSession.configure(bind=engine)
        Base.metadata.bind = engine
        config = Configurator(settings=settings)

        # setup auth
        config = Configurator(settings=settings, root_factory=RootFactory)
        config.set_authentication_policy(
            AuthTktAuthenticationPolicy('seekr1t', callback=groupfinder)
            )
        config.set_authorization_policy(ACLAuthorizationPolicy())

        # ldap setup
        config.ldap_setup('ldap://dc.ge.lan', bind='CN=Firstname Lastname,CN=Users,DC=ge,DC=lan', passwd='thepassword')
        config.ldap_set_login_query(base_dn='CN=Users,DC=ge,DC=lan',
            filter_tmpl='(sAMAccountName=%(login)s)', scope=ldap.SCOPE_ONELEVEL)
        config.ldap_set_groups_query(base_dn='CN=Users,DC=ge,DC=lan',
            filter_tmpl='(&(objectCategory=group)(member=%(userdn)s))',
            scope=ldap.SCOPE_SUBTREE, cache_period=600)

        config.add_route('root', '/')
        config.add_route('login', '/login')
        config.add_route('logout', '/logout')
        config.scan()
        return config.make_wsgi_app()
  • views.py:
    from pyramid.response import Response
    from pyramid.view import view_config, forbidden_view_config
    from pyramid.httpexceptions import HTTPFound
    from pyramid.security import (remember, forget)

    from pyramid_ldap import (get_ldap_connector, groupfinder)

    @view_config(route_name='login',
                 renderer='templates/login.pt')
    @forbidden_view_config(renderer='templates/login.pt')
    def login(request):
        url = request.current_route_url()
        login = ''
        password = ''
        error = ''

        if 'form.submitted' in request.POST:
            login = request.POST['login']
            password = request.POST['password']
            connector = get_ldap_connector(request)
            data = connector.authenticate(login, password)
            if data is not None:
                dn = data[0]
                headers = remember(request, dn)
                return HTTPFound('/', headers=headers)
            else:
                error = 'Invalid credentials'

        return dict(login_url=url, login=login, password=password, error=error)

    @view_config(route_name='root', permission='view')
    def logged_in(request):
        return Response('OK')

    @view_config(route_name='logout')
    def logout(request):
        headers = forget(request)
        return Response('Logged out', headers=headers)
  • Login.pt
<html>
    <head>
    <title>Login</title>
    </head>
    <body>
    <h1>Log In</h1>
      <p tal:replace="error"/>
      <form action="${login_url}" method="POST">
        Login: <input type="text" name="login" value="${login}"/>
        <br/>
        Password: <input type="password" name="password" value="${password}"/>
        <br/>
        <input type="hidden" name="form.submitted"/>
        <input type="submit" name="submit" value="Log In"/>
      </form>
    </body>
    </html>
  • Start the application using ..bin/pserve development.ini.
  • Try logging in as active directory user:
    • login with the wrong password doesn't work.
    • login with the correct password works.
    • login with the empty string as password works.

Can anyone confirm this or am I missing something?

@travisfischer
Copy link
Contributor

This is an issue. It is a common issue among LDAP client implementations. It is documented here: http://tools.ietf.org/html/rfc4513#section-6.3.1 in RFC-4513.

The quick summary is that if the LDAP server is configured to allow what is called "Unauthenticated Authentication" a bind request with an empty password will be authenticated.

The best practice on the server side is to not allow this: http://www.ldapguru.info/ldap/authentication-best-practices.html

However, on the client side, in order to use LDAP for the purposes of authentication, only bind requests with greater than zero length passwords should be allowed as to only ever allow "authenticated" bind requests.

@ericrasmussen or I will be submitting a pull request shortly.

mcdonc added a commit that referenced this issue Jul 17, 2013
Added check for zero length password w/ tests and docs (Fixes Issue #9)
@dataflake
Copy link

Allowing or disallowing bind attempts with a DN and an empty password is an LDAP server setting and should remain the LDAP server administrator's responsibility. It's not up to a middleware package such as pyramid_ldap to force a policy here. The merge should never have been done and the issue rejected. This is not a pyramid_ldap issue, just a LDAP server configuration.

@mcdonc
Copy link
Member

mcdonc commented Aug 17, 2013

Closing due to already-merged.

@mcdonc mcdonc closed this as completed Aug 17, 2013
@travisfischer
Copy link
Contributor

I want to clarify information for the record in case this issue comes under review in the future. @dataflake: Please read this section of the LDAP rfc: http://tools.ietf.org/html/rfc4513#section-6.3.1 . While you are correct that this issue can be resolved on the LDAP server side by configuring LDAP to not allow unauthenticated binds, the RFC clearly calls out that client code can and should be aware of unauthenticated binds and should not use them for user authentication. It specifically describes implementing this on the client side by not allowing users to "authenticate" with a zero-length password. There are legitimate cases for allowing unauthenticated binds while still using LDAP to do proper authentication in which case it does become the client's job to differentiate between a bind and an "authentication." Thankfully, pyramid_ldap has a specific function for "authentication" which is where this patch was applied. To disallow empty passwords in a "bind" operation on the client side would be a mistake but that is not what was done in this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants