Mercurial > hg > thermostat-ng > agent
changeset 1735:4a5706c9f0cb
Prevent NPE after CVE-2015-3201 fix in web-storage-service.
Reviewed-by: omajid, neugens
Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2015-May/013734.html
PR2372
author | Severin Gehwolf <sgehwolf@redhat.com> |
---|---|
date | Fri, 22 May 2015 12:25:48 +0200 |
parents | 8d3ce721d520 |
children | 767301642c26 |
files | distribution/config/web.auth distribution/pom.xml distribution/scripts/thermostat-setup integration-tests/itest-run/src/test/java/com/redhat/thermostat/itest/WebAppTest.java web/server/src/main/java/com/redhat/thermostat/web/server/FileBasedStorageCredentials.java web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java web/war/pom.xml web/war/src/main/webapp/WEB-INF/web.auth |
diffstat | 10 files changed, 204 insertions(+), 49 deletions(-) [+] |
line wrap: on
line diff
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/distribution/config/web.auth Fri May 22 12:25:48 2015 +0200 @@ -0,0 +1,11 @@ +# Credentials to use for connecting to the backing storage +# (currently mongodb). Uncomment the following two blocks in +# order to use this username/password for the connection. + +# Username to use for connecting to the backing storage +# implementation +${web.war.backingstorage.username.snippet} + +# Password to use for connecting to the backing storage +# implementation +${web.war.backingstorage.password.snippet}
--- a/distribution/pom.xml Thu May 21 15:36:44 2015 -0400 +++ b/distribution/pom.xml Fri May 22 12:25:48 2015 +0200 @@ -157,6 +157,7 @@ <include>agent.properties</include> <include>web-storage-service.properties</include> <include>agent.auth</include> + <include>web.auth</include> <include>ssl.properties</include> <include>thermostat-users.properties</include> <include>thermostat-roles.properties</include>
--- a/distribution/scripts/thermostat-setup Thu May 21 15:36:44 2015 -0400 +++ b/distribution/scripts/thermostat-setup Fri May 22 12:25:48 2015 +0200 @@ -184,20 +184,7 @@ runSetup() { unlockThermostat setupMongodbUser - # Location of the exploded web archive. We need this - # in order to be able to inject mongodb user credentials - # in there. Allow THERMOSTAT_WEBAPP_LOCATION to be set - # as env variable. This allows for running setup on - # packaged thermostat. - if [ "x$THERMOSTAT_WEBAPP_LOCATION" = "x" ]; then - # Expecting an upstream build where THERMOSTAT_HOME is - # distribution/target/image. The web archive gets copied - # as an exploded archive to the webapp subdir of the - # image. - default_webapp="$THERMOSTAT_HOME/webapp" - THERMOSTAT_WEBAPP_LOCATION="$(readlink -f $default_webapp)" - fi - TH_WEB_AUTH="$THERMOSTAT_WEBAPP_LOCATION/WEB-INF/web.auth" + TH_WEB_AUTH="$THERMOSTAT_HOME/etc/web.auth" if [ ! -e $TH_WEB_AUTH ]; then echo "File not found: $TH_WEB_AUTH" 1>&2
--- a/integration-tests/itest-run/src/test/java/com/redhat/thermostat/itest/WebAppTest.java Thu May 21 15:36:44 2015 -0400 +++ b/integration-tests/itest-run/src/test/java/com/redhat/thermostat/itest/WebAppTest.java Fri May 22 12:25:48 2015 +0200 @@ -243,7 +243,7 @@ private static final double EQUALS_DELTA = 0.00000000000001; private static final String THERMOSTAT_USERS_FILE = getConfigurationDir() + "/thermostat-users.properties"; private static final String THERMOSTAT_ROLES_FILE = getConfigurationDir() + "/thermostat-roles.properties"; - private static final String THERMOSTAT_WEB_AUTH_FILE = getThermostatHome() + "/webapp/WEB-INF/web.auth"; + private static final String THERMOSTAT_WEB_AUTH_FILE = getConfigurationDir() + "/web.auth"; private static final String VM_ID1 = "vmId1"; private static final String VM_ID2 = "vmId2"; private static final String VM_ID3 = "vmId3";
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/FileBasedStorageCredentials.java Thu May 21 15:36:44 2015 -0400 +++ b/web/server/src/main/java/com/redhat/thermostat/web/server/FileBasedStorageCredentials.java Fri May 22 12:25:48 2015 +0200 @@ -66,10 +66,6 @@ } - public FileBasedStorageCredentials(InputStream in) throws IOException { - initialize(in); - } - private void initialize(InputStream in) throws IOException { props = new Properties(); props.load(in);
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java Thu May 21 15:36:44 2015 -0400 +++ b/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java Fri May 22 12:25:48 2015 +0200 @@ -141,6 +141,7 @@ // our strings can contain non-ASCII characters. Use UTF-8 // see also PR 1344 private static final String RESPONSE_JSON_CONTENT_TYPE = "application/json; charset=UTF-8"; + private static final String CREDENTIALS_FILE = "web.auth"; private static final Logger logger = LoggingUtils.getLogger(WebStorageEndPoint.class); @@ -243,23 +244,11 @@ @Override protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException, ServletException { if (storage == null) { - StorageCredentials creds = null; - final String CREDENTIALS_FILE = "web.auth"; - // this assumes this is always an exploded war - File systemFile = new File(getServletContext().getRealPath("/WEB-INF/" + CREDENTIALS_FILE)); - if (systemFile.exists() && systemFile.canRead()) { - logger.log(Level.CONFIG, "Loading authentication data from WEB-INF/" + CREDENTIALS_FILE); - try (InputStream in = new FileInputStream(systemFile)) { - creds = new FileBasedStorageCredentials(in); - } - } else { - File userCredentials = new File(paths.getUserConfigurationDirectory(), CREDENTIALS_FILE); - logger.log(Level.CONFIG, "Loading authentication data from " + userCredentials); - if (userCredentials.isFile() && userCredentials.canRead()) { - creds = new FileBasedStorageCredentials(userCredentials); - } else { - logger.warning("Unable to read database credentials from " + userCredentials); - } + StorageCredentials creds = getStorageCredentials(paths); + // if creds are null there is no point to continue, fail prominently. + if (creds == null) { + String errorMsg = "No backing storage credentials file (" + CREDENTIALS_FILE + ") available"; + throw new InvalidConfigurationException(errorMsg); } String storageClass = getServletConfig().getInitParameter(STORAGE_CLASS); String storageEndpoint = getServletConfig().getInitParameter(STORAGE_ENDPOINT); @@ -292,6 +281,34 @@ getMore(req, resp); } } + + // package private for testing + StorageCredentials getStorageCredentials(CommonPaths commonPaths) throws IOException { + File systemFile = getStorageCredentialsFile(commonPaths.getSystemConfigurationDirectory()); + if (systemFile.exists() && systemFile.canRead()) { + logger.log(Level.CONFIG, "Loading authentication data from " + systemFile); + return createStorageCredentials(systemFile); + } else { + File userCredentials = getStorageCredentialsFile(commonPaths.getUserConfigurationDirectory()); + logger.log(Level.CONFIG, "Loading authentication data from " + userCredentials); + if (userCredentials.isFile() && userCredentials.canRead()) { + return createStorageCredentials(userCredentials); + } else { + logger.warning("Unable to read database credentials from " + userCredentials); + return null; + } + } + } + + // package private for testing + File getStorageCredentialsFile(File parent) { + return new File(parent, CREDENTIALS_FILE); + } + + // package private for testing + StorageCredentials createStorageCredentials(File underlyingFile) { + return new FileBasedStorageCredentials(underlyingFile); + } // Side effect: sets this.paths private void sanityCheckNecessaryFiles() {
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java Thu May 21 15:36:44 2015 -0400 +++ b/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java Fri May 22 12:25:48 2015 +0200 @@ -39,6 +39,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; @@ -64,6 +65,8 @@ import org.junit.Test; import org.mockito.ArgumentCaptor; +import com.redhat.thermostat.shared.config.CommonPaths; +import com.redhat.thermostat.storage.core.StorageCredentials; import com.redhat.thermostat.web.server.auth.WebStoragePathHandler; /** @@ -250,6 +253,114 @@ verify(registry).shutDown(); } + /** + * Storage credentials should be read from USER_THERMOSTAT_HOME if + * relevant credentials file is not readable or does not exist in + * THERMOSTAT_HOME. + * + * Only the user file is readable. + * + * @throws IOException + */ + @Test + public void canGetUserStorageCredentials() throws IOException { + StorageCredsTestSetupResult setupResult = setupForStorageCredentialsTest(false, true); + WebStorageEndPoint endPoint = setupResult.endpoint; + CommonPaths paths = setupResult.commonPaths; + StorageCredentials creds = endPoint.getStorageCredentials(paths); + assertNotNull(creds); + assertTrue(creds instanceof TestStorageCredentials); + TestStorageCredentials testCreds = (TestStorageCredentials)creds; + assertTrue(setupResult.userCredsFile == testCreds.underlyingFile); + } + + /** + * If Storage credentials file exists and is readable it should + * get used over USER_THERMOSTAT_HOME's version. In this test *both* + * files are present and readable. + * + * @throws IOException + */ + @Test + public void canGetSystemStorageCredentialsBothReadable() throws IOException { + StorageCredsTestSetupResult setupResult = setupForStorageCredentialsTest(true, true); + WebStorageEndPoint endPoint = setupResult.endpoint; + CommonPaths paths = setupResult.commonPaths; + StorageCredentials creds = endPoint.getStorageCredentials(paths); + assertNotNull(creds); + assertTrue(creds instanceof TestStorageCredentials); + TestStorageCredentials testCreds = (TestStorageCredentials)creds; + assertTrue(setupResult.systemCredsFile == testCreds.underlyingFile); + } + + /** + * If Storage credentials file exists and is readable it should + * get used over USER_THERMOSTAT_HOME's version. In this test only the + * system version is readable. + * + * @throws IOException + */ + @Test + public void canGetSystemStorageCredentialsOnlySystemReadable() throws IOException { + StorageCredsTestSetupResult setupResult = setupForStorageCredentialsTest(true, false); + WebStorageEndPoint endPoint = setupResult.endpoint; + CommonPaths paths = setupResult.commonPaths; + StorageCredentials creds = endPoint.getStorageCredentials(paths); + assertNotNull(creds); + assertTrue(creds instanceof TestStorageCredentials); + TestStorageCredentials testCreds = (TestStorageCredentials)creds; + assertTrue(setupResult.systemCredsFile == testCreds.underlyingFile); + } + + private StorageCredsTestSetupResult setupForStorageCredentialsTest(boolean systemCanRead, boolean userCanRead) { + final File systemCredsFile = mock(File.class); + when(systemCredsFile.canRead()).thenReturn(systemCanRead); + when(systemCredsFile.exists()).thenReturn(true); + when(systemCredsFile.getPath()).thenReturn("system_creds_file"); + final File systemCredsDirectory = mock(File.class); + final File userCredsDirectory = mock(File.class); + final File userCredsFile = mock(File.class); + when(userCredsFile.canRead()).thenReturn(userCanRead); + when(userCredsFile.isFile()).thenReturn(true); + when(userCredsFile.getPath()).thenReturn("user_creds_file"); + CommonPaths paths = mock(CommonPaths.class); + when(paths.getSystemConfigurationDirectory()).thenReturn(systemCredsDirectory); + when(paths.getUserConfigurationDirectory()).thenReturn(userCredsDirectory); + @SuppressWarnings("serial") + WebStorageEndPoint endpoint = new WebStorageEndPoint() { + @Override + File getStorageCredentialsFile(File parent) { + // intentionally compare exact instances + if (systemCredsDirectory == parent) { + return systemCredsFile; + } + // intentionally compare exact instances + if (userCredsDirectory == parent) { + return userCredsFile; + } + return null; + } + + @Override + StorageCredentials createStorageCredentials(File underlyingFile) { + return new TestStorageCredentials(underlyingFile); + } + }; + return new StorageCredsTestSetupResult(endpoint, paths, systemCredsFile, userCredsFile); + } + + /** + * If neither storage credentials exist (none in USER_THERMOSTAT_HOME *and* + * in THERMOSTAT_HOME) then null is expected to get returned. + * @throws IOException + */ + @Test + public void storageCredentialsNull() throws IOException { + StorageCredsTestSetupResult setupResult = setupForStorageCredentialsTest(false, false); + StorageCredentials creds = setupResult.endpoint.getStorageCredentials(setupResult.commonPaths); + assertNull(creds); + } + private ThCreatorResult creatWorkingThermostatHome() throws IOException { Path testThermostatHome = Files.createTempDirectory( "foo-thermostat-home-", new FileAttribute[] {}); @@ -276,4 +387,42 @@ } } + private static class StorageCredsTestSetupResult { + private WebStorageEndPoint endpoint; + private CommonPaths commonPaths; + private File systemCredsFile; + private File userCredsFile; + + private StorageCredsTestSetupResult(WebStorageEndPoint endpoint, + CommonPaths commonPaths, + File systemCredsFile, + File userCredsFile) { + this.endpoint = endpoint; + this.commonPaths = commonPaths; + this.systemCredsFile = systemCredsFile; + this.userCredsFile = userCredsFile; + } + + } + + private static class TestStorageCredentials implements StorageCredentials { + + private File underlyingFile; + + private TestStorageCredentials(File underlyingFile) { + this.underlyingFile = underlyingFile; + } + + @Override + public String getUsername() { + throw new AssertionError("not implemented"); + } + + @Override + public char[] getPassword() { + throw new AssertionError("not implemented"); + } + + } + }
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java Thu May 21 15:36:44 2015 -0400 +++ b/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java Fri May 22 12:25:48 2015 +0200 @@ -266,6 +266,13 @@ } catch (IOException e) { throw new RuntimeException(e); } + File webAuthFile = new File(configDirectory, "web.auth"); + // only creates file if it doesn't exist yet + try { + webAuthFile.createNewFile(); + } catch (IOException e) { + throw new RuntimeException(e); + } } @Override
--- a/web/war/pom.xml Thu May 21 15:36:44 2015 -0400 +++ b/web/war/pom.xml Fri May 22 12:25:48 2015 +0200 @@ -226,7 +226,6 @@ <directory>src/main/webapp</directory> <includes> <include>**/web.xml</include> - <include>**/web.auth</include> </includes> </resource> </webResources> @@ -249,7 +248,6 @@ <directory>src/main/webapp</directory> <includes> <include>**/web.xml</include> - <include>**/web.auth</include> </includes> </resource> </webResources>
--- a/web/war/src/main/webapp/WEB-INF/web.auth Thu May 21 15:36:44 2015 -0400 +++ /dev/null Thu Jan 01 00:00:00 1970 +0000 @@ -1,11 +0,0 @@ -# Credentials to use for connecting to the backing storage -# (currently mongodb). Uncomment the following two blocks in -# order to use this username/password for the connection. - -# Username to use for connecting to the backing storage -# implementation -${web.war.backingstorage.username.snippet} - -# Password to use for connecting to the backing storage -# implementation -${web.war.backingstorage.password.snippet}