#67 add TLS support #80

j3t · 2021-07-13T12:17:41Z

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

I have added TLS support so that NomadServer which are secured by client/server certificates can used as well.
corresponding Issue: #67

j3t · 2021-07-14T11:51:55Z

Could please one of the maintainers review this PR? BTW, I cannot link the corresponding issue, could you please take care of that as well?

multani · 2021-07-14T11:54:05Z

Hi @j3t!
Thanks a lot for the PR, I'll take a look in the coming days 👋

j3t · 2021-07-15T09:17:18Z

Thanks @multani! I have added some more tests.

j3t · 2021-07-15T09:29:41Z

src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java

@@ -335,7 +399,8 @@ public Node call() throws Exception {

            // Support for Jenkins security
            String jnlpSecret = "";
-            if (Jenkins.get().isUseSecurity()) {
+            if (Jenkins.get().isUseSecurity() || template.isDockerDriver()) {


I'm not sure about this condition at all. It depends very much on how the agent.jar is executed. When java -cp is used then the secret is mandatory but when -jar is used then it's optional I guess. It probably makes sense to refactor this part in the future so that either the cp or the jar approach is used but not both.

I don't see how this is relevant to adding TLS to talk with the Nomad agent though?

Can you simply remove this and address it in a different issue?

src/main/java/org/jenkinsci/plugins/nomad/NomadApi.java

pom.xml

multani

@j3t Thanks for the PR!

Disclaimer: I'm not familiar at all with the Java PKI APIs.

I had a look and tested locally, the only issue I found was the "Test connection" in doTestConnection failed to report the TLS connection couldn't be established. Could you update it to return something useful if the plugin , without TLS correctly configured, tries to connect to a TLS-enabled Nomad server?

I quickly hacked something like this:

                Response response = clientBuilder.build().newCall(request).execute();
                ResponseBody body = response.body();
                String content ="";
                if (body != null) {
                    content = body.string();
                    body.close();
                }

                if (response.code() != 200) {
                    throw new Exception("HTTP Status " + response.code() + ": " + content);
                }

Apart from that, if I understood your PR correctly, you made the client certificate mandatory but not the server certificate / CA.
Also, not having the server certificate/CA doesn't validate the server identity, right?

If this is the case, you should flip around the setup you proposed then:

The server certificate must be fully validated, instead of being optional. I think this one is the most important: if you bother to build up a PKI and enable TLS on Nomad, you must have a way to validate the client->server TLS communications.
The client certificate should be optional: verification of the client certificate can be disabled on the server's side, so this is not strictly mandatory. It's pretty cool that you build it up in the PR in the first place, so keep it 👍

Also, I'd like the README to be extended a bit in the future (it's almost useless as it is, and it's actually not easy to get started with the plugin). Would you mind adding a small section on what would be possible when this gets merged and how to configure TLS with the plugin?

multani · 2021-07-24T17:46:17Z

src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java

@@ -335,7 +399,8 @@ public Node call() throws Exception {

            // Support for Jenkins security
            String jnlpSecret = "";
-            if (Jenkins.get().isUseSecurity()) {
+            if (Jenkins.get().isUseSecurity() || template.isDockerDriver()) {


I don't see how this is relevant to adding TLS to talk with the Nomad agent though?

Can you simply remove this and address it in a different issue?

src/main/java/org/jenkinsci/plugins/nomad/OkHttpClientHelper.java

multani · 2021-07-24T17:50:35Z

src/test/java/org/jenkinsci/plugins/nomad/NomadApiClientTLSTest.java

+    }
+
+    /**
+     * Checks that mutual TLS is working as expected (client proves the identity of the server and vise versa).


Suggested change

* Checks that mutual TLS is working as expected (client proves the identity of the server and vise versa).

* Checks that mutual TLS is working as expected (client proves the identity of the server and vice versa).

multani · 2021-07-24T17:51:00Z

src/test/java/org/jenkinsci/plugins/nomad/NomadApiClientTLSTest.java

+    }
+
+    /**
+     * Checks that one way TLS is working as expected (server proves the identity of the client but not vise versa).


Suggested change

* Checks that one way TLS is working as expected (server proves the identity of the client but not vise versa).

* Checks that one way TLS is working as expected (server proves the identity of the client but not vice versa).

Like I said above, not proving the identity of the server is a bit awkward at best: if you rely on TLS to protect your communication but not authenticating who you are talking to, you can send sensitive information to a rogue server that just has TLS enabled.

Actually, if the connection can't fully validate the TLS connection, I would fail the connection altogether.

multani · 2021-07-24T17:51:14Z

src/test/java/org/jenkinsci/plugins/nomad/NomadApiClientTLSTest.java

+    }
+
+    /**
+     * Checks that one way TLS is working as expected (client proves the identity of the server but not vise versa).


Suggested change

* Checks that one way TLS is working as expected (client proves the identity of the server but not vise versa).

* Checks that one way TLS is working as expected (client proves the identity of the server but not vice versa).

multani · 2021-07-24T17:55:24Z

src/main/resources/org/jenkinsci/plugins/nomad/NomadCloud/config.jelly

+        <f:entry title="Client certificate password (optional)" >
+          <f:password field="clientPassword" />
+        </f:entry>
+        <f:entry title="Server certificate path (optional)" description="Path to the PKCS12 server certificate (aka Truststore). Usually a common authority or a public key.">


It should be worth mentioning that not setting this will not validate the certificate from the server.

I'd would even say not having this configure should not even be a valid configuration: if you have TLS enabled, you must validate the full TLS setup, otherwise it's a bit pointless.

What do you think?

In my opinion we have to support the following use cases

mutual TLS (client proves the identity of the server and vice versa)

client auth only (server proves the identity of the client)

server auth only (client proves the identity of the server)

The current solution supports them but is not clear. What is the expectation when TLS is enabled and both certificate fields are empty?

Suggestion: Differentiate between enable TLS (client-auth) and enable TLS (server-auth). Then the certificate is not optional anymore.

The server-auth is only important when the server is using self-signed certificates. Providing a server certificate, overrides JVM's default truststore.

multani · 2021-07-24T17:55:35Z

src/test/java/org/jenkinsci/plugins/nomad/NomadApiClientTLSTest.java

+    }
+
+    /**
+     * Checks that TLS in general is working as expected (client not proves the identity of the server and vise versa).


Suggested change

* Checks that TLS in general is working as expected (client not proves the identity of the server and vise versa).

* Checks that TLS in general is working as expected (client not proves the identity of the server and vice versa).

multani · 2021-07-24T18:15:04Z

src/main/java/org/jenkinsci/plugins/nomad/NomadApi.java

@@ -162,12 +207,12 @@ void stopWorker(String workerName, String nomadToken) {
                args.add(cloud.getJenkinsUrl());
            }

-            if (!cloud.getJenkinsTunnel().isEmpty()) {
+            if (cloud.getJenkinsTunnel() != null && !cloud.getJenkinsTunnel().isEmpty()) {


This is not relevant to your PR.

multani · 2021-07-24T18:15:08Z

src/main/java/org/jenkinsci/plugins/nomad/NomadApi.java

                args.add("-tunnel");
                args.add(cloud.getJenkinsTunnel());
            }

-            if (!template.getRemoteFs().isEmpty()) {
+            if (template.getRemoteFs() != null && !template.getRemoteFs().isEmpty()) {


This is not relevant to your PR.

multani · 2021-07-24T18:15:13Z

src/main/java/org/jenkinsci/plugins/nomad/NomadApi.java

@@ -180,7 +225,7 @@ void stopWorker(String workerName, String nomadToken) {

            String prefixCmd = template.getPrefixCmd();
            // If an addtional command is defined - prepend it to jenkins worker invocation
-            if (!prefixCmd.isEmpty()) {
+            if (prefixCmd != null && !prefixCmd.isEmpty()) {


This is not relevant to your PR.

multani · 2021-07-24T18:18:36Z

src/main/java/org/jenkinsci/plugins/nomad/NomadApi.java

+     * Provides an {@link OkHttpClient} instance. The existing client is reused. <b>Note</b> The client initialization is synchronized
+     * which means this method is thread safe and can be called from different threads in parallel.


I think this is enough:

Suggested change

* Provides an {@link OkHttpClient} instance. The existing client is reused. <b>Note</b> The client initialization is synchronized

* which means this method is thread safe and can be called from different threads in parallel.

* Provides an {@link OkHttpClient} instance. Create a new client or reuse the existing one.

* This method is threadsafe.

src/main/java/org/jenkinsci/plugins/nomad/NomadApi.java

j3t

Thanks for your valuable comments! I have improved the description for the client and server path and I also adjusted the test setup a little bit. Now, tests are separated by the client trust-store (Default CA and Custom CA). I hope that makes things more clear.

j3t · 2021-07-25T08:51:32Z

src/main/resources/org/jenkinsci/plugins/nomad/NomadCloud/config.jelly

+        <f:entry title="Client certificate password (optional)" >
+          <f:password field="clientPassword" />
+        </f:entry>
+        <f:entry title="Server certificate path (optional)" description="Path to the PKCS12 server certificate (aka Truststore). Usually a common authority or a public key.">


In my opinion we have to support the following use cases

mutual TLS (client proves the identity of the server and vice versa)

client auth only (server proves the identity of the client)

server auth only (client proves the identity of the server)

The current solution supports them but is not clear. What is the expectation when TLS is enabled and both certificate fields are empty?

Suggestion: Differentiate between enable TLS (client-auth) and enable TLS (server-auth). Then the certificate is not optional anymore.

j3t · 2021-07-25T09:14:47Z

src/main/resources/org/jenkinsci/plugins/nomad/NomadCloud/config.jelly

+        <f:entry title="Client certificate password (optional)" >
+          <f:password field="clientPassword" />
+        </f:entry>
+        <f:entry title="Server certificate path (optional)" description="Path to the PKCS12 server certificate (aka Truststore). Usually a common authority or a public key.">


The server-auth is only important when the server is using self-signed certificates. Providing a server certificate, overrides JVM's default truststore.

multani

I finally took some time to give it another look. It looks quite cool on my side, there were a few more things to fix and I tested them and comited them in the branch directly (some typos, a security fix to prevent d451234 from happening again, some doc, etc.)

@j3t If you are good with the change, I'm OK to merge this and create a new release 👍

src/main/resources/org/jenkinsci/plugins/nomad/NomadCloud/config.jelly

src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java

src/main/resources/org/jenkinsci/plugins/nomad/NomadCloud/config.jelly

j3t · 2021-09-01T15:08:08Z

Hi @multani, that's great! Unfortunately, I don't have time this week to test it manually but it looks good to me what I have seen so far. So, I would say it's fine. 👍

…executed

j3t · 2021-09-04T15:36:50Z

Just a small adjusted so that NomadApiClientTLSTest is not failing on my local machine and a typo. I have also tested it manually against our Nomad environment. It is working as expected. 👍

multani · 2021-09-04T15:41:43Z

@j3t Awesome!

Let me merge this and prepare a new release just after that 👍 This is a fine addition to the plugin ❤️

multani · 2021-09-04T16:22:31Z

It's out in v0.8.0 🎉

Note that's there some issues with Jenkins infrastructure at the moment, so I haven't been able to release it fully.

multani · 2021-09-09T09:01:05Z

It's now released, it should be available through the Update Center soon.

j3t changed the title ~~Feature 67 add TLS support~~ #67 add TLS support Jul 13, 2021

j3t force-pushed the feature-67-add-tls-support branch 5 times, most recently from 9fe3664 to 21e26e2 Compare July 14, 2021 11:01

multani linked an issue Jul 14, 2021 that may be closed by this pull request

Support configuring certificates for the connection to nomad #67

Closed

j3t force-pushed the feature-67-add-tls-support branch 3 times, most recently from 0780f75 to a4e984e Compare July 15, 2021 09:14

j3t commented Jul 15, 2021

View reviewed changes

src/main/java/org/jenkinsci/plugins/nomad/NomadApi.java Show resolved Hide resolved

j3t commented Jul 15, 2021

View reviewed changes

pom.xml Show resolved Hide resolved

multani requested changes Jul 24, 2021

View reviewed changes

multani reviewed Jul 24, 2021

View reviewed changes

add TLS support

bae6927

j3t force-pushed the feature-67-add-tls-support branch from a4e984e to bae6927 Compare July 25, 2021 10:59

j3t commented Jul 25, 2021

View reviewed changes

multani added 4 commits August 31, 2021 16:02

Convert secret String to Jenkin's Secret object

518d765

Fix typos in the Jelly file, move connection-related things together

3df6d77

Add README

f17c589

Lint the create_certs.sh file

ed71038

multani approved these changes Aug 31, 2021

View reviewed changes

src/main/resources/org/jenkinsci/plugins/nomad/NomadCloud/config.jelly Outdated Show resolved Hide resolved

src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java Outdated Show resolved Hide resolved

multani reviewed Aug 31, 2021

View reviewed changes

src/main/resources/org/jenkinsci/plugins/nomad/NomadCloud/config.jelly Show resolved Hide resolved

multani closed this Sep 1, 2021

multani reopened this Sep 1, 2021

Jens Thielscher added 2 commits September 4, 2021 17:31

fix that Jenkins.instance is missing when NomadApiClientTLSTest gets …

d706e1c

…executed

fix typo

2d01a2a

multani merged commit f245444 into jenkinsci:master Sep 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#67 add TLS support #80

#67 add TLS support #80

j3t commented Jul 13, 2021 •

edited

Loading

j3t commented Jul 14, 2021

multani commented Jul 14, 2021

j3t commented Jul 15, 2021

j3t Jul 15, 2021

multani Jul 24, 2021

multani left a comment

multani Jul 24, 2021

multani Jul 24, 2021

multani Jul 24, 2021

multani Jul 24, 2021

multani Jul 24, 2021

j3t Jul 25, 2021

j3t Jul 25, 2021

multani Jul 24, 2021

multani Jul 24, 2021

multani Jul 24, 2021

multani Jul 24, 2021

multani Jul 24, 2021

j3t left a comment •

edited

Loading

j3t Jul 25, 2021

j3t Jul 25, 2021

multani left a comment

j3t commented Sep 1, 2021

j3t commented Sep 4, 2021

multani commented Sep 4, 2021

multani commented Sep 4, 2021

multani commented Sep 9, 2021

	* Checks that mutual TLS is working as expected (client proves the identity of the server and vise versa).
	* Checks that mutual TLS is working as expected (client proves the identity of the server and vice versa).

	* Checks that one way TLS is working as expected (server proves the identity of the client but not vise versa).
	* Checks that one way TLS is working as expected (server proves the identity of the client but not vice versa).

	* Checks that TLS in general is working as expected (client not proves the identity of the server and vise versa).
	* Checks that TLS in general is working as expected (client not proves the identity of the server and vice versa).

		* Provides an {@link OkHttpClient} instance. The existing client is reused. <b>Note</b> The client initialization is synchronized
		* which means this method is thread safe and can be called from different threads in parallel.

#67 add TLS support #80

#67 add TLS support #80

Conversation

j3t commented Jul 13, 2021 • edited Loading

j3t commented Jul 14, 2021

multani commented Jul 14, 2021

j3t commented Jul 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

multani 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

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

j3t left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

multani left a comment

Choose a reason for hiding this comment

j3t commented Sep 1, 2021

j3t commented Sep 4, 2021

multani commented Sep 4, 2021

multani commented Sep 4, 2021

multani commented Sep 9, 2021

j3t commented Jul 13, 2021 •

edited

Loading

j3t left a comment •

edited

Loading