[JENKINS-36432 followup] Switch to SecretBytes rather than home grown secret key #6

stephenc · 2016-09-20T14:45:33Z

Now that JENKINS-36432 is available, switch to use the SecretBytes API rather than home rolled encryption from a CryptoConfidentialKey.

Advantages:

Makes it possible to programmatically construct file credentials

Makes it possible to supply credentials in XML form, e.g.

    <org.jenkinsci.plugins.plaincredentials.impl.FileCredentialsImpl plugin="plain-credentials@1.3-SNAPSHOT">
      <scope>...</scope>
      <id>...</id>
      <description>...</description>
      <fileName>secret.txt</fileName>
      <secretBytes><!-- Base64 encoded content goes here--></secretBytes>
    </org.jenkinsci.plugins.plaincredentials.impl.FileCredentialsImpl>

will be read in from disk and written down in encrypted form next time the credentials are persisted.

Uses a salted encryption so that the same content will not result in the same encrypted form... but the salt is maintained for any SecretBytes instance so that repeated saves of the same credential instance will not cause unnecessary deltas in the credential store xml file.

@reviewbybees @jenkinsci/code-reviewers @jglick

…ontent

ghost · 2016-09-20T14:48:17Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

stephenc · 2016-09-20T15:09:30Z

FTR I have manually tested the migration of secrets from the old format.

jglick · 2016-09-20T15:13:43Z

Makes it possible to supply credentials in XML form, e.g. […] will be read in from disk

Well this was already true before AFAICT.

jglick

Need a @LocalData test demonstrating migration. Also strongly recommend an addition to acceptance-test-harness demonstrating the cycle of create with upload, use from credentials-binding, update with new upload, use, update with only changes to other fields like description, use.

jglick · 2016-09-20T15:14:11Z

pom.xml

+ -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+  <modelVersion>4.0.0</modelVersion>


Why are you reformatting? Please construct a minimal diff.

jglick · 2016-09-20T15:17:23Z

src/main/java/org/jenkinsci/plugins/plaincredentials/impl/FileCredentialsImpl.java

+    @Deprecated
+    public FileCredentialsImpl(@CheckForNull CredentialsScope scope, @CheckForNull String id,
+                               @CheckForNull String description, @Nonnull FileItem file, @CheckForNull String fileName,
+                               @CheckForNull String secretBytes) throws IOException {


Better to keep the original parameter name data to avoid confusion.

jglick · 2016-09-20T15:18:30Z

src/main/java/org/jenkinsci/plugins/plaincredentials/impl/FileCredentialsImpl.java

+            this.secretBytes = SecretBytes.fromBytes(file.get());
+        } else {
+            this.fileName = fileName;
+            this.secretBytes = secretBytes;


Recommended to do some assertions here: in the first clause, secretBytes should be null; in the second, should be non-null. (Right?)

well if you re-upload the content then secretBytes may be non null when there is a file... so only the non-null check is valid

jglick · 2016-09-20T15:19:59Z

src/main/java/org/jenkinsci/plugins/plaincredentials/impl/FileCredentialsImpl.java

+         * {@inheritDoc}
+         */
+        @Override
+        public String getDisplayName() {


All gratuitous. Please avoid making changes unrelated to the stated purpose of the PR.

Once I started adding javadoc I decided it was better to just apply the standard formatting as very little remained

jglick · 2016-09-20T15:20:10Z

src/main/java/org/jenkinsci/plugins/plaincredentials/impl/StringCredentialsImpl.java

@@ -35,6 +35,8 @@

 public final class StringCredentialsImpl extends BaseStandardCredentials implements StringCredentials {

+    private static final long serialVersionUID = 4239232115673493707L;


Unrelated, better to put in a separate PR.

build was failing without it due to findbugs

Because of the parent POM update?

stephenc · 2016-09-20T15:23:27Z

Well this was already true before AFAICT.

Nope because the data was the Base64 of the encoded bytes... and there was no way to access the encoding key.

With SecretBytes, it will read Base64 of unencoded but will only write the encoded Base64 version

oleg-nenashev · 2016-09-20T15:03:54Z

pom.xml

+ ~ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ ~ THE SOFTWARE.
+ -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"


I'm fine with the formatting change, but it's unrelated

oleg-nenashev · 2016-09-20T15:03:58Z

src/main/java/org/jenkinsci/plugins/plaincredentials/impl/FileCredentialsImpl.java

@@ -1,7 +1,7 @@
 /*
 * The MIT License
 *
- * Copyright 2013 jglick.
+ * Copyright 2013 Jesse Glick, Stephen Connolly and CloudBees, Inc.


And year then?

oleg-nenashev · 2016-09-20T16:45:56Z

src/main/java/org/jenkinsci/plugins/plaincredentials/impl/FileCredentialsImpl.java


-    private final @Nonnull String fileName;
-    private final @Nonnull byte[] data;


oleg-nenashev · 2016-09-20T16:47:03Z

src/main/java/org/jenkinsci/plugins/plaincredentials/impl/FileCredentialsImpl.java

+            this.fileName = fileName;
+            this.secretBytes = secretBytes;
+        }
+        if (this.fileName == null || this.fileName.isEmpty()) {


StringUtils.isBlank() ? Otherwise space-only params may cause some issues even if such file names are actually valid

This is the logic from before just in the new constructor

oleg-nenashev · 2016-09-20T16:47:50Z

.../resources/org/jenkinsci/plugins/plaincredentials/impl/FileCredentialsImpl/credentials.jelly

@@ -35,7 +35,7 @@ THE SOFTWARE.
                <f:textbox field="fileName"/>
            </f:invisibleEntry>
            <f:invisibleEntry>
-                <f:textbox field="data"/>
+                <f:textbox field="secretBytes"/>


Causes some confusion. I'd rather keep the original var name

Perhaps not possible due to readResolve, unless the field name deliberately differs from the public getter & constructor parameter names (which is permitted but also confusing).

@oleg-nenashev it would cause more confusion to retain the previous field name given that the content of the property has changed.

- The reading of plain-text from the on-disk XML is for e.g. people using chef/puppet scripts to pre-populate their JENKINS_HOME - The reading of plain-text from the CLI create credentials command is an obvious additional use case. This also applies to the REST API for credentials creation, but as the REST API is already tested in the credentials plugin I do not see any value in adding a specific test for that scenario. - The migration of legacy data intact is also an obvious requirement

stephenc · 2016-09-23T10:09:12Z

@jglick I have addressed your request for test cases

jglick

🐝 but in the future please do not reformat unrelated lines of code.

jglick · 2016-09-23T13:23:02Z

pom.xml

+  </scm>
+
+  <properties>
+    <jenkins.version>1.609</jenkins.version>


This seems to have been an actual change.

jglick · 2016-09-23T13:23:09Z

pom.xml

+    <dependency>
+      <groupId>org.jenkins-ci.plugins</groupId>
+      <artifactId>credentials</artifactId>
+      <version>2.1.5</version>


jglick · 2016-09-23T13:23:43Z

pom.xml

+  <parent>
+    <groupId>org.jenkins-ci.plugins</groupId>
+    <artifactId>plugin</artifactId>
+    <version>2.15</version>


never said it was just a reformat... I thought I did them in separate commits... but it seems they got squashed somehow (sadpanda)

jglick · 2016-09-23T13:24:33Z

src/main/java/org/jenkinsci/plugins/plaincredentials/impl/StringCredentialsImpl.java

@@ -35,6 +35,8 @@

 public final class StringCredentialsImpl extends BaseStandardCredentials implements StringCredentials {

+    private static final long serialVersionUID = 4239232115673493707L;


Because of the parent POM update?

jglick · 2016-09-23T13:25:35Z

src/test/java/org/jenkinsci/plugins/plaincredentials/SecretBytesTest.java

+import static org.junit.Assert.*;
+import static org.junit.Assume.*;
+
+public class SecretBytesTest {


Fine, though it would make sense in FileCredentialsTest (which probably should have been FileCredentialsImplTest).

jglick · 2016-09-23T13:27:34Z

src/test/java/org/jenkinsci/plugins/plaincredentials/SecretBytesTest.java

+
+    /**
+     * Verifies that {@link SecretBytes} will treat a Base64 encoded plain text content as the content to be encrypted
+     * with the instance's secret key which gets applied when the {@link FileCredentialsImpl} is written to disk.


Same Javadoc as loadUnencrypted yet presumably this is testing something different?

jglick · 2016-09-23T13:30:01Z

.../resources/org/jenkinsci/plugins/plaincredentials/SecretBytesTest/loadUnencrypted/config.xml

@@ -0,0 +1,2 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<hudson/>


An empty root config.xml is no longer necessary with current test harness versions.

well the tests were failing.... perhaps I just need to upgrade the test harness in the pom.xml to fix that... but I'll leave that as an exercise for a future PR

jglick · 2016-09-23T13:30:12Z

...esources/org/jenkinsci/plugins/plaincredentials/SecretBytesTest/migrateLegacyData/config.xml

@@ -0,0 +1,2 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<hudson/>


jglick · 2016-09-23T13:31:00Z

...esources/org/jenkinsci/plugins/plaincredentials/SecretBytesTest/migrateLegacyData/secret.key

@@ -0,0 +1 @@
+479791e961495d820f337e3afd8fcb70aa36d8c4738093de71d13f7fe6ed6fa0


I do not think this or InstanceIdentity.KEY are actually used, are they?

stephenc added 2 commits September 20, 2016 15:01

[JENKINS-36432 followup] Upgrade dependencies and reformat pom

aefd4bd

[JENKINS-36432 followup] Switch to SecretBytes for storing the file c…

64a83e5

…ontent

Findbugs!!!

3f08ccc

jglick requested changes Sep 20, 2016

View reviewed changes

oleg-nenashev reviewed Sep 20, 2016

View reviewed changes

stephenc added 2 commits September 23, 2016 10:20

[JENKINS-36432 followup] Address some review comments

505f4e4

jglick approved these changes Sep 23, 2016

View reviewed changes

Fix javadoc comment

7bd4f7e

stephenc merged commit c3cc486 into master Sep 23, 2016

stephenc deleted the jenkins-36432-followup branch September 23, 2016 15:21

jglick mentioned this pull request Oct 28, 2016

Adding symbols to binding types for easier usage jenkinsci/credentials-binding-plugin#24

Merged

		@@ -35,6 +35,8 @@

		public final class StringCredentialsImpl extends BaseStandardCredentials implements StringCredentials {

		private static final long serialVersionUID = 4239232115673493707L;


		private final @Nonnull String fileName;
		private final @Nonnull byte[] data;

		@@ -0,0 +1,2 @@
		<?xml version='1.0' encoding='UTF-8'?>
		<hudson/>

		@@ -0,0 +1 @@
		479791e961495d820f337e3afd8fcb70aa36d8c4738093de71d13f7fe6ed6fa0

[JENKINS-36432 followup] Switch to SecretBytes rather than home grown secret key #6

[JENKINS-36432 followup] Switch to SecretBytes rather than home grown secret key #6

Conversation

stephenc commented Sep 20, 2016

ghost commented Sep 20, 2016

stephenc commented Sep 20, 2016

jglick commented Sep 20, 2016

jglick left a 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

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

stephenc commented Sep 20, 2016

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

Choose a reason for hiding this comment

stephenc commented Sep 23, 2016

jglick left a 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

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