Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/m
Locked
Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/m
 
|
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)
Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/m
Administrator
|
Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/m
|
Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/m
|
Administrator
|
Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/m
|
|
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. 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 >>>>> >>>>> >>>>> >>>> > |
Re: svn commit: r1814349 - in /ofbiz/ofbiz-framework/trunk: applications/securityext/src/m
|
Administrator
|
| Free forum by Nabble | Edit this page |