Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLOUDSTACK-9993: Securing Agents Communications #2239

Merged
merged 3 commits into from
Aug 28, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ env:
matrix:
- TESTS="smoke/test_affinity_groups
smoke/test_affinity_groups_projects
smoke/test_certauthority_root
smoke/test_deploy_vgpu_enabled_vm
smoke/test_deploy_vm_iso
smoke/test_deploy_vm_root_resize
Expand Down
127 changes: 121 additions & 6 deletions agent/src/com/cloud/agent/Agent.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@
// under the License.
package com.cloud.agent;

import java.io.File;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.net.InetAddress;
import java.net.UnknownHostException;
import java.nio.channels.ClosedChannelException;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
Expand All @@ -35,7 +37,13 @@

import javax.naming.ConfigurationException;

import org.apache.cloudstack.ca.SetupCertificateAnswer;
import org.apache.cloudstack.ca.SetupCertificateCommand;
import org.apache.cloudstack.ca.SetupKeyStoreCommand;
import org.apache.cloudstack.ca.SetupKeystoreAnswer;
import org.apache.cloudstack.managed.context.ManagedContextTimerTask;
import org.apache.cloudstack.utils.security.KeyStoreUtils;
import org.apache.commons.io.FileUtils;
import org.apache.log4j.Logger;
import org.slf4j.MDC;

Expand Down Expand Up @@ -68,6 +76,7 @@
import com.cloud.utils.nio.Task;
import com.cloud.utils.script.OutputInterpreter;
import com.cloud.utils.script.Script;
import com.google.common.base.Strings;

/**
* @config
Expand Down Expand Up @@ -126,6 +135,9 @@ public int value() {
private final ThreadPoolExecutor _ugentTaskPool;
ExecutorService _executor;

private String _keystoreSetupPath;
private String _keystoreCertImportPath;

// for simulator use only
public Agent(final IAgentShell shell) {
_shell = shell;
Expand Down Expand Up @@ -166,7 +178,8 @@ public Agent(final IAgentShell shell, final int localAgentId, final ServerResour
throw new ConfigurationException("Unable to configure " + _resource.getName());
}

_connection = new NioClient("Agent", _shell.getHost(), _shell.getPort(), _shell.getWorkers(), this);
final String host = _shell.getHost();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This final seems strange, as we intend to loop over adresses, right? (I might be mixing things up here, please do tell)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland yes, the getHost is supposed to internally loop/return the next address.

_connection = new NioClient("Agent", host, _shell.getPort(), _shell.getWorkers(), this);

// ((NioClient)_connection).setBindAddress(_shell.getPrivateIp());

Expand All @@ -182,7 +195,7 @@ public Agent(final IAgentShell shell, final int localAgentId, final ServerResour
"agentRequest-Handler"));

s_logger.info("Agent [id = " + (_id != null ? _id : "new") + " : type = " + getResourceName() + " : zone = " + _shell.getZone() + " : pod = " + _shell.getPod() +
" : workers = " + _shell.getWorkers() + " : host = " + _shell.getHost() + " : port = " + _shell.getPort());
" : workers = " + _shell.getWorkers() + " : host = " + host + " : port = " + _shell.getPort());
}

public String getVersion() {
Expand Down Expand Up @@ -224,15 +237,27 @@ public void start() {
throw new CloudRuntimeException("Unable to start the resource: " + _resource.getName());
}

_keystoreSetupPath = Script.findScript("scripts/util/", KeyStoreUtils.keyStoreSetupScript);
if (_keystoreSetupPath == null) {
throw new CloudRuntimeException(String.format("Unable to find the '%s' script", KeyStoreUtils.keyStoreSetupScript));
}

_keystoreCertImportPath = Script.findScript("scripts/util/", KeyStoreUtils.keyStoreImportScript);
if (_keystoreCertImportPath == null) {
throw new CloudRuntimeException(String.format("Unable to find the '%s' script", KeyStoreUtils.keyStoreImportScript));
}

try {
_connection.start();
} catch (final NioConnectionException e) {
s_logger.warn("NIO Connection Exception " + e);
s_logger.info("Attempted to connect to the server, but received an unexpected exception, trying again...");
}
while (!_connection.isStartup()) {
final String host = _shell.getHost();
_shell.getBackoffAlgorithm().waitBeforeRetry();
_connection = new NioClient("Agent", _shell.getHost(), _shell.getPort(), _shell.getWorkers(), this);
_connection = new NioClient("Agent", host, _shell.getPort(), _shell.getWorkers(), this);
s_logger.info("Connecting to host:" + host);
try {
_connection.start();
} catch (final NioConnectionException e) {
Expand Down Expand Up @@ -408,14 +433,21 @@ protected void reconnect(final Link link) {
_shell.getBackoffAlgorithm().waitBeforeRetry();
}

_connection = new NioClient("Agent", _shell.getHost(), _shell.getPort(), _shell.getWorkers(), this);
final String host = _shell.getHost();
do {
s_logger.info("Reconnecting...");
_connection = new NioClient("Agent", host, _shell.getPort(), _shell.getWorkers(), this);
s_logger.info("Reconnecting to host:" + host);
try {
_connection.start();
} catch (final NioConnectionException e) {
s_logger.warn("NIO Connection Exception " + e);
s_logger.info("Attempted to connect to the server, but received an unexpected exception, trying again...");
_connection.stop();
try {
_connection.cleanUp();
} catch (final IOException ex) {
s_logger.warn("Fail to clean up old connection. " + ex);
}
}
_shell.getBackoffAlgorithm().waitBeforeRetry();
} while (!_connection.isStartup());
Expand Down Expand Up @@ -515,7 +547,10 @@ protected void processRequest(final Request request, final Link link) {
s_logger.warn("No handler found to process cmd: " + cmd.toString());
answer = new AgentControlAnswer(cmd);
}

} else if (cmd instanceof SetupKeyStoreCommand && ((SetupKeyStoreCommand) cmd).isHandleByAgent()) {
answer = setupAgentKeystore((SetupKeyStoreCommand) cmd);
} else if (cmd instanceof SetupCertificateCommand && ((SetupCertificateCommand) cmd).isHandleByAgent()) {
answer = setupAgentCertificate((SetupCertificateCommand) cmd);
} else {
if (cmd instanceof ReadyCommand) {
processReadyCommand(cmd);
Expand Down Expand Up @@ -565,6 +600,86 @@ protected void processRequest(final Request request, final Link link) {
}
}

public Answer setupAgentKeystore(final SetupKeyStoreCommand cmd) {
final String keyStorePassword = cmd.getKeystorePassword();
final long validityDays = cmd.getValidityDays();

s_logger.debug("Setting up agent keystore file and generating CSR");

final File agentFile = PropertiesUtil.findConfigFile("agent.properties");
if (agentFile == null) {
return new Answer(cmd, false, "Failed to find agent.properties file");
}
final String keyStoreFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultKeystoreFile;
final String csrFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultCsrFile;

String storedPassword = _shell.getPersistentProperty(null, KeyStoreUtils.passphrasePropertyName);
if (Strings.isNullOrEmpty(storedPassword)) {
storedPassword = keyStorePassword;
_shell.setPersistentProperty(null, KeyStoreUtils.passphrasePropertyName, storedPassword);
}

Script script = new Script(_keystoreSetupPath, 60000, s_logger);
script.add(agentFile.getAbsolutePath());
script.add(keyStoreFile);
script.add(storedPassword);
script.add(String.valueOf(validityDays));
script.add(csrFile);
String result = script.execute();
if (result != null) {
throw new CloudRuntimeException("Unable to setup keystore file");
}

final String csrString;
try {
csrString = FileUtils.readFileToString(new File(csrFile), Charset.defaultCharset());
} catch (IOException e) {
throw new CloudRuntimeException("Unable to read generated CSR file", e);
}
return new SetupKeystoreAnswer(csrString);
}

private Answer setupAgentCertificate(final SetupCertificateCommand cmd) {
final String certificate = cmd.getCertificate();
final String privateKey = cmd.getPrivateKey();
final String caCertificates = cmd.getCaCertificates();

s_logger.debug("Importing received certificate to agent's keystore");

final File agentFile = PropertiesUtil.findConfigFile("agent.properties");
if (agentFile == null) {
return new Answer(cmd, false, "Failed to find agent.properties file");
}
final String keyStoreFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultKeystoreFile;
final String certFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultCertFile;
final String privateKeyFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultPrivateKeyFile;
final String caCertFile = agentFile.getParent() + "/" + KeyStoreUtils.defaultCaCertFile;

try {
FileUtils.writeStringToFile(new File(certFile), certificate, Charset.defaultCharset());
FileUtils.writeStringToFile(new File(caCertFile), caCertificates, Charset.defaultCharset());
s_logger.debug("Saved received client certificate to: " + certFile);
} catch (IOException e) {
throw new CloudRuntimeException("Unable to save received agent client and ca certificates", e);
}

Script script = new Script(_keystoreCertImportPath, 60000, s_logger);
script.add(agentFile.getAbsolutePath());
script.add(keyStoreFile);
script.add(KeyStoreUtils.agentMode);
script.add(certFile);
script.add("");
script.add(caCertFile);
script.add("");
script.add(privateKeyFile);
script.add(privateKey);
String result = script.execute();
if (result != null) {
throw new CloudRuntimeException("Unable to import certificate into keystore file");
}
return new SetupCertificateAnswer(true);
}

public void processResponse(final Response response, final Link link) {
final Answer answer = response.getAnswer();
if (s_logger.isDebugEnabled()) {
Expand Down
13 changes: 12 additions & 1 deletion agent/src/com/cloud/agent/AgentShell.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public class AgentShell implements IAgentShell, Daemon {
private int _proxyPort;
private int _workers;
private String _guid;
private int _hostCounter = 0;
private int _nextAgentId = 1;
private volatile boolean _exit = false;
private int _pingRetries;
Expand Down Expand Up @@ -107,7 +108,17 @@ public String getPod() {

@Override
public String getHost() {
return _host;
final String[] hosts = _host.split(",");
if (_hostCounter >= hosts.length) {
_hostCounter = 0;
}
final String host = hosts[_hostCounter % hosts.length];
_hostCounter++;
return host;
}

public void setHost(final String host) {
_host = host;
}

@Override
Expand Down
29 changes: 18 additions & 11 deletions agent/src/com/cloud/agent/dao/impl/PropertiesStorage.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ public synchronized String get(String key) {

@Override
public synchronized void persist(String key, String value) {
if (!loadFromFile(_file)) {
s_logger.error("Failed to load changes and then write to them");
}
_properties.setProperty(key, value);
FileOutputStream output = null;
try {
Expand All @@ -65,6 +68,20 @@ public synchronized void persist(String key, String value) {
}
}

private synchronized boolean loadFromFile(final File file) {
try {
PropertiesUtil.loadFromFile(_properties, file);
_file = file;
} catch (FileNotFoundException e) {
s_logger.error("How did we get here? ", e);
return false;
} catch (IOException e) {
s_logger.error("IOException: ", e);
return false;
}
return true;
}

@Override
public synchronized boolean configure(String name, Map<String, Object> params) {
_name = name;
Expand All @@ -86,17 +103,7 @@ public synchronized boolean configure(String name, Map<String, Object> params) {
return false;
}
}
try {
PropertiesUtil.loadFromFile(_properties, file);
_file = file;
} catch (FileNotFoundException e) {
s_logger.error("How did we get here? ", e);
return false;
} catch (IOException e) {
s_logger.error("IOException: ", e);
return false;
}
return true;
return loadFromFile(file);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,8 @@

import javax.naming.ConfigurationException;

import org.apache.log4j.Logger;

import com.google.gson.Gson;

import org.apache.cloudstack.managed.context.ManagedContextRunnable;
import org.apache.log4j.Logger;

import com.cloud.agent.Agent.ExitStatus;
import com.cloud.agent.api.AgentControlAnswer;
Expand Down Expand Up @@ -64,6 +61,7 @@
import com.cloud.utils.NumbersUtil;
import com.cloud.utils.net.NetUtils;
import com.cloud.utils.script.Script;
import com.google.gson.Gson;

/**
*
Expand Down Expand Up @@ -240,9 +238,11 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
_proxyVmId = NumbersUtil.parseLong(value, 0);

if (_localgw != null) {
String mgmtHost = (String)params.get("host");
String mgmtHosts = (String)params.get("host");
if (_eth1ip != null) {
addRouteToInternalIpOrCidr(_localgw, _eth1ip, _eth1mask, mgmtHost);
for (final String mgmtHost : mgmtHosts.split(",")) {
addRouteToInternalIpOrCidr(_localgw, _eth1ip, _eth1mask, mgmtHost);
}
String internalDns1 = (String) params.get("internaldns1");
if (internalDns1 == null) {
s_logger.warn("No DNS entry found during configuration of NfsSecondaryStorage");
Expand Down
15 changes: 15 additions & 0 deletions agent/test/com/cloud/agent/AgentShellTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,17 @@
// under the License.
package com.cloud.agent;

import java.util.Arrays;
import java.util.List;
import java.util.UUID;

import javax.naming.ConfigurationException;

import org.junit.Assert;
import org.junit.Test;

import com.cloud.utils.StringUtils;

public class AgentShellTest {
@Test
public void parseCommand() throws ConfigurationException {
Expand All @@ -44,4 +48,15 @@ public void loadProperties() throws ConfigurationException {
Assert.assertNotNull(shell.getProperties());
Assert.assertFalse(shell.getProperties().entrySet().isEmpty());
}

@Test
public void testGetHost() {
AgentShell shell = new AgentShell();
List<String> hosts = Arrays.asList("10.1.1.1", "20.2.2.2", "30.3.3.3", "2001:db8::1");
shell.setHost(StringUtils.listToCsvTags(hosts));
for (String host : hosts) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) 👍 I'd add the comment line that we expect the algorithm to be round-robbing for this test to succeed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the case for now.

Assert.assertEquals(host, shell.getHost());
}
Assert.assertEquals(shell.getHost(), hosts.get(0));
}
}
5 changes: 5 additions & 0 deletions api/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@
<artifactId>cloud-framework-config</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.apache.cloudstack</groupId>
<artifactId>cloud-framework-ca</artifactId>
<version>${project.version}</version>
</dependency>
</dependencies>
<build>
<plugins>
Expand Down
Loading