cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Clement Chen <clement.c...@citrix.com>
Subject RE: Make the authenticator responsible for encoding the password and add a SHA256 salted authenticator
Date Wed, 31 Oct 2012 17:47:56 GMT
I agree. UI should send passwords in clear text and it should rely on the underlying network
transport (e.g, SSL) for security. There is a bug for hashing user password with salt in the
old bugs.cloudstack.org (http://bugs.cloudstack.org/browse/CS-13902). Not sure whether it
has been migrated to Apache JIRA though.

I also strongly agree that we should make this the default authenticator.

Thanks.

-Clement 

-----Original Message-----
From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com] 
Sent: Wednesday, October 31, 2012 1:00 AM
To: CloudStack DeveloperList
Subject: RE: Make the authenticator responsible for encoding the password and add a SHA256
salted authenticator



> -----Original Message-----
> From: Chiradeep Vittal [mailto:Chiradeep.Vittal@citrix.com]
> Sent: Wednesday, October 31, 2012 12:09 AM
> To: CloudStack DeveloperList
> Subject: Re: Make the authenticator responsible for encoding the 
> password and add a SHA256 salted authenticator
> 
> No, I saw this:
> 
>  // Default password is MD5 hashed.  Set the following variable to 
> false to disable this.
> -var md5Hashed = true;
> -var md5HashedLogin = true;
> +var md5Hashed = false;
> +var md5HashedLogin = false;
> 
> 
> This led me to understand that the ui will send a plaintext password 
> to the backend.

Correct, the UI will send the passwords in clear text. That is the actual thing the user is
authenticating with. The hash is just a mechanism to store information in the database without
having to store the actual cleartext password. By sending the hash on the wire you basically
make the hash the users password and store it in the database as plaintext. 

> 
> It looks like the backend will encode it before comparing it with the 
> DB hence ensuring compatibility?

Yup, the backend will ask each configured authenticator to authenticate the user. The hash
based authentication functions will encode the user supplied password and compare it with
the stored hash.

> 
> Anyway, it is probably wise to give a heads-up BEFORE doing such a change.

This is up for debate, I've noticed that the best way to get constructive feedback is to implement
a change and have people review it. It's the trust we put in our committers that they take
this responsibility seriously and provide peer review on commits. In most cases it's the actual
committed code that clearly indicates what the committer intended to do, where an email to
the dev list could fail to properly convey the intentions or implications. Another factor
is the scope of the change, for example a complete refactor that touches a lot of different
pieces of the code could warrant a separate feature branch. Smaller changes (and I consider
my change small) should be committed to master directly and like David mentioned on the list
earlier more effort in unittests and integration test should give us the (automated) confidence
that commits don't brake stuff.

> Ideally we should have a bug id.

There wasn't really a bug for this, I just discussed that we wanted this feature in with our
security officer and build it. Do we want to have tickets / bug reports for features that
people are developing?

> Also, it looks like some unrelated stuff went into the same changeset 
> (enableAdminUser).

enableAdminUser is required as part of this change. For a new installation the admin user
is configured in the persistDefaultValues() function of the configuration server. However
at this time the userauthenticator is not yet configured, as they are part of the managementserver.
So I changed the initial step to leave the admin user disabled and implemented the enableAdminUser
to set the password using the configured authenticator and enable the user. This is only user
in new installations, existing installations will not overwrite the admin user in the saveUser
call.

Thanks for the feedback!


Cheers,

Hugo

> 
> 
> On 10/30/12 2:47 PM, "Hugo Trippaers" <HTrippaers@schubergphilis.com>
> wrote:
> 
> >It shouldn't break anything, i did test this with a 4.0 database and 
> >had no trouble at all.
> >
> >Did you see something going wrong Chiradeep?
> >
> >Cheers,
> >Hugo
> >
> >Sent from my iPhone
> >
> >On 30 okt. 2012, at 21:10, "Wido den Hollander" <wido@widodh.nl> wrote:
> >
> >>
> >>
> >> On 30-10-12 19:50, Chiradeep Vittal wrote:
> >>> This probably breaks upgrade from 4.0. I would revert this until 
> >>>we find a  solution that does not break upgrades.
> >>
> >> Does it? As long as you don't enable this component it won't do a thing?
> >>
> >> Wido
> >>
> >>>
> >>> On 10/30/12 5:16 AM, "Hugo Trippaers"
> >>> <HTrippaers@schubergphilis.com>
> >>> wrote:
> >>>
> >>>> Hey all,
> >>>>
> >>>> I just pushed some changes to the master branch. This is change 
> >>>>based on  some security requirements that we have for storing 
> >>>>passwords and hashes.
> >>>> The commit is here
> >>>>
> >>>>https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git
> >>>>;a
> >>>>=co
> >>>>mmi
> >>>> t;h=bd58ceccd8d08a2484384a7eef6ef3c681a1e188
> >>>>
> >>>> The main goal of this change was to add a new authenticator that 
> >>>>uses the
> >>>> SHA256 algorithm and uses a salt.  This is now implemented, but 
> >>>>to get it  working I needed to make a few changes to how 
> >>>>encryption was done.
> >>>>
> >>>> I've tested with new code with an existing database and verified 
> >>>>that  users can be created, can be updated (including passwords) 
> >>>>and that they  can login on the UI without any changes to the database.
> >>>>The default  authenticator is still set to the MD5Authenticator.
> >>>>
> >>>> For people that want to use the new authenticator, just change 
> >>>>the components.xml.in and add the following line '<adapter 
> >>>>name="SHA256SALT"
> >>>> class="com.cloud.server.auth.SHA256SaltedUserAuthenticator">' to

> >>>>UserAuthenticator. Note that this prevent any existing users for 
> >>>>logging  in as their passwords will be incorrect with the new 
> >>>>authenticator.
> >>>>
> >>>> Reference: http://crackstation.net/hashing-security.htm
> >>>>
> >>>> Cheers,
> >>>>
> >>>> Hugo
> >>>>
> >>>> Below the text of the commit for reference:
> >>>>
> >>>> The authenticators now have an encode function that cloudstack 
> >>>>will use  to encode the user supplied password before storing it 
> >>>>in the database.
> >>>> This makes it easier to add other authenticators with other 
> >>>>hashing algorithms. The requires a two step approach to creating 
> >>>>the admin account at first start as the authenticators are only 
> >>>>present in the management-server component locator.
> >>>>
> >>>> The SHA256 salted authenticator make use of this new system and 
> >>>>adds a  hashing algorithm based on SHA256 with a salt. This type 
> >>>>of hash is far  less susceptible to rainbow table attacks.
> >>>>
> >>>> To make use of these new features the users password will be sent 
> >>>> over the wire just as he typed it and it will be transformed into 
> >>>> a hash on the server and compared with the stored password. This 
> >>>> means that the hash will not go over the wire anymore.
> >>>>
> >>>> The default authenticator in components.xml is still set to md5 
> >>>> for backwards compatibility. For new installations the sha256 
> >>>> could be enabled.
> >>>


Mime
View raw message