[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Pulp-list] Cleaning up user authentication

+1. Great suggestions and yes, I am definitely planning to re-structure and improve this part of the code when porting users and permissions to v2 with the help of jconnor, since he had worked on the authorization in v1 and he was not too happy with the way it is structured either :-) .

On 06/20/2012 06:55 PM, Nick Coghlan wrote:
I'm currently looking into patching Pulp to enable Kerberos logins to
the REST API, and noticed that the current authentication handling
mechanism is a little less than elegant (the comments in the code don't
put it quite so politely).

I figured I would share some thoughts on one possible way to clean it up.

First, some design goals:

1. Don't check authentication methods that aren't configured (i.e. if
the oauth fields aren't set, don't even check for oauth authentication)
2. Make it easy to add new authentication methods (like web server
3. Ensure each authenticator can customise the auth failure message when

The approach that comes to mind is to replace the current if/elif chain
in pulp.server.webservices.controllers.decorators.auth_required with:

         user = get_authenticated_user()
     except AuthenticationFailed as ex: # AuthenticationFailed, ex to
support 2.4
         return self.unauthorised(str(ex))

get_authenticated_user() would just be a search loop that did the following:

     def get_authenticated_user():
         for authenticate_user in <registered authentication functions>:
             user = authenticate_user()
             if user is not None:
                 return user
         raise AuthenticationFailed('Authentication Required')

And an initial list of authentication functions:

     def password_authentication():
         username, password = http.username_password()
         if username is None:
             return None
         user = check_username_password(username, password)
         if user is None:
             raise AuthenticationFailed('Invalid username or password')
         return user

     def user_cert_authentication(cert_checker):
         cert_pem = http.ssl_client_cert()
         if cert_pem is None:
             return None
         return check_user_cert(cert_pem)

     def consumer_cert_authentication(cert_checker):
         cert_pem = http.ssl_client_cert()
         if cert_pem is None:
             return None
         return check_consumer_cert(cert_pem)

     # Only install this one if oauth is configured
     def oauth_authentication():
         username = http.request_info('HTTP_PULP_USER')
         auth = http.http_authorization()
         cert_pem = http.ssl_client_cert()
         if username is None or auth is None:
             if cert_pem is not None:
                 raise AuthenticationFailure('Invalid SSL Certificate')
             return None
         meth = http.request_info('REQUEST_METHOD')
         url = http.request_url()
         query = http.request_info('QUERY_STRING')
         user = check_oauth(username, meth, url, auth, query)
         if user is None:
             raise AuthenticationFailure('Invalid OAuth Credentials')
         return user

With such a setup, it would also be possible to change the way LDAP is
handled to install a different password authenticator when it is
enabled, so the check for whether or not to fallback to an LDAP lookup
doesn't need to be repeated on each login attempt.

I haven't gone into detail on how the list of authentication functions
would get populated, since I don't know enough about the startup and
configuration sequence for the server to sensibly comment on that aspect.

Anyway, hopefully these thoughts might prove useful if anyone decides to
bite the bullet and actually clean up the authentication handling code :)


[1] https://bugzilla.redhat.com/show_bug.cgi?id=831937

Sayli Karmarkar

Software Engineer
Systems Management and Cloud Enablement

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]