MYRIAD-244 Add https/ssl support for Myriad REST APIs and UI by yufeldman · Pull Request #95 · apache/incubator-myriad

yufeldman · 2016-10-31T00:24:35Z

This is a PR to cover enabling ssl for Myriad UI and REST APIs. It heavily relies on Hadoop/YARN since it is plugin to RM anyway.

DarinJ · 2016-11-03T23:59:51Z

myriad-scheduler/src/main/java/org/apache/myriad/webapp/HttpConnectorProvider.java

+    }
+
    ret.setName("Myriad");
    ret.setHost("0.0.0.0");


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

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

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).

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.

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

DarinJ · 2016-11-04T00:05:30Z

myriad-scheduler/src/main/java/org/apache/myriad/webapp/HttpConnectorProvider.java

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


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?

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

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.

DarinJ · 2016-11-04T00:09:47Z

myriad-scheduler/src/test/resources/myriad-config-test-default.yml

 mesosMaster: 10.0.2.15:5050
 haEnabled:  false
 checkpoint: false
+isSSLEnabled: false


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

+1 to documenting new config options

DarinJ · 2016-11-04T00:22:18Z

myriad-scheduler/src/test/resources/ssl-server.xml

@@ -0,0 +1,63 @@
+<?xml version="1.0"?>


should this have an Apache Header?

Most poms have:

DarinJ · 2016-11-04T00:22:38Z

myriad-scheduler/src/test/java/org/apache/myriad/webapp/MyriadWebServerTest.java

@@ -18,26 +18,84 @@
 package org.apache.myriad.webapp;


Apache header

It does have header, no?

My bad, I'm blaming github.

DarinJ · 2016-11-04T00:29:48Z

Again minor nits, I think the headers have to be resolved before the merge.

yufeldman · 2016-11-04T00:37:11Z

Will update PR with headers and other comments

yufeldman · 2016-11-08T19:41:17Z

@DarinJ - wonder if you have more comments to this PR?

hokiegeek2 · 2016-11-13T11:31:40Z

myriad-scheduler/src/main/java/org/apache/myriad/webapp/HttpConnectorProvider.java

+
+      String trustStore = sslConf.get("ssl.server.truststore.location");
+
+      if (trustStore != null) {


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

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

hokiegeek2 · 2016-11-13T11:32:09Z

myriad-scheduler/src/main/java/org/apache/myriad/webapp/HttpConnectorProvider.java

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


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

hokiegeek2 · 2016-11-13T11:32:52Z

myriad-scheduler/src/main/java/org/apache/myriad/webapp/HttpConnectorProvider.java

+        password = new String(passchars);
+      }
+    } catch (IOException ioe) {
+      password = null;


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

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

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

I agree with not duplicating parameters.

DarinJ · 2017-01-03T17:02:50Z

Tested, a few caveats with self-signed certs and mesos but I think we should merge.

adam-mesos

ASF doesn't like owning binaries (like ssl_keystore and ssl_truststore). Is there any way these can be generated or retrieved as part of the build process?
A few minor nits beyond that, and a +1 on adding docs.

adam-mesos · 2017-02-07T07:32:09Z

myriad-scheduler/src/main/java/org/apache/myriad/configuration/MyriadConfiguration.java


+  public Boolean isSSLEnabled() {
+    return Optional.fromNullable(isSSLEnabled).or(DEFAULT_SSL_ENABLED);
+  }


nit: needs blank line after

adam-mesos · 2017-02-07T07:43:10Z

myriad-scheduler/src/main/java/org/apache/myriad/webapp/HttpConnectorProvider.java

+      password = null;
+    }
+    if (password == null) {
+      LOGGER.warn("No password found in config as property: {}. If you don't want prompted please set ...", alias);


"If you don't want to be prompted..."

adam-mesos · 2017-02-07T07:46:50Z

myriad-scheduler/src/test/resources/myriad-config-test-default.yml

 mesosMaster: 10.0.2.15:5050
 haEnabled:  false
 checkpoint: false
+isSSLEnabled: false


+1 to documenting new config options

adam-mesos · 2017-02-07T07:48:12Z

myriad-scheduler/src/test/resources/ssl-server.xml

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


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

adam-mesos · 2017-02-07T07:48:52Z

myriad-scheduler/src/test/resources/ssl-server.xml

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


Is the default "" or "myriad"?

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

84eb26b

DarinJ reviewed Nov 3, 2016

View reviewed changes

DarinJ reviewed Nov 4, 2016

View reviewed changes

MYRIAD-244 Addressing review comments

a8c522c

hokiegeek2 reviewed Nov 13, 2016

View reviewed changes

adam-mesos reviewed Feb 7, 2017

View reviewed changes


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

		if (trustStore != null) {

Conversation

yufeldman commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarinJ commented Nov 4, 2016

Uh oh!

yufeldman commented Nov 4, 2016

Uh oh!

yufeldman commented Nov 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarinJ commented Jan 3, 2017

Uh oh!

adam-mesos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants