Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-74964] Display error message when adding invalid certificate credentials #580

Merged
merged 9 commits into from
Dec 20, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Priya-CB marked this conversation as resolved.
Show resolved Hide resolved
super(scope, id, description);
Objects.requireNonNull(keyStoreSource);
if (FIPS140.useCompliantAlgorithms() && StringUtils.length(password) < 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);
Priya-CB marked this conversation as resolved.
Show resolved Hide resolved
throw new Descriptor.FormException(Messages.CertificateCredentialsImpl_InvalidKeystore(), e,
"keyStoreSource");
}
}

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
Original file line number Diff line number Diff line change
Expand Up @@ -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
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"]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Add Credentials" popup does not handle HTML responses ("angry Jenkins"), causing the catch block to execute. Added notificationBar to inform the user about the failure(unexpected cases) in credential creation.

})
}
};
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
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");
Priya-CB marked this conversation as resolved.
Show resolved Hide resolved

@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 = "foo";

@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(INVALID_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(Descriptor.FormException.class,
() -> new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "password-length-validation",
"Validate the certificate password", "", storeSource));
assertThrows(Descriptor.FormException.class,
() -> new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "password-length-validation",
"Validate the certificate password", INVALID_PASSWORD,
storeSource));
assertThrows(Descriptor.FormException.class,
() -> new CertificateCredentialsImpl(CredentialsScope.GLOBAL, "invalid-certificate-validation",
"Validate the certificate keyStore", VALID_PASSWORD,
new CertificateCredentialsImpl.PEMEntryKeyStoreSource(
null, null)));
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
-----BEGIN CERTIFICATE-----
Priya-CB marked this conversation as resolved.
Show resolved Hide resolved
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-----
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
-----BEGIN PRIVATE KEY-----
Priya-CB marked this conversation as resolved.
Show resolved Hide resolved
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-----
Loading