[JENKINS-57420] - Use f:secretTextarea for client private key (#76)

* Use f:secretTextarea for client private key This updates the form UI for client keys to use the newly introduced f:secretTextarea Jelly tag from Jenkins 2.171. Now the client key can be more securely entered and updated. Signed-off-by: Matt Sicker <boards@gmail.com> * Deprecate old property name and use updated code Signed-off-by: Matt Sicker <boards@gmail.com> * Fix formatting Signed-off-by: Matt Sicker <boards@gmail.com> * Apply suggestions from code review Co-Authored-By: jvz <boards@gmail.com> * Use temp lib version of secretTextarea Signed-off-by: Matt Sicker <boards@gmail.com>
jenkinsci · May 13, 2019 · b088f7d · b088f7d
1 parent cffb304
commit b088f7d
Show file tree

Hide file tree

Showing 11 changed files with 91 additions and 20 deletions.
diff --git a/.mvn/extensions.xml b/.mvn/extensions.xml
@@ -2,6 +2,6 @@
   <extension>
     <groupId>io.jenkins.tools.incrementals</groupId>
     <artifactId>git-changelist-maven-extension</artifactId>
-    <version>1.0-beta-3</version>
+    <version>1.0-beta-7</version>
   </extension>
 </extensions>
diff --git a/pom.xml b/pom.xml
@@ -5,12 +5,12 @@
   <parent>
     <groupId>org.jenkins-ci.plugins</groupId>
     <artifactId>plugin</artifactId>
-    <version>3.12</version>
+    <version>3.43</version>
     <relativePath />
   </parent>
   <groupId>org.jenkins-ci.plugins</groupId>
   <artifactId>docker-commons</artifactId>
-  <version>1.15-SNAPSHOT</version>
+  <version>${revision}${changelist}</version>
   <packaging>hpi</packaging>
 
   <name>Docker Commons Plugin</name>
@@ -31,7 +31,7 @@
   </scm>
 
   <properties>
-    <revision>1.14</revision>
+    <revision>1.15</revision>
     <changelist>-SNAPSHOT</changelist>
     <jenkins.version>2.60.3</jenkins.version>
     <java.level>8</java.level>
@@ -67,6 +67,12 @@
       <artifactId>authentication-tokens</artifactId>
       <version>1.3</version>
     </dependency>
+    <!-- TODO: remove after upgrading to Jenkins 2.171+ and migrating to f:secretTextarea -->
+    <dependency>
+      <groupId>io.jenkins.temp.jelly</groupId>
+      <artifactId>multiline-secrets-ui</artifactId>
+      <version>1.0</version>
+    </dependency>
 
     <!-- for Pipeline-based unit tests -->
     <dependency>

diff --git a/src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentials.java b/src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentials.java
@@ -50,24 +50,44 @@ public class DockerServerCredentials extends BaseStandardCredentials {
     @CheckForNull 
     private final String serverCaCertificate;
 
-    @DataBoundConstructor
+    /**
+     * @deprecated use {@link #DockerServerCredentials(CredentialsScope, String, String, Secret, String, String)}
+     */
+    @Deprecated
     public DockerServerCredentials(CredentialsScope scope, String id, String description,
                                    @CheckForNull String clientKey, @CheckForNull String clientCertificate,
                                    @CheckForNull String serverCaCertificate) {
+        this(scope, id, description, Util.fixEmptyAndTrim(clientKey) == null ? null : Secret.fromString(clientKey),
+                clientCertificate, serverCaCertificate);
+    }
+
+    @DataBoundConstructor
+    public DockerServerCredentials(CredentialsScope scope, String id, String description,
+                                   @CheckForNull Secret clientKeySecret, @CheckForNull String clientCertificate,
+                                   @CheckForNull String serverCaCertificate) {
         super(scope, id, description);
-        this.clientKey = Util.fixEmptyAndTrim(clientKey) == null ? null : Secret.fromString(clientKey);
+        this.clientKey = clientKeySecret;
         this.clientCertificate = Util.fixEmptyAndTrim(clientCertificate);
         this.serverCaCertificate = Util.fixEmptyAndTrim(serverCaCertificate);
     }
 
+    /**
+     * @deprecated use {@link #getClientKeySecret()}
+     */
+    @CheckForNull
+    @Deprecated
+    public String getClientKey() {
+        return Secret.toString(clientKey);
+    }
+
     /**
      * Gets the PEM formatted secret key to identify the client. The {@code --tlskey} option in docker(1)
      *
      * @return null if there's no authentication
      */
     @CheckForNull
-    public String getClientKey() {
-        return clientKey == null ? null : clientKey.getPlainText();
+    public Secret getClientKeySecret() {
+        return clientKey;
     }
 
     /**

diff --git a/...java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentialsBinding.java b/...java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentialsBinding.java
@@ -10,6 +10,7 @@
 
 import hudson.Extension;
 import hudson.FilePath;
+import hudson.util.Secret;
 
 public class DockerServerCredentialsBinding extends AbstractOnDiskBinding<DockerServerCredentials> {
 
@@ -26,7 +27,7 @@ protected Class<DockerServerCredentials> type() {
     @Override
     protected FilePath write(DockerServerCredentials credentials, FilePath dir) throws IOException, InterruptedException {
         FilePath clientKey = dir.child("key.pem");
-        clientKey.write(credentials.getClientKey(), null);
+        clientKey.write(Secret.toString(credentials.getClientKeySecret()), null);
         clientKey.chmod(0600);
 
         FilePath clientCert = dir.child("cert.pem");

diff --git a/src/main/java/org/jenkinsci/plugins/docker/commons/impl/ServerKeyMaterialFactory.java b/src/main/java/org/jenkinsci/plugins/docker/commons/impl/ServerKeyMaterialFactory.java
@@ -25,6 +25,7 @@
 
 import hudson.EnvVars;
 import hudson.FilePath;
+import hudson.util.Secret;
 import org.jenkinsci.plugins.docker.commons.credentials.DockerServerCredentials;
 import org.jenkinsci.plugins.docker.commons.credentials.KeyMaterial;
 import org.jenkinsci.plugins.docker.commons.credentials.KeyMaterialFactory;
@@ -57,7 +58,7 @@ public class ServerKeyMaterialFactory extends KeyMaterialFactory {
 
     public ServerKeyMaterialFactory(@CheckForNull final DockerServerCredentials credentials) {
         if (credentials != null) {
-            key = credentials.getClientKey();
+            key = Secret.toString(credentials.getClientKeySecret());
             cert = credentials.getClientCertificate();
             ca = credentials.getServerCaCertificate();
         } else {

diff --git a/.../jenkinsci/plugins/docker/commons/impl/ServerKeyMaterialFactoryFromDockerCredentials.java b/.../jenkinsci/plugins/docker/commons/impl/ServerKeyMaterialFactoryFromDockerCredentials.java
@@ -25,6 +25,7 @@
 
 import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.Extension;
+import hudson.util.Secret;
 import jenkins.authentication.tokens.api.AuthenticationTokenException;
 import jenkins.authentication.tokens.api.AuthenticationTokenSource;
 import org.jenkinsci.plugins.docker.commons.credentials.DockerServerCredentials;
@@ -43,6 +44,6 @@ public ServerKeyMaterialFactoryFromDockerCredentials() {
     @NonNull
     @Override
     public KeyMaterialFactory convert(@NonNull DockerServerCredentials credential) throws AuthenticationTokenException {
-        return new ServerKeyMaterialFactory(credential.getClientKey(), credential.getClientCertificate(), credential.getServerCaCertificate());
+        return new ServerKeyMaterialFactory(Secret.toString(credential.getClientKeySecret()), credential.getClientCertificate(), credential.getServerCaCertificate());
     }
 }
diff --git a/...rg/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentials/credentials.jelly b/...rg/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentials/credentials.jelly
@@ -25,8 +25,8 @@
 
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:st="jelly:stapler">
-  <f:entry title="${%Client Key}" field="clientKey">
-    <f:textarea/>
+  <f:entry title="${%Client Key}" field="clientKeySecret">
+    <s:secretTextarea xmlns:s="/io/jenkins/temp/jelly"/>
   </f:entry>
   <f:entry title="${%Client Certificate}" field="clientCertificate">
     <f:textarea/>

diff --git a/src/test/java/org/jenkinsci/plugins/docker/commons/ConfigTest.java b/src/test/java/org/jenkinsci/plugins/docker/commons/ConfigTest.java
@@ -24,6 +24,7 @@
 
 package org.jenkinsci.plugins.docker.commons;
 
+import hudson.util.Secret;
 import org.jenkinsci.plugins.docker.commons.tools.DockerTool;
 import org.jenkinsci.plugins.docker.commons.util.SampleDockerBuilder;
 import org.jenkinsci.plugins.docker.commons.credentials.DockerServerCredentials;
@@ -48,7 +49,7 @@ public class ConfigTest {
 
     @Test public void configRoundTrip() throws Exception {
         CredentialsStore store = CredentialsProvider.lookupStores(r.jenkins).iterator().next();
-        IdCredentials serverCredentials = new DockerServerCredentials(CredentialsScope.GLOBAL, "serverCreds", null, "clientKey", "clientCertificate", "serverCaCertificate");
+        IdCredentials serverCredentials = new DockerServerCredentials(CredentialsScope.GLOBAL, "serverCreds", null, Secret.fromString("clientKey"), "clientCertificate", "serverCaCertificate");
         store.addCredentials(Domain.global(), serverCredentials);
         IdCredentials registryCredentials = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, "registryCreds", null, "me", "pass");
         store.addCredentials(Domain.global(), registryCredentials);

diff --git a/.../org/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentialsBindingTest.java b/.../org/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentialsBindingTest.java
@@ -28,6 +28,7 @@
 import java.io.InputStream;
 import java.util.Collections;
 
+import hudson.util.Secret;
 import org.apache.commons.io.IOUtils;
 import org.jenkinsci.plugins.credentialsbinding.MultiBinding;
 import org.jenkinsci.plugins.credentialsbinding.impl.BindingStep;
@@ -73,7 +74,7 @@ public void evaluate() throws Throwable {
                 Domain domain = new Domain("docker", "A domain for docker credentials",
                         Collections.<DomainSpecification> singletonList(new DockerServerDomainSpecification()));
                 DockerServerCredentials c = new DockerServerCredentials(CredentialsScope.GLOBAL,
-                        "docker-client-cert", "desc", "clientKey", "clientCertificate", "serverCaCertificate");
+                        "docker-client-cert", "desc", Secret.fromString("clientKey"), "clientCertificate", "serverCaCertificate");
                 store.addDomain(domain, c);
                 BindingStep s = new StepConfigTester(story.j)
                         .configRoundTrip(new BindingStep(Collections.<MultiBinding> singletonList(
@@ -90,7 +91,7 @@ public void basics() throws Exception {
             @Override
             public void evaluate() throws Throwable {
                 DockerServerCredentials c = new DockerServerCredentials(CredentialsScope.GLOBAL,
-                        "docker-client-cert", "desc", "clientKey", "clientCertificate", "serverCaCertificate");
+                        "docker-client-cert", "desc", Secret.fromString("clientKey"), "clientCertificate", "serverCaCertificate");
                 CredentialsProvider.lookupStores(story.j.jenkins).iterator().next().addCredentials(Domain.global(), c);
                 WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
                 String pipelineScript = IOUtils.toString(getTestResourceInputStream("basics-Jenkinsfile"));

diff --git a/...st/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentialsTest.java b/...st/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentialsTest.java
@@ -31,11 +31,16 @@
 import com.cloudbees.plugins.credentials.common.IdCredentials;
 import com.cloudbees.plugins.credentials.domains.Domain;
 import com.cloudbees.plugins.credentials.domains.DomainSpecification;
+import com.gargoylesoftware.htmlunit.html.HtmlElement;
+import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import hudson.security.ACL;
+import hudson.util.Secret;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.xml.sax.SAXException;
 
+import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
 
@@ -55,7 +60,7 @@ public void configRoundTripEmpty() throws Exception {
         assertThat(store, instanceOf(SystemCredentialsProvider.StoreImpl.class));
         Domain domain = new Domain("docker", "A domain for docker credentials",
                 Collections.<DomainSpecification>singletonList(new DockerServerDomainSpecification()));
-        DockerServerCredentials credentials = new DockerServerCredentials(CredentialsScope.GLOBAL, "foo", "desc", "", "", "");
+        DockerServerCredentials credentials = new DockerServerCredentials(CredentialsScope.GLOBAL, "foo", "desc", Secret.fromString(""), "", "");
         store.addDomain(domain, credentials);
 
         j.submit(j.createWebClient().goTo("credentials/store/system/domain/" + domain.getName() + "/credential/"+credentials.getId()+"/update")
@@ -71,7 +76,7 @@ public void configRoundTripData() throws Exception {
         assertThat(store, instanceOf(SystemCredentialsProvider.StoreImpl.class));
         Domain domain = new Domain("docker", "A domain for docker credentials",
                 Collections.<DomainSpecification>singletonList(new DockerServerDomainSpecification()));
-        DockerServerCredentials credentials = new DockerServerCredentials(CredentialsScope.GLOBAL, "foo", "desc", "a", "b", "c");
+        DockerServerCredentials credentials = new DockerServerCredentials(CredentialsScope.GLOBAL, "foo", "desc", Secret.fromString("a"), "b", "c");
         store.addDomain(domain, credentials);
 
         j.submit(j.createWebClient().goTo("credentials/store/system/domain/" + domain.getName() + "/credential/"+credentials.getId()+"/update")
@@ -80,5 +85,39 @@ public void configRoundTripData() throws Exception {
         j.assertEqualDataBoundBeans(credentials, CredentialsMatchers.firstOrNull(CredentialsProvider.lookupCredentials(IdCredentials.class, j.getInstance(),
                 ACL.SYSTEM, new DockerServerDomainRequirement()), CredentialsMatchers.withId(credentials.getId())));
     }
-
+
+    @Test
+    public void configRoundTripUpdateCertificates() throws Exception {
+        CredentialsStore store = CredentialsProvider.lookupStores(j.getInstance()).iterator().next();
+        assertThat(store, instanceOf(SystemCredentialsProvider.StoreImpl.class));
+        Domain domain = new Domain("docker", "A domain for docker credentials", Collections.singletonList(new DockerServerDomainSpecification()));
+        DockerServerCredentials credentials = new DockerServerCredentials(CredentialsScope.GLOBAL, "foo", "desc", Secret.fromString("key"), "client-cert", "ca-cert");
+        store.addDomain(domain, credentials);
+
+        HtmlForm form = getUpdateForm(domain, credentials);
+        for (HtmlElement button : form.getElementsByAttribute("input", "class", "secret-update-btn")) {
+            button.click();
+        }
+
+        form.getTextAreaByName("_.clientKeySecret").setText("new key");
+        form.getTextAreaByName("_.clientCertificate").setText("new cert");
+        form.getTextAreaByName("_.serverCaCertificate").setText("new ca cert");
+        j.submit(form);
+
+        DockerServerCredentials expected = new DockerServerCredentials(
+                credentials.getScope(), credentials.getId(), credentials.getDescription(),
+                Secret.fromString("new key"), "new cert", "new ca cert");
+        j.assertEqualDataBoundBeans(expected, findFirstWithId(credentials.getId()));
+    }
+
+    private HtmlForm getUpdateForm(Domain domain, DockerServerCredentials credentials) throws IOException, SAXException {
+        return j.createWebClient().goTo("credentials/store/system/domain/" + domain.getName() + "/credential/" + credentials.getId() + "/update")
+                .getFormByName("update");
+    }
+
+    private IdCredentials findFirstWithId(String credentialsId) {
+        return CredentialsMatchers.firstOrNull(
+                CredentialsProvider.lookupCredentials(IdCredentials.class, j.getInstance(), ACL.SYSTEM, new DockerServerDomainRequirement()),
+                CredentialsMatchers.withId(credentialsId));
+    }
 }
diff --git a/src/test/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerEndpointTest.java b/src/test/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerEndpointTest.java
@@ -34,6 +34,7 @@
 import hudson.model.FreeStyleProject;
 import hudson.remoting.VirtualChannel;
 import hudson.slaves.DumbSlave;
+import hudson.util.Secret;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.JenkinsRule;
@@ -63,7 +64,7 @@ public void smokes() throws Exception {
         assertThat(store, instanceOf(SystemCredentialsProvider.StoreImpl.class));
         Domain domain = new Domain("docker", "A domain for docker credentials",
                 Collections.<DomainSpecification>singletonList(new DockerServerDomainSpecification()));
-        DockerServerCredentials credentials = new DockerServerCredentials(CredentialsScope.GLOBAL, "foo", "desc", "a", "b", "c");
+        DockerServerCredentials credentials = new DockerServerCredentials(CredentialsScope.GLOBAL, "foo", "desc", Secret.fromString("a"), "b", "c");
         store.addDomain(domain, credentials);
         DockerServerEndpoint endpoint = new DockerServerEndpoint("tcp://localhost:2736", credentials.getId());
         FilePath dotDocker = DockerServerEndpoint.dotDocker(channel);