Skip to content

Commit

Permalink
[FIXED JENKINS-31610] User may view some information in credential-st…
Browse files Browse the repository at this point in the history 
…ore of other users

- Not really considered as a security issue as the user could not view the actual secrets of the credentials, but better to lock it down in any case
  • Loading branch information
@stephenc
stephenc committed Mar 23, 2016
1 parent b3e7589 commit 3876043
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 23 deletions.
50 changes: 37 additions & 13 deletions src/main/java/com/cloudbees/plugins/credentials/UserCredentialsProvider.java
View file
Expand Up @@ -43,18 +43,9 @@
import hudson.model.UserProperty;
import hudson.model.UserPropertyDescriptor;
import hudson.security.ACL;
import hudson.security.AccessDeniedException2;
import hudson.security.Permission;
import hudson.util.CopyOnWriteMap;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.acegisecurity.Authentication;
import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.export.Exported;
import org.kohsuke.stapler.export.ExportedBean;

import java.io.IOException;
import java.io.ObjectStreamException;
import java.util.ArrayList;
Expand All @@ -67,6 +58,17 @@
import java.util.logging.Level;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.acegisecurity.Authentication;
import org.acegisecurity.context.SecurityContext;
import org.acegisecurity.context.SecurityContextHolder;
import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.export.Exported;
import org.kohsuke.stapler.export.ExportedBean;

import static com.cloudbees.plugins.credentials.CredentialsMatchers.always;

Expand Down Expand Up @@ -142,14 +144,27 @@ public <C extends Credentials> List<C> getCredentials(@NonNull Class<C> type,
if (user != null) {
UserCredentialsProperty property = user.getProperty(UserCredentialsProperty.class);
if (property != null) {
return DomainCredentials
.getCredentials(property.getDomainCredentialsMap(), type, domainRequirements, always());
// we need to impersonate if the requesting authentication is not the current authentication.
boolean needImpersonation = user.equals(User.current());
SecurityContext old = needImpersonation ? null : ACL.impersonate(user.impersonate());

This comment has been minimized.

Sign in to view

Copy link
@jglick

jglick Mar 31, 2016

Member

cf. #45
try {
return DomainCredentials
.getCredentials(property.getDomainCredentialsMap(), type, domainRequirements, always());
} finally {
if (needImpersonation) {
// restore the current authentication if we impersonated.
SecurityContextHolder.setContext(old);
}
}
}
}
}
return new ArrayList<C>();
}

/**
* {@inheritDoc}
*/
@Override
public CredentialsStore getStore(@CheckForNull ModelObject object) {
if (object instanceof User) {
Expand Down Expand Up @@ -223,6 +238,7 @@ private Object readResolve() throws ObjectStreamException {
* @return the subset of the user's credentials that are of the specified type.
*/
public <C extends Credentials> List<C> getCredentials(Class<C> type) {
checkPermission(CredentialsProvider.VIEW);
List<C> result = new ArrayList<C>();
for (Credentials credential : getCredentials()) {
if (type.isInstance(credential)) {
Expand All @@ -239,6 +255,7 @@ public <C extends Credentials> List<C> getCredentials(Class<C> type) {
*/
@SuppressWarnings("unused") // used by stapler
public List<Credentials> getCredentials() {
checkPermission(CredentialsProvider.VIEW);
return domainCredentialsMap.get(Domain.global());
}

Expand All @@ -250,6 +267,7 @@ public List<Credentials> getCredentials() {
*/
@SuppressWarnings("unused") // used by stapler
public List<DomainCredentials> getDomainCredentials() {
checkPermission(CredentialsProvider.VIEW);
return DomainCredentials.asList(getDomainCredentialsMap());
}

Expand All @@ -261,6 +279,7 @@ public List<DomainCredentials> getDomainCredentials() {
@SuppressWarnings("deprecation")
@NonNull
public synchronized Map<Domain, List<Credentials>> getDomainCredentialsMap() {
checkPermission(CredentialsProvider.VIEW);
return domainCredentialsMap = DomainCredentials.migrateListToMap(domainCredentialsMap, credentials);
}

Expand All @@ -271,6 +290,7 @@ public synchronized Map<Domain, List<Credentials>> getDomainCredentialsMap() {
* @since 1.5
*/
public synchronized void setDomainCredentialsMap(Map<Domain, List<Credentials>> domainCredentialsMap) {
checkPermission(CredentialsProvider.MANAGE_DOMAINS);
this.domainCredentialsMap = DomainCredentials.toCopyOnWriteMap(domainCredentialsMap);
}

Expand Down Expand Up @@ -408,7 +428,11 @@ private synchronized boolean updateCredentials(@NonNull Domain domain, @NonNull
}

private void checkPermission(Permission p) {
user.checkPermission(p);
if (user.equals(User.current())) {
user.checkPermission(p);
} else {
throw new AccessDeniedException2(Jenkins.getAuthentication(), p);
}
}

private void save() throws IOException {
Expand Down
36 changes: 26 additions & 10 deletions src/test/java/com/cloudbees/plugins/credentials/impl/BaseStandardCredentialsTest.java
View file
Expand Up @@ -33,15 +33,21 @@
import hudson.model.User;
import hudson.security.ACL;
import hudson.util.FormValidation;
import static hudson.util.FormValidation.Kind.*;
import java.io.IOException;
import java.util.Iterator;
import org.junit.Test;
import static org.junit.Assert.*;
import org.acegisecurity.context.SecurityContext;
import org.acegisecurity.context.SecurityContextHolder;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.MockFolder;

import static hudson.util.FormValidation.Kind.ERROR;
import static hudson.util.FormValidation.Kind.OK;
import static hudson.util.FormValidation.Kind.WARNING;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class BaseStandardCredentialsTest {

@Rule public JenkinsRule r = new JenkinsRule();
Expand All @@ -57,16 +63,26 @@ public class BaseStandardCredentialsTest {
// First set up two users, each of which has an existing credentials named ‘per-user’.
r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
final User alice = User.get("alice");
CredentialsStore store = lookupStore(alice);
addCreds(store, CredentialsScope.USER, "alice");
addCreds(store, CredentialsScope.USER, "per-user");
SecurityContext ctx = ACL.impersonate(alice.impersonate());
try {
CredentialsStore store = lookupStore(alice);
addCreds(store, CredentialsScope.USER, "alice");
addCreds(store, CredentialsScope.USER, "per-user");
} finally {
SecurityContextHolder.setContext(ctx);
}
User bob = User.get("bob");
store = lookupStore(bob);
addCreds(store, CredentialsScope.USER, "bob");
addCreds(store, CredentialsScope.USER, "per-user");
ctx = ACL.impersonate(bob.impersonate());
try {
CredentialsStore store = lookupStore(bob);
addCreds(store, CredentialsScope.USER, "bob");
addCreds(store, CredentialsScope.USER, "per-user");
} finally {
SecurityContextHolder.setContext(ctx);
}

// Now set up a folder tree with some masking of credentials.
store = lookupStore(r.jenkins);
CredentialsStore store = lookupStore(r.jenkins);
addCreds(store, CredentialsScope.GLOBAL, "masked");
addCreds(store, CredentialsScope.GLOBAL, "root");
// TODO not currently testing SYSTEM; should this make any difference to behavior here?
Expand Down

0 comments on commit 3876043

Please sign in to comment.