changeset 1691:3de392cf6a31

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 PR2373
author Severin Gehwolf <sgehwolf@redhat.com>
date Tue, 26 May 2015 19:30:10 +0200
parents ebb0f807c7aa
children 507c6521cbc4
files distribution/config/web.auth distribution/pom.xml distribution/scripts/thermostat-setup 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 9 files changed, 203 insertions(+), 48 deletions(-) [+]
line wrap: on
line diff
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/distribution/config/web.auth	Tue May 26 19:30:10 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	Wed May 20 14:40:55 2015 -0400
+++ b/distribution/pom.xml	Tue May 26 19:30:10 2015 +0200
@@ -168,6 +168,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	Wed May 20 14:40:55 2015 -0400
+++ b/distribution/scripts/thermostat-setup	Tue May 26 19:30:10 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/web/server/src/main/java/com/redhat/thermostat/web/server/FileBasedStorageCredentials.java	Wed May 20 14:40:55 2015 -0400
+++ b/web/server/src/main/java/com/redhat/thermostat/web/server/FileBasedStorageCredentials.java	Tue May 26 19:30:10 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	Wed May 20 14:40:55 2015 -0400
+++ b/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java	Tue May 26 19:30:10 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	Wed May 20 14:40:55 2015 -0400
+++ b/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java	Tue May 26 19:30:10 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	Wed May 20 14:40:55 2015 -0400
+++ b/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndpointTest.java	Tue May 26 19:30:10 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	Wed May 20 14:40:55 2015 -0400
+++ b/web/war/pom.xml	Tue May 26 19:30:10 2015 +0200
@@ -221,7 +221,6 @@
                   <directory>src/main/webapp</directory>
                   <includes>
                     <include>**/web.xml</include>
-                    <include>**/web.auth</include>
                   </includes>
                 </resource>
               </webResources>
@@ -244,7 +243,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	Wed May 20 14:40:55 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}