[JENKINS-72611] Forbid updating credentials ID for `IdCredentials` #506

yaroslavafenkin · 2024-01-29T12:53:31Z

Another attempt at https://issues.jenkins.io/browse/JENKINS-72611 as #502 caused regressions.

Testing done

Tested interactively, made sure that two credentials cannot have the same ID. Tested interactively with credentials that are not IdCredentials (used this).

Submitter checklist

Give feedback

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
Options

yaroslavafenkin · 2024-01-29T12:56:15Z

src/main/java/com/cloudbees/plugins/credentials/SystemCredentialsProvider.java

@@ -325,6 +326,11 @@ private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull
        checkPermission(CredentialsProvider.UPDATE);
        Map<Domain, List<Credentials>> domainCredentialsMap = getDomainCredentialsMap();
        if (domainCredentialsMap.containsKey(domain)) {
+            if (current instanceof IdCredentials && replacement instanceof IdCredentials) {


Unsure what makes more sense here AND or OR.

depends if we should be able to replace an IdCredential with a non IdCredential
To me adding or removing an ID (replacing an IdCredential with a non IdCredential or vice versa) would sound like a change in the actual Id so OR would make sense.

Any migration from a non IdCredential to IdCredential should occur seemlessly in data migration based on readResolve generating a unique Id on load, and hence this method should not have to worry about that.

But I think either AND or OR is acceptable here.

Would like to have more opinions here. Maybe @jglick or @daniel-beck ?

Tentatively would prefer || for the same reasons as James argues for it.

jtnord · 2024-01-29T14:17:14Z

(waiting for resolution of #506 (comment) before merging)

daniel-beck · 2024-01-31T11:09:20Z

src/test/java/com/cloudbees/plugins/credentials/impl/DummyCredentials.java

@@ -35,15 +34,19 @@
 /**
 * A test credentials impl.
 */
-public class DummyCredentials extends BaseCredentials implements UsernamePasswordCredentials {
+public class DummyCredentials extends BaseStandardCredentials implements UsernamePasswordCredentials {

    private final String username;

    private final Secret password;

    @DataBoundConstructor


Should the old constructor remain the DataBoundConstructor or should that move to the new one? Do we have UI tests that even use this? Should a null ID be a problem?

Do we have UI tests that even use this

there's a jelly file at least so at some point in time Stephen went to the effort of ensuring the UI at least worked...

<st:include page="id-and-description" class="${descriptor.clazz}"/>

Should a null ID be a problem?

Shouldn;t be as BaseStandardCredentials will generate a uniqueID if null (so long as not one is expecting anything stable here!).

credentials-plugin/src/main/java/com/cloudbees/plugins/credentials/impl/BaseStandardCredentials.java

Lines 78 to 82 in 7eb51b3

public BaseStandardCredentials(@CheckForNull String id, @CheckForNull String description) {

super();

this.id = IdCredentials.Helpers.fixEmptyId(id);

this.description = Util.fixNull(description);

}

credentials-plugin/src/main/java/com/cloudbees/plugins/credentials/common/IdCredentials.java

Lines 94 to 96 in 7eb51b3

public static String fixEmptyId(@CheckForNull String id) {

return StringUtils.isEmpty(id) ? UUID.randomUUID().toString() : id;

}

Just found out that https://github.com/jenkinsci/credentials-plugin/blob/master/src/test/java/com/cloudbees/plugins/credentials/impl/DummyIdCredentials.java exists. I'll revert the change here and only adapt affected tests to use DummyIdCredentials.java

jtnord · 2024-02-01T13:58:06Z

src/test/java/com/cloudbees/plugins/credentials/CredentialsProviderTest.java

@@ -457,4 +458,14 @@ public void credentialsSortedByNameInUI() {
        assertThat(options.get(0).value, is("2"));
        assertThat(options.get(1).value, is("1"));
    }
+
+    @Test
+    @Issue("SECURITY-3252")


wrong reference?

Suggested change

@Issue("SECURITY-3252")

@Issue("JENKINS-72611")

Yup, updated in eb2d6ca

basil · 2024-03-02T17:43:34Z

[ERROR] Tests run: 3, Failures: 0, Errors: 3, Skipped: 0, Time elapsed: 4.531 s <<< FAILURE! -- in org.jenkinsci.plugins.plaincredentials.BaseTest
[ERROR] org.jenkinsci.plugins.plaincredentials.BaseTest.secretFileBaseTestWithDeprecatedCtor -- Time elapsed: 4.192 s <<< ERROR!
java.lang.IllegalArgumentException: Credentials' IDs do not match, will not update.
        at com.cloudbees.plugins.credentials.SystemCredentialsProvider.updateCredentials(SystemCredentialsProvider.java:331)
        at com.cloudbees.plugins.credentials.SystemCredentialsProvider$StoreImpl.updateCredentials(SystemCredentialsProvider.java:573)
        at org.jenkinsci.plugins.plaincredentials.BaseTest.testCreateUpdateDelete(BaseTest.java:102)
        at org.jenkinsci.plugins.plaincredentials.BaseTest.secretFileTest(BaseTest.java:79)
        at org.jenkinsci.plugins.plaincredentials.BaseTest.secretFileBaseTestWithDeprecatedCtor(BaseTest.java:64)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:656)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
        at java.base/java.lang.Thread.run(Thread.java:1583)

[ERROR] org.jenkinsci.plugins.plaincredentials.BaseTest.secretFileBaseTest -- Time elapsed: 0.170 s <<< ERROR!
java.lang.IllegalArgumentException: Credentials' IDs do not match, will not update.
        at com.cloudbees.plugins.credentials.SystemCredentialsProvider.updateCredentials(SystemCredentialsProvider.java:331)
        at com.cloudbees.plugins.credentials.SystemCredentialsProvider$StoreImpl.updateCredentials(SystemCredentialsProvider.java:573)
        at org.jenkinsci.plugins.plaincredentials.BaseTest.testCreateUpdateDelete(BaseTest.java:102)
        at org.jenkinsci.plugins.plaincredentials.BaseTest.secretFileTest(BaseTest.java:79)
        at org.jenkinsci.plugins.plaincredentials.BaseTest.secretFileBaseTest(BaseTest.java:59)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:656)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
        at java.base/java.lang.Thread.run(Thread.java:1583)

[ERROR] org.jenkinsci.plugins.plaincredentials.BaseTest.secretTextBaseTest -- Time elapsed: 0.159 s <<< ERROR!
java.lang.IllegalArgumentException: Credentials' IDs do not match, will not update.
        at com.cloudbees.plugins.credentials.SystemCredentialsProvider.updateCredentials(SystemCredentialsProvider.java:331)
        at com.cloudbees.plugins.credentials.SystemCredentialsProvider$StoreImpl.updateCredentials(SystemCredentialsProvider.java:573)
        at org.jenkinsci.plugins.plaincredentials.BaseTest.testCreateUpdateDelete(BaseTest.java:102)
        at org.jenkinsci.plugins.plaincredentials.BaseTest.secretTextBaseTest(BaseTest.java:54)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:656)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
        at java.base/java.lang.Thread.run(Thread.java:1583)

[JENKINS-72611] Forbid updating credentials ID for IdCredentials

662b16b

yaroslavafenkin requested a review from a team as a code owner January 29, 2024 12:53

yaroslavafenkin commented Jan 29, 2024

View reviewed changes

yaroslavafenkin mentioned this pull request Jan 29, 2024

[JENKINS-72611] Forbid updating credentials ID for IdCredentials jenkinsci/cloudbees-folder-plugin#369

Merged

3 tasks

jtnord approved these changes Jan 29, 2024

View reviewed changes

jtnord requested a review from a team January 29, 2024 14:16

Address review feedback

0620d25

daniel-beck reviewed Jan 31, 2024

View reviewed changes

Revert DummyCredentials, use DummyIdCredentials instead

4772231

jtnord reviewed Feb 1, 2024

View reviewed changes

Fix the issue reference in a test

eb2d6ca

jtnord approved these changes Feb 1, 2024

View reviewed changes

jtnord enabled auto-merge February 1, 2024 14:37

jtnord merged commit e7f2f06 into jenkinsci:master Feb 12, 2024
15 checks passed

yaroslavafenkin mentioned this pull request Mar 4, 2024

[JENKINS-72611] Do not update credentials ID in BaseTest jenkinsci/plain-credentials-plugin#65

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-72611] Forbid updating credentials ID for `IdCredentials` #506

[JENKINS-72611] Forbid updating credentials ID for `IdCredentials` #506

yaroslavafenkin commented Jan 29, 2024

Submitter checklist

yaroslavafenkin Jan 29, 2024

jtnord Jan 29, 2024

yaroslavafenkin Jan 29, 2024

daniel-beck Jan 30, 2024

jtnord commented Jan 29, 2024

daniel-beck Jan 31, 2024

jtnord Jan 31, 2024 •

edited

Loading

yaroslavafenkin Feb 1, 2024

jtnord Feb 1, 2024

yaroslavafenkin Feb 1, 2024

basil commented Mar 2, 2024

	public BaseStandardCredentials(@CheckForNull String id, @CheckForNull String description) {
	super();
	this.id = IdCredentials.Helpers.fixEmptyId(id);
	this.description = Util.fixNull(description);
	}

	public static String fixEmptyId(@CheckForNull String id) {
	return StringUtils.isEmpty(id) ? UUID.randomUUID().toString() : id;
	}

[JENKINS-72611] Forbid updating credentials ID for IdCredentials #506

[JENKINS-72611] Forbid updating credentials ID for IdCredentials #506

Conversation

yaroslavafenkin commented Jan 29, 2024

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

jtnord commented Jan 29, 2024

Choose a reason for hiding this comment

jtnord Jan 31, 2024 • edited Loading

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

basil commented Mar 2, 2024

[JENKINS-72611] Forbid updating credentials ID for `IdCredentials` #506

[JENKINS-72611] Forbid updating credentials ID for `IdCredentials` #506

jtnord Jan 31, 2024 •

edited

Loading