-
Notifications
You must be signed in to change notification settings - Fork 35
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-36432 followup] Switch to SecretBytes rather than home grown secret key #6
Conversation
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. |
FTR I have manually tested the migration of secrets from the old format. |
Well this was already true before AFAICT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
--> | ||
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you reformatting? Please construct a minimal diff.
@Deprecated | ||
public FileCredentialsImpl(@CheckForNull CredentialsScope scope, @CheckForNull String id, | ||
@CheckForNull String description, @Nonnull FileItem file, @CheckForNull String fileName, | ||
@CheckForNull String secretBytes) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to keep the original parameter name data
to avoid confusion.
this.secretBytes = SecretBytes.fromBytes(file.get()); | ||
} else { | ||
this.fileName = fileName; | ||
this.secretBytes = secretBytes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommended to do some assertions here: in the first clause, secretBytes
should be null; in the second, should be non-null. (Right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
* {@inheritDoc} | ||
*/ | ||
@Override | ||
public String getDisplayName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All gratuitous. Please avoid making changes unrelated to the stated purpose of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once I started adding javadoc I decided it was better to just apply the standard formatting as very little remained
@@ -35,6 +35,8 @@ | |||
|
|||
public final class StringCredentialsImpl extends BaseStandardCredentials implements StringCredentials { | |||
|
|||
private static final long serialVersionUID = 4239232115673493707L; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated, better to put in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build was failing without it due to findbugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the parent POM update?
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 |
~ 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the formatting change, but it's unrelated
@@ -1,7 +1,7 @@ | |||
/* | |||
* The MIT License | |||
* | |||
* Copyright 2013 jglick. | |||
* Copyright 2013 Jesse Glick, Stephen Connolly and CloudBees, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And year then?
|
||
private final @Nonnull String fileName; | ||
private final @Nonnull byte[] data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+💯
this.fileName = fileName; | ||
this.secretBytes = secretBytes; | ||
} | ||
if (this.fileName == null || this.fileName.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringUtils.isBlank() ? Otherwise space-only params may cause some issues even if such file names are actually valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the logic from before just in the new constructor
@@ -35,7 +35,7 @@ THE SOFTWARE. | |||
<f:textbox field="fileName"/> | |||
</f:invisibleEntry> | |||
<f:invisibleEntry> | |||
<f:textbox field="data"/> | |||
<f:textbox field="secretBytes"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Causes some confusion. I'd rather keep the original var name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
@jglick I have addressed your request for test cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐝 but in the future please do not reformat unrelated lines of code.
</scm> | ||
|
||
<properties> | ||
<jenkins.version>1.609</jenkins.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have been an actual change.
<dependency> | ||
<groupId>org.jenkins-ci.plugins</groupId> | ||
<artifactId>credentials</artifactId> | ||
<version>2.1.5</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This too.
<parent> | ||
<groupId>org.jenkins-ci.plugins</groupId> | ||
<artifactId>plugin</artifactId> | ||
<version>2.15</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never said it was just a reformat... I thought I did them in separate commits... but it seems they got squashed somehow (sadpanda)
@@ -35,6 +35,8 @@ | |||
|
|||
public final class StringCredentialsImpl extends BaseStandardCredentials implements StringCredentials { | |||
|
|||
private static final long serialVersionUID = 4239232115673493707L; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the parent POM update?
import static org.junit.Assert.*; | ||
import static org.junit.Assume.*; | ||
|
||
public class SecretBytesTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine, though it would make sense in FileCredentialsTest
(which probably should have been FileCredentialsImplTest
).
|
||
/** | ||
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same Javadoc as loadUnencrypted
yet presumably this is testing something different?
@@ -0,0 +1,2 @@ | |||
<?xml version='1.0' encoding='UTF-8'?> | |||
<hudson/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty root config.xml
is no longer necessary with current test harness versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -0,0 +1,2 @@ | |||
<?xml version='1.0' encoding='UTF-8'?> | |||
<hudson/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -0,0 +1 @@ | |||
479791e961495d820f337e3afd8fcb70aa36d8c4738093de71d13f7fe6ed6fa0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think this or InstanceIdentity.KEY
are actually used, are they?
Now that JENKINS-36432 is available, switch to use the
SecretBytes
API rather than home rolled encryption from aCryptoConfidentialKey
.Advantages:
Makes it possible to programmatically construct file credentials
Makes it possible to supply credentials in XML form, e.g.
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