Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-72611] Forbid editing credentials ID #502

Merged
merged 1 commit into from
Jan 25, 2024
Merged

[JENKINS-72611] Forbid editing credentials ID #502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/main/java/com/cloudbees/plugins/credentials/SystemCredentialsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,14 @@ private synchronized boolean removeCredentials(@NonNull Domain domain, @NonNull
private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull Credentials current,
@NonNull Credentials replacement) throws IOException {
checkPermission(CredentialsProvider.UPDATE);

// See IdCredentials.Helper#equals.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR

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

Lines 50 to 85 in cf0a900

/**
* The contract implementations of {@link #equals(Object)} and {@link #hashCode()} that implementations of
* this class must use as the basis for equality tests and as the hash code.
*
* @since 1.6
*/
final class Helpers {
/**
* Standard {@link #equals(Object)} implementation.
*
* @param self the {@code this} reference.
* @param o the other object.
* @return true if equal.
*/
public static boolean equals(IdCredentials self, Object o) {
if (self == o) {
return true;
}
if (!(o instanceof IdCredentials)) {
return false;
}
IdCredentials that = (IdCredentials) o;
return self.getId().equals(that.getId());
}
/**
* The standard {@link #hashCode()} implementation.
*
* @param self the {@code this} reference.
* @return the hash code.
*/
public static int hashCode(IdCredentials self) {
return self.getId().hashCode();
}

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

Lines 116 to 130 in cf0a900

/**
* {@inheritDoc}
*/
@Override
public final int hashCode() {
return IdCredentials.Helpers.hashCode(this);
}
/**
* {@inheritDoc}
*/
@Override
public final boolean equals(Object o) {
return IdCredentials.Helpers.equals(this, o);
}

// Note: We probably should not assume that all credentials are IdCredentials. We also do this a few lines below in list.indexOf(current) call.
// If this one breaks, that one breaks as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how so - the list is of type Credentials not IdCredentials so that would not break with a big bad angry jenkins.
At worst it would not find current, as the list would use equals and it is up to current credential may have been correctly obtained by it's name or otherwise in the code path.

if (!current.equals(replacement)) {
throw new IllegalArgumentException("Credentials' IDs do not match, will not update.");
}

Map<Domain, List<Credentials>> domainCredentialsMap = getDomainCredentialsMap();
if (domainCredentialsMap.containsKey(domain)) {
List<Credentials> list = domainCredentialsMap.get(domain);
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/com/cloudbees/plugins/credentials/UserCredentialsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,14 @@ private synchronized boolean removeCredentials(@NonNull Domain domain, @NonNull
private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull Credentials current,
@NonNull Credentials replacement) throws IOException {
checkPermission(CredentialsProvider.UPDATE);

// See IdCredentials.Helper#equals.
// Note: We probably should not assume that all credentials are IdCredentials. We also do this a few lines below in list.indexOf(current) call.
// If this one breaks, that one breaks as well.
if (!current.equals(replacement)) {
throw new IllegalArgumentException("Credentials' IDs do not match, will not update.");
}

Map<Domain, List<Credentials>> domainCredentialsMap = getDomainCredentialsMap();
if (domainCredentialsMap.containsKey(domain)) {
List<Credentials> list = domainCredentialsMap.get(domain);
Expand Down
15 changes: 13 additions & 2 deletions src/test/java/com/cloudbees/plugins/credentials/CredentialsProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import hudson.security.ACL;
import hudson.util.ListBoxModel;
import jenkins.model.Jenkins;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.Issue;
Expand Down Expand Up @@ -255,7 +256,7 @@ public void testManageUserCredentials() throws IOException {
final User alice = User.getById("alice", true);
DummyCredentials aliceCred1 = new DummyCredentials(CredentialsScope.USER, "aliceCred1", "pwd");
DummyCredentials aliceCred2 = new DummyCredentials(CredentialsScope.USER, "aliceCred2", "pwd");
DummyCredentials aliceCred3 = new DummyCredentials(CredentialsScope.USER, "aliceCred3", "pwd");
DummyCredentials aliceCred3 = new DummyCredentials(CredentialsScope.USER, aliceCred1.getId(), aliceCred1.getDescription(), "aliceCred3", "pwd");

r.jenkins.setSecurityRealm(r.createDummySecurityRealm());

Expand Down Expand Up @@ -289,7 +290,7 @@ public void testUpdateAndDeleteCredentials() throws IOException {
DummyCredentials systemCred = new DummyCredentials(CredentialsScope.SYSTEM, "systemCred", "pwd");
DummyCredentials systemCred2 = new DummyCredentials(CredentialsScope.SYSTEM, "systemCred2", "pwd");
DummyCredentials globalCred = new DummyCredentials(CredentialsScope.GLOBAL, "globalCred", "pwd");
DummyCredentials modCredential = new DummyCredentials(CredentialsScope.GLOBAL, "modCredential", "pwd");
DummyCredentials modCredential = new DummyCredentials(CredentialsScope.GLOBAL, globalCred.getId(), globalCred.getDescription(), "modCredential", "pwd");

CredentialsStore store = CredentialsProvider.lookupStores(Jenkins.get()).iterator().next();

Expand Down Expand Up @@ -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")
public void cannotUpdateCredentialsId() {
DummyCredentials cred1 = new DummyCredentials(CredentialsScope.GLOBAL, "cred1", "pwd");
DummyCredentials cred2 = new DummyCredentials(CredentialsScope.GLOBAL, "cred2", "pwd");
CredentialsStore store = CredentialsProvider.lookupStores(Jenkins.get()).iterator().next();

Assert.assertThrows(IllegalArgumentException.class, () -> store.updateCredentials(Domain.global(), cred1, cred2));
}
}
8 changes: 8 additions & 0 deletions src/test/java/com/cloudbees/plugins/credentials/MockFolderCredentialsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,14 @@ private synchronized boolean removeCredentials(@NonNull Domain domain, @NonNull
private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull Credentials current,
@NonNull Credentials replacement) throws IOException {
checkPermission(CredentialsProvider.UPDATE);

// See IdCredentials.Helper#equals.
// Note: We probably should not assume that all credentials are IdCredentials. We also do this a few lines below in list.indexOf(current) call.
// If this one breaks, that one breaks as well.
if (!current.equals(replacement)) {
throw new IllegalArgumentException("Credentials' IDs do not match, will not update.");
}

Map<Domain, List<Credentials>> domainCredentialsMap = getDomainCredentialsMap();
if (domainCredentialsMap.containsKey(domain)) {
List<Credentials> list = domainCredentialsMap.get(domain);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/cloudbees/plugins/credentials/domains/DomainTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public void testCredentialsInCustomDomains() throws IOException {
Domain domainBar = new Domain("domainBar", "Path domain", Arrays.asList(new DomainSpecification[] { new HostnameSpecification("bar.com", "") }));
DummyCredentials systemCred = new DummyCredentials(CredentialsScope.SYSTEM, "systemCred", "pwd");
DummyCredentials systemCred1 = new DummyCredentials(CredentialsScope.SYSTEM, "systemCred1", "pwd");
DummyCredentials systemCredMod = new DummyCredentials(CredentialsScope.SYSTEM, "systemCredMod", "pwd");
DummyCredentials systemCredMod = new DummyCredentials(CredentialsScope.SYSTEM, systemCred.getId(), systemCred.getDescription(), "systemCredMod", "pwd");

CredentialsStore store = CredentialsProvider.lookupStores(Jenkins.get()).iterator().next();

Expand Down
9 changes: 6 additions & 3 deletions src/test/java/com/cloudbees/plugins/credentials/impl/DummyCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
*/
package com.cloudbees.plugins.credentials.impl;

import com.cloudbees.plugins.credentials.BaseCredentials;
import com.cloudbees.plugins.credentials.CredentialsDescriptor;
import com.cloudbees.plugins.credentials.CredentialsScope;
import com.cloudbees.plugins.credentials.common.UsernamePasswordCredentials;
Expand All @@ -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
public DummyCredentials(CredentialsScope scope, String username, String password) {
super(scope);
this(scope, null, null, username, password);
}

public DummyCredentials(CredentialsScope scope, String id, String description, String username, String password) {
super(scope, id, description);
this.username = username;
this.password = Secret.fromString(password);
}
Expand Down