Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-8537

LoginWorker HashCrypt the type of hash for one-way encryption

    Details

      Description

      PBKDF2 (Password-Based Key Derivation Function 2) is part of RSA Laboratories' Public-Key Cryptography Standards (PKCS) series, specifically PKCS #5 v2.0, also published as Internet Engineering Task Force's RFC 2898. It replaces an earlier key derivation function, PBKDF1, which could only produce derived keys up to 160 bits long.Add this function to ofbiz ,this PBKDF2 has four types in Java:'PBKDF2WithHmacSHA1','PBKDF2WithHmacSHA256','PBKDF2WithHmacSHA384','PBKDF2WithHmacSHA512'

      1. HashCrypt_new.patch
        6 kB
        wangjunyuan
      2. HashCrypt.patch
        9 kB
        wangjunyuan

        Activity

        Hide
        wangjunyuan wangjunyuan added a comment - - edited

        There are three files in this patch. This file that HashCrypt.java has been modified,this modification is reflected in the increase in support for PBDKF2,PBKDF2's iteration option are added in the other file that security.properties,the third file that PasswordSecurityDemoData.xml has been modified, flexadmin's password is admin hashed by PBDKF2_SHA1

        Show
        wangjunyuan wangjunyuan added a comment - - edited There are three files in this patch. This file that HashCrypt.java has been modified,this modification is reflected in the increase in support for PBDKF2,PBKDF2's iteration option are added in the other file that security.properties,the third file that PasswordSecurityDemoData.xml has been modified, flexadmin's password is admin hashed by PBDKF2_SHA1
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Thank you Junyuan for this fuction! (谢谢王军元!)

        Well done!

        It would be better if the following errors be corrected:
        1. I have to remove the first 2 lines of the patch to apply it in my local environment as my project is not named as "trunk"

        2. the getIterations() should be removed, simply using this line would be ok:
        private static final int PBKDF2_Iterations = UtilProperties.getPropertyAsInteger("security.properties", "password.encrypt.pbkdf2.iterations", 1000);

        3. change PBKDF2_SHA1 and other variables from public to private.

        4. remove TODOs.

        5. change flexadmin's password from 'admin' to our brand 'ofbiz'

        Please DO submit a new patch tomorrow. Thanks again!

        Show
        shi.jinghai Shi Jinghai added a comment - Thank you Junyuan for this fuction! (谢谢王军元!) Well done! It would be better if the following errors be corrected: 1. I have to remove the first 2 lines of the patch to apply it in my local environment as my project is not named as "trunk" 2. the getIterations() should be removed, simply using this line would be ok: private static final int PBKDF2_Iterations = UtilProperties.getPropertyAsInteger("security.properties", "password.encrypt.pbkdf2.iterations", 1000); 3. change PBKDF2_SHA1 and other variables from public to private. 4. remove TODOs. 5. change flexadmin's password from 'admin' to our brand 'ofbiz' Please DO submit a new patch tomorrow. Thanks again!
        Hide
        wangjunyuan wangjunyuan added a comment -

        Thank for Mr Jinghai's guidance with patience!
        I have solved the above problem and submitted a new patch.

        Show
        wangjunyuan wangjunyuan added a comment - Thank for Mr Jinghai's guidance with patience! I have solved the above problem and submitted a new patch.
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Thank you Junyuan!

        Your patch is in rev.1772589. I changed currentPassword from short-varchar to long-varchar as a PBKDF2 hashed password is longer than 60 charactors.

        Show
        shi.jinghai Shi Jinghai added a comment - Thank you Junyuan! Your patch is in rev.1772589. I changed currentPassword from short-varchar to long-varchar as a PBKDF2 hashed password is longer than 60 charactors.
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Hi Junyuan,

        I'm not sure whether your PBKDF2 hashed password format can be supported by OpenLDAP or ApacheDS. If not, the format should be changed so that the password string can be used in OpenLDAP directly.

        Kind Regards,

        Show
        shi.jinghai Shi Jinghai added a comment - Hi Junyuan, I'm not sure whether your PBKDF2 hashed password format can be supported by OpenLDAP or ApacheDS. If not, the format should be changed so that the password string can be used in OpenLDAP directly. Kind Regards,
        Hide
        mbrohl Michael Brohl added a comment -

        Hi wangjunyuan, Shi Jinghai,

        thanks for your contributions!

        I briefly reviewed the patch and think that we should change back the change of the demo data flexadmin password. The encryption configuration is still SHA (as it should be for backwards compatibility) and the demo data should be consistent with the configuration.

        If you want to provide an example for PBKDF2 I'd suggest to put it in the documentation or as a comment in the demo data.

        Thanky,
        Michael

        Show
        mbrohl Michael Brohl added a comment - Hi wangjunyuan , Shi Jinghai , thanks for your contributions! I briefly reviewed the patch and think that we should change back the change of the demo data flexadmin password. The encryption configuration is still SHA (as it should be for backwards compatibility) and the demo data should be consistent with the configuration. If you want to provide an example for PBKDF2 I'd suggest to put it in the documentation or as a comment in the demo data. Thanky, Michael
        Hide
        pfm.smits Pierre Smits added a comment - - edited

        Hi wangjunyuan, Shi Jinghai,

        I wonder who said that OFBiz trunk could not be volatile, and not be breaking with the past...

        I suggest not to change back, but rather remove the flexadmin references everywhere. Preferably in a new JIRA issue. We're talking about demo data....

        Show
        pfm.smits Pierre Smits added a comment - - edited Hi wangjunyuan , Shi Jinghai , I wonder who said that OFBiz trunk could not be volatile, and not be breaking with the past... I suggest not to change back, but rather remove the flexadmin references everywhere. Preferably in a new JIRA issue. We're talking about demo data....
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Thanks Michael for reviewing and Pierre for the suggestion on password format (see https://github.com/hamano/openldap-pbkdf2)!

        I'll change flexadmin's password back to SHA as currently the password format of PBKDF2 is not complied with RFC standard.

        Show
        shi.jinghai Shi Jinghai added a comment - Thanks Michael for reviewing and Pierre for the suggestion on password format (see https://github.com/hamano/openldap-pbkdf2 )! I'll change flexadmin's password back to SHA as currently the password format of PBKDF2 is not complied with RFC standard.
        Hide
        mbrohl Michael Brohl added a comment -

        Noone said that, I guess.

        For this issue, the entry should be changed back because it introduces an inconsistency in the commit.

        If you want to remove the flexadmin entries, this is another case and should be filed in another JIRA. It has nothing to do with this issue.

        Show
        mbrohl Michael Brohl added a comment - Noone said that, I guess. For this issue, the entry should be changed back because it introduces an inconsistency in the commit. If you want to remove the flexadmin entries, this is another case and should be filed in another JIRA. It has nothing to do with this issue.
        Hide
        mbrohl Michael Brohl added a comment -

        Thank you, Shi Jinghai!

        Show
        mbrohl Michael Brohl added a comment - Thank you, Shi Jinghai !
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Guys we crossed on wire, see my comments at http://markmail.org/message/n6mpoklnecsmmuwi

        I was not aware that "PBKDF2 is not compliant with RFC standard" as you said Jinghai. Where can I find this information?

        BTW note that it has already been superceded https://en.wikipedia.org/wiki/PBKDF2#Alternatives_to_PBKDF2

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Guys we crossed on wire, see my comments at http://markmail.org/message/n6mpoklnecsmmuwi I was not aware that "PBKDF2 is not compliant with RFC standard" as you said Jinghai. Where can I find this information? BTW note that it has already been superceded https://en.wikipedia.org/wiki/PBKDF2#Alternatives_to_PBKDF2
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Also this is interesting https://cryptosense.com/parameter-choice-for-pbkdf2/ That's why I suggest we use PBKDF2 rather than the old SHA-1

        Show
        jacques.le.roux Jacques Le Roux added a comment - Also this is interesting https://cryptosense.com/parameter-choice-for-pbkdf2/ That's why I suggest we use PBKDF2 rather than the old SHA-1
        Hide
        mbrohl Michael Brohl added a comment - - edited

        I ask myself if we should introduce PBKDF2 if it is not RFC compliant (which I have not checked) and has known weaknesses and/or better solutions are available?

        Show
        mbrohl Michael Brohl added a comment - - edited I ask myself if we should introduce PBKDF2 if it is not RFC compliant (which I have not checked) and has known weaknesses and/or better solutions are available?
        Hide
        pfm.smits Pierre Smits added a comment -

        That discussion is much broader than this issue tries to solve. It is also more fitting to be discussed in the dev ML as it should pobably be part of http://ofbiz.markmail.org/message/bjcwhitfd3elutgi ,

        Show
        pfm.smits Pierre Smits added a comment - That discussion is much broader than this issue tries to solve. It is also more fitting to be discussed in the dev ML as it should pobably be part of http://ofbiz.markmail.org/message/bjcwhitfd3elutgi ,
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        When it comes to security it's better to rely on last improvements than an old RFC from year 2000. There is also an improvement on PBKDF2, but at least PBKDF2 is better than SHA-1. I also agree with Pierre that we should better discuss this on the dev ML, notably by asking Grégory (ou security expert) about what he thinks about that. I'll do...

        Show
        jacques.le.roux Jacques Le Roux added a comment - When it comes to security it's better to rely on last improvements than an old RFC from year 2000. There is also an improvement on PBKDF2, but at least PBKDF2 is better than SHA-1. I also agree with Pierre that we should better discuss this on the dev ML, notably by asking Grégory (ou security expert) about what he thinks about that. I'll do...
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        I concur, thanks Junyuan, this is much appreciated

        Show
        jacques.le.roux Jacques Le Roux added a comment - I concur, thanks Junyuan, this is much appreciated
        Hide
        mbrohl Michael Brohl added a comment -

        Agree, thanks Jacques.

        Show
        mbrohl Michael Brohl added a comment - Agree, thanks Jacques.
        Show
        jacques.le.roux Jacques Le Roux added a comment - Done at http://markmail.org/message/vtwktynlecx7lczl
        Hide
        mbrohl Michael Brohl added a comment -

        Hi Shi Jinghai,

        can you please correct this as we currently have an inconsistency between configuration and data.

        Thanks and best regards,
        Michael

        Show
        mbrohl Michael Brohl added a comment - Hi Shi Jinghai , can you please correct this as we currently have an inconsistency between configuration and data. Thanks and best regards, Michael
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Hi Michael,

        I have reverted the framework/security/data/PasswordSecurityDemoData.xml.

        Kind Regards,

        Show
        shi.jinghai Shi Jinghai added a comment - Hi Michael, I have reverted the framework/security/data/PasswordSecurityDemoData.xml. Kind Regards,
        Hide
        mbrohl Michael Brohl added a comment -

        Thanks, Shi Jinghai!

        Show
        mbrohl Michael Brohl added a comment - Thanks, Shi Jinghai !
        Hide
        wangjunyuan wangjunyuan added a comment -

        there two modification in this new patch ,one is the fromat of password has been modified to '

        {PBKDF2}

        <Iteration>$<Adapted Base64 Salt>$<Adapted Base64 DK>',other one is that i changed back flexadmin's password and added four examples of PBKDF2.

        Show
        wangjunyuan wangjunyuan added a comment - there two modification in this new patch ,one is the fromat of password has been modified to ' {PBKDF2} <Iteration>$<Adapted Base64 Salt>$<Adapted Base64 DK>',other one is that i changed back flexadmin's password and added four examples of PBKDF2.
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Thank you Junyuan!

        Your patch is in rev.1773066. Please check if it's right. If yes, please close this issue for now.

        You can open a new issue if there's any further improvement such as auto upgrading password from SHA/MD5 to PBKDF2 after user logged in successfully.

        Kind Regards,

        Show
        shi.jinghai Shi Jinghai added a comment - Thank you Junyuan! Your patch is in rev.1773066. Please check if it's right. If yes, please close this issue for now. You can open a new issue if there's any further improvement such as auto upgrading password from SHA/MD5 to PBKDF2 after user logged in successfully. Kind Regards,
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Junyuan, you also changed the number of iterations from 1000 to 10 000

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Junyuan, you also changed the number of iterations from 1000 to 10 000

          People

          • Assignee:
            shi.jinghai Shi Jinghai
            Reporter:
            wangjunyuan wangjunyuan
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development