Updating Openflow controller properties
- Moving TLS related config to separate config object
- Added support for adding/removing OF ports without restarting channel group
Change-Id: I79552334519434780d9174a1a9ff4eebc0d17194
diff --git a/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/Controller.java b/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/Controller.java
index 6122e63..92358c4 100644
--- a/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/Controller.java
+++ b/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/Controller.java
@@ -16,9 +16,12 @@
package org.onosproject.openflow.controller.impl;
+import com.google.common.base.MoreObjects;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Sets;
import io.netty.bootstrap.ServerBootstrap;
+import io.netty.channel.Channel;
import io.netty.channel.ChannelOption;
import io.netty.channel.EventLoopGroup;
import io.netty.channel.epoll.EpollEventLoopGroup;
@@ -51,6 +54,8 @@
import java.io.IOException;
import java.lang.management.ManagementFactory;
import java.lang.management.RuntimeMXBean;
+import java.net.InetSocketAddress;
+import java.net.SocketAddress;
import java.security.KeyManagementException;
import java.security.KeyStore;
import java.security.KeyStoreException;
@@ -58,12 +63,17 @@
import java.security.UnrecoverableKeyException;
import java.security.cert.Certificate;
import java.security.cert.CertificateException;
+import java.util.Collection;
+import java.util.Collections;
import java.util.Dictionary;
+import java.util.EnumSet;
import java.util.HashMap;
+import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
+import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
@@ -80,7 +90,6 @@
private static final Logger log = LoggerFactory.getLogger(Controller.class);
- private static final boolean TLS_DISABLED = false;
private static final short MIN_KS_LENGTH = 6;
protected HashMap<String, String> controllerNodeIPsCache;
@@ -99,10 +108,14 @@
private EventLoopGroup bossGroup;
private EventLoopGroup workerGroup;
- protected String ksLocation;
- protected String tsLocation;
- protected char[] ksPwd;
- protected char[] tsPwd;
+ enum TlsMode {
+ DISABLED, // TLS is not used for OpenFlow connections
+ ENABLED, // Clients are required use TLS and present a client certificate
+ STRICT, // Clients must use TLS, and certificate must match the one specified in netcfg
+ }
+ private static final EnumSet<TlsMode> TLS_ENABLED = EnumSet.of(TlsMode.ENABLED, TlsMode.STRICT);
+
+ protected TlsParams tlsParams;
protected SSLContext sslContext;
protected KeyStore keyStore;
@@ -110,34 +123,62 @@
protected static final int SEND_BUFFER_SIZE = 4 * 1024 * 1024;
private DriverService driverService;
- private boolean enableOfTls = TLS_DISABLED;
private NetworkConfigRegistry netCfgService;
+
+
// **************
// Initialization
// **************
- /**
- * Tell controller that we're ready to accept switches loop.
- */
- public void run() {
-
- final ServerBootstrap bootstrap = createServerBootStrap();
- bootstrap.option(ChannelOption.SO_REUSEADDR, true);
- bootstrap.childOption(ChannelOption.SO_KEEPALIVE, true);
- bootstrap.childOption(ChannelOption.TCP_NODELAY, true);
- bootstrap.childOption(ChannelOption.SO_SNDBUF, Controller.SEND_BUFFER_SIZE);
+ private void addListeningPorts(Collection<Integer> ports) {
+ if (cg == null) {
+ return;
+ }
+ final ServerBootstrap bootstrap = createServerBootStrap();
+ bootstrap.option(ChannelOption.SO_REUSEADDR, true);
+ bootstrap.childOption(ChannelOption.SO_KEEPALIVE, true);
+ bootstrap.childOption(ChannelOption.TCP_NODELAY, true);
+ bootstrap.childOption(ChannelOption.SO_SNDBUF, Controller.SEND_BUFFER_SIZE);
// bootstrap.childOption(ChannelOption.WRITE_BUFFER_WATER_MARK,
// new WriteBufferWaterMark(8 * 1024, 32 * 1024));
- bootstrap.childHandler(new OFChannelInitializer(this, null, sslContext));
+ bootstrap.childHandler(new OFChannelInitializer(this, null, sslContext));
- openFlowPorts.forEach(port -> {
- // TODO revisit if this is best way to listen to multiple ports
- cg.add(bootstrap.bind(port).syncUninterruptibly().channel());
- log.info("Listening for switch connections on {}", port);
- });
+ Set<Integer> existingPorts = cg.stream()
+ .map(Channel::localAddress)
+ .filter(InetSocketAddress.class::isInstance)
+ .map(InetSocketAddress.class::cast)
+ .map(InetSocketAddress::getPort)
+ .collect(Collectors.toSet());
+ ports.removeAll(existingPorts);
+ ports.forEach(port -> {
+ // TODO revisit if this is best way to listen to multiple ports
+ cg.add(bootstrap.bind(port).syncUninterruptibly().channel());
+ log.info("Listening for OF switch connections on {}", port);
+ });
+ }
+
+ private void removeListeningPorts(Collection<Integer> ports) {
+ if (cg == null) {
+ return;
+ }
+ Iterator<Channel> itr = cg.iterator();
+ while (itr.hasNext()) {
+ Channel c = itr.next();
+ SocketAddress addr = c.localAddress();
+ if (addr instanceof InetSocketAddress) {
+ InetSocketAddress inetAddr = (InetSocketAddress) addr;
+ Integer port = inetAddr.getPort();
+ if (ports.contains(port)) {
+ log.info("No longer listening for OF switch connections on {}", port);
+ c.close();
+ itr.remove();
+ }
+ }
+
+ }
}
private ServerBootstrap createServerBootStrap() {
@@ -177,21 +218,188 @@
}
public void setConfigParams(Dictionary<?, ?> properties) {
- // TODO should be possible to reconfigure ports without restart,
- // by updating ChannelGroup
- String ports = get(properties, "openflowPorts");
- if (!Strings.isNullOrEmpty(ports)) {
- this.openFlowPorts = Stream.of(ports.split(","))
- .map(s -> Integer.parseInt(s))
- .collect(Collectors.toList());
+ boolean restartRequired = setOpenFlowPorts(properties);
+ restartRequired |= setWorkerThreads(properties);
+ restartRequired |= setTlsParameters(properties);
+ if (restartRequired) {
+ restart();
}
+ }
+
+ /**
+ * Gets the list of listening ports from property dict.
+ *
+ * @param properties dictionary
+ * @return true if restart is required
+ */
+ private boolean setWorkerThreads(Dictionary<?, ?> properties) {
+ List<Integer> oldPorts = this.openFlowPorts;
+ String ports = get(properties, "openflowPorts");
+ List<Integer> newPorts = Collections.emptyList();
+ if (!Strings.isNullOrEmpty(ports)) {
+ newPorts = Stream.of(ports.split(","))
+ .map(s -> Integer.parseInt(s))
+ .collect(Collectors.toList());
+ }
+
+ Set<Integer> portsToAdd = Sets.newHashSet(newPorts);
+ portsToAdd.removeAll(oldPorts);
+ addListeningPorts(portsToAdd);
+
+ Set<Integer> portsToRemove = Sets.newHashSet(oldPorts);
+ portsToRemove.removeAll(newPorts);
+ removeListeningPorts(portsToRemove);
+
+ this.openFlowPorts = newPorts;
log.debug("OpenFlow ports set to {}", this.openFlowPorts);
+ return false; // restart is never required
+ }
+
+ /**
+ * Gets the number of worker threads from property dict.
+ *
+ * @param properties dictionary
+ * @return true if restart is required
+ */
+ private boolean setOpenFlowPorts(Dictionary<?, ?> properties) {
+ Integer oldValue = this.workerThreads;
String threads = get(properties, "workerThreads");
if (!Strings.isNullOrEmpty(threads)) {
this.workerThreads = Integer.parseInt(threads);
}
log.debug("Number of worker threads set to {}", this.workerThreads);
+ return oldValue != this.workerThreads; // restart if number of threads has changed
+ }
+
+ private static class TlsParams {
+ TlsMode mode;
+ String ksLocation;
+ String tsLocation;
+ String ksPwd;
+ String tsPwd;
+ //TODO add the hash of the keystore file contents, so that we restart if the keystore has changed
+
+ public char[] ksPwd() {
+ return ksPwd.toCharArray();
+ }
+
+ public char[] tsPwd() {
+ return tsPwd.toCharArray();
+ }
+
+ public boolean isTlsEnabled() {
+ return TLS_ENABLED.contains(mode);
+ }
+
+ @Override
+ public int hashCode() {
+ return 1; //TODO
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (obj instanceof TlsParams) {
+ final TlsParams that = (TlsParams) obj;
+ if (this.getClass() != that.getClass()) {
+ return false;
+ } else if (this.mode == that.mode && this.mode == TlsMode.DISABLED) {
+ // All disabled objects should be equal regardless of other params
+ return true;
+ }
+ return this.mode == that.mode &&
+ Objects.equals(this.ksLocation, that.ksLocation) &&
+ Objects.equals(this.tsLocation, that.tsLocation) &&
+ Objects.equals(this.ksPwd, that.ksPwd) &&
+ Objects.equals(this.tsPwd, that.tsPwd);
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this)
+ .add("tlsMode", mode.toString().toLowerCase())
+ .add("ksLocation", ksLocation)
+ .add("tsLocation", tsLocation)
+ .toString();
+ }
+ }
+
+ /**
+ * Gets the TLS parameters from the properties dict, but fallback to the old approach of
+ * system properties if the parameters are missing from the dict.
+ *
+ * @param properties dictionary
+ * @return true if restart is required
+ */
+ private boolean setTlsParameters(Dictionary<?, ?> properties) {
+ TlsParams oldParams = this.tlsParams;
+
+ TlsParams newParams = new TlsParams();
+ String tlsString = get(properties, "tlsMode");
+ if (!Strings.isNullOrEmpty(tlsString)) {
+ try {
+ newParams.mode = TlsMode.valueOf(tlsString.toUpperCase());
+ } catch (IllegalArgumentException e) {
+ log.info("Invalid TLS mode {}. TLS is disabled.", tlsString);
+ newParams.mode = TlsMode.DISABLED;
+ }
+ } else {
+ // Fallback to system properties
+ // TODO this method of configuring TLS is deprecated and should be removed eventually
+ tlsString = System.getProperty("enableOFTLS");
+ newParams.mode = !Strings.isNullOrEmpty(tlsString) && Boolean.parseBoolean(tlsString) ?
+ TlsMode.ENABLED : TlsMode.DISABLED;
+ }
+
+ if (newParams.isTlsEnabled()) {
+ newParams.ksLocation = get(properties, "keyStore");
+ if (Strings.isNullOrEmpty(newParams.ksLocation)) {
+ // Fallback to system properties
+ // TODO remove this eventually
+ newParams.ksLocation = System.getProperty("javax.net.ssl.keyStore");
+ }
+ if (Strings.isNullOrEmpty(newParams.ksLocation)) {
+ newParams.mode = TlsMode.DISABLED;
+ }
+
+ newParams.tsLocation = get(properties, "trustStore");
+ if (Strings.isNullOrEmpty(newParams.tsLocation)) {
+ // Fallback to system properties
+ // TODO remove this eventually
+ newParams.tsLocation = System.getProperty("javax.net.ssl.trustStore");
+ }
+ if (Strings.isNullOrEmpty(newParams.tsLocation)) {
+ newParams.mode = TlsMode.DISABLED;
+ }
+
+ newParams.ksPwd = get(properties, "keyStorePassword");
+ if (Strings.isNullOrEmpty(newParams.ksPwd)) {
+ // Fallback to system properties
+ // TODO remove this eventually
+ newParams.ksPwd = System.getProperty("javax.net.ssl.keyStorePassword");
+ }
+ if (Strings.isNullOrEmpty(newParams.ksPwd) || MIN_KS_LENGTH > newParams.ksPwd.length()) {
+ newParams.mode = TlsMode.DISABLED;
+ }
+
+ newParams.tsPwd = get(properties, "trustStorePassword");
+ if (Strings.isNullOrEmpty(newParams.tsPwd)) {
+ // Fallback to system properties
+ // TODO remove this eventually
+ newParams.tsPwd = System.getProperty("javax.net.ssl.trustStorePassword");
+ }
+ if (Strings.isNullOrEmpty(newParams.tsPwd) || MIN_KS_LENGTH > newParams.tsPwd.length()) {
+ newParams.mode = TlsMode.DISABLED;
+ }
+ }
+ this.tlsParams = newParams;
+ log.info("OpenFlow TLS Params: {}", tlsParams);
+ return !Objects.equals(newParams, oldParams); // restart if TLS params change
}
/**
@@ -206,51 +414,22 @@
cg = new DefaultChannelGroup(GlobalEventExecutor.INSTANCE);
- getTlsParameters();
- if (enableOfTls) {
+ if (tlsParams.isTlsEnabled()) {
initSsl();
}
}
- private void getTlsParameters() {
- String tempString = System.getProperty("enableOFTLS");
- enableOfTls = Strings.isNullOrEmpty(tempString) ? TLS_DISABLED : Boolean.parseBoolean(tempString);
- log.info("OpenFlow Security is {}", enableOfTls ? "enabled" : "disabled");
- if (enableOfTls) {
- ksLocation = System.getProperty("javax.net.ssl.keyStore");
- if (Strings.isNullOrEmpty(ksLocation)) {
- enableOfTls = TLS_DISABLED;
- return;
- }
- tsLocation = System.getProperty("javax.net.ssl.trustStore");
- if (Strings.isNullOrEmpty(tsLocation)) {
- enableOfTls = TLS_DISABLED;
- return;
- }
- ksPwd = System.getProperty("javax.net.ssl.keyStorePassword").toCharArray();
- if (MIN_KS_LENGTH > ksPwd.length) {
- enableOfTls = TLS_DISABLED;
- return;
- }
- tsPwd = System.getProperty("javax.net.ssl.trustStorePassword").toCharArray();
- if (MIN_KS_LENGTH > tsPwd.length) {
- enableOfTls = TLS_DISABLED;
- return;
- }
- }
- }
-
private void initSsl() {
try {
TrustManagerFactory tmFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
KeyStore ts = KeyStore.getInstance("JKS");
- ts.load(new FileInputStream(tsLocation), tsPwd);
+ ts.load(new FileInputStream(tlsParams.tsLocation), tlsParams.tsPwd());
tmFactory.init(ts);
KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
keyStore = KeyStore.getInstance("JKS");
- keyStore.load(new FileInputStream(ksLocation), ksPwd);
- kmf.init(keyStore, ksPwd);
+ keyStore.load(new FileInputStream(tlsParams.ksLocation), tlsParams.ksPwd());
+ kmf.init(keyStore, tlsParams.ksPwd());
sslContext = SSLContext.getInstance("TLS");
sslContext.init(kmf.getKeyManagers(), tmFactory.getTrustManagers(), null);
@@ -283,28 +462,32 @@
}
public boolean isValidCertificate(Long dpid, Certificate peerCert) {
- if (netCfgService == null) {
- // netcfg service not available; accept any cert
+ if (!tlsParams.isTlsEnabled()) {
return true;
}
+ if (netCfgService == null) {
+ // netcfg service not available; accept any cert if not in strict mode
+ return tlsParams.mode == TlsMode.ENABLED;
+ }
+
DeviceId deviceId = DeviceId.deviceId(Dpid.uri(new Dpid(dpid)));
OpenFlowDeviceConfig config =
netCfgService.getConfig(deviceId, OpenFlowDeviceConfig.class);
if (config == null) {
- // Config not set for device, accept any certificate
- return true;
+ // Config not set for device, accept any cert if not in strict mode
+ return tlsParams.mode == TlsMode.ENABLED;
}
Optional<String> alias = config.keyAlias();
if (!alias.isPresent()) {
- // Config for device does not specify a certificate chain, accept any cert
- return true;
+ // Config for device does not specify a certificate chain, accept any cert if not in strict mode
+ return tlsParams.mode == TlsMode.ENABLED;
}
try {
Certificate configuredCert = keyStore.getCertificate(alias.get());
- //FIXME there's probably a better way to compare these
+ //TODO there's probably a better way to compare these
return Objects.deepEquals(peerCert, configuredCert);
} catch (KeyStoreException e) {
log.info("failed to load key", e);
@@ -366,13 +549,16 @@
this.driverService = driverService;
this.netCfgService = netCfgService;
this.init();
- this.run();
+ this.addListeningPorts(this.openFlowPorts);
}
public void stop() {
log.info("Stopping OpenFlow IO");
- cg.close();
+ if (cg != null) {
+ cg.close();
+ cg = null;
+ }
// Shut down all event loops to terminate all threads.
bossGroup.shutdownGracefully();
@@ -388,4 +574,12 @@
}
}
+ private void restart() {
+ // only restart if we are already running
+ if (cg != null) {
+ stop();
+ start(this.agent, this.driverService, this.netCfgService);
+ }
+ }
+
}
diff --git a/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OpenFlowControllerImpl.java b/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OpenFlowControllerImpl.java
index 9113e5c..6224842 100644
--- a/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OpenFlowControllerImpl.java
+++ b/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OpenFlowControllerImpl.java
@@ -123,6 +123,26 @@
label = "Number of controller worker threads")
private int workerThreads = DEFAULT_WORKER_THREADS;
+ @Property(name = "tlsMode", value = "",
+ label = "TLS mode for OpenFlow channel; options are: disabled [default], enabled, strict")
+ private String tlsModeString;
+
+ @Property(name = "keyStore", value = "",
+ label = "File path to key store for TLS connections")
+ private String keyStore;
+
+ @Property(name = "keyStorePassword", value = "",
+ label = "Key store password")
+ private String keyStorePassword;
+
+ @Property(name = "trustStore", value = "",
+ label = "File path to trust store for TLS connections")
+ private String trustStore;
+
+ @Property(name = "trustStorePassword", value = "",
+ label = "Trust store password")
+ private String trustStorePassword;
+
protected ExecutorService executorMsgs =
Executors.newFixedThreadPool(32, groupedThreads("onos/of", "event-stats-%d", log));
@@ -200,6 +220,7 @@
netCfgService.registerConfigFactory(factory);
ctrl.setConfigParams(context.getProperties());
ctrl.start(agent, driverService, netCfgService);
+ // TODO register a netcfg listener that disconnects switches when the keyAlias changes
}
private void cleanup() {
@@ -221,9 +242,9 @@
@Modified
public void modified(ComponentContext context) {
- ctrl.stop();
+ //ctrl.stop();
ctrl.setConfigParams(context.getProperties());
- ctrl.start(agent, driverService, netCfgService);
+ //ctrl.start(agent, driverService, netCfgService);
}
@Override
diff --git a/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/controller/impl/ControllerTest.java b/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/controller/impl/ControllerTest.java
index 335ebfb..8928976 100644
--- a/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/controller/impl/ControllerTest.java
+++ b/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/controller/impl/ControllerTest.java
@@ -129,7 +129,7 @@
*/
@Test
public void switchInstanceNotFoundTest() {
- controller.start(null, new MockDriverService());
+ controller.start(null, new MockDriverService(), null);
OpenFlowSwitchDriver driver =
controller.getOFSwitchInstance(MockDriverService.NO_SUCH_DRIVER_ID,
null,
@@ -143,7 +143,7 @@
*/
@Test
public void switchItemNotFoundTest() {
- controller.start(null, new MockDriverService());
+ controller.start(null, new MockDriverService(), null);
OFDescStatsReply stats =
new OFDescStatsReplyAdapter();
OpenFlowSwitchDriver driver =
@@ -159,7 +159,7 @@
*/
@Test
public void driverExistsTest() {
- controller.start(null, new MockDriverService());
+ controller.start(null, new MockDriverService(), null);
OFDescStatsReply stats =
new OFDescStatsReplyAdapter();
OpenFlowSwitchDriver driver =
@@ -204,7 +204,7 @@
properties.put("workerThreads", "0");
controller.setConfigParams(properties);
- controller.start(null, new MockDriverService());
+ controller.start(null, new MockDriverService(), null);
assertThat(controller.sslContext, notNullValue());