modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/core/CoreServerServiceImpl.java | 55 + modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/core/CoreServerServiceImplTest.java | 432 ++++++++++ 2 files changed, 482 insertions(+), 5 deletions(-)
New commits: commit 6f1033574f6e2541677382373f9960f29f8a2956 Author: John Mazzitelli mazz@redhat.com Date: Tue Jan 17 12:04:00 2012 -0500
[BZ 772318] the agent registration process was lax and allowed somethings that it shouldn't have. This closes the known holes and adds unit tests to check that all known use-cases are handled properly.
diff --git a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/core/CoreServerServiceImpl.java b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/core/CoreServerServiceImpl.java index 3b5ecc7..85aa364 100644 --- a/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/core/CoreServerServiceImpl.java +++ b/modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/core/CoreServerServiceImpl.java @@ -94,9 +94,6 @@ public class CoreServerServiceImpl implements CoreServerService { // If not, no point in continuing - the server won't be able to talk to the agent anyway. pingEndpoint(request.getRemoteEndpoint());
- // TODO (ghinkle): Check platform limit - do we still care about this? - //getPlatformManager().enforceLicenseLimit(args.getCpuCount()); - Agent agentByName = getAgentManager().getAgentByName(request.getName());
/* @@ -125,6 +122,38 @@ public class CoreServerServiceImpl implements CoreServerService { String msg = "The agent asking for registration is already registered with the name [" + agentByToken.getName() + "], it cannot change its name to [" + request.getName() + "]"; throw new AgentRegistrationException(msg); + } else { + Agent agentByAddressPort = getAgentManager().getAgentByAddressAndPort(request.getAddress(), + request.getPort()); + if (agentByAddressPort != null) { + // the agent request provided information about an authentic agent but it is trying to + // steal another agent's host/port. Thus, we will abort this request. + String msg = "The agent asking for registration [" + request.getName() + + "] is trying to register the same address/port [" + request.getAddress() + ":" + + request.getPort() + "] that is already registered under a different name [" + + agentByAddressPort.getName() + "]"; + throw new AgentRegistrationException(msg); + } + } + } else { + if (agentByName != null) { + // the agent request provided a name that already is in use by an agent. However, the request + // provided a security token that was not assigned to any agent! How can this be? Something is fishy. + String msg = "The agent asking for registration under the name [" + request.getName() + + "] provided an invalid security token. This request will fail."; + throw new AgentRegistrationException(msg); + } + Agent agentByAddressPort = getAgentManager().getAgentByAddressAndPort(request.getAddress(), + request.getPort()); + if (agentByAddressPort != null) { + // the agent request provided a security token but it is an unknown/unused/bogus token. + // However, the IP/port it wants to use is already in-use. This sounds fishy. If we let this + // go through, this agent with an unknown/bogus token will essentially hijack this IP/port + // belonging to an existing agent. If the agent wants to reuse an IP/port already in existence, it should + // already know its security token associated with that IP/port. Thus, we will abort this request. + String msg = "The agent asking for registration under the name [" + request.getName() + + "] is attempting to authenticate using an unknown security token. This request will fail."; + throw new AgentRegistrationException(msg); } } } else { @@ -141,6 +170,22 @@ public class CoreServerServiceImpl implements CoreServerService { + "]; if this new agent is actually the same as the original, then re-register with the same name"; throw new AgentRegistrationException(msg); } + } else { + if (agentByName != null) { + // the name being registered already exists, however, the agent request is trying to set it + // to some unknown IP/port combination and there is no security token to authenticate this request! + // Therefore, because this agent name is already registered and because this current request + // cannot authenticate itself with the proper security token, we fail. + String msg = "An agent is trying to register with an existing agent name [" + + request.getName() + + "]. The registration request is attempting to assign the agent an unknown address/port [" + + request.getAddress() + + ":" + + request.getPort() + + "] without providing a valid security token. If you are attempting to re-register this agent, " + + "make sure you register with its prior address/port."; + throw new AgentRegistrationException(msg); + } } }
@@ -196,8 +241,8 @@ public class CoreServerServiceImpl implements CoreServerService {
// the agent does not yet exist, we need to create it try { - agentByName = new Agent(request.getName(), request.getAddress(), request.getPort(), request - .getRemoteEndpoint(), generateAgentToken()); + agentByName = new Agent(request.getName(), request.getAddress(), request.getPort(), + request.getRemoteEndpoint(), generateAgentToken());
agentByName.setServer(registeringServer); agentManager.createAgent(agentByName); diff --git a/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/core/CoreServerServiceImplTest.java b/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/core/CoreServerServiceImplTest.java new file mode 100644 index 0000000..0abbbeb --- /dev/null +++ b/modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/core/CoreServerServiceImplTest.java @@ -0,0 +1,432 @@ +/* + * RHQ Management Platform + * Copyright 2011, Red Hat Middleware LLC, and individual contributors + * as indicated by the @author tags. See the copyright.txt file in the + * distribution for a full listing of individual contributors. + * + * This program 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 version 2 of the License. + * + * This program 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 this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +package org.rhq.enterprise.server.core; + +import java.io.File; +import java.io.FileOutputStream; +import java.util.Date; +import java.util.List; +import java.util.Properties; + +import javax.management.MBeanServer; +import javax.persistence.Query; + +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import org.rhq.core.clientapi.server.core.AgentRegistrationException; +import org.rhq.core.clientapi.server.core.AgentRegistrationRequest; +import org.rhq.core.clientapi.server.core.AgentRegistrationResults; +import org.rhq.core.clientapi.server.core.AgentVersion; +import org.rhq.core.domain.cloud.Server; +import org.rhq.core.domain.cloud.Server.OperationMode; +import org.rhq.core.domain.common.ProductInfo; +import org.rhq.core.domain.resource.Agent; +import org.rhq.enterprise.server.test.AbstractEJB3Test; +import org.rhq.enterprise.server.util.LookupUtil; + +/** + * @author John Mazzitelli + */ +@Test +public class CoreServerServiceImplTest extends AbstractEJB3Test { + private static final String TEST_AGENT_NAME_PREFIX = "CoreServerServiceImplTest.Agent"; + private static final String RHQ_SERVER_NAME_PROPERTY = "rhq.server.high-availability.name"; + private AgentVersion agentVersion; + private Server server; + private String oldServerNamePropertyValue = null; + private AgentRegistrationRequest aReq = null; + private AgentRegistrationResults aResults = null; + private AgentRegistrationRequest zReq = null; + private AgentRegistrationResults zResults = null; + + private static final int A_PORT = 11111; + private static final String A_HOST = "hostA"; + private static final int B_PORT = 22222; + private static final String B_HOST = "hostB"; + + public void testNewAgentRegistrationWithOldToken() throws Exception { + // this tests the case where someone purged an agent from the DB, but then + // changed their mind and want to re-run that agent and re-register it again. + // In this case, the agent (if not using --cleanconfig) would still have the old token. + // The agent should still be allowed to register again. + CoreServerServiceImpl service = new CoreServerServiceImpl(); + AgentRegistrationRequest request = createRequest(prefixName("old"), "hostOld", 12345, "oldtoken"); + AgentRegistrationResults results = service.registerAgent(request); + assert results != null : "cannot re-register an old agent"; + Agent agent = LookupUtil.getAgentManager().getAgentByAgentToken(results.getAgentToken()); + assert agent.getName().equals(request.getName()); + assert agent.getAddress().equals(request.getAddress()); + assert agent.getPort() == request.getPort(); + LookupUtil.getAgentManager().deleteAgent(agent); + } + + public void testChangeAddressPort() throws Exception { + CoreServerServiceImpl service = new CoreServerServiceImpl(); + AgentRegistrationRequest request; + AgentRegistrationResults results; + + String zName = prefixName("Z"); + + // create a new agent Z with host/port of hostZ/55550 + request = createRequest(zName, "hostZ", 55550, null); + results = service.registerAgent(request); + assert results != null : "got null results"; + + // now change Z's host to hostZprime + request = createRequest(zName, "hostZprime", 55550, results.getAgentToken()); + results = service.registerAgent(request); + assert results != null; + Agent agent = LookupUtil.getAgentManager().getAgentByAgentToken(results.getAgentToken()); + assert agent.getName().equals(zName); + assert agent.getAddress().equals("hostZprime"); + assert agent.getPort() == 55550; + + // now change Z's port to 55551 + request = createRequest(zName, "hostZprime", 55551, results.getAgentToken()); + results = service.registerAgent(request); + assert results != null; + agent = LookupUtil.getAgentManager().getAgentByAgentToken(results.getAgentToken()); + assert agent.getName().equals(zName); + assert agent.getAddress().equals("hostZprime"); + assert agent.getPort() == 55551; + + // now change Z's host/port to hostZdoubleprime/55552 + request = createRequest(zName, "hostZdoubleprime", 55552, results.getAgentToken()); + results = service.registerAgent(request); + assert results != null; + agent = LookupUtil.getAgentManager().getAgentByAgentToken(results.getAgentToken()); + assert agent.getName().equals(zName); + assert agent.getAddress().equals("hostZdoubleprime"); + assert agent.getPort() == 55552; + + // now don't change Z's host/port but re-register everything the same, but with no token + request = createRequest(zName, "hostZdoubleprime", 55552, null); + results = service.registerAgent(request); + assert results != null; + agent = LookupUtil.getAgentManager().getAgentByAgentToken(results.getAgentToken()); + assert agent.getName().equals(zName); + assert agent.getAddress().equals("hostZdoubleprime"); + assert agent.getPort() == 55552; + + // remember this agent so our later tests can use it + zReq = request; + zResults = results; + } + + @Test(dependsOnMethods = "testChangeAddressPort") + public void testNormalAgentRegistration() throws Exception { + CoreServerServiceImpl service = new CoreServerServiceImpl(); + aReq = createRequest(prefixName("A"), A_HOST, A_PORT, null); + aResults = service.registerAgent(aReq); + assert aResults != null : "got null results"; + } + + @Test(dependsOnMethods = "testNormalAgentRegistration") + public void testHijackExistingAgentAddressPort() throws Exception { + CoreServerServiceImpl service = new CoreServerServiceImpl(); + AgentRegistrationRequest request; + request = createRequest(prefixName("B"), aReq.getAddress(), aReq.getPort(), null); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack a used host/port with new agent name"; + } catch (AgentRegistrationException ok) { + } + } + + @Test(dependsOnMethods = "testNormalAgentRegistration") + public void testHijackExistingAgentName() throws Exception { + CoreServerServiceImpl service = new CoreServerServiceImpl(); + AgentRegistrationRequest request; + request = createRequest(aReq.getName(), aReq.getAddress(), B_PORT, null); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack a used agent name without a token"; + } catch (AgentRegistrationException ok) { + } + request = createRequest(aReq.getName(), B_HOST, aReq.getPort(), null); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack a used agent name without a token"; + } catch (AgentRegistrationException ok) { + } + request = createRequest(aReq.getName(), B_HOST, B_PORT, null); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack a used agent name without a token"; + } catch (AgentRegistrationException ok) { + } + } + + @Test(dependsOnMethods = "testNormalAgentRegistration") + public void testHijackExistingAgentAddressPortWithBogusToken() throws Exception { + CoreServerServiceImpl service = new CoreServerServiceImpl(); + AgentRegistrationRequest request; + request = createRequest(prefixName("B"), aReq.getAddress(), aReq.getPort(), "badtoken"); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack a used host/port with new agent name and invalid token"; + } catch (AgentRegistrationException ok) { + } + } + + @Test(dependsOnMethods = "testNormalAgentRegistration") + public void testHijackExistingAgentNameWithBogusToken() throws Exception { + CoreServerServiceImpl service = new CoreServerServiceImpl(); + AgentRegistrationRequest request; + request = createRequest(aReq.getName(), aReq.getAddress(), aReq.getPort(), "badtoken"); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack a used agent name with an invalid token"; + } catch (AgentRegistrationException ok) { + } + request = createRequest(aReq.getName(), aReq.getAddress(), B_PORT, "badtoken"); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack a used agent name with an invalid token"; + } catch (AgentRegistrationException ok) { + } + request = createRequest(aReq.getName(), B_HOST, aReq.getPort(), "badtoken"); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack a used agent name with an invalid token"; + } catch (AgentRegistrationException ok) { + } + request = createRequest(aReq.getName(), B_HOST, B_PORT, "badtoken"); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack a used agent name with an invalid token"; + } catch (AgentRegistrationException ok) { + } + } + + @Test(dependsOnMethods = "testNormalAgentRegistration") + public void testHijackExistingAgentNameWithAnotherAgentToken() throws Exception { + CoreServerServiceImpl service = new CoreServerServiceImpl(); + AgentRegistrationRequest request; + request = createRequest(aReq.getName(), aReq.getAddress(), aReq.getPort(), zResults.getAgentToken()); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack agent A using Z's token"; + } catch (AgentRegistrationException ok) { + } + request = createRequest(aReq.getName(), B_HOST, aReq.getPort(), zResults.getAgentToken()); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack agent A using Z's token"; + } catch (AgentRegistrationException ok) { + } + request = createRequest(aReq.getName(), aReq.getAddress(), B_PORT, zResults.getAgentToken()); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack agent A using Z's token"; + } catch (AgentRegistrationException ok) { + } + request = createRequest(aReq.getName(), B_HOST, B_PORT, zResults.getAgentToken()); + try { + service.registerAgent(request); + assert false : "Should not have been able to hijack agent A using Z's token"; + } catch (AgentRegistrationException ok) { + } + } + + @Test(dependsOnMethods = "testNormalAgentRegistration") + public void testAgentHijackingAnotherAgentAddressPort() throws Exception { + CoreServerServiceImpl service = new CoreServerServiceImpl(); + AgentRegistrationRequest request; + request = createRequest(aReq.getName(), zReq.getAddress(), zReq.getPort(), aResults.getAgentToken()); + try { + service.registerAgent(request); + assert false : "An agent should not have been able to hijack another agent's host/port"; + } catch (AgentRegistrationException ok) { + } + } + + @Test(dependsOnMethods = "testNormalAgentRegistration") + public void testAttemptToChangeAgentName() throws Exception { + CoreServerServiceImpl service = new CoreServerServiceImpl(); + AgentRegistrationRequest request; + request = createRequest(prefixName("newName"), zReq.getAddress(), zReq.getPort(), zResults.getAgentToken()); + try { + service.registerAgent(request); + assert false : "An agent should not be able to change its name"; + } catch (AgentRegistrationException ok) { + } + } + + private AgentRegistrationRequest createRequest(String name, String address, int port, String token) { + return new AgentRegistrationRequest(name, address, port, "socket://" + address + ":" + port + + "/?rhq.communications.connector.rhqtype=agent", true, token, agentVersion); + } + + private String prefixName(String name) { + return TEST_AGENT_NAME_PREFIX + name; + } + + @BeforeClass + public void prepare() throws Exception { + // mock the name of our server via the sysprop (in production, this is normally set in rhq-server.properties) + oldServerNamePropertyValue = System.getProperty(RHQ_SERVER_NAME_PROPERTY); + String newServerNamePropertyValue = "CoreServerServiceImplTest.Server"; + System.setProperty(RHQ_SERVER_NAME_PROPERTY, newServerNamePropertyValue); + + // mock up our core server MBean that provides information about where the jboss home dir is + MBeanServer mbs = getJBossMBeanServer(); + DummyCoreServer mbean = new DummyCoreServer(); + mbs.registerMBean(mbean, CoreServerMBean.OBJECT_NAME); + + // in order to register, we need to mock out the agent version file used by the server + // to determine the agent version it supports. + agentVersion = new AgentVersion("1.2.3", "12345"); + File agentVersionFile = new File(mbean.getJBossServerHomeDir(), + "deploy/rhq.ear/rhq-downloads/rhq-agent/rhq-server-agent-versions.properties"); + agentVersionFile.getParentFile().mkdirs(); + agentVersionFile.delete(); + Properties agentVersionProps = new Properties(); + agentVersionProps.put("rhq-agent.latest.version", agentVersion.getVersion()); + agentVersionProps.put("rhq-agent.latest.build-number", agentVersion.getBuild()); + FileOutputStream fos = new FileOutputStream(agentVersionFile); + try { + agentVersionProps.store(fos, "This file was created by " + CoreServerServiceImplTest.class.getName()); + } finally { + fos.close(); + } + + // this mocks out the endpoint ping - the server will think the agent that is registering is up and pingable + prepareForTestAgents(); + + // mock our server + server = new Server(); + server.setName(newServerNamePropertyValue); + server.setAddress("CoreServerServiceImplTest.localhost"); + server.setPort(12345); + server.setSecurePort(12346); + server.setOperationMode(OperationMode.NORMAL); + int serverId = LookupUtil.getServerManager().create(server); + server.setId(serverId); + } + + @AfterClass + public void unprepare() throws Exception { + // clean up any agents we might have created + Query q = getEntityManager().createQuery( + "select a from Agent a where name like '" + TEST_AGENT_NAME_PREFIX + "%'"); + List<Agent> doomed = (List<Agent>) q.getResultList(); + for (Agent deleteMe : doomed) { + LookupUtil.getAgentManager().deleteAgent(deleteMe); + } + + // cleanup our test server + LookupUtil.getCloudManager().updateServerMode(new Integer[] { server.getId() }, OperationMode.DOWN); + LookupUtil.getCloudManager().deleteServer(server.getId()); + + // shutdown our mock mbean server + MBeanServer mbs = getJBossMBeanServer(); + mbs.unregisterMBean(CoreServerMBean.OBJECT_NAME); + + // in case this was set before our tests, put it back the way it was + if (oldServerNamePropertyValue != null) { + System.setProperty(RHQ_SERVER_NAME_PROPERTY, oldServerNamePropertyValue); + } + } + + interface DummyCoreServerMBean extends CoreServerMBean { + }; + + class DummyCoreServer implements DummyCoreServerMBean { + + @Override + public String getName() { + return "CoreServer"; + } + + @Override + public int getState() { + return 0; + } + + @Override + public String getStateString() { + return ""; + } + + @Override + public void jbossInternalLifecycle(String arg0) throws Exception { + } + + @Override + public void create() throws Exception { + } + + @Override + public void destroy() { + } + + @Override + public void start() throws Exception { + } + + @Override + public void stop() { + } + + @Override + public String getVersion() { + return null; + } + + @Override + public String getBuildNumber() { + return null; + } + + @Override + public Date getBootTime() { + return null; + } + + @Override + public File getInstallDir() { + return null; + } + + @Override + public File getJBossServerHomeDir() { + return new File(System.getProperty("java.io.tmpdir"), "CoreServerServiceImplTest"); + } + + @Override + public File getJBossServerDataDir() { + return null; + } + + @Override + public File getJBossServerTempDir() { + return null; + } + + @Override + public ProductInfo getProductInfo() { + return null; + } + } +}