> -----Original Message----- > From: Chiradeep Vittal [mailto:chiradeep.vit...@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" <htrippa...@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" <w...@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" > >>> <htrippa...@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. > >>>
