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.

Sign up for GitHub

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

MYRIAD-244 Add https/ssl support for Myriad REST APIs and UI #95

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

MYRIAD-244 Add https/ssl support for Myriad REST APIs and UI #95

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84eb26b
MYRIAD-244 Add https/ssl support for Myriad REST APIs and UI
Oct 11, 2016
a8c522c
MYRIAD-244 Addressing review comments
Nov 4, 2016
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
7 changes: 7 additions & 0 deletions myriad-scheduler/src/main/java/org/apache/myriad/configuration/MyriadConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ public class MyriadConfiguration {
*/
public static final Boolean DEFAULT_HA_ENABLED = false;

public static final Boolean DEFAULT_SSL_ENABLED = false;
/**
* By default framework failover timeout is 1 day.
*/
Expand Down Expand Up @@ -157,6 +158,9 @@ public class MyriadConfiguration {
@JsonProperty
private Boolean haEnabled;

@JsonProperty
private Boolean isSSLEnabled;

@JsonProperty
private NodeManagerConfiguration nodemanager;

Expand Down Expand Up @@ -253,6 +257,9 @@ public Boolean isHAEnabled() {
return Optional.fromNullable(haEnabled).or(DEFAULT_HA_ENABLED);
}

public Boolean isSSLEnabled() {
return Optional.fromNullable(isSSLEnabled).or(DEFAULT_SSL_ENABLED);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: needs blank line after

public NodeManagerConfiguration getNodeManagerConfiguration() {
return nodemanager;
}
Expand Down
63 changes: 61 additions & 2 deletions myriad-scheduler/src/main/java/org/apache/myriad/webapp/HttpConnectorProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,30 @@

import com.google.inject.Provider;
import javax.inject.Inject;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.security.ssl.SslSocketConnectorSecure;
import org.apache.hadoop.yarn.conf.YarnConfiguration;
import org.apache.myriad.configuration.MyriadConfiguration;
import org.mortbay.jetty.AbstractConnector;
import org.mortbay.jetty.Connector;
import org.mortbay.jetty.nio.SelectChannelConnector;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;

import static org.apache.hadoop.yarn.webapp.util.WebAppUtils.WEB_APP_KEYSTORE_PASSWORD_KEY;
import static org.apache.hadoop.yarn.webapp.util.WebAppUtils.WEB_APP_KEY_PASSWORD_KEY;
import static org.apache.hadoop.yarn.webapp.util.WebAppUtils.WEB_APP_TRUSTSTORE_PASSWORD_KEY;

/**
* The factory for creating the http connector for the myriad scheduler
*/
public class HttpConnectorProvider implements Provider<Connector> {

private static final Logger LOGGER = LoggerFactory.getLogger(HttpConnectorProvider.class);

private MyriadConfiguration myriadConf;

@Inject
Expand All @@ -38,11 +53,55 @@ public HttpConnectorProvider(MyriadConfiguration myriadConf) {

@Override
public Connector get() {
SelectChannelConnector ret = new SelectChannelConnector();
final AbstractConnector ret;
if (!myriadConf.isSSLEnabled()) {
final SelectChannelConnector listener = new SelectChannelConnector();
ret = listener;
} else {
final Configuration sslConf = new Configuration(false);
boolean needsClientAuth = YarnConfiguration.YARN_SSL_CLIENT_HTTPS_NEED_AUTH_DEFAULT;
sslConf.addResource(YarnConfiguration.YARN_SSL_SERVER_RESOURCE_DEFAULT);

final SslSocketConnectorSecure listener = new SslSocketConnectorSecure();
listener.setHeaderBufferSize(1024 * 64);
listener.setNeedClientAuth(needsClientAuth);
listener.setKeyPassword(getPassword(sslConf, WEB_APP_KEY_PASSWORD_KEY));

String keyStore = sslConf.get("ssl.server.keystore.location");

if (keyStore != null) {
listener.setKeystore(keyStore);
listener.setKeystoreType(sslConf.get("ssl.server.keystore.type", "jks"));
listener.setPassword(getPassword(sslConf, WEB_APP_KEYSTORE_PASSWORD_KEY));
Copy link
Contributor

Choose a reason for hiding this comment

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

getPassword has the potential to return null, how does listener.setPassword handle this? Should we throw a more informative exception earlier or pass a sane value?

Copy link
Author

Choose a reason for hiding this comment

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

It is true that if password is null it will actually prompt for password, so we rely on Hadoop configuration to supply it. We could certainly give at least a warning on what may happen

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth a log.warn("No password found in config if you don't want prompted please set ..."), would help debugging if running the RM in Marathon.

}

String trustStore = sslConf.get("ssl.server.truststore.location");

if (trustStore != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a config parameter to check as opposed to a null check? I think the former is a better design as the user will have to explicitly state whether he/she wants to enable SSL authentication and we'll perform checks based upon that as opposed to checking for null

Copy link
Author

Choose a reason for hiding this comment

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

we do have config param to enable SSL and this code is executed only is check is satisfied. sslConf is hadoop Configuration object, not a Myriad one so it will be as good as turning on SSL in RM UI/REST

listener.setTruststore(trustStore);
listener.setTruststoreType(sslConf.get("ssl.server.truststore.type", "jks"));
listener.setTrustPassword(getPassword(sslConf, WEB_APP_TRUSTSTORE_PASSWORD_KEY));
}
ret = listener;
}

ret.setName("Myriad");
ret.setHost("0.0.0.0");
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to be careful here, some people may prefer to only bind to one interface.

Copy link
Author

Choose a reason for hiding this comment

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

Could you elaborate - I did not quite understand your comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the case of machines with multiple network interfaces each will have a separate ip address, it's sometimes preferable to only bind to only one ip. Cases include: 1 ip is used to admin, 1 nic is public, etc. I think it's fine for now but we may wish to revisit (I think I'd like it to bind to exactly the same ip(s) as the resourcemanager, though others may disagree).

Copy link
Author

Choose a reason for hiding this comment

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

Aah, I was thinking you were commenting on my changes. Yes - we could bind it on RM host/ip - I think it is actually host not IP though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I noticed it without paying attention to when it was created. Let's make a mental note but shelve for now.

ret.setPort(myriadConf.getRestApiPort());

return ret;
}

static String getPassword(Configuration conf, String alias) {
String password = null;
try {
char[] passchars = conf.getPassword(alias);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a config parameter to check as opposed to a null check? I think the former is a better design as the user will have to explicitly state whether he/she wants to enable SSL authentication and we'll perform checks based upon that as opposed to checking for null

if (passchars != null) {
password = new String(passchars);
}
} catch (IOException ioe) {
password = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is returning a null password going to cause a difficult-to-debug NPE elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of @hokiegeek2 suggestions could be alleviated with guava's optional.

Copy link
Author

Choose a reason for hiding this comment

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

Those params are hadoop Configuration params, not Myriad ones and I don't think we should duplicate them in Myriad configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with not duplicating parameters.

}
return password;
}

}
62 changes: 62 additions & 0 deletions myriad-scheduler/src/test/java/org/apache/myriad/webapp/HttpConnectorProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,21 @@
*/
package org.apache.myriad.webapp;

import static junit.framework.TestCase.assertNotNull;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

import org.apache.hadoop.security.ssl.SslSocketConnectorSecure;
import org.apache.myriad.BaseConfigurableTest;
import org.apache.myriad.configuration.MyriadConfiguration;
import org.junit.Test;
import org.mortbay.jetty.Connector;

import java.io.FileOutputStream;
import java.io.InputStream;
import java.io.OutputStream;
import java.lang.reflect.Field;

/**
* Unit tests for HttpConnectionProvider
*/
Expand All @@ -36,4 +45,57 @@ public void testConnector() throws Exception {
assertEquals("0.0.0.0", connector.getHost());
assertEquals("Myriad", connector.getName());
}

@Test
public void testConnectorSSLOn() throws Exception {
Field[] fields = MyriadConfiguration.class.getDeclaredFields();
for (Field field : fields) {
if ("isSSLEnabled".equalsIgnoreCase(field.getName())) {
field.setAccessible(true);
field.set(cfg, true);
break;
}
}
assertTrue(cfg.isSSLEnabled());

try (InputStream keystore = MyriadWebServerTest.class.getResourceAsStream("/ssl_keystore")) {
try (InputStream truststore = MyriadWebServerTest.class.getResourceAsStream("/ssl_truststore")) {
if (keystore != null && truststore != null) {
try (OutputStream keyStoreOS = new FileOutputStream(MyriadWebServerTest.tmpKeystore)) {
byte[] bytes = new byte[1024];
int length = 0;
while ((length = keystore.read(bytes)) != -1) {
keyStoreOS.write(bytes, 0, length);
}
}
try (OutputStream trustStoreOS = new FileOutputStream(MyriadWebServerTest.tmpTruststore)) {
byte[] bytes = new byte[1024];
int length = 0;
while ((length = truststore.read(bytes)) != -1) {
trustStoreOS.write(bytes, 0, length);
}
}
}
}
}
System.setProperty("tmptest.dir", MyriadWebServerTest.tmpDir);
try {
HttpConnectorProvider provider = new HttpConnectorProvider(cfg);
Connector connector = provider.get();
assertTrue(connector instanceof SslSocketConnectorSecure);
SslSocketConnectorSecure secureConnector = (SslSocketConnectorSecure) connector;
assertNotNull(secureConnector.getKeystore());
assertEquals(secureConnector.getKeystore(), MyriadWebServerTest.tmpDir + "/ssl_keystore");

assertNotNull(secureConnector.getTruststore());
assertEquals(secureConnector.getTruststore(), MyriadWebServerTest.tmpDir + "/ssl_truststore");

assertEquals(8192, connector.getPort());
assertEquals("0.0.0.0", connector.getHost());
assertEquals("Myriad", connector.getName());
} finally {
assertTrue(MyriadWebServerTest.tmpKeystore.delete());
assertTrue(MyriadWebServerTest.tmpTruststore.delete());
}
}
}
60 changes: 59 additions & 1 deletion myriad-scheduler/src/test/java/org/apache/myriad/webapp/MyriadWebServerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,26 +18,84 @@
package org.apache.myriad.webapp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Apache header

Copy link
Author

Choose a reason for hiding this comment

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

It does have header, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I'm blaming github.


import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

import org.apache.myriad.BaseConfigurableTest;
import org.apache.myriad.TestObjectFactory;
import org.apache.myriad.configuration.MyriadConfiguration;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

import java.io.File;
import java.io.FileOutputStream;
import java.io.InputStream;
import java.io.OutputStream;
import java.lang.reflect.Field;

/**
* Unit test cases for MyriadWebServer class
*/
public class MyriadWebServerTest extends BaseConfigurableTest {
MyriadWebServer webServer;

static String tmpDir = System.getProperty("java.io.tmpdir");
static File tmpKeystore = new File(tmpDir, "ssl_keystore");
static File tmpTruststore = new File(tmpDir, "ssl_truststore");

@Before
public void setUp() throws Exception {
super.setUp();
webServer = TestObjectFactory.getMyriadWebServer(cfg);

try (InputStream keystore = MyriadWebServerTest.class.getResourceAsStream("/ssl_keystore")) {
try (InputStream truststore = MyriadWebServerTest.class.getResourceAsStream("/ssl_truststore")) {
if (keystore != null && truststore != null) {
try (OutputStream keyStoreOS = new FileOutputStream(tmpKeystore)) {
byte[] bytes = new byte[1024];
int length = 0;
while ((length = keystore.read(bytes)) != -1) {
keyStoreOS.write(bytes, 0, length);
}
}
try (OutputStream trustStoreOS = new FileOutputStream(tmpTruststore)) {
byte[] bytes = new byte[1024];
int length = 0;
while ((length = truststore.read(bytes)) != -1) {
trustStoreOS.write(bytes, 0, length);
}
}
}
}
}
System.setProperty("tmptest.dir", tmpDir);
}

@After
public void tearDown() throws Exception {
assertTrue(tmpKeystore.delete());
assertTrue(tmpTruststore.delete());
}
@Test
public void testStartStopMyriadWebServer() throws Exception {
webServerStartStop();
}

@Test
public void testsStartStopMyriadWebServerWithSSL() throws Exception {
Field[] fields = MyriadConfiguration.class.getDeclaredFields();
for (Field field : fields) {
if ("isSSLEnabled".equalsIgnoreCase(field.getName())) {
field.setAccessible(true);
field.set(cfg, true);
break;
}
}
assertTrue(cfg.isSSLEnabled());
webServerStartStop();
}

private void webServerStartStop() throws Exception {
webServer = TestObjectFactory.getMyriadWebServer(cfg);
webServer.start();
assertEquals(MyriadWebServer.Status.STARTED, webServer.getStatus());
webServer.stop();
Expand Down
1 change: 1 addition & 0 deletions myriad-scheduler/src/test/resources/myriad-config-test-default.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ servedBinaryPath: /tmp/myriadBinary
mesosMaster: 10.0.2.15:5050
haEnabled: false
checkpoint: false
isSSLEnabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Add to src/main/resources/myriad-default.conf as minimal documentaion.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to documenting new config options

frameworkFailoverTimeout: 44200000
frameworkName: MyriadTest
frameworkRole: "*"
Expand Down
63 changes: 63 additions & 0 deletions myriad-scheduler/src/test/resources/ssl-server.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?xml version="1.0"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

should this have an Apache Header?

Copy link
Contributor

Choose a reason for hiding this comment

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

Most poms have:

<!-- ~ Licensed to the Apache Software Foundation (ASF) under one or more 
    ~ contributor license agreements. See the NOTICE file distributed with ~ 
    this work for additional information regarding copyright ownership. ~ The 
    ASF licenses this file to You under the Apache License, Version 2.0 ~ (the 
    "License"); you may not use this file except in compliance with ~ the License. 
    You may obtain a copy of the License at ~ ~ http://www.apache.org/licenses/LICENSE-2.0 
    ~ ~ Unless required by applicable law or agreed to in writing, software ~ 
    distributed under the License is distributed on an "AS IS" BASIS, ~ WITHOUT 
    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. ~ See the 
    License for the specific language governing permissions and ~ limitations 
    under the License. -->

<?xml-stylesheet type="text/xsl" href="configuration.xsl"?>

<configuration>

<property>
<name>ssl.server.truststore.location</name>
<value>${tmptest.dir}/ssl_truststore</value>
<description>Truststore to be used by webservers. Must be specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "Must be specified", how about "Required"?

</description>
</property>

<property>
<name>ssl.server.truststore.password</name>
<value>myriad</value>
<description>Optional. Default value is "".
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the default "" or "myriad"?

</description>
</property>

<property>
<name>ssl.server.truststore.type</name>
<value>jks</value>
<description>Optional. The keystore file format, default value is "jks".
</description>
</property>

<property>
<name>ssl.server.truststore.reload.interval</name>
<value>10000</value>
<description>Truststore reload check interval, in milliseconds.
Default value is 10000 (10 seconds).
</description>
</property>

<property>
<name>ssl.server.keystore.location</name>
<value>${tmptest.dir}/ssl_keystore</value>
<description>Keystore to be used by webservers. Must be specified.
</description>
</property>

<property>
<name>ssl.server.keystore.password</name>
<value>myriad</value>
<description>Must be specified.
</description>
</property>

<property>
<name>ssl.server.keystore.keypassword</name>
<value>myriad</value>
<description>Must be specified.
</description>
</property>

<property>
<name>ssl.server.keystore.type</name>
<value>jks</value>
<description>Optional. The keystore file format, default value is "jks".
</description>
</property>

</configuration>
Binary file added myriad-scheduler/src/test/resources/ssl_keystore
View file
Open in desktop
Binary file not shown.
Binary file added myriad-scheduler/src/test/resources/ssl_truststore
View file
Open in desktop
Binary file not shown.