Skip to content

Commit

Permalink
[SECURITY-3022]
Browse files Browse the repository at this point in the history
  • Loading branch information
@jtnord
jtnord committed Jan 11, 2023
1 parent 128ee98 commit 862c6e5
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 6 deletions.
26 changes: 25 additions & 1 deletion ...oudbees/jenkins/plugins/kubernetes_credentials_provider/KubernetesCredentialProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.ExtensionList;
import hudson.model.AdministrativeMonitor;
import hudson.model.Item;
import hudson.triggers.SafeTimerTask;
import hudson.util.AdministrativeError;
import io.fabric8.kubernetes.api.model.LabelSelector;
Expand Down Expand Up @@ -64,8 +65,10 @@
import jenkins.model.Jenkins;
import com.cloudbees.plugins.credentials.Credentials;
import com.cloudbees.plugins.credentials.CredentialsProvider;
import com.cloudbees.plugins.credentials.CredentialsScope;
import com.cloudbees.plugins.credentials.CredentialsStore;
import com.cloudbees.plugins.credentials.common.IdCredentials;
import com.cloudbees.plugins.credentials.domains.DomainRequirement;

@Extension
public class KubernetesCredentialProvider extends CredentialsProvider implements Watcher<Secret> {
Expand Down Expand Up @@ -213,7 +216,9 @@ public <C extends Credentials> List<C> getCredentials(Class<C> type, ItemGroup i
for (IdCredentials credential : credentials.values()) {
// is s a type of type then populate the list...
LOG.log(Level.FINEST, "getCredentials {0} is a possible candidate", credential.getId());
if (type.isAssignableFrom(credential.getClass())) {
if (CredentialsScope.SYSTEM == credential.getScope() && !(itemGroup instanceof Jenkins)) {
LOG.log(Level.FINEST, "getCredentials {0} has SYSTEM scope, but the context is not Jenkins, ignoring", credential.getId());
} else if (type.isAssignableFrom(credential.getClass())) {
LOG.log(Level.FINEST, "getCredentials {0} matches, adding to list", credential.getId());
// cast to keep generics happy even though we are assignable..
list.add(type.cast(credential));
Expand All @@ -226,6 +231,25 @@ public <C extends Credentials> List<C> getCredentials(Class<C> type, ItemGroup i
return emptyList();
}

@Override
@NonNull
public <C extends Credentials> List<C> getCredentials(@NonNull Class<C> type,
@NonNull Item item,
Authentication authentication) {
// we do not support scoping to Items, so we just need to use null to not expose SYSTEM credentials to Items.
Objects.requireNonNull(item);
return getCredentials(type, (ItemGroup)null, authentication);
}

@Override
public <C extends Credentials> List<C> getCredentials(@NonNull Class<C> type,
@NonNull Item item,
Authentication authentication,
List<DomainRequirement> domainRequirements) {
// we do not support domain requirements
return getCredentials(type, item, authentication);
}

@SuppressWarnings("null")
private final @NonNull <T> List<T> emptyList() {
// just a separate method to avoid having to suppress "null" for the entirety of getCredentials
Expand Down
71 changes: 66 additions & 5 deletions ...es/jenkins/plugins/kubernetes_credentials_provider/KubernetesCredentialsProviderTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
package com.cloudbees.jenkins.plugins.kubernetes_credentials_provider;

import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.hasProperty;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.*;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.*;
Expand All @@ -9,14 +13,17 @@
import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;

import com.cloudbees.jenkins.plugins.kubernetes_credentials_provider.convertors.UsernamePasswordCredentialsConvertor;
import com.cloudbees.plugins.credentials.CredentialsScope;
import com.cloudbees.plugins.credentials.common.UsernamePasswordCredentials;
import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl;
import hudson.ExtensionList;
import hudson.model.AdministrativeMonitor;
import hudson.model.Item;
import hudson.model.ItemGroup;
import hudson.security.ACL;
import io.fabric8.kubernetes.api.model.*;
Expand All @@ -29,6 +36,7 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.jvnet.hudson.test.Issue;
import org.mockito.Answers;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
Expand All @@ -43,7 +51,7 @@ public class KubernetesCredentialsProviderTest {

private @Mock(answer = Answers.CALLS_REAL_METHODS) MockedStatic<ExtensionList> extensionList;
private @Mock MockedStatic<Timer> timer;

@Before
public void setUp() {
// mocked to validate add/remove of administrative errors
Expand Down Expand Up @@ -73,9 +81,9 @@ private void mockKubernetesResponses(String labelSelector) {

@Test
public void startWatchingForSecrets() {
Secret s1 = createSecret("s1");
Secret s2 = createSecret("s2");
Secret s3 = createSecret("s3");
Secret s1 = createSecret("s1", null);
Secret s2 = createSecret("s2", null);
Secret s3 = createSecret("s3", null);

// returns s1 and s3, the credentials map should be reset to this list
server.expect().withPath("/api/v1/namespaces/test/secrets?labelSelector=jenkins.io%2Fcredentials-type")
Expand Down Expand Up @@ -104,12 +112,65 @@ public void startWatchingForSecrets() {
assertTrue("secret s3 exists", credentials.stream().anyMatch(c -> "s3".equals(((UsernamePasswordCredentialsImpl) c).getId())));
}

private Secret createSecret(String name) {
@Issue("SECURITY-3022")
@Test
public void credentialScope() {
Secret s1 = createSecret("s1", CredentialsScope.GLOBAL);
Secret s2 = createSecret("s2", CredentialsScope.SYSTEM);
Secret s3 = createSecret("s3", CredentialsScope.GLOBAL);

// returns s1 and s3, the credentials map should be reset to this list
server.expect().withPath("/api/v1/namespaces/test/secrets?labelSelector=jenkins.io%2Fcredentials-type")
.andReturn(200, new SecretListBuilder()
.withNewMetadata()
.withResourceVersion("1")
.endMetadata()
.addToItems(s1, s2, s3)
.build())
.once();

// expect the s2 will get dropped when the credentials map is reset to the full list
server.expect().withPath("/api/v1/namespaces/test/secrets?labelSelector=jenkins.io%2Fcredentials-type&watch=true")
.andReturnChunked(200, new WatchEvent(s1, "ADDED"), new WatchEvent(s2, "ADDED"))
.once();
server.expect().withPath("/api/v1/namespaces/test/secrets?labelSelector=jenkins.io%2Fcredentials-type&watch=true")
.andReturn(200, null)
.always();

KubernetesCredentialProvider provider = new MockedKubernetesCredentialProvider();
provider.startWatchingForSecrets();

List<UsernamePasswordCredentials> credentials;

credentials = provider.getCredentials(UsernamePasswordCredentials.class, (ItemGroup) null, ACL.SYSTEM);
assertThat("null itemgroup so system scoped credentials (s2) are *not* available", credentials,
containsInAnyOrder(hasProperty("id", is("s1")), hasProperty("id", is("s3"))));

credentials = provider.getCredentials(UsernamePasswordCredentials.class, mock(Jenkins.class), ACL.SYSTEM);
assertThat("itemgroup is Jenkins so system scoped credentials (s2) are available", credentials,
containsInAnyOrder(hasProperty("id", is("s1")), hasProperty("id", is("s2")), hasProperty("id", is("s3"))));

credentials = provider.getCredentials(UsernamePasswordCredentials.class, mock(ItemGroup.class), ACL.SYSTEM);
assertThat("itemgroup is not Jenkins so system scoped credentials (s2) are *not* available", credentials,
containsInAnyOrder(hasProperty("id", is("s1")), hasProperty("id", is("s3"))));

credentials = provider.getCredentials(UsernamePasswordCredentials.class, mock(Item.class), ACL.SYSTEM);
assertThat("items do not have access to system scoped credentials (s2) so should not be available", credentials,
containsInAnyOrder(hasProperty("id", is("s1")), hasProperty("id", is("s3"))));

credentials = provider.getCredentials(UsernamePasswordCredentials.class, mock(Item.class), ACL.SYSTEM, Collections.emptyList());
assertThat("items do not have access to system scoped credentials (s2) so should not be available", credentials,
containsInAnyOrder(hasProperty("id", is("s1")), hasProperty("id", is("s3"))));

}

private Secret createSecret(String name, CredentialsScope scope) {
return new SecretBuilder()
.withNewMetadata()
.withNamespace("test")
.withName(name)
.addToLabels("jenkins.io/credentials-type", "usernamePassword")
.addToLabels("jenkins.io/credentials-scope", scope == null ? "global" : scope.name().toLowerCase(Locale.ROOT))
.endMetadata()
.addToData("username", "bXlVc2VybmFtZQ==")
.addToData("password", "UGEkJHdvcmQ=")
Expand Down

0 comments on commit 862c6e5

Please sign in to comment.