https between components #7767

adamgoucher · 2019-11-10T18:56:56Z

Alright. This is for #7662

Essentially there is a big on/off flag (--https) and then you need to provide a certificate (--https-certificate) and a key (--https-private-key). There is also an info section on how to wire it up including making yourself some certificates and how to teach Java to trust them.

Not sure how to write tests as it requires certificates, but can confirm I have locally been able to light up a browser and drive it.

This does not do the follow, each will have to be a separate item

secure zeromq which is still plain tcp
teach the client bindings about certificates (my tests were with the python ones and they just work by saying the remote_connection is https but it spits out a warning about verifying certificates on every communication over the wire)
By placing an X in the preceding checkbox, I verify that I have signed the Contributor License Agreement

shs96c

Looks like an excellent start. Thank you!

shs96c · 2019-11-19T16:05:00Z

java/server/src/org/openqa/selenium/grid/commands/security.txt

+Selenium Grid by default communicates over HTTP. This is fine for a lot
+of use cases, especially if everything is contained within the firewall
+and against test sites with testing data. However, if your server is
+exposed to the Internet or us being used in environments with production


shs96c · 2019-11-19T16:06:21Z

java/server/src/org/openqa/selenium/grid/commands/security.txt

+create and sign your certificates.
+
+```
+  ~/go/bin/minica --domains sessions.grid.com,distributor.grid.com,router.grid.com


Assume commands are on the PATH already. If someone's installed minica via (eg) homebrew, the location will be different.

shs96c · 2019-11-19T16:08:51Z

java/server/src/org/openqa/selenium/grid/commands/security.txt

+      -import \
+      -file /path/to/minica.pem \
+      -alias minica \
+      -keystore /Library/Java/JavaVirtualMachines/jdk-11.jdk/Contents/Home/lib/security/cacerts \


$JAVA_HOME, perhaps?

shs96c · 2019-11-19T16:09:34Z

java/server/src/org/openqa/selenium/grid/server/BaseServerFlags.java

@@ -53,6 +53,18 @@
  @ConfigValue(section = "server", name = "allow-cors")
  private boolean allowCORS = false;

+  @Parameter(description = "Whether the Selenium server should communicate over https", names = "--https")


It would seem that having either --http-private-key or --http-certificate implies that the connections should be secured....

java/server/src/org/openqa/selenium/grid/server/BaseServerOptions.java

shs96c · 2019-11-19T16:15:54Z

java/server/src/org/openqa/selenium/netty/server/NettyServer.java

+      b.group(bossGroup, workerGroup)
+        .channel(NioServerSocketChannel.class)
+        .handler(new LoggingHandler(LogLevel.INFO))
+        .childHandler(new SeleniumHttpsInitializer(handler, sslCtx));  


Why not just modify the existing SeleniumHttpInitializer to accept a null sslCtx and have it only add the SSL handler if the value is not null?

shs96c · 2019-11-19T16:18:27Z

java/server/src/org/openqa/selenium/netty/server/SeleniumHttpsInitializer.java

+  @Override
+  protected void initChannel(SocketChannel ch) {
+    ch.pipeline().addLast("ssl", sslCtx.newHandler(ch.alloc()));
+    ch.pipeline().addLast("codec", new HttpServerCodec());


Is this the same as the SeleniumHttpInitializer? If so, and you're not following the suggestion from above, consider something like:

ch.pipeline().addLast("ssl", sslCtx.newHandler(ch.alloc())); new SeleniumHttpInitializer(seleniumHandler).initChannel(ch);

to minimise the amount of duplicated code, and to highlight the relationship between the two.

shs96c · 2019-11-19T16:20:09Z

java/server/test/org/openqa/selenium/grid/router/EndToEndTest.java

@@ -94,7 +97,7 @@
        () -> {
          try {
            return createRemotes();
-          } catch (URISyntaxException e) {
+          } catch (URISyntaxException | SSLException | CertificateException e) {


Nit please alphabetise.

shs96c · 2019-11-19T16:20:26Z

java/server/test/org/openqa/selenium/grid/router/EndToEndTest.java

@@ -121,7 +124,7 @@ public void setFields() {
    this.clientFactory = (HttpClient.Factory) raw[1];
  }

-  private static Object[] createInMemory() throws URISyntaxException, MalformedURLException {
+  private static Object[] createInMemory() throws URISyntaxException, MalformedURLException, SSLException, CertificateException {


At this point, throw Exception is fine. It's just a test.

CLAassistant · 2019-11-23T16:51:56Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Adam Goucher seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

shs96c

Looks like it's basically ready. I've left a few comments, but you should still have the commit bit. Land it when you feel good to go :) We can always iterate on this once it's in the tree.

shs96c · 2019-12-02T23:01:11Z

java/server/src/org/openqa/selenium/grid/server/BaseServerOptions.java

+	return config.get("server", "https-private-key").isPresent() && config.get("server", "https-certificate").isPresent();
+  }
+
+  public File getPrivateKey() {


In a follow-up PR, we should make this a Path

shs96c · 2019-12-02T23:03:04Z

java/server/src/org/openqa/selenium/netty/server/NettyServer.java

    Objects.requireNonNull(options, "Server options must be set.");
    Objects.requireNonNull(handler, "Handler to use must be set.");

+    Boolean secure = options.isSecure();


boolean: we never expect this to be null, and the unboxing that happens on the next line will throw an NPE if that happens.

I think the current implementation of isSecure ensures it always is Boolean.

shs96c · 2019-12-02T23:03:41Z

java/server/src/org/openqa/selenium/netty/server/SeleniumHttpInitializer.java

  }

  @Override
  protected void initChannel(SocketChannel ch) {
+  	if (sslCtx != null) {
+	  ch.pipeline().addLast("ssl", sslCtx.newHandler(ch.alloc()));
+	}


Nit: weird indentation.

shs96c · 2019-12-02T23:04:21Z

java/server/src/org/openqa/selenium/netty/server/SeleniumHttpsInitializer.java

@@ -0,0 +1,53 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one


This file can be deleted now.

shs96c · 2019-12-02T23:05:00Z

java/server/src/org/openqa/selenium/grid/server/BaseServerOptions.java

@@ -96,4 +97,24 @@ public URI getExternalUri() {
  public boolean getAllowCORS() {
    return config.getBool("server", "allow-cors").orElse(false);
  }
+
+  public boolean isSecure() {
+	return config.get("server", "https-private-key").isPresent() && config.get("server", "https-certificate").isPresent();


Nit: indentation

shs96c · 2019-12-02T23:06:41Z

java/server/src/org/openqa/selenium/netty/server/NettyServer.java


  private Channel channel;

-  public NettyServer(BaseServerOptions options, HttpHandler handler) {
+  public NettyServer(BaseServerOptions options, HttpHandler handler) 
+    throws SSLException, CertificateException {


Consider wrapping the SSLException with an UncheckedIOException so that everything coming out of this is a RuntimeException. Our own code wraps and rethrows, so it's not like we're hiding much.

Merging in support to run the standalone server and/or component parts of the server as https. Default is still http due to configuration pain. Note: none of the client bindings have been updated to communicate to a https server

Adam Goucher added 6 commits October 22, 2019 10:24

self-signed certificate https

fd5b78e

Merge branch 'master' of github.com:SeleniumHQ/selenium into https

207f536

provide certificate and key for https establishment

0c5560b

Merge branch 'master' of github.com:SeleniumHQ/selenium into https

7076275

Merge branch 'master' of github.com:SeleniumHQ/selenium into https

88606db

some documentation

7cff794

adamgoucher requested a review from shs96c November 10, 2019 18:56

fixing some tests

e8fd76f

shs96c reviewed Nov 19, 2019

View reviewed changes

Adam Goucher added 2 commits December 1, 2019 13:02

Merge branch 'master' of github.com:SeleniumHQ/selenium into https

015d87d

updates from pr review

fb48b81

shs96c approved these changes Dec 2, 2019

View reviewed changes

Adam Goucher added 3 commits December 3, 2019 06:58

updating https support with latest feedback

31b9521

Merge branch 'master' of github.com:SeleniumHQ/selenium into https

54c59a6

right. 'build grid' is not the same as 'build //java/...'

a790187

adamgoucher merged commit ee15b26 into SeleniumHQ:master Dec 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

https between components #7767

https between components #7767

adamgoucher commented Nov 10, 2019

shs96c left a comment

shs96c Nov 19, 2019

shs96c Nov 19, 2019

shs96c Nov 19, 2019

shs96c Nov 19, 2019

shs96c Nov 19, 2019

shs96c Nov 19, 2019

shs96c Nov 19, 2019

shs96c Nov 19, 2019

CLAassistant commented Nov 23, 2019 •

edited

shs96c left a comment

shs96c Dec 2, 2019

shs96c Dec 2, 2019

adamgoucher Dec 3, 2019

shs96c Dec 2, 2019

shs96c Dec 2, 2019

shs96c Dec 2, 2019

shs96c Dec 2, 2019

		@@ -0,0 +1,53 @@
		// Licensed to the Software Freedom Conservancy (SFC) under one

https between components #7767

https between components #7767

Conversation

adamgoucher commented Nov 10, 2019

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Nov 23, 2019 • edited

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Nov 23, 2019 •

edited