Handle password with empty string for certificate auth #581

timja · 2024-12-07T20:10:53Z

Testing done

Uploaded a cert generated inside Azure Key Vault

Tested with jenkinsci/azure-credentials-plugin#274
and checked end 2 end and I can provision VMs with https://plugins.jenkins.io/azure-vm-agents using certificate auth

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

timja · 2024-12-07T22:59:26Z

src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

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


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

Shouldn't modify what the user has passed here

timja · 2024-12-07T23:00:11Z

src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

@@ -248,7 +246,7 @@ public FormValidation doCheckPassword(@QueryParameter String value) {
                return FormValidation.error(Messages.CertificateCredentialsImpl_ShortPasswordFIPS());
            }
            if (pw.isEmpty()) {
-                return FormValidation.warning(Messages.CertificateCredentialsImpl_NoPassword());
+                return FormValidation.ok(Messages.CertificateCredentialsImpl_NoPassword());


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

this is a valid usecase but its still worth making people aware so I left it as ok

timja · 2024-12-07T23:00:20Z

src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

@@ -624,9 +622,7 @@ protected static FormValidation validateCertificateKeystore(byte[] keystoreBytes
                } catch (KeyStoreException | CertificateException | NoSuchAlgorithmException | IOException e) {
                    return FormValidation.warning(e, Messages.CertificateCredentialsImpl_LoadKeystoreFailed());
                } finally {
-                    if (passwordChars != null) {


no longer possible to be null here

timja · 2024-12-07T23:01:30Z

src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

@@ -739,6 +735,9 @@ public FormValidation doCheckCertChain(@QueryParameter String value) {
                    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) {


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:

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.

timja · 2024-12-07T23:01:35Z

src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

@@ -771,6 +770,9 @@ public FormValidation doCheckPrivateKey(@QueryParameter String value,
                    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) {


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:

timja · 2024-12-07T23:01:54Z

src/main/resources/com/cloudbees/plugins/credentials/impl/Messages.properties

-CertificateCredentialsImpl.LoadKeyFailed=Could retrieve key "{0}"
-CertificateCredentialsImpl.LoadKeyFailedQueryEmptyPassword=Could retrieve key "{0}". You may need to provide a password


these error messages made no sense before

jtnord

I think this fixes the issue on azure, but breaks for others where there is no password at all (rather than an empty password)

jtnord · 2024-12-08T23:17:14Z

src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java

     */
-    @CheckForNull


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.

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

jtnord

tested with manual uploads of unprotected PEMs and worked fine.

Attempted to create a passwordless pkcs12 and failed (openssl and via java KeyStore) so taking this as a non password protected pkcs12 store is exceptionally rare.

jtnord · 2024-12-10T11:30:01Z

Thanks for the investigation and fix

timja · 2024-12-10T12:16:35Z

Attempted to create a passwordless pkcs12 and failed (openssl and via java KeyStore) so taking this as a non password protected pkcs12 store is exceptionally rare.

I saw a bug report on the Java issue tracker which says it should be possible from Java 18 but YMMV, I expect this is the more common case...

timja force-pushed the azure-pfx branch from 3a1d390 to 866ecfc Compare December 7, 2024 21:01

timja mentioned this pull request Dec 7, 2024

Fix certificate auth jenkinsci/azure-credentials-plugin#274

Merged

6 tasks

Handle password with empty string for certificate auth

d0177a1

timja force-pushed the azure-pfx branch from 866ecfc to d0177a1 Compare December 7, 2024 22:05

timja commented Dec 7, 2024

View reviewed changes

timja marked this pull request as ready for review December 7, 2024 23:02

timja requested a review from a team as a code owner December 7, 2024 23:02

timja requested a review from jtnord December 7, 2024 23:02

timja added the enhancement label Dec 7, 2024

jtnord reviewed Dec 8, 2024

View reviewed changes

jtnord approved these changes Dec 10, 2024

View reviewed changes

jtnord merged commit 6017143 into jenkinsci:master Dec 10, 2024
18 checks passed

timja deleted the azure-pfx branch December 10, 2024 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle password with empty string for certificate auth #581

Handle password with empty string for certificate auth #581

timja commented Dec 7, 2024 •

edited

Loading

timja Dec 7, 2024

timja Dec 7, 2024

timja Dec 7, 2024

timja Dec 7, 2024

jtnord Dec 10, 2024

timja Dec 7, 2024

timja Dec 7, 2024

jtnord left a comment

jtnord Dec 8, 2024

timja Dec 9, 2024

jtnord left a comment

jtnord commented Dec 10, 2024

timja commented Dec 10, 2024 •

edited

Loading

		CertificateCredentialsImpl.LoadKeyFailed=Could retrieve key "{0}"
		CertificateCredentialsImpl.LoadKeyFailedQueryEmptyPassword=Could retrieve key "{0}". You may need to provide a password

Handle password with empty string for certificate auth #581

Handle password with empty string for certificate auth #581

Conversation

timja commented Dec 7, 2024 • edited Loading

Testing done

Submitter checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

jtnord commented Dec 10, 2024

timja commented Dec 10, 2024 • edited Loading

timja commented Dec 7, 2024 •

edited

Loading

timja commented Dec 10, 2024 •

edited

Loading