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 :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.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.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 mightas 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 andwhat about the socket for AdminServer? What about anything you want to keepsecret? Do you hard code everything?Yes that would be an easy secure way to also for those things IMOSecurity is a strategy. It is a combination of approaches for encryption, networking, file systems, databases, templating and dozens of other factorsYes 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.and each person implements these factors differently.
I'll repeat: just change to the use of EntityUtilProperties and the user has the tool he needs.
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.My recommendation is to revert and get community consensus on adopting anyThere is no hurry to revert in trunk. I'm ready to discuss a security strategy and get a consensus for our next releases.security strategy (or any strategy for that matter).
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!
JacquesOn Nov 5, 2017 8:49 PM, "Jacques Le Roux" <jacques.le.r...@les7arts.com> 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 responsibleperson should take care of changing this when he is setting up a production environment. And using a database driven SystemProperty entry completelyavoids 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 jler...@apache.org: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 thatcan be changed just when compiling using sed and uuidgen. So then the key is temporayand final and it gets quite harder for a possible attacker to use thismean. 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.propertiesModified: 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/app lications/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/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/security.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.propertiesSun 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 inforgot password functionality. -login.secret_key_string=Secret Key \ No newline at end of file
smime.p7s
Description: S/MIME Cryptographic Signature
