Skip to content

Commit

Permalink
Merge pull request #580 from Priya-CB/JENKINS-74964
Browse files Browse the repository at this point in the history
[JENKINS-74964] Display error message when adding invalid certificate credentials
  • Loading branch information
fcojfernandez authored Dec 20, 2024
2 parents 1e306e8 + 1f17086 commit bcda74a
Show file tree
Hide file tree
Showing 8 changed files with 327 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import hudson.security.AccessControlled;
import hudson.security.Permission;
import java.io.IOException;
import java.security.GeneralSecurityException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
Expand All @@ -47,10 +48,14 @@
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.logging.Level;
import java.util.logging.Logger;

import jakarta.servlet.ServletException;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang.exception.ExceptionUtils;
import org.jenkins.ui.icon.IconSpec;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
Expand All @@ -76,6 +81,8 @@ public class CredentialsSelectHelper extends Descriptor<CredentialsSelectHelper>
*/
public static final Permission CREATE = CredentialsProvider.CREATE;

private static final Logger LOGGER = Logger.getLogger(CredentialsSelectHelper.class.getName());

/**
* {@inheritDoc}

Check warning on line 87 in src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java

View check run for this annotation

ci.jenkins.io / JavaDoc

JavaDoc @inheritDoc

NORMAL: Tag @inheritdoc cannot be used in constructor documentation. It can only be used in the following types of documentation: method.
*/
Expand Down Expand Up @@ -608,8 +615,34 @@ 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 (LinkageError e) {
/*
* Descriptor#newInstanceImpl throws a LinkageError if the DataBoundConstructor or any DataBoundSetter
* throw any exception other than RuntimeException implementing HttpResponse.
*
* Checked exceptions implementing HttpResponse like FormException are wrapped and
* rethrown as HttpResponseException (a RuntimeException implementing HttpResponse) in
* RequestImpl#invokeConstructor.
*
* This approach is taken to maintain backward compatibility, as throwing a FormException directly
* from the constructor would result in a source-incompatible change, potentially breaking dependent plugins.
*
* Here, known exceptions are caught specifically to provide meaningful error response.
*/
Throwable rootCause = ExceptionUtils.getRootCause(e);
if (rootCause instanceof IOException || rootCause instanceof IllegalArgumentException

Check warning on line 638 in src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 638 is only partially covered, one branch is missing
|| rootCause instanceof GeneralSecurityException) {
LOGGER.log(Level.WARNING, "Failed to create Credentials", e);
return new JSONObject().element("message", rootCause.getMessage()).element("notificationType",
"ERROR");
}
throw e;

Check warning on line 644 in src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 644 is not covered by tests
}
if (credentialsWereAdded) {
return new JSONObject()
.element("message", "Credentials created")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ public CertificateCredentialsImpl(@CheckForNull CredentialsScope scope,
@NonNull KeyStoreSource keyStoreSource) {
super(scope, id, description);
Objects.requireNonNull(keyStoreSource);
if (FIPS140.useCompliantAlgorithms() && StringUtils.length(password) < 14) {
throw new IllegalArgumentException(Messages.CertificateCredentialsImpl_ShortPasswordFIPS());
}
this.password = Secret.fromString(password);
this.keyStoreSource = keyStoreSource;
// ensure the keySore is valid
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.interceptor.RequirePOST;

import java.util.Objects;

/**
* Concrete implementation of {@link StandardUsernamePasswordCredentials}.
*
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/lib/credentials/select/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"]);
})
}
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,25 +1,58 @@
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.CertificateCredentialsImplTest;

import hudson.model.UnprotectedRootAction;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.List;
import net.sf.json.JSONObject;
import org.apache.commons.io.IOUtils;
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.Before;
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;

public class CredentialsSelectHelperTest {
@Rule
public JenkinsRule j = new JenkinsRule();

private String pemCert;
private String pemKey;

private static final String VALID_PASSWORD = "password";
private static final String INVALID_PASSWORD = "bla";

@Before
public void setup() throws IOException {
pemCert = IOUtils.toString(CertificateCredentialsImplTest.class.getResource("certs.pem"),
StandardCharsets.UTF_8);
pemKey = IOUtils.toString(CertificateCredentialsImplTest.class.getResource("key.pem"),
StandardCharsets.UTF_8);
}


@Test
public void doAddCredentialsFromPopupWorksAsExpected() throws Exception {
try (JenkinsRule.WebClient wc = j.createWebClient()) {
Expand Down Expand Up @@ -56,6 +89,125 @@ public void doAddCredentialsFromPopupWorksAsExpected() throws Exception {
}
}

@Test
@Issue("JENKINS-74964")
public void doAddCredentialsFromPopupForPEMCertificateKeystore() throws Exception {

try (JenkinsRule.WebClient wc = j.createWebClient()) {
HtmlPage htmlPage = wc.goTo("credentials-selection");
HtmlForm form = selectPEMCertificateKeyStore(htmlPage, wc);
form.getTextAreaByName("_.certChain").setTextContent(pemCert);
form.getTextAreaByName("_.privateKey").setTextContent(pemKey);
form.getInputsByName("_.password").forEach(input -> input.setValue(VALID_PASSWORD));
Page submit = HtmlFormUtil.submit(form);
JSONObject responseJson = JSONObject.fromObject(submit.getWebResponse().getContentAsString());
assertThat(responseJson.getString("notificationType"), is("SUCCESS"));
}
}

@Test
@Issue("JENKINS-74964")
public void doAddCredentialsFromPopupForPEMCertificateKeystore_missingKeyStore() throws Exception {

try (JenkinsRule.WebClient wc = j.createWebClient()) {
HtmlPage htmlPage = wc.goTo("credentials-selection");
HtmlForm form = selectPEMCertificateKeyStore(htmlPage, wc);
Page submit = HtmlFormUtil.submit(form);
JSONObject responseJson = JSONObject.fromObject(submit.getWebResponse().getContentAsString());
assertThat(responseJson.getString("notificationType"), is("ERROR"));
}
}

@Test
@Issue("JENKINS-74964")
public void doAddCredentialsFromPopupForInvalidPEMCertificateKeystore_missingCert() throws Exception {

try (JenkinsRule.WebClient wc = j.createWebClient()) {
HtmlPage htmlPage = wc.goTo("credentials-selection");
HtmlForm form = selectPEMCertificateKeyStore(htmlPage, wc);
form.getTextAreaByName("_.certChain").setTextContent(null);
form.getTextAreaByName("_.privateKey").setTextContent(pemKey);
form.getInputsByName("_.password").forEach(input -> input.setValue(VALID_PASSWORD));
Page submit = HtmlFormUtil.submit(form);
JSONObject responseJson = JSONObject.fromObject(submit.getWebResponse().getContentAsString());
assertThat(responseJson.getString("notificationType"), is("ERROR"));
}
}

@Test
@Issue("JENKINS-74964")
public void doAddCredentialsFromPopupForInvalidPEMCertificateKeystore_missingPassword() throws Exception {

try (JenkinsRule.WebClient wc = j.createWebClient()) {
HtmlPage htmlPage = wc.goTo("credentials-selection");
HtmlForm form = selectPEMCertificateKeyStore(htmlPage, wc);
form.getTextAreaByName("_.certChain").setTextContent(pemCert);
form.getTextAreaByName("_.privateKey").setTextContent(pemKey);
Page submit = HtmlFormUtil.submit(form);
JSONObject responseJson = JSONObject.fromObject(submit.getWebResponse().getContentAsString());
assertThat(responseJson.getString("notificationType"), is("ERROR"));
}
}

@Test
@Issue("JENKINS-74964")
public void doAddCredentialsFromPopupForInvalidPEMCertificateKeystore_invalidPassword() throws Exception {

try (JenkinsRule.WebClient wc = j.createWebClient()) {
HtmlPage htmlPage = wc.goTo("credentials-selection");
HtmlForm form = selectPEMCertificateKeyStore(htmlPage, wc);
form.getTextAreaByName("_.certChain").setTextContent(pemCert);
form.getTextAreaByName("_.privateKey").setTextContent(pemKey);
form.getInputsByName("_.password").forEach(input -> input.setValue(INVALID_PASSWORD));
Page submit = HtmlFormUtil.submit(form);
JSONObject responseJson = JSONObject.fromObject(submit.getWebResponse().getContentAsString());
assertThat(responseJson.getString("notificationType"), is("ERROR"));
}
}

private HtmlForm selectPEMCertificateKeyStore(HtmlPage htmlPage, JenkinsRule.WebClient wc) throws IOException {
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);
return form;
}

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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
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.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().withFIPSEnabled().javaOptions("-Xmx512m");

@Rule
public TemporaryFolder tmp = new TemporaryFolder();

private String pemCert;
private String pemKey;
private static final String VALID_PASSWORD = "passwordFipsCheck";
private static final String INVALID_PASSWORD = "invalidPasswordFipsCheck";
private static final String SHORT_PASSWORD = "password";

@Before
public void setup() throws IOException {
pemCert = IOUtils.toString(CertificateCredentialsImplFIPSTest.class.getResource("validCerts.pem"),
StandardCharsets.UTF_8);
pemKey = IOUtils.toString(CertificateCredentialsImplFIPSTest.class.getResource("validKey.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(VALID_PASSWORD);
assertThat(result.kind, is(FormValidation.Kind.OK));
result = descriptor.doCheckPassword(SHORT_PASSWORD);
assertThat(result.kind, is(FormValidation.Kind.ERROR));
assertThat(result.getMessage(),
is(StringEscapeUtils.escapeHtml4(Messages.CertificateCredentialsImpl_ShortPasswordFIPS())));
});
}

@Test
public void invalidPEMKeyStoreAndPasswordTest() throws Throwable {
CertificateCredentialsImpl.PEMEntryKeyStoreSource storeSource = new CertificateCredentialsImpl.PEMEntryKeyStoreSource(
pemCert, pemKey);
rule.then(r -> {
new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "valid-certificate-and-password-validation",
"Validate the certificate credentials", VALID_PASSWORD, storeSource);
assertThrows(IllegalArgumentException.class,
() -> new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "empty-password-validation",
"Validate the certificate empty password", "",
storeSource));
assertThrows(IllegalArgumentException.class,
() -> new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "password-length-validation",
"Validate the certificate password length",
SHORT_PASSWORD, storeSource));
assertThrows(IllegalArgumentException.class,
() -> new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "invalid-password-validation",
"Validate the certificate password", INVALID_PASSWORD,
storeSource));
assertThrows(IllegalArgumentException.class,
() -> new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "empty-keystore-validation",
"Validate the invalid certificate keyStore",
VALID_PASSWORD,
new CertificateCredentialsImpl.PEMEntryKeyStoreSource(
null, null)));
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
-----BEGIN CERTIFICATE-----
MIIDUTCCAjmgAwIBAgIUM2p34T12bOzrnyga5GarEMbBI3QwDQYJKoZIhvcNAQEL
BQAwUDELMAkGA1UEBhMCSU4xCzAJBgNVBAgMAlROMREwDwYDVQQHDAhUaXJ1cHB1
cjEhMB8GA1UECgwYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMCAXDTI0MTIxMjA5
NTgzM1oYDzIxMjQxMTE4MDk1ODMzWjBQMQswCQYDVQQGEwJJTjELMAkGA1UECAwC
VE4xETAPBgNVBAcMCFRpcnVwcHVyMSEwHwYDVQQKDBhJbnRlcm5ldCBXaWRnaXRz
IFB0eSBMdGQwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCoI5pI11oR
2nIvBMdViNN9RpztDYZuJ/tDl5JZz3jFHxwUFuneABMIqiySUFS3HK1Kfsz6Lvbz
qZlLriE1cKKk84GBOlkpISaizAMUyYOkZagmVrsf/QrneLQdYjNZFpOjTCduWmsC
3WLXWGA+t2qlT3jmJr+J5yyHBGme27dtab7UgoSdD2o0FFxXCG15NMkWMi+m+3zW
UK9f4wmaY1wIBd3/umsHM1dv+vwqAfgMMp/I4ISTda2sm3txmDzYGBPzvc/bnsCe
zyahE85Fi/otkqxqzDww2a7YhGY0hjcyihpqYffFw7zWo2syylLcFMFJN1nRoQyj
lshfAeLcGYLHAgMBAAGjITAfMB0GA1UdDgQWBBSSmy62eRSGGzkJnFWrcJDoU3Za
4jANBgkqhkiG9w0BAQsFAAOCAQEAW1YhiizwHdxIqf336vqk0Qn8omcg3v/GotjF
hNqFHiCnr2Y1/+7NwqsvpIpjb354atU5nxJ1X1LkIJz/yPztbVbHzqKn/RUztSV6
+wCOArAZzTfXcgOf/jgqPRQMJxhtUeU01MKcoliBLzCIdYvNe3XdkjMkPUHxK5vt
Yv5hxx+sfZ1T68xCFCgdkXtJKxkBDj9tR56vPOTN7kxpVPKFQzsI644xxID+JRbi
RFwV8GYK8rF6nVw9EQAfi+PuJj+V8DecUshMy6d7heUKWdu5i/+HgVKx46/RZMkj
gwAW4FDt8+qsdvgxYzCTkrkxbREwAtTv6xgPO2BAPZsECG/DSg==
-----END CERTIFICATE-----
Loading

0 comments on commit bcda74a

Please sign in to comment.