Feature #1741

X.509 authn rewrite

Added by Boris Parak over 6 years ago. Updated over 6 years ago.

Status:ClosedStart date:01/27/2013
Priority:NormalDue date:
Assignee:Javi Fontan% Done:

0%

Category:-
Target version:Release 4.0
Resolution:fixed Pull request:

Description

  • X.509 shouldn't be implemented using reverse lookup password -> username; passwords are not unique. Couldn't DNs be used as usernames? Or much better, couldn't introduce the concept of multiple user identities tied to a single username (similar current "DN1|DN2|DN3" approach, but separated from the password field)?
  • Spaces in DNs shouldn't be removed, but rather replaced with one of the reserved characters, e.g. '+'.

Associated revisions

Revision efbaff9b
Added by Javi Fontan over 6 years ago

feature #1741: escape spaces instead of deleting them in x509 auth

Revision 7b9c6561
Added by Javi Fontan over 6 years ago

feature #1741: test every dn in x509 authentication

This patch was provided by Boris Parak <> in
http://dev.opennebula.org/issues/1741

Minor cosmetic changes

Revision 02cd738d
Added by Javi Fontan over 6 years ago

feature #1741: escape dn in X509CloudAuth.rb

Revision 60e0a6f2
Added by Javi Fontan over 6 years ago

feature #1741: add method each_with_xpath to object pools

Revision b5feaadd
Added by Javi Fontan over 6 years ago

feature #1741: filter possible user matches using xpath

History

#1 Updated by Boris Parak over 6 years ago

Requested on behalf of the EGI Federated Cloud Task Force -- http://www.egi.eu/infrastructure/cloud/

#2 Updated by Ruben S. Montero over 6 years ago

Hi

We've a couple of questions about the first item of this issue. In EGI's use case, Do we need to assign a given DN to just one user account? So do we need OpenNebula check that DN to user assignment is unique?.

About the second bullet we're thinking of using a simple encoding (URL).

BTW, there are other features for x509 underway like CRL support. And for federated authentication systems there is a SAML plugin, check #1731

Cheers

Ruben

#3 Updated by Boris Parak over 6 years ago

Hi Ruben,

In EGI's use case, Do we need to assign a given DN to just one user account?
So do we need OpenNebula check that DN to user assignment is unique?

Since there is no consistent way to choose a specific user account to log into when using x509 authn, it would be useful to keep DNs unique and assigned to a single user account. Current approach works nicely for OCA shell utils since they generate a token and the token includes a username; but not with X509CloudAuth (e.g., Sunstone) and its 'username = get_username(cert.subject.to_s.delete("\s"))'.

So, to sum things up: X.509 authn in OpenNebula doesn't work consistently across all its APIs, in some of them you can pick a user account to log into, in some of them you cannot (and multiple users with the same DN will result in a First-come-First-served login). The 4.0 release might give you the opportunity to review/re-design/re-implement the authn subsystem and make it more consistent (supported globally across all APIs and GUIs). This would be definitely appreciated and would help with EGI's efforts.

About the second bullet we're thinking of using a simple encoding (URL).

That would be great but any fully reversible process would do. At the moment we have to keep a copy of user's DN in his properties to be able to identify him or her in a X.509-only environment.

Thank you.

Cheers, Boris

Ruben S. Montero wrote:

Hi

We've a couple of questions about the first item of this issue. In EGI's use case, Do we need to assign a given DN to just one user account? So do we need OpenNebula check that DN to user assignment is unique?.

About the second bullet we're thinking of using a simple encoding (URL).

BTW, there are other features for x509 underway like CRL support. And for federated authentication systems there is a SAML plugin, check #1731

Cheers

Ruben

#4 Updated by Boris Parak over 6 years ago

There is one very dangerous practice used in the current implementation of get_username in src/cloud/common/CloudAuth.rb:

xpath = "USER[contains(PASSWORD, \"#{password}\")]/NAME"

you should never use contains(...) when selecting a single username based on the content of its password field since contains() is checking for substring matches. This is a big problem for X.509 authN. DN of one user could theoretically contain a DN of another user as a substring or one user can have multiple DNs that differ only in appended CN attributes (and we've encountered such a user in production, it's quite common).

Here is a simple (also a very slow fix) for this issue -- https://gist.github.com/arax/4722445

It is one of the reasons forcing me to file this request. Is there a possibility this will change in the future?

Thank you.

#5 Updated by Ruben S. Montero over 6 years ago

  • Tracker changed from Request to Feature
  • Assignee set to Daniel Molina

Good catch!

We'll do our best to finish this before release 4.0 (Moved from request to feature). And thanks for the patch, this can be at least used to re-implement the method for X509Auth class.

Pasting the gist here for reference

# Gets the username associated with a password
# password:: _String_ the password
# [return] _Hash_ with the username
def get_username(password)
    xpath = "USER[PASSWORD=\"#{password}\"]/NAME" 
    username = retrieve_from_userpool(xpath)

    if username.nil?
        @lock.synchronize {
            @user_pool.each do |x509_user|
                x509_user["PASSWORD"].split('|').each do |x509_user_dn|
                    if x509_user_dn == password
                        username = x509_user["NAME"]
                        break
                    end
                end if x509_user["AUTH_DRIVER"] == "x509" 

                break unless username.nil?
            end
        }
    end

    username
end

Boris Parak wrote:

There is one very dangerous practice used in the current implementation of get_username in src/cloud/common/CloudAuth.rb:

xpath = "USER[contains(PASSWORD, \"#{password}\")]/NAME"

you should never use contains(...) when selecting a single username based on the content of its password field since contains() is checking for substring matches. This is a big problem for X.509 authN. DN of one user could theoretically contain a DN of another user as a substring or one user can have multiple DNs that differ only in appended CN attributes (and we've encountered such a user in production, it's quite common).

Here is a simple (also a very slow fix) for this issue -- https://gist.github.com/arax/4722445

It is one of the reasons forcing me to file this request. Is there a possibility this will change in the future?

Thank you.

#6 Updated by Boris Parak over 6 years ago

Here is an updated version of the fix (more ruby-esque)

# Gets the username associated with a password
# password:: _String_ the password
# [return] _Hash_ with the username
def get_username(password)
    xpath = "USER[PASSWORD=\"#{password}\"]/NAME" 
    username = retrieve_from_userpool(xpath)

    # No exact match, trying to match password with each
    # of the pipe-separated DNs stored in USER/PASSWORD
    if username.nil?
        @lock.synchronize {
            @user_pool.each { |user|
                return user["NAME"] if user["AUTH_DRIVER"] == "x509" && user["PASSWORD"].split('|').include?(password)
            }
        }
    end

    username
end

Is @lock.synchronize really necessary here? Nothing in this class is multi-threaded, @lock is an instance variable and it's not shared with any other class in a separate thread and multi-threading in Ruby is implemented within the Ruby interpreter (it won't create threads 1:1 on an OS level, except maybe on jRuby). Am I missing something?

Thank you.

Ruben S. Montero wrote:

Good catch!

We'll do our best to finish this before release 4.0 (Moved from request to feature). And thanks for the patch, this can be at least used to re-implement the method for X509Auth class.

Pasting the gist here for reference
[...]

Boris Parak wrote:

There is one very dangerous practice used in the current implementation of get_username in src/cloud/common/CloudAuth.rb:

xpath = "USER[contains(PASSWORD, \"#{password}\")]/NAME"

you should never use contains(...) when selecting a single username based on the content of its password field since contains() is checking for substring matches. This is a big problem for X.509 authN. DN of one user could theoretically contain a DN of another user as a substring or one user can have multiple DNs that differ only in appended CN attributes (and we've encountered such a user in production, it's quite common).

Here is a simple (also a very slow fix) for this issue -- https://gist.github.com/arax/4722445

It is one of the reasons forcing me to file this request. Is there a possibility this will change in the future?

Thank you.

#7 Updated by Ruben S. Montero over 6 years ago

Thanks. Yes an instance of this class is shared by Sinatra requests.
Boris Parak wrote:

Here is an updated version of the fix (more ruby-esque)

[...]

Is @lock.synchronize really necessary here? Nothing in this class is multi-threaded, @lock is an instance variable and it's not shared with any other class in a separate thread and multi-threading in Ruby is implemented within the Ruby interpreter (it won't create threads 1:1 on an OS level, except maybe on jRuby). Am I missing something?

Thank you.

#8 Updated by Ruben S. Montero over 6 years ago

  • Assignee changed from Daniel Molina to Javi Fontan

#9 Updated by Javi Fontan over 6 years ago

Now the DNs are escaped using \<hex value> for space characters. I am not using + as this is a reserved character with meaning.

I've also changed a bit the get_username method. Now it searches for the dn substring using xpath (the each_with_xpath method) and only tries the users that match. Xpath search is done with rexml or nokogiri, this will make it much faster if nokogiri is installed.

#10 Updated by Ruben S. Montero over 6 years ago

  • Status changed from New to Closed
  • Resolution set to fixed

Also available in: Atom PDF