oh that is all perfect feedback -- Jean-Louis Monteiro http://twitter.com/jlouismonteiro http://www.tomitribe.com
On Mon, Nov 2, 2015 at 11:36 AM, Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > Ok just checked and we didnt expose the setPassword to JMX for dbcp so we > are fine, sorry for the noise! > > > Romain Manni-Bucau > @rmannibucau <https://twitter.com/rmannibucau> | Blog > <http://rmannibucau.wordpress.com> | Github < > https://github.com/rmannibucau> | > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Tomitriber > <http://www.tomitribe.com> > > 2015-10-30 17:39 GMT-07:00 Jean-Louis Monteiro <jlmonte...@tomitribe.com>: > > > Sure. Maybe the fix can be improved. Let's discuss > > Le 30 oct. 2015 17:37, "Romain Manni-Bucau" <rmannibu...@gmail.com> a > > écrit : > > > > > It prevents to reset the pwd at runtime if i read it correctly. It > should > > > be fixed since it is used at least through jmx. > > > ---------- Message transféré ---------- > > > De : <jlmonte...@apache.org> > > > Date : 30 oct. 2015 16:14 > > > Objet : tomee git commit: TOMEE-1648: The password cipher is called > each > > > time the datasource is built even if we already have the clear password > > > À : <comm...@tomee.apache.org> > > > Cc : > > > > > > Repository: tomee > > > Updated Branches: > > > refs/heads/master 34c4cc7a2 -> 436089b65 > > > > > > > > > TOMEE-1648: The password cipher is called each time the datasource is > > built > > > even if we already have the clear password > > > > > > > > > Project: http://git-wip-us.apache.org/repos/asf/tomee/repo > > > Commit: http://git-wip-us.apache.org/repos/asf/tomee/commit/436089b6 > > > Tree: http://git-wip-us.apache.org/repos/asf/tomee/tree/436089b6 > > > Diff: http://git-wip-us.apache.org/repos/asf/tomee/diff/436089b6 > > > > > > Branch: refs/heads/master > > > Commit: 436089b658e239757e0a51b49be01974ca339627 > > > Parents: 34c4cc7 > > > Author: Jean-Louis Monteiro <jlmonte...@apache.org> > > > Authored: Fri Oct 30 16:13:56 2015 -0700 > > > Committer: Jean-Louis Monteiro <jlmonte...@apache.org> > > > Committed: Fri Oct 30 16:13:56 2015 -0700 > > > > > > ---------------------------------------------------------------------- > > > .../resource/jdbc/dbcp/BasicDataSource.java | 20 +- > > > .../jdbc/dbcp/BasicManagedDataSource.java | 23 ++- > > > .../jdbc/CipheredPasswordDataSourceTest.java | 193 > > +++++++++++++++++++ > > > 3 files changed, 233 insertions(+), 3 deletions(-) > > > ---------------------------------------------------------------------- > > > > > > > > > > > > > > > http://git-wip-us.apache.org/repos/asf/tomee/blob/436089b6/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/dbcp/BasicDataSource.java > > > ---------------------------------------------------------------------- > > > diff --git > > > > > > > > > a/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/dbcp/BasicDataSource.java > > > > > > > > > b/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/dbcp/BasicDataSource.java > > > index ffb5bba..995221c 100644 > > > --- > > > > > > > > > a/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/dbcp/BasicDataSource.java > > > +++ > > > > > > > > > b/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/dbcp/BasicDataSource.java > > > @@ -55,6 +55,10 @@ public class BasicDataSource extends > > > org.apache.commons.dbcp2.BasicDataSource im > > > * not ciphered. The {@link > > > org.apache.openejb.cipher.PlainTextPasswordCipher} can also be used. > > > */ > > > private String passwordCipher; > > > + > > > + // keep tracking the user configured password in case we need it > to > > be > > > decrypted again > > > + private String initialPassword; > > > + > > > private JMXBasicDataSource jmxDs; > > > private CommonDataSource delegate; > > > private String name; > > > @@ -128,6 +132,18 @@ public class BasicDataSource extends > > > org.apache.commons.dbcp2.BasicDataSource im > > > } > > > } > > > > > > + @Override > > > + public void setPassword(final String password) { > > > + final ReentrantLock l = lock; > > > + l.lock(); > > > + try { > > > + // keep the encrypted value if it's encrypted > > > + this.initialPassword = password; > > > + super.setPassword(password); > > > + } finally { > > > + l.unlock(); > > > + } > > > + } > > > > > > public String getUserName() { > > > final ReentrantLock l = lock; > > > @@ -226,7 +242,9 @@ public class BasicDataSource extends > > > org.apache.commons.dbcp2.BasicDataSource im > > > // check password codec if available > > > if (null != passwordCipher && > > > !"PlainText".equals(passwordCipher)) { > > > final PasswordCipher cipher = > > > PasswordCipherFactory.getPasswordCipher(passwordCipher); > > > - final String plainPwd = > > > cipher.decrypt(getPassword().toCharArray()); > > > + > > > + // always use the initial encrypted value > > > + final String plainPwd = > > > cipher.decrypt(initialPassword.toCharArray()); > > > > > > // override previous password value > > > super.setPassword(plainPwd); > > > > > > > > > > > > http://git-wip-us.apache.org/repos/asf/tomee/blob/436089b6/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/dbcp/BasicManagedDataSource.java > > > ---------------------------------------------------------------------- > > > diff --git > > > > > > > > > a/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/dbcp/BasicManagedDataSource.java > > > > > > > > > b/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/dbcp/BasicManagedDataSource.java > > > index ec604b0..0846f21 100644 > > > --- > > > > > > > > > a/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/dbcp/BasicManagedDataSource.java > > > +++ > > > > > > > > > b/container/openejb-core/src/main/java/org/apache/openejb/resource/jdbc/dbcp/BasicManagedDataSource.java > > > @@ -54,6 +54,10 @@ public class BasicManagedDataSource extends > > > org.apache.commons.dbcp2.managed.Bas > > > * not ciphered. The {@link > > > org.apache.openejb.cipher.PlainTextPasswordCipher} can also be used. > > > */ > > > private String passwordCipher; > > > + > > > + // keep tracking the user configured password in case we need it > to > > be > > > decrypted again > > > + private String initialPassword; > > > + > > > private JMXBasicDataSource jmxDs; > > > > > > public BasicManagedDataSource(final String name) { > > > @@ -85,7 +89,7 @@ public class BasicManagedDataSource extends > > > org.apache.commons.dbcp2.managed.Bas > > > > > > private void setJndiXaDataSource(final String xaDataSource) { > > > setXaDataSourceInstance( // proxy cause we don't know if this > > > datasource was created before or not the delegate > > > - XADataSourceResource.proxy(getDriverClassLoader() != null > ? > > > getDriverClassLoader() : > Thread.currentThread().getContextClassLoader(), > > > xaDataSource)); > > > + XADataSourceResource.proxy(getDriverClassLoader() != > > null > > > ? getDriverClassLoader() : > > Thread.currentThread().getContextClassLoader(), > > > xaDataSource)); > > > > > > if (getTransactionManager() == null) { > > > setTransactionManager(OpenEJB.getTransactionManager()); > > > @@ -133,6 +137,19 @@ public class BasicManagedDataSource extends > > > org.apache.commons.dbcp2.managed.Bas > > > } > > > } > > > > > > + @Override > > > + public void setPassword(final String password) { > > > + final ReentrantLock l = lock; > > > + l.lock(); > > > + try { > > > + // keep the encrypted value if it's encrypted > > > + this.initialPassword = password; > > > + super.setPassword(password); > > > + } finally { > > > + l.unlock(); > > > + } > > > + } > > > + > > > public String getUserName() { > > > final ReentrantLock l = lock; > > > l.lock(); > > > @@ -229,7 +246,9 @@ public class BasicManagedDataSource extends > > > org.apache.commons.dbcp2.managed.Bas > > > // check password codec if available > > > if (null != passwordCipher) { > > > final PasswordCipher cipher = > > > PasswordCipherFactory.getPasswordCipher(passwordCipher); > > > - final String plainPwd = > > > cipher.decrypt(getPassword().toCharArray()); > > > + > > > + // always use the initial encrypted value > > > + final String plainPwd = > > > cipher.decrypt(initialPassword.toCharArray()); > > > > > > // override previous password value > > > super.setPassword(plainPwd); > > > > > > > > > > > > http://git-wip-us.apache.org/repos/asf/tomee/blob/436089b6/container/openejb-core/src/test/java/org/apache/openejb/resource/jdbc/CipheredPasswordDataSourceTest.java > > > ---------------------------------------------------------------------- > > > diff --git > > > > > > > > > a/container/openejb-core/src/test/java/org/apache/openejb/resource/jdbc/CipheredPasswordDataSourceTest.java > > > > > > > > > b/container/openejb-core/src/test/java/org/apache/openejb/resource/jdbc/CipheredPasswordDataSourceTest.java > > > new file mode 100644 > > > index 0000000..0f1224f > > > --- /dev/null > > > +++ > > > > > > > > > b/container/openejb-core/src/test/java/org/apache/openejb/resource/jdbc/CipheredPasswordDataSourceTest.java > > > @@ -0,0 +1,193 @@ > > > +/* > > > + * Licensed to the Apache Software Foundation (ASF) under one or more > > > + * contributor license agreements. See the NOTICE file distributed > with > > > + * this work for additional information regarding copyright ownership. > > > + * The ASF licenses this file to You under the Apache License, Version > > 2.0 > > > + * (the "License"); you may not use this file except in compliance > with > > > + * the License. You may obtain a copy of the License at > > > + * > > > + * http://www.apache.org/licenses/LICENSE-2.0 > > > + * > > > + * Unless required by applicable law or agreed to in writing, software > > > + * distributed under the License is distributed on an "AS IS" BASIS, > > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > > > implied. > > > + * See the License for the specific language governing permissions and > > > + * limitations under the License. > > > + */ > > > +package org.apache.openejb.resource.jdbc; > > > + > > > +import org.apache.commons.dbcp.PoolableConnection; > > > +import org.apache.openejb.cipher.PasswordCipher; > > > +import org.apache.openejb.jee.EjbJar; > > > +import org.apache.openejb.jee.SingletonBean; > > > +import org.apache.openejb.junit.ApplicationComposer; > > > +import org.apache.openejb.resource.jdbc.dbcp.BasicManagedDataSource; > > > +import org.apache.openejb.testing.Configuration; > > > +import org.apache.openejb.testing.Module; > > > +import org.apache.openejb.util.Strings; > > > +import org.apache.openejb.util.reflection.Reflections; > > > +import org.junit.Before; > > > +import org.junit.BeforeClass; > > > +import org.junit.Test; > > > +import org.junit.runner.RunWith; > > > + > > > +import javax.annotation.Resource; > > > +import javax.ejb.EJB; > > > +import javax.ejb.EJBContext; > > > +import javax.ejb.LocalBean; > > > +import javax.ejb.Singleton; > > > +import javax.ejb.Startup; > > > +import javax.ejb.TransactionAttribute; > > > +import javax.ejb.TransactionAttributeType; > > > +import javax.sql.DataSource; > > > +import java.sql.Connection; > > > +import java.sql.DriverManager; > > > +import java.sql.DriverPropertyInfo; > > > +import java.sql.ResultSet; > > > +import java.sql.SQLException; > > > +import java.sql.SQLFeatureNotSupportedException; > > > +import java.sql.Statement; > > > +import java.util.Properties; > > > +import java.util.logging.Logger; > > > + > > > +import static org.junit.Assert.assertFalse; > > > +import static org.junit.Assert.assertTrue; > > > + > > > +@RunWith(ApplicationComposer.class) > > > +public class CipheredPasswordDataSourceTest { > > > + private static final String URL = "jdbc:fake://sthg:3306"; > > > + private static final String USER = "pete"; > > > + private static final String ENCRYPTED_PASSWORD = "This is the > > > encrypted value."; > > > + > > > + @EJB > > > + private Persister persistManager; > > > + > > > + @Resource > > > + private DataSource ds; > > > + > > > + @Configuration > > > + public Properties config() { > > > + final Properties p = new Properties(); > > > + p.put("openejb.jdbc.datasource-creator", "dbcp"); > > > + > > > + p.put("managed", "new://Resource?type=DataSource"); > > > + p.put("managed.JdbcDriver", FakeDriver.class.getName()); > > > + p.put("managed.JdbcUrl", URL); > > > + p.put("managed.UserName", USER); > > > + p.put("managed.Password", ENCRYPTED_PASSWORD); > > > + p.put("managed.PasswordCipher", > > > EmptyPasswordCipher.class.getName()); > > > + p.put("managed.JtaManaged", "true"); > > > + p.put("managed.initialSize", "10"); > > > + p.put("managed.maxActive", "10"); > > > + p.put("managed.maxIdle", "10"); > > > + p.put("managed.minIdle", "10"); > > > + p.put("managed.maxWait", "200"); > > > + p.put("managed.defaultAutoCommit", "false"); > > > + p.put("managed.defaultReadOnly", "false"); > > > + p.put("managed.testOnBorrow", "true"); > > > + p.put("managed.testOnReturn", "true"); > > > + p.put("managed.testWhileIdle", "true"); > > > + > > > + return p; > > > + } > > > + > > > + @Module > > > + public EjbJar app() throws Exception { > > > + return new EjbJar() > > > + .enterpriseBean(new > > > SingletonBean(Persister.class).localBean()); > > > + > > > + } > > > + > > > + @Before > > > + public void createDatasource(){ > > > + // This is to make sure TomEE created the data source. > > > + persistManager.save(); > > > + } > > > + > > > + @Test > > > + public void rebuild() { > > > + // because of the exception at startup, the pool creating > > aborted > > > + // before fixing this bug, the password was already decrypted, > > but > > > the data source field > > > + // wasn't yet initialized, so this.data source == null > > > + // but the next time we try to initialize it, it tries to > > decrypt > > > and already decrypted password > > > + > > > + try { > > > + ds.getConnection(); > > > + > > > + } catch (final SQLException e) { > > > + System.out.println(e.getMessage()); > > > + // success - it throws the SQLException from the connect > > > + // but it does not fail with the IllegalArgumentException > > from > > > the Password Cipher > > > + } > > > + } > > > + > > > + public static class EmptyPasswordCipher implements PasswordCipher > { > > > + > > > + @Override > > > + public char[] encrypt(String plainPassword) { > > > + throw new RuntimeException("Should never be called in this > > > test."); > > > + } > > > + > > > + @Override > > > + public String decrypt(char[] encryptedPassword) { > > > + System.out.println(String.format(">>> Decrypt password > > '%s'", > > > new String(encryptedPassword))); > > > + > > > + // we want to know if the password as already been called > > > + if (encryptedPassword.length == 0) { > > > + throw new IllegalArgumentException("Can only decrypt a > > non > > > empty string."); > > > + } > > > + > > > + return ""; > > > + } > > > + } > > > + > > > + public static class FakeDriver implements java.sql.Driver { > > > + public boolean acceptsURL(final String url) throws > SQLException > > { > > > + return false; > > > + } > > > + > > > + public Logger getParentLogger() throws > > > SQLFeatureNotSupportedException { > > > + return null; > > > + } > > > + > > > + public Connection connect(final String url, final Properties > > info) > > > throws SQLException { > > > + throw new SQLException("Pete said it first fails here!"); > > > + } > > > + > > > + public int getMajorVersion() { > > > + return 0; > > > + } > > > + > > > + public int getMinorVersion() { > > > + return 0; > > > + } > > > + > > > + public DriverPropertyInfo[] getPropertyInfo(final String url, > > > final Properties info) throws SQLException { > > > + return new DriverPropertyInfo[0]; > > > + } > > > + > > > + public boolean jdbcCompliant() { > > > + return false; > > > + } > > > + } > > > + > > > + @LocalBean > > > + @Singleton > > > + @TransactionAttribute(TransactionAttributeType.REQUIRES_NEW) > > > + public static class Persister { > > > + > > > + @Resource(name = "managed") > > > + private DataSource ds; > > > + > > > + public void save() { > > > + try { > > > + ds.getConnection(); > > > + > > > + } catch (SQLException e) { > > > + System.err.println("Generated SQL error > " + > > > e.getMessage()); > > > + } > > > + } > > > + } > > > + > > > + > > > +} > > > > > >
