-
Notifications
You must be signed in to change notification settings - Fork 370
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
Add new options to GitHub App credentials to allow dynamic restrictions of the repositories and permissions available to installation access tokens in some contexts #822
base: master
Are you sure you want to change the base?
Conversation
…ns of the repositories and permissions available to installation access tokens in some contexts Co-authored-by: Jérôme Pochat <[email protected]>
.../org/jenkinsci/plugins/github_branch_source/app_credentials/AccessSpecifiedRepositories.java
Dismissed
Show dismissed
Hide dismissed
.../org/jenkinsci/plugins/github_branch_source/app_credentials/AccessSpecifiedRepositories.java
Dismissed
Show dismissed
Hide dismissed
...enkinsci/plugins/github_branch_source/app_credentials/InstallationAccessTokenPermission.java
Fixed
Show fixed
Hide fixed
...enkinsci/plugins/github_branch_source/app_credentials/InstallationAccessTokenPermission.java
Fixed
Show fixed
Hide fixed
* All permissions available to the app installation (default) | ||
* The access tokens generated in untrusted contexts will have the same permissions as the app installation in GitHub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether this is the best default, I am not sure. I think for the default we just want to choose whatever is least surprising. Perhaps read-only access would be more appropriate as a default.
|
||
GitHub App Credentials offer advanced configuration options that can provide additional security in some scenarios. | ||
In particular, when GitHub App Credentials are used by an Organization Folder or Multibranch Pipeline, these strategies may dynamically restrict the accessible repositories and permissions available to the credentials when they are accessed in untrusted contexts, such as when they are accessed by a `withCredentials` step in one of the individual Pipeline jobs. | ||
See TODO for additional information on why it may be beneficial to limit credentials in this way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be replaced with a reference to jenkins-infra/jenkins.io#7770.
*/ | ||
private transient Map<String, GitHubAppCredentials> byOwner; | ||
private transient Map<GitHubAppUsageContext, GitHubAppCredentials> cachedCredentials = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still dynamically create different credentials for each context, but now the contexts are more complicated.
// This method is not deprecated or restricted only to preserve compatibility with existing CasC YAML files. | ||
/** Do not call this method, use {@link #setRepositoryAccessStrategy} instead. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could mark this method @Deprecated
, in which case all CasC users that configure GitHub App credentials would have to edit their YAML scripts when applying this plugin update to avoid startup failures. I do not know what approach is generally preferred when making significant configuration changes, so I went for trying to preserve CasC compatibility for now.
We do probably want to keep the method around though so that we do not need to do coordinated releases of plugins that currently call this method, which are mentioned in the PR description.
GHAppCreateTokenBuilder builder = appInstallation.createToken(); | ||
if (!repositories.isEmpty()) { | ||
builder.repositories(repositories); | ||
} | ||
if (!permissions.isEmpty()) { | ||
builder.permissions(permissions); | ||
} else { | ||
builder.permissions(appInstallation.getPermissions()); | ||
} | ||
GHAppInstallationToken appInstallationToken = builder.create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new methods from https://github.com/hub4j/github-api are what actually change the accessible repositories and permissions for the generated tokens.
...enkinsci/plugins/github_branch_source/app_credentials/InstallationAccessTokenPermission.java
Outdated
Show resolved
Hide resolved
@@ -22,12 +22,21 @@ | |||
</f:entry> | |||
|
|||
<f:advanced> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should still hide these options behind the advanced dropdown or not. Any thoughts?
<ul> | ||
<j:forEach var="c" items="${it.migratedCredentialIds}"> | ||
<li>${c}</li> | ||
</j:forEach> | ||
</ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this needs to be in a <details>
tag or have fixed height so that it doesn't take up the whole page if there are a lot of credentials.
// TODO: CasC plugin incorrectly oversimplifies the YAML export for new-specific-empty by | ||
// fully removing the repositoryAccessStrategy configuration because its inner | ||
// configuration is all empty, but that means it no longer round-trips. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially, if you have a Describable
with multiple possible implementations, and a single implementation whose inner values are nullable or support empty, a configuration like this:
outerContext:
whatever: foo
describable:
myDescribable1:
innerConfiguration: []
Will be exported as this:
outerContext:
whatever: foo
Which is not good if describable
is heterogeneous and has multiple possible implementations! We'd like for it to be exported as the following, which is what happens if myDescribable1
has no nested configuration (I think because it ends up as a SCALAR
in ConfigurationAsCode.toYaml
instead of a MAPPING
):
outerContext:
whatever: foo
describable: myDescribable1
Note also that Jenkins users who have Job/Configure permission in a context where the credentials are available are considered trusted and can bypass these strategies by configuring jobs as desired. | ||
In trusted contexts, the generated access tokens will have the same access as configured for the app installation in GitHub. | ||
|
||
The following repository access strategies are available: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these strategies make sense? Do they work they way you would expect? Do you think the default is appropriate? Is there any other strategy that you think should be supported?
Description
See JENKINS-75058. See also jenkins-infra/jenkins.io#7770 for greater context around this change. For now this is a draft PR. I will go through the PR and add review comments to help clarify things.
This PR adds new configuration options to GitHub App credentials that make use of the
repositories
andpermissions
parameters when using the/app/installations/{installation_id}/access_tokens
GitHub API endpoint to create installation access tokens.There are two new high-level options:
repositories
parameter to control which repositories are available to the installation access tokens. There are three strategies, please see the changes to docs/github-app.adoc and the correspondinghelp-*.html
for details.permissions
parameter to control which permissions are available to the installation access tokens, but only when the tokens are generated and used in an untrusted context. There are three strategies, please see the changes to docs/github-app.adoc for details.If you have any recommendations for renaming of these options or any of their sub-options, please feel free to suggest them.
I will go through and add comments to the PR to various points of interest. Please feel free to ask about anything that is not clear.
Backwards compatibility:
owner
field set, we can 1) either preserve compatibility for users who have the app installed in multiple orgs and only use the credentials in contexts where owner inference is supported by usingAccessInferredOwner
as the migration, or 2) we can preserve compatibility for users who have the app installed in a single org and use it in contexts where inference is not supported by usingAccessSpecifiedRepositories
with anull
owner. None of the new strategies currnetly support these two use cases simultaneously.Notes for downstream plugins:
Connector.lookupScanCredentials
so that the retrieved credentials recognize that they are in a trusted context so that the generated token is not restricted. See for example Use Connector.lookupScanCredentials to contextualize GitHubAppCredentials github-checks-plugin#398.GitHubAppCredentials.getOwner
andsetOwner
methods should no longer be used. The following plugins along with a CloudBees-internal one will need (minor) updates to match this PR. I will file PRs for this soon:Submitter checklist
Reviewer checklist
Documentation changes
Users/aliases to notify