SECURITY-1621 Store global config API key as Secret

jenkinsci · Oct 7, 2019 · 2a9dd6c · 2a9dd6c
1 parent 9ccf85b
commit 2a9dd6c
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 20 deletions.
diff --git a/src/main/java/jenkins/plugins/zulip/DescriptorImpl.java b/src/main/java/jenkins/plugins/zulip/DescriptorImpl.java
@@ -9,6 +9,7 @@
 import hudson.model.AbstractProject;
 import hudson.tasks.BuildStepDescriptor;
 import hudson.tasks.Publisher;
+import hudson.util.Secret;
 import hudson.util.XStream2;
 import jenkins.model.Jenkins;
 import net.sf.json.JSONObject;
@@ -23,9 +24,11 @@ public class DescriptorImpl extends BuildStepDescriptor<Publisher> {
 
     private static final Logger logger = Logger.getLogger(DescriptorImpl.class.getName());
 
+    private static final String OLD_CONFIG_FILE_NAME = "hudson.plugins.humbug.HumbugNotifier.xml";
+
     private String url;
     private String email;
-    private String apiKey;
+    private Secret apiKey;
     private String stream;
     private String topic;
     private transient String hudsonUrl; // backwards compatibility
@@ -40,7 +43,7 @@ public DescriptorImpl() {
         } else {
             XStream2 xstream = new XStream2();
             xstream.alias("hudson.plugins.humbug.DescriptorImpl", DescriptorImpl.class);
-            XmlFile oldConfig = new XmlFile(xstream, new File(Jenkins.getInstance().getRootDir(),"hudson.plugins.humbug.HumbugNotifier.xml"));
+            XmlFile oldConfig = new XmlFile(xstream, new File(Jenkins.getInstance().getRootDir(), OLD_CONFIG_FILE_NAME));
             if (oldConfig.exists()) {
                 try {
                     oldConfig.unmarshal(this);
@@ -67,11 +70,11 @@ public void setEmail(String email) {
         this.email = email;
     }
 
-    public String getApiKey() {
+    public Secret getApiKey() {
         return apiKey;
     }
 
-    public void setApiKey(String apiKey) {
+    public void setApiKey(Secret apiKey) {
         this.apiKey = apiKey;
     }
 
@@ -115,12 +118,23 @@ public boolean isApplicable(Class<? extends AbstractProject> aClass) {
     public boolean configure(StaplerRequest req, JSONObject json) throws FormException {
         url = (String) json.get("url");
         email = (String) json.get("email");
-        apiKey = (String) json.get("apiKey");
+        apiKey = Secret.fromString((String) json.get("apiKey"));
         stream = (String) json.get("stream");
         topic = (String) json.get("topic");
         jenkinsUrl = (String) json.get("jenkinsUrl");
         smartNotify = Boolean.TRUE.equals(json.get("smartNotify"));
         save();
+
+        // Cleanup the configuration file from previous plugin id - humbug
+        File oldConfig = new File(Jenkins.getInstance().getRootDir(), OLD_CONFIG_FILE_NAME);
+        if (oldConfig.exists()) {
+            if (oldConfig.delete()) {
+                logger.log(Level.INFO, "Old humbug configuration file successfully cleaned up.");
+            } else {
+                logger.log(Level.WARNING, "Failed to cleanup old humbug configuration file.");
+            }
+        }
+
         return super.configure(req, json);
     }
 

diff --git a/src/main/java/jenkins/plugins/zulip/Zulip.java b/src/main/java/jenkins/plugins/zulip/Zulip.java
@@ -10,6 +10,7 @@
 import java.util.logging.Logger;
 
 import hudson.ProxyConfiguration;
+import hudson.util.Secret;
 import jenkins.model.Jenkins;
 import org.apache.commons.codec.binary.Base64;
 import org.apache.commons.httpclient.HttpClient;
@@ -32,14 +33,14 @@ public class Zulip {
     private String apiKey;
     private static final Logger LOGGER = Logger.getLogger(Zulip.class.getName());
 
-    public Zulip(String url, String email, String apiKey) {
+    public Zulip(String url, String email, Secret apiKey) {
         super();
         if (url != null && url.length() > 0 && !url.endsWith("/") ) {
             url = url + "/";
         }
         this.url = url;
         this.email = email;
-        this.apiKey = apiKey;
+        this.apiKey = Secret.toString(apiKey);
     }
 
     /**

diff --git a/src/main/resources/jenkins/plugins/zulip/ZulipNotifier/global.jelly b/src/main/resources/jenkins/plugins/zulip/ZulipNotifier/global.jelly
@@ -21,7 +21,7 @@
         <f:textbox name="email" value="${descriptor.getEmail()}" />
     </f:entry>
     <f:entry title="Zulip API Key" help="/plugin/zulip/help-globalConfig-apiKey.html">
-        <f:textbox name="apiKey" value="${descriptor.getApiKey()}" />
+        <f:password name="apiKey" value="${descriptor.getApiKey()}" />
     </f:entry>
     <f:entry title="Default Stream Name" help="/plugin/zulip/help-globalConfig-stream.html">
         <f:textbox name="stream" value="${descriptor.getStream()}" />

diff --git a/src/test/java/jenkins/plugins/zulip/IntegrationTest.java b/src/test/java/jenkins/plugins/zulip/IntegrationTest.java
@@ -3,6 +3,7 @@
 import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import hudson.model.FreeStyleProject;
+import hudson.util.Secret;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.junit.AfterClass;
@@ -120,7 +121,7 @@ private void verifyGlobalConfig() {
         DescriptorImpl globalConfig = j.jenkins.getDescriptorByType(DescriptorImpl.class);
         assertEquals("ZulipUrl", globalConfig.getUrl());
         assertEquals("jenkins-bot@zulip.com", globalConfig.getEmail());
-        assertEquals("secret", globalConfig.getApiKey());
+        assertEquals("secret", Secret.toString(globalConfig.getApiKey()));
         assertEquals("defaultStream", globalConfig.getStream());
         assertEquals("defaultTopic", globalConfig.getTopic());
         assertTrue(globalConfig.isSmartNotify());

diff --git a/src/test/java/jenkins/plugins/zulip/ZulipNotifierTest.java b/src/test/java/jenkins/plugins/zulip/ZulipNotifierTest.java
@@ -13,6 +13,7 @@
 import hudson.model.User;
 import hudson.scm.ChangeLogSet;
 import hudson.tasks.test.AbstractTestResultAction;
+import hudson.util.Secret;
 import jenkins.model.Jenkins;
 import org.junit.Before;
 import org.junit.Test;
@@ -31,7 +32,10 @@
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
+import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Matchers.same;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -40,7 +44,7 @@
 
 @RunWith(PowerMockRunner.class)
 @PrepareForTest({Jenkins.class, User.class, ZulipNotifier.class, DescriptorImpl.class,
-        AbstractBuild.class, Job.class})
+        AbstractBuild.class, Job.class, Secret.class})
 public class ZulipNotifierTest {
 
     private static final int TOTAL_TEST_COUNT = 100;
@@ -49,6 +53,9 @@ public class ZulipNotifierTest {
     @Mock
     private Jenkins jenkins;
 
+    @Mock
+    private Secret secret;
+
     @Mock
     private Zulip zulip;
 
@@ -99,7 +106,7 @@ public User answer(InvocationOnMock invocation) throws Throwable {
         });
         when(descMock.getUrl()).thenReturn("zulipUrl");
         when(descMock.getEmail()).thenReturn("jenkins-bot@zulip.com");
-        when(descMock.getApiKey()).thenReturn("secret");
+        when(descMock.getApiKey()).thenReturn(secret);
         when(descMock.getStream()).thenReturn("defaultStream");
         when(descMock.getTopic()).thenReturn("defaultTopic");
         PowerMockito.whenNew(DescriptorImpl.class).withAnyArguments().thenReturn(descMock);
@@ -123,7 +130,7 @@ public String answer(InvocationOnMock invocation) throws Throwable {
     public void testShouldUseDefaults() throws Exception {
         ZulipNotifier notifier = new ZulipNotifier();
         notifier.perform(build, null, buildListener);
-        verifyNew(Zulip.class).withArguments("zulipUrl", "jenkins-bot@zulip.com", "secret");
+        verifyNew(Zulip.class).withArguments(eq("zulipUrl"), eq("jenkins-bot@zulip.com"), any(Secret.class));
         verify(envVars, times(2)).expand(expandCaptor.capture());
         assertThat("Should expand stream, topic and message", expandCaptor.getAllValues(),
                 is(Arrays.asList("defaultStream", "defaultTopic")));

diff --git a/src/test/java/jenkins/plugins/zulip/ZulipSendStepTest.java b/src/test/java/jenkins/plugins/zulip/ZulipSendStepTest.java
@@ -6,6 +6,7 @@
 import hudson.model.Job;
 import hudson.model.Run;
 import hudson.model.TaskListener;
+import hudson.util.Secret;
 import jenkins.model.Jenkins;
 import org.junit.Before;
 import org.junit.Test;
@@ -23,20 +24,25 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThat;
+import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.powermock.api.mockito.PowerMockito.verifyNew;
 import static org.powermock.api.mockito.PowerMockito.when;
 
 @RunWith(PowerMockRunner.class)
-@PrepareForTest({Jenkins.class, ZulipSendStep.class})
+@PrepareForTest({Jenkins.class, ZulipSendStep.class, Secret.class})
 public class ZulipSendStepTest {
 
     @Mock
     private Jenkins jenkins;
 
+    @Mock
+    private Secret secret;
+
     @Mock
     private Zulip zulip;
 
@@ -75,7 +81,7 @@ public void setUp() throws Exception {
         when(jenkins.getDescriptorByType(DescriptorImpl.class)).thenReturn(descMock);
         when(descMock.getUrl()).thenReturn("zulipUrl");
         when(descMock.getEmail()).thenReturn("jenkins-bot@zulip.com");
-        when(descMock.getApiKey()).thenReturn("secret");
+        when(descMock.getApiKey()).thenReturn(secret);
         when(descMock.getStream()).thenReturn("defaultStream");
         when(descMock.getTopic()).thenReturn("defaultTopic");
         when(run.getParent()).thenReturn(job);
@@ -94,7 +100,7 @@ public void testShouldUseDefaults() throws Exception {
         ZulipSendStep sendStep = new ZulipSendStep();
         sendStep.setMessage("message");
         sendStep.perform(run, null, null, taskListener);
-        verifyNew(Zulip.class).withArguments("zulipUrl", "jenkins-bot@zulip.com", "secret");
+        verifyNew(Zulip.class).withArguments(eq("zulipUrl"), eq("jenkins-bot@zulip.com"), any(Secret.class));
         verify(envVars, times(3)).expand(expandCaptor.capture());
         assertThat("Should expand stream, topic and message", expandCaptor.getAllValues(),
                 is(Arrays.asList("defaultStream", "defaultTopic", "message")));

diff --git a/src/test/java/jenkins/plugins/zulip/ZulipTest.java b/src/test/java/jenkins/plugins/zulip/ZulipTest.java
@@ -4,6 +4,7 @@
 import java.nio.charset.Charset;
 
 import com.google.common.net.HttpHeaders;
+import hudson.util.Secret;
 import jenkins.model.Jenkins;
 import org.apache.commons.httpclient.NameValuePair;
 import org.junit.AfterClass;
@@ -18,20 +19,24 @@
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
+import static org.mockito.Matchers.any;
 import static org.mockserver.model.HttpRequest.request;
 import static org.mockserver.model.HttpResponse.response;
 import static org.mockserver.model.StringBody.exact;
 import static org.powermock.api.mockito.PowerMockito.when;
 
 @RunWith(PowerMockRunner.class)
-@PrepareForTest(Jenkins.class)
+@PrepareForTest({Jenkins.class, Secret.class})
 public class ZulipTest {
 
     private static ClientAndServer mockServer;
 
     @Mock
     private Jenkins jenkins;
 
+    @Mock
+    private Secret secret;
+
     @BeforeClass
     public static void startMockServer() {
         mockServer = ClientAndServer.startClientAndServer(1080);
@@ -46,13 +51,15 @@ public static void stopMockServer() {
     public void setUp() {
         PowerMockito.mockStatic(Jenkins.class);
         when(Jenkins.getInstance()).thenReturn(jenkins);
+        PowerMockito.mockStatic(Secret.class);
+        when(Secret.toString(any(Secret.class))).thenReturn("secret");
     }
 
     @Test
     public void testSendStreamMessage() throws Exception {
         mockServer.reset();
         mockServer.when(request().withPath("/api/v1/messages")).respond(response().withStatusCode(200));
-        Zulip zulip = new Zulip("http://localhost:1080", "jenkins-bot@zulip.com", "secret");
+        Zulip zulip = new Zulip("http://localhost:1080", "jenkins-bot@zulip.com", secret);
         zulip.sendStreamMessage("testStreamůř", "testTopic", "testMessage");
         mockServer.verify(
                 request()
@@ -78,21 +85,21 @@ public void testSendStreamMessage() throws Exception {
     public void testFailGracefullyOnError() {
         mockServer.reset();
         mockServer.when(request().withPath("/api/v1/messages")).respond(response().withStatusCode(500));
-        Zulip zulip = new Zulip("http://localhost:1080", "jenkins-bot@zulip.com", "secret");
+        Zulip zulip = new Zulip("http://localhost:1080", "jenkins-bot@zulip.com", secret);
         // Test that this does not throw exception
         zulip.sendStreamMessage("testStream", "testTopic", "testMessage");
     }
 
     @Test
     public void testFailGracefullyWhenUnreachable() {
-        Zulip zulip = new Zulip("http://localhost:1081", "jenkins-bot@zulip.com", "secret");
+        Zulip zulip = new Zulip("http://localhost:1081", "jenkins-bot@zulip.com", secret);
         // Test that this does not throw exception
         zulip.sendStreamMessage("testStream", "testTopic", "testMessage");
     }
 
     @Test
     public void testFailGracefullyUnknonwHost() {
-        Zulip zulip = new Zulip("http://unreachable:1080", "jenkins-bot@zulip.com", "secret");
+        Zulip zulip = new Zulip("http://unreachable:1080", "jenkins-bot@zulip.com", secret);
         // Test that this does not throw exception
         zulip.sendStreamMessage("testStream", "testTopic", "testMessage");
     }