Merge pull request #83 from cyrille-leclerc/JENKINS-41952

[JENKINS-41952] mismatch on the encoding of the keystore: sometimes as `Secret(base64(keystore))` and sometimes as `new SecretBytes(keystore).toString()`
jenkinsci · Feb 20, 2017 · cc0cd3a · cc0cd3a
2 parents 72aef0d + e0382ce
commit cc0cd3a
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 18 deletions.
diff --git a/src/main/java/com/cloudbees/plugins/credentials/SecretBytes.java b/src/main/java/com/cloudbees/plugins/credentials/SecretBytes.java
@@ -45,6 +45,7 @@
 import javax.crypto.Cipher;
 import jcifs.util.Base64;
 import jenkins.security.ConfidentialStore;
+import org.apache.commons.lang.IllegalClassException;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.Stapler;
@@ -351,17 +352,15 @@ public Object unmarshal(HierarchicalStreamReader reader, final UnmarshallingCont
         }
     }
 
-    /*
-     * Register a converter for Stapler form binding.
-     */
-    static {
-        Stapler.CONVERT_UTILS.register(new org.apache.commons.beanutils.Converter() {
-            /**
-             * {@inheritDoc}
-             */
-            public SecretBytes convert(Class type, Object value) {
-                return SecretBytes.fromString(value.toString());
+    @Restricted(NoExternalUse.class)
+    public static class StaplerConverterImpl implements org.apache.commons.beanutils.Converter {
+        public SecretBytes convert(Class type, Object value) {
+            if (value==null)
+                return null;
+            if (value instanceof String) {
+                return SecretBytes.fromString((String) value);
             }
-        }, SecretBytes.class);
+            throw new IllegalClassException(SecretBytes.class, value.getClass());
+        }
     }
 }
diff --git a/src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java b/src/main/java/com/cloudbees/plugins/credentials/impl/CertificateCredentialsImpl.java
@@ -31,6 +31,7 @@
 import com.trilead.ssh2.crypto.Base64;
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
+import edu.umd.cs.findbugs.annotations.Nullable;
 import hudson.Extension;
 import hudson.Util;
 import hudson.model.AbstractDescribableImpl;
@@ -305,6 +306,10 @@ protected KeyStoreSourceDescriptor(Class<? extends KeyStoreSource> clazz) {
         protected static FormValidation validateCertificateKeystore(String type, byte[] keystoreBytes,
                                                                     String password) {
 
+            if (keystoreBytes == null || keystoreBytes.length == 0) {
+                return FormValidation.warning(Messages.CertificateCredentialsImpl_LoadKeystoreFailed());
+            }
+
             char[] passwordChars = toCharArray(Secret.fromString(password));
             try {
                 KeyStore keyStore = KeyStore.getInstance(type);
@@ -594,7 +599,8 @@ public static class DescriptorImpl extends KeyStoreSourceDescriptor {
              * @return the keystore bytes.
              * @see #toSecret(byte[])
              */
-            public static byte[] toByteArray(Secret secret) {
+            @NonNull
+            public static byte[] toByteArray(@Nullable Secret secret) {
                 if (secret != null) {
                     try {
                         return Base64.decode(secret.getPlainText().toCharArray());
@@ -611,8 +617,11 @@ public static byte[] toByteArray(Secret secret) {
              * @param contents the keystore bytes.
              * @return the keystore as a secret.
              * @see #toByteArray(Secret)
+             * @deprecated use {@link SecretBytes#fromBytes(byte[])}
              */
-            public static Secret toSecret(byte[] contents) {
+            @Deprecated
+            @CheckForNull
+            public static Secret toSecret(@Nullable byte[] contents) {
                 return contents == null || contents.length == 0
                         ? null
                         : Secret.fromString(new String(Base64.encode(contents)));
@@ -640,7 +649,14 @@ public FormValidation doCheckUploadedKeystore(@QueryParameter String value,
                 if (StringUtils.isBlank(value)) {
                     return FormValidation.error(Messages.CertificateCredentialsImpl_NoCertificateUploaded());
                 }
-                return validateCertificateKeystore("PKCS12", toByteArray(Secret.fromString(value)), password);
+
+                SecretBytes secretBytes = SecretBytes.fromString(value);
+                byte[] keystoreBytes = secretBytes.getPlainData();
+                if (keystoreBytes == null || keystoreBytes.length == 0) {
+                    return FormValidation.error(Messages.CertificateCredentialsImpl_LoadKeystoreFailed());
+                }
+
+                return validateCertificateKeystore("PKCS12", keystoreBytes, password);
             }
 
             /**
@@ -673,7 +689,7 @@ public static class Upload {
              * The uploaded content.
              */
             @CheckForNull
-            private final Secret uploadedKeystore;
+            private final SecretBytes uploadedKeystore;
 
             /**
              * Our constructor.
@@ -682,7 +698,7 @@ public static class Upload {
              *                         pop-up to inject the uploaded content into.
              * @param uploadedKeystore the content.
              */
-            public Upload(@NonNull String divId, @CheckForNull Secret uploadedKeystore) {
+            public Upload(@NonNull String divId, @CheckForNull SecretBytes uploadedKeystore) {
                 this.divId = divId;
                 this.uploadedKeystore = uploadedKeystore;
             }
@@ -705,7 +721,7 @@ public String getDivId() {
              * @return the content.
              */
             @SuppressWarnings("unused") // used by Jelly EL
-            public Secret getUploadedKeystore() {
+            public SecretBytes getUploadedKeystore() {
                 return uploadedKeystore;
             }
 
@@ -728,8 +744,9 @@ public HttpResponse doUpload(@NonNull StaplerRequest req) throws ServletExceptio
                 // view for that instance. The "complete" view can then do the injection and close itself so that
                 // the user experience is the pop-up then click upload and finally we inject back in the content to
                 // the form.
+                SecretBytes uploadedKeystore = SecretBytes.fromBytes(file.get());
                 return HttpResponses.forwardToView(
-                        new Upload(getDivId(), UploadedKeyStoreSource.DescriptorImpl.toSecret(file.get())), "complete");
+                        new Upload(getDivId(), uploadedKeystore), "complete");
             }
         }
     }