Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java framework/security/config/security.properties

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java framework/security/config/security.properties

Michael Brohl-3
Hi Jacques,

why don't we just discuss such changes before they got implemented?

I don't think it is a valid solution to change the key in a versioned
class file during compile time, that's a really strange proposal.

The configuration through the properties was fine, every responsible
person should take care of changing this when he is setting up a
production environment. And using a database driven SystemProperty entry
completely avoids having a productive key somewhere in a file.

I don't see how this is really a security problem.

This also breaks configuration solutions which deal with property file
manipulations like proposed by Gil and me.

I propose to revert this.

Thanks,

Michael


Am 05.11.17 um 12:28 schrieb [hidden email]:

> Author: jleroux
> Date: Sun Nov  5 11:28:42 2017
> New Revision: 1814349
>
> URL: http://svn.apache.org/viewvc?rev=1814349&view=rev
> Log:
> Fixed: Secure the login.secret_key_string
> (OFBIZ-9966)
>
> When OFBIZ-4983 was implemented I missed that we put the login.secret_key_string
>   as a property in security properties. This should not have been because it
> eases attackers work.
>
> The recommended way is to have it as a private static final String that can be
> changed just when compiling using sed and uuidgen. So then the key is temporay
> and final and it gets quite harder for a possible attacker to use this mean.
>
>
> Modified:
>      ofbiz/ofbiz-framework/trunk/applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>      ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java?rev=1814349&r1=1814348&r2=1814349&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java Sun Nov  5 11:28:42 2017
> @@ -69,7 +69,12 @@ public class LoginEvents {
>       public static final String module = LoginEvents.class.getName();
>       public static final String resource = "SecurityextUiLabels";
>       public static final String usernameCookieName = "OFBiz.Username";
> -    private static final String keyValue = UtilProperties.getPropertyValue(LoginWorker.securityProperties, "login.secret_key_string");
> +    // OOTB the loginSecretKeyString is not properly initialised and can not be OOTB.
> +    // The best way to create the loginSecretKeyString is to use a temporary way to load in a static final key when compiling.
> +    // This is simple and most secure. One of the proposed way is to use sed and uuidgen to modify the loginSecretKeyString value
> +    // The magic words here are TEMPORARY and FINAL!
> +    private static final String loginSecretKeyString = "loginSecretKeyString";
> +
>       /**
>        * Save USERNAME and PASSWORD for use by auth pages even if we start in non-auth pages.
>        *
> @@ -253,7 +258,7 @@ public class LoginEvents {
>                   autoPassword = RandomStringUtils.randomAlphanumeric(EntityUtilProperties.getPropertyAsInteger("security", "password.length.min", 5).intValue());
>                   EntityCrypto entityCrypto = new EntityCrypto(delegator,null);
>                   try {
> -                    passwordToSend = entityCrypto.encrypt(keyValue, EncryptMethod.TRUE, autoPassword);
> +                    passwordToSend = entityCrypto.encrypt(loginSecretKeyString, EncryptMethod.TRUE, autoPassword);
>                   } catch (GeneralException e) {
>                       Debug.logWarning(e, "Problem in encryption", module);
>                   }
>
> Modified: ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties?rev=1814349&r1=1814348&r2=1814349&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties (original)
> +++ ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties Sun Nov  5 11:28:42 2017
> @@ -126,7 +126,5 @@ protect-view.preprocessor=java.org.apach
>   #default.error.response.view=none:
>   default.error.response.view=view:viewBlocked
>  
> -# If false, then no externalLoginKey parameters will be added to cross-webapp urls
> +# -- If false, then no externalLoginKey parameters will be added to cross-webapp urls
>   security.login.externalLoginKey.enabled=true
> -#Security key used to encrypt and decrypt the autogenerated password in forgot password functionality.
> -login.secret_key_string=Secret Key
> \ No newline at end of file
>
>


smime.p7s (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/L oginEvents.java framework/security/config/security.properties

Jacques Le Roux
Administrator
OK before I revert that you might consider reading

https://stackoverflow.com/questions/13991100/where-do-you-store-your-secret-key-in-a-java-web-application

http://movingfast.io/articles/environment-variables-considered-harmful/

https://security.stackexchange.com/questions/49725/is-it-really-secure-to-store-api-keys-in-environment-variables

https://stackoverflow.com/questions/13991100/where-do-you-store-your-secret-key-in-a-java-web-application

But it's up to you of course, if you are still OK to use this way I'm totally OK to revert it

Jacques


Le 05/11/2017 à 15:10, Michael Brohl a écrit :

> Hi Jacques,
>
> why don't we just discuss such changes before they got implemented?
>
> I don't think it is a valid solution to change the key in a versioned class file during compile time, that's a really strange proposal.
>
> The configuration through the properties was fine, every responsible person should take care of changing this when he is setting up a production
> environment. And using a database driven SystemProperty entry completely avoids having a productive key somewhere in a file.
>
> I don't see how this is really a security problem.
>
> This also breaks configuration solutions which deal with property file manipulations like proposed by Gil and me.
>
> I propose to revert this.
>
> Thanks,
>
> Michael
>
>
> Am 05.11.17 um 12:28 schrieb [hidden email]:
>> Author: jleroux
>> Date: Sun Nov  5 11:28:42 2017
>> New Revision: 1814349
>>
>> URL: http://svn.apache.org/viewvc?rev=1814349&view=rev
>> Log:
>> Fixed: Secure the login.secret_key_string
>> (OFBIZ-9966)
>>
>> When OFBIZ-4983 was implemented I missed that we put the login.secret_key_string
>>   as a property in security properties. This should not have been because it
>> eases attackers work.
>>
>> The recommended way is to have it as a private static final String that can be
>> changed just when compiling using sed and uuidgen. So then the key is temporay
>> and final and it gets quite harder for a possible attacker to use this mean.
>>
>>
>> Modified:
>> ofbiz/ofbiz-framework/trunk/applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>> ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java?rev=1814349&r1=1814348&r2=1814349&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/LoginEvents.java Sun Nov  5 11:28:42 2017
>> @@ -69,7 +69,12 @@ public class LoginEvents {
>>       public static final String module = LoginEvents.class.getName();
>>       public static final String resource = "SecurityextUiLabels";
>>       public static final String usernameCookieName = "OFBiz.Username";
>> -    private static final String keyValue = UtilProperties.getPropertyValue(LoginWorker.securityProperties, "login.secret_key_string");
>> +    // OOTB the loginSecretKeyString is not properly initialised and can not be OOTB.
>> +    // The best way to create the loginSecretKeyString is to use a temporary way to load in a static final key when compiling.
>> +    // This is simple and most secure. One of the proposed way is to use sed and uuidgen to modify the loginSecretKeyString value
>> +    // The magic words here are TEMPORARY and FINAL!
>> +    private static final String loginSecretKeyString = "loginSecretKeyString";
>> +
>>       /**
>>        * Save USERNAME and PASSWORD for use by auth pages even if we start in non-auth pages.
>>        *
>> @@ -253,7 +258,7 @@ public class LoginEvents {
>>                   autoPassword = RandomStringUtils.randomAlphanumeric(EntityUtilProperties.getPropertyAsInteger("security", "password.length.min",
>> 5).intValue());
>>                   EntityCrypto entityCrypto = new EntityCrypto(delegator,null);
>>                   try {
>> -                    passwordToSend = entityCrypto.encrypt(keyValue, EncryptMethod.TRUE, autoPassword);
>> +                    passwordToSend = entityCrypto.encrypt(loginSecretKeyString, EncryptMethod.TRUE, autoPassword);
>>                   } catch (GeneralException e) {
>>                       Debug.logWarning(e, "Problem in encryption", module);
>>                   }
>>
>> Modified: ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties?rev=1814349&r1=1814348&r2=1814349&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties (original)
>> +++ ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties Sun Nov  5 11:28:42 2017
>> @@ -126,7 +126,5 @@ protect-view.preprocessor=java.org.apach
>>   #default.error.response.view=none:
>>   default.error.response.view=view:viewBlocked
>>   -# If false, then no externalLoginKey parameters will be added to cross-webapp urls
>> +# -- If false, then no externalLoginKey parameters will be added to cross-webapp urls
>>   security.login.externalLoginKey.enabled=true
>> -#Security key used to encrypt and decrypt the autogenerated password in forgot password functionality.
>> -login.secret_key_string=Secret Key
>> \ No newline at end of file
>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/L oginEvents.java framework/security/config/security.properties

taher
There are multiple ways to implement this solution, by far my least
favorite is hardcoding and then recompiling! Also hardening security and
credentials is usually not done in the generic code but in the specific
deployment environment which differs from person to person or company to
company.

If you are not careful enough in securing your file system then you might
as well consider your security a disaster. Many many frameworks store
credentials in text in the file system e.g. drupal, suitecrm, rails,
wordpress, django, joomla ... and the list goes on. Oh .. and where does
OFBiz store JDBC passwords? Wouldn't that be in entityengine.xml? Oh and
what about the socket for AdminServer? What about anything you want to keep
secret? Do you hard code everything?

Security is a strategy. It is a combination of approaches for encryption,
networking, file systems, databases, templating and dozens of other factors
and each person implements these factors differently.

My recommendation is to revert and get community consensus on adopting any
security strategy (or any strategy for that matter).

On Nov 5, 2017 8:49 PM, "Jacques Le Roux" <[hidden email]>
wrote:

> OK before I revert that you might consider reading
>
> https://stackoverflow.com/questions/13991100/where-do-you-
> store-your-secret-key-in-a-java-web-application
>
> http://movingfast.io/articles/environment-variables-considered-harmful/
>
> https://security.stackexchange.com/questions/49725/is-it-
> really-secure-to-store-api-keys-in-environment-variables
>
> https://stackoverflow.com/questions/13991100/where-do-you-
> store-your-secret-key-in-a-java-web-application
>
> But it's up to you of course, if you are still OK to use this way I'm
> totally OK to revert it
>
> Jacques
>
>
> Le 05/11/2017 à 15:10, Michael Brohl a écrit :
>
>> Hi Jacques,
>>
>> why don't we just discuss such changes before they got implemented?
>>
>> I don't think it is a valid solution to change the key in a versioned
>> class file during compile time, that's a really strange proposal.
>>
>> The configuration through the properties was fine, every responsible
>> person should take care of changing this when he is setting up a production
>> environment. And using a database driven SystemProperty entry completely
>> avoids having a productive key somewhere in a file.
>>
>> I don't see how this is really a security problem.
>>
>> This also breaks configuration solutions which deal with property file
>> manipulations like proposed by Gil and me.
>>
>> I propose to revert this.
>>
>> Thanks,
>>
>> Michael
>>
>>
>> Am 05.11.17 um 12:28 schrieb [hidden email]:
>>
>>> Author: jleroux
>>> Date: Sun Nov  5 11:28:42 2017
>>> New Revision: 1814349
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1814349&view=rev
>>> Log:
>>> Fixed: Secure the login.secret_key_string
>>> (OFBIZ-9966)
>>>
>>> When OFBIZ-4983 was implemented I missed that we put the
>>> login.secret_key_string
>>>   as a property in security properties. This should not have been
>>> because it
>>> eases attackers work.
>>>
>>> The recommended way is to have it as a private static final String that
>>> can be
>>> changed just when compiling using sed and uuidgen. So then the key is
>>> temporay
>>> and final and it gets quite harder for a possible attacker to use this
>>> mean.
>>>
>>>
>>> Modified:
>>> ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>>> ofbiz/ofbiz-framework/trunk/framework/security/config/securi
>>> ty.properties
>>>
>>> Modified: ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
>>> lications/securityext/src/main/java/org/apache/ofbiz/securit
>>> yext/login/LoginEvents.java?rev=1814349&r1=1814348&r2=1814349&view=diff
>>> ============================================================
>>> ==================
>>> --- ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java (original)
>>> +++ ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java Sun Nov  5
>>> 11:28:42 2017
>>> @@ -69,7 +69,12 @@ public class LoginEvents {
>>>       public static final String module = LoginEvents.class.getName();
>>>       public static final String resource = "SecurityextUiLabels";
>>>       public static final String usernameCookieName = "OFBiz.Username";
>>> -    private static final String keyValue =
>>> UtilProperties.getPropertyValue(LoginWorker.securityProperties,
>>> "login.secret_key_string");
>>> +    // OOTB the loginSecretKeyString is not properly initialised and
>>> can not be OOTB.
>>> +    // The best way to create the loginSecretKeyString is to use a
>>> temporary way to load in a static final key when compiling.
>>> +    // This is simple and most secure. One of the proposed way is to
>>> use sed and uuidgen to modify the loginSecretKeyString value
>>> +    // The magic words here are TEMPORARY and FINAL!
>>> +    private static final String loginSecretKeyString =
>>> "loginSecretKeyString";
>>> +
>>>       /**
>>>        * Save USERNAME and PASSWORD for use by auth pages even if we
>>> start in non-auth pages.
>>>        *
>>> @@ -253,7 +258,7 @@ public class LoginEvents {
>>>                   autoPassword = RandomStringUtils.randomAlphan
>>> umeric(EntityUtilProperties.getPropertyAsInteger("security",
>>> "password.length.min", 5).intValue());
>>>                   EntityCrypto entityCrypto = new
>>> EntityCrypto(delegator,null);
>>>                   try {
>>> -                    passwordToSend = entityCrypto.encrypt(keyValue,
>>> EncryptMethod.TRUE, autoPassword);
>>> +                    passwordToSend = entityCrypto.encrypt(loginSecretKeyString,
>>> EncryptMethod.TRUE, autoPassword);
>>>                   } catch (GeneralException e) {
>>>                       Debug.logWarning(e, "Problem in encryption",
>>> module);
>>>                   }
>>>
>>> Modified: ofbiz/ofbiz-framework/trunk/framework/security/config/securi
>>> ty.properties
>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>> mework/security/config/security.properties?rev=1814349&r1=
>>> 1814348&r2=1814349&view=diff
>>> ============================================================
>>> ==================
>>> --- ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>>> (original)
>>> +++ ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>>> Sun Nov  5 11:28:42 2017
>>> @@ -126,7 +126,5 @@ protect-view.preprocessor=java.org.apach
>>>   #default.error.response.view=none:
>>>   default.error.response.view=view:viewBlocked
>>>   -# If false, then no externalLoginKey parameters will be added to
>>> cross-webapp urls
>>> +# -- If false, then no externalLoginKey parameters will be added to
>>> cross-webapp urls
>>>   security.login.externalLoginKey.enabled=true
>>> -#Security key used to encrypt and decrypt the autogenerated password in
>>> forgot password functionality.
>>> -login.secret_key_string=Secret Key
>>> \ No newline at end of file
>>>
>>>
>>>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/L oginEvents.java framework/security/config/security.properties

Jacques Le Roux
Administrator
Le 05/11/2017 à 19:35, Taher Alkhateeb a écrit :
> There are multiple ways to implement this solution, by far my least
> favorite is hardcoding and then recompiling!
It's not harcoding it's automatically generating using uuidgen while compiling. So you get a temporary uuid that only you know.
Of course you should not keep the source files on the production server after compiling.

> Also hardening security and
> credentials is usually not done in the generic code but in the specific
> deployment environment which differs from person to person or company to
> company.
Of course this is was is intended to do with this way.
It's not the only way, and maybe not  the best, we need a ML brainstorming and a consensus...

> If you are not careful enough in securing your file system then you might
> as well consider your security a disaster. Many many frameworks store
> credentials in text in the file system e.g. drupal, suitecrm, rails,
> wordpress, django, joomla ... and the list goes on. Oh .. and where does
> OFBiz store JDBC passwords? Wouldn't that be in entityengine.xml? Oh and
> what about the socket for AdminServer? What about anything you want to keep
> secret? Do you hard code everything?
Yes that would be an easy secure way to also for those things IMO

> Security is a strategy. It is a combination of approaches for encryption,
> networking, file systems, databases, templating and dozens of other factors
> and each person implements these factors differently.
Yes I don't prevent you to use your own :) I just suggest this as it's better than what we have currently and is easy to use.
And by proposing something OOTB we help our users to build more secure production environments and be aware about it.

> My recommendation is to revert and get community consensus on adopting any
> security strategy (or any strategy for that matter).
There is no hurry to revert in trunk. I'm ready to discuss a security strategy and get a consensus for our next releases.

Jacques

>
> On Nov 5, 2017 8:49 PM, "Jacques Le Roux" <[hidden email]>
> wrote:
>
>> OK before I revert that you might consider reading
>>
>> https://stackoverflow.com/questions/13991100/where-do-you-
>> store-your-secret-key-in-a-java-web-application
>>
>> http://movingfast.io/articles/environment-variables-considered-harmful/
>>
>> https://security.stackexchange.com/questions/49725/is-it-
>> really-secure-to-store-api-keys-in-environment-variables
>>
>> https://stackoverflow.com/questions/13991100/where-do-you-
>> store-your-secret-key-in-a-java-web-application
>>
>> But it's up to you of course, if you are still OK to use this way I'm
>> totally OK to revert it
>>
>> Jacques
>>
>>
>> Le 05/11/2017 à 15:10, Michael Brohl a écrit :
>>
>>> Hi Jacques,
>>>
>>> why don't we just discuss such changes before they got implemented?
>>>
>>> I don't think it is a valid solution to change the key in a versioned
>>> class file during compile time, that's a really strange proposal.
>>>
>>> The configuration through the properties was fine, every responsible
>>> person should take care of changing this when he is setting up a production
>>> environment. And using a database driven SystemProperty entry completely
>>> avoids having a productive key somewhere in a file.
>>>
>>> I don't see how this is really a security problem.
>>>
>>> This also breaks configuration solutions which deal with property file
>>> manipulations like proposed by Gil and me.
>>>
>>> I propose to revert this.
>>>
>>> Thanks,
>>>
>>> Michael
>>>
>>>
>>> Am 05.11.17 um 12:28 schrieb [hidden email]:
>>>
>>>> Author: jleroux
>>>> Date: Sun Nov  5 11:28:42 2017
>>>> New Revision: 1814349
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1814349&view=rev
>>>> Log:
>>>> Fixed: Secure the login.secret_key_string
>>>> (OFBIZ-9966)
>>>>
>>>> When OFBIZ-4983 was implemented I missed that we put the
>>>> login.secret_key_string
>>>>    as a property in security properties. This should not have been
>>>> because it
>>>> eases attackers work.
>>>>
>>>> The recommended way is to have it as a private static final String that
>>>> can be
>>>> changed just when compiling using sed and uuidgen. So then the key is
>>>> temporay
>>>> and final and it gets quite harder for a possible attacker to use this
>>>> mean.
>>>>
>>>>
>>>> Modified:
>>>> ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>>>> ofbiz/ofbiz-framework/trunk/framework/security/config/securi
>>>> ty.properties
>>>>
>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
>>>> lications/securityext/src/main/java/org/apache/ofbiz/securit
>>>> yext/login/LoginEvents.java?rev=1814349&r1=1814348&r2=1814349&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java (original)
>>>> +++ ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java Sun Nov  5
>>>> 11:28:42 2017
>>>> @@ -69,7 +69,12 @@ public class LoginEvents {
>>>>        public static final String module = LoginEvents.class.getName();
>>>>        public static final String resource = "SecurityextUiLabels";
>>>>        public static final String usernameCookieName = "OFBiz.Username";
>>>> -    private static final String keyValue =
>>>> UtilProperties.getPropertyValue(LoginWorker.securityProperties,
>>>> "login.secret_key_string");
>>>> +    // OOTB the loginSecretKeyString is not properly initialised and
>>>> can not be OOTB.
>>>> +    // The best way to create the loginSecretKeyString is to use a
>>>> temporary way to load in a static final key when compiling.
>>>> +    // This is simple and most secure. One of the proposed way is to
>>>> use sed and uuidgen to modify the loginSecretKeyString value
>>>> +    // The magic words here are TEMPORARY and FINAL!
>>>> +    private static final String loginSecretKeyString =
>>>> "loginSecretKeyString";
>>>> +
>>>>        /**
>>>>         * Save USERNAME and PASSWORD for use by auth pages even if we
>>>> start in non-auth pages.
>>>>         *
>>>> @@ -253,7 +258,7 @@ public class LoginEvents {
>>>>                    autoPassword = RandomStringUtils.randomAlphan
>>>> umeric(EntityUtilProperties.getPropertyAsInteger("security",
>>>> "password.length.min", 5).intValue());
>>>>                    EntityCrypto entityCrypto = new
>>>> EntityCrypto(delegator,null);
>>>>                    try {
>>>> -                    passwordToSend = entityCrypto.encrypt(keyValue,
>>>> EncryptMethod.TRUE, autoPassword);
>>>> +                    passwordToSend = entityCrypto.encrypt(loginSecretKeyString,
>>>> EncryptMethod.TRUE, autoPassword);
>>>>                    } catch (GeneralException e) {
>>>>                        Debug.logWarning(e, "Problem in encryption",
>>>> module);
>>>>                    }
>>>>
>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/security/config/securi
>>>> ty.properties
>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>> mework/security/config/security.properties?rev=1814349&r1=
>>>> 1814348&r2=1814349&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>>>> (original)
>>>> +++ ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>>>> Sun Nov  5 11:28:42 2017
>>>> @@ -126,7 +126,5 @@ protect-view.preprocessor=java.org.apach
>>>>    #default.error.response.view=none:
>>>>    default.error.response.view=view:viewBlocked
>>>>    -# If false, then no externalLoginKey parameters will be added to
>>>> cross-webapp urls
>>>> +# -- If false, then no externalLoginKey parameters will be added to
>>>> cross-webapp urls
>>>>    security.login.externalLoginKey.enabled=true
>>>> -#Security key used to encrypt and decrypt the autogenerated password in
>>>> forgot password functionality.
>>>> -login.secret_key_string=Secret Key
>>>> \ No newline at end of file
>>>>
>>>>
>>>>
>>>

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/L oginEvents.java framework/security/config/security.properties

Michael Brohl-3
Jacques,

we already have a completely secure solution if we use
EntityUtilProperties instead of UtilProperties to get the key.

Your solution introduces a completely new pattern which we have nowhere
else in the system. It introduces additional complexity and makes some
invalid/new assumptions (e.g. not having the source code on the server,
having some key generation mechanism ouside the OFBiz ecosystem).

See inline for more ...


Am 05.11.17 um 19:50 schrieb Jacques Le Roux:
> Le 05/11/2017 à 19:35, Taher Alkhateeb a écrit :
>> There are multiple ways to implement this solution, by far my least
>> favorite is hardcoding and then recompiling!
> It's not harcoding it's automatically generating using uuidgen while
> compiling. So you get a temporary uuid that only you know.
> Of course you should not keep the source files on the production
> server after compiling.
!
>
>> Also hardening security and
>> credentials is usually not done in the generic code but in the specific
>> deployment environment which differs from person to person or company to
>> company.
> Of course this is was is intended to do with this way.
> It's not the only way, and maybe not  the best, we need a ML
> brainstorming and a consensus...

Right, and as we had discussed several times before, this should happen
BEFORE the commit. There was only one hour between the issue creation
and the commit. That's far from brainstorming and getting consensus in
the community.

>
>> If you are not careful enough in securing your file system then you
>> might
>> as well consider your security a disaster. Many many frameworks store
>> credentials in text in the file system e.g. drupal, suitecrm, rails,
>> wordpress, django, joomla ... and the list goes on. Oh .. and where does
>> OFBiz store JDBC passwords? Wouldn't that be in entityengine.xml? Oh and
>> what about the socket for AdminServer? What about anything you want
>> to keep
>> secret? Do you hard code everything?
> Yes that would be an easy secure way to also for those things IMO
>
>> Security is a strategy. It is a combination of approaches for
>> encryption,
>> networking, file systems, databases, templating and dozens of other
>> factors
>> and each person implements these factors differently.
> Yes I don't prevent you to use your own :) I just suggest this as it's
> better than what we have currently and is easy to use.
> And by proposing something OOTB we help our users to build more secure
> production environments and be aware about it.
I'll repeat: just change to the use of EntityUtilProperties and the user
has the tool he needs.

>
>> My recommendation is to revert and get community consensus on
>> adopting any
>> security strategy (or any strategy for that matter).
> There is no hurry to revert in trunk. I'm ready to discuss a security
> strategy and get a consensus for our next releases.
There is the same hurry as you had when you introduced this. We are
planning a new release and should not introduce half-baked solutions
which make things worse.

So please revert as you have offered just before.

And please let us have a discussion and review of a patch for such new
solutions BEFORE it is committed. Thanks!


>
> Jacques
>>
>> On Nov 5, 2017 8:49 PM, "Jacques Le Roux" <[hidden email]>
>> wrote:
>>
>>> OK before I revert that you might consider reading
>>>
>>> https://stackoverflow.com/questions/13991100/where-do-you-
>>> store-your-secret-key-in-a-java-web-application
>>>
>>> http://movingfast.io/articles/environment-variables-considered-harmful/
>>>
>>> https://security.stackexchange.com/questions/49725/is-it-
>>> really-secure-to-store-api-keys-in-environment-variables
>>>
>>> https://stackoverflow.com/questions/13991100/where-do-you-
>>> store-your-secret-key-in-a-java-web-application
>>>
>>> But it's up to you of course, if you are still OK to use this way I'm
>>> totally OK to revert it
>>>
>>> Jacques
>>>
>>>
>>> Le 05/11/2017 à 15:10, Michael Brohl a écrit :
>>>
>>>> Hi Jacques,
>>>>
>>>> why don't we just discuss such changes before they got implemented?
>>>>
>>>> I don't think it is a valid solution to change the key in a versioned
>>>> class file during compile time, that's a really strange proposal.
>>>>
>>>> The configuration through the properties was fine, every responsible
>>>> person should take care of changing this when he is setting up a
>>>> production
>>>> environment. And using a database driven SystemProperty entry
>>>> completely
>>>> avoids having a productive key somewhere in a file.
>>>>
>>>> I don't see how this is really a security problem.
>>>>
>>>> This also breaks configuration solutions which deal with property file
>>>> manipulations like proposed by Gil and me.
>>>>
>>>> I propose to revert this.
>>>>
>>>> Thanks,
>>>>
>>>> Michael
>>>>
>>>>
>>>> Am 05.11.17 um 12:28 schrieb [hidden email]:
>>>>
>>>>> Author: jleroux
>>>>> Date: Sun Nov  5 11:28:42 2017
>>>>> New Revision: 1814349
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1814349&view=rev
>>>>> Log:
>>>>> Fixed: Secure the login.secret_key_string
>>>>> (OFBIZ-9966)
>>>>>
>>>>> When OFBIZ-4983 was implemented I missed that we put the
>>>>> login.secret_key_string
>>>>>    as a property in security properties. This should not have been
>>>>> because it
>>>>> eases attackers work.
>>>>>
>>>>> The recommended way is to have it as a private static final String
>>>>> that
>>>>> can be
>>>>> changed just when compiling using sed and uuidgen. So then the key is
>>>>> temporay
>>>>> and final and it gets quite harder for a possible attacker to use
>>>>> this
>>>>> mean.
>>>>>
>>>>>
>>>>> Modified:
>>>>> ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>>>>> ofbiz/ofbiz-framework/trunk/framework/security/config/securi
>>>>> ty.properties
>>>>>
>>>>> Modified:
>>>>> ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
>>>>> lications/securityext/src/main/java/org/apache/ofbiz/securit
>>>>> yext/login/LoginEvents.java?rev=1814349&r1=1814348&r2=1814349&view=diff
>>>>>
>>>>> ============================================================
>>>>> ==================
>>>>> --- ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java (original)
>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java Sun Nov  5
>>>>> 11:28:42 2017
>>>>> @@ -69,7 +69,12 @@ public class LoginEvents {
>>>>>        public static final String module =
>>>>> LoginEvents.class.getName();
>>>>>        public static final String resource = "SecurityextUiLabels";
>>>>>        public static final String usernameCookieName =
>>>>> "OFBiz.Username";
>>>>> -    private static final String keyValue =
>>>>> UtilProperties.getPropertyValue(LoginWorker.securityProperties,
>>>>> "login.secret_key_string");
>>>>> +    // OOTB the loginSecretKeyString is not properly initialised and
>>>>> can not be OOTB.
>>>>> +    // The best way to create the loginSecretKeyString is to use a
>>>>> temporary way to load in a static final key when compiling.
>>>>> +    // This is simple and most secure. One of the proposed way is to
>>>>> use sed and uuidgen to modify the loginSecretKeyString value
>>>>> +    // The magic words here are TEMPORARY and FINAL!
>>>>> +    private static final String loginSecretKeyString =
>>>>> "loginSecretKeyString";
>>>>> +
>>>>>        /**
>>>>>         * Save USERNAME and PASSWORD for use by auth pages even if we
>>>>> start in non-auth pages.
>>>>>         *
>>>>> @@ -253,7 +258,7 @@ public class LoginEvents {
>>>>>                    autoPassword = RandomStringUtils.randomAlphan
>>>>> umeric(EntityUtilProperties.getPropertyAsInteger("security",
>>>>> "password.length.min", 5).intValue());
>>>>>                    EntityCrypto entityCrypto = new
>>>>> EntityCrypto(delegator,null);
>>>>>                    try {
>>>>> -                    passwordToSend = entityCrypto.encrypt(keyValue,
>>>>> EncryptMethod.TRUE, autoPassword);
>>>>> +                    passwordToSend =
>>>>> entityCrypto.encrypt(loginSecretKeyString,
>>>>> EncryptMethod.TRUE, autoPassword);
>>>>>                    } catch (GeneralException e) {
>>>>>                        Debug.logWarning(e, "Problem in encryption",
>>>>> module);
>>>>>                    }
>>>>>
>>>>> Modified:
>>>>> ofbiz/ofbiz-framework/trunk/framework/security/config/securi
>>>>> ty.properties
>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>> mework/security/config/security.properties?rev=1814349&r1=
>>>>> 1814348&r2=1814349&view=diff
>>>>> ============================================================
>>>>> ==================
>>>>> ---
>>>>> ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>>>>> (original)
>>>>> +++
>>>>> ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>>>>> Sun Nov  5 11:28:42 2017
>>>>> @@ -126,7 +126,5 @@ protect-view.preprocessor=java.org.apach
>>>>>    #default.error.response.view=none:
>>>>>    default.error.response.view=view:viewBlocked
>>>>>    -# If false, then no externalLoginKey parameters will be added to
>>>>> cross-webapp urls
>>>>> +# -- If false, then no externalLoginKey parameters will be added to
>>>>> cross-webapp urls
>>>>>    security.login.externalLoginKey.enabled=true
>>>>> -#Security key used to encrypt and decrypt the autogenerated
>>>>> password in
>>>>> forgot password functionality.
>>>>> -login.secret_key_string=Secret Key
>>>>> \ No newline at end of file
>>>>>
>>>>>
>>>>>
>>>>
>


smime.p7s (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/main/java/org/apache/ofbiz/securityext/login/L oginEvents.java framework/security/config/security.properties

Jacques Le Roux
Administrator
Hi Michael,

As promised I reverted at revision 1814392, rest inline...


Le 05/11/2017 à 21:21, Michael Brohl a écrit :
> Jacques,
>
> we already have a completely secure solution if we use EntityUtilProperties instead of UtilProperties to get the key.
I disagree on that, look for instance at the point 7 of https://security.stackexchange.com/questions/12332/where-to-store-a-key-for-encryption

>
> Your solution introduces a completely new pattern which we have nowhere else in the system. It introduces additional complexity and makes some
> invalid/new assumptions (e.g. not having the source code on the server, having some key generation mechanism ouside the OFBiz ecosystem).
I did it for good reasons and not after having thought at it by myself and then look at what others say about it on the net. You have to store the
secret key somewhere, where it's the best place is the problem.
So I believe that by

 1. dynamically generating an uuid as a secret key just before compiling;
 2. get this value before compiling and store it somewhere out of the production server/s. An usb key for instance, you are then following the point 1
    of the link above.
 3. then compile;
 4. remove the source from the production, or at least the file where the secret key has been generated;

you get a solid security solution for secret key. I'm all ears to listen to your propositions if you have a better ones.


>
> See inline for more ...
>
>
> Am 05.11.17 um 19:50 schrieb Jacques Le Roux:
>> Le 05/11/2017 à 19:35, Taher Alkhateeb a écrit :
>>> There are multiple ways to implement this solution, by far my least
>>> favorite is hardcoding and then recompiling!
>> It's not harcoding it's automatically generating using uuidgen while compiling. So you get a temporary uuid that only you know.
>> Of course you should not keep the source files on the production server after compiling.
> !
Yes why would you need the source on the production server? Apart of course the XML, FTL files and such where I suppose you will not store a secret key!

>>
>>> Also hardening security and
>>> credentials is usually not done in the generic code but in the specific
>>> deployment environment which differs from person to person or company to
>>> company.
>> Of course this is was is intended to do with this way.
>> It's not the only way, and maybe not  the best, we need a ML brainstorming and a consensus...
>
> Right, and as we had discussed several times before, this should happen BEFORE the commit. There was only one hour between the issue creation and
> the commit. That's far from brainstorming and getting consensus in the community.
I reverted, we can discuss now :)

>
>>
>>> If you are not careful enough in securing your file system then you might
>>> as well consider your security a disaster. Many many frameworks store
>>> credentials in text in the file system e.g. drupal, suitecrm, rails,
>>> wordpress, django, joomla ... and the list goes on. Oh .. and where does
>>> OFBiz store JDBC passwords? Wouldn't that be in entityengine.xml? Oh and
>>> what about the socket for AdminServer? What about anything you want to keep
>>> secret? Do you hard code everything?
>> Yes that would be an easy secure way to also for those things IMO
>>
>>> Security is a strategy. It is a combination of approaches for encryption,
>>> networking, file systems, databases, templating and dozens of other factors
>>> and each person implements these factors differently.
>> Yes I don't prevent you to use your own :) I just suggest this as it's better than what we have currently and is easy to use.
>> And by proposing something OOTB we help our users to build more secure production environments and be aware about it.
>
> I'll repeat: just change to the use of EntityUtilProperties and the user has the tool he needs.
Not secure enough for me, see why above

>
>>
>>> My recommendation is to revert and get community consensus on adopting any
>>> security strategy (or any strategy for that matter).
>> There is no hurry to revert in trunk. I'm ready to discuss a security strategy and get a consensus for our next releases.
> There is the same hurry as you had when you introduced this. We are planning a new release and should not introduce half-baked solutions which make
> things worse.
There you are going to far, you have nothing serious to propose but the status-quo. Of course if you don't care about security it's OK, I don't.

>
> So please revert as you have offered just before.
>
> And please let us have a discussion and review of a patch for such new solutions BEFORE it is committed. Thanks!
Done

Jacques

>
>
>>
>> Jacques
>>>
>>> On Nov 5, 2017 8:49 PM, "Jacques Le Roux" <[hidden email]>
>>> wrote:
>>>
>>>> OK before I revert that you might consider reading
>>>>
>>>> https://stackoverflow.com/questions/13991100/where-do-you-
>>>> store-your-secret-key-in-a-java-web-application
>>>>
>>>> http://movingfast.io/articles/environment-variables-considered-harmful/
>>>>
>>>> https://security.stackexchange.com/questions/49725/is-it-
>>>> really-secure-to-store-api-keys-in-environment-variables
>>>>
>>>> https://stackoverflow.com/questions/13991100/where-do-you-
>>>> store-your-secret-key-in-a-java-web-application
>>>>
>>>> But it's up to you of course, if you are still OK to use this way I'm
>>>> totally OK to revert it
>>>>
>>>> Jacques
>>>>
>>>>
>>>> Le 05/11/2017 à 15:10, Michael Brohl a écrit :
>>>>
>>>>> Hi Jacques,
>>>>>
>>>>> why don't we just discuss such changes before they got implemented?
>>>>>
>>>>> I don't think it is a valid solution to change the key in a versioned
>>>>> class file during compile time, that's a really strange proposal.
>>>>>
>>>>> The configuration through the properties was fine, every responsible
>>>>> person should take care of changing this when he is setting up a production
>>>>> environment. And using a database driven SystemProperty entry completely
>>>>> avoids having a productive key somewhere in a file.
>>>>>
>>>>> I don't see how this is really a security problem.
>>>>>
>>>>> This also breaks configuration solutions which deal with property file
>>>>> manipulations like proposed by Gil and me.
>>>>>
>>>>> I propose to revert this.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Michael
>>>>>
>>>>>
>>>>> Am 05.11.17 um 12:28 schrieb [hidden email]:
>>>>>
>>>>>> Author: jleroux
>>>>>> Date: Sun Nov  5 11:28:42 2017
>>>>>> New Revision: 1814349
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1814349&view=rev
>>>>>> Log:
>>>>>> Fixed: Secure the login.secret_key_string
>>>>>> (OFBIZ-9966)
>>>>>>
>>>>>> When OFBIZ-4983 was implemented I missed that we put the
>>>>>> login.secret_key_string
>>>>>>    as a property in security properties. This should not have been
>>>>>> because it
>>>>>> eases attackers work.
>>>>>>
>>>>>> The recommended way is to have it as a private static final String that
>>>>>> can be
>>>>>> changed just when compiling using sed and uuidgen. So then the key is
>>>>>> temporay
>>>>>> and final and it gets quite harder for a possible attacker to use this
>>>>>> mean.
>>>>>>
>>>>>>
>>>>>> Modified:
>>>>>> ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>>>>>> ofbiz/ofbiz-framework/trunk/framework/security/config/securi
>>>>>> ty.properties
>>>>>>
>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java
>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
>>>>>> lications/securityext/src/main/java/org/apache/ofbiz/securit
>>>>>> yext/login/LoginEvents.java?rev=1814349&r1=1814348&r2=1814349&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java (original)
>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/securityext/src/mai
>>>>>> n/java/org/apache/ofbiz/securityext/login/LoginEvents.java Sun Nov  5
>>>>>> 11:28:42 2017
>>>>>> @@ -69,7 +69,12 @@ public class LoginEvents {
>>>>>>        public static final String module = LoginEvents.class.getName();
>>>>>>        public static final String resource = "SecurityextUiLabels";
>>>>>>        public static final String usernameCookieName = "OFBiz.Username";
>>>>>> -    private static final String keyValue =
>>>>>> UtilProperties.getPropertyValue(LoginWorker.securityProperties,
>>>>>> "login.secret_key_string");
>>>>>> +    // OOTB the loginSecretKeyString is not properly initialised and
>>>>>> can not be OOTB.
>>>>>> +    // The best way to create the loginSecretKeyString is to use a
>>>>>> temporary way to load in a static final key when compiling.
>>>>>> +    // This is simple and most secure. One of the proposed way is to
>>>>>> use sed and uuidgen to modify the loginSecretKeyString value
>>>>>> +    // The magic words here are TEMPORARY and FINAL!
>>>>>> +    private static final String loginSecretKeyString =
>>>>>> "loginSecretKeyString";
>>>>>> +
>>>>>>        /**
>>>>>>         * Save USERNAME and PASSWORD for use by auth pages even if we
>>>>>> start in non-auth pages.
>>>>>>         *
>>>>>> @@ -253,7 +258,7 @@ public class LoginEvents {
>>>>>>                    autoPassword = RandomStringUtils.randomAlphan
>>>>>> umeric(EntityUtilProperties.getPropertyAsInteger("security",
>>>>>> "password.length.min", 5).intValue());
>>>>>>                    EntityCrypto entityCrypto = new
>>>>>> EntityCrypto(delegator,null);
>>>>>>                    try {
>>>>>> -                    passwordToSend = entityCrypto.encrypt(keyValue,
>>>>>> EncryptMethod.TRUE, autoPassword);
>>>>>> +                    passwordToSend = entityCrypto.encrypt(loginSecretKeyString,
>>>>>> EncryptMethod.TRUE, autoPassword);
>>>>>>                    } catch (GeneralException e) {
>>>>>>                        Debug.logWarning(e, "Problem in encryption",
>>>>>> module);
>>>>>>                    }
>>>>>>
>>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/security/config/securi
>>>>>> ty.properties
>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>>>> mework/security/config/security.properties?rev=1814349&r1=
>>>>>> 1814348&r2=1814349&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>>>>>> (original)
>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/security/config/security.properties
>>>>>> Sun Nov  5 11:28:42 2017
>>>>>> @@ -126,7 +126,5 @@ protect-view.preprocessor=java.org.apach
>>>>>>    #default.error.response.view=none:
>>>>>>    default.error.response.view=view:viewBlocked
>>>>>>    -# If false, then no externalLoginKey parameters will be added to
>>>>>> cross-webapp urls
>>>>>> +# -- If false, then no externalLoginKey parameters will be added to
>>>>>> cross-webapp urls
>>>>>>    security.login.externalLoginKey.enabled=true
>>>>>> -#Security key used to encrypt and decrypt the autogenerated password in
>>>>>> forgot password functionality.
>>>>>> -login.secret_key_string=Secret Key
>>>>>> \ No newline at end of file
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>
>
>