Mercurial > hg > thermostat
changeset 2515:6227b428daa7
Multi-user IPC support
Reviewed-by: stooke
Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2016-October/021391.html
line wrap: on
line diff
--- a/agent/command/src/main/java/com/redhat/thermostat/agent/command/internal/CommandChannelDelegate.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/command/src/main/java/com/redhat/thermostat/agent/command/internal/CommandChannelDelegate.java Tue Nov 08 15:09:04 2016 -0500 @@ -40,6 +40,10 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.nio.charset.Charset; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.UserPrincipal; import java.util.Arrays; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicInteger; @@ -92,20 +96,23 @@ private final AgentRequestDecoder requestDecoder; private final AgentResponseEncoder responseEncoder; private final ProcessCreator procCreator; + private final ProcessUserInfoBuilder userInfoBuilder; + private final FileSystemUtils fsUtils; private Process process; private AtomicInteger state; CommandChannelDelegate(ReceiverRegistry receivers, SSLConfiguration sslConf, File binPath, AgentIPCService ipcService, File ipcConfig) { this(receivers, sslConf, binPath, ipcService, ipcConfig, new CountDownLatch(1), new SSLConfigurationEncoder(), - new AgentRequestDecoder(), new AgentResponseEncoder(), new StorageGetter(), new ProcessCreator()); + new AgentRequestDecoder(), new AgentResponseEncoder(), new StorageGetter(), new ProcessUserInfoBuilder(), + new FileSystemUtils(), new ProcessCreator()); } /** For testing only */ CommandChannelDelegate(ReceiverRegistry receivers, SSLConfiguration sslConf, File binPath, AgentIPCService ipcService, File ipcConfig, CountDownLatch readyLatch, SSLConfigurationEncoder sslEncoder, - AgentRequestDecoder requestDecoder, AgentResponseEncoder responseEncoder, - StorageGetter getter, ProcessCreator procCreator) { + AgentRequestDecoder requestDecoder, AgentResponseEncoder responseEncoder, StorageGetter getter, + ProcessUserInfoBuilder userInfoBuilder, FileSystemUtils fsUtils, ProcessCreator procCreator) { this.storageGetter = getter; this.receivers = receivers; this.sslConf = sslConf; @@ -117,13 +124,24 @@ this.requestDecoder = requestDecoder; this.responseEncoder = responseEncoder; this.procCreator = procCreator; + this.userInfoBuilder = userInfoBuilder; + this.fsUtils = fsUtils; this.state = new AtomicInteger(); } @Override public void startListening(String hostname, int port) throws IOException { - // Create IPC server - ipcService.createServer(IPC_SERVER_NAME, this); + // Determine if this process is running as a privileged user + if (userInfoBuilder.isPrivilegedUser()) { + // Get owner of command channel script, which will also be the user running it + Path cmdPath = fsUtils.getPath(binPath.getAbsolutePath(), CMD_NAME); + UserPrincipal unprivilegedPrincipal = fsUtils.getOwner(cmdPath); + // Create IPC server owned by user running command channel script + ipcService.createServer(IPC_SERVER_NAME, this, unprivilegedPrincipal); + } else { + // Create IPC server owned by current user + ipcService.createServer(IPC_SERVER_NAME, this); + } startServer(hostname, port); } @@ -365,6 +383,17 @@ } } + /** for testing only */ + static class FileSystemUtils { + Path getPath(String first, String... more) { + return FileSystems.getDefault().getPath(first, more); + } + + UserPrincipal getOwner(Path path) throws IOException { + return Files.getOwner(path); + } + } + }
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/agent/command/src/main/java/com/redhat/thermostat/agent/command/internal/ProcessUserInfoBuilder.java Tue Nov 08 15:09:04 2016 -0500 @@ -0,0 +1,110 @@ +/* + * Copyright 2012-2016 Red Hat, Inc. + * + * This file is part of Thermostat. + * + * Thermostat is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published + * by the Free Software Foundation; either version 2, or (at your + * option) any later version. + * + * Thermostat is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Thermostat; see the file COPYING. If not see + * <http://www.gnu.org/licenses/>. + * + * Linking this code with other modules is making a combined work + * based on this code. Thus, the terms and conditions of the GNU + * General Public License cover the whole combination. + * + * As a special exception, the copyright holders of this code give + * you permission to link this code with independent modules to + * produce an executable, regardless of the license terms of these + * independent modules, and to copy and distribute the resulting + * executable under terms of your choice, provided that you also + * meet, for each linked independent module, the terms and conditions + * of the license of that module. An independent module is a module + * which is not derived from or based on this code. If you modify + * this code, you may extend this exception to your version of the + * library, but you are not obligated to do so. If you do not wish + * to do so, delete this exception statement from your version. + */ + +package com.redhat.thermostat.agent.command.internal; + +import java.io.BufferedReader; +import java.io.File; +import java.io.FileReader; +import java.io.IOException; + +/* + * FIXME: This class was copied from system-backend. + * Replace when this information is available from an API. + */ +class ProcessUserInfoBuilder { + + private static final String PROC_STATUS_SELF_PATH = "/proc/self/status"; + private static final String PROC_STATUS_UID = "Uid:"; + private final FileReaderCreator readerCreator; + + ProcessUserInfoBuilder() { + this(new FileReaderCreator()); + } + + ProcessUserInfoBuilder(FileReaderCreator readerCreator) { + this.readerCreator = readerCreator; + } + + private long getUid() throws IOException { + FileReader reader = readerCreator.create(PROC_STATUS_SELF_PATH); + long uid = getUidFromProcfs(new BufferedReader(reader)); + return uid; + } + + boolean isPrivilegedUser() throws IOException { + return (getUid() == 0); + } + + /* + * Look for the following line: + * Uid: <RealUid> <EffectiveUid> <SavedUid> <FSUid> + */ + private long getUidFromProcfs(BufferedReader br) throws IOException { + long uid = -1; + String line; + while ((line = br.readLine()) != null) { + line = line.trim(); + if (line.startsWith(PROC_STATUS_UID)) { + String[] parts = line.split("\\s+"); + if (parts.length == 5) { + try { + // Use Real UID + uid = Long.parseLong(parts[1]); + } catch (NumberFormatException e) { + throw new IOException("Unexpected output from ps command", e); + } + } + else { + throw new IOException("Expected 5 parts from split /proc/${pid}/status output, got " + parts.length); + } + } + } + if (uid < 0) { + throw new IOException("Unable to determine UID from /proc/${pid}/status"); + } + return uid; + } + + // For testing purposes + static class FileReaderCreator { + FileReader create(String path) throws IOException { + return new FileReader(new File(path)); + } + } + +} +
--- a/agent/command/src/test/java/com/redhat/thermostat/agent/command/internal/CommandChannelDelegateTest.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/command/src/test/java/com/redhat/thermostat/agent/command/internal/CommandChannelDelegateTest.java Tue Nov 08 15:09:04 2016 -0500 @@ -52,6 +52,8 @@ import java.lang.ProcessBuilder.Redirect; import java.nio.ByteBuffer; import java.nio.charset.Charset; +import java.nio.file.Path; +import java.nio.file.attribute.UserPrincipal; import java.util.Arrays; import java.util.List; import java.util.concurrent.CountDownLatch; @@ -65,6 +67,7 @@ import com.redhat.thermostat.agent.command.ReceiverRegistry; import com.redhat.thermostat.agent.command.RequestReceiver; +import com.redhat.thermostat.agent.command.internal.CommandChannelDelegate.FileSystemUtils; import com.redhat.thermostat.agent.command.internal.CommandChannelDelegate.ProcessCreator; import com.redhat.thermostat.agent.command.internal.CommandChannelDelegate.StorageGetter; import com.redhat.thermostat.agent.ipc.server.AgentIPCService; @@ -101,6 +104,8 @@ private CountDownLatch latch; private SSLConfiguration sslConf; private IPCMessage startedMessage; + private FileSystemUtils fsUtils; + private ProcessUserInfoBuilder userInfoBuilder; @Before public void setUp() throws Exception { @@ -139,8 +144,11 @@ when(processCreator.startProcess(any(ProcessBuilder.class))).thenReturn(process); latch = mock(CountDownLatch.class); + fsUtils = mock(FileSystemUtils.class); + userInfoBuilder = mock(ProcessUserInfoBuilder.class); delegate = new CommandChannelDelegate(receivers, sslConf, binPath, ipcService, ipcConfig, - latch, sslConfEncoder, requestDecoder, responseEncoder, storageGetter, processCreator); + latch, sslConfEncoder, requestDecoder, responseEncoder, storageGetter, userInfoBuilder, + fsUtils, processCreator); startedMessage = mock(IPCMessage.class); when(startedMessage.get()).thenReturn(ByteBuffer.wrap(CommandChannelConstants.SERVER_STARTED_TOKEN)); @@ -169,6 +177,19 @@ } @Test + public void testServerStartedPrivUser() throws Exception { + when(userInfoBuilder.isPrivilegedUser()).thenReturn(true); + Path scriptPath = mock(Path.class); + when(fsUtils.getPath(binPath.getAbsolutePath(), "thermostat-command-channel")).thenReturn(scriptPath); + UserPrincipal principal = mock(UserPrincipal.class); + when(fsUtils.getOwner(scriptPath)).thenReturn(principal); + delegate.startListening("127.0.0.1", 123); + + verify(ipcService).createServer(IPC_SERVER_NAME, delegate, principal); + verify(processCreator).startProcess(any(ProcessBuilder.class)); + } + + @Test public void testServerFailsToStart() throws Exception { doAnswer(new Answer<Void>() { @Override
--- a/agent/core/src/main/java/com/redhat/thermostat/utils/management/internal/AgentProxyClient.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/core/src/main/java/com/redhat/thermostat/utils/management/internal/AgentProxyClient.java Tue Nov 08 15:09:04 2016 -0500 @@ -50,23 +50,25 @@ private final File binPath; private final String username; private final File ipcConfigFile; + private final String serverName; - AgentProxyClient(int pid, String user, File binPath, File ipcConfigFile) { - this(pid, user, binPath, ipcConfigFile, new ProcessCreator()); + AgentProxyClient(int pid, String user, File binPath, File ipcConfigFile, String serverName) { + this(pid, user, binPath, ipcConfigFile, serverName, new ProcessCreator()); } - AgentProxyClient(int pid, String user, File binPath, File ipcConfigFile, ProcessCreator procCreator) { + AgentProxyClient(int pid, String user, File binPath, File ipcConfigFile, String serverName, ProcessCreator procCreator) { this.pid = pid; this.binPath = binPath; this.procCreator = procCreator; this.username = user; this.ipcConfigFile = ipcConfigFile; + this.serverName = serverName; } void runProcess() throws IOException, InterruptedException { // Start the agent proxy String serverPath = binPath + File.separator + SERVER_NAME; - String[] args = new String[] { serverPath, String.valueOf(pid), username, ipcConfigFile.getAbsolutePath() }; + String[] args = new String[] { serverPath, String.valueOf(pid), username, ipcConfigFile.getAbsolutePath(), serverName }; ProcessBuilder builder = new ProcessBuilder(args); builder.inheritIO(); Process proxy = procCreator.startProcess(builder);
--- a/agent/core/src/main/java/com/redhat/thermostat/utils/management/internal/MXBeanConnectionPoolImpl.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/core/src/main/java/com/redhat/thermostat/utils/management/internal/MXBeanConnectionPoolImpl.java Tue Nov 08 15:09:04 2016 -0500 @@ -41,9 +41,14 @@ import java.nio.ByteBuffer; import java.nio.CharBuffer; import java.nio.charset.Charset; +import java.nio.file.FileSystems; +import java.nio.file.attribute.UserPrincipal; +import java.nio.file.attribute.UserPrincipalLookupService; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; import java.util.Objects; +import java.util.Set; import com.google.gson.Gson; import com.google.gson.GsonBuilder; @@ -57,13 +62,14 @@ import com.redhat.thermostat.agent.utils.ProcDataSource; import com.redhat.thermostat.agent.utils.management.MXBeanConnection; import com.redhat.thermostat.agent.utils.management.MXBeanConnectionException; +import com.redhat.thermostat.agent.utils.management.MXBeanConnectionPool; import com.redhat.thermostat.agent.utils.username.UserNameUtil; import com.redhat.thermostat.utils.management.internal.ProcessUserInfoBuilder.ProcessUserInfo; public class MXBeanConnectionPoolImpl implements MXBeanConnectionPoolControl, ThermostatIPCCallbacks { private static final Object CURRENT_ENTRY_LOCK = new Object(); - static final String IPC_SERVER_NAME = "agent-proxy"; + private static final String IPC_SERVER_PREFIX = "agent-proxy"; static final String JSON_PID = "pid"; static final String JSON_JMX_URL = "jmxUrl"; @@ -74,6 +80,9 @@ private final ProcessUserInfoBuilder userInfoBuilder; private final AgentIPCService ipcService; private final File ipcConfigFile; + private final FileSystemUtils fsUtils; + // Keep track of IPC servers we created + private final Set<String> ipcServerNames; /** * Current {@link MXBeanConnectionPoolEntry} being created by {@link #acquire(int)} for use @@ -88,48 +97,48 @@ public MXBeanConnectionPoolImpl(File binPath, UserNameUtil userNameUtil, AgentIPCService ipcService, File ipcConfigFile) { this(new ConnectorCreator(), binPath, new ProcessUserInfoBuilder(new ProcDataSource(), userNameUtil), - ipcService, ipcConfigFile); + ipcService, ipcConfigFile, new FileSystemUtils()); } MXBeanConnectionPoolImpl(ConnectorCreator connectorCreator, File binPath, ProcessUserInfoBuilder userInfoBuilder, - AgentIPCService ipcService, File ipcConfigFile) { + AgentIPCService ipcService, File ipcConfigFile, FileSystemUtils fsUtils) { this.pool = new HashMap<>(); this.creator = connectorCreator; this.binPath = binPath; this.userInfoBuilder = userInfoBuilder; this.ipcService = ipcService; this.ipcConfigFile = ipcConfigFile; + this.fsUtils = fsUtils; this.currentNewEntry = null; this.started = false; + this.ipcServerNames = new HashSet<>(); } @Override - public void start() throws IOException { - // Create IPC server for agent proxies - startIPCServer(); + public synchronized void start() throws IOException { this.started = true; } @Override - public boolean isStarted() { + public synchronized boolean isStarted() { return started; } - private void startIPCServer() throws IOException { - // IPC server may have been left behind - deleteServerIfExists(); - ipcService.createServer(IPC_SERVER_NAME, this); - } - @Override - public void shutdown() throws IOException { - deleteServerIfExists(); + public synchronized void shutdown() throws IOException { this.started = false; + + // Delete all IPC servers created by this class + Set<String> serverNames = new HashSet<>(ipcServerNames); + for (String serverName : serverNames) { + deleteServerIfExists(serverName); + ipcServerNames.remove(serverName); + } } - private void deleteServerIfExists() throws IOException { - if (ipcService.serverExists(IPC_SERVER_NAME)) { - ipcService.destroyServer(IPC_SERVER_NAME); + private void deleteServerIfExists(String serverName) throws IOException { + if (ipcService.serverExists(serverName)) { + ipcService.destroyServer(serverName); } } @@ -191,6 +200,7 @@ @Override public synchronized MXBeanConnection acquire(int pid) throws MXBeanConnectionException { + checkRunning(); MXBeanConnectionPoolEntry data = pool.get(pid); if (data == null) { MXBeanConnector connector = null; @@ -199,7 +209,14 @@ if (username == null) { throw new MXBeanConnectionException("Unable to determine owner of " + pid); } + // Create an Agent Proxy IPC server for this user if it does not already exist + String serverName = IPC_SERVER_PREFIX + "-" + String.valueOf(info.getUid()); try { + // Check if we created an IPC server for this user already + if (!ipcServerNames.contains(serverName)) { + createIPCServer(username, serverName); + } + data = new MXBeanConnectionPoolEntry(pid); // Synchronized to ensure any previous callback has completely finished // before changing currentNewEntry @@ -210,7 +227,7 @@ pool.put(pid, data); // Start agent proxy which will send the JMX service URL to the IPC server we created - AgentProxyClient proxy = creator.createAgentProxy(pid, username, binPath, ipcConfigFile); + AgentProxyClient proxy = creator.createAgentProxy(pid, username, binPath, ipcConfigFile, serverName); proxy.runProcess(); // Process completed when this returns // Block until we get a JMX service URL, or Exception @@ -237,8 +254,24 @@ return data.getConnection(); } + private void createIPCServer(String username, String serverName) throws IOException { + // Lookup UserPrincipal using username + UserPrincipalLookupService lookup = fsUtils.getUserPrincipalLookupService(); + UserPrincipal principal = lookup.lookupPrincipalByName(username); + deleteServerIfExists(serverName); // Chance of old server left behind + ipcService.createServer(serverName, this, principal); + ipcServerNames.add(serverName); + } + + private void checkRunning() throws MXBeanConnectionException { + if (!started) { + throw new MXBeanConnectionException(MXBeanConnectionPool.class.getSimpleName() + " service is not running"); + } + } + @Override public synchronized void release(int pid, MXBeanConnection toRelease) throws MXBeanConnectionException { + checkRunning(); MXBeanConnectionPoolEntry data = pool.get(pid); if (data == null) { throw new MXBeanConnectionException("Unknown pid: " + pid); @@ -263,8 +296,8 @@ } static class ConnectorCreator { - AgentProxyClient createAgentProxy(int pid, String user, File binPath, File ipcConfigFile) { - return new AgentProxyClient(pid, user, binPath, ipcConfigFile); + AgentProxyClient createAgentProxy(int pid, String user, File binPath, File ipcConfigFile, String serverName) { + return new AgentProxyClient(pid, user, binPath, ipcConfigFile, serverName); } MXBeanConnector createConnector(String jmxUrl) throws IOException { @@ -273,10 +306,21 @@ } } + static class FileSystemUtils { + UserPrincipalLookupService getUserPrincipalLookupService() { + return FileSystems.getDefault().getUserPrincipalLookupService(); + } + } + // For testing purposes MXBeanConnectionPoolEntry getPoolEntry(int pid) { return pool.get(pid); } + // For testing purposes + synchronized Set<String> getIPCServerNames() { + return ipcServerNames; + } + }
--- a/agent/core/src/test/java/com/redhat/thermostat/utils/management/internal/AgentProxyClientTest.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/core/src/test/java/com/redhat/thermostat/utils/management/internal/AgentProxyClientTest.java Tue Nov 08 15:09:04 2016 -0500 @@ -56,7 +56,9 @@ import com.redhat.thermostat.utils.management.internal.AgentProxyClient.ProcessCreator; public class AgentProxyClientTest { - + + private static final String SERVER_NAME = "agent-proxy-2000"; + private AgentProxyClient client; private String user; private File binPath; @@ -72,7 +74,7 @@ procCreator = mock(ProcessCreator.class); proxy = mock(Process.class); when(procCreator.startProcess(any(ProcessBuilder.class))).thenReturn(proxy); - client = new AgentProxyClient(9000, user, binPath, ipcConfigFile, procCreator); + client = new AgentProxyClient(9000, user, binPath, ipcConfigFile, SERVER_NAME, procCreator); } @Test @@ -91,7 +93,7 @@ // Check process arguments List<String> args = builder.command(); - assertEquals(4, args.size()); + assertEquals(5, args.size()); final String arg0 = TestUtils.convertWinPathToUnixPath(args.get(0)); final String arg3 = TestUtils.convertWinPathToUnixPath(args.get(3)); @@ -100,6 +102,7 @@ assertEquals("9000", args.get(1)); assertEquals("Hello", args.get(2)); assertEquals("/path/to/ipc/config", arg3); + assertEquals(SERVER_NAME, args.get(4)); // Check cleanup verify(proxy).waitFor();
--- a/agent/core/src/test/java/com/redhat/thermostat/utils/management/internal/MXBeanConnectionPoolImplTest.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/core/src/test/java/com/redhat/thermostat/utils/management/internal/MXBeanConnectionPoolImplTest.java Tue Nov 08 15:09:04 2016 -0500 @@ -41,7 +41,6 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; -import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -51,6 +50,8 @@ import java.io.File; import java.nio.ByteBuffer; import java.nio.charset.Charset; +import java.nio.file.attribute.UserPrincipal; +import java.nio.file.attribute.UserPrincipalLookupService; import org.junit.Before; import org.junit.Test; @@ -65,9 +66,12 @@ import com.redhat.thermostat.agent.utils.management.MXBeanConnection; import com.redhat.thermostat.agent.utils.management.MXBeanConnectionException; import com.redhat.thermostat.utils.management.internal.MXBeanConnectionPoolImpl.ConnectorCreator; +import com.redhat.thermostat.utils.management.internal.MXBeanConnectionPoolImpl.FileSystemUtils; import com.redhat.thermostat.utils.management.internal.ProcessUserInfoBuilder.ProcessUserInfo; public class MXBeanConnectionPoolImplTest { + + private static final String IPC_SERVER_NAME = "agent-proxy-8000"; private File binDir; private AgentIPCService ipcService; @@ -77,6 +81,11 @@ private ConnectorCreator creator; private MXBeanConnector connector; private File ipcConfigFile; + private FileSystemUtils fsUtils; + private UserPrincipalLookupService lookup; + private UserPrincipal principal; + + private ProcessUserInfoBuilder builder; @Before public void setup() throws Exception { @@ -86,16 +95,22 @@ connector = mock(MXBeanConnector.class); creator = mock(ConnectorCreator.class); ipcConfigFile = mock(File.class); + fsUtils = mock(FileSystemUtils.class); proxy = mock(AgentProxyClient.class); - when(creator.createConnector(any(String.class))).thenReturn(connector); + when(creator.createConnector("jmxUrl://hello")).thenReturn(connector); when(connector.connect()).thenReturn(connection); - ProcessUserInfoBuilder builder = mock(ProcessUserInfoBuilder.class); + builder = mock(ProcessUserInfoBuilder.class); ProcessUserInfo info = new ProcessUserInfo(8000, "Test"); when(builder.build(8000)).thenReturn(info); + + lookup = mock(UserPrincipalLookupService.class); + when(fsUtils.getUserPrincipalLookupService()).thenReturn(lookup); + principal = mock(UserPrincipal.class); + when(lookup.lookupPrincipalByName("Test")).thenReturn(principal); - pool = new MXBeanConnectionPoolImpl(creator, binDir, builder, ipcService, ipcConfigFile); + pool = new MXBeanConnectionPoolImpl(creator, binDir, builder, ipcService, ipcConfigFile, fsUtils); } @Test @@ -103,25 +118,9 @@ assertFalse(pool.isStarted()); pool.start(); assertTrue(pool.isStarted()); - verify(ipcService).createServer(MXBeanConnectionPoolImpl.IPC_SERVER_NAME, pool); } @Test - public void testStartCleanup() throws Exception { - pool.start(); - verify(ipcService).serverExists(MXBeanConnectionPoolImpl.IPC_SERVER_NAME); - verify(ipcService, never()).destroyServer(MXBeanConnectionPoolImpl.IPC_SERVER_NAME); - } - - @Test - public void testStartCleanupServerExists() throws Exception { - when(ipcService.serverExists(MXBeanConnectionPoolImpl.IPC_SERVER_NAME)).thenReturn(true); - pool.start(); - verify(ipcService).serverExists(MXBeanConnectionPoolImpl.IPC_SERVER_NAME); - verify(ipcService).destroyServer(MXBeanConnectionPoolImpl.IPC_SERVER_NAME); - } - - @Test public void testShutdown() throws Exception { pool.start(); assertTrue(pool.isStarted()); @@ -132,17 +131,37 @@ @Test public void testShutdownCleanup() throws Exception { + pool.getIPCServerNames().add(IPC_SERVER_NAME); pool.shutdown(); - verify(ipcService).serverExists(MXBeanConnectionPoolImpl.IPC_SERVER_NAME); - verify(ipcService, never()).destroyServer(MXBeanConnectionPoolImpl.IPC_SERVER_NAME); + verify(ipcService).serverExists(IPC_SERVER_NAME); + verify(ipcService, never()).destroyServer(IPC_SERVER_NAME); + assertTrue(pool.getIPCServerNames().isEmpty()); } @Test public void testShutdownCleanupServerExists() throws Exception { - when(ipcService.serverExists(MXBeanConnectionPoolImpl.IPC_SERVER_NAME)).thenReturn(true); + pool.getIPCServerNames().add(IPC_SERVER_NAME); + when(ipcService.serverExists(IPC_SERVER_NAME)).thenReturn(true); pool.shutdown(); - verify(ipcService).serverExists(MXBeanConnectionPoolImpl.IPC_SERVER_NAME); - verify(ipcService).destroyServer(MXBeanConnectionPoolImpl.IPC_SERVER_NAME); + verify(ipcService).serverExists(IPC_SERVER_NAME); + verify(ipcService).destroyServer(IPC_SERVER_NAME); + assertTrue(pool.getIPCServerNames().isEmpty()); + } + + @Test + public void testShutdownCleanupMultipleServers() throws Exception { + pool.getIPCServerNames().add(IPC_SERVER_NAME); + pool.getIPCServerNames().add("agent-proxy-1001"); + + when(ipcService.serverExists(IPC_SERVER_NAME)).thenReturn(true); + when(ipcService.serverExists("agent-proxy-1001")).thenReturn(true); + + pool.shutdown(); + verify(ipcService).serverExists(IPC_SERVER_NAME); + verify(ipcService).destroyServer(IPC_SERVER_NAME); + verify(ipcService).serverExists("agent-proxy-1001"); + verify(ipcService).destroyServer("agent-proxy-1001"); + assertTrue(pool.getIPCServerNames().isEmpty()); } @Test @@ -150,8 +169,13 @@ final byte[] data = getJsonString(8000, "jmxUrl://hello"); invokeCallbacksOnProxyCreation(data); + pool.start(); MXBeanConnection result = pool.acquire(8000); + verify(lookup).lookupPrincipalByName("Test"); + verify(ipcService).serverExists(IPC_SERVER_NAME); + verify(ipcService, never()).destroyServer(IPC_SERVER_NAME); + verify(ipcService).createServer(IPC_SERVER_NAME, pool, principal); verify(creator).createConnector("jmxUrl://hello"); assertNotNull(result); @@ -161,7 +185,11 @@ } private void invokeCallbacksOnProxyCreation(final byte[] data) { - when(creator.createAgentProxy(8000, "Test", binDir, ipcConfigFile)).thenAnswer(new Answer<AgentProxyClient>() { + invokeCallbacksOnProxyCreation(data, 8000, "Test", IPC_SERVER_NAME); + } + + private void invokeCallbacksOnProxyCreation(final byte[] data, int pid, String username, String ipcServerName) { + when(creator.createAgentProxy(pid, username, binDir, ipcConfigFile, ipcServerName)).thenAnswer(new Answer<AgentProxyClient>() { @Override public AgentProxyClient answer(InvocationOnMock invocation) throws Throwable { // Invoke callback @@ -273,29 +301,107 @@ verify(connector, never()).connect(); } } + + @Test + public void testAcquireNotRunning() throws Exception { + try { + pool.acquire(8000); + fail("Expected MXBeanConnectionException"); + } catch (MXBeanConnectionException e) { + verify(creator, never()).createConnector("jmxUrl://hello"); + verify(connector, never()).connect(); + } + } + + @Test + public void testAcquireOldServer() throws Exception { + final byte[] data = getJsonString(8000, "jmxUrl://hello"); + invokeCallbacksOnProxyCreation(data); + when(ipcService.serverExists(IPC_SERVER_NAME)).thenReturn(true); + + pool.start(); + MXBeanConnection result = pool.acquire(8000); + + verify(lookup).lookupPrincipalByName("Test"); + verify(ipcService).serverExists(IPC_SERVER_NAME); + verify(ipcService).destroyServer(IPC_SERVER_NAME); + verify(ipcService).createServer(IPC_SERVER_NAME, pool, principal); + verify(creator).createConnector("jmxUrl://hello"); + + assertNotNull(result); + assertEquals(result, connection); + + verify(connector).connect(); + } @Test - public void testAcquireTwice() throws Exception { + public void testAcquireTwiceSameUser() throws Exception { byte[] data = getJsonString(8000, "jmxUrl://hello"); invokeCallbacksOnProxyCreation(data); + pool.start(); MXBeanConnection connection1 = pool.acquire(8000); verify(connector).connect(); MXBeanConnection connection2 = pool.acquire(8000); + // Should only be invoked once + verify(lookup).lookupPrincipalByName("Test"); + verify(ipcService).serverExists(IPC_SERVER_NAME); + verify(ipcService, never()).destroyServer(IPC_SERVER_NAME); + verify(ipcService).createServer(IPC_SERVER_NAME, pool, principal); + assertEquals(connection1, connection); assertEquals(connection2, connection); verifyNoMoreInteractions(connector); } + + @Test + public void testAcquireTwiceDifferentUser() throws Exception { + byte[] data = getJsonString(8000, "jmxUrl://hello"); + invokeCallbacksOnProxyCreation(data); + data = getJsonString(8001, "jmxUrl://hello1"); + invokeCallbacksOnProxyCreation(data, 8001, "Test1", "agent-proxy-1001"); + + ProcessUserInfo info = new ProcessUserInfo(1001, "Test1"); + when(builder.build(8001)).thenReturn(info); + UserPrincipal otherPrincipal = mock(UserPrincipal.class); + when(lookup.lookupPrincipalByName("Test1")).thenReturn(otherPrincipal); + MXBeanConnector otherConnector = mock(MXBeanConnector.class); + when(creator.createConnector("jmxUrl://hello1")).thenReturn(otherConnector); + MXBeanConnectionImpl otherConnection = mock(MXBeanConnectionImpl.class); + when(otherConnector.connect()).thenReturn(otherConnection); + + pool.start(); + MXBeanConnection connection1 = pool.acquire(8000); + MXBeanConnection connection2 = pool.acquire(8001); + + verify(lookup).lookupPrincipalByName("Test"); + verify(ipcService).serverExists(IPC_SERVER_NAME); + verify(ipcService, never()).destroyServer(IPC_SERVER_NAME); + verify(ipcService).createServer(IPC_SERVER_NAME, pool, principal); + verify(creator).createConnector("jmxUrl://hello"); + verify(connector).connect(); + + verify(lookup).lookupPrincipalByName("Test1"); + verify(ipcService).serverExists("agent-proxy-1001"); + verify(ipcService, never()).destroyServer("agent-proxy-1001"); + verify(ipcService).createServer("agent-proxy-1001", pool, otherPrincipal); + verify(creator).createConnector("jmxUrl://hello1"); + verify(otherConnector).connect(); + + assertEquals(connection1, connection); + assertEquals(connection2, otherConnection); + } @Test public void testRelease() throws Exception { byte[] data = getJsonString(8000, "jmxUrl://hello"); invokeCallbacksOnProxyCreation(data); + pool.start(); MXBeanConnection result = pool.acquire(8000); verify(connection, never()).close(); @@ -310,6 +416,7 @@ byte[] data = getJsonString(8000, "jmxUrl://hello"); invokeCallbacksOnProxyCreation(data); + pool.start(); // connection1 == connection1 == actualConnection MXBeanConnection connection1 = pool.acquire(8000); MXBeanConnection connection2 = pool.acquire(8000); @@ -321,7 +428,11 @@ pool.release(8000, connection2); verify(connection).close(); + } + @Test(expected=MXBeanConnectionException.class) + public void testReleaseNotRunning() throws Exception { + pool.release(8000, connection); } private byte[] getJsonString(Integer pid, String jmxUrl) {
--- a/agent/ipc/server/src/main/java/com/redhat/thermostat/agent/ipc/server/AgentIPCService.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/ipc/server/src/main/java/com/redhat/thermostat/agent/ipc/server/AgentIPCService.java Tue Nov 08 15:09:04 2016 -0500 @@ -37,6 +37,7 @@ package com.redhat.thermostat.agent.ipc.server; import java.io.IOException; +import java.nio.file.attribute.UserPrincipal; import com.redhat.thermostat.annotations.Service; @@ -51,6 +52,7 @@ /** * Creates an IPC server identified by the provided name, which can be connected to * by IPC clients. The caller will be notified when this server receives data. + * The created server will be owned by the user running the agent. * <p> * The server name must contain only letters, numbers, hyphens, and underscores. * @param name - A unique name for this IPC server, must not be null @@ -60,6 +62,19 @@ void createServer(String name, ThermostatIPCCallbacks callbacks) throws IOException; /** + * Creates an IPC server identified by the provided name, which can be connected to + * by IPC clients. The caller will be notified when this server receives data. + * The created server will be owned by the user specified by the supplied {@link UserPrincipal}. + * <p> + * The server name must contain only letters, numbers, hyphens, and underscores. + * @param name - A unique name for this IPC server, must not be null + * @param callbacks - Object to be notified when data is received, must not be null + * @param owner - principal representing the intended owner of the server + * @throws IOException if this IPC server cannot be created for any reason + */ + void createServer(String name, ThermostatIPCCallbacks callbacks, UserPrincipal owner) throws IOException; + + /** * Check if an IPC server exists with a given name. * @param name - name of server, must not be null * @return whether the named server exists
--- a/agent/ipc/server/src/main/java/com/redhat/thermostat/agent/ipc/server/internal/AgentIPCServiceImpl.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/ipc/server/src/main/java/com/redhat/thermostat/agent/ipc/server/internal/AgentIPCServiceImpl.java Tue Nov 08 15:09:04 2016 -0500 @@ -38,6 +38,7 @@ import java.io.File; import java.io.IOException; +import java.nio.file.attribute.UserPrincipal; import java.util.HashMap; import java.util.Map; @@ -112,6 +113,15 @@ } transport.createServer(name, callbacks); } + + @Override + public synchronized void createServer(String name, ThermostatIPCCallbacks callbacks, UserPrincipal owner) throws IOException { + // Start the service if not already started + if (!started) { + startService(); + } + transport.createServer(name, callbacks, owner); + } @Override public synchronized boolean serverExists(String name) throws IOException {
--- a/agent/ipc/server/src/test/java/com/redhat/thermostat/agent/ipc/server/internal/AgentIPCServiceImplTest.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/ipc/server/src/test/java/com/redhat/thermostat/agent/ipc/server/internal/AgentIPCServiceImplTest.java Tue Nov 08 15:09:04 2016 -0500 @@ -47,6 +47,7 @@ import java.io.File; import java.io.IOException; +import java.nio.file.attribute.UserPrincipal; import java.util.Map; import org.junit.Before; @@ -165,7 +166,23 @@ verify(writer).write(); verify(transport, times(2)).createServer(SERVER_NAME, callbacks); } - + + @Test + public void testCreateServerWithOwner() throws Exception { + context.registerService(ServerTransport.class.getName(), transport, null); + AgentIPCServiceImpl service = createService(); + ThermostatIPCCallbacks callbacks = mock(ThermostatIPCCallbacks.class); + UserPrincipal owner = mock(UserPrincipal.class); + + assertFalse(service.isStarted()); + service.createServer(SERVER_NAME, callbacks, owner); + assertTrue(service.isStarted()); + + verify(transport).start(props); + verify(writer).write(); + verify(transport).createServer(SERVER_NAME, callbacks, owner); + } + @Test public void testServerExists() throws Exception { context.registerService(ServerTransport.class.getName(), transport, null);
--- a/agent/ipc/tcp-socket/server/src/main/java/com/redhat/thermostat/agent/ipc/tcpsocket/server/internal/TcpSocketServerTransport.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/ipc/tcp-socket/server/src/main/java/com/redhat/thermostat/agent/ipc/tcpsocket/server/internal/TcpSocketServerTransport.java Tue Nov 08 15:09:04 2016 -0500 @@ -36,7 +36,6 @@ package com.redhat.thermostat.agent.ipc.tcpsocket.server.internal; -import java.io.File; import java.io.IOException; import java.net.SocketAddress; import java.nio.channels.Selector; @@ -44,16 +43,13 @@ import java.nio.file.DirectoryStream; import java.nio.file.FileSystems; import java.nio.file.Files; -import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.attribute.FileAttribute; import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; import java.nio.file.attribute.UserPrincipal; import java.nio.file.attribute.UserPrincipalLookupService; -import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -151,6 +147,12 @@ } @Override + public void createServer(String name, ThermostatIPCCallbacks callbacks, UserPrincipal owner) throws IOException { + // UserPrincipal is unused in TCP implementation + createServer(name, callbacks); + } + + @Override public synchronized boolean serverExists(String name) throws IOException { return sockets.containsKey(name); }
--- a/agent/ipc/unix-socket/client/src/main/java/com/redhat/thermostat/agent/ipc/unixsocket/client/internal/UnixSocketTransportImpl.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/ipc/unix-socket/client/src/main/java/com/redhat/thermostat/agent/ipc/unixsocket/client/internal/UnixSocketTransportImpl.java Tue Nov 08 15:09:04 2016 -0500 @@ -47,9 +47,6 @@ public class UnixSocketTransportImpl implements ClientTransport { - // Filename prefix for socket file - static final String SOCKET_PREFIX = "sock-"; - private final UnixSocketIPCProperties socketProps; private final SocketHelper sockHelper; @@ -78,7 +75,10 @@ if (!socketDir.exists()) { throw new IOException("Server address is invalid"); } - File socketFile = sockHelper.getSocketFile(socketDir, SOCKET_PREFIX + name); + + // Get subdirectory for current user + String username = sockHelper.getUsername(); + File socketFile = socketProps.getSocketFile(name, username); if (!socketFile.exists()) { throw new IOException("IPC server with name \"" + name + "\" does not exist"); } @@ -102,9 +102,13 @@ return new UnixSocketMessageChannel(sockChannel); } - File getSocketFile(File socketDir, String name) { + File getFile(File socketDir, String name) { return new File(socketDir, name); } + + String getUsername() { + return System.getProperty("user.name"); + } } }
--- a/agent/ipc/unix-socket/client/src/test/java/com/redhat/thermostat/agent/ipc/unixsocket/client/internal/UnixSocketTransportImplTest.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/ipc/unix-socket/client/src/test/java/com/redhat/thermostat/agent/ipc/unixsocket/client/internal/UnixSocketTransportImplTest.java Tue Nov 08 15:09:04 2016 -0500 @@ -59,6 +59,7 @@ public class UnixSocketTransportImplTest { private static final String SERVER_NAME = "test"; + private static final String USERNAME = "testUser"; private File socketDir; private File socketFile; private SocketHelper sockHelper; @@ -76,10 +77,11 @@ ThermostatLocalSocketChannelImpl sockChannel = mock(ThermostatLocalSocketChannelImpl.class); when(sockHelper.openSocketChannel(eq(SERVER_NAME), eq(socketFile))).thenReturn(sockChannel); when(sockHelper.createMessageChannel(sockChannel)).thenReturn(messageChannel); - when(sockHelper.getSocketFile(socketDir, UnixSocketTransportImpl.SOCKET_PREFIX + SERVER_NAME)).thenReturn(socketFile); + when(sockHelper.getUsername()).thenReturn(USERNAME); props = mock(UnixSocketIPCProperties.class); when(props.getSocketDirectory()).thenReturn(socketDir); + when(props.getSocketFile(SERVER_NAME, USERNAME)).thenReturn(socketFile); } @Test
--- a/agent/ipc/unix-socket/common/src/main/java/com/redhat/thermostat/agent/ipc/unixsocket/common/internal/UnixSocketIPCProperties.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/ipc/unix-socket/common/src/main/java/com/redhat/thermostat/agent/ipc/unixsocket/common/internal/UnixSocketIPCProperties.java Tue Nov 08 15:09:04 2016 -0500 @@ -45,6 +45,8 @@ public class UnixSocketIPCProperties extends IPCProperties { + // Filename prefix for socket file + static final String SOCKET_PREFIX = "sock-"; static final String PROP_UNIX_SOCKET_DIR = "unixsocket.dir"; private static final String SOCKET_DIR_NAME = "thermostat-socks"; @@ -72,6 +74,12 @@ return sockDir; } + public File getSocketFile(String serverName, String ownerName) { + File ownerDir = new File(sockDir, ownerName); + String socketFilename = SOCKET_PREFIX.concat(serverName); + return new File(ownerDir, socketFilename); + } + /* * Default socket directory is calculated using the first available from the following: * 1. Environment variable "$XDG_RUNTIME_DIR" (e.g. /run/user/1000/)
--- a/agent/ipc/unix-socket/server/src/main/java/com/redhat/thermostat/agent/ipc/unixsocket/server/internal/UnixSocketServerTransport.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/ipc/unix-socket/server/src/main/java/com/redhat/thermostat/agent/ipc/unixsocket/server/internal/UnixSocketServerTransport.java Tue Nov 08 15:09:04 2016 -0500 @@ -40,17 +40,22 @@ import java.io.IOException; import java.nio.channels.Selector; import java.nio.channels.spi.SelectorProvider; -import java.nio.file.DirectoryStream; import java.nio.file.FileSystems; +import java.nio.file.FileVisitOption; +import java.nio.file.FileVisitResult; +import java.nio.file.FileVisitor; import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileAttribute; import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; import java.nio.file.attribute.UserPrincipal; import java.nio.file.attribute.UserPrincipalLookupService; import java.util.Collections; +import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -72,17 +77,28 @@ class UnixSocketServerTransport implements ServerTransport { private static final Logger logger = LoggingUtils.getLogger(UnixSocketServerTransport.class); - // Filename prefix for socket file - static final String SOCKET_PREFIX = "sock-"; - // Permissions to allow only the owner and group access to the directory + // Permissions for the top-level sockets directory private static final Set<PosixFilePermission> SOCKET_DIR_PERM; static { Set<PosixFilePermission> perms = new HashSet<>(); perms.add(PosixFilePermission.OWNER_READ); perms.add(PosixFilePermission.OWNER_WRITE); perms.add(PosixFilePermission.OWNER_EXECUTE); + perms.add(PosixFilePermission.GROUP_READ); + perms.add(PosixFilePermission.GROUP_EXECUTE); + perms.add(PosixFilePermission.OTHERS_READ); + perms.add(PosixFilePermission.OTHERS_EXECUTE); SOCKET_DIR_PERM = Collections.unmodifiableSet(perms); } + // Permissions for the user-specific socket directories, under the top-level directory + private static final Set<PosixFilePermission> OWNER_DIR_PERM; + static { + Set<PosixFilePermission> perms = new HashSet<>(); + perms.add(PosixFilePermission.OWNER_READ); + perms.add(PosixFilePermission.OWNER_WRITE); + perms.add(PosixFilePermission.OWNER_EXECUTE); + OWNER_DIR_PERM = Collections.unmodifiableSet(perms); + } private final SelectorProvider selectorProvider; // Access/modification of this field should by synchronized @@ -97,6 +113,7 @@ private AcceptThread acceptThread; private Selector selector; private Path socketDir; + private UserPrincipal currentUser; UnixSocketServerTransport(SelectorProvider selectorProvider) { this(selectorProvider, Executors.newFixedThreadPool(determineDefaultThreadPoolSize(), new CountingThreadFactory()), @@ -121,6 +138,10 @@ throw new IOException("Unsupported IPC type: " + type.getConfigValue()); } this.props = (UnixSocketIPCProperties) props; + + // Get UserPrincipal for currently logged-in user + this.currentUser = getCurrentUser(); + // Prepare socket directory with strict permissions, which will contain the socket file when bound File sockDirFile = ((UnixSocketIPCProperties) props).getSocketDirectory(); this.socketDir = createSocketDirPath(sockDirFile); @@ -147,9 +168,10 @@ } } - private Path getPathToServer(String name) throws IOException { + private Path getPathToServer(String name, UserPrincipal owner) throws IOException { checkName(name); - return socketDir.resolve(SOCKET_PREFIX + name); + String ownerName = owner.getName(); + return props.getSocketFile(name, ownerName).toPath(); } private void checkName(String name) throws IOException { @@ -166,31 +188,49 @@ @Override public synchronized void createServer(String name, ThermostatIPCCallbacks callbacks) throws IOException { + createServer(name, callbacks, currentUser); + } + + public synchronized void createServer(String name, ThermostatIPCCallbacks callbacks, UserPrincipal owner) throws IOException { // Check if the socket has already been created and we know about it if (sockets.containsKey(name)) { throw new IOException("IPC server with name \"" + name + "\" already exists"); } // Check for existing socket - Path socketPath = getPathToServer(name); + Path socketPath = getPathToServer(name, owner); if (fileUtils.exists(socketPath)) { // Must have been left behind, so delete before attempting to bind fileUtils.delete(socketPath); } // Check that socket directory permissions haven't changed - if (!permissionsMatch(socketDir)) { + if (!permissionsMatch(socketDir, SOCKET_DIR_PERM)) { throw new IOException("Socket directory permissions are insecure"); } checkOwner(socketDir, "Socket directory"); + // Check if owner subdirectory exists, create it if not + String ownerName = owner.getName(); // TODO uid would be better for directory name to keep paths short + Path ownerDir = socketDir.resolve(ownerName); + if (!fileUtils.exists(ownerDir)) { + fileUtils.createDirectory(ownerDir, fileUtils.toFileAttribute(OWNER_DIR_PERM)); + fileUtils.setOwner(ownerDir, owner); + } + // Check the permissions are what we expect + if (!permissionsMatch(ownerDir, OWNER_DIR_PERM)) { + throw new IOException("Socket directory permissions are insecure for user: " + ownerName); + } + checkOwner(ownerDir, owner, "User-specific socket directory"); + // Create socket ThermostatLocalServerSocketChannelImpl socket = channelUtils.createServerSocketChannel(name, socketPath, callbacks, props, selector); // Verify owner of new socket file File socketFile = socket.getSocketFile(); - checkOwner(socketFile.toPath(), "Socket file " + socketFile.getName()); + fileUtils.setOwner(socketFile.toPath(), owner); + checkOwner(socketFile.toPath(), owner, "Socket file " + socketFile.getName()); sockets.put(name, socket); } @@ -200,7 +240,7 @@ prepareSocketDir(socketDir); } else if (!fileUtils.isDirectory(socketDir)) { throw new IOException("Socket directory exists, but is not a directory"); - } else if (!permissionsMatch(socketDir)) { + } else if (!permissionsMatch(socketDir, SOCKET_DIR_PERM)) { throw new IOException("Socket directory has incorrect permissions"); } // else -> socket directory exists and is valid @@ -209,29 +249,36 @@ logger.fine("Using Unix socket directory: " + socketDir.toString()); } - private boolean permissionsMatch(Path path) throws IOException { - Set<PosixFilePermission> acutalPerms = fileUtils.getPosixFilePermissions(path); - return SOCKET_DIR_PERM.equals(acutalPerms); + private boolean permissionsMatch(Path path, Set<PosixFilePermission> permissions) throws IOException { + Set<PosixFilePermission> actualPerms = fileUtils.getPosixFilePermissions(path); + return permissions.equals(actualPerms); } private void checkOwner(Path path, String errorMessagePrefix) throws IOException { + checkOwner(path, currentUser, errorMessagePrefix); + } + + private void checkOwner(Path path, UserPrincipal expectedOwner, String errorMessagePrefix) throws IOException { + try { + UserPrincipal owner = fileUtils.getOwner(path); + if (owner == null) { + throw new IOException("Unable to determine owner for path: " + path.toString()); + } else if (!owner.equals(expectedOwner)) { + throw new IOException(errorMessagePrefix + " insecure with owner: " + owner.getName()); + } + } catch (UnsupportedOperationException e) { + throw new IOException("Cannot determine owner from file system", e); + } + } + + private UserPrincipal getCurrentUser() throws IOException { String username = fileUtils.getUsername(); UserPrincipalLookupService lookup = fileUtils.getUserPrincipalLookupService(); UserPrincipal principal = lookup.lookupPrincipalByName(username); if (principal == null) { throw new IOException("No Principal found for user: " + username); } - - try { - UserPrincipal owner = fileUtils.getOwner(path); - if (owner == null) { - throw new IOException("Unable to determine owner for path: " + path.toString()); - } else if (!owner.equals(principal)) { - throw new IOException(errorMessagePrefix + " insecure with owner: " + owner.getName()); - } - } catch (UnsupportedOperationException e) { - throw new IOException("Cannot determine owner from file system", e); - } + return principal; } private void prepareSocketDir(Path path) throws IOException { @@ -246,13 +293,26 @@ private void deleteSocketDir(Path path) throws IOException { if (fileUtils.exists(path)) { - DirectoryStream<Path> entries = fileUtils.newDirectoryStream(path); - // Empty directory - for (Path entry : entries) { - fileUtils.delete(entry); - } - // Delete directory - fileUtils.delete(path); + // Empty sockets directory + int maxDepth = 2; // top-level socket directory + user-specific subdirectories + fileUtils.walkFileTree(path, EnumSet.noneOf(FileVisitOption.class), maxDepth, new SimpleFileVisitor<Path>() { + @Override + public FileVisitResult visitFile(Path file, + BasicFileAttributes attrs) throws IOException { + fileUtils.delete(file); + return FileVisitResult.CONTINUE; + } + @Override + public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + if (exc == null) { + fileUtils.delete(dir); + return FileVisitResult.CONTINUE; + } else { + // Exception occurred, abort cleanup + throw exc; + } + } + }); } } @@ -355,8 +415,9 @@ return PosixFilePermissions.asFileAttribute(perms); } - DirectoryStream<Path> newDirectoryStream(Path dir) throws IOException { - return Files.newDirectoryStream(dir); + Path walkFileTree(Path start, Set<FileVisitOption> options, int maxDepth, + FileVisitor<? super Path> visitor) throws IOException { + return Files.walkFileTree(start, options, maxDepth, visitor); } UserPrincipalLookupService getUserPrincipalLookupService() { @@ -371,6 +432,9 @@ return Files.getOwner(path); } + Path setOwner(Path path, UserPrincipal owner) throws IOException { + return Files.setOwner(path, owner); + } } /* For testing purposes */
--- a/agent/ipc/unix-socket/server/src/test/java/com/redhat/thermostat/agent/ipc/unixsocket/server/internal/UnixSocketServerTransportTest.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/ipc/unix-socket/server/src/test/java/com/redhat/thermostat/agent/ipc/unixsocket/server/internal/UnixSocketServerTransportTest.java Tue Nov 08 15:09:04 2016 -0500 @@ -42,17 +42,20 @@ import static org.junit.Assert.fail; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anySetOf; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.times; import java.io.File; import java.io.IOException; import java.nio.channels.spi.AbstractSelector; import java.nio.channels.spi.SelectorProvider; -import java.nio.file.DirectoryStream; +import java.nio.file.FileVisitOption; +import java.nio.file.FileVisitor; import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.attribute.FileAttribute; @@ -60,13 +63,14 @@ import java.nio.file.attribute.PosixFilePermissions; import java.nio.file.attribute.UserPrincipal; import java.nio.file.attribute.UserPrincipalLookupService; -import java.util.Iterator; import java.util.Set; import java.util.concurrent.ExecutorService; import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentCaptor; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; import com.redhat.thermostat.agent.ipc.common.internal.IPCProperties; import com.redhat.thermostat.agent.ipc.common.internal.IPCType; @@ -88,6 +92,7 @@ private FilenameValidator validator; private FileUtils fileUtils; private Path socketDirPath; + private Path ownerDirPath; private AcceptThread acceptThread; private ThreadCreator threadCreator; private FileAttribute<Set<PosixFilePermission>> fileAttr; @@ -97,6 +102,7 @@ private ThermostatLocalServerSocketChannelImpl channel; private UnixSocketIPCProperties props; private UserPrincipalLookupService lookup; + private UserPrincipal currentUser; @SuppressWarnings("unchecked") @Before @@ -112,12 +118,17 @@ when(socketDirPath.toAbsolutePath()).thenReturn(socketDirPath); when(socketDirPath.normalize()).thenReturn(socketDirPath); when(sockDirFile.toPath()).thenReturn(socketDirPath); + ownerDirPath = mock(Path.class); socketPath = mock(Path.class); - when(socketDirPath.resolve(UnixSocketServerTransport.SOCKET_PREFIX + SERVER_NAME)).thenReturn(socketPath); + when(socketDirPath.resolve(USERNAME)).thenReturn(ownerDirPath); + File socketFile = mock(File.class); + when(props.getSocketFile(SERVER_NAME, USERNAME)).thenReturn(socketFile); + when(socketFile.toPath()).thenReturn(socketPath); fileUtils = mock(FileUtils.class); when(fileUtils.exists(socketDirPath)).thenReturn(false); - when(fileUtils.getPosixFilePermissions(socketDirPath)).thenReturn(PosixFilePermissions.fromString("rwx------")); + when(fileUtils.getPosixFilePermissions(socketDirPath)).thenReturn(PosixFilePermissions.fromString("rwxr-xr-x")); + when(fileUtils.getPosixFilePermissions(ownerDirPath)).thenReturn(PosixFilePermissions.fromString("rwx------")); fileAttr = mock(FileAttribute.class); when(fileUtils.toFileAttribute(any(Set.class))).thenReturn(fileAttr); @@ -125,9 +136,11 @@ lookup = mock(UserPrincipalLookupService.class); when(fileUtils.getUserPrincipalLookupService()).thenReturn(lookup); when(fileUtils.getUsername()).thenReturn(USERNAME); - UserPrincipal principal = mock(UserPrincipal.class); - when(lookup.lookupPrincipalByName(USERNAME)).thenReturn(principal); - when(fileUtils.getOwner(socketDirPath)).thenReturn(principal); + currentUser = mock(UserPrincipal.class); + when(currentUser.getName()).thenReturn(USERNAME); + when(lookup.lookupPrincipalByName(USERNAME)).thenReturn(currentUser); + when(fileUtils.getOwner(socketDirPath)).thenReturn(currentUser); + when(fileUtils.getOwner(ownerDirPath)).thenReturn(currentUser); execService = mock(ExecutorService.class); validator = mock(FilenameValidator.class); @@ -139,10 +152,8 @@ channelUtils = mock(ChannelUtils.class); channel = mock(ThermostatLocalServerSocketChannelImpl.class); - File socketFile = mock(File.class); - when(socketFile.toPath()).thenReturn(socketPath); when(channel.getSocketFile()).thenReturn(socketFile); - when(fileUtils.getOwner(socketPath)).thenReturn(principal); + when(fileUtils.getOwner(socketPath)).thenReturn(currentUser); callbacks = mock(ThermostatIPCCallbacks.class); when(channelUtils.createServerSocketChannel(SERVER_NAME, socketPath, callbacks, props, selector)).thenReturn(channel); @@ -196,7 +207,7 @@ verify(fileUtils).toFileAttribute(permsCaptor.capture()); Set<PosixFilePermission> perms = (Set<PosixFilePermission>) permsCaptor.getValue(); - Set<PosixFilePermission> expectedPerms = PosixFilePermissions.fromString("rwx------"); + Set<PosixFilePermission> expectedPerms = PosixFilePermissions.fromString("rwxr-xr-x"); assertEquals(perms, expectedPerms); verify(fileUtils).createDirectory(socketDirPath, fileAttr); @@ -319,11 +330,92 @@ } } + @SuppressWarnings({ "unchecked", "rawtypes" }) @Test public void testCreateServer() throws Exception { transport.start(props); transport.createServer(SERVER_NAME, callbacks); + // Verify the user-specific socket directory is created with the proper permissions + verify(fileUtils).exists(ownerDirPath); + + ArgumentCaptor<Set> permsCaptor = ArgumentCaptor.forClass(Set.class); + verify(fileUtils, times(2)).toFileAttribute(permsCaptor.capture()); + + // First invocation is for top-level socket dir, second is for user-specific dir + Set<PosixFilePermission> perms = (Set<PosixFilePermission>) permsCaptor.getAllValues().get(1); + Set<PosixFilePermission> expectedPerms = PosixFilePermissions.fromString("rwx------"); + assertEquals(perms, expectedPerms); + + verify(fileUtils).createDirectory(ownerDirPath, fileAttr); + verify(fileUtils).getPosixFilePermissions(ownerDirPath); + verify(fileUtils).setOwner(ownerDirPath, currentUser); + + verify(fileUtils).exists(socketPath); + verify(fileUtils, never()).delete(socketPath); + + checkChannel(); + } + + @SuppressWarnings({ "unchecked", "rawtypes" }) + @Test + public void testCreateServerWithOwner() throws Exception { + String otherUsername = "otherUser"; + UserPrincipal owner = mock(UserPrincipal.class); + when(owner.getName()).thenReturn(otherUsername); + Path otherOwnerDirPath = mock(Path.class); + when(socketDirPath.resolve(otherUsername)).thenReturn(otherOwnerDirPath); + when(fileUtils.getPosixFilePermissions(otherOwnerDirPath)).thenReturn(PosixFilePermissions.fromString("rwx------")); + when(fileUtils.getOwner(otherOwnerDirPath)).thenReturn(owner); + + Path otherSocketPath = mock(Path.class); + when(fileUtils.getOwner(otherSocketPath)).thenReturn(owner); + File otherSocketFile = mock(File.class); + when(props.getSocketFile(SERVER_NAME, otherUsername)).thenReturn(otherSocketFile); + ThermostatLocalServerSocketChannelImpl otherChannel = mock(ThermostatLocalServerSocketChannelImpl.class); + when(channelUtils.createServerSocketChannel(SERVER_NAME, otherSocketPath, callbacks, props, selector)).thenReturn(otherChannel); + when(otherChannel.getSocketFile()).thenReturn(otherSocketFile); + when(otherSocketFile.toPath()).thenReturn(otherSocketPath); + + transport.start(props); + transport.createServer(SERVER_NAME, callbacks, owner); + + // Verify the user-specific socket directory is created with the proper permissions + verify(fileUtils).exists(otherOwnerDirPath); + + ArgumentCaptor<Set> permsCaptor = ArgumentCaptor.forClass(Set.class); + verify(fileUtils, times(2)).toFileAttribute(permsCaptor.capture()); + + // First invocation is for top-level socket dir, second is for user-specific dir + Set<PosixFilePermission> perms = (Set<PosixFilePermission>) permsCaptor.getAllValues().get(1); + Set<PosixFilePermission> expectedPerms = PosixFilePermissions.fromString("rwx------"); + assertEquals(perms, expectedPerms); + + verify(fileUtils).createDirectory(otherOwnerDirPath, fileAttr); + verify(fileUtils).getPosixFilePermissions(otherOwnerDirPath); + verify(fileUtils).setOwner(otherOwnerDirPath, owner); + + verify(fileUtils).exists(otherSocketPath); + verify(fileUtils, never()).delete(otherSocketPath); + + verify(channelUtils).createServerSocketChannel(SERVER_NAME, otherSocketPath, callbacks, props, selector); + verify(fileUtils).setOwner(otherSocketPath, owner); + ThermostatLocalServerSocketChannelImpl result = transport.getSockets().get(SERVER_NAME); + assertEquals(otherChannel, result); + } + + @Test + public void testCreateServerOwnerDirExists() throws Exception { + transport.start(props); + + when(fileUtils.exists(ownerDirPath)).thenReturn(true); + transport.createServer(SERVER_NAME, callbacks); + verify(fileUtils, never()).createDirectory(eq(ownerDirPath), any(FileAttribute[].class)); + verify(fileUtils, never()).setOwner(eq(ownerDirPath), any(UserPrincipal.class)); + + // Should still check permissions + verify(fileUtils).getPosixFilePermissions(ownerDirPath); + verify(fileUtils).exists(socketPath); verify(fileUtils, never()).delete(socketPath); @@ -332,6 +424,7 @@ private void checkChannel() throws IOException { verify(channelUtils).createServerSocketChannel(SERVER_NAME, socketPath, callbacks, props, selector); + verify(fileUtils).setOwner(socketPath, currentUser); ThermostatLocalServerSocketChannelImpl result = transport.getSockets().get(SERVER_NAME); assertEquals(channel, result); } @@ -363,14 +456,21 @@ } @Test(expected=IOException.class) - public void testCreateServerPermsChanged() throws Exception { + public void testCreateServerSocketDirPermsChanged() throws Exception { transport.start(props); when(fileUtils.getPosixFilePermissions(socketDirPath)).thenReturn(PosixFilePermissions.fromString("rwxrwxrwx")); transport.createServer(SERVER_NAME, callbacks); } @Test(expected=IOException.class) - public void testCreateServerOwnerChanged() throws Exception { + public void testCreateServerOwnerDirPermsChanged() throws Exception { + transport.start(props); + when(fileUtils.getPosixFilePermissions(ownerDirPath)).thenReturn(PosixFilePermissions.fromString("rwxrwxrwx")); + transport.createServer(SERVER_NAME, callbacks); + } + + @Test(expected=IOException.class) + public void testCreateServerSocketDirOwnerChanged() throws Exception { transport.start(props); UserPrincipal badPrincipal = mock(UserPrincipal.class); when(fileUtils.getOwner(socketDirPath)).thenReturn(badPrincipal); @@ -378,6 +478,14 @@ } @Test(expected=IOException.class) + public void testCreateServerOwnerDirOwnerChanged() throws Exception { + transport.start(props); + UserPrincipal badPrincipal = mock(UserPrincipal.class); + when(fileUtils.getOwner(ownerDirPath)).thenReturn(badPrincipal); + transport.createServer(SERVER_NAME, callbacks); + } + + @Test(expected=IOException.class) public void testCreateServerBadSocketOwner() throws Exception { transport.start(props); UserPrincipal badPrincipal = mock(UserPrincipal.class); @@ -421,20 +529,19 @@ } // Mock a socket directory containing a socket file + @SuppressWarnings("unchecked") private void mockSocketDirOnShutdown() throws IOException { when(fileUtils.exists(socketDirPath)).thenReturn(true); - DirectoryStream<Path> dirStream = mockDirectoryStream(socketPath); - when(fileUtils.newDirectoryStream(socketDirPath)).thenReturn(dirStream); + when(fileUtils.walkFileTree(eq(socketDirPath), anySetOf(FileVisitOption.class), eq(2), any(FileVisitor.class))).thenAnswer(new Answer<Path>() { + @Override + public Path answer(InvocationOnMock invocation) throws Throwable { + FileVisitor<? super Path> visitor = (FileVisitor<? super Path>) invocation.getArguments()[3]; + // Invoke each of the methods we override once + visitor.visitFile(socketPath, null); + visitor.postVisitDirectory(socketDirPath, null); + return socketDirPath; + } + }); } - @SuppressWarnings("unchecked") - private DirectoryStream<Path> mockDirectoryStream(Path socketFile) { - DirectoryStream<Path> dirStream = (DirectoryStream<Path>) mock(DirectoryStream.class); - Iterator<Path> iterator = mock(Iterator.class); - when(iterator.hasNext()).thenReturn(true).thenReturn(false); - when(iterator.next()).thenReturn(socketFile); - when(dirStream.iterator()).thenReturn(iterator); - return dirStream; - } - }
--- a/agent/proxy/server/src/main/java/com/redhat/thermostat/agent/proxy/server/AgentProxy.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/proxy/server/src/main/java/com/redhat/thermostat/agent/proxy/server/AgentProxy.java Tue Nov 08 15:09:04 2016 -0500 @@ -54,7 +54,6 @@ public class AgentProxy { private static final Logger logger = LoggingUtils.getLogger(AgentProxy.class); - static final String IPC_SERVER_NAME = "agent-proxy"; static final String CONFIG_FILE_PROP = "ipcConfigFile"; static final String JSON_PID = "pid"; static final String JSON_JMX_URL = "jmxUrl"; @@ -63,7 +62,7 @@ private static ClientIPCService ipcService = null; public static void main(String[] args) throws IOException { - if (args.length < 1) { + if (args.length < 2) { usage(); } @@ -84,9 +83,9 @@ } catch (NumberFormatException e) { usage(); } - + String ipcServerName = args[1]; // Connect to IPC server - IPCMessageChannel channel = ipcService.connectToServer(IPC_SERVER_NAME); + IPCMessageChannel channel = ipcService.connectToServer(ipcServerName); // Start proxy agent AgentProxyControlImpl agent = creator.create(pid); @@ -150,7 +149,7 @@ } private static void usage() { - throw new RuntimeException("usage: java " + AgentProxy.class.getName() + " <pidOfTargetJvm>"); + throw new RuntimeException("usage: java " + AgentProxy.class.getName() + " <pidOfTargetJvm> <userNameOfJvmOwner>"); } static class ControlCreator {
--- a/agent/proxy/server/src/test/java/com/redhat/thermostat/agent/proxy/server/AgentProxyTest.java Tue Nov 08 16:57:32 2016 +0100 +++ b/agent/proxy/server/src/test/java/com/redhat/thermostat/agent/proxy/server/AgentProxyTest.java Tue Nov 08 15:09:04 2016 -0500 @@ -63,6 +63,7 @@ public class AgentProxyTest { private static final String JMX_URL = "service:jmx:rmi://myHost:1099/blah"; + private static final String IPC_SERVER_NAME = "agent-proxy-8000"; private AgentProxyControlImpl control; private ClientIPCService ipcService; @@ -80,7 +81,7 @@ ipcService = mock(ClientIPCService.class); channel = mock(IPCMessageChannel.class); - when(ipcService.connectToServer(AgentProxy.IPC_SERVER_NAME)).thenReturn(channel); + when(ipcService.connectToServer(IPC_SERVER_NAME)).thenReturn(channel); AgentProxy.setIPCService(ipcService); } @@ -94,7 +95,7 @@ @Test public void testMainSuccess() throws Exception { // Invoke main with PID of 8000 - AgentProxy.main(new String[] { "8000" }); + AgentProxy.main(new String[] { "8000", IPC_SERVER_NAME }); verify(control).attach(); verify(control).getConnectorAddress(); @@ -121,7 +122,7 @@ try { // Invoke main with PID of 0 - AgentProxy.main(new String[] { "0" }); + AgentProxy.main(new String[] { "0", IPC_SERVER_NAME }); fail("Expected IOException"); } catch (IOException e) { // Should only call attach and close channel @@ -142,7 +143,7 @@ try { // Invoke main with PID of 0 - AgentProxy.main(new String[] { "0" }); + AgentProxy.main(new String[] { "0", IPC_SERVER_NAME }); fail("Expected IOException"); } catch (IOException e) { verify(control).attach(); @@ -163,7 +164,7 @@ try { // Invoke main with PID of 0 - AgentProxy.main(new String[] { "0" }); + AgentProxy.main(new String[] { "0", IPC_SERVER_NAME }); fail("Expected IOException"); } catch (IOException e) { verify(control).attach(); @@ -183,7 +184,7 @@ doThrow(new IOException()).when(control).detach(); // Invoke main with PID of 0 - AgentProxy.main(new String[] { "0" }); + AgentProxy.main(new String[] { "0", IPC_SERVER_NAME }); // All should be called, should not be fatal verify(control).attach(); @@ -201,7 +202,7 @@ doThrow(new IOException()).when(channel).close(); // Invoke main with PID of 0 - AgentProxy.main(new String[] { "0" }); + AgentProxy.main(new String[] { "0", IPC_SERVER_NAME }); // All should be called, should not be fatal verify(control).attach();
--- a/distribution/scripts/thermostat-agent-proxy Tue Nov 08 16:57:32 2016 +0100 +++ b/distribution/scripts/thermostat-agent-proxy Tue Nov 08 15:09:04 2016 -0500 @@ -36,13 +36,14 @@ # ##################################################################### # -if [ "$#" -lt 3 ]; then - echo "usage: $0 <pidOfTargetJvm> <userNameOfJvmOwner> <ipcConfigFile>" >&2 +if [ "$#" -lt 4 ]; then + echo "usage: $0 <pidOfTargetJvm> <userNameOfJvmOwner> <ipcConfigFile> <ipcServerName>" >&2 exit 1 fi TARGET_PID="$1" TARGET_USER="$2" CONFIG_FILE="$3" +IPC_SERVER_NAME="$4" # Source thermostat-ipc-client-common from same directory as this script # Defines IPC_CLASSPATH variable with JARs necessary for the IPC service @@ -76,16 +77,16 @@ CONFIG_FILE_ARG="-DipcConfigFile=`cygpath -w ${CONFIG_FILE}`" # Drop permissions, if root if [ "$(id -u)" -eq 0 ]; then - /bin/su -s /bin/bash -c "${JAVA} -cp `cygpath -w -p ${IPC_CLASSPATH}` ${CONFIG_FILE_ARG} ${LOGGING_ARGS} ${DEBUG_OPTS} ${AGENT_PROXY_CLASS} ${TARGET_PID}" "${TARGET_USER}" + /bin/su -s /bin/bash -c "${JAVA} -cp `cygpath -w -p ${IPC_CLASSPATH}` ${CONFIG_FILE_ARG} ${LOGGING_ARGS} ${DEBUG_OPTS} ${AGENT_PROXY_CLASS} ${TARGET_PID} ${IPC_SERVER_NAME}" "${TARGET_USER}" else - ${JAVA} -cp `cygpath -w -p ${IPC_CLASSPATH}` "${CONFIG_FILE_ARG}" ${DEBUG_OPTS} ${LOGGING_ARGS} ${AGENT_PROXY_CLASS} "${TARGET_PID}" + ${JAVA} -cp `cygpath -w -p ${IPC_CLASSPATH}` "${CONFIG_FILE_ARG}" ${DEBUG_OPTS} ${LOGGING_ARGS} ${AGENT_PROXY_CLASS} "${TARGET_PID}" "${IPC_SERVER_NAME}" fi else CONFIG_FILE_ARG="-DipcConfigFile=${CONFIG_FILE}" # Drop permissions, if root if [ "$(id -u)" -eq 0 ]; then - /bin/su -s /bin/bash -c "${JAVA} -cp ${IPC_CLASSPATH} ${CONFIG_FILE_ARG} ${LOGGING_ARGS} ${DEBUG_OPTS} ${AGENT_PROXY_CLASS} ${TARGET_PID}" "${TARGET_USER}" + /bin/su -s /bin/bash -c "${JAVA} -cp ${IPC_CLASSPATH} ${CONFIG_FILE_ARG} ${LOGGING_ARGS} ${DEBUG_OPTS} ${AGENT_PROXY_CLASS} ${TARGET_PID} ${IPC_SERVER_NAME}" "${TARGET_USER}" else - ${JAVA} -cp ${IPC_CLASSPATH} "${CONFIG_FILE_ARG}" ${DEBUG_OPTS} ${LOGGING_ARGS} ${AGENT_PROXY_CLASS} "${TARGET_PID}" + ${JAVA} -cp ${IPC_CLASSPATH} "${CONFIG_FILE_ARG}" ${DEBUG_OPTS} ${LOGGING_ARGS} ${AGENT_PROXY_CLASS} "${TARGET_PID}" "${IPC_SERVER_NAME}" fi fi
--- a/vm-byteman/agent/src/main/java/com/redhat/thermostat/vm/byteman/agent/internal/BytemanAgentAttachManager.java Tue Nov 08 16:57:32 2016 +0100 +++ b/vm-byteman/agent/src/main/java/com/redhat/thermostat/vm/byteman/agent/internal/BytemanAgentAttachManager.java Tue Nov 08 15:09:04 2016 -0500 @@ -37,6 +37,10 @@ package com.redhat.thermostat.vm.byteman.agent.internal; import java.io.File; +import java.io.IOException; +import java.nio.file.FileSystems; +import java.nio.file.attribute.UserPrincipal; +import java.nio.file.attribute.UserPrincipalLookupService; import java.util.ArrayList; import java.util.List; import java.util.Properties; @@ -50,6 +54,7 @@ import com.redhat.thermostat.shared.config.CommonPaths; import com.redhat.thermostat.storage.core.VmId; import com.redhat.thermostat.storage.core.WriterID; +import com.redhat.thermostat.vm.byteman.agent.internal.ProcessUserInfoBuilder.ProcessUserInfo; import com.redhat.thermostat.vm.byteman.common.VmBytemanDAO; import com.redhat.thermostat.vm.byteman.common.VmBytemanStatus; @@ -72,26 +77,37 @@ static List<String> helperJars; private final SubmitHelper submit; + private final FileSystemUtils fsUtils; private BytemanAttacher attacher; private IPCEndpointsManager ipcManager; private VmBytemanDAO vmBytemanDao; private WriterID writerId; + private ProcessUserInfoBuilder userInfoBuilder; BytemanAgentAttachManager() { this.submit = new SubmitHelper(); + this.fsUtils = new FileSystemUtils(); } // for testing only - BytemanAgentAttachManager(BytemanAttacher attacher, IPCEndpointsManager ipcManager, VmBytemanDAO vmBytemanDao, SubmitHelper submit, WriterID writerId) { + BytemanAgentAttachManager(BytemanAttacher attacher, IPCEndpointsManager ipcManager, VmBytemanDAO vmBytemanDao, SubmitHelper submit, + WriterID writerId, ProcessUserInfoBuilder userInfoBuilder, FileSystemUtils fsUtils) { this.attacher = attacher; this.ipcManager = ipcManager; this.vmBytemanDao = vmBytemanDao; this.submit = submit; this.writerId = writerId; + this.userInfoBuilder = userInfoBuilder; + this.fsUtils = fsUtils; } VmBytemanStatus attachBytemanToVm(VmId vmId, int vmPid) { logger.fine("Attaching byteman agent to VM '" + vmPid + "'"); + // Fail early if we can't determine process owner + UserPrincipal owner = getUserPrincipalForPid(vmPid); + if (owner == null) { + return null; + } BytemanAgentInfo info = attacher.attach(vmId.get(), vmPid, writerId.getWriterID()); if (info == null) { logger.warning("Failed to attach byteman agent for VM '" + vmPid + "'. Skipping rule updater and IPC channel."); @@ -103,7 +119,7 @@ return null; } ThermostatIPCCallbacks callback = new BytemanMetricsReceiver(vmBytemanDao, socketId); - ipcManager.startIPCEndpoint(socketId, callback); + ipcManager.startIPCEndpoint(socketId, callback, owner); // Add a status record to storage VmBytemanStatus status = new VmBytemanStatus(writerId.getWriterID()); status.setListenPort(info.getAgentListenPort()); @@ -113,6 +129,23 @@ return status; } + private UserPrincipal getUserPrincipalForPid(int vmPid) { + UserPrincipal principal = null; + ProcessUserInfo info = userInfoBuilder.build(vmPid); + String username = info.getUsername(); + if (username == null) { + logger.warning("Unable to determine owner of VM '" + vmPid + "'. Skipping rule updater and IPC channel."); + } else { + UserPrincipalLookupService lookup = fsUtils.getUserPrincipalLookupService(); + try { + principal = lookup.lookupPrincipalByName(username); + } catch (IOException e) { + logger.log(Level.WARNING, "Invalid user name '" + username + "' for VM '" + vmPid + "'. Skipping rule updater and IPC channel.", e); + } + } + return principal; + } + private boolean performPostAttachSteps(BytemanAgentInfo info, VmSocketIdentifier socketId) { if (info.isAttachFailedNoSuchProcess()) { logger.finest("Process with pid " + info.getVmPid() + " went away before we could attach the byteman agent to it."); @@ -185,6 +218,10 @@ this.writerId = writerId; } + void setUserInfoBuilder(ProcessUserInfoBuilder userInfoBuilder) { + this.userInfoBuilder = userInfoBuilder; + } + static class SubmitHelper { boolean addJarsToSystemClassLoader(List<String> jars, BytemanAgentInfo info) { @@ -212,4 +249,12 @@ } } + + static class FileSystemUtils { + + UserPrincipalLookupService getUserPrincipalLookupService() { + return FileSystems.getDefault().getUserPrincipalLookupService(); + } + + } }
--- a/vm-byteman/agent/src/main/java/com/redhat/thermostat/vm/byteman/agent/internal/BytemanRequestReceiver.java Tue Nov 08 16:57:32 2016 +0100 +++ b/vm-byteman/agent/src/main/java/com/redhat/thermostat/vm/byteman/agent/internal/BytemanRequestReceiver.java Tue Nov 08 15:09:04 2016 -0500 @@ -53,6 +53,8 @@ import com.redhat.thermostat.agent.command.RequestReceiver; import com.redhat.thermostat.agent.ipc.server.AgentIPCService; +import com.redhat.thermostat.agent.utils.ProcDataSource; +import com.redhat.thermostat.agent.utils.username.UserNameUtil; import com.redhat.thermostat.common.command.Request; import com.redhat.thermostat.common.command.Response; import com.redhat.thermostat.common.command.Response.ResponseType; @@ -103,6 +105,9 @@ @Reference private AgentIPCService agentIpcService; + @Reference + private UserNameUtil userNameUtil; + //////////////////////////////////////////////// // methods used by DS //////////////////////////////////////////////// @@ -148,6 +153,15 @@ attachManager.setIpcManager(null); } + protected void bindUserNameUtil(UserNameUtil userNameUtil) { + ProcessUserInfoBuilder userInfoBuilder = new ProcessUserInfoBuilder(new ProcDataSource(), userNameUtil); + attachManager.setUserInfoBuilder(userInfoBuilder); + } + + protected void unbindUserNameUtil(UserNameUtil userNameUtil) { + attachManager.setUserInfoBuilder(null); + } + //////////////////////////////////////////////// // end methods used by DS ////////////////////////////////////////////////
--- a/vm-byteman/agent/src/main/java/com/redhat/thermostat/vm/byteman/agent/internal/IPCEndpointsManager.java Tue Nov 08 16:57:32 2016 +0100 +++ b/vm-byteman/agent/src/main/java/com/redhat/thermostat/vm/byteman/agent/internal/IPCEndpointsManager.java Tue Nov 08 15:09:04 2016 -0500 @@ -37,6 +37,7 @@ package com.redhat.thermostat.vm.byteman.agent.internal; import java.io.IOException; +import java.nio.file.attribute.UserPrincipal; import java.util.HashSet; import java.util.Set; import java.util.logging.Level; @@ -63,7 +64,8 @@ this.ipcService = ipcService; } - synchronized void startIPCEndpoint(final VmSocketIdentifier socketId, final ThermostatIPCCallbacks callback) { + synchronized void startIPCEndpoint(final VmSocketIdentifier socketId, final ThermostatIPCCallbacks callback, + final UserPrincipal owner) { logger.fine("Starting IPC socket for byteman helper"); String sId = socketId.getName(); if (!sockets.contains(sId)) { @@ -74,7 +76,7 @@ logger.warning("Socket with id: " + sId + " already exists. Bug?"); return; } - ipcService.createServer(sId, callback); + ipcService.createServer(sId, callback, owner); sockets.add(sId); logger.fine("Created IPC endpoint for id: " + sId); } catch (IOException e) {
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/vm-byteman/agent/src/main/java/com/redhat/thermostat/vm/byteman/agent/internal/ProcessUserInfoBuilder.java Tue Nov 08 15:09:04 2016 -0500 @@ -0,0 +1,142 @@ +/* + * Copyright 2012-2016 Red Hat, Inc. + * + * This file is part of Thermostat. + * + * Thermostat is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published + * by the Free Software Foundation; either version 2, or (at your + * option) any later version. + * + * Thermostat is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Thermostat; see the file COPYING. If not see + * <http://www.gnu.org/licenses/>. + * + * Linking this code with other modules is making a combined work + * based on this code. Thus, the terms and conditions of the GNU + * General Public License cover the whole combination. + * + * As a special exception, the copyright holders of this code give + * you permission to link this code with independent modules to + * produce an executable, regardless of the license terms of these + * independent modules, and to copy and distribute the resulting + * executable under terms of your choice, provided that you also + * meet, for each linked independent module, the terms and conditions + * of the license of that module. An independent module is a module + * which is not derived from or based on this code. If you modify + * this code, you may extend this exception to your version of the + * library, but you are not obligated to do so. If you do not wish + * to do so, delete this exception statement from your version. + */ + +package com.redhat.thermostat.vm.byteman.agent.internal; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.Reader; +import java.util.logging.Level; +import java.util.logging.Logger; + +import com.redhat.thermostat.agent.utils.ProcDataSource; +import com.redhat.thermostat.agent.utils.username.UserNameLookupException; +import com.redhat.thermostat.agent.utils.username.UserNameUtil; +import com.redhat.thermostat.common.utils.LoggingUtils; + +/* + * FIXME: This class was copied from system-backend. + * Replace when this information is available from an API. + */ +class ProcessUserInfoBuilder { + + private static final ProcessUserInfo NON_EXISTENT_USER = new ProcessUserInfo(); + private static final String PROC_STATUS_UID = "Uid:"; + private static final Logger logger = LoggingUtils.getLogger(ProcessUserInfoBuilder.class); + private ProcDataSource source; + private UserNameUtil userNameUtil; + + ProcessUserInfoBuilder(ProcDataSource source, UserNameUtil userNameUtil) { + this.source = source; + this.userNameUtil = userNameUtil; + } + + static class ProcessUserInfo { + + private long uid; + private String username; + + ProcessUserInfo(long uid, String username) { + this.uid = uid; + this.username = username; + } + + ProcessUserInfo() { + this.uid = -1; + this.username = null; + } + + public long getUid() { + return uid; + } + + public String getUsername() { + return username; + } + } + + ProcessUserInfo build(int pid) { + ProcessUserInfo info = NON_EXISTENT_USER; + try { + Reader reader = source.getStatusReader(pid); + long uid = getUidFromProcfs(new BufferedReader(reader)); + String name = null; + try { + name = userNameUtil.getUserName(uid); + } catch (UserNameLookupException e) { + logger.log(Level.WARNING, "Unable to retrieve username for uid: " + uid, e); + } + info = new ProcessUserInfo(uid, name); + } catch (IOException e) { + logger.log(Level.WARNING, "Unable to read user info for " + pid, e); + } + + return info; + } + + /* + * Look for the following line: + * Uid: <RealUid> <EffectiveUid> <SavedUid> <FSUid> + */ + private long getUidFromProcfs(BufferedReader br) throws IOException { + long uid = -1; + String line; + while ((line = br.readLine()) != null) { + line = line.trim(); + if (line.startsWith(PROC_STATUS_UID)) { + String[] parts = line.split("\\s+"); + if (parts.length == 5) { + try { + // Use Real UID + uid = Long.parseLong(parts[1]); + } catch (NumberFormatException e) { + throw new IOException("Unexpected output from ps command", e); + } + } + else { + throw new IOException("Expected 5 parts from split /proc/${pid}/status output, got " + parts.length); + } + } + } + if (uid < 0) { + throw new IOException("Unable to determine UID from /proc/${pid}/status"); + } + return uid; + } + + +} +
--- a/vm-byteman/agent/src/test/java/com/redhat/thermostat/vm/byteman/agent/internal/BytemanAgentAttachManagerTest.java Tue Nov 08 16:57:32 2016 +0100 +++ b/vm-byteman/agent/src/test/java/com/redhat/thermostat/vm/byteman/agent/internal/BytemanAgentAttachManagerTest.java Tue Nov 08 15:09:04 2016 -0500 @@ -51,6 +51,8 @@ import java.io.File; import java.io.IOException; +import java.nio.file.attribute.UserPrincipal; +import java.nio.file.attribute.UserPrincipalLookupService; import java.util.List; import java.util.Properties; @@ -64,8 +66,10 @@ import com.redhat.thermostat.shared.config.CommonPaths; import com.redhat.thermostat.storage.core.VmId; import com.redhat.thermostat.storage.core.WriterID; +import com.redhat.thermostat.vm.byteman.agent.internal.BytemanAgentAttachManager.FileSystemUtils; import com.redhat.thermostat.vm.byteman.agent.internal.BytemanAgentAttachManager.SubmitHelper; import com.redhat.thermostat.vm.byteman.agent.internal.BytemanAttacher.BtmInstallHelper; +import com.redhat.thermostat.vm.byteman.agent.internal.ProcessUserInfoBuilder.ProcessUserInfo; import com.redhat.thermostat.vm.byteman.common.VmBytemanDAO; import com.redhat.thermostat.vm.byteman.common.VmBytemanStatus; @@ -74,6 +78,8 @@ private static final VmId SOME_VM_ID = new VmId("some-vm-id"); private static final int SOME_VM_PID = 99910; private static final String SOME_AGENT_ID = "some-agent-id"; + private static final int SOME_UID = 1111; + private static final String SOME_USERNAME = "someUser"; private static final BytemanAgentInfo SOME_SUCCESS_BYTEMAN_INFO = new BytemanAgentInfo(SOME_VM_PID, 3344, null, SOME_VM_ID.get(), SOME_AGENT_ID, false, false); private BytemanAgentAttachManager manager; @@ -81,16 +87,27 @@ private VmBytemanDAO vmBytemanDao; private SubmitHelper submit; private BytemanAttacher bytemanAttacher; + private ProcessUserInfoBuilder userInfoBuilder; + private UserPrincipalLookupService lookup; @Before - public void setup() { + public void setup() throws IOException { ipcManager = mock(IPCEndpointsManager.class); vmBytemanDao = mock(VmBytemanDAO.class); submit = mock(SubmitHelper.class); bytemanAttacher = mock(BytemanAttacher.class); WriterID writerId = mock(WriterID.class); when(writerId.getWriterID()).thenReturn(SOME_AGENT_ID); - manager = new BytemanAgentAttachManager(bytemanAttacher, ipcManager, vmBytemanDao, submit, writerId); + userInfoBuilder = mock(ProcessUserInfoBuilder.class); + FileSystemUtils fsUtils = mock(FileSystemUtils.class); + + ProcessUserInfo info = new ProcessUserInfo(SOME_UID, SOME_USERNAME); + when(userInfoBuilder.build(SOME_VM_PID)).thenReturn(info); + lookup = mock(UserPrincipalLookupService.class); + when(fsUtils.getUserPrincipalLookupService()).thenReturn(lookup); + UserPrincipal owner = mock(UserPrincipal.class); + when(lookup.lookupPrincipalByName(SOME_USERNAME)).thenReturn(owner); + manager = new BytemanAgentAttachManager(bytemanAttacher, ipcManager, vmBytemanDao, submit, writerId, userInfoBuilder, fsUtils); } @After @@ -100,7 +117,7 @@ @SuppressWarnings({ "unchecked", "rawtypes" }) @Test - public void canAttachAgentToVmStartIPCandAddsStatus() { + public void canAttachAgentToVmStartIPCandAddsStatus() throws IOException { String workingVmId = "working-vm-id"; int vmPid = 1001; String agentId = "working-agent-id"; @@ -111,6 +128,11 @@ // mock that installing of helper jars works when(submit.addJarsToSystemClassLoader(any(List.class), any(BytemanAgentInfo.class))).thenReturn(true); + ProcessUserInfo info = new ProcessUserInfo(5000, "testUser"); + when(userInfoBuilder.build(vmPid)).thenReturn(info); + UserPrincipal owner = mock(UserPrincipal.class); + when(lookup.lookupPrincipalByName("testUser")).thenReturn(owner); + VmId vmId = new VmId(workingVmId); WriterID writerId = mock(WriterID.class); when(writerId.getWriterID()).thenReturn(agentId); @@ -119,7 +141,7 @@ VmSocketIdentifier socketId = new VmSocketIdentifier(workingVmId, vmPid, agentId); // IPC endpoint must be started - verify(ipcManager).startIPCEndpoint(eq(socketId), isA(BytemanMetricsReceiver.class)); + verify(ipcManager).startIPCEndpoint(eq(socketId), isA(BytemanMetricsReceiver.class), eq(owner)); // Status should have been updated/inserted ArgumentCaptor<VmBytemanStatus> statusCaptor = ArgumentCaptor.forClass(VmBytemanStatus.class); @@ -140,7 +162,7 @@ @SuppressWarnings("unchecked") @Test - public void canUseOldAttachedAgentStartIPCandAddsStatus() { + public void canUseOldAttachedAgentStartIPCandAddsStatus() throws IOException { String workingVmId = "working-vm-id"; int vmPid = 1001; String agentId = "working-agent-id"; @@ -151,6 +173,11 @@ // mock setting system properties' success when(submit.setSystemProperties(any(Properties.class), any(BytemanAgentInfo.class))).thenReturn(true); + ProcessUserInfo info = new ProcessUserInfo(5000, "testUser"); + when(userInfoBuilder.build(vmPid)).thenReturn(info); + UserPrincipal owner = mock(UserPrincipal.class); + when(lookup.lookupPrincipalByName("testUser")).thenReturn(owner); + VmId vmId = new VmId(workingVmId); WriterID writerId = mock(WriterID.class); when(writerId.getWriterID()).thenReturn(agentId); @@ -159,7 +186,7 @@ VmSocketIdentifier socketId = new VmSocketIdentifier(workingVmId, vmPid, agentId); // IPC endpoint must be started - verify(ipcManager).startIPCEndpoint(eq(socketId), isA(BytemanMetricsReceiver.class)); + verify(ipcManager).startIPCEndpoint(eq(socketId), isA(BytemanMetricsReceiver.class), eq(owner)); // Status should have been updated/inserted ArgumentCaptor<VmBytemanStatus> statusCaptor = ArgumentCaptor.forClass(VmBytemanStatus.class); @@ -186,11 +213,32 @@ } @Test + public void usernameFailureDoesNotAttach() throws IOException { + ProcessUserInfo info = new ProcessUserInfo(SOME_UID, null); + when(userInfoBuilder.build(SOME_VM_PID)).thenReturn(info); + + VmBytemanStatus status = manager.attachBytemanToVm(SOME_VM_ID, SOME_VM_PID); + verify(bytemanAttacher, never()).attach(any(String.class), any(int.class), any(String.class)); + verify(ipcManager, never()).startIPCEndpoint(any(VmSocketIdentifier.class), any(ThermostatIPCCallbacks.class), any(UserPrincipal.class)); + assertNull(status); + } + + @Test + public void userPrincipalExceptionDoesNotAttach() throws IOException { + when(lookup.lookupPrincipalByName(SOME_USERNAME)).thenThrow(new IOException("TEST")); + + VmBytemanStatus status = manager.attachBytemanToVm(SOME_VM_ID, SOME_VM_PID); + verify(bytemanAttacher, never()).attach(any(String.class), any(int.class), any(String.class)); + verify(ipcManager, never()).startIPCEndpoint(any(VmSocketIdentifier.class), any(ThermostatIPCCallbacks.class), any(UserPrincipal.class)); + assertNull(status); + } + + @Test public void failureToAttachDoesNotStartIPC() throws Exception { BytemanAttacher failAttacher = getFailureAttacher(); manager.setAttacher(failAttacher); VmBytemanStatus status = manager.attachBytemanToVm(SOME_VM_ID, SOME_VM_PID); - verify(ipcManager, never()).startIPCEndpoint(any(VmSocketIdentifier.class), any(ThermostatIPCCallbacks.class)); + verify(ipcManager, never()).startIPCEndpoint(any(VmSocketIdentifier.class), any(ThermostatIPCCallbacks.class), any(UserPrincipal.class)); assertNull(status); }
--- a/vm-byteman/agent/src/test/java/com/redhat/thermostat/vm/byteman/agent/internal/IPCEndpointsManagerTest.java Tue Nov 08 16:57:32 2016 +0100 +++ b/vm-byteman/agent/src/test/java/com/redhat/thermostat/vm/byteman/agent/internal/IPCEndpointsManagerTest.java Tue Nov 08 15:09:04 2016 -0500 @@ -45,6 +45,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import java.io.IOException; +import java.nio.file.attribute.UserPrincipal; import org.junit.Before; import org.junit.Test; @@ -63,11 +64,13 @@ private IPCEndpointsManager ipcManager; private AgentIPCService ipcService; private VmBytemanDAO vmBytemanDao; + private UserPrincipal owner; @Before public void setup() { vmBytemanDao = mock(VmBytemanDAO.class); ipcService = mock(AgentIPCService.class); + owner = mock(UserPrincipal.class); ipcManager = new IPCEndpointsManager(ipcService); } @@ -79,9 +82,9 @@ private String doStartTest() throws IOException { ThermostatIPCCallbacks callback = new BytemanMetricsReceiver(vmBytemanDao, SOME_SOCK_ID); - ipcManager.startIPCEndpoint(SOME_SOCK_ID, callback); + ipcManager.startIPCEndpoint(SOME_SOCK_ID, callback, owner); String name = SOME_SOCK_ID.getName(); - verify(ipcService).createServer(eq(name), isA(BytemanMetricsReceiver.class)); + verify(ipcService).createServer(eq(name), isA(BytemanMetricsReceiver.class), eq(owner)); verify(ipcService).serverExists(name); return name; } @@ -113,7 +116,7 @@ // start it again, which should be a no-op ThermostatIPCCallbacks callback = new BytemanMetricsReceiver(vmBytemanDao, SOME_SOCK_ID); - ipcManager.startIPCEndpoint(SOME_SOCK_ID, callback); + ipcManager.startIPCEndpoint(SOME_SOCK_ID, callback, owner); verifyNoMoreInteractions(ipcService); } }