Feature #1741
X.509 authn rewrite
Status: | Closed | Start date: | 01/27/2013 | |
---|---|---|---|---|
Priority: | Normal | Due 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
feature #1741: escape spaces instead of deleting them in x509 auth
feature #1741: test every dn in x509 authentication
This patch was provided by Boris Parak <256254@mail.muni.cz> in
http://dev.opennebula.org/issues/1741
Minor cosmetic changes
feature #1741: escape dn in X509CloudAuth.rb
feature #1741: add method each_with_xpath to object pools
feature #1741: filter possible user matches using xpath
History
#1 Updated by Boris Parak over 8 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 8 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 8 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 8 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 8 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 8 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 8 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 8 years ago
- Assignee changed from Daniel Molina to Javi Fontan
#9 Updated by Javi Fontan over 8 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 8 years ago
- Status changed from New to Closed
- Resolution set to fixed