[JENKINS-74964] Fix: Display error message when adding invalid certif…

…icate credentials
jenkinsci · Dec 6, 2024 · 632611f · 632611f
1 parent f51f5d5
commit 632611f
Show file tree

Hide file tree

Showing 10 changed files with 210 additions and 7 deletions.
diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java
@@ -57,6 +57,7 @@
 import org.kohsuke.args4j.Argument;
 import org.kohsuke.args4j.CmdLineException;
 import org.kohsuke.args4j.Localizable;
+import org.kohsuke.stapler.HttpResponses;
 import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerRequest2;
 import org.kohsuke.stapler.StaplerResponse2;
@@ -608,8 +609,14 @@ public JSONObject doAddCredentials(StaplerRequest2 req, StaplerResponse2 rsp) th
                         .element("notificationType", "ERROR");
             }
             store.checkPermission(CredentialsStoreAction.CREATE);
-            Credentials credentials = Descriptor.bindJSON(req, Credentials.class, data.getJSONObject("credentials"));
-            boolean credentialsWereAdded = store.addCredentials(wrapper.getDomain(), credentials);
+            boolean credentialsWereAdded;
+            try {
+                Credentials credentials = Descriptor.bindJSON(req, Credentials.class,
+                                                              data.getJSONObject("credentials"));
+                credentialsWereAdded = store.addCredentials(wrapper.getDomain(), credentials);
+            } catch (HttpResponses.HttpResponseException e) {
+                return new JSONObject().element("message", e.getMessage()).element("notificationType", "ERROR");
+            }
             if (credentialsWereAdded) {
                 return new JSONObject()
                         .element("message", "Credentials created")

diff --git a/src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java b/src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java
@@ -129,17 +129,22 @@ public class CertificateCredentialsImpl extends BaseStandardCredentials implemen
     public CertificateCredentialsImpl(@CheckForNull CredentialsScope scope,
                                       @CheckForNull String id, @CheckForNull String description,
                                       @CheckForNull String password,
-                                      @NonNull KeyStoreSource keyStoreSource) {
+                                      @NonNull KeyStoreSource keyStoreSource) throws Descriptor.FormException {
         super(scope, id, description);
         Objects.requireNonNull(keyStoreSource);
+        if (FIPS140.useCompliantAlgorithms() && StringUtils.isNotBlank(password) && password.length() < 14) {
+            throw new Descriptor.FormException(Messages.CertificateCredentialsImpl_ShortPasswordFIPS(), "password");
+        }
         this.password = Secret.fromString(password);
         this.keyStoreSource = keyStoreSource;
         // ensure the keySore is valid
         // we check here as otherwise it will lead to hard to diagnose errors when used
         try {
             keyStoreSource.toKeyStore(toCharArray(this.password));
         } catch (GeneralSecurityException | IOException e) {
-            throw new IllegalArgumentException("KeyStore is not valid.", e);
+            LOGGER.log(Level.WARNING, Messages.CertificateCredentialsImpl_InvalidKeystore(), e);
+            throw new Descriptor.FormException(Messages.CertificateCredentialsImpl_InvalidKeystore(), e,
+                                               "keyStoreSource");
         }
     }
 

diff --git a/src/main/java/com/cloudbees/plugins/credentials/impl/UsernamePasswordCredentialsImpl.java b/src/main/java/com/cloudbees/plugins/credentials/impl/UsernamePasswordCredentialsImpl.java
@@ -42,8 +42,6 @@
 import org.kohsuke.stapler.QueryParameter;
 import org.kohsuke.stapler.interceptor.RequirePOST;
 
-import java.util.Objects;
-
 /**
  * Concrete implementation of {@link StandardUsernamePasswordCredentials}.
  *

diff --git a/src/main/resources/com/cloudbees/plugins/credentials/impl/Messages.properties b/src/main/resources/com/cloudbees/plugins/credentials/impl/Messages.properties
@@ -24,6 +24,7 @@
 UsernamePasswordCredentialsImpl.DisplayName=Username with password
 CertificateCredentialsImpl.DisplayName=Certificate
 CertificateCredentialsImpl.EmptyKeystore=Empty keystore
+CertificateCredentialsImpl.InvalidKeystore=KeyStore is not valid
 CertificateCredentialsImpl.LoadKeyFailed=Could retrieve key "{0}"
 CertificateCredentialsImpl.LoadKeyFailedQueryEmptyPassword=Could retrieve key "{0}". You may need to provide a password
 CertificateCredentialsImpl.LoadKeystoreFailed=Could not load keystore

diff --git a/src/main/resources/lib/credentials/select/select.js b/src/main/resources/lib/credentials/select/select.js
@@ -123,6 +123,7 @@ window.credentials.addSubmit = function (_) {
             .catch((e) => {
                 // notificationBar.show(...) with logging ID could be handy here?
                 console.error("Could not add credentials:", e);
+                window.notificationBar.show("Credentials creation failed.", window.notificationBar["ERROR"]);
             })
     }
 };

diff --git a/src/test/java/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest.java b/src/test/java/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest.java
@@ -1,18 +1,32 @@
 package com.cloudbees.plugins.credentials;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertTrue;
 
 import com.cloudbees.plugins.credentials.common.UsernamePasswordCredentials;
+import com.cloudbees.plugins.credentials.impl.CertificateCredentialsImpl;
+import com.cloudbees.plugins.credentials.impl.Messages;
 import hudson.model.UnprotectedRootAction;
+import java.io.IOException;
 import java.util.List;
+import net.sf.json.JSONObject;
 import org.hamcrest.Matchers;
+import org.htmlunit.Page;
+import org.htmlunit.html.DomNode;
+import org.htmlunit.html.DomNodeList;
 import org.htmlunit.html.HtmlButton;
+import org.htmlunit.html.HtmlElementUtil;
 import org.htmlunit.html.HtmlForm;
+import org.htmlunit.html.HtmlFormUtil;
 import org.htmlunit.html.HtmlInput;
+import org.htmlunit.html.HtmlOption;
 import org.htmlunit.html.HtmlPage;
+import org.htmlunit.html.HtmlRadioButtonInput;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.TestExtension;
 
@@ -56,6 +70,59 @@ public void doAddCredentialsFromPopupWorksAsExpected() throws Exception {
         }
     }
 
+    @Test
+    @Issue("JENKINS-74964")
+    public void doAddCredentialsFromPopupForInvalidPEMCertificateKeystore() throws Exception {
+
+        try (JenkinsRule.WebClient wc = j.createWebClient()) {
+            HtmlPage htmlPage = wc.goTo("credentials-selection");
+
+            HtmlButton addCredentialsButton = htmlPage.querySelector(".credentials-add-menu");
+            addCredentialsButton.fireEvent("mouseenter");
+            addCredentialsButton.click();
+
+            HtmlButton jenkinsCredentialsOption = htmlPage.querySelector(".jenkins-dropdown__item");
+            jenkinsCredentialsOption.click();
+
+            wc.waitForBackgroundJavaScript(4000);
+            HtmlForm form = htmlPage.querySelector("#credentials-dialog-form");
+            String certificateDisplayName = j.jenkins.getDescriptor(CertificateCredentialsImpl.class).getDisplayName();
+            String KeyStoreSourceDisplayName = j.jenkins.getDescriptor(
+                    CertificateCredentialsImpl.PEMEntryKeyStoreSource.class).getDisplayName();
+            DomNodeList<DomNode> allOptions = htmlPage.getDocumentElement().querySelectorAll(
+                    "select.dropdownList option");
+            boolean optionFound = selectOption(allOptions, certificateDisplayName);
+            assertTrue("The Certificate option was not found in the credentials type select", optionFound);
+            List<HtmlRadioButtonInput> inputs = htmlPage.getDocumentElement().getByXPath(
+                    "//input[contains(@name, 'keyStoreSource') and following-sibling::label[contains(.,'"
+                    + KeyStoreSourceDisplayName + "')]]");
+            assertThat("query should return only a singular input", inputs, hasSize(1));
+            HtmlElementUtil.click(inputs.get(0));
+            wc.waitForBackgroundJavaScript(4000);
+            Page submit = HtmlFormUtil.submit(form);
+            JSONObject responseJson = JSONObject.fromObject(submit.getWebResponse().getContentAsString());
+            assertThat(responseJson.getString("message"), is(Messages.CertificateCredentialsImpl_InvalidKeystore()));
+            assertThat(responseJson.getString("notificationType"), is("ERROR"));
+        }
+    }
+
+    private static boolean selectOption(DomNodeList<DomNode> allOptions, String optionName) {
+        return allOptions.stream().anyMatch(domNode -> {
+            if (domNode instanceof HtmlOption option) {
+                if (option.getVisibleText().equals(optionName)) {
+                    try {
+                        HtmlElementUtil.click(option);
+                    } catch (IOException e) {
+                        throw new RuntimeException(e);
+                    }
+                    return true;
+                }
+            }
+
+            return false;
+        });
+    }
+
     @TestExtension
     public static class CredentialsSelectionAction implements UnprotectedRootAction {
         @Override

diff --git a/src/test/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImplFIPSTest.java b/src/test/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImplFIPSTest.java
@@ -0,0 +1,75 @@
+package com.cloudbees.plugins.credentials.impl;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+
+import org.apache.commons.io.IOUtils;
+import org.apache.commons.text.StringEscapeUtils;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.jvnet.hudson.test.RealJenkinsRule;
+
+import hudson.ExtensionList;
+import hudson.model.Descriptor;
+import hudson.util.FormValidation;
+
+import com.cloudbees.plugins.credentials.CredentialsScope;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertThrows;
+
+public class CertificateCredentialsImplFIPSTest {
+
+    @Rule
+    public RealJenkinsRule rule = new RealJenkinsRule().javaOptions("-Djenkins.security.FIPS140.COMPLIANCE=true");
+
+    @Rule
+    public TemporaryFolder tmp = new TemporaryFolder();
+
+    String pemCert;
+    String pemKey;
+
+    @Before
+    public void setup() throws IOException {
+        pemCert = IOUtils.toString(CertificateCredentialsImplFIPSTest.class.getResource("FIPSCerts.pem"),
+                                   StandardCharsets.UTF_8);
+        pemKey = IOUtils.toString(CertificateCredentialsImplFIPSTest.class.getResource("FIPSKey.pem"),
+                                  StandardCharsets.UTF_8);
+    }
+
+    @Test
+    public void doCheckPasswordTest() throws Throwable {
+        rule.then(r -> {
+            CertificateCredentialsImpl.DescriptorImpl descriptor = ExtensionList.lookupSingleton(
+                    CertificateCredentialsImpl.DescriptorImpl.class);
+            FormValidation result = descriptor.doCheckPassword("passwordFipsCheck");
+            assertThat(result.kind, is(FormValidation.Kind.OK));
+            result = descriptor.doCheckPassword("foo");
+            assertThat(result.kind, is(FormValidation.Kind.ERROR));
+            assertThat(result.getMessage(),
+                       is(StringEscapeUtils.escapeHtml4(Messages.CertificateCredentialsImpl_ShortPasswordFIPS())));
+        });
+    }
+
+    @Test
+    public void invalidPEMKeyStoreTest() throws Throwable {
+        CertificateCredentialsImpl.PEMEntryKeyStoreSource storeSource = new CertificateCredentialsImpl.PEMEntryKeyStoreSource(
+                pemCert, pemKey);
+        rule.then(r -> {
+            new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "certificate-validation",
+                                           "Validate the certificate credentials", "passwordFipsCheck", storeSource);
+
+            assertThrows(Descriptor.FormException.class,
+                         () -> new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "certificate-validation",
+                                                              "Validate the certificate password", "foo", storeSource));
+            assertThrows(Descriptor.FormException.class,
+                         () -> new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "certificate-validation",
+                                                              "Validate the certificate keyStore", "passwordFipsCheck",
+                                                              new CertificateCredentialsImpl.PEMEntryKeyStoreSource(
+                                                                      null, null)));
+        });
+    }
+}
diff --git a/src/test/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImplTest.java b/src/test/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImplTest.java
@@ -47,6 +47,7 @@
 import org.htmlunit.html.HtmlRadioButtonInput;
 
 import hudson.Util;
+import hudson.model.Descriptor;
 import hudson.model.ItemGroup;
 import hudson.security.ACL;
 import hudson.util.Secret;
@@ -106,7 +107,7 @@ public void setup() throws IOException {
     }
 
     @Test
-    public void displayName() throws IOException {
+    public void displayName() throws IOException, Descriptor.FormException {
         SecretBytes uploadedKeystore = SecretBytes.fromBytes(Files.readAllBytes(p12.toPath()));
         CertificateCredentialsImpl.UploadedKeyStoreSource storeSource = new CertificateCredentialsImpl.UploadedKeyStoreSource(uploadedKeystore);
         assertEquals(EXPECTED_DISPLAY_NAME, CredentialsNameProvider.name(new CertificateCredentialsImpl(null, "abc123", null, "password", storeSource)));

diff --git a/src/test/resources/com/cloudbees/plugins/credentials/impl/FIPSCerts.pem b/src/test/resources/com/cloudbees/plugins/credentials/impl/FIPSCerts.pem
@@ -0,0 +1,20 @@
+-----BEGIN CERTIFICATE-----
+MIIDTzCCAjegAwIBAgIUWcaB9PB40lCu4um/CD8Ni6yNxkwwDQYJKoZIhvcNAQEL
+BQAwUDELMAkGA1UEBhMCSU4xCzAJBgNVBAgMAlROMREwDwYDVQQHDAhUaXJ1cHB1
+cjEhMB8GA1UECgwYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMB4XDTI0MTEyODE2
+MzI1OFoXDTI1MTEyODE2MzI1OFowUDELMAkGA1UEBhMCSU4xCzAJBgNVBAgMAlRO
+MREwDwYDVQQHDAhUaXJ1cHB1cjEhMB8GA1UECgwYSW50ZXJuZXQgV2lkZ2l0cyBQ
+dHkgTHRkMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAo8dgK5e2+wy3
+2uRxF8hiaSFnUjfKOVDYrj+RbZ+ss9Kzxm2T3nfGcjGKHVHTvkaoduVdF95V8RtQ
+I2lbwPnb/RKY6+Rlx+74e6Aw+KvCAEFa0lt0dST7IpErEetDppkkR5N/RbVXvon8
+sbJSHRZQOazFD2fG6MJJYiHqLC9xqfKOrZ07JQ9/J/mXN05mTG3gWXIlaJxf6hv3
++n2NfkaTioor9dHGwr03S9D4CJWbJvk2006WCh9TEKCrG+oOAU/Fm+mlWndeResv
+EWHiH13BMOsqWj/q9QlMYnmRl4MN00SSKKREQAUAAd6WbXg6+iavxGpN8eJw4O72
+wti2bS6I4wIDAQABoyEwHzAdBgNVHQ4EFgQUsACQZeota/vgn2vOY0EVIMoKM7Iw
+DQYJKoZIhvcNAQELBQADggEBAEZ7qOLFPL4QJYcD3DYohFyOR2QeQQ5sceLlCg+/
+vtET3TPqBGswHER4fkOUgehud9i/h6ff33KCgTGMpGCWkYISaAbCQgGeD8AJQIOX
+aWQ12Ux/E98a4Bk9nid0moiXKTXzMBule44JolGWroxayrJnYDCo39IraJO6XxlK
+SJnI1dA1uRuJl6XKHr1N/knH3QBE4wI3CjqHc9qjTXVprhRosmTykvIeLaNL/ZxW
+/QnKD5VRWRmHFom03COkmPSyGWNp225dLgN+rv9e2Pvn0AP8CeUJAoWSXwxGpv9Y
+AgIsGfIhwQIt3AsmmujlaLvjRQpxBh00ba77kteFR4S3WMk=
+-----END CERTIFICATE-----
diff --git a/src/test/resources/com/cloudbees/plugins/credentials/impl/FIPSKey.pem b/src/test/resources/com/cloudbees/plugins/credentials/impl/FIPSKey.pem
@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
+MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQChhYxialRHFZdE
+Y1d6A0zTCi6pMnQlk/DU2xt2qV6xrI/Jkz0xIuDjCA/vD4EyEEh7aPEY5Ooxz6zz
+tmrhF8nG2Nis4UBAxtTHqV2D5eLPfxtrRMccrIYh6121K0B45vqcHpBq/nFMNGyK
+q1WJBmb4JTZ4MsZXcpe7rGBhApwgUdP/ey6jwf7oG2YCBOywZ7hu0Vs6yeSbF84R
+uTuVBhVdNm2BJHMy7P2BxyDpm5RSbHQGYox68pn3kfVEdv/iC/8qwq7sbyzbKsgv
+YJrpqAunTEkgLBkG/NtsSFlTzssEAtSXErfI+6b8Zd4UK6E9CoclKWyzEqMT9mVc
+hox4+XL/AgMBAAECggEANYYAqBgeB1QzRRk6QpdXXNOR9MVgUZd9hbt5lU+4rl3F
+ZAGjlGW/adwhE5HquQFGU4bJ5frtVEZCRJxdPGvalEcFPfyCgzSgC+2mrG+AQkwX
+dOtco7bT1+ebrM5BVg8MWrGSH7JjLuJsWWM/O+HgOzhxnVEOAqpZd3o+kccAn4CX
+XjxbIc3/VJ4PXm7kOfU4NL7tJ1G81UQMTNMODisvi++yAa+Rw30NpnQ15GcdnWfF
+D/w2ZQW1MSbNFJzXWettDUm87oszNtIBRbyqQZD1WmHjCc/DON7vx8+8sN152zjM
+ZKsb/4K/3pG0JTvR7C0MM+5ZGUU/0j+JNUavF5gEuQKBgQDNfi2lP0YZ0oesA+Yu
+I6c1TKJefXfLY2HMFTagEhI879hbcNvWKYL+/yIKf4GJCAvUTcmOzPCPGW90H7S8
+dTanmD2xW2yf+8ZMf1mqdbOflhebPCQwZz2QY5ccBZ8oaa/uvKrBagoPPOm1YCAc
+LJGWxRGx5cmKg212sC+DKW/IvQKBgQDJOKgcgT7CvRR2pFn1oAgraFsfarw5XQuC
+4LvE1m6q/qzfv3hZeqPTEMlFy4SGX+veuxfrbdtB5VBSWiXKNkHu+BBVd1uTfEbD
+tx/M3Lxb/wOZ0cADZh0kOBLxvLtim/1wfH85WtImUfGzVLdB329HtoEIE6TdTOoq
+iO4bZul8awKBgC0voMPkfPqyo6i8lsHwjxUWS+HxPwVXTir9QyzBrIb/ypiY4Y5f
+RHHkEk0yqn5Caa9+h2LCR+d/lVV4n1qNf74sqOw2CVXInFs36bSk+yGNdJVrDR4j
+pZL5g0HjLpNJYiliDT5Infupzk5W29i2KDF6FiEDQWUW71wY8+mok+8VAoGAa2m7
+E7xKbFnSmqKRAvUyZzmFqvenElgA1RRyJ1jwKodYcPgcnmdBHGJRjthdHf4GQxdM
+ZXh3Gm32un80vQTJnW7+CSF12Pz2KXOPniQWyGUQ3wOApE/WLodgVXqR7MmoOGu8
+3jkFBT+o7jnCuX80P+vEZTNXRmrQdXQy5p3A9ZECgYEAiMTdP/GEGhD4LSfkmOh/
+jjL2llf68YgLyRvTCFh+iFWbpb3fVd2L9dUt1lQpgliO7vPhw5AYxLoEx3kne8BB
+zGoTWMuGboeBgGS/1iuPLMvNfJum2CHXsofr9ICO0tLHrDUw94mY25TmxlZeB0hc
+UPPZOQ5AsQX82MTBSnkdHQI=
+-----END PRIVATE KEY-----