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.

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

Support scoping of credentials to lists of ItemGroups #93

Merged
merged 3 commits into from
Feb 29, 2024

Conversation

schiasileon
Copy link
Contributor

This supersedes #40.
Linked issues:
https://issues.jenkins.io/browse/JENKINS-63416
https://issues.jenkins.io/browse/JENKINS-53105

This PR introduces folder-scoping of credentials by adding the jenkins.io/credentials-item-group annotation. Credentials will then only be available to the specified ItemGroup/Folder/Job + children.

Improvements to #40 :

  • Introduced a new type for storing parsed credential data apart from IdCredential
  • Syntax of annotation is now ["/job/thisIsJobA/", "/job/thisIsJobB/job/nestedJob/"] allowing for all paths to be parsed to list
  • All paths must begin and end with '/'
  • Previous label is now an annotation

@schiasileon schiasileon requested a review from a team as a code owner December 20, 2023 11:09
@@ -62,6 +66,8 @@ public abstract class SecretUtils {

static final String JENKINS_IO_CREDENTIALS_SCOPE_LABEL = "jenkins.io/credentials-scope";

/** Optional annotation containing a list of job folders this credential is available to */
static final String JENKINS_IO_CREDENTIALS_ITEM_GROUP_ANNOTATION = "jenkins.io/credentials-item-group";
Copy link
Contributor Author

@schiasileon schiasileon Dec 20, 2023

Choose a reason for hiding this comment

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

open for discussion on a better annotation name

Copy link
Member

Choose a reason for hiding this comment

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

naming is hard :)

jenkins.io/credentials-store-location (s) ?

@schiasileon
Copy link
Contributor Author

schiasileon commented Dec 20, 2023

FYI: @jtnord @danielwegener

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

what about the UI/UX? Would this not show credentials as avaialble at the root which are not actually available (as they are only available in a specific folder)?

Additionally the docs should describe how to use the feature.

// Only the global domain is supported
if (Domain.global().equals(domain) && Jenkins.getInstance().hasPermission(CredentialsProvider.VIEW))
return provider.getCredentials(Credentials.class, Jenkins.getInstance(), ACL.SYSTEM);
if(Jenkins.getInstance().hasPermission(CredentialsProvider.VIEW)) {
Copy link
Member

@jtnord jtnord Dec 20, 2023

Choose a reason for hiding this comment

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

the Access check should be scoped to the context. if you have CredentialsProvider.View on a folder (but not Jenkins itself) you should be able to view credentials assigned to that folder

Additionally I beleive we still only support a global domain?

Suggested change
if(Jenkins.getInstance().hasPermission(CredentialsProvider.VIEW)) {
// Only the global domain is supported
if (Domain.global().equals(domain)) {
// walk the parent until we get the AccessControlled object
AccessControlled ac = null;
ItemGroup ig = context;
while (ac == null) {
if (ig instanceOf AccessControlled) {
ac = (AccessControlled)ig;
} else {
ig = ((Item) ig).getParent();
}
}
if (ac.hasPermission(CredentialsProvider.VIEW) {
return provider.getCredentials(Credentials.class, context, ACL.SYSTEM);
}
return Collections.emptyList();
}

@schiasileon schiasileon force-pushed the feat/JENKINS-53105 branch 2 times, most recently from 6fe7df8 to 28fb140 Compare December 21, 2023 14:43
@schiasileon schiasileon force-pushed the feat/JENKINS-53105 branch 2 times, most recently from 3191b6e to 3a776a5 Compare January 8, 2024 09:42
@schiasileon
Copy link
Contributor Author

@jtnord any more feedback?

@jtnord
Copy link
Member

jtnord commented Jan 8, 2024

@jtnord any more feedback?

I think it looks ok, but I want to find some time to fully check for any security related implications, I may not be able to get around to that until next week.

@schiasileon
Copy link
Contributor Author

@jtnord any more feedback?

I think it looks ok, but I want to find some time to fully check for any security related implications, I may not be able to get around to that until next week.

bumping @jtnord

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

@Override
public boolean hasPermission(@NonNull Authentication authentication, @NonNull Permission permission) {
return CredentialsProvider.VIEW.equals(permission) &&
Jenkins.getInstance().getACL().hasPermission(authentication, permission);
}
looks incorrect now and should be checking the permission on the context similar to
AccessControlled ac = null;
ItemGroup<?> ig = context;
while (ac == null) {
if (ig instanceof AccessControlled) {
ac = (AccessControlled)ig;
} else if(ig instanceof Item){
ig = ((Item) ig).getParent();
} else {
break;
}
}
if (ac == null || ac.hasPermission(CredentialsProvider.VIEW)) {
return provider.getCredentials(Credentials.class, context, ACL.SYSTEM);
}
but I think both should fallback to checking the permission on Jenkins should the AccessControlled be null

@jtnord jtnord merged commit 2670ef7 into jenkinsci:master Feb 29, 2024
15 checks passed
@jtnord
Copy link
Member

jtnord commented Feb 29, 2024

many thanks @schiasileon

@jtnord jtnord added enhancement New feature or request major-enhancement A really big new feature and removed enhancement New feature or request labels Feb 29, 2024
@Fenil-Patelll

This comment was marked as off-topic.

@jtnord
Copy link
Member

jtnord commented Mar 11, 2024

@jtnord @schiasileon Sorry to disturb you guys. This is out of topic but I am not able to find the answer on google, though to talk here

please use https://www.jenkins.io/participate/connect/#join-our-chats-and-forums

@RaveNoX
Copy link

RaveNoX commented Mar 27, 2024

It looks like this PR is produces credential leakage of SYSTEM scoped credential to the top-level jobs.
Tested on Freestyle job with Credentials Binding Plugin.

Previous version 1.258.v95949f923a_a_e of the plugin is not affected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-enhancement A really big new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants