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.

Sign up for GitHub

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

Handle password with empty string for certificate auth #581

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Handle password with empty string for certificate auth #581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,10 @@
* Helper to convert a {@link Secret} password into a {@code char[]}
*
* @param password the password.
* @return a {@code char[]} containing the password or {@code null}
* @return a {@code char[]} containing the password
*/
@CheckForNull
Copy link
Member

Choose a reason for hiding this comment

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

There is a difference between no password and an empty password.

It seems like Azure is using the empty password rather than no password (why I have no idea).

https://stackoverflow.com/a/53523999 appears to confirm this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure looks like other systems try both empty string and null

private static char[] toCharArray(@NonNull Secret password) {
String plainText = Util.fixEmpty(password.getPlainText());
return plainText == null ? null : plainText.toCharArray();
return password.getPlainText().toCharArray();
Copy link
Member Author

Choose a reason for hiding this comment

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

Empty string is used at least commonly in Azure if not elsewhere for PFX files.

Shouldn't modify what the user has passed here

}

/**
Expand Down Expand Up @@ -248,7 +246,7 @@
return FormValidation.error(Messages.CertificateCredentialsImpl_ShortPasswordFIPS());
}
if (pw.isEmpty()) {
return FormValidation.warning(Messages.CertificateCredentialsImpl_NoPassword());
return FormValidation.ok(Messages.CertificateCredentialsImpl_NoPassword());
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. warnings/errors shouldn't be shown when you open a form
  2. this is a valid usecase but its still worth making people aware so I left it as ok

}
if (pw.length() < 14) {
return FormValidation.warning(Messages.CertificateCredentialsImpl_ShortPassword());
Expand Down Expand Up @@ -624,9 +622,7 @@
} catch (KeyStoreException | CertificateException | NoSuchAlgorithmException | IOException e) {
return FormValidation.warning(e, Messages.CertificateCredentialsImpl_LoadKeystoreFailed());
} finally {
if (passwordChars != null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

no longer possible to be null here

Arrays.fill(passwordChars, ' ');
}
Arrays.fill(passwordChars, ' ');
}
}

Expand Down Expand Up @@ -739,6 +735,9 @@
List<PEMEncodable> pemEncodables = PEMEncodable.decodeAll(pemCerts, null);
long count = pemEncodables.stream().map(PEMEncodable::toCertificate).filter(Objects::nonNull).count();
if (count < 1) {
if (Util.fixEmpty(value) == null) {

Check warning on line 738 in src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 738 is only partially covered, one branch is missing
Copy link
Member Author

Choose a reason for hiding this comment

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

warnings/errors shouldn't be shown when you open a form

The form has a clear empty state so we don't need the same content as we did with the password for the pfx:

image

Copy link
Member

Choose a reason for hiding this comment

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

I would agree if Jenkins did not allow you to hit "Save" or Apply when the form was in an invalid state or otherwise called out mandatory parameters from optional ones, but it does neither.

Hitting save leads to an angry Jenkins which is IMO a worse UX that the error.

2024-12-10 11:23:30.372+0000 [id=41]    WARNING h.i.i.InstallUncaughtExceptionHandler#handleException: Caught unhandled exception with ID a655cb61-a72d-4d07-a85b-97a161a42acd
java.io.IOException: expected one key but got 0
        at PluginClassLoader for credentials//com.cloudbees.plugins.credentials.impl.CertificateCredentialsImpl$PEMEntryKeyStoreSource.toKeyStore(CertificateCredentialsImpl.java:703)
        at PluginClassLoader for credentials//com.cloudbees.plugins.credentials.impl.CertificateCredentialsImpl$PEMEntryKeyStoreSource.toKeyStore(CertificateCredentialsImpl.java:691)
        at PluginClassLoader for credentials//com.cloudbees.plugins.credentials.impl.CertificateCredentialsImpl.<init>(CertificateCredentialsImpl.java:140)
Caused: java.lang.IllegalArgumentException: KeyStore is not valid.

return FormValidation.ok();
}
return FormValidation.error(Messages.CertificateCredentialsImpl_PEMNoCertificates());
}
// ensure only certs are provided.
Expand Down Expand Up @@ -771,6 +770,9 @@
List<PEMEncodable> pemEncodables = PEMEncodable.decodeAll(key, toCharArray(Secret.fromString(password)));
long count = pemEncodables.stream().map(PEMEncodable::toPrivateKey).filter(Objects::nonNull).count();
if (count == 0) {
if (Util.fixEmpty(value) == null) {

Check warning on line 773 in src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 773 is only partially covered, one branch is missing
Copy link
Member Author

Choose a reason for hiding this comment

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

warnings/errors shouldn't be shown when you open a form

The form has a clear empty state so we don't need the same content as we did with the password for the pfx:

image

return FormValidation.ok();
}
return FormValidation.error(Messages.CertificateCredentialsImpl_PEMNoKeys());
}
if (count > 1) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/resources/com/cloudbees/plugins/credentials/impl/Messages.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@
UsernamePasswordCredentialsImpl.DisplayName=Username with password
CertificateCredentialsImpl.DisplayName=Certificate
CertificateCredentialsImpl.EmptyKeystore=Empty keystore
CertificateCredentialsImpl.LoadKeyFailed=Could retrieve key "{0}"
CertificateCredentialsImpl.LoadKeyFailedQueryEmptyPassword=Could retrieve key "{0}". You may need to provide a password
Comment on lines -27 to -28
Copy link
Member Author

Choose a reason for hiding this comment

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

these error messages made no sense before

CertificateCredentialsImpl.LoadKeyFailed=Couldn''t retrieve key for alias "{0}"
CertificateCredentialsImpl.LoadKeyFailedQueryEmptyPassword=Couldn''t retrieve key for alias "{0}". You may need to provide a password
CertificateCredentialsImpl.LoadKeystoreFailed=Could not load keystore
CertificateCredentialsImpl.NoCertificateUploaded=No certificate uploaded
CertificateCredentialsImpl.UploadedKeyStoreSourceDisplayName=Upload PKCS#12 certificate and key
Expand Down
18 changes: 6 additions & 12 deletions src/test/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.cloudbees.plugins.credentials.SecretBytes;
import com.cloudbees.plugins.credentials.common.CertificateCredentials;
import com.cloudbees.plugins.credentials.common.StandardCertificateCredentials;
import hudson.util.FormValidation;
import org.htmlunit.FormEncodingType;
import org.htmlunit.HttpMethod;
import org.htmlunit.Page;
Expand Down Expand Up @@ -128,14 +129,6 @@ public void doCheckUploadedKeystore_uploadedFileValid_encryptedPassword() throws
assertThat(content, containsString(EXPECTED_DISPLAY_NAME));
}

@Test
@Issue("JENKINS-64542")
public void doCheckUploadedKeystore_uploadedFileValid_butMissingPassword() throws Exception {
String content = getContentFrom_doCheckUploadedKeystore("", getValidP12_base64(), "");
assertThat(content, containsString("warning"));
assertThat(content, containsString(Util.escape(Messages.CertificateCredentialsImpl_LoadKeyFailedQueryEmptyPassword("1"))));
}

@Test
@Issue("JENKINS-64542")
public void doCheckUploadedKeystore_uploadedFileValid_butInvalidPassword() throws Exception {
Expand Down Expand Up @@ -193,10 +186,11 @@ public void doCheckUploadedKeystore_keyStoreValid_encryptedPassword() throws Exc

@Test
@Issue("JENKINS-64542")
public void doCheckUploadedKeystore_keyStoreValid_butMissingPassword() throws Exception {
String content = getContentFrom_doCheckUploadedKeystore(getValidP12_secretBytes(), "", "");
assertThat(content, containsString("warning"));
assertThat(content, containsString(Util.escape(Messages.CertificateCredentialsImpl_LoadKeyFailedQueryEmptyPassword("1"))));
public void doCheckPassword_missing() {
CertificateCredentialsImpl.DescriptorImpl descriptor = r.jenkins.getDescriptorByType(CertificateCredentialsImpl.DescriptorImpl.class);
FormValidation formValidation = descriptor.doCheckPassword("");
assertThat(formValidation.kind, is(FormValidation.Kind.OK));
assertThat(formValidation.getMessage(), is(Messages.CertificateCredentialsImpl_NoPassword()));
}

@Test
Expand Down
Loading